From f091db924883ef4a053d3619e0af6ff05956ae8c Mon Sep 17 00:00:00 2001 From: Richard Yao Date: Mon, 23 Jan 2023 16:12:37 -0500 Subject: [PATCH] free_blocks(): Fix reports from 2016 PVS Studio FreeBSD report In 2016, the authors of PVS Studio ran it on the FreeBSD kernel, which identified a number of bugs / cleanup opportunities in the FreeBSD ZFS kernel code. A few of them persist to the present day: https://reviews.freebsd.org/D5245 Note that the scan was done against freebsd/freebsd-src@46763fd4ca8a37f836c9bf2333f9d687509278f3. In particular, we have the following in free_blocks(): \sys\cddl\contrib\opensolaris\uts\common\fs\zfs\dnode_sync.c (174): error V547: Expression '__left >= __right' is always true. Unsigned type value is always >= 0. \sys\cddl\contrib\opensolaris\uts\common\fs\zfs\dnode_sync.c (171): error V634: The priority of the '*' operation is higher than that of the '<<' operation. It's possible that parentheses should be used in the expression. \sys\cddl\contrib\opensolaris\uts\common\fs\zfs\dnode_sync.c (175): error V547: Expression '__left >= __right' is always true. Unsigned type value is always >= 0. A couple of assertions accidentally typecast the arguments they check to unsigned in such a way that the result is always true. Also, parentheses are missing around `1<db_blkid * 1<db_blkid << epbs)`. A few of the function local variables probably never should have been 32-bit in the first place, so we make them 64-bit. We also replace the existing assertions with additional assertions to ensure that 64-bit unsigned arithmetic is safe. Reviewed-by: Alexander Motin Reviewed-by: Brian Behlendorf Signed-off-by: Richard Yao Closes #14407 --- module/zfs/dnode_sync.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/module/zfs/dnode_sync.c b/module/zfs/dnode_sync.c index 2ed8058e8..8e39af83b 100644 --- a/module/zfs/dnode_sync.c +++ b/module/zfs/dnode_sync.c @@ -175,19 +175,21 @@ free_blocks(dnode_t *dn, blkptr_t *bp, int num, dmu_tx_t *tx) static void free_verify(dmu_buf_impl_t *db, uint64_t start, uint64_t end, dmu_tx_t *tx) { - int off, num; - int i, err, epbs; + uint64_t off, num, i, j; + unsigned int epbs; + int err; uint64_t txg = tx->tx_txg; dnode_t *dn; DB_DNODE_ENTER(db); dn = DB_DNODE(db); epbs = dn->dn_phys->dn_indblkshift - SPA_BLKPTRSHIFT; - off = start - (db->db_blkid * 1<db_blkid << epbs); num = end - start + 1; - ASSERT3U(off, >=, 0); - ASSERT3U(num, >=, 0); + ASSERT3U(dn->dn_phys->dn_indblkshift, >=, SPA_BLKPTRSHIFT); + ASSERT3U(end + 1, >=, start); + ASSERT3U(start, >=, (db->db_blkid << epbs)); ASSERT3U(db->db_level, >, 0); ASSERT3U(db->db.db_size, ==, 1 << dn->dn_phys->dn_indblkshift); ASSERT3U(off+num, <=, db->db.db_size >> SPA_BLKPTRSHIFT); @@ -197,7 +199,6 @@ free_verify(dmu_buf_impl_t *db, uint64_t start, uint64_t end, dmu_tx_t *tx) uint64_t *buf; dmu_buf_impl_t *child; dbuf_dirty_record_t *dr; - int j; ASSERT(db->db_level == 1); @@ -217,8 +218,11 @@ free_verify(dmu_buf_impl_t *db, uint64_t start, uint64_t end, dmu_tx_t *tx) for (j = 0; j < child->db.db_size >> 3; j++) { if (buf[j] != 0) { panic("freed data not zero: " - "child=%p i=%d off=%d num=%d\n", - (void *)child, i, off, num); + "child=%p i=%llu off=%llu " + "num=%llu\n", + (void *)child, (u_longlong_t)i, + (u_longlong_t)off, + (u_longlong_t)num); } } } @@ -234,8 +238,11 @@ free_verify(dmu_buf_impl_t *db, uint64_t start, uint64_t end, dmu_tx_t *tx) for (j = 0; j < child->db.db_size >> 3; j++) { if (buf[j] != 0) { panic("freed data not zero: " - "child=%p i=%d off=%d num=%d\n", - (void *)child, i, off, num); + "child=%p i=%llu off=%llu " + "num=%llu\n", + (void *)child, (u_longlong_t)i, + (u_longlong_t)off, + (u_longlong_t)num); } } }