On Fri, 22 Nov 2024 at 00:08, Eduard Zingerman <eddyz87@xxxxxxxxx> 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.
>
> [...]
>
> > > > +{
> > > > + u64 *ptr = (u64 *)flags__irq_flag;
> > > > + unsigned long flags;
> > > > +
> > > > + local_irq_save(flags);
> > > > + *ptr = flags;
> > > > +}
>
