spa: Fix FreeBSD sysctl handlers #15719

markjdb · 2023-12-29T15:40:17Z

The change fixes a misuse of FreeBSD's sbuf API which triggers an assertion failure in debug kernels.

Motivation and Context

The patch fixes the panic reported here: https://lists.freebsd.org/archives/freebsd-fs/2023-December/002943.html

Description

We can allocate the sbuf on the stack and use the buffer that we want to return instead of allocating a copy. This is simpler and more efficient, and avoids the bug.

How Has This Been Tested?

I reproduced the panic and verified that it is gone after this patch.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the OpenZFS code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
I have run the ZFS Test Suite with this change applied.
All commit messages are properly formatted and contain Signed-off-by.

amotin

While previous code used sbuf_cpy() on sbuf with drain, that is forbidden according to man page and the assertion, the proposed version is also wrong IMO. If I understand it right, it will allocate empty sbuf with buf as a storage. I don't see a point to use sbufs here at all since we any way prepare a linear buffer to return. IMO we should just use SYSCTL_OUT(req, buf, len + 1).

markjdb · 2023-12-29T16:11:29Z

While previous code used sbuf_cpy() on sbuf with drain, that is forbidden according to man page and the assertion, the proposed version is also wrong IMO. If I understand it right, it will allocate empty sbuf with buf as a storage. I don't see a point to use sbufs here at all since we any way prepare a linear buffer to return. IMO we should just use SYSCTL_OUT(req, buf, len + 1).

Oops, indeed, thank you. The new version does as you suggest.

amotin

Actually, sysctl_handle_string() is already able to do copyout also. Most examples in kernel actually us it. See sysctl_debug_ktr_cpumask() for example.

markjdb · 2023-12-29T16:28:34Z

Actually, sysctl_handle_string() is already able to do copyout also. Most examples in kernel actually us it. See sysctl_debug_ktr_cpumask() for example.

Hmm, yes. But looking a bit further, spa_taskq_param_get() does no bounds checking and simply assumes that the output buffer is large enough. SPA_TASKQ_PARAM_MAX is 128, which looks like it's probably big enough today, but that might become false in the future. Really, spa_taskq_param_get() is an ideal use-case for sbufs, but it's shared with Linux.

We should really fix that while here. Any suggestions for how best to deal with it without introducing some code duplication?

amotin · 2023-12-29T16:43:50Z

On a quick look it seems 128 bytes may actually be just enough for the worst case string, at least now it should be OK. I have vague memories that it is a Linux'ism to pass a page-sized buffer to read module parameters. That's not very nice. If we want some additional security, we could probably pass a buffer size to spa_taskq_param_get(). Considering there are only 2 sprintf's, it should not be too hard to change them to snprintf's. Sure sbufs would fit nicely, but I don't think it worth a second function.

BTW, while there, '\n' appended to the buffer looks like another Linux'ism. FreeBSD should not need it, it probably corrupts output.

sbuf_cpy() resets the sbuf state, which is wrong for sbufs allocated by sbuf_new_for_sysctl(). In particular, this code triggers an assertion failure in sbuf_clear(). Simplify by just using sysctl_handle_string() for both reading and setting the tunable. Fixes: 6930ecb ("spa: make read/write queues configurable") Reported-by: Peter Holm <[email protected]> Signed-off-by: Mark Johnston <[email protected]>

markjdb · 2023-12-29T17:56:09Z

Ok, the latest version uses sysctl_handle_string() for both directions and I tested that it works properly.

On a quick look it seems 128 bytes may actually be just enough for the worst case string, at least now it should be OK. I have vague memories that it is a Linux'ism to pass a page-sized buffer to read module parameters. That's not very nice. If we want some additional security, we could probably pass a buffer size to spa_taskq_param_get(). Considering there are only 2 sprintf's, it should not be too hard to change them to snprintf's. Sure sbufs would fit nicely, but I don't think it worth a second function.

In the Linux case, it's unclear to me how large the buffer actually is, so I don't know what size to pass.

BTW, while there, '\n' appended to the buffer looks like another Linux'ism. FreeBSD should not need it, it probably corrupts output.

Yes, it pollutes sysctl output. I'll push a separate commit for that.

For FreeBSD sysctls, we don't want the extra newline, since the sysctl(8) utility will format strings appropriately. Signed-off-by: Mark Johnston <[email protected]>

amotin · 2023-12-29T18:03:24Z

In the Linux case, it's unclear to me how large the buffer actually is, so I don't know what size to pass.

I vaguely remember its being a page size, but not sure how exactly it is spelled in the KPI:

mav@testm50:/sys/module/clocksource/parameters$ ls -l
total 0
-rw-r--r-- 1 root root 4096 Dec 29 13:02 max_cswd_read_retries
-rw-r--r-- 1 root root 4096 Dec 29 13:02 verify_n_cpus

robn

Thank you. This wasn't easy to figure out; sucks that it wasn't good, but I'm glad its all been checked over by people that know. Cheers :)

opsec · 2024-01-01T17:21:11Z

Tested on my testbox with FreeBSD 15.0-CURRENT, via patch from markj@. sysctl -a works again.

sbuf_cpy() resets the sbuf state, which is wrong for sbufs allocated by sbuf_new_for_sysctl(). In particular, this code triggers an assertion failure in sbuf_clear(). Simplify by just using sysctl_handle_string() for both reading and setting the tunable. Apply to FreeBSD directly since this bug causes "sysctl -a" to crash the kernel. PR: 276039 Reported by: pho Reviewed by: mav Pull Request: openzfs/zfs#15719

For FreeBSD sysctls, we don't want the extra newline, since the sysctl(8) utility will format strings appropriately. Reviewed-by: Rob Norris <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reported-by: Peter Holm <[email protected]> Signed-off-by: Mark Johnston <[email protected]> Closes #15719

sbuf_cpy() resets the sbuf state, which is wrong for sbufs allocated by sbuf_new_for_sysctl(). In particular, this code triggers an assertion failure in sbuf_clear(). Simplify by just using sysctl_handle_string() for both reading and setting the tunable. Fixes: 6930ecb ("spa: make read/write queues configurable") Reviewed-by: Rob Norris <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reported-by: Peter Holm <[email protected]> Signed-off-by: Mark Johnston <[email protected]> Closes openzfs#15719

For FreeBSD sysctls, we don't want the extra newline, since the sysctl(8) utility will format strings appropriately. Reviewed-by: Rob Norris <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reported-by: Peter Holm <[email protected]> Signed-off-by: Mark Johnston <[email protected]> Closes openzfs#15719

sbuf_cpy() resets the sbuf state, which is wrong for sbufs allocated by sbuf_new_for_sysctl(). In particular, this code triggers an assertion failure in sbuf_clear(). Simplify by just using sysctl_handle_string() for both reading and setting the tunable. Fixes: 6930ecb ("spa: make read/write queues configurable") Reviewed-by: Rob Norris <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reported-by: Peter Holm <[email protected]> Signed-off-by: Mark Johnston <[email protected]> Closes openzfs#15719

For FreeBSD sysctls, we don't want the extra newline, since the sysctl(8) utility will format strings appropriately. Reviewed-by: Rob Norris <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reported-by: Peter Holm <[email protected]> Signed-off-by: Mark Johnston <[email protected]> Closes openzfs#15719

sbuf_cpy() resets the sbuf state, which is wrong for sbufs allocated by sbuf_new_for_sysctl(). In particular, this code triggers an assertion failure in sbuf_clear(). Simplify by just using sysctl_handle_string() for both reading and setting the tunable. Fixes: 6930ecb ("spa: make read/write queues configurable") Reviewed-by: Rob Norris <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reported-by: Peter Holm <[email protected]> Signed-off-by: Mark Johnston <[email protected]> Closes openzfs#15719

For FreeBSD sysctls, we don't want the extra newline, since the sysctl(8) utility will format strings appropriately. Reviewed-by: Rob Norris <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reported-by: Peter Holm <[email protected]> Signed-off-by: Mark Johnston <[email protected]> Closes openzfs#15719

sbuf_cpy() resets the sbuf state, which is wrong for sbufs allocated by sbuf_new_for_sysctl(). In particular, this code triggers an assertion failure in sbuf_clear(). Simplify by just using sysctl_handle_string() for both reading and setting the tunable. Fixes: 6930ecb ("spa: make read/write queues configurable") Reviewed-by: Rob Norris <[email protected]> Reviewed-by: Alexander Motin <[email protected]> Reported-by: Peter Holm <[email protected]> Signed-off-by: Mark Johnston <[email protected]> Closes #15719