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

[kensanya <kensanya@163.com>]

At 2026-07-08 17:57:22, sashiko-bot@kernel.org wrote:
>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)?
>
This patch currently only supports the statistics of scsiPortBusyStatuses.
>> +}
>[ ... ]
>> 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()?
>
Count after a successful queue_status() because RFC 4455 counts statuses actually sent 
by the port. Fabric may fail and retry via queue_full, and may even change the SCSI 
status on failure (e.g. tcm_fc BUSY → TASK_SET_FULL). Counting before send would
over-count and double-count on retries.

Regarding the UAF issue, I will release the second patch.
>> +
>> +	return ret;
>> +}
>
>-- 
>Sashiko AI review · https://sashiko.dev/#/patchset/20260708093433.838647-1-kensanya@163.com?part=1

Best regard!
TanZheng