Re: [PATCH] IB/mthca: Fix how mthca_map_user_db() calls gup

[Doug Ledford]

On Tue, 2018-01-23 at 12:54 -0800, Davidlohr Bueso wrote:
> mthca_map_user_db() has two problems regarding the call to
> get_user_pages():
> 
> (i) It is not done under mmap_sem.
> 
> (ii) It is done under the db_table mutex, which protects all
> database related operations. Should any of these be called
> under mmap_sem, we get an ABBA deadlock. In addition, gup can
> be performance intensive, which could contend other mapping/
> unmapping ops.
> 
> To fix this, we can drop the mutex while doing a gup_fast(),
> once done, recheck to see the page was mapped while we didn't
> hold the mutex, and exit out with the corresponding housekeeping.
> 
> Suggested-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Davidlohr Bueso <dbueso@xxxxxxx>
> ---
> 
> - Compile tested only.

Jason and I talked about this offline a bit.  We're concerned about
taking a patch like this into an ancient, unmaintained, but working
driver.  Especially when it's only compile tested.  I have mthca
hardware on hand and I can test it, but in this case, testing your patch
would require triggering a race condition that we really don't have a
way to test.

> - Should I be wrong about no callers already holding mmap_sem,
>   I still think calling gup without the mutex makes sense for
>   improved paralellism. Now, if callers can hold the mmap_sem,
>   it's wrong to do copy_from_user right before calling mthca_map_user_db.

So, if I understand you correctly, we (well, you and Al would be more
correct, we haven't looked into the situation yet, so Mellanox people
that worked on this in the day might now, or someone taking the time to
research it could find out) don't have a clear understanding of all the
conditions this function is called under, and so we actually don't know
what the best way forward is to fix it?

> 
>  drivers/infiniband/hw/mthca/mthca_memfree.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/hw/mthca/mthca_memfree.c b/drivers/infiniband/hw/mthca/mthca_memfree.c
> index c6fe89d79248..046871878a02 100644
> --- a/drivers/infiniband/hw/mthca/mthca_memfree.c
> +++ b/drivers/infiniband/hw/mthca/mthca_memfree.c
> @@ -472,9 +472,27 @@ int mthca_map_user_db(struct mthca_dev *dev, struct mthca_uar *uar,
>  		goto out;
>  	}
>  
> -	ret = get_user_pages(uaddr & PAGE_MASK, 1, FOLL_WRITE, pages, NULL);
> +	mutex_unlock(&db_tab->mutex);
> +
> +	ret = get_user_pages_fast(uaddr & PAGE_MASK, 1, FOLL_WRITE, pages);
>  	if (ret < 0)
> +		return ret;
> +
> +	mutex_lock(&db_tab->mutex);
> +
> +	if (db_tab->page[i].refcount >= MTHCA_DB_REC_PER_PAGE ||
> +	    (db_tab->page[i].uvirt && db_tab->page[i].uvirt != uaddr)) {
> +		put_page(pages[0]);
> +		ret = -EINVAL;
>  		goto out;
> +	}
> +
> +	/* page was already mapped by another task while we were doing gup */
> +	if (db_tab->page[i].refcount) {
> +		put_page(pages[0]);
> +		++db_tab->page[i].refcount;
> +		goto out;
> +	}
>  
>  	sg_set_page(&db_tab->page[i].mem, pages[0], MTHCA_ICM_PAGE_SIZE,
>  			uaddr & ~PAGE_MASK);

-- 
Doug Ledford <dledford@xxxxxxxxxx>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

Attachment: signature.asc
Description: This is a digitally signed message part