Re: [PATCH v2 bpf-next 3/3] mm: Introduce VM_SPARSE kind and vm_area_[un]map_pages().

[Christoph Hellwig <hch@xxxxxxxxxxxxx>]

> privately-managed pages into a sparse vm area with the following steps:
> 
>   area = get_vm_area(area_size, VM_SPARSE);  // at bpf prog verification time
>   vm_area_map_pages(area, kaddr, 1, page);   // on demand
>                     // it will return an error if kaddr is out of range
>   vm_area_unmap_pages(area, kaddr, 1);
>   free_vm_area(area);                        // after bpf prog is unloaded

I'm still wondering if this should just use an opaque cookie instead
of exposing the vm_area.  But otherwise this mostly looks fine to me.

> +     if (addr < (unsigned long)area->addr || (void *)end > area->addr + 
> area->size)
> +             return -ERANGE;

This check is duplicated so many times that it really begs for a helper.

> +int vm_area_unmap_pages(struct vm_struct *area, unsigned long addr, unsigned 
> int count)
> +{
> +     unsigned long size = ((unsigned long)count) * PAGE_SIZE;
> +     unsigned long end = addr + size;
> +
> +     if (WARN_ON_ONCE(!(area->flags & VM_SPARSE)))
> +             return -EINVAL;
> +     if (addr < (unsigned long)area->addr || (void *)end > area->addr + 
> area->size)
> +             return -ERANGE;
> +
> +     vunmap_range(addr, end);
> +     return 0;

Does it make much sense to have an error return here vs just debug
checks?  It's not like the caller can do much if it violates these
basic invariants.