lkml.org 
[lkml]   [2017]   [Nov]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
Subjectmthca misuse of get_user_pages() (was Re: [git pull] vfs.git get_user_pages_fast() conversion)
On Fri, Nov 17, 2017 at 09:32:15PM +0000, Al Viro wrote:

[snip]
> drivers/infiniband/hw/mthca/mthca_memfree.c:475: ret = get_user_pages(uaddr & PAGE_MASK, 1, FOLL_WRITE, pages, NULL);
[snip]

> Itanic
> one is almost certainly buggered - we are not holding ->mmap_sem there.

So's the mthca one - that caller (mthca_map_user_db()) does not take ->mmap_sem
and at least some of its calls are immediately preceded by copy_from_user().
If we don't have ->mmap_sem there, this get_user_pages() is oopsably racy; if
we do, those copy_from_user() calls is deadlock-prone.

To make things even nastier, mthca_map_user_db() does grab a mutex around the
chunk that contains get_user_pages() call:
mutex_lock(&db_tab->mutex);
several lines prior. And _that_ is taken on a whole lot of codepaths; if any of
those happen to be under ->mmap_sem, we can't grab ->mmap_sem just around that
get_user_pages(). I've poked around a bit, but I'm really unfamiliar with that
code... The questions so far:
* can mthca_map_user_db() ever be called under ->mmap_sem? Looks like
it shouldn't, but...
* do we really need that get_user_pages() under ->mutex? Would something
like
....
if (db_tab->page[i].refcount) {
++db_tab->page[i].refcount;
goto out;
}
mutex_unlock(&db_tab->mutex);
ret = get_user_pages_fast(uaddr & PAGE_MASK, 1, true, pages);
if (ret < 0)
return ret;
mutex_lock(&db_tab->mutex);
if (unlikely(db_tab->page[i].refcount)) {
/* lost the race */
put_page(pages[0]);
++db_tab->page[i].refcount;
goto out;
}
....
in place of the current
if (db_tab->page[i].refcount) {
++db_tab->page[i].refcount;
goto out;
}

ret = get_user_pages(uaddr & PAGE_MASK, 1, FOLL_WRITE, pages, NULL);
if (ret < 0)
goto out;
be a usable solution?

Comments?

\
 
 \ /
  Last update: 2017-11-19 22:24    [W:0.047 / U:6.804 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site