Messages in this thread | | | Date | Fri, 18 Sep 2020 09:15:35 +0300 | From | Dan Carpenter <> | Subject | Re: [linux-next PATCH] rapidio: Fix error handling path |
| |
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
| |