| Message ID | 20180123205459.432-1-dave@stgolabs.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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);
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 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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?
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 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(-)