* [PATCH] IB/mthca: Fix how mthca_map_user_db() calls gup @ 2018-01-23 20:54 Davidlohr Bueso 2018-01-25 16:34 ` Doug Ledford 0 siblings, 1 reply; 7+ messages in thread From: Davidlohr Bueso @ 2018-01-23 20:54 UTC (permalink / raw) To: dledford, roland, linux-rdma; +Cc: linux-kernel, dave, 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] IB/mthca: Fix how mthca_map_user_db() calls gup 2018-01-23 20:54 [PATCH] IB/mthca: Fix how mthca_map_user_db() calls gup Davidlohr Bueso @ 2018-01-25 16:34 ` Doug Ledford 2018-01-25 17:50 ` Jason Gunthorpe 0 siblings, 1 reply; 7+ messages in thread From: Doug Ledford @ 2018-01-25 16:34 UTC (permalink / raw) To: Davidlohr Bueso, roland, linux-rdma; +Cc: linux-kernel, Davidlohr Bueso [-- 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 --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] IB/mthca: Fix how mthca_map_user_db() calls gup 2018-01-25 16:34 ` Doug Ledford @ 2018-01-25 17:50 ` Jason Gunthorpe 2018-01-25 18:06 ` Doug Ledford 0 siblings, 1 reply; 7+ messages in thread From: Jason Gunthorpe @ 2018-01-25 17:50 UTC (permalink / raw) To: Doug Ledford Cc: Davidlohr Bueso, roland, linux-rdma, linux-kernel, Davidlohr Bueso 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] IB/mthca: Fix how mthca_map_user_db() calls gup 2018-01-25 17:50 ` Jason Gunthorpe @ 2018-01-25 18:06 ` Doug Ledford 2018-01-25 18:53 ` Jason Gunthorpe 0 siblings, 1 reply; 7+ messages in thread From: Doug Ledford @ 2018-01-25 18:06 UTC (permalink / raw) To: Jason Gunthorpe Cc: Davidlohr Bueso, roland, linux-rdma, linux-kernel, Davidlohr Bueso [-- 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 --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] IB/mthca: Fix how mthca_map_user_db() calls gup 2018-01-25 18:06 ` Doug Ledford @ 2018-01-25 18:53 ` Jason Gunthorpe 2018-01-25 19:27 ` Davidlohr Bueso 0 siblings, 1 reply; 7+ messages in thread From: Jason Gunthorpe @ 2018-01-25 18:53 UTC (permalink / raw) To: Doug Ledford Cc: Davidlohr Bueso, roland, linux-rdma, linux-kernel, Davidlohr Bueso 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] IB/mthca: Fix how mthca_map_user_db() calls gup 2018-01-25 18:53 ` Jason Gunthorpe @ 2018-01-25 19:27 ` Davidlohr Bueso 2018-01-26 15:44 ` Doug Ledford 0 siblings, 1 reply; 7+ messages in thread From: Davidlohr Bueso @ 2018-01-25 19:27 UTC (permalink / raw) To: Jason Gunthorpe Cc: Doug Ledford, roland, linux-rdma, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] IB/mthca: Fix how mthca_map_user_db() calls gup 2018-01-25 19:27 ` Davidlohr Bueso @ 2018-01-26 15:44 ` Doug Ledford 0 siblings, 0 replies; 7+ messages in thread From: Doug Ledford @ 2018-01-26 15:44 UTC (permalink / raw) To: Davidlohr Bueso, Jason Gunthorpe Cc: roland, linux-rdma, linux-kernel, Davidlohr Bueso [-- 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 --] ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-01-26 15:44 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-01-23 20:54 [PATCH] IB/mthca: Fix how mthca_map_user_db() calls gup Davidlohr Bueso 2018-01-25 16:34 ` Doug Ledford 2018-01-25 17:50 ` Jason Gunthorpe 2018-01-25 18:06 ` Doug Ledford 2018-01-25 18:53 ` Jason Gunthorpe 2018-01-25 19:27 ` Davidlohr Bueso 2018-01-26 15:44 ` Doug Ledford
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).