On Tue, Feb 13, 2024 at 3:29 PM Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote: > > 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. right, so I expected to see KERN_VM_SZ - GUARD_SZ, but instead we have KERN_VM_SZ - GUARD_SZ/2 (so we'll iterate GUARD_SZ/2 too much, into upper guard pages which are supposed to be not allocated), which is why I'm asking. It's minor, I was probing if I'm missing something subtle. > > > > + 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. ok, no big deal
