* [kbuild] [djwong-xfs:djwong-wtf 349/351] fs/xfs/xfs_bmap_util.c:1372 xfs_map_free_extent() warn: missing error code 'error'
@ 2022-03-21 7:33 Dan Carpenter
2022-03-21 21:59 ` Darrick J. Wong
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2022-03-21 7:33 UTC (permalink / raw)
To: kbuild, Darrick J. Wong; +Cc: lkp, kbuild-all, Darrick J. Wong, linux-kernel
tree: https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git djwong-wtf
head: baa8d54bb35559f0bc36ec9e9933bec9ab23e25f
commit: b82670045aab66d3d66eca40f277354c3da07f1c [349/351] xfs: fallocate free space into a file
config: openrisc-randconfig-m031-20220317 (https://download.01.org/0day-ci/archive/20220319/202203190831.AYu7l0vX-lkp@intel.com/config )
compiler: or1k-linux-gcc (GCC) 11.2.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
New smatch warnings:
fs/xfs/xfs_bmap_util.c:1372 xfs_map_free_extent() warn: missing error code 'error'
Old smatch warnings:
fs/xfs/xfs_buf.h:430 xfs_buftarg_zeroout() warn: bitwise AND condition is false here
vim +/error +1372 fs/xfs/xfs_bmap_util.c
b82670045aab66 Darrick J. Wong 2022-01-06 1341 /* Find a free extent in this AG and map it into the file. */
b82670045aab66 Darrick J. Wong 2022-01-06 1342 STATIC int
b82670045aab66 Darrick J. Wong 2022-01-06 1343 xfs_map_free_extent(
b82670045aab66 Darrick J. Wong 2022-01-06 1344 struct xfs_inode *ip,
b82670045aab66 Darrick J. Wong 2022-01-06 1345 struct xfs_perag *pag,
b82670045aab66 Darrick J. Wong 2022-01-06 1346 xfs_agblock_t *cursor,
b82670045aab66 Darrick J. Wong 2022-01-06 1347 xfs_agblock_t end_agbno,
b82670045aab66 Darrick J. Wong 2022-01-06 1348 xfs_agblock_t *last_enospc_agbno)
b82670045aab66 Darrick J. Wong 2022-01-06 1349 {
b82670045aab66 Darrick J. Wong 2022-01-06 1350 struct xfs_bmbt_irec irec;
b82670045aab66 Darrick J. Wong 2022-01-06 1351 struct xfs_mount *mp = ip->i_mount;
b82670045aab66 Darrick J. Wong 2022-01-06 1352 struct xfs_trans *tp;
b82670045aab66 Darrick J. Wong 2022-01-06 1353 xfs_off_t endpos;
b82670045aab66 Darrick J. Wong 2022-01-06 1354 xfs_fsblock_t fsbno;
b82670045aab66 Darrick J. Wong 2022-01-06 1355 xfs_extlen_t len;
b82670045aab66 Darrick J. Wong 2022-01-06 1356 int error;
b82670045aab66 Darrick J. Wong 2022-01-06 1357
b82670045aab66 Darrick J. Wong 2022-01-06 1358 if (fatal_signal_pending(current))
b82670045aab66 Darrick J. Wong 2022-01-06 1359 return -EINTR;
b82670045aab66 Darrick J. Wong 2022-01-06 1360
b82670045aab66 Darrick J. Wong 2022-01-06 1361 error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write, 0, 0, false,
b82670045aab66 Darrick J. Wong 2022-01-06 1362 &tp);
b82670045aab66 Darrick J. Wong 2022-01-06 1363 if (error)
b82670045aab66 Darrick J. Wong 2022-01-06 1364 return error;
b82670045aab66 Darrick J. Wong 2022-01-06 1365
b82670045aab66 Darrick J. Wong 2022-01-06 1366 error = xfs_alloc_find_freesp(tp, pag, cursor, end_agbno, &len);
b82670045aab66 Darrick J. Wong 2022-01-06 1367 if (error)
b82670045aab66 Darrick J. Wong 2022-01-06 1368 goto out_cancel;
b82670045aab66 Darrick J. Wong 2022-01-06 1369
b82670045aab66 Darrick J. Wong 2022-01-06 1370 /* Bail out if the cursor is beyond what we asked for. */
b82670045aab66 Darrick J. Wong 2022-01-06 1371 if (*cursor >= end_agbno)
b82670045aab66 Darrick J. Wong 2022-01-06 @1372 goto out_cancel;
This looks like it should have an error = -EINVAL;
b82670045aab66 Darrick J. Wong 2022-01-06 1373
b82670045aab66 Darrick J. Wong 2022-01-06 1374 error = xfs_map_free_reserve_more(tp, ip, &len);
b82670045aab66 Darrick J. Wong 2022-01-06 1375 if (error)
b82670045aab66 Darrick J. Wong 2022-01-06 1376 goto out_cancel;
b82670045aab66 Darrick J. Wong 2022-01-06 1377
b82670045aab66 Darrick J. Wong 2022-01-06 1378 fsbno = XFS_AGB_TO_FSB(mp, pag->pag_agno, *cursor);
b82670045aab66 Darrick J. Wong 2022-01-06 1379 do {
b82670045aab66 Darrick J. Wong 2022-01-06 1380 error = xfs_bmapi_freesp(tp, ip, fsbno, len, &irec);
b82670045aab66 Darrick J. Wong 2022-01-06 1381 if (error == -EAGAIN) {
b82670045aab66 Darrick J. Wong 2022-01-06 1382 /* Failed to map space but were told to try again. */
b82670045aab66 Darrick J. Wong 2022-01-06 1383 error = xfs_trans_commit(tp);
b82670045aab66 Darrick J. Wong 2022-01-06 1384 goto out;
b82670045aab66 Darrick J. Wong 2022-01-06 1385 }
b82670045aab66 Darrick J. Wong 2022-01-06 1386 if (error != -ENOSPC)
b82670045aab66 Darrick J. Wong 2022-01-06 1387 break;
b82670045aab66 Darrick J. Wong 2022-01-06 1388 /*
b82670045aab66 Darrick J. Wong 2022-01-06 1389 * If we can't get the space, try asking for successively less
b82670045aab66 Darrick J. Wong 2022-01-06 1390 * space in case we're bumping up against per-AG metadata
b82670045aab66 Darrick J. Wong 2022-01-06 1391 * reservation limits...
b82670045aab66 Darrick J. Wong 2022-01-06 1392 */
b82670045aab66 Darrick J. Wong 2022-01-06 1393 len >>= 1;
b82670045aab66 Darrick J. Wong 2022-01-06 1394 } while (len > 0);
b82670045aab66 Darrick J. Wong 2022-01-06 1395 if (error == -ENOSPC && *last_enospc_agbno != *cursor) {
b82670045aab66 Darrick J. Wong 2022-01-06 1396 /*
b82670045aab66 Darrick J. Wong 2022-01-06 1397 * ...but even that might not work if an AGFL fixup allocated
b82670045aab66 Darrick J. Wong 2022-01-06 1398 * the block at *cursor. The first time this happens, remember
b82670045aab66 Darrick J. Wong 2022-01-06 1399 * that we ran out of space here, and try again.
b82670045aab66 Darrick J. Wong 2022-01-06 1400 */
b82670045aab66 Darrick J. Wong 2022-01-06 1401 *last_enospc_agbno = *cursor;
b82670045aab66 Darrick J. Wong 2022-01-06 1402 error = 0;
b82670045aab66 Darrick J. Wong 2022-01-06 1403 goto out_cancel;
b82670045aab66 Darrick J. Wong 2022-01-06 1404 }
b82670045aab66 Darrick J. Wong 2022-01-06 1405 if (error)
b82670045aab66 Darrick J. Wong 2022-01-06 1406 goto out_cancel;
b82670045aab66 Darrick J. Wong 2022-01-06 1407
b82670045aab66 Darrick J. Wong 2022-01-06 1408 /* Update isize if needed. */
b82670045aab66 Darrick J. Wong 2022-01-06 1409 endpos = XFS_FSB_TO_B(mp, irec.br_startoff + irec.br_blockcount);
b82670045aab66 Darrick J. Wong 2022-01-06 1410 if (endpos > i_size_read(VFS_I(ip))) {
b82670045aab66 Darrick J. Wong 2022-01-06 1411 i_size_write(VFS_I(ip), endpos);
b82670045aab66 Darrick J. Wong 2022-01-06 1412 ip->i_disk_size = endpos;
b82670045aab66 Darrick J. Wong 2022-01-06 1413 xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
b82670045aab66 Darrick J. Wong 2022-01-06 1414 }
b82670045aab66 Darrick J. Wong 2022-01-06 1415
b82670045aab66 Darrick J. Wong 2022-01-06 1416 error = xfs_trans_commit(tp);
b82670045aab66 Darrick J. Wong 2022-01-06 1417 xfs_iunlock(ip, XFS_ILOCK_EXCL);
b82670045aab66 Darrick J. Wong 2022-01-06 1418 if (error)
b82670045aab66 Darrick J. Wong 2022-01-06 1419 return error;
b82670045aab66 Darrick J. Wong 2022-01-06 1420
b82670045aab66 Darrick J. Wong 2022-01-06 1421 *cursor += irec.br_blockcount;
b82670045aab66 Darrick J. Wong 2022-01-06 1422 return 0;
b82670045aab66 Darrick J. Wong 2022-01-06 1423 out_cancel:
b82670045aab66 Darrick J. Wong 2022-01-06 1424 xfs_trans_cancel(tp);
b82670045aab66 Darrick J. Wong 2022-01-06 1425 out:
b82670045aab66 Darrick J. Wong 2022-01-06 1426 xfs_iunlock(ip, XFS_ILOCK_EXCL);
b82670045aab66 Darrick J. Wong 2022-01-06 1427 return error;
b82670045aab66 Darrick J. Wong 2022-01-06 1428 }
---
0-DAY CI Kernel Test Service
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
_______________________________________________
kbuild mailing list -- kbuild@lists.01.org
To unsubscribe send an email to kbuild-leave@lists.01.org
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [kbuild] [djwong-xfs:djwong-wtf 349/351] fs/xfs/xfs_bmap_util.c:1372 xfs_map_free_extent() warn: missing error code 'error'
2022-03-21 7:33 [kbuild] [djwong-xfs:djwong-wtf 349/351] fs/xfs/xfs_bmap_util.c:1372 xfs_map_free_extent() warn: missing error code 'error' Dan Carpenter
@ 2022-03-21 21:59 ` Darrick J. Wong
2022-03-22 5:47 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Darrick J. Wong @ 2022-03-21 21:59 UTC (permalink / raw)
To: Dan Carpenter; +Cc: kbuild, lkp, kbuild-all, Darrick J. Wong, linux-kernel
On Mon, Mar 21, 2022 at 10:33:02AM +0300, Dan Carpenter wrote:
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git djwong-wtf
> head: baa8d54bb35559f0bc36ec9e9933bec9ab23e25f
> commit: b82670045aab66d3d66eca40f277354c3da07f1c [349/351] xfs: fallocate free space into a file
> config: openrisc-randconfig-m031-20220317 (https://download.01.org/0day-ci/archive/20220319/202203190831.AYu7l0vX-lkp@intel.com/config )
> compiler: or1k-linux-gcc (GCC) 11.2.0
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> New smatch warnings:
> fs/xfs/xfs_bmap_util.c:1372 xfs_map_free_extent() warn: missing error code 'error'
>
> Old smatch warnings:
> fs/xfs/xfs_buf.h:430 xfs_buftarg_zeroout() warn: bitwise AND condition is false here
>
> vim +/error +1372 fs/xfs/xfs_bmap_util.c
>
> b82670045aab66 Darrick J. Wong 2022-01-06 1341 /* Find a free extent in this AG and map it into the file. */
> b82670045aab66 Darrick J. Wong 2022-01-06 1342 STATIC int
> b82670045aab66 Darrick J. Wong 2022-01-06 1343 xfs_map_free_extent(
> b82670045aab66 Darrick J. Wong 2022-01-06 1344 struct xfs_inode *ip,
> b82670045aab66 Darrick J. Wong 2022-01-06 1345 struct xfs_perag *pag,
> b82670045aab66 Darrick J. Wong 2022-01-06 1346 xfs_agblock_t *cursor,
> b82670045aab66 Darrick J. Wong 2022-01-06 1347 xfs_agblock_t end_agbno,
> b82670045aab66 Darrick J. Wong 2022-01-06 1348 xfs_agblock_t *last_enospc_agbno)
> b82670045aab66 Darrick J. Wong 2022-01-06 1349 {
> b82670045aab66 Darrick J. Wong 2022-01-06 1350 struct xfs_bmbt_irec irec;
> b82670045aab66 Darrick J. Wong 2022-01-06 1351 struct xfs_mount *mp = ip->i_mount;
> b82670045aab66 Darrick J. Wong 2022-01-06 1352 struct xfs_trans *tp;
> b82670045aab66 Darrick J. Wong 2022-01-06 1353 xfs_off_t endpos;
> b82670045aab66 Darrick J. Wong 2022-01-06 1354 xfs_fsblock_t fsbno;
> b82670045aab66 Darrick J. Wong 2022-01-06 1355 xfs_extlen_t len;
> b82670045aab66 Darrick J. Wong 2022-01-06 1356 int error;
> b82670045aab66 Darrick J. Wong 2022-01-06 1357
> b82670045aab66 Darrick J. Wong 2022-01-06 1358 if (fatal_signal_pending(current))
> b82670045aab66 Darrick J. Wong 2022-01-06 1359 return -EINTR;
> b82670045aab66 Darrick J. Wong 2022-01-06 1360
> b82670045aab66 Darrick J. Wong 2022-01-06 1361 error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write, 0, 0, false,
> b82670045aab66 Darrick J. Wong 2022-01-06 1362 &tp);
> b82670045aab66 Darrick J. Wong 2022-01-06 1363 if (error)
> b82670045aab66 Darrick J. Wong 2022-01-06 1364 return error;
> b82670045aab66 Darrick J. Wong 2022-01-06 1365
> b82670045aab66 Darrick J. Wong 2022-01-06 1366 error = xfs_alloc_find_freesp(tp, pag, cursor, end_agbno, &len);
> b82670045aab66 Darrick J. Wong 2022-01-06 1367 if (error)
> b82670045aab66 Darrick J. Wong 2022-01-06 1368 goto out_cancel;
> b82670045aab66 Darrick J. Wong 2022-01-06 1369
> b82670045aab66 Darrick J. Wong 2022-01-06 1370 /* Bail out if the cursor is beyond what we asked for. */
> b82670045aab66 Darrick J. Wong 2022-01-06 1371 if (*cursor >= end_agbno)
> b82670045aab66 Darrick J. Wong 2022-01-06 @1372 goto out_cancel;
>
> This looks like it should have an error = -EINVAL;
Nope. If xfs_alloc_find_freesp moves @cursor goes beyond end_agbno, we
want to exit early so that the xfs_map_free_extent caller will return to
userspace.
--D
>
> b82670045aab66 Darrick J. Wong 2022-01-06 1373
> b82670045aab66 Darrick J. Wong 2022-01-06 1374 error = xfs_map_free_reserve_more(tp, ip, &len);
> b82670045aab66 Darrick J. Wong 2022-01-06 1375 if (error)
> b82670045aab66 Darrick J. Wong 2022-01-06 1376 goto out_cancel;
> b82670045aab66 Darrick J. Wong 2022-01-06 1377
> b82670045aab66 Darrick J. Wong 2022-01-06 1378 fsbno = XFS_AGB_TO_FSB(mp, pag->pag_agno, *cursor);
> b82670045aab66 Darrick J. Wong 2022-01-06 1379 do {
> b82670045aab66 Darrick J. Wong 2022-01-06 1380 error = xfs_bmapi_freesp(tp, ip, fsbno, len, &irec);
> b82670045aab66 Darrick J. Wong 2022-01-06 1381 if (error == -EAGAIN) {
> b82670045aab66 Darrick J. Wong 2022-01-06 1382 /* Failed to map space but were told to try again. */
> b82670045aab66 Darrick J. Wong 2022-01-06 1383 error = xfs_trans_commit(tp);
> b82670045aab66 Darrick J. Wong 2022-01-06 1384 goto out;
> b82670045aab66 Darrick J. Wong 2022-01-06 1385 }
> b82670045aab66 Darrick J. Wong 2022-01-06 1386 if (error != -ENOSPC)
> b82670045aab66 Darrick J. Wong 2022-01-06 1387 break;
> b82670045aab66 Darrick J. Wong 2022-01-06 1388 /*
> b82670045aab66 Darrick J. Wong 2022-01-06 1389 * If we can't get the space, try asking for successively less
> b82670045aab66 Darrick J. Wong 2022-01-06 1390 * space in case we're bumping up against per-AG metadata
> b82670045aab66 Darrick J. Wong 2022-01-06 1391 * reservation limits...
> b82670045aab66 Darrick J. Wong 2022-01-06 1392 */
> b82670045aab66 Darrick J. Wong 2022-01-06 1393 len >>= 1;
> b82670045aab66 Darrick J. Wong 2022-01-06 1394 } while (len > 0);
> b82670045aab66 Darrick J. Wong 2022-01-06 1395 if (error == -ENOSPC && *last_enospc_agbno != *cursor) {
> b82670045aab66 Darrick J. Wong 2022-01-06 1396 /*
> b82670045aab66 Darrick J. Wong 2022-01-06 1397 * ...but even that might not work if an AGFL fixup allocated
> b82670045aab66 Darrick J. Wong 2022-01-06 1398 * the block at *cursor. The first time this happens, remember
> b82670045aab66 Darrick J. Wong 2022-01-06 1399 * that we ran out of space here, and try again.
> b82670045aab66 Darrick J. Wong 2022-01-06 1400 */
> b82670045aab66 Darrick J. Wong 2022-01-06 1401 *last_enospc_agbno = *cursor;
> b82670045aab66 Darrick J. Wong 2022-01-06 1402 error = 0;
> b82670045aab66 Darrick J. Wong 2022-01-06 1403 goto out_cancel;
> b82670045aab66 Darrick J. Wong 2022-01-06 1404 }
> b82670045aab66 Darrick J. Wong 2022-01-06 1405 if (error)
> b82670045aab66 Darrick J. Wong 2022-01-06 1406 goto out_cancel;
> b82670045aab66 Darrick J. Wong 2022-01-06 1407
> b82670045aab66 Darrick J. Wong 2022-01-06 1408 /* Update isize if needed. */
> b82670045aab66 Darrick J. Wong 2022-01-06 1409 endpos = XFS_FSB_TO_B(mp, irec.br_startoff + irec.br_blockcount);
> b82670045aab66 Darrick J. Wong 2022-01-06 1410 if (endpos > i_size_read(VFS_I(ip))) {
> b82670045aab66 Darrick J. Wong 2022-01-06 1411 i_size_write(VFS_I(ip), endpos);
> b82670045aab66 Darrick J. Wong 2022-01-06 1412 ip->i_disk_size = endpos;
> b82670045aab66 Darrick J. Wong 2022-01-06 1413 xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> b82670045aab66 Darrick J. Wong 2022-01-06 1414 }
> b82670045aab66 Darrick J. Wong 2022-01-06 1415
> b82670045aab66 Darrick J. Wong 2022-01-06 1416 error = xfs_trans_commit(tp);
> b82670045aab66 Darrick J. Wong 2022-01-06 1417 xfs_iunlock(ip, XFS_ILOCK_EXCL);
> b82670045aab66 Darrick J. Wong 2022-01-06 1418 if (error)
> b82670045aab66 Darrick J. Wong 2022-01-06 1419 return error;
> b82670045aab66 Darrick J. Wong 2022-01-06 1420
> b82670045aab66 Darrick J. Wong 2022-01-06 1421 *cursor += irec.br_blockcount;
> b82670045aab66 Darrick J. Wong 2022-01-06 1422 return 0;
> b82670045aab66 Darrick J. Wong 2022-01-06 1423 out_cancel:
> b82670045aab66 Darrick J. Wong 2022-01-06 1424 xfs_trans_cancel(tp);
> b82670045aab66 Darrick J. Wong 2022-01-06 1425 out:
> b82670045aab66 Darrick J. Wong 2022-01-06 1426 xfs_iunlock(ip, XFS_ILOCK_EXCL);
> b82670045aab66 Darrick J. Wong 2022-01-06 1427 return error;
> b82670045aab66 Darrick J. Wong 2022-01-06 1428 }
>
> ---
> 0-DAY CI Kernel Test Service
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
> _______________________________________________
> kbuild mailing list -- kbuild@lists.01.org
> To unsubscribe send an email to kbuild-leave@lists.01.org
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [kbuild] [djwong-xfs:djwong-wtf 349/351] fs/xfs/xfs_bmap_util.c:1372 xfs_map_free_extent() warn: missing error code 'error'
2022-03-21 21:59 ` Darrick J. Wong
@ 2022-03-22 5:47 ` Dan Carpenter
2022-03-22 16:38 ` Darrick J. Wong
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2022-03-22 5:47 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: kbuild, lkp, kbuild-all, Darrick J. Wong, linux-kernel
On Mon, Mar 21, 2022 at 02:59:08PM -0700, Darrick J. Wong wrote:
> On Mon, Mar 21, 2022 at 10:33:02AM +0300, Dan Carpenter wrote:
> > b82670045aab66 Darrick J. Wong 2022-01-06 1365
> > b82670045aab66 Darrick J. Wong 2022-01-06 1366 error = xfs_alloc_find_freesp(tp, pag, cursor, end_agbno, &len);
> > b82670045aab66 Darrick J. Wong 2022-01-06 1367 if (error)
> > b82670045aab66 Darrick J. Wong 2022-01-06 1368 goto out_cancel;
> > b82670045aab66 Darrick J. Wong 2022-01-06 1369
> > b82670045aab66 Darrick J. Wong 2022-01-06 1370 /* Bail out if the cursor is beyond what we asked for. */
> > b82670045aab66 Darrick J. Wong 2022-01-06 1371 if (*cursor >= end_agbno)
> > b82670045aab66 Darrick J. Wong 2022-01-06 @1372 goto out_cancel;
> >
> > This looks like it should have an error = -EINVAL;
>
> Nope. If xfs_alloc_find_freesp moves @cursor goes beyond end_agbno, we
> want to exit early so that the xfs_map_free_extent caller will return to
> userspace.
>
> --D
I'm generally pretty happy with this static checker rule. Returning
success on a failure path almost always results if something bad like a
NULL deref or a use after free. But false positives are a real risk
because it's tempting to add an error code to this and introduce a bug.
Smatch will not print the warning if error is set within 4 lines of the
goto.
error = 0;
if (*cursor >= end_agbno)
goto out_cancel;
Another option is that people have started adding comments to these
blocks in response to the checker warning.
Or if you had a different idea about how to silence the checker warning
I can also probably implement that.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [kbuild] [djwong-xfs:djwong-wtf 349/351] fs/xfs/xfs_bmap_util.c:1372 xfs_map_free_extent() warn: missing error code 'error'
2022-03-22 5:47 ` Dan Carpenter
@ 2022-03-22 16:38 ` Darrick J. Wong
2022-03-24 10:45 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Darrick J. Wong @ 2022-03-22 16:38 UTC (permalink / raw)
To: Dan Carpenter; +Cc: kbuild, lkp, kbuild-all, Darrick J. Wong, linux-kernel, xfs
On Tue, Mar 22, 2022 at 08:47:26AM +0300, Dan Carpenter wrote:
> On Mon, Mar 21, 2022 at 02:59:08PM -0700, Darrick J. Wong wrote:
> > On Mon, Mar 21, 2022 at 10:33:02AM +0300, Dan Carpenter wrote:
> > > b82670045aab66 Darrick J. Wong 2022-01-06 1365
> > > b82670045aab66 Darrick J. Wong 2022-01-06 1366 error = xfs_alloc_find_freesp(tp, pag, cursor, end_agbno, &len);
> > > b82670045aab66 Darrick J. Wong 2022-01-06 1367 if (error)
> > > b82670045aab66 Darrick J. Wong 2022-01-06 1368 goto out_cancel;
> > > b82670045aab66 Darrick J. Wong 2022-01-06 1369
> > > b82670045aab66 Darrick J. Wong 2022-01-06 1370 /* Bail out if the cursor is beyond what we asked for. */
> > > b82670045aab66 Darrick J. Wong 2022-01-06 1371 if (*cursor >= end_agbno)
> > > b82670045aab66 Darrick J. Wong 2022-01-06 @1372 goto out_cancel;
> > >
> > > This looks like it should have an error = -EINVAL;
> >
> > Nope. If xfs_alloc_find_freesp moves @cursor goes beyond end_agbno, we
> > want to exit early so that the xfs_map_free_extent caller will return to
> > userspace.
> >
> > --D
>
> I'm generally pretty happy with this static checker rule. Returning
> success on a failure path almost always results if something bad like a
> NULL deref or a use after free. But false positives are a real risk
> because it's tempting to add an error code to this and introduce a bug.
>
> Smatch will not print the warning if error is set within 4 lines of the
> goto.
> error = 0;
> if (*cursor >= end_agbno)
> goto out_cancel;
The trouble is, if I do that:
error = xfs_alloc_find_freesp(...);
if (error)
goto out_cancel;
error = 0;
if (*cursor >= end_agbno)
goto out_cancel;
then I'll get patch reviewers and/or tools complaining about setting
error to zero unnecessarily. Either way we end up with a lot of code
golf for something the compiler will probably remove for us.
Question: Can sparse detect that the if() test involves a comparison
between a non-pointer function argument and a dereferenced pointer
argument? Would that be sufficient to detect functions that advance a
cursor passed in by the caller and return early when the cursor moves
outside of a range also specified by the caller?
--D
> Another option is that people have started adding comments to these
> blocks in response to the checker warning.
>
> Or if you had a different idea about how to silence the checker warning
> I can also probably implement that.
>
> regards,
> dan carpenter
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [kbuild] [djwong-xfs:djwong-wtf 349/351] fs/xfs/xfs_bmap_util.c:1372 xfs_map_free_extent() warn: missing error code 'error'
2022-03-22 16:38 ` Darrick J. Wong
@ 2022-03-24 10:45 ` Dan Carpenter
0 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2022-03-24 10:45 UTC (permalink / raw)
To: Darrick J. Wong
Cc: kbuild, lkp, kbuild-all, Darrick J. Wong, linux-kernel, xfs
On Tue, Mar 22, 2022 at 09:38:27AM -0700, Darrick J. Wong wrote:
> On Tue, Mar 22, 2022 at 08:47:26AM +0300, Dan Carpenter wrote:
> > On Mon, Mar 21, 2022 at 02:59:08PM -0700, Darrick J. Wong wrote:
> > > On Mon, Mar 21, 2022 at 10:33:02AM +0300, Dan Carpenter wrote:
> > > > b82670045aab66 Darrick J. Wong 2022-01-06 1365
> > > > b82670045aab66 Darrick J. Wong 2022-01-06 1366 error = xfs_alloc_find_freesp(tp, pag, cursor, end_agbno, &len);
> > > > b82670045aab66 Darrick J. Wong 2022-01-06 1367 if (error)
> > > > b82670045aab66 Darrick J. Wong 2022-01-06 1368 goto out_cancel;
> > > > b82670045aab66 Darrick J. Wong 2022-01-06 1369
> > > > b82670045aab66 Darrick J. Wong 2022-01-06 1370 /* Bail out if the cursor is beyond what we asked for. */
> > > > b82670045aab66 Darrick J. Wong 2022-01-06 1371 if (*cursor >= end_agbno)
> > > > b82670045aab66 Darrick J. Wong 2022-01-06 @1372 goto out_cancel;
> > > >
> > > > This looks like it should have an error = -EINVAL;
> > >
> > > Nope. If xfs_alloc_find_freesp moves @cursor goes beyond end_agbno, we
> > > want to exit early so that the xfs_map_free_extent caller will return to
> > > userspace.
> > >
> > > --D
> >
> > I'm generally pretty happy with this static checker rule. Returning
> > success on a failure path almost always results if something bad like a
> > NULL deref or a use after free. But false positives are a real risk
> > because it's tempting to add an error code to this and introduce a bug.
> >
> > Smatch will not print the warning if error is set within 4 lines of the
> > goto.
> > error = 0;
> > if (*cursor >= end_agbno)
> > goto out_cancel;
>
> The trouble is, if I do that:
>
> error = xfs_alloc_find_freesp(...);
> if (error)
> goto out_cancel;
>
> error = 0;
> if (*cursor >= end_agbno)
> goto out_cancel;
>
> then I'll get patch reviewers and/or tools complaining about setting
> error to zero unnecessarily.
Currently nothing would complain. What causes complaints if the
assignments are not used. Places where we assign a value and then
immediately re-assign over it.
It would only take a few minutes to write a checker rule which would
complain about assigning "ret = 0;" if we already know that foo was
zero, but hopefully no one will write it.
The closest is that Christophe JAILLET has a script to remove
duplicative memset()s to zero.
> Either way we end up with a lot of code
> golf for something the compiler will probably remove for us.
>
> Question: Can sparse detect that the if() test involves a comparison
> between a non-pointer function argument and a dereferenced pointer
> argument? Would that be sufficient to detect functions that advance a
> cursor passed in by the caller and return early when the cursor moves
> outside of a range also specified by the caller?
This is a Smatch test (not Sparse). Smatch doesn't have code to
detect/describe that right now... I'm not sure if the heuristic is very
useful. I will look at future false positives and see if it applies.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-03-24 10:45 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-03-21 7:33 [kbuild] [djwong-xfs:djwong-wtf 349/351] fs/xfs/xfs_bmap_util.c:1372 xfs_map_free_extent() warn: missing error code 'error' Dan Carpenter
2022-03-21 21:59 ` Darrick J. Wong
2022-03-22 5:47 ` Dan Carpenter
2022-03-22 16:38 ` Darrick J. Wong
2022-03-24 10:45 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox