Re: [PATCH bpf-next v1 5/7] bpf: Introduce support for bpf_local_irq_{save,restore}

[Eduard Zingerman <eddyz87@gmail.com>]

On Fri, 2024-11-22 at 00:12 +0100, Kumar Kartikeya Dwivedi wrote:
> On Fri, 22 Nov 2024 at 00:08, Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
> > On Thu, 2024-11-21 at 23:06 +0100, Kumar Kartikeya Dwivedi wrote:
> > 
> > [...]
> > 
> > > > > +/* Keep unsinged long in prototype so that kfunc is usable when emitted to
> > > > > + * vmlinux.h in BPF programs directly, but since unsigned long may potentially
> > > > > + * be 4 byte, always cast to u64 when reading/writing from this pointer as it
> > > > > + * always points to an 8-byte memory region in BPF stack.
> > > > > + */
> > > > > +__bpf_kfunc void bpf_local_irq_save(unsigned long *flags__irq_flag)
> > > > 
> > > > Nit: 'unsigned long long' is guaranteed to be at-least 64 bit.
> > > >      What would go wrong if 'u64' is used here?
> > > 
> > > It goes like this:
> > > If I make this unsigned long long * or u64 *, the kfunc emitted to
> > > vmlinux.h expects a pointer of that type.
> > > Typically, kernel code is always passing unsigned long flags to these
> > > functions, and that's what people are used to.
> > > Given for --target=bpf unsigned long * is always a 8-byte value, I
> > > just did this, so that in kernels that are 32-bit,
> > > we don't end up relying on unsigned long still being 8 when
> > > fetching/storing flags on BPF stack.
> > 
> > So, the goal is to enable the following pattern:
> > 
> >   unsigned long flags;
> >   bpf_local_irq_save(&flags);
> > 
> > Right?
> > 
> > For a 32-bit system 'flags' would be 4 bytes long.
> > Consider the following example:
> > 
> >   unsigned long flags; // assume 'flags' and 'foo'
> >   int foo;             // are allocated sequentially.
> > 
> >   bpf_local_irq_save(&flags);
> > 
> > I think that in such case '*ptr = flags;' would overwrite foo.
> 
> In the kernel or userspace, yes, but I'm assuming unsigned long will
> always be 64-bit for target=BPF.
> Would that be incorrect? This pattern will only happen within BPF programs.

Discussed off-list.
Kumar is right, and there is no problem, as on BPF side 'unsigned
long' is always 8 bytes.

[...]