Re: [PATCH v4 03/11] seq_buf: Copy what fits when seq_buf_puts() and seq_buf_putmem() overflow

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 3117 bytes --]

> seq_buf: Copy what fits when seq_buf_puts() and seq_buf_putmem() overflow

TL;DR: seq_buf_puts() now copies a partial string on overflow, which may break
lpfc_rx_monitor_report(), which relies on whole-record drops. Debugfs output
could then end in a truncated record followed by a duplicate.

> diff --git a/lib/seq_buf.c b/lib/seq_buf.c
> index 35a5964370b4..60e9eadb3ef7 100644
> --- a/lib/seq_buf.c
> +++ b/lib/seq_buf.c

[ ... ]

> @@ -194,6 +196,11 @@ int seq_buf_puts(struct seq_buf *s, const char *str)
>  		s->len += len - 1;
>  		return 0;
>  	}
> +	/* Copy what fits, so the buffer never holds stale bytes */
> +	if (s->len < s->size) {
> +		strscpy(s->buffer + s->len, str, s->size - s->len);
> +		s->len = s->size;
> +	}
>  	seq_buf_set_overflow(s);
>  	return -1;
>  }

Can this break callers that rely on seq_buf_puts() dropping a string whole
when it does not fit?

lpfc_rx_monitor_report() in drivers/scsi/lpfc/lpfc_sli.c was converted to
seq_buf in 22d4cbf6f750 ("scsi: lpfc: Replace strlcat() with seq_buf in
lpfc_rx_monitor_report()"), and that changelog says seq_buf_puts() "appends
it only when it fits whole, so the output keeps ending at the last complete
record".  The code still documents this:

	/*
	 * Drop a record whole if it does not fit, without
	 * consuming its ring entry.
	 */
	scnprintf(tmp, sizeof(tmp), "%03d:\t%-16llu...", ...);

	if (seq_buf_puts(&s, tmp) < 0)
		break;

With this patch, a record that does not fit is now copied up to
buf[buf_len - 2] and NUL-terminated at buf[buf_len - 1] before the loop
breaks.

lpfc_rx_monitor_read() then returns strlen(buffer) bytes through
simple_read_from_buffer(), so the debugfs output ends in a truncated record
with no newline.

Since the loop breaks before *head_idx is advanced, the same ring entry is
printed again in full at the start of the next read, so the reader sees a
partial copy followed by a duplicate.

Both the old strlcat() code ("if ((strlen(buf) + strlen(tmp)) >= buf_len)
break;") and the seq_buf version before this patch dropped the record whole.

Getting there needs the 800 rows to average more than about 128 bytes in the
128 * LPFC_MAX_RXMONITOR_ENTRY buffer, which can happen when the u64/u32
counters (cmf_info, max_read_cnt, avg_io_latency, timer_utilization, ...)
are wider than their %-8/%-16 columns.

The commit message does not mention auditing seq_buf_puts() callers that
rely on the old semantics, and nothing later in the series changes lpfc.

Would it make sense to have lpfc_rx_monitor_report() check
strlen(tmp) < seq_buf_buffer_left(&s) before calling seq_buf_puts()?

The other callers I checked (setup_trace_event(), the usbhid name building,
the partition pp_buf users, string_stream_get_string(), dynevent_str_add()
and the hist command builders) either treat overflow as an error or used
strlcat() before, which also copied a partial string.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/37096036516