netdev - Re: [PATCH bpf-next 5/6] bpf, arm32: Always zero extend for LDX with B/H/W

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAADnVQLzbyG3xWVDFyTsDPRSC=fnAskaeyc1erQVLYo_b6Lg_w@mail.gmail.com>
Date: Tue, 12 Sep 2023 17:10:42 -0700
From: Alexei Starovoitov <alexei.starovoitov@...il.com>
To: Puranjay Mohan <puranjay12@...il.com>
Cc: "Russell King (Oracle)" <linux@...linux.org.uk>, Alexei Starovoitov <ast@...nel.org>, 
	Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>, 
	Martin KaFai Lau <martin.lau@...ux.dev>, Song Liu <song@...nel.org>, 
	Yonghong Song <yonghong.song@...ux.dev>, John Fastabend <john.fastabend@...il.com>, 
	KP Singh <kpsingh@...nel.org>, Stanislav Fomichev <sdf@...gle.com>, Hao Luo <haoluo@...gle.com>, 
	Jiri Olsa <jolsa@...nel.org>, Shubham Bansal <illusionist.neo@...il.com>, 
	"James E.J. Bottomley" <James.Bottomley@...senpartnership.com>, Helge Deller <deller@....de>, 
	"Naveen N. Rao" <naveen.n.rao@...ux.ibm.com>, Michael Ellerman <mpe@...erman.id.au>, 
	Nicholas Piggin <npiggin@...il.com>, Christophe Leroy <christophe.leroy@...roup.eu>, 
	Luke Nelson <luke.r.nels@...il.com>, Xi Wang <xi.wang@...il.com>, 
	Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>, 
	Albert Ou <aou@...s.berkeley.edu>, Wang YanQing <udknight@...il.com>, bpf <bpf@...r.kernel.org>, 
	linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>, LKML <linux-kernel@...r.kernel.org>, 
	linux-parisc@...r.kernel.org, ppc-dev <linuxppc-dev@...ts.ozlabs.org>, 
	linux-riscv <linux-riscv@...ts.infradead.org>, 
	Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf-next 5/6] bpf, arm32: Always zero extend for LDX with B/H/W

On Tue, Sep 12, 2023 at 4:17 PM Puranjay Mohan <puranjay12@...il.com> wrote:
>
> On Wed, Sep 13, 2023 at 1:04 AM Russell King (Oracle)
> <linux@...linux.org.uk> wrote:
> >
> > On Tue, Sep 12, 2023 at 10:46:53PM +0000, Puranjay Mohan wrote:
> > > The JITs should not depend on the verifier for zero extending the upper
> > > 32 bits of the destination register when loading a byte, half-word, or
> > > word.
> > >
> > > A following patch will make the verifier stop patching zext instructions
> > > after LDX.
> >
> > This was introduced by:
> >
> > 163541e6ba34 ("arm: bpf: eliminate zero extension code-gen")
> >
> > along with an additional function. So three points:
> >
> > 1) the commit should probably explain why it has now become undesirable
> > to access this verifier state, whereas it appears it was explicitly
> > added to permit this optimisation.
>
> I added some details in the cover letter.
>
> For the complete discussion see: [1]
>
> > 2) you state that jits should not depend on this state, but the above
> > commit adds more references than you're removing, so aren't there still
> > references to the verifier remaining after this patch? I count a total
> > of 10, and the patch below removes three.
>
> The JITs should not depend on this state for LDX (loading
> a B/H/W.
> This patch removes the usage only for LDX.
>
> > 3) what about the bpf_jit_needs_zext() function that was added to
> > support the export of this zext state?
>
> That is still applicable, The verifier will still emit zext
> instructions for other
> instructions like BPF_ALU / BPF_ALU64
>
> >
> > Essentially, the logic stated in the commit message doesn't seem to be
> > reflected by the proposed code change.
>
> I will try to provide more information.
> Currently I have asked Alexei if we really need this in [2].
> I still think this optimization is useful and we should keep it.

Right. subreg tracking is indeed functional for narrow loads.
Let's drop this patch set.