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

[Eduard Zingerman <eddyz87@gmail.com>]

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.

[...]

> > > +{
> > > +     u64 *ptr = (u64 *)flags__irq_flag;
> > > +     unsigned long flags;
> > > +
> > > +     local_irq_save(flags);
> > > +     *ptr = flags;
> > > +}