Thread (32 messages) 32 messages, 10 authors, 8d ago

Re: [PATCH 08/11] params: Convert generic kernel_param_ops .get helpers to seq_buf

From: Petr Pavlu <petr.pavlu@suse.com>
Date: 2026-05-25 17:10:08
Also in: dmaengine, dri-devel, intel-gfx, kvm, linux-acpi, linux-arch, linux-fsdevel, linux-hardening, linux-media, linux-mm, linux-modules, linux-pci, linux-pm, linux-rdma, linux-scsi, linux-security-module, linux-serial, linux-um, linux-usb, lkml, netdev, qemu-devel, virtualization

On 5/21/26 3:33 PM, Kees Cook wrote:
quoted hunk ↗ jump to hunk
Convert the generic struct kernel_param_ops .get helpers in
kernel/params.c directly to the seq_buf signature, drop their legacy
"char *" form, and refresh prototypes in <linux/moduleparam.h>:

  param_get_byte/short/ushort/int/uint/long/ulong/ullong/hexint
  param_get_charp/bool/invbool/string
  param_array_get

The STANDARD_PARAM_DEF() macro expands to a seq_buf body for every
numeric helper. param_array_get() now writes element output directly
into the parent seq_buf when the element ops provide .get; it only
allocates the per-call PAGE_SIZE bounce buffer when the element ops
still use the legacy .get_str path. The common "rewrite the prior
element's trailing newline as a comma" step lives outside both
branches so the two paths share it.

The non-core changes in this commit (arch/x86/kvm, mm/kfence,
drivers/dma/dmatest, security/apparmor) are the small set of callers that
directly invoke one of the converted generic helpers from their own .get
callback (e.g. an apparmor wrapper that adds a capability check and then
delegates to param_get_bool()). Because the helpers' signature changes
here, these wrappers must move in lockstep. Each of them is updated
to take "struct seq_buf *" and pass it through; param_get_debug() in
apparmor also pulls aa_print_debug_params() (and its val_mask_to_str()
helper, in security/apparmor/lib.c) over to seq_buf, since that is the
only consumer. No other behavioural change is intended.

Custom .get callbacks that do not delegate to a generic helper (and
therefore still match the .get_str signature) are routed automatically
to the .get_str field by the DEFINE_KERNEL_PARAM_OPS _Generic dispatcher
and are deliberately left alone here, to be changed separately within
their respective subsystems.

Signed-off-by: Kees Cook <kees@kernel.org>
---
[...]
@@ -453,36 +457,46 @@ static int param_array_set(const char *val, const struct kernel_param *kp)
 			   arr->num ?: &temp_num);
 }
 
-static int param_array_get(char *buffer, const struct kernel_param *kp)
+static int param_array_get(struct seq_buf *s, const struct kernel_param *kp)
 {
-	int i, off, ret;
-	char *elem_buf;
 	const struct kparam_array *arr = kp->arr;
 	struct kernel_param p = *kp;
+	char *elem_buf = NULL;
+	int i, ret = 0;
 
-	elem_buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
-	if (!elem_buf)
-		return -ENOMEM;
+	for (i = 0; i < (arr->num ? *arr->num : arr->max); i++) {
+		size_t before = s->len;
 
-	for (i = off = 0; i < (arr->num ? *arr->num : arr->max); i++) {
 		p.arg = arr->elem + arr->elemsize * i;
 		check_kparam_locked(p.mod);
-		ret = arr->ops->get_str(elem_buf, &p);
-		if (ret < 0)
-			goto out;
-		ret = min(ret, (int)(PAGE_SIZE - 1 - off));
-		if (!ret)
+
+		if (arr->ops->get) {
+			ret = arr->ops->get(s, &p);
+			if (ret < 0)
+				goto out;
+		} else {
+			if (!elem_buf) {
+				elem_buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
+				if (!elem_buf) {
+					ret = -ENOMEM;
+					goto out;
+				}
+			}
+			ret = arr->ops->get_str(elem_buf, &p);
+			if (ret < 0)
+				goto out;
+			seq_buf_putmem(s, elem_buf, ret);
+		}
+
+		/* Nothing got written (e.g. overflow) — stop. */
+		if (s->len == before)
 			break;
+
 		/* Replace the previous element's trailing newline with a comma. */
-		if (i)
-			buffer[off - 1] = ',';
-		memcpy(buffer + off, elem_buf, ret);
-		off += ret;
-		if (off == PAGE_SIZE - 1)
-			break;
+		if (i && s->buffer[before - 1] == '\n')
+			s->buffer[before - 1] = ',';
 	}
-	buffer[off] = '\0';
-	ret = off;
+	ret = 0;
 out:
 	kfree(elem_buf);
 	return ret;
Since you're almost completely rewriting the logic in param_array_get(),
I suggest tightening it up a bit. The function could warn or return an
error when a kernel_param_ops::get/get_str() call adds a string that
doesn't terminate with '\n', specifically, when the call adds either
a zero-length string or a non-zero-length string that ends with
a different character (unless an overflow occurred).

The updated code silently stops the loop when a get call returns
a zero-length string. Similarly, handling of a string not terminated by
'\n' is halfway there because of the added check
"s->buffer[before - 1] == '\n'".

-- 
Thanks,
Petr
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help