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

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

[Davidlohr Bueso]

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@zeniv.linux.org.uk>
Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---

- Compile tested only.

- 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.


 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);
-- 
2.13.6

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

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 3281 bytes --]

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@zeniv.linux.org.uk>
> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
> ---
> 
> - 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@redhat.com>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Jason Gunthorpe]

On Thu, Jan 25, 2018 at 11:34:23AM -0500, Doug Ledford wrote:
> > - 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?

I looked at it enough to be confident that mthca_map_user_db is never
called with mmap_sem held.

Also pretty confident that mthca_unmap_user_db is never called with
mmap_sem.

So how about just grabbing mmap_sem around the call to get_user_pages
and no other changes?

Jason

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

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 1429 bytes --]

On Thu, 2018-01-25 at 10:50 -0700, Jason Gunthorpe wrote:
> On Thu, Jan 25, 2018 at 11:34:23AM -0500, Doug Ledford wrote:
> > > - 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?
> 
> I looked at it enough to be confident that mthca_map_user_db is never
> called with mmap_sem held.
> 
> Also pretty confident that mthca_unmap_user_db is never called with
> mmap_sem.
> 
> So how about just grabbing mmap_sem around the call to get_user_pages
> and no other changes?

Since the original post was referred to an ABBA deadlock, wouldn't we
have to drop db_tab->mutex, then grab both in the proper order?

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Jason Gunthorpe]

On Thu, Jan 25, 2018 at 01:06:24PM -0500, Doug Ledford wrote:
> On Thu, 2018-01-25 at 10:50 -0700, Jason Gunthorpe wrote:
> > On Thu, Jan 25, 2018 at 11:34:23AM -0500, Doug Ledford wrote:
> > > > - 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?
> > 
> > I looked at it enough to be confident that mthca_map_user_db is never
> > called with mmap_sem held.
> > 
> > Also pretty confident that mthca_unmap_user_db is never called with
> > mmap_sem.
> > 
> > So how about just grabbing mmap_sem around the call to get_user_pages
> > and no other changes?
> 
> Since the original post was referred to an ABBA deadlock, wouldn't we
> have to drop db_tab->mutex, then grab both in the proper order?

I had understood that was only a concern because Davidlohr was having
trouble proving the callchain didn't include mmap_sem already..

I can see the call chain all ends on verbs ops, and I know verbs ops
with ucontext's are never called under mmap_sem by the core code..

Jason

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

[Davidlohr Bueso]

On Thu, 25 Jan 2018, Jason Gunthorpe wrote:

>>
>> Since the original post was referred to an ABBA deadlock, wouldn't we
>> have to drop db_tab->mutex, then grab both in the proper order?
>
>I had understood that was only a concern because Davidlohr was having
>trouble proving the callchain didn't include mmap_sem already..
>
>I can see the call chain all ends on verbs ops, and I know verbs ops
>with ucontext's are never called under mmap_sem by the core code..

Right. Ok so this simplifies things and we can just use gup_fast().

Thanks,
Davidlohr

---8<---------------------------------------------------
[PATCH v2] IB/mthca: Fix gup usage in mthca_map_user_db()

get_user_pages() must be called with mmap_sem held, currently
it is not. In fact it is called under the user db_table->mutex.
To fix this we can convert gup to use the fast alternative,
and safely avoid taking mmap_sem, if possible. Furthermore
this is safe wrt to the mutex as other callers that take the
lock (unmap and alloc_db) are not called under mmap_sem
(hence possible deadlock).

Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
 drivers/infiniband/hw/mthca/mthca_memfree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/mthca/mthca_memfree.c b/drivers/infiniband/hw/mthca/mthca_memfree.c
index c6fe89d79248..9a412738d5c3 100644
--- a/drivers/infiniband/hw/mthca/mthca_memfree.c
+++ b/drivers/infiniband/hw/mthca/mthca_memfree.c
@@ -472,7 +472,7 @@ 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);
+	ret = get_user_pages_fast(uaddr & PAGE_MASK, 1, FOLL_WRITE, pages);
 	if (ret < 0)
 		goto out;
 
-- 
2.13.6

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

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 839 bytes --]

On Thu, 2018-01-25 at 11:27 -0800, Davidlohr Bueso wrote:
> On Thu, 25 Jan 2018, Jason Gunthorpe wrote:
> 
> > > 
> > > Since the original post was referred to an ABBA deadlock, wouldn't we
> > > have to drop db_tab->mutex, then grab both in the proper order?
> > 
> > I had understood that was only a concern because Davidlohr was having
> > trouble proving the callchain didn't include mmap_sem already..
> > 
> > I can see the call chain all ends on verbs ops, and I know verbs ops
> > with ucontext's are never called under mmap_sem by the core code..
> 
> Right. Ok so this simplifies things and we can just use gup_fast().
> 
> Thanks,
> Davidlohr

Thanks, applied.

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]