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?
