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

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

On 26/04/03 11:22AM, Alexei Starovoitov wrote:
> On Fri, Apr 3, 2026 at 11:01 AM Anton Protopopov
> <a.s.protopopov@gmail.com> wrote:
> >
> > 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.)
> 
> I wouldn't worry about backports. Size of the patch doesn't matter much.
> 
> Is spill issue just a matter of adding one line to is_spillable_regtype ?
> If so, let's do that and have a test in C (in addition to asm) that
> actually using the offset.

Ok, looks I've found a simple selftest which compiles to
a non-zero offload from realistic C code (and triggers a NULL
deref without this patch). So I will send the next version with
that selftest added, and with a better commit description.