- 28 Sep, 2022 2 commits
-
-
Mateusz Guzik authored
The current value causes significant artificial slowdown during mass parallel file removal, which can be observed both on FreeBSD and Linux when running real workloads. Sample results from Linux doing make -j 96 clean after an allyesconfig modules build: before: 4.14s user 6.79s system 48% cpu 22.631 total after: 4.17s user 6.44s system 153% cpu 6.927 total FreeBSD results in the ticket. Reviewed-by:
Alexander Motin <mav@FreeBSD.org> Reviewed-by:
Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by:
Richard Yao <richard.yao@alumni.stonybrook.edu> Reviewed-by:
George Melikov <mail@gmelikov.ru> Signed-off-by:
Mateusz Guzik <mjguzik@gmail.com> Closes #13932 Closes #13938
-
Toomas Soome authored
Add missing header. Properly ignore return values. Memory leak/unchecked malloc. We do allocate a bit too early (and fail to validate the result). From this, smatch is angry when we overwrite the value of 'node' later. Reviewed-by:
Igor Kozhukhov <igor@dilos.org> Reviewed-by:
Toomas Soome <tsoome@me.com> Closes #13941
-
- 27 Sep, 2022 13 commits
-
-
Christian Schwarz authored
Bit 28 is used by an internal Nutanix feature which might be upstreamed in the future. Bit 29 is the last unused bit. It is reserved to indicate a to-be-designed extension to the stream format which will accomodate more feature flags. Reviewed-by:
Tino Reichardt <milky-zfs@mcmilk.de> Reviewed-by:
Christian Schwarz <christian.schwarz@nutanix.com> Issue #13795 Closes #13796
-
Christian Schwarz authored
Commit 985c33b1 added DMU_BACKUP_FEATURE_BLAKE3 but it is not used by the code. Reviewed-by:
-
Richard Yao authored
Clang's static analyzer found that config.uid is uninitialized when zfs_key_config_load() returns an error. Oddly, this was not included in the unchecked return values that Coverity found. Reviewed-by:
-
Richard Yao authored
Coverity caught unsafe use of `strcpy()` in `ztest_dmu_objset_own()`, `nfs_init_tmpfile()` and `dump_snapshot()`. It also caught an unsafe use of `strlcat()` in `nfs_init_tmpfile()`. Inspired by this, I did an audit of every single usage of `strcpy()` and `strcat()` in the code. If I could not prove that the usage was safe, I changed the code to use either `strlcpy()` or `strlcat()`, depending on which function was originally used. In some cases, `snprintf()` was used to replace multiple uses of `strcat` because it was cleaner. Whenever I changed a function, I preferred to use `sizeof(dst)` when the compiler is able to provide the string size via that. When it could not because the string was passed by a caller, I checked the entire call tree of the function to find out how big the buffer was and hard coded it. Hardcoding is less than ideal, but it is safe unless someone shrinks the buffer sizes being passed. Additionally, Coverity reported three more string related issues: * It caught a case where we do an overlapping memory copy in a call to `snprintf()`. We fix that via `kmem_strdup()` and `kmem_strfree()`. * It caught `sizeof (buf)` being used instead of `buflen` in `zdb_nicenum()`'s call to `zfs_nicenum()`, which is passed to `snprintf()`. We change that to pass `buflen`. * It caught a theoretical unterminated string passed to `strcmp()`. This one is likely a false positive, but we have the information needed to do this more safely, so we change this to silence the false positive not just in coverity, but potentially other static analysis tools too. We switch to `strncmp()`. * There was a false positive in tests/zfs-tests/cmd/dir_rd_update.c. We suppress it by switching to `snprintf()` since other static analysis tools might complain about it too. Interestingly, there is a possible real bug there too, since it assumes that the passed directory path ends with '/'. We add a '/' to fix that potential bug. Reviewed-by:
-
Richard Yao authored
Coverity complains about a possible NULL pointer dereference. This is impossible, but it suspects it because we do a NULL check against `spa->spa_root_vdev`. This NULL check was never necessary and makes the code harder to understand, so we drop it. In particular, we dereference `spa->spa_root_vdev` when `new_state != POOL_STATE_UNINITIALIZED && !hardforce`. The first is only true when spa_reset is called, which only occurs under fault injection. The second is true unless `zpool export -F $POOLNAME` is used. Therefore, we effectively *always* dereference the pointer. In the cases where we do not, there is no reason to think it is unsafe. Therefore this change is safe to make. Reviewed-by:
-
Richard Yao authored
Apply the fix from upstream. http://www.lua.org/bugs.html#5.2.2-1 https://www.opencve.io/cve/CVE-2014-5461 It should be noted that exploiting this requires the `SYS_CONFIG` privilege, and anyone with that privilege likely has other opportunities to do exploits, so it is unlikely that bad actors could exploit this unless system administrators are executing untrusted ZFS Channel Programs. Reviewed-by:
-
Richard Yao authored
In #13871, zfs_vdev_aggregation_limit_non_rotating and zfs_vdev_aggregation_limit being signed was pointed out as a possible reason not to eliminate an unnecessary MAX(unsigned, 0) since the unsigned value was assigned from them. There is no reason for these module parameters to be signed and upon inspection, it was found that there are a number of other module parameters that are signed, but should not be, so we make them unsigned. Making them unsigned made it clear that some other variables in the code should also be unsigned, so we also make those unsigned. This prevents users from setting negative values that could potentially cause bad behaviors. It also makes the code slightly easier to understand. Mostly module parameters that deal with timeouts, limits, bitshifts and percentages are made unsigned by this. Any that are boolean are left signed, since whether booleans should be considered signed or unsigned does not matter. Making zfs_arc_lotsfree_percent unsigned caused a `zfs_arc_lotsfree_percent >= 0` check to become redundant, so it was removed. Removing the check was also necessary to prevent a compiler error from -Werror=type-limits. Several end of line comments had to be moved to their own lines because replacing int with uint_t caused us to exceed the 80 character limit enforced by cstyle.pl. The following were kept signed because they are passed to taskq_create(), which expects signed values and modifying the OpenSolaris/Illumos DDI is out of scope of this patch: * metaslab_load_pct * zfs_sync_taskq_batch_pct * zfs_zil_clean_taskq_nthr_pct * zfs_zil_clean_taskq_minalloc * zfs_zil_clean_taskq_maxalloc * zfs_arc_prune_task_threads Also, negative values in those parameters was found to be harmless. The following were left signed because either negative values make sense, or more analysis was needed to determine whether negative values should be disallowed: * zfs_metaslab_switch_threshold * zfs_pd_bytes_max * zfs_livelist_min_percent_shared zfs_multihost_history was made static to be consistent with other parameters. A number of module parameters were marked as signed, but in reality referenced unsigned variables. upgrade_errlog_limit is one of the numerous examples. In the case of zfs_vdev_async_read_max_active, it was already uint32_t, but zdb had an extern int declaration for it. Interestingly, the documentation in zfs.4 was right for upgrade_errlog_limit despite the module parameter being wrongly marked, while the documentation for zfs_vdev_async_read_max_active (and friends) was wrong. It was also wrong for zstd_abort_size, which was unsigned, but was documented as signed. Also, the documentation in zfs.4 incorrectly described the following parameters as ulong when they were int: * zfs_arc_meta_adjust_restarts * zfs_override_estimate_recordsize They are now uint_t as of this patch and thus the man page has been updated to describe them as uint. dbuf_state_index was left alone since it does nothing and perhaps should be removed in another patch. If any module parameters were missed, they were not found by `grep -r 'ZFS_MODULE_PARAM' | grep ', INT'`. I did find a few that grep missed, but only because they were in files that had hits. This patch intentionally did not attempt to address whether some of these module parameters should be elevated to 64-bit parameters, because the length of a long on 32-bit is 32-bit. Lastly, it was pointed out during review that uint_t is a better match for these variables than uint32_t because FreeBSD kernel parameter definitions are designed for uint_t, whose bit width can change in future memory models. As a result, we change the existing parameters that are uint32_t to use uint_t. Reviewed-by:
Neal Gompa <ngompa@datto.com> Signed-off-by:
-
Richard Yao authored
Coverity found a bug in `zfs_secpolicy_create_clone()` where it is possible for us to pass an unterminated string when `zfs_get_parent()` returns an error. Upon inspection, it is clear that using `strlcpy()` would have avoided this issue. Looking at the codebase, there are a number of other uses of `strncpy()` that are unsafe and even when it is used safely, switching to `strlcpy()` would make the code more readable. Therefore, we switch all instances where we use `strncpy()` to use `strlcpy()`. Unfortunately, we do not portably have access to `strlcpy()` in tests/zfs-tests/cmd/zfs_diff-socket.c because it does not link to libspl. Modifying the appropriate Makefile.am to try to link to it resulted in an error from the naming choice used in the file. Trying to disable the check on the file did not work on FreeBSD because Clang ignores `#undef` when a definition is provided by `-Dstrncpy(...)=...`. We workaround that by explictly including the C file from libspl into the test. This makes things build correctly everywhere. We add a deprecation warning to `config/Rules.am` and suppress it on the remaining `strncpy()` usage. `strlcpy()` is not portably avaliable in tests/zfs-tests/cmd/zfs_diff-socket.c, so we use `snprintf()` there as a substitute. This patch does not tackle the related problem of `strcpy()`, which is even less safe. Thankfully, a quick inspection found that it is used far more correctly than strncpy() was used. A quick inspection did not find any problems with `strcpy()` usage outside of zhack, but it should be said that I only checked around 90% of them. Lastly, some of the fields in kstat_t varied in size by 1 depending on whether they were in userspace or in the kernel. The origin of this discrepancy appears to be 04a479f7 where it was made for no apparent reason. It conflicts with the comment on KSTAT_STRLEN, so we shrink the kernel field sizes to match the userspace field sizes. Reviewed-by:
Ryan Moeller <ryan@iXsystems.com> Signed-off-by:
-
Jitendra Patidar authored
When receiving full/newfs on existing dataset, then it should be done with "-F" flag. Its enforced for initial receive in checks done in zfs_receive_one function of libzfs. Similarly, on resuming full/newfs recv on existing dataset, it should be done with "-F" flag. When dataset doesn't exist, then full/new recv is done on newly created dataset and it's marked INCONSISTENT. But when receiving on existing dataset, recv is first done on %recv and its marked INCONSISTENT. Existing dataset is not marked INCONSISTENT. Resume of full/newfs receive with dataset not INCONSISTENT indicates that its resuming newfs on existing dataset. So, enforce "-F" flag in this case. Also return an error from dmu_recv_resume_begin_check() in zfs kernel, when its resuming full/newfs recv without force. Reviewed-by:
Chunwei Chen <david.chen@nutanix.com> Signed-off-by:
Jitendra Patidar <jitendra.patidar@nutanix.com> Closes #13856 Closes #13857
-
Richard Yao authored
Clang's static analyzer found a bad free caused by skein_mac_atomic(). It will allocate a context on the stack and then pass it to skein_final(), which attempts to free it. Upon inspection, skein_digest_atomic() also has the same problem. These functions were created to match the OpenSolaris ICP API, so I was curious how we avoided this in other providers and looked at the SHA2 code. It appears that SHA2 has a SHA2Final() helper function that is called by the exported sha2_mac_final()/sha2_digest_final() as well as the sha2_mac_atomic() and sha2_digest_atomic() functions. The real work is done in SHA2Final() while some checks and the free are done in sha2_mac_final()/sha2_digest_final(). We fix the use after free in the skein code by taking inspiration from the SHA2 code. We introduce a skein_final_nofree() that does most of the work, and make skein_final() into a function that calls it and then frees the memory. Reviewed-by:
Tony Hutter <hutter2@llnl.gov> Signed-off-by:
-
Richard Yao authored
Recently, I have been making a push to fix things that coverity found. However, I was curious what Clang's static analyzer reported, so I ran it and found things that coverity had missed. * contrib/pam_zfs_key/pam_zfs_key.c: If prop_mountpoint is passed more than once, we leak memory. * module/zfs/zcp_get.c: We leak memory on temporary properties in userspace. * tests/zfs-tests/cmd/draid.c: On error from vdev_draid_rand(), we leak memory if best_map had been allocated by a prior iteration. * tests/zfs-tests/cmd/mkfile.c: Memory used by the loop is not freed before program termination. Arguably, these are all minor issues, but if we ignore them, then they could obscure serious bugs, so we fix them. Reviewed-by:
-
Chris Zubrzycki authored
Reviewed-by:
Chris Zubrzycki <github@mid-earth.net> Closes #13895
-
Richard Yao authored
Coverity found a number of places where we either do MAX(unsigned, 0) or do assertions that a unsigned variable is >= 0. These do nothing, so let us drop them all. It also found a spot where we do `if (unsigned >= 0 && ...)`. Let us also drop the unsigned >= 0 check. Reviewed-by:
-
- 26 Sep, 2022 2 commits
-
-
Richard Yao authored
Coverity complained about this. An error from `hkdf_sha512()` before uio initialization will cause pointers to uninitialized memory to be passed to `zio_crypt_destroy_uio()`. This is a regression that was introduced by cf637391 . Interestingly, this never affected FreeBSD, since the FreeBSD version never had that patch ported. Since moving uio initialization to the top of this function would slow down the qat_crypt() path, we only move the `memset()` calls to the top of the function. This is sufficient to fix this problem. Reviewed-by:
-
Tino Reichardt authored
The extern declaration is only for Linux, move this line into the right #ifdef section. Reviewed-by:
Martin Matuska <mm@FreeBSD.org> Signed-off-by:
-
- 23 Sep, 2022 4 commits
-
-
Richard Yao authored
Coverity caught these. With the exception of the file descriptor leak in tests/zfs-tests/cmd/draid.c, they are all memory leaks. Also, there is a piece of dead code in zfs_get_enclosure_sysfs_path(). We delete it as cleanup. Reviewed-by:
-
Richard Yao authored
Coverity complained about unchecked return values and unused values that turned out to be unused return values. Different approaches were used to handle the different cases of unchecked return values: * cmd/zdb/zdb.c: VERIFY0 was used in one place since the existing code had no error handling. An error message was printed in another to match the rest of the code. * cmd/zed/agents/zfs_retire.c: We dismiss the return value with `(void)` because the value is expected to be potentially unset. * cmd/zpool_influxdb/zpool_influxdb.c: We dismiss the return value with `(void)` because the values are expected to be potentially unset. * cmd/ztest.c: VERIFY0 was used since we want failures if something goes wrong in ztest. * module/zfs/dsl_dir.c: We dismiss the return value with `(void)` because there is no guarantee that the zap entry will always be there. For example, old pools imported readonly would not have it and we do not want to fail here because of that. * module/zfs/zfs_fm.c: `fnvlist_add_*()` was used since the allocations sleep and thus can never fail. * module/zfs/zvol.c: We dismiss the return value with `(void)` because we do not need it. This matches what is already done in the analogous `zfs_replay_write2()`. * tests/zfs-tests/cmd/draid.c: We suppress one return value with `(void)` since the code handles errors already. The other return value is handled by switching to `fnvlist_lookup_uint8_array()`. * tests/zfs-tests/cmd/file/file_fadvise.c: We add error handling. * tests/zfs-tests/cmd/mmap_sync.c: We add error handling for munmap, but ignore failures on remove() with (void) since it is expected to be able to fail. * tests/zfs-tests/cmd/mmapwrite.c: We add error handling. As for unused return values, they were all in places where there was error handling, so logic was added to handle the return values. Reviewed-by:
-
Richard Yao authored
A comment says that the caller should free k_out, but the pointer passed via k_out is not the same pointer we received from strdup(). Instead, it is a pointer into the region we received from strdup(). The free function should always be called with the original pointer, so this is likely a bug. We solve this by calling `strdup()` a second time and then freeing the original pointer. Coverity reported this as a memory leak. Reviewed-by:
-
Tony Hutter authored
If you force fault a drive that's resilvering, it's scan stats can get frozen in time, giving the false impression that it's being resilvered. This commit checks the vdev state to see if the vdev is healthy before reporting "resilvering" or "repairing" in zpool status. Reviewed-by:
-
- 22 Sep, 2022 4 commits
-
-
John Wren Kennedy authored
Currently, these two tests pass on disks with 512 byte sectors. In environments where the backing store is different, the number of blocks allocated to write the same file may differ. This change modifies the reported size check to detect an expected change in the reported number of blocks without specifying a particular number. Reviewed-by:
Tony Nguyen <tony.nguyen@delphix.com> Signed-off-by:
John Kennedy <john.kennedy@delphix.com> Closes #13931
-
Brian Behlendorf authored
Incorrectly sizing the array of hash locks used to protect the dbuf hash table can lead to contention and reduce performance. We could unconditionally allocate a larger array for the locks but it's wasteful, particularly for a low-memory system. Instead, dynamically allocate the array of locks and scale it based on total system memory. Additionally, add a new `dbuf_mutex_cache_shift` module option which can be used to override the hash lock array size. This is disabled by default (dbuf_mutex_hash_shift=0) and can only be set at module load time. The minimum target array size is set to 8192, this matches the current constant value. Note that the count of the dbuf hash table and count of the mutex array were added to the /proc/spl/kstat/zfs/dbufstats kstat. Finally, this change removes the _KERNEL conditional checks. These were not required since for the user space build there is no difference between the kmem and vmem interfaces. Reviewed-by:
-
Brian Behlendorf authored
This reverts commit 34dbc618 . While this change resolved the lock contention observed for certain workloads, it inadventantly reduced the maximum hash inserts/removes per second. This appears to be due to the slightly higher acquisition cost of a rwlock vs a mutex. Reviewed-by:
-
Richard Yao authored
Coverity complains about this. It is not a bug as long as we never shift by more than 31, but it is not terrible to change the constants from 1 to 1ULL as clean up. Reviewed-by:
-
- 20 Sep, 2022 15 commits
-
-
Mateusz Guzik authored
There were never any users and it so happens the operation is not even supported by rrm locks -- the macros were wrong for Linux and FreeBSD when not using it's RMS locks. Reviewed-by:
-
Mateusz Guzik authored
Provides the missing full barrier variant to the membar primitive set. While not used right now, this is probably going to change down the road. Name taken from Solaris, to follow the existing routines. Reviewed-by:
-
youzhongyang authored
get_user_ns() is only done once for each namespace, so put_user_ns() should be done once too. Fix two typos in user_namespace/user_namespace_002.ksh and user_namespace/user_namespace_003.ksh. Reviewed-by:Youzhong Yang <yyang@mathworks.com> Closes #13918
-
Mateusz Guzik authored
See https://cgit.FreeBSD.org/src/commit/?id=a75d1ddd74312f5dd79bc1e965f7077679659f2e Reviewed-by:
-
Mateusz Guzik authored
Reviewed-by:
-
Richard Yao authored
Commit ecd6cf800b63704be73fb264c3f5b6e0dafc068d by marks in OpenSolaris at Tue Jun 26 07:44:24 2007 -0700 introduced a bug where we fail to call `va_end()` before returning. The man page for va_start() says: "Each invocation of va_start() must be matched by a corresponding invocation of va_end() in the same function." Coverity complained about this. Reviewed-by:
-
Richard Yao authored
Coverity complained about this. Reviewed-by:
-
Ameer Hamza authored
For encrypted raw receive, objset creation is delayed until a call to dmu_recv_stream(). ZFS_PROP_SHARESMB property requires objset to be populated when calling zpl_earlier_version(). To correctly handle the ZFS_PROP_SHARESMB property for encrypted raw receive, this change delays setting the property. Reviewed-by:
Ameer Hamza <ahamza@ixsystems.com> Closes #13878
-
Richard Yao authored
The FreeBSD project's coverity scans found dead code in `zfs_readdir()`. Also, the comment above `zfs_readdir()` is out of date. I fixed the comment and deleted all of the dead code, plus additional dead code that was found upon review. Reviewed-by:
-
Richard Yao authored
The FreeBSD project's coverity scans found this. Reviewed-by:
-
Richard Yao authored
Coverity caught a possible NULL pointer dereference in dead code. We can delete it all. Reviewed-by:
-
Richard Yao authored
Coverity complained about the format specifiers not matching variables. In one case, the variable is a constant, so we fix it. In another, we were missing an argument (about which coverity also complained). Reviewed-by:
-
Richard Yao authored
Coverity reported that we pass a pointer to zfsvfs to `dmu_objset_disown()` after freeing zfsvfs in zfsvfs_create_impl() after a failure in zfsvfs_init(). We have nearly identical duplicate versions of this code for FreeBSD and Linux, but interestingly, the FreeBSD version of this code differs in such a way that it does not suffer from this bug. We remove the difference from the FreeBSD version to fix this bug. Reviewed-by:
-
Martin Matuška authored
param_set_arc_free_target(SYSCTL_HANDLER_ARGS) and param_set_arc_no_grow_shift(SYSCTL_HANDLER_ARGS) defined in sysctl_os.c must be made available to arc_os.c. Reviewed-by:
-
Mateusz Guzik authored
There is an ongoing effort to eliminate this feature. Reviewed-by:
-
