Re: [PATCH v2 bpf-next 05/20] bpf: Introduce bpf_arena.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Feb 13, 2024 at 3:14 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
>
> here we can then use MAX_ARENA_SZ

I thought about it, but decided against it, since it will be
too tempting to change it without understanding the consequences.
Like...

> > +       if ((attr->map_extra >> 32) != ((attr->map_extra + vm_range - 1) >> 32))
> > +               /* user vma must not cross 32-bit boundary */
> > +               return ERR_PTR(-ERANGE);

here >> 32 is relevant to size and pretty much every such shift.
Hence 1ull << 32 matches all those shifts.
And they have to be analyzed together.

> > +       apply_to_existing_page_range(&init_mm, bpf_arena_get_kern_vm_start(arena),
> > +                                    KERN_VM_SZ - GUARD_SZ / 2, for_each_pte, NULL);
>
> I'm still reading the rest (so it might become obvious), but this
> KERN_VM_SZ - GUARD_SZ / 2 is a bit surprising. I understand that
> kern_vm_start is shifted by GUARD_SZ/2, but is the intent here is to
> actually go beyond maximum 4GB by GUARD_SZ/2, or the intent was to
> unmap 4GB (MAX_ARENA_SZ)?

here it's just the range for apply_to_existing_page_range() to walk.
There are no pages mapped into the lower GUARD_SZ / 2 and upper GUARD_SZ / 2.
So no reason to ask apply_to_existing_page_range() to walk
the whole KERN_VM_SZ.

> > +       ret = current->mm->get_unmapped_area(filp, addr, len * 2, 0, flags);
> > +       if (IS_ERR_VALUE(ret))
> > +                return 0;
>
> Can you leave a comment why we are swallowing errors, if this is intentional?

argh. good catch. it's a bug.

> > +       if ((ret >> 32) == ((ret + len - 1) >> 32))
> > +               return ret;
> > +       if (WARN_ON_ONCE(arena->user_vm_start))
> > +               /* checks at map creation time should prevent this */
> > +               return -EFAULT;
> > +       return round_up(ret, 1ull << 32);
>
> this is still probably MAX_ARENA_SZ, no?

and here it would be wrong to do that.
This line has to match the logic with 'if' few lines above.
Hiding behind macro is a dangerous obfuscation.





[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux