From: Dan Carpenter <dan.carpenter@oracle.com>
To: Souptick Joarder <jrdr.linux@gmail.com>
Cc: John Hubbard <jhubbard@nvidia.com>,
mporter@kernel.crashing.org, alex.bou9@gmail.com,
Andrew Morton <akpm@linux-foundation.org>,
gustavoars@kernel.org, madhuparnabhowmik10@gmail.com,
linux-kernel@vger.kernel.org, Ira Weiny <ira.weiny@intel.com>,
Matthew Wilcox <willy@infradead.org>
Subject: Re: [linux-next PATCH] rapidio: Fix error handling path
Date: Fri, 18 Sep 2020 09:15:35 +0300 [thread overview]
Message-ID: <20200918061535.GQ18329@kadam> (raw)
In-Reply-To: <CAFqt6zZUAtjU872kpem+nv7QM7h13Z9hH9gZFR=8=VKWxfxdmQ@mail.gmail.com>
On Fri, Sep 18, 2020 at 07:55:05AM +0530, Souptick Joarder wrote:
> Hi Dan,
>
> On Thu, Sep 17, 2020 at 6:10 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Wed, Sep 16, 2020 at 01:02:32PM +0300, Dan Carpenter wrote:
> > > On Wed, Sep 16, 2020 at 09:12:17AM +0530, Souptick Joarder wrote:
> > > > There is an error when pin_user_pages_fast() returns -ERRNO and
> > > > inside error handling path driver end up calling unpin_user_pages()
> > > > with -ERRNO which is not correct.
> > > >
> > > > This patch will fix the problem.
> > >
> > > There are a few ways we could prevent bug in the future.
> > >
> > > 1) This could have been caught with existing static analysis tools
> > > which warn about when a value is set but not used.
> > >
> > > 2) I've created a Smatch check which warngs about:
> > >
> > > drivers/rapidio/devices/rio_mport_cdev.c:955 rio_dma_transfer() warn: unpinning negative pages 'nr_pages'
> > >
> > > I'll test it out tonight and see how well it works. I don't
> > > immediately see any other bugs allthough Smatch doesn't like the code
> > > in siw_umem_release(). It uses "min_t(int" which suggests that
> > > negative pages are okay.
> > >
> > > int to_free = min_t(int, PAGES_PER_CHUNK, num_pages);
> > >
> >
> > I only found one bug but I'm going to add unpin_user_pages_dirty_lock()
> > to the mix a retest. There were a few other false positives. In
> > reviewing the code, I noticed that orangefs_bufmap_map() is also buggy.
> >
> > I sort of feel like returning partial successes is not working. We
> > could easily make a wrapper which either pins everything or it returns
> > an error code.
> >
> > drivers/misc/mic/scif/scif_rma.c:1399 __scif_pin_pages() warn: unpinning negative pages 'pinned_pages->nr_pages'
> >
> > drivers/misc/mic/scif/scif_rma.c
> > 1355 vmalloc_addr = true;
> > 1356
> > 1357 for (i = 0; i < nr_pages; i++) {
> > 1358 if (vmalloc_addr)
> > 1359 pinned_pages->pages[i] =
> > 1360 vmalloc_to_page(addr + (i * PAGE_SIZE));
> > 1361 else
> > 1362 pinned_pages->pages[i] =
> > 1363 virt_to_page(addr + (i * PAGE_SIZE));
> > 1364 }
> > 1365 pinned_pages->nr_pages = nr_pages;
> > 1366 pinned_pages->map_flags = SCIF_MAP_KERNEL;
> > 1367 } else {
> > 1368 /*
> > 1369 * SCIF supports registration caching. If a registration has
> > 1370 * been requested with read only permissions, then we try
> > 1371 * to pin the pages with RW permissions so that a subsequent
> > 1372 * transfer with RW permission can hit the cache instead of
> > 1373 * invalidating it. If the upgrade fails with RW then we
> > 1374 * revert back to R permission and retry
> > 1375 */
> > 1376 if (prot == SCIF_PROT_READ)
> > 1377 try_upgrade = true;
> > 1378 prot |= SCIF_PROT_WRITE;
> > 1379 retry:
> > 1380 mm = current->mm;
> > 1381 if (ulimit) {
> > 1382 err = __scif_check_inc_pinned_vm(mm, nr_pages);
> > 1383 if (err) {
> > 1384 pinned_pages->nr_pages = 0;
> > 1385 goto error_unmap;
> > 1386 }
> > 1387 }
> > 1388
> > 1389 pinned_pages->nr_pages = pin_user_pages_fast(
> > 1390 (u64)addr,
> > 1391 nr_pages,
> > 1392 (prot & SCIF_PROT_WRITE) ? FOLL_WRITE : 0,
> > 1393 pinned_pages->pages);
> > 1394 if (nr_pages != pinned_pages->nr_pages) {
> > 1395 if (try_upgrade) {
> > 1396 if (ulimit)
> > 1397 __scif_dec_pinned_vm_lock(mm, nr_pages);
> > 1398 /* Roll back any pinned pages */
> > 1399 unpin_user_pages(pinned_pages->pages,
> > 1400 pinned_pages->nr_pages);
> > ^^^^^^^^^^^^^^^^^^^^^^
> > Negative.
> >
> > 1401 prot &= ~SCIF_PROT_WRITE;
> > 1402 try_upgrade = false;
> > 1403 goto retry;
> > 1404 }
> > 1405 }
> > 1406 pinned_pages->map_flags = 0;
> > 1407 }
> > 1408
> > 1409 if (pinned_pages->nr_pages < nr_pages) {
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > These are both signed so it negative ->nr_pages are less than nr_pages.
> >
> > 1410 err = -EFAULT;
> > 1411 pinned_pages->nr_pages = nr_pages;
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > This sets it to "everything was pinned".
> >
> > 1412 goto dec_pinned;
> > 1413 }
> > 1414
> > 1415 *out_prot = prot;
> > 1416 atomic_set(&pinned_pages->ref_count, 1);
> > 1417 *pages = pinned_pages;
> > 1418 return err;
> > 1419 dec_pinned:
> > 1420 if (ulimit)
> > 1421 __scif_dec_pinned_vm_lock(mm, nr_pages);
> > 1422 /* Something went wrong! Rollback */
> > 1423 error_unmap:
> > 1424 pinned_pages->nr_pages = nr_pages;
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > This assumes everything was pinned successfully.
> >
> > 1425 scif_destroy_pinned_pages(pinned_pages);
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > We absolutely don't want to pass negative ->nr_pages to this function
> > either.
> >
> > 1426 *pages = NULL;
> > 1427 dev_dbg(scif_info.mdev.this_device,
> > 1428 "%s %d err %d len 0x%lx\n", __func__, __LINE__, err, len);
> > 1429 return err;
> > 1430 }
> >
> > regards,
> > dan carpenter
>
>
> I have drafted a patch for this bug. Shall I post it ?
Absolutely, please. :)
regards,
dan carpenter
next prev parent reply other threads:[~2020-09-18 6:18 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-16 3:42 [linux-next PATCH] rapidio: Fix error handling path Souptick Joarder
2020-09-16 6:37 ` John Hubbard
2020-09-16 10:02 ` Dan Carpenter
2020-09-16 10:07 ` Dan Carpenter
2020-09-16 15:16 ` Ira Weiny
2020-09-16 15:27 ` Dan Carpenter
2020-09-17 6:57 ` [PATCH] mm/gup: protect unpin_user_pages() against npages==-ERRNO John Hubbard
2020-09-17 7:40 ` Dan Carpenter
2020-09-20 3:03 ` Souptick Joarder
2020-09-20 4:13 ` John Hubbard
2020-09-21 9:34 ` Dan Carpenter
2020-09-17 12:39 ` [linux-next PATCH] rapidio: Fix error handling path Dan Carpenter
2020-09-17 17:34 ` Ira Weiny
2020-09-17 17:47 ` John Hubbard
2020-09-18 2:21 ` Souptick Joarder
2020-09-18 6:33 ` John Hubbard
2020-09-18 2:25 ` Souptick Joarder
2020-09-18 6:15 ` Dan Carpenter [this message]
2020-09-16 15:20 ` Ira Weiny
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200918061535.GQ18329@kadam \
--to=dan.carpenter@oracle.com \
--cc=akpm@linux-foundation.org \
--cc=alex.bou9@gmail.com \
--cc=gustavoars@kernel.org \
--cc=ira.weiny@intel.com \
--cc=jhubbard@nvidia.com \
--cc=jrdr.linux@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=madhuparnabhowmik10@gmail.com \
--cc=mporter@kernel.crashing.org \
--cc=willy@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.