Re: [PATCH] scsi: core: Eliminate scsi_log_{reserve,release}_buffer()

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The newly introduced stack buffers `extd_sense[64]` and `hb_buf[32]` are too small for some valid strings, causing unintended log truncations that contradict the patch's stated goal of eliminating truncation.
--

--- Patch [1]: [PATCH] scsi: core: Eliminate scsi_log_{reserve,release}_buffer() ---
commit 677007193ec0d6990c587a8dbcce949ece4341ff
Author: Bart Van Assche <[email protected]>

scsi: core: Eliminate scsi_log_{reserve,release}_buffer()

This patch eliminates temporary buffer allocations in SCSI logging by using
struct va_format (%pV) and formatting directly with dev_printk(). Helper
functions and stack buffers are introduced for CDB formatting, sense headers,
and result strings.

> diff --git a/drivers/scsi/scsi_logging.c b/drivers/scsi/scsi_logging.c
> index 3cd0d30740855..5bf2f1c74ec70 100644
> --- a/drivers/scsi/scsi_logging.c
> +++ b/drivers/scsi/scsi_logging.c

[ ... ]

> @@ -292,51 +253,29 @@ static void
>  scsi_log_print_sense_hdr(const struct scsi_device *sdev, const char *name,
>  			 int tag, const struct scsi_sense_hdr *sshdr)
>  {
> -	char *logbuf;
> -	size_t off, logbuf_len;
> +	char sense_hdr[64];
> +	char extd_sense[64];
        ^^^^

[Severity: Medium]
Will the extd_sense array truncate valid additional sense codes?

The commit message notes this patch resolves the truncation of log messages,
but it seems this 64-byte stack buffer might be too small. When formatting
long Additional Sense strings such as "Invalid token operation, remote rod
token creation not supported" (64 characters) plus the "Add. Sense: " prefix
added in scsi_format_extd_sense(), the result requires 77 bytes including the
null terminator.

Could this array be expanded to prevent truncation?

>  
> -	logbuf = scsi_log_reserve_buffer(&logbuf_len);
> -	if (!logbuf)
> -		return;
> -	off = sdev_format_header(logbuf, logbuf_len, name, tag);
> -	off += scsi_format_sense_hdr(logbuf + off, logbuf_len - off, sshdr);
> -	dev_printk(KERN_INFO, &sdev->sdev_gendev, "%s", logbuf);
> -	scsi_log_release_buffer(logbuf);
> +	scsi_format_sense_hdr(sense_hdr, sizeof(sense_hdr), sshdr);
> +	sdev_tag_prefix_printk(KERN_INFO, sdev, name, tag, "%s", sense_hdr);
>  
> -	logbuf = scsi_log_reserve_buffer(&logbuf_len);
> -	if (!logbuf)
> -		return;
> -	off = sdev_format_header(logbuf, logbuf_len, name, tag);
> -	off += scsi_format_extd_sense(logbuf + off, logbuf_len - off,
> -				      sshdr->asc, sshdr->ascq);
> -	dev_printk(KERN_INFO, &sdev->sdev_gendev, "%s", logbuf);
> -	scsi_log_release_buffer(logbuf);
> +	scsi_format_extd_sense(extd_sense, sizeof(extd_sense), sshdr->asc,
> +			       sshdr->ascq);

[ ... ]

> @@ -381,58 +320,35 @@ EXPORT_SYMBOL(scsi_print_sense);
>  
>  void scsi_print_result(struct scsi_cmnd *cmd, const char *msg, int disposition)
>  {
> -	char *logbuf;
> -	size_t off, logbuf_len;
>  	const char *mlret_string = scsi_mlreturn_string(disposition);
>  	const char *hb_string = scsi_hostbyte_string(cmd->result);
>  	unsigned long cmd_age = (jiffies - cmd->jiffies_at_alloc) / HZ;
> +	char mlret_buf[32];
> +	char hb_buf[32];
        ^^^^

[Severity: Medium]
Does this code truncate hb_buf when formatting DID_TRANSPORT_DISRUPTED?

If scsi_hostbyte_string() returns "DID_TRANSPORT_DISRUPTED", formatting it as
"hostbyte=DID_TRANSPORT_DISRUPTED" requires 33 bytes including the null
terminator. This will be truncated by the 32-byte array.

Should this array size be increased to comfortably fit the longest possible
hostbyte string?

>  
> -	logbuf = scsi_log_reserve_buffer(&logbuf_len);
> -	if (!logbuf)
> -		return;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0b34db4ad0baadcdf2e223cc1a2c134b4f67ea1a.1786739033.git.bvanassche@acm.org?part=1