Re: [PATCH] target/stat: implement scsiPortBusyStatuses counter

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Accessing `cmd` after calling `queue_status()` can cause a Use-After-Free, as the command may be completed and freed asynchronously by the fabric driver.
- [Medium] The `scsiPortBusyStatuses` implementation only tracks `SAM_STAT_BUSY`, violating RFC 4455.
--

--- Patch [1]: [PATCH] target/stat: implement scsiPortBusyStatuses counter ---
commit 0e48192c7d342b22881ecb0dee446ae287e24607
Author: TanZheng <tanzheng@kylinos.cn>

target/stat: implement scsiPortBusyStatuses counter
    
Implement RFC 4455 scsiPortBusyStatuses (ConfigFS busy_count) by adding a
per-CPU busy_statuses counter to scsi_port_stats, incrementing it when
the target sends SAM_STAT_BUSY, and summing all CPUs in the show handler.

> diff --git a/drivers/target/target_core_stat.c b/drivers/target/target_core_stat.c
> --- a/drivers/target/target_core_stat.c
> +++ b/drivers/target/target_core_stat.c
[ ... ]
> +void target_stat_count_busy_status(struct se_cmd *cmd)
> +{
> +	struct se_lun *lun = cmd->se_lun;
> +
> +	if (cmd->se_cmd_flags & SCF_SCSI_TMR_CDB)
> +		return;
> +
> +	if (cmd->scsi_status == SAM_STAT_BUSY && lun && lun->lun_stats)
> +		this_cpu_inc(lun->lun_stats->busy_statuses);

[Severity: Medium]
Does this code fully implement the RFC 4455 scsiPortBusyStatuses counter?

RFC 4455 states that scsiPortBusyStatuses must increment for BUSY, TASK SET
FULL, and ACA ACTIVE statuses. By checking only for SAM_STAT_BUSY, does this
undercount port busy statuses during queue full events (like
SAM_STAT_TASK_SET_FULL)?

> +}
[ ... ]
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
[ ... ]
> +static int target_queue_status(struct se_cmd *cmd)
> +{
> +	int ret;
> +
> +	ret = cmd->se_tfo->queue_status(cmd);
> +	if (!ret)
> +		target_stat_count_busy_status(cmd);

[Severity: High]
Can this result in a use-after-free on the cmd pointer?

If a fabric driver (such as ib_srpt) calls target_send_busy(cmd) during a
failure path, target_queue_status() will invoke the fabric driver's
queue_status() callback. The fabric driver might transmit the response and its
asynchronous completion handler could drop the final reference to the command,
freeing its memory.

When queue_status() returns, target_queue_status() calls
target_stat_count_busy_status(cmd) which accesses cmd->se_lun and
cmd->scsi_status. Is it safe to dereference cmd here after relinquishing
ownership to queue_status()?

> +
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708093433.838647-1-kensanya@163.com?part=1