Re: [PATCH] RDMA/umem: Minor optimization

[Jason Gunthorpe <jgg@xxxxxxxx>]

On Thu, Sep 20, 2018 at 04:23:00PM -0400, Doug Ledford wrote:
> Noticed while reviewing d4b4dd1b9706 ("RDMA/umem: Do not use
> current->tgid to track the mm_struct") patch.  Why would we take a lock,
> adjust a protected variable, drop the lock, and *then* check the input
> into our protected variable adjustment?  Then we have to take the lock
> again on our error unwind.  Let's just check the input early and skip
> taking the locks needlessly if the input isn't valid.
>
> Signed-off-by: Doug Ledford <dledford@xxxxxxxxxx>
>  drivers/infiniband/core/umem.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/infiniband/core/umem.c b/drivers/infiniband/core/umem.c
> index c32a3e27a896..34d7256f5ba0 100644
> +++ b/drivers/infiniband/core/umem.c
> @@ -147,6 +147,10 @@ struct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr,
>  		umem->hugetlb = 0;
>
>  	npages = ib_umem_num_pages(umem);
> +	if (npages == 0 || npages > UINT_MAX) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
>  
>  	lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;

Reviewed-by: Jason Gunthorpe <jgg@xxxxxxxxxxxx>

Though this bit is doing the same nonsense:

	down_write(&current->mm->mmap_sem);
	current->mm->pinned_vm += npages;
	if ((current->mm->pinned_vm > lock_limit) && !capable(CAP_IPC_LOCK)) {
		up_write(&current->mm->mmap_sem);
		ret = -ENOMEM;
		goto vma;

And would be better written like

if (check_add_overflow(current->mm->pinned_vm, npages, &newpinned) ||
    (newpinned > lock_limit && !capable(CAP_IPC_LOCK))) {
		up_write(&current->mm->mmap_sem);
		ret = -ENOMEM;
		goto out;
}
current->mm->pinned_vm = newpinned;

Ie we have a bug here, if a process creates a large enough MR it can
overflow pinned_vm and violate lock_limit. :(

Jason