Re: [PATCH] block: partitions: Replace pp_buf with struct seq_buf

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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 <andriy.shevchenko@linux.intel.com>

> Link: https://github.com/KSPP/linux/issues/370 [1]

> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Josh Law <objecting@objecting.org>

While not long, this still can be placed...

> Signed-off-by: Kees Cook <kees@kernel.org>
> ---

...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, &sect))) {

...

> -			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