Re: [PATCH] scsi: csiostor: fix inverted sscanf() check in csio_store_dbg_level

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Medium] The sysfs store function `csio_store_dbg_level` returns 0 on success instead of the number of bytes consumed (`count`), which will cause user-space writers to enter an infinite loop.
--

--- Patch [1]: [PATCH] scsi: csiostor: fix inverted sscanf() check in csio_store_dbg_level ---
commit e7ce8f094df22bd78f194c7117aa937439236e6d
Author: Rodrigo Gobbi <[email protected]>

scsi: csiostor: fix inverted sscanf() check in csio_store_dbg_level

csio_store_dbg_level() has an inverted return check on sscanf():
the existing check treats a truthy return as an error, so every
valid write to the "dbg_level" sysfs attribute was rejected with
-EINVAL. This has been broken since the driver was first added.

> diff --git a/drivers/scsi/csiostor/csio_scsi.c b/drivers/scsi/csiostor/csio_scsi.c
> index b1de615cf3162..d64b47ee1bbe0 100644
> --- a/drivers/scsi/csiostor/csio_scsi.c
> +++ b/drivers/scsi/csiostor/csio_scsi.c
[ ... ]
> @@ -1441,12 +1440,11 @@ csio_store_dbg_level(struct device *dev,
>  	struct csio_lnode *ln = shost_priv(class_to_shost(dev));
>  	struct csio_hw *hw = csio_lnode_to_hw(ln);
>  	uint32_t dbg_level = 0;
> +	int ret;
>  
> -	if (!isdigit(buf[0]))
> -		return -EINVAL;
> -
> -	if (sscanf(buf, "%i", &dbg_level))
> -		return -EINVAL;
> +	ret = kstrtou32(buf, 16, &dbg_level);
> +	if (ret)
> +		return ret;
>  
>  	ln->params.log_level = dbg_level;
>  	hw->params.log_level = dbg_level;

[Severity: Medium]
This is a pre-existing issue, but does csio_store_dbg_level() need to return
the number of bytes consumed (count) instead of 0 on success?

According to the sysfs API contract, a successful store callback must return
the number of bytes consumed. Returning 0 instructs the VFS that 0 bytes were
written, which causes user-space utilities like echo to indefinitely retry
writing the remaining buffer, resulting in an infinite loop.

Since this patch fixes the parsing bug that previously rejected all valid
numeric input, it looks like the success path will now be reachable for the
first time:

drivers/scsi/csiostor/csio_scsi.c:csio_store_dbg_level() {
    ...
	ln->params.log_level = dbg_level;
	hw->params.log_level = dbg_level;

	return 0;
}

Could this return value be updated to count to prevent the infinite loop?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1