On Fri, Mar 20, 2026 at 05:48:44PM -0700, Kees Cook wrote:
> In preparation for removing the strlcat API[1], replace the char *pp_buf
> with a struct seq_buf, which tracks the current write position and
> remaining space internally. This allows for:
>
> - Direct use of seq_buf_printf() in place of snprintf()+strlcat()
> pairs, eliminating local tmp buffers throughout.
> - Adjacent strlcat() calls that build strings piece-by-piece
> (e.g., strlcat("["); strlcat(name); strlcat("]")) to be collapsed
> into single seq_buf_printf() calls.
> - Simpler call sites: seq_buf_puts() takes only the buffer and string,
> with no need to pass PAGE_SIZE at every call.
>
> The backing buffer allocation is unchanged (__get_free_page), and the
> output path uses seq_buf_str() to NUL-terminate before passing to
> printk().
Thanks a lot! A few comments below.
Personally I'm in favour of this patch as it also removes a lot of ugly code
(which is scoped string manipulations), FWIW,
Reviewed-by: Andy Shevchenko <[email protected]>
> Link: https://github.com/KSPP/linux/issues/370 [1]
> Cc: Andy Shevchenko <[email protected]>
> Cc: Josh Law <[email protected]>
While not long, this still can be placed...
> Signed-off-by: Kees Cook <[email protected]>
> ---
...somewhere here to reduce unneeded noise in the commit message.
> I couldn't help myself. Here's the full patch, as I suggested in
> https://lore.kernel.org/lkml/202603201230.74BBFFABAD@keescook/
> There are plenty more like this to do...
Indeed, but thanks for the example on how to do that!
...
> @@ -78,14 +76,14 @@ static int riscix_partition(struct parsed_partitions
> *state,
> if (!rr)
> return -1;
>
> - strlcat(state->pp_buf, " [RISCiX]", PAGE_SIZE);
> + seq_buf_puts(&state->pp_buf, " [RISCiX]");
>
>
While at it, you can also drop a second blank line.
> if (rr->magic == RISCIX_MAGIC) {
> unsigned long size = nr_sects > 2 ? 2 : nr_sects;
> int part;
>
> - strlcat(state->pp_buf, " <", PAGE_SIZE);
> + seq_buf_puts(&state->pp_buf, " <");
>
> put_partition(state, slot++, first_sect, size);
> for (part = 0; part < 8; part++) {
...
> @@ -173,24 +173,22 @@ int aix_partition(struct parsed_partitions *state)
> if (d) {
> struct lvm_rec *p = (struct lvm_rec *)d;
> u16 lvm_version = be16_to_cpu(p->version);
> - char tmp[64];
>
> if (lvm_version == 1) {
> int pp_size_log2 = be16_to_cpu(p->pp_size);
>
> pp_bytes_size = 1 << pp_size_log2;
> pp_blocks_size = pp_bytes_size / 512;
> - snprintf(tmp, sizeof(tmp),
> - " AIX LVM header version %u found\n",
> - lvm_version);
> + seq_buf_printf(&state->pp_buf,
> + " AIX LVM header version %u found\n",
> + lvm_version);
Hmm... here it's %u...
> vgda_len = be32_to_cpu(p->vgda_len);
> vgda_sector = be32_to_cpu(p->vgda_psn[0]);
> } else {
> - snprintf(tmp, sizeof(tmp),
> - " unsupported AIX LVM version %d found\n",
> - lvm_version);
> + seq_buf_printf(&state->pp_buf,
> + " unsupported AIX LVM version %d
> found\n",
> + lvm_version);
...and here it's %d. Perhaps also fix specifiers to be aligned with the actual
data type?
> }
> - strlcat(state->pp_buf, tmp, PAGE_SIZE);
> put_dev_sector(sect);
> }
> if (vgda_sector && (d = read_part_sector(state, vgda_sector, §))) {
...
> - char tmp[42];
>
> __be32 *dt = (__be32 *)dostype;
> *dt = pb->pb_Environment[16];
> if (dostype[3] < ' ')
> - snprintf(tmp, sizeof(tmp), " (%c%c%c^%c)",
> - dostype[0], dostype[1],
> - dostype[2], dostype[3] + '@' );
> + seq_buf_printf(&state->pp_buf,
> + " (%c%c%c^%c)",
> + dostype[0], dostype[1],
> + dostype[2],
> + dostype[3] + '@');
> else
> - snprintf(tmp, sizeof(tmp), " (%c%c%c%c)",
> - dostype[0], dostype[1],
> - dostype[2], dostype[3]);
> - strlcat(state->pp_buf, tmp, PAGE_SIZE);
> - snprintf(tmp, sizeof(tmp), "(res %d spb %d)",
> - be32_to_cpu(pb->pb_Environment[6]),
> - be32_to_cpu(pb->pb_Environment[4]));
> - strlcat(state->pp_buf, tmp, PAGE_SIZE);
> + seq_buf_printf(&state->pp_buf,
> + " (%c%c%c%c)",
> + dostype[0], dostype[1],
> + dostype[2], dostype[3]);
Wouldn't
seq_buf_printf(&state->pp_buf, " (%.4s)",
dostype);
work?
But probably okay as in the previous branch it needs more work to follow,
something like
char dostype[8];
...
if (dostype[3] < ' ') {
/* Escape control character */
dostype[4] = dostype[3] + '@';
dostype[3] = '^';
seq_buf_printf(&state->pp_buf, " (%.5s)",
dostype);
} else {
seq_buf_printf(&state->pp_buf, " (%.4s)",
dostype);
}
Taking how invasive is this, it might be better to done separately.
> + seq_buf_printf(&state->pp_buf, "(res %d spb %d)",
> + be32_to_cpu(pb->pb_Environment[6]),
> + be32_to_cpu(pb->pb_Environment[4]));
> }
> res = 1;
--
With Best Regards,
Andy Shevchenko
