From 2e7f664f04bae4375ab6bebf232983b2326c9370 Mon Sep 17 00:00:00 2001 From: Richard Yao Date: Tue, 17 Jan 2023 12:57:12 -0500 Subject: [PATCH] Cleanup of dead code suggested by Clang Static Analyzer (#14380) I recently gained the ability to run Clang's static analyzer on the linux kernel modules via a few hacks. This extended coverage to code that was previously missed since Clang's static analyzer only looked at code that we built in userspace. Running it against the Linux kernel modules built from my local branch produced a total of 72 reports against my local branch. Of those, 50 were reports of logic errors and 22 were reports of dead code. Since we already had cleaned up all of the previous dead code reports, I felt it would be a good next step to clean up these dead code reports. Clang did a further breakdown of the dead code reports into: Dead assignment 15 Dead increment 2 Dead nested assignment 5 The benefit of cleaning these up, especially in the case of dead nested assignment, is that they can expose places where our error handling is incorrect. A number of them were fairly straight forward. However several were not: In vdev_disk_physio_completion(), not only were we not using the return value from the static function vdev_disk_dio_put(), but nothing used it, so I changed it to return void and removed the existing (void) cast in the other area where we call it in addition to no longer storing it to a stack value. In FSE_createDTable(), the function is dead code. Its helper function FSE_freeDTable() is also dead code, as are the CPP definitions in `module/zstd/include/zstd_compat_wrapper.h`. We just delete it all. In zfs_zevent_wait(), we have an optimization opportunity. cv_wait_sig() returns 0 if there are waiting signals and 1 if there are none. The Linux SPL version literally returns `signal_pending(current) ? 0 : 1)` and FreeBSD implements the same semantics, we can just do `!cv_wait_sig()` in place of `signal_pending(current)` to avoid unnecessarily calling it again. zfs_setattr() on FreeBSD version did not have error handling issue because the code was removed entirely from FreeBSD version. The error is from updating the attribute directory's files. After some thought, I decided to propapage errors on it to userspace. In zfs_secpolicy_tmp_snapshot(), we ignore a lack of permission from the first check in favor of checking three other permissions. I assume this is intentional. In zfs_create_fs(), the return value of zap_update() was not checked despite setting an important version number. I see no backward compatibility reason to permit failures, so we add an assertion to catch failures. Interestingly, Linux is still using ASSERT(error == 0) from OpenSolaris while FreeBSD has switched to the improved ASSERT0(error) from illumos, although illumos has yet to adopt it here. ASSERT(error == 0) was used on Linux while ASSERT0(error) was used on FreeBSD since the entire file needs conversion and that should be the subject of another patch. dnode_move()'s issue was caused by us not having implemented POINTER_IS_VALID() on Linux. We have a stub in `include/os/linux/spl/sys/kmem_cache.h` for it, when it really should be in `include/os/linux/spl/sys/kmem.h` to be consistent with Illumos/OpenSolaris. FreeBSD put both `POINTER_IS_VALID()` and `POINTER_INVALIDATE()` in `include/os/freebsd/spl/sys/kmem.h`, so we copy what it did. Whenever a report was in platform-specific code, I checked the FreeBSD version to see if it also applied to FreeBSD, but it was only relevant a few times. Lastly, the patch that enabled Clang's static analyzer to be run on the Linux kernel modules needs more work before it can be put into a PR. I plan to do that in the future as part of the on-going static analysis work that I am doing. Reviewed-by: Brian Behlendorf Signed-off-by: Richard Yao Closes #14380 --- include/os/linux/spl/sys/kmem.h | 3 +++ include/os/linux/spl/sys/kmem_cache.h | 3 --- module/os/freebsd/zfs/zfs_acl.c | 1 - module/os/freebsd/zfs/zfs_znode.c | 1 + module/os/linux/spl/spl-kmem-cache.c | 8 ++------ module/os/linux/spl/spl-thread.c | 5 ++--- module/os/linux/zfs/vdev_disk.c | 9 +++------ module/os/linux/zfs/zfs_acl.c | 3 --- module/os/linux/zfs/zfs_vnops_os.c | 3 +-- module/os/linux/zfs/zfs_znode.c | 3 +-- module/zcommon/zfs_fletcher.c | 4 ++-- module/zfs/fm.c | 3 +-- module/zfs/zfs_ioctl.c | 12 +++++------- module/zfs/zvol.c | 2 +- module/zstd/include/zstd_compat_wrapper.h | 2 -- module/zstd/lib/common/fse_decompress.c | 11 ----------- 16 files changed, 22 insertions(+), 51 deletions(-) diff --git a/include/os/linux/spl/sys/kmem.h b/include/os/linux/spl/sys/kmem.h index 111924303e..594425f7b2 100644 --- a/include/os/linux/spl/sys/kmem.h +++ b/include/os/linux/spl/sys/kmem.h @@ -40,6 +40,9 @@ extern void kmem_strfree(char *str); #define kmem_scnprintf scnprintf +#define POINTER_IS_VALID(p) (!((uintptr_t)(p) & 0x3)) +#define POINTER_INVALIDATE(pp) (*(pp) = (void *)((uintptr_t)(*(pp)) | 0x1)) + /* * Memory allocation interfaces */ diff --git a/include/os/linux/spl/sys/kmem_cache.h b/include/os/linux/spl/sys/kmem_cache.h index bd0ad5052d..cc9cafa84f 100644 --- a/include/os/linux/spl/sys/kmem_cache.h +++ b/include/os/linux/spl/sys/kmem_cache.h @@ -98,9 +98,6 @@ extern struct rw_semaphore spl_kmem_cache_sem; #define SPL_MAX_KMEM_ORDER_NR_PAGES (KMALLOC_MAX_SIZE >> PAGE_SHIFT) #endif -#define POINTER_IS_VALID(p) 0 /* Unimplemented */ -#define POINTER_INVALIDATE(pp) /* Unimplemented */ - typedef int (*spl_kmem_ctor_t)(void *, void *, int); typedef void (*spl_kmem_dtor_t)(void *, void *); diff --git a/module/os/freebsd/zfs/zfs_acl.c b/module/os/freebsd/zfs/zfs_acl.c index a9318f7a05..9f735dbb55 100644 --- a/module/os/freebsd/zfs/zfs_acl.c +++ b/module/os/freebsd/zfs/zfs_acl.c @@ -2414,7 +2414,6 @@ zfs_zaccess(znode_t *zp, int mode, int flags, boolean_t skipaclchk, cred_t *cr, * read_acl/read_attributes */ - error = 0; ASSERT3U(working_mode, !=, 0); if ((working_mode & (ACE_READ_ACL|ACE_READ_ATTRIBUTES) && diff --git a/module/os/freebsd/zfs/zfs_znode.c b/module/os/freebsd/zfs/zfs_znode.c index 9575302431..e691e60a8f 100644 --- a/module/os/freebsd/zfs/zfs_znode.c +++ b/module/os/freebsd/zfs/zfs_znode.c @@ -1708,6 +1708,7 @@ zfs_create_fs(objset_t *os, cred_t *cr, nvlist_t *zplprops, dmu_tx_t *tx) } ASSERT3U(version, !=, 0); error = zap_update(os, moid, ZPL_VERSION_STR, 8, 1, &version, tx); + ASSERT0(error); /* * Create zap object used for SA attribute registration diff --git a/module/os/linux/spl/spl-kmem-cache.c b/module/os/linux/spl/spl-kmem-cache.c index 4cceeb8bdf..963e7a1ec9 100644 --- a/module/os/linux/spl/spl-kmem-cache.c +++ b/module/os/linux/spl/spl-kmem-cache.c @@ -791,10 +791,8 @@ spl_kmem_cache_create(const char *name, size_t size, size_t align, } else { unsigned long slabflags = 0; - if (size > (SPL_MAX_KMEM_ORDER_NR_PAGES * PAGE_SIZE)) { - rc = EINVAL; + if (size > (SPL_MAX_KMEM_ORDER_NR_PAGES * PAGE_SIZE)) goto out; - } #if defined(SLAB_USERCOPY) /* @@ -815,10 +813,8 @@ spl_kmem_cache_create(const char *name, size_t size, size_t align, skc->skc_linux_cache = kmem_cache_create( skc->skc_name, size, align, slabflags, NULL); #endif - if (skc->skc_linux_cache == NULL) { - rc = ENOMEM; + if (skc->skc_linux_cache == NULL) goto out; - } } down_write(&spl_kmem_cache_sem); diff --git a/module/os/linux/spl/spl-thread.c b/module/os/linux/spl/spl-thread.c index b863945a1c..b4ef86a5e4 100644 --- a/module/os/linux/spl/spl-thread.c +++ b/module/os/linux/spl/spl-thread.c @@ -178,12 +178,11 @@ issig(int why) sigorsets(&set, &task->blocked, &set); spin_lock_irq(&task->sighand->siglock); - int ret; #ifdef HAVE_DEQUEUE_SIGNAL_4ARG enum pid_type __type; - if ((ret = dequeue_signal(task, &set, &__info, &__type)) != 0) { + if (dequeue_signal(task, &set, &__info, &__type) != 0) { #else - if ((ret = dequeue_signal(task, &set, &__info)) != 0) { + if (dequeue_signal(task, &set, &__info) != 0) { #endif #ifdef HAVE_SIGNAL_STOP spin_unlock_irq(&task->sighand->siglock); diff --git a/module/os/linux/zfs/vdev_disk.c b/module/os/linux/zfs/vdev_disk.c index 2b5c4956dd..925ee9d9fe 100644 --- a/module/os/linux/zfs/vdev_disk.c +++ b/module/os/linux/zfs/vdev_disk.c @@ -425,7 +425,7 @@ vdev_disk_dio_get(dio_request_t *dr) atomic_inc(&dr->dr_ref); } -static int +static void vdev_disk_dio_put(dio_request_t *dr) { int rc = atomic_dec_return(&dr->dr_ref); @@ -449,14 +449,11 @@ vdev_disk_dio_put(dio_request_t *dr) zio_delay_interrupt(zio); } } - - return (rc); } BIO_END_IO_PROTO(vdev_disk_physio_completion, bio, error) { dio_request_t *dr = bio->bi_private; - int rc; if (dr->dr_error == 0) { #ifdef HAVE_1ARG_BIO_END_IO_T @@ -470,7 +467,7 @@ BIO_END_IO_PROTO(vdev_disk_physio_completion, bio, error) } /* Drop reference acquired by __vdev_disk_physio */ - rc = vdev_disk_dio_put(dr); + vdev_disk_dio_put(dr); } static inline void @@ -742,7 +739,7 @@ retry: if (dr->dr_bio_count > 1) blk_finish_plug(&plug); - (void) vdev_disk_dio_put(dr); + vdev_disk_dio_put(dr); return (error); } diff --git a/module/os/linux/zfs/zfs_acl.c b/module/os/linux/zfs/zfs_acl.c index 75638399ef..db1bb95771 100644 --- a/module/os/linux/zfs/zfs_acl.c +++ b/module/os/linux/zfs/zfs_acl.c @@ -2581,7 +2581,6 @@ zfs_fastaccesschk_execute(znode_t *zdp, cred_t *cr) } if (uid == KUID_TO_SUID(ZTOI(zdp)->i_uid)) { - owner = B_TRUE; if (zdp->z_mode & S_IXUSR) { mutex_exit(&zdp->z_acl_lock); return (0); @@ -2591,7 +2590,6 @@ zfs_fastaccesschk_execute(znode_t *zdp, cred_t *cr) } } if (groupmember(KGID_TO_SGID(ZTOI(zdp)->i_gid), cr)) { - groupmbr = B_TRUE; if (zdp->z_mode & S_IXGRP) { mutex_exit(&zdp->z_acl_lock); return (0); @@ -2720,7 +2718,6 @@ zfs_zaccess(znode_t *zp, int mode, int flags, boolean_t skipaclchk, cred_t *cr, * read_acl/read_attributes */ - error = 0; ASSERT(working_mode != 0); if ((working_mode & (ACE_READ_ACL|ACE_READ_ATTRIBUTES) && diff --git a/module/os/linux/zfs/zfs_vnops_os.c b/module/os/linux/zfs/zfs_vnops_os.c index a94af0ea3b..47f132a38a 100644 --- a/module/os/linux/zfs/zfs_vnops_os.c +++ b/module/os/linux/zfs/zfs_vnops_os.c @@ -721,7 +721,6 @@ top: if (have_acl) zfs_acl_ids_free(&acl_ids); - have_acl = B_FALSE; /* * A directory entry already exists for this name. @@ -2532,7 +2531,7 @@ out: dmu_tx_commit(tx); if (attrzp) { if (err2 == 0 && handle_eadir) - err2 = zfs_setattr_dir(attrzp); + err = zfs_setattr_dir(attrzp); zrele(attrzp); } zfs_znode_update_vfs(zp); diff --git a/module/os/linux/zfs/zfs_znode.c b/module/os/linux/zfs/zfs_znode.c index d673da3846..395e2cf57e 100644 --- a/module/os/linux/zfs/zfs_znode.c +++ b/module/os/linux/zfs/zfs_znode.c @@ -495,13 +495,11 @@ zfs_set_inode_flags(znode_t *zp, struct inode *ip) void zfs_znode_update_vfs(znode_t *zp) { - zfsvfs_t *zfsvfs; struct inode *ip; uint32_t blksize; u_longlong_t i_blocks; ASSERT(zp != NULL); - zfsvfs = ZTOZSB(zp); ip = ZTOI(zp); /* Skip .zfs control nodes which do not exist on disk. */ @@ -1885,6 +1883,7 @@ zfs_create_fs(objset_t *os, cred_t *cr, nvlist_t *zplprops, dmu_tx_t *tx) } ASSERT(version != 0); error = zap_update(os, moid, ZPL_VERSION_STR, 8, 1, &version, tx); + ASSERT(error == 0); /* * Create zap object used for SA attribute registration diff --git a/module/zcommon/zfs_fletcher.c b/module/zcommon/zfs_fletcher.c index 44c8f486f6..fa9b8447e9 100644 --- a/module/zcommon/zfs_fletcher.c +++ b/module/zcommon/zfs_fletcher.c @@ -628,7 +628,7 @@ fletcher_4_kstat_data(char *buf, size_t size, void *data) off += snprintf(buf + off, size - off, "%-17s", "fastest"); off += snprintf(buf + off, size - off, "%-15s", fletcher_4_supp_impls[fastest_stat->native]->name); - off += snprintf(buf + off, size - off, "%-15s\n", + (void) snprintf(buf + off, size - off, "%-15s\n", fletcher_4_supp_impls[fastest_stat->byteswap]->name); } else { ptrdiff_t id = curr_stat - fletcher_4_stat_data; @@ -637,7 +637,7 @@ fletcher_4_kstat_data(char *buf, size_t size, void *data) fletcher_4_supp_impls[id]->name); off += snprintf(buf + off, size - off, "%-15llu", (u_longlong_t)curr_stat->native); - off += snprintf(buf + off, size - off, "%-15llu\n", + (void) snprintf(buf + off, size - off, "%-15llu\n", (u_longlong_t)curr_stat->byteswap); } diff --git a/module/zfs/fm.c b/module/zfs/fm.c index 3f05d75977..52ea6262a2 100644 --- a/module/zfs/fm.c +++ b/module/zfs/fm.c @@ -380,8 +380,7 @@ zfs_zevent_wait(zfs_zevent_t *ze) break; } - error = cv_wait_sig(&zevent_cv, &zevent_lock); - if (signal_pending(current)) { + if (cv_wait_sig(&zevent_cv, &zevent_lock) == 0) { error = SET_ERROR(EINTR); break; } else if (!list_is_empty(&zevent_list)) { diff --git a/module/zfs/zfs_ioctl.c b/module/zfs/zfs_ioctl.c index f913c3509b..a1717d4d60 100644 --- a/module/zfs/zfs_ioctl.c +++ b/module/zfs/zfs_ioctl.c @@ -1081,7 +1081,7 @@ zfs_secpolicy_diff(zfs_cmd_t *zc, nvlist_t *innvl, cred_t *cr) (void) innvl; int error; - if ((error = secpolicy_sys_config(cr, B_FALSE)) == 0) + if (secpolicy_sys_config(cr, B_FALSE) == 0) return (0); error = zfs_secpolicy_write_perms(zc->zc_name, ZFS_DELEG_PERM_DIFF, cr); @@ -1230,8 +1230,8 @@ zfs_secpolicy_tmp_snapshot(zfs_cmd_t *zc, nvlist_t *innvl, cred_t *cr) */ int error; - if ((error = zfs_secpolicy_write_perms(zc->zc_name, - ZFS_DELEG_PERM_DIFF, cr)) == 0) + if (zfs_secpolicy_write_perms(zc->zc_name, + ZFS_DELEG_PERM_DIFF, cr) == 0) return (0); error = zfs_secpolicy_snapshot_perms(zc->zc_name, cr); @@ -1279,8 +1279,7 @@ get_nvlist(uint64_t nvl, uint64_t size, int iflag, nvlist_t **nvp) packed = vmem_alloc(size, KM_SLEEP); - if ((error = ddi_copyin((void *)(uintptr_t)nvl, packed, size, - iflag)) != 0) { + if (ddi_copyin((void *)(uintptr_t)nvl, packed, size, iflag) != 0) { vmem_free(packed, size); return (SET_ERROR(EFAULT)); } @@ -2682,7 +2681,6 @@ retry: pair = NULL; while ((pair = nvlist_next_nvpair(genericnvl, pair)) != NULL) { const char *propname = nvpair_name(pair); - err = 0; propval = pair; if (nvpair_type(pair) == DATA_TYPE_NVLIST) { @@ -3095,7 +3093,7 @@ zfs_ioc_set_fsacl(zfs_cmd_t *zc) /* * Verify nvlist is constructed correctly */ - if ((error = zfs_deleg_verify_nvlist(fsaclnv)) != 0) { + if (zfs_deleg_verify_nvlist(fsaclnv) != 0) { nvlist_free(fsaclnv); return (SET_ERROR(EINVAL)); } diff --git a/module/zfs/zvol.c b/module/zfs/zvol.c index 1371e51875..1511f763fd 100644 --- a/module/zfs/zvol.c +++ b/module/zfs/zvol.c @@ -1076,7 +1076,7 @@ zvol_create_minors_cb(const char *dsname, void *arg) * traverse snapshots only, do not traverse children, * and skip the 'dsname' */ - error = dmu_objset_find(dsname, + (void) dmu_objset_find(dsname, zvol_create_snap_minor_cb, (void *)job, DS_FIND_SNAPSHOTS); } diff --git a/module/zstd/include/zstd_compat_wrapper.h b/module/zstd/include/zstd_compat_wrapper.h index 2c4baad27d..4e6561f31a 100644 --- a/module/zstd/include/zstd_compat_wrapper.h +++ b/module/zstd/include/zstd_compat_wrapper.h @@ -73,11 +73,9 @@ #define FSE_buildDTable_raw zfs_FSE_buildDTable_raw #define FSE_buildDTable_rle zfs_FSE_buildDTable_rle #define FSE_buildDTable zfs_FSE_buildDTable -#define FSE_createDTable zfs_FSE_createDTable #define FSE_decompress_usingDTable zfs_FSE_decompress_usingDTable #define FSE_decompress_wksp zfs_FSE_decompress_wksp #define FSE_decompress zfs_FSE_decompress -#define FSE_freeDTable zfs_FSE_freeDTable /* lib/common/pool.o: */ #define POOL_add zfs_POOL_add diff --git a/module/zstd/lib/common/fse_decompress.c b/module/zstd/lib/common/fse_decompress.c index bcc2223ccc..6b3205c63c 100644 --- a/module/zstd/lib/common/fse_decompress.c +++ b/module/zstd/lib/common/fse_decompress.c @@ -56,17 +56,6 @@ /* Function templates */ -FSE_DTable* FSE_createDTable (unsigned tableLog) -{ - if (tableLog > FSE_TABLELOG_ABSOLUTE_MAX) tableLog = FSE_TABLELOG_ABSOLUTE_MAX; - return (FSE_DTable*)malloc( FSE_DTABLE_SIZE_U32(tableLog) * sizeof (U32) ); -} - -void FSE_freeDTable (FSE_DTable* dt) -{ - free(dt); -} - size_t FSE_buildDTable(FSE_DTable* dt, const short* normalizedCounter, unsigned maxSymbolValue, unsigned tableLog) { void* const tdPtr = dt+1; /* because *dt is unsigned, 32-bits aligned on 32-bits */