Use designated initializers for range_tree_ops_t #1

sempervictus · 2019-09-01T04:04:22Z

The RANDSTRUCT GCC plugin from grsecurity randomizes structure
layout at compile-time to increase the difficulty of targeting
struct members by attackers working blindly against the
resulting binary.
Upstream Linux has adopted a derivative of this plugin, which
requires the use of designated initializers to avoid calling the
wrong function at the post-randomization-wrong-position (and
seeing the relevant incompatible pointer type warnings from GCC).

Signed-off-by: Paul Dagnelie <[email protected]>

The RANDSTRUCT GCC plugin from grsecurity randomizes structure layout at compile-time to increase the difficulty of targeting struct members by attackers working blindly against the resulting binary. Upstream Linux has adopted a derivative of this plugin, which requires the use of designated initializers to avoid calling the wrong function at the post-randomization-wrong-position (and seeing the relevant incompatible pointer type warnings from GCC).

Address 'error: conflicting type qualifiers for rt_btree_ops' build error.

Avoid collisions with other btree implementations which could be linked into/defined in a common namespace. Source bug during builtin compilation: ``` LD vmlinux.o ld: lib/btree.o: in function `btree_last': (.text+0x20): multiple definition of `btree_last'; fs/zfs/zfs/btree.o:btree.c:(.text+0x1350): first defined here ld: lib/btree.o: in function `btree_insert': (.text+0x1470): multiple definition of `btree_insert'; fs/zfs/zfs/btree.o:btree.c:(.text+0x1bd0): first defined here ld: lib/btree.o: in function `btree_remove': (.text+0xe50): multiple definition of `btree_remove'; fs/zfs/zfs/btree.o:btree.c:(.text+0x2930): first defined here ld: lib/btree.o: in function `btree_init': (.text+0x4a0): multiple definition of `btree_init'; fs/zfs/zfs/btree.o:btree.c:(.text+0xf90): first defined here ld: lib/btree.o: in function `btree_destroy': (.text+0x6c0): multiple definition of `btree_destroy'; fs/zfs/zfs/btree.o:btree.c:(.text+0x1a60): first defined here ```

After spa_vdev_remove_aux() is called, the config nvlist is no longer valid, as it's been replaced by the new one (with the specified device removed). Therefore any pointers into the nvlist are no longer valid. So we can't save the result of `fnvlist_lookup_string(nv, ZPOOL_CONFIG_PATH)` (in vd_path) across the call to spa_vdev_remove_aux(). Instead, use spa_strdup() to save a copy of the string before calling spa_vdev_remove_aux. Found by AddressSanitizer: ERROR: AddressSanitizer: heap-use-after-free on address ... READ of size 34 at 0x608000a1fcd0 thread T686 #0 0x7fe88b0c166d (/usr/lib/x86_64-linux-gnu/libasan.so.4+0x5166d) #1 0x7fe88a5acd6e in spa_strdup spa_misc.c:1447 openzfs#2 0x7fe88a688034 in spa_vdev_remove vdev_removal.c:2259 openzfs#3 0x55ffbc7748f8 in ztest_vdev_aux_add_remove ztest.c:3229 openzfs#4 0x55ffbc769fba in ztest_execute ztest.c:6714 openzfs#5 0x55ffbc779a90 in ztest_thread ztest.c:6761 openzfs#6 0x7fe889cbc6da in start_thread openzfs#7 0x7fe8899e588e in __clone 0x608000a1fcd0 is located 48 bytes inside of 88-byte region freed by thread T686 here: #0 0x7fe88b14e7b8 in __interceptor_free #1 0x7fe88ae541c5 in nvlist_free nvpair.c:874 openzfs#2 0x7fe88ae543ba in nvpair_free nvpair.c:844 openzfs#3 0x7fe88ae57400 in nvlist_remove_nvpair nvpair.c:978 openzfs#4 0x7fe88a683c81 in spa_vdev_remove_aux vdev_removal.c:185 openzfs#5 0x7fe88a68857c in spa_vdev_remove vdev_removal.c:2221 openzfs#6 0x55ffbc7748f8 in ztest_vdev_aux_add_remove ztest.c:3229 openzfs#7 0x55ffbc769fba in ztest_execute ztest.c:6714 openzfs#8 0x55ffbc779a90 in ztest_thread ztest.c:6761 openzfs#9 0x7fe889cbc6da in start_thread Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Ryan Moeller <[email protected]> Signed-off-by: Matthew Ahrens <[email protected]> Closes openzfs#9706

`zpool_do_import()` passes `argv[0]`, (optionally) `argv[1]`, and `pool_specified` to `import_pools()`. If `pool_specified==FALSE`, the `argv[]` arguments are not used. However, these values may be off the end of the `argv[]` array, so loading them could dereference unmapped memory. This error is reported by the asan build: ``` ================================================================= ==6003==ERROR: AddressSanitizer: heap-buffer-overflow READ of size 8 at 0x6030000004a8 thread T0 #0 0x562a078b50eb in zpool_do_import zpool_main.c:3796 #1 0x562a078858c5 in main zpool_main.c:10709 openzfs#2 0x7f5115231bf6 in __libc_start_main openzfs#3 0x562a07885eb9 in _start 0x6030000004a8 is located 0 bytes to the right of 24-byte region allocated by thread T0 here: #0 0x7f5116ac6b40 in __interceptor_malloc #1 0x562a07885770 in main zpool_main.c:10699 openzfs#2 0x7f5115231bf6 in __libc_start_main ``` This commit passes NULL for these arguments if they are off the end of the `argv[]` array. Reviewed-by: George Wilson <[email protected]> Reviewed-by: John Kennedy <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Allan Jude <[email protected]> Signed-off-by: Matthew Ahrens <[email protected]> Closes openzfs#12339

In Pool::end_txg(), we wait for the deletion of objects that were obsoleted in the previous txg. This can become a performance problem, since it may take dozens of seconds, but we need to push a txg every few seconds to keep up with the write workload. This system can be nearly idle for dozens of seconds while waiting for these object deletions to complete. This PR changes the behavior such that the object deletion can take place over many TXG's. We can add more objects to the "delete queue" (list of obsolete objects) while still processing the previous batch. And we can record the deletion progress each txg so that if we crash, we don't have to re-issue many completed deletions. Side benefit: refactored the object deletion code into its own file, to slightly reduce the size of pool.rs.

Before this patch, in zfs_domount, if zfs_root or d_make_root fails, we leave zfsvfs != NULL. This will lead to execution of the error handling `if` statement at the `out` label, and hence to a call to dmu_objset_disown and zfsvfs_free. However, zfs_umount, which we call upon failure of zfs_root and d_make_root already does dmu_objset_disown and zfsvfs_free. I suppose this patch rather adds to the brittleness of this part of the code base, but I don't want to invest more time in this right now. To add a regression test, we'd need some kind of fault injection facility for zfs_root or d_make_root, which doesn't exist right now. And even then, I think that regression test would be too closely tied to the implementation. To repro the double-disown / double-free, do the following: 1. patch zfs_root to always return an error 2. mount a ZFS filesystem Here's the stack trace you would see then: VERIFY3(ds->ds_owner == tag) failed (0000000000000000 == ffff9142361e8000) PANIC at dsl_dataset.c:1003:dsl_dataset_disown() Showing stack for process 28332 CPU: 2 PID: 28332 Comm: zpool Tainted: G O 5.10.103-1.nutanix.el7.x86_64 #1 Call Trace: dump_stack+0x74/0x92 spl_dumpstack+0x29/0x2b [spl] spl_panic+0xd4/0xfc [spl] dsl_dataset_disown+0xe9/0x150 [zfs] dmu_objset_disown+0xd6/0x150 [zfs] zfs_domount+0x17b/0x4b0 [zfs] zpl_mount+0x174/0x220 [zfs] legacy_get_tree+0x2b/0x50 vfs_get_tree+0x2a/0xc0 path_mount+0x2fa/0xa70 do_mount+0x7c/0xa0 __x64_sys_mount+0x8b/0xe0 do_syscall_64+0x38/0x50 entry_SYSCALL_64_after_hwframe+0x44/0xa9 Reviewed-by: Richard Yao <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Ryan Moeller <[email protected]> Co-authored-by: Christian Schwarz <[email protected]> Signed-off-by: Christian Schwarz <[email protected]> Closes openzfs#14025

Under certain loads, the following panic is hit: panic: page fault KDB: stack backtrace: #0 0xffffffff805db025 at kdb_backtrace+0x65 #1 0xffffffff8058e86f at vpanic+0x17f openzfs#2 0xffffffff8058e6e3 at panic+0x43 openzfs#3 0xffffffff808adc15 at trap_fatal+0x385 openzfs#4 0xffffffff808adc6f at trap_pfault+0x4f openzfs#5 0xffffffff80886da8 at calltrap+0x8 openzfs#6 0xffffffff80669186 at vgonel+0x186 openzfs#7 0xffffffff80669841 at vgone+0x31 openzfs#8 0xffffffff8065806d at vfs_hash_insert+0x26d openzfs#9 0xffffffff81a39069 at sfs_vgetx+0x149 openzfs#10 0xffffffff81a39c54 at zfsctl_snapdir_lookup+0x1e4 openzfs#11 0xffffffff8065a28c at lookup+0x45c openzfs#12 0xffffffff806594b9 at namei+0x259 openzfs#13 0xffffffff80676a33 at kern_statat+0xf3 openzfs#14 0xffffffff8067712f at sys_fstatat+0x2f openzfs#15 0xffffffff808ae50c at amd64_syscall+0x10c openzfs#16 0xffffffff808876bb at fast_syscall_common+0xf8 The page fault occurs because vgonel() will call VOP_CLOSE() for active vnodes. For this reason, define vop_close for zfsctl_ops_snapshot. While here, define vop_open for consistency. After adding the necessary vop, the bug progresses to the following panic: panic: VERIFY3(vrecycle(vp) == 1) failed (0 == 1) cpuid = 17 KDB: stack backtrace: #0 0xffffffff805e29c5 at kdb_backtrace+0x65 #1 0xffffffff8059620f at vpanic+0x17f openzfs#2 0xffffffff81a27f4a at spl_panic+0x3a openzfs#3 0xffffffff81a3a4d0 at zfsctl_snapshot_inactive+0x40 openzfs#4 0xffffffff8066fdee at vinactivef+0xde openzfs#5 0xffffffff80670b8a at vgonel+0x1ea openzfs#6 0xffffffff806711e1 at vgone+0x31 openzfs#7 0xffffffff8065fa0d at vfs_hash_insert+0x26d openzfs#8 0xffffffff81a39069 at sfs_vgetx+0x149 openzfs#9 0xffffffff81a39c54 at zfsctl_snapdir_lookup+0x1e4 openzfs#10 0xffffffff80661c2c at lookup+0x45c openzfs#11 0xffffffff80660e59 at namei+0x259 openzfs#12 0xffffffff8067e3d3 at kern_statat+0xf3 openzfs#13 0xffffffff8067eacf at sys_fstatat+0x2f openzfs#14 0xffffffff808b5ecc at amd64_syscall+0x10c openzfs#15 0xffffffff8088f07b at fast_syscall_common+0xf8 This is caused by a race condition that can occur when allocating a new vnode and adding that vnode to the vfs hash. If the newly created vnode loses the race when being inserted into the vfs hash, it will not be recycled as its usecount is greater than zero, hitting the above assertion. Fix this by dropping the assertion. FreeBSD-issue: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=252700 Reviewed-by: Andriy Gapon <[email protected]> Reviewed-by: Mateusz Guzik <[email protected]> Reviewed-by: Alek Pinchuk <[email protected]> Reviewed-by: Ryan Moeller <[email protected]> Signed-off-by: Rob Wing <[email protected]> Co-authored-by: Rob Wing <[email protected]> Submitted-by: Klara, Inc. Sponsored-by: rsync.net Closes openzfs#14501

* etc: mask zfs-load-key.service Otherwise, systemd-sysv-generator will generate a service equivalent that breaks the boot: under systemd this is covered by zfs-mount-generator We already do this for zfs-import.service, and other init scripts are suppressed automatically by the "actual" .service files Fixes: commit f04b976 ("Add init script to load keys") Reviewed-by: George Melikov <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes openzfs#14010 Closes openzfs#14019 * Linux: Remove ZFS_AC_KERNEL_SRC_MODULE_PARAM_CALL_CONST autotools check On older kernels, the definition for `module_param_call()` typecasts function pointers to `(void *)`, which triggers -Werror, causing the check to return false when it should return true. Fixing this breaks the build process on some older kernels because they define a `__check_old_set_param()` function in their headers that checks for a non-constified `->set()`. We workaround that through the c preprocessor by defining `__check_old_set_param(set)` to `(set)`, which prevents the build failures. However, it is now apparent that all kernels that we support have adopted the GRSecurity change, so there is no need to have an explicit autotools check for it anymore. We therefore remove the autotools check, while adding the workaround to our headers for the build time non-constified `->set()` check done by older kernel headers. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Jorgen Lundman <[email protected]> Reviewed-by: Ryan Moeller <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#13984 Closes openzfs#14004 * Cleanup: 64-bit kernel module parameters should use fixed width types Various module parameters such as `zfs_arc_max` were originally `uint64_t` on OpenSolaris/Illumos, but were changed to `unsigned long` for Linux compatibility because Linux's kernel default module parameter implementation did not support 64-bit types on 32-bit platforms. This caused problems when porting OpenZFS to Windows because its LLP64 memory model made `unsigned long` a 32-bit type on 64-bit, which created the undesireable situation that parameters that should accept 64-bit values could not on 64-bit Windows. Upon inspection, it turns out that the Linux kernel module parameter interface is extensible, such that we are allowed to define our own types. Rather than maintaining the original type change via hacks to to continue shrinking module parameters on 32-bit Linux, we implement support for 64-bit module parameters on Linux. After doing a review of all 64-bit kernel parameters (found via the man page and also proposed changes by Andrew Innes), the kernel module parameters fell into a few groups: Parameters that were originally 64-bit on Illumos: * dbuf_cache_max_bytes * dbuf_metadata_cache_max_bytes * l2arc_feed_min_ms * l2arc_feed_secs * l2arc_headroom * l2arc_headroom_boost * l2arc_write_boost * l2arc_write_max * metaslab_aliquot * metaslab_force_ganging * zfetch_array_rd_sz * zfs_arc_max * zfs_arc_meta_limit * zfs_arc_meta_min * zfs_arc_min * zfs_async_block_max_blocks * zfs_condense_max_obsolete_bytes * zfs_condense_min_mapping_bytes * zfs_deadman_checktime_ms * zfs_deadman_synctime_ms * zfs_initialize_chunk_size * zfs_initialize_value * zfs_lua_max_instrlimit * zfs_lua_max_memlimit * zil_slog_bulk Parameters that were originally 32-bit on Illumos: * zfs_per_txg_dirty_frees_percent Parameters that were originally `ssize_t` on Illumos: * zfs_immediate_write_sz Note that `ssize_t` is `int32_t` on 32-bit and `int64_t` on 64-bit. It has been upgraded to 64-bit. Parameters that were `long`/`unsigned long` because of Linux/FreeBSD influence: * l2arc_rebuild_blocks_min_l2size * zfs_key_max_salt_uses * zfs_max_log_walking * zfs_max_logsm_summary_length * zfs_metaslab_max_size_cache_sec * zfs_min_metaslabs_to_flush * zfs_multihost_interval * zfs_unflushed_log_block_max * zfs_unflushed_log_block_min * zfs_unflushed_log_block_pct * zfs_unflushed_max_mem_amt * zfs_unflushed_max_mem_ppm New parameters that do not exist in Illumos: * l2arc_trim_ahead * vdev_file_logical_ashift * vdev_file_physical_ashift * zfs_arc_dnode_limit * zfs_arc_dnode_limit_percent * zfs_arc_dnode_reduce_percent * zfs_arc_meta_limit_percent * zfs_arc_sys_free * zfs_deadman_ziotime_ms * zfs_delete_blocks * zfs_history_output_max * zfs_livelist_max_entries * zfs_max_async_dedup_frees * zfs_max_nvlist_src_size * zfs_rebuild_max_segment * zfs_rebuild_vdev_limit * zfs_unflushed_log_txg_max * zfs_vdev_max_auto_ashift * zfs_vdev_min_auto_ashift * zfs_vnops_read_chunk_size * zvol_max_discard_blocks Rather than clutter the lists with commentary, the module parameters that need comments are repeated below. A few parameters were defined in Linux/FreeBSD specific code, where the use of ulong/long is not an issue for portability, so we leave them alone: * zfs_delete_blocks * zfs_key_max_salt_uses * zvol_max_discard_blocks The documentation for a few parameters was found to be incorrect: * zfs_deadman_checktime_ms - incorrectly documented as int * zfs_delete_blocks - not documented as Linux only * zfs_history_output_max - incorrectly documented as int * zfs_vnops_read_chunk_size - incorrectly documented as long * zvol_max_discard_blocks - incorrectly documented as ulong The documentation for these has been fixed, alongside the changes to document the switch to fixed width types. In addition, several kernel module parameters were percentages or held ashift values, so being 64-bit never made sense for them. They have been downgraded to 32-bit: * vdev_file_logical_ashift * vdev_file_physical_ashift * zfs_arc_dnode_limit_percent * zfs_arc_dnode_reduce_percent * zfs_arc_meta_limit_percent * zfs_per_txg_dirty_frees_percent * zfs_unflushed_log_block_pct * zfs_vdev_max_auto_ashift * zfs_vdev_min_auto_ashift Of special note are `zfs_vdev_max_auto_ashift` and `zfs_vdev_min_auto_ashift`, which were already defined as `uint64_t`, and passed to the kernel as `ulong`. This is inherently buggy on big endian 32-bit Linux, since the values would not be written to the correct locations. 32-bit FreeBSD was unaffected because its sysctl code correctly treated this as a `uint64_t`. Lastly, a code comment suggests that `zfs_arc_sys_free` is Linux-specific, but there is nothing to indicate to me that it is Linux-specific. Nothing was done about that. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Jorgen Lundman <[email protected]> Reviewed-by: Ryan Moeller <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Original-patch-by: Andrew Innes <[email protected]> Original-patch-by: Jorgen Lundman <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#13984 Closes openzfs#14004 * cstyle: Allow URLs in C++ comments If a C++ comment contained a URL, the `://` part of the URL would trigger an error because there was no trailing blank, but trailing blanks make for an invalid URL. Modify the check to ignore text within the C++ comment. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Chris Lindee <[email protected]> Closes openzfs#13987 * zfs_domount: fix double-disown of dataset / double-free of zfsvfs_t Before this patch, in zfs_domount, if zfs_root or d_make_root fails, we leave zfsvfs != NULL. This will lead to execution of the error handling `if` statement at the `out` label, and hence to a call to dmu_objset_disown and zfsvfs_free. However, zfs_umount, which we call upon failure of zfs_root and d_make_root already does dmu_objset_disown and zfsvfs_free. I suppose this patch rather adds to the brittleness of this part of the code base, but I don't want to invest more time in this right now. To add a regression test, we'd need some kind of fault injection facility for zfs_root or d_make_root, which doesn't exist right now. And even then, I think that regression test would be too closely tied to the implementation. To repro the double-disown / double-free, do the following: 1. patch zfs_root to always return an error 2. mount a ZFS filesystem Here's the stack trace you would see then: VERIFY3(ds->ds_owner == tag) failed (0000000000000000 == ffff9142361e8000) PANIC at dsl_dataset.c:1003:dsl_dataset_disown() Showing stack for process 28332 CPU: 2 PID: 28332 Comm: zpool Tainted: G O 5.10.103-1.nutanix.el7.x86_64 #1 Call Trace: dump_stack+0x74/0x92 spl_dumpstack+0x29/0x2b [spl] spl_panic+0xd4/0xfc [spl] dsl_dataset_disown+0xe9/0x150 [zfs] dmu_objset_disown+0xd6/0x150 [zfs] zfs_domount+0x17b/0x4b0 [zfs] zpl_mount+0x174/0x220 [zfs] legacy_get_tree+0x2b/0x50 vfs_get_tree+0x2a/0xc0 path_mount+0x2fa/0xa70 do_mount+0x7c/0xa0 __x64_sys_mount+0x8b/0xe0 do_syscall_64+0x38/0x50 entry_SYSCALL_64_after_hwframe+0x44/0xa9 Reviewed-by: Richard Yao <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Ryan Moeller <[email protected]> Co-authored-by: Christian Schwarz <[email protected]> Signed-off-by: Christian Schwarz <[email protected]> Closes openzfs#14025 * Fix potential NULL pointer dereference in lzc_ioctl() Users are allowed to pass NULL to resultp, but we unconditionally assume that they never do. When an external user does pass NULL to resultp, we dereference a NULL pointer. Clang's static analyzer complained about this. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Ryan Moeller <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14008 * Cleanup: Address Clang's static analyzer's unused code complaints These were categorized as the following: * Dead assignment 23 * Dead increment 4 * Dead initialization 6 * Dead nested assignment 18 Most of these are harmless, but since actual issues can hide among them, we correct them. That said, there were a few return values that were being ignored that appeared to merit some correction: * `destroy_callback()` in `cmd/zfs/zfs_main.c` ignored the error from `destroy_batched()`. We handle it by returning -1 if there is an error. * `zfs_do_upgrade()` in `cmd/zfs/zfs_main.c` ignored the error from `zfs_for_each()`. We handle it by doing a binary OR of the error value from the subsequent `zfs_for_each()` call to the existing value. This is how errors are mostly handled inside `zfs_for_each()`. The error value here is passed to exit from the zfs command, so doing a binary or on it is better than what we did previously. * `get_zap_prop()` in `module/zfs/zcp_get.c` ignored the error from `dsl_prop_get_ds()` when the property is not of type string. We return an error when it does. There is a small concern that the `zfs_get_temporary_prop()` call would handle things, but in the case that it does not, we would be pushing an uninitialized numval onto the lua stack. It is expected that `dsl_prop_get_ds()` will succeed anytime that `zfs_get_temporary_prop()` does, so that not giving it a chance to fix things is not a problem. * `draid_merge_impl()` in `tests/zfs-tests/cmd/draid.c` used `nvlist_add_nvlist()` twice in ways in which errors are expected to be impossible, so we switch to `fnvlist_add_nvlist()`. A few notable ones did not merit use of the return value, so we suppressed it with `(void)`: * `write_free_diffs()` in `lib/libzfs/libzfs_diff.c` ignored the error value from `describe_free()`. A look through the commit history revealed that this was intentional. * `arc_evict_hdr()` in `module/zfs/arc.c` did not need to use the returned handle from `arc_hdr_realloc()` because it is already referenced in lists. * `spa_vdev_detach()` in `module/zfs/spa.c` has a comment explicitly saying not to use the error from `vdev_label_init()` because whatever causes the error could be the reason why a detach is being done. Unfortunately, I am not presently able to analyze the kernel modules with Clang's static analyzer, so I could have missed some cases of this. In cases where reports were present in code that is duplicated between Linux and FreeBSD, I made a conscious effort to fix the FreeBSD version too. After this commit is merged, regressions like dee8934 should become extremely obvious with Clang's static analyzer since a regression would appear in the results as the only instance of unused code. That assumes that Coverity does not catch the issue first. My local branch with fixes from all of my outstanding non-draft pull requests shows 118 reports from Clang's static anlayzer after this patch. That is down by 51 from 169. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Cedric Berger <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#13986 * zstream: allow decompress to fix metadata for uncompressed records If a record is uncompressed on-disk but the block pointer insists otherwise, reading it will return EIO. This commit adds an "off" type to the "zstream decompress" command. Using it will set the compression field in a zfs stream to "off" without changing the record's data. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: George Melikov <[email protected]> Signed-off-by: Alan Somers <[email protected]> Sponsored by: Axcient Closes openzfs#13997 * Fix theoretical array overflow in lua_typename() Out of the 12 defects in lua that coverity reports, 5 of them involve `lua_typename()` and out of the dozens of defects in ZFS that lua reports, 3 of them involve `lua_typename()` due to the ZCP code. Given all of the uses of `lua_typename()` in the ZCP code, I was surprised that there were not more. It appears that only 2 were reported because only 3 called `lua_type()`, which does a defective sanity check that allows invalid types to be passed. lua/lua@d4fb848 addressed this in upstream lua 5.3. Unfortunately, we did not get that fix since we use lua 5.2 and we do not have assertions enabled in lua, so the upstream solution would not do anything. While we could adopt the upstream solution and enable assertions, a simpler solution is to fix the issue by making `lua_typename()` return `internal_type_error` whenever it is called with an invalid type. This avoids the array overflow and if we ever see it appear somewhere, we will know there is a problem with the lua interpreter. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#13947 * Linux compat: fix DECLARE_EVENT_CLASS() test when ZFS is built-in ZFS_LINUX_TRY_COMPILE_HEADER macro doesn't take CONFIG_ZFS=y into account. As a result, on several latest Linux versions, configure script marks DECLARE_EVENT_CLASS() available for non-GPL when ZFS is being built as a module, but marks it unavailable when ZFS is built-in. Follow the logic of the neighbor macros and adjust ZFS_LINUX_TRY_COMPILE_HEADER accordingly, so that it doesn't try to look for a .ko when ZFS is built-in. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Richard Yao <[email protected]> Signed-off-by: Alexander Lobakin <[email protected]> Closes openzfs#14006 * Fix declarations of non-global variables This patch inserts the `static` keyword to non-global variables, which where found by the analysis tool smatch. Reviewed-by: Matthew Ahrens <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Tino Reichardt <[email protected]> Closes openzfs#13970 * Coverity model file update Upon review, it was found that the model for malloc() was incorrect. In addition, several general purpose memory allocation functions were missing models: * kmem_vasprintf() * kmem_asprintf() * kmem_strdup() * kmem_strfree() * spl_vmem_alloc() * spl_vmem_zalloc() * spl_vmem_free() * calloc() As an experiment to try to find more bugs, some less than general purpose memory allocation functions were also given models: * zfsvfs_create() * zfsvfs_free() * nvlist_alloc() * nvlist_dup() * nvlist_free() * nvlist_pack() * nvlist_unpack() Finally, the models were improved using additional coverity primitives: * __coverity_negative_sink__() * __coverity_writeall0__() * __coverity_mark_as_uninitialized_buffer__() * __coverity_mark_as_afm_allocated__() In addition, an attempt to inform coverity that certain modelled functions read entire buffers was used by adding the following to certain models: int first = buf[0]; int last = buf[buflen-1]; It was inspired by the QEMU model file. No additional false positives were found by this, but it is believed that the more accurate model file will help to catch false positives in the future. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14048 * Linux 6.1 compat: change order of sys/mutex.h includes After Linux 6.1-rc1 came out, the build started failing to build a couple of the files in the linux spl code due to the mutex_init redefinition. Moving the sys/mutex.h include to a lower position within these two files appears to fix the problem. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Coleman Kane <[email protected]> Closes openzfs#14040 * ZED: Fix uninitialized value reads Coverity complained about a couple of uninitialized value reads in ZED. * zfs_deliver_dle() can pass an uninitialized string to zed_log_msg() * An uninitialized sev.sigev_signo is passed to timer_create() The former would log garbage while the latter is not a real issue, but we might as well suppress it by initializing the field to 0 for consistency's sake. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14047 * Fix NULL pointer dereference in zdb Clang's static analyzer complained that we dereference a NULL pointer in dump_path() if we return 0 when there is an error. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14044 * fm_fmri_hc_create() must call va_end() before returning clang-tidy caught this. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14044 * Fix NULL pointer passed to strlcpy from zap_lookup_impl() Clang's static analyzer pointed out that whenever zap_lookup_by_dnode() is called, we have the following stack where strlcpy() is passed a NULL pointer for realname from zap_lookup_by_dnode(): strlcpy() zap_lookup_impl() zap_lookup_norm_by_dnode() zap_lookup_by_dnode() Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14044 * Fix NULL pointer dereference in spa_open_common() Calling spa_open() will pass a NULL pointer to spa_open_common()'s config parameter. Under the right circumstances, we will dereference the config parameter without doing a NULL check. Clang's static analyzer found this. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14044 * set_global_var() should not pass NULL pointers to dlclose() Both Coverity and Clang's static analyzer caught this. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14044 * Fix possible NULL pointer dereference in sha2_mac_init() If mechanism->cm_param is NULL, passing mechanism to PROV_SHA2_GET_DIGEST_LEN() will dereference a NULL pointer. Coverity reported this. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14044 * Cleanup: Simplify userspace abd_free_chunks() Clang's static analyzer complained that we could use after free here if the inner loop ever iterated. That is a false positive, but upon inspection, the userland abd_alloc_chunks() function never will put multiple consecutive pages into a `struct scatterlist`, so there is no need to loop. We delete the inner loop. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14042 * Cleanup: Delete unnecessary pointer check from vdev_to_nvlist_iter() This confused Clang's static analyzer, making it think there was a possible NULL pointer dereference. There is no NULL pointer dereference. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14042 * Cleanup: metaslab_alloc_dva() should not NULL check mg->mg_next This is a circularly linked list. mg->mg_next can never be NULL. This caused 3 defect reports in Coverity. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14042 * Cleanup: zvol_add_clones() should not NULL check dp It is never NULL because we return early if dsl_pool_hold() fails. This caused Coverity to complain. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14042 * Cleanup: Delete dead code from send_merge_thread() range is always deferenced before it reaches this check, such that the kmem_zalloc() call is never executed. There is also no need to set `range->eos_marker = B_TRUE` because it is already set. Coverity incorrectly complained about a potential NULL pointer dereference because of this. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14042 * Cleanup: Remove NULL pointer check from dmu_send_impl() The pointer is to a structure member, so it is never NULL. Coverity complained about this. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14042 * Fix memory leaks in dmu_send()/dmu_send_obj() If we encounter an EXDEV error when using the redacted snapshots feature, the memory used by dspp.fromredactsnaps is leaked. Clang's static analyzer caught this during an experiment in which I had annotated various headers in an attempt to improve the results of static analysis. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#13973 * Support idmapped mount Adds support for idmapped mounts. Supported as of Linux 5.12 this functionality allows user and group IDs to be remapped without changing their state on disk. This can be useful for portable home directories and a variety of container related use cases. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Ryan Moeller <[email protected]> Signed-off-by: Youzhong Yang <[email protected]> Closes openzfs#12923 Closes openzfs#13671 * Fix sequential resilver drive failure race condition This patch handles the race condition on simultaneous failure of 2 drives, which misses the vdev_rebuild_reset_wanted signal in vdev_rebuild_thread. We retry to catch this inside the vdev_rebuild_complete_sync function. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Richard Yao <[email protected]> Reviewed-by: Dipak Ghosh <[email protected]> Reviewed-by: Akash B <[email protected]> Signed-off-by: Samuel Wycliffe J <[email protected]> Closes openzfs#14041 Closes openzfs#14050 * Add options to zfs redundant_metadata property Currently, additional/extra copies are created for metadata in addition to the redundancy provided by the pool(mirror/raidz/draid), due to this 2 times more space is utilized per inode and this decreases the total number of inodes that can be created in the filesystem. By setting redundant_metadata to none, no additional copies of metadata are created, hence can reduce the space consumed by the additional metadata copies and increase the total number of inodes that can be created in the filesystem. Additionally, this can improve file create performance due to the reduced amount of metadata which needs to be written. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Dipak Ghosh <[email protected]> Signed-off-by: Akash B <[email protected]> Closes openzfs#13680 * Fix userland memory leak in zfs_do_send() Clang 15's static analyzer caught this. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14045 * Fix theoretical use of uninitialized values Clang's static analyzer complains about this. In get_configs(), if we have an invalid configuration that has no top level vdevs, we can read a couple of uninitialized variables. Aborting upon seeing this would break the userland tools for healthy pools, so we instead initialize the two variables to 0 to allow the userland tools to continue functioning for the pools with valid configurations. In zfs_do_wait(), if no wait activities are enabled, we read an uninitialized error variable. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14043 * Silence static analyzer warnings about spa_sync_props() Both Coverity and Clang's static analyzer complain about reading an uninitialized intval if the property is not passed as DATA_TYPE_UINT64 in the nvlist. This is impossible becuase spa_prop_validate() already checked this, but they are unlikely to be the last static analyzers to complain about this, so lets just refactor the code to suppress the warnings. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14043 * crypto_get_ptrs() should always write to *out_data_2 Callers will check if it has been set to NULL before trying to access it, but never initialize it themselves. Whenever "one block spans two iovecs", `crypto_get_ptrs()` will return, without ever setting `*out_data_2 = NULL`. The caller will then do a NULL check against the uninitailized pointer and if it is not zero, pass it to `memcpy()`. The only reason this has not caused horrible runtime issues is because `memcpy()` should be told to copy zero bytes when this happens. That said, this is technically undefined behavior, so we should correct it so that future changes to the code cannot trigger it. Clang's static analyzer found this with the help of CodeChecker's CTU analysis. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14043 * abd_return_buf() should call zfs_refcount_remove_many() early Calling zfs_refcount_remove_many() after freeing memory means we pass a reference to freed memory as the holder. This is not believed to be able to cause a problem, but there is a bit of a tradition of fixing these issues when they appear so that they do not obscure more serious issues in static analyzer output, so we fix this one too. Clang's static analyzer found this with the help of CodeChecker's CTU analysis. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14043 * Add defensive assertion to vdev_queue_aggregate() a6ccb36 had been intended to include this to silence Coverity reports, but this one was missed by mistake. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14043 * Fix build failures Co-authored-by: наб <[email protected]> Co-authored-by: Richard Yao <[email protected]> Co-authored-by: ColMelvin <[email protected]> Co-authored-by: Christian Schwarz <[email protected]> Co-authored-by: Alan Somers <[email protected]> Co-authored-by: Alexander <[email protected]> Co-authored-by: Tino Reichardt <[email protected]> Co-authored-by: Coleman Kane <[email protected]> Co-authored-by: youzhongyang <[email protected]> Co-authored-by: samwyc <[email protected]> Co-authored-by: Akash B <[email protected]>

pcd1193182 and others added 11 commits August 29, 2019 10:51

Reduce loaded range tree memory usage

3b2aa2f

Signed-off-by: Paul Dagnelie <[email protected]>

include bitops.h in makefile

dc39b32

Signed-off-by: Paul Dagnelie <[email protected]>

feedback1

c598aff

Signed-off-by: Paul Dagnelie <[email protected]>

switch to one kstat

5835230

REFACTOR

8a9e254

style

bae389c

matt feedback

975a2a1

bugfix

935d669

change AVL_ macros to TREE_ macros

a89d3ce

style

bc6c2ce

Signed-off-by: Paul Dagnelie <[email protected]>

sempervictus force-pushed the feature/btree-designated_initializers branch from f521996 to 97fb9c1 Compare September 1, 2019 04:11

Ensure rt_btree_ops is extern as range_tree_ops_t

404f9d4

Address 'error: conflicting type qualifiers for rt_btree_ops' build error.

sempervictus mentioned this pull request Sep 1, 2019

Reduce loaded range tree memory usage openzfs/zfs#9181

Merged

12 tasks

pcd1193182 force-pushed the btree branch 2 times, most recently from eeb74cc to 87b194c Compare September 19, 2019 16:29

pcd1193182 force-pushed the btree branch 7 times, most recently from 7fbb13e to 6c41085 Compare September 26, 2019 20:40

pcd1193182 force-pushed the btree branch 3 times, most recently from 42dfc54 to 3ea9b78 Compare October 9, 2019 05:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use designated initializers for range_tree_ops_t #1

Use designated initializers for range_tree_ops_t #1

sempervictus commented Sep 1, 2019

Use designated initializers for range_tree_ops_t #1

Are you sure you want to change the base?

Use designated initializers for range_tree_ops_t #1

Conversation

sempervictus commented Sep 1, 2019