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.
