Re: [PATCH v2 bpf-next 1/2] bpf: Do not ignore offsets for loads from insn_arrays

[Anton Protopopov <a.s.protopopov@gmail.com>]

O 26/04/03 08:10AM, Alexei Starovoitov wrote:
> On Fri, Apr 3, 2026 at 12:47 AM Anton Protopopov
> <a.s.protopopov@gmail.com> wrote:
> >
> > On 26/04/02 02:32PM, Alexei Starovoitov wrote:
> > > On Thu, Apr 2, 2026 at 1:44 PM Anton Protopopov
> > > <a.s.protopopov@gmail.com> wrote:
> > > >
> > > > On 26/04/02 12:00PM, Alexei Starovoitov wrote:
> > > > > On Thu, Apr 2, 2026 at 11:38 AM Anton Protopopov
> > > > > <a.s.protopopov@gmail.com> wrote:
> > > > > >
> > > > > > When a pointer to PTR_TO_INSN is dereferenced it is possible to
> > > > > > specify an offset inside the load instruction. This is a bug,
> > > > > > because while the verifier ignores the field, JITs are not.
> > > > > > So, patch the verifier to not ignore this field.
> > > > >
> > > > > This is too terse to understand.
> > > > > In 2nd patch you're saying:
> > > > >
> > > > >    r1 = &map + offset1
> > > > >    r1 += offset2
> > > > >    r1 = *(r1 + offset3)
> > > > >    gotox r1
> > > > >
> > > > > Here offset3 is, normally, equal to zero; but this is not guaranteed.
> > > > >
> > > > > What is this 'offset3'? Where did it come from?
> > > >
> > > > The offset3 is the .off field of the BPF_LDX_MEM instruction.
> > > > The BPF assembler will correctly work with non-zero offsets:
> > > >
> > > >     r1 = &map;
> > > >     r1 = *(r1 + offset)
> > > >
> > > > > What do you mean JIT handles it?
> > > >
> > > > JIT will issue a memory load with this offset,
> > > > but verifier will ignore it (before this patch).
> > > >
> > > > > can llvm ever generate such code?
> > > > > if not, reject it early ?
> > > >
> > > > LLVM can generate code with non-zero offset in BPF_LDX_MEM,
> > > > say, if one jumps to a structure field, like in `goto *p->j`
> > >
> > > sorry, I still don't get it. What kind of syntax is that?
> > > Could you share the godbolt link where llvm actually generates
> > > such code? If the verifier will reject it anyway it's fine.
> > > I just want to make sure we're fixing real problem.
> >
> > I am not aware of any real-life code, only can construct
> > some artificial examples:
> >
> >         SEC("syscall")
> >         int test(unsigned int n)
> >         {
> >                 struct {
> >                         int i;
> >                         void *j[3];
> >                 } x = {
> >                         .i = n,
> >                         .j = { &&l1, &&l2, &&l3 },
> >                 };
> >
> >                 if (n < 3)
> >                         goto *x.j[n];
> >
> >                 return 0;
> >         l1:
> >                 return 1;
> >         l2:
> >                 return 3;
> >         l3:
> >                 return 5;
> >         }
> >
> > It will compile into
> >
> >         <test>:
> >         ;               .j = { &&l1, &&l2, &&l3 },
> >              160:       18 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r2 = 0x0 ll
> >                         0000000000000500:  R_BPF_64_64  BPF.JT.4.0
> >              162:       79 23 10 00 00 00 00 00 r3 = *(u64 *)(r2 + 0x10)
> >                                                                    ^ offet != 0
> >              163:       7b 3a f8 ff 00 00 00 00 *(u64 *)(r10 - 0x8) = r3
> >              164:       79 23 08 00 00 00 00 00 r3 = *(u64 *)(r2 + 0x8)
> >              165:       7b 3a f0 ff 00 00 00 00 *(u64 *)(r10 - 0x10) = r3
> 
> Great, but where is an actual gotox that is using that r3?
> Looks like it's spilled into stack at 163 ?
> Does it pass the verifier?
> Would be great to add it as C selftest.

No, this one doesn't pass the verifier (it is spilled to stack, and when
it is loaded back the error is "invalid unbounded variable-offset
read from stack R2", because it is loaded back from stack as
"scalar", not "insn"). All "normal" use cases from [C-level] selftests
(a switch or a 'goto *j[i]') compile into code which accumulate
offset into previous instructions.

Do you want me to work on this example above to pass the verifier?
Or, for now, the best thing is to just reject non-zero offsets?
(The latter has the benefit that the patch will be auto-backportable
to stable trees.)