On 5/21/26 3:33 PM, Kees Cook wrote: > 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 <[email protected]> > --- > [...] > @@ -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
