Re: [PATCH v4 5/6] scsi: core: Protect host state changes with the host lock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The commit message claims to use READ_ONCE() for all SCSI host state reads, but scsi_host_set_state() still performs a plain read.
--

--- Patch [5]: [PATCH v4 5/6] scsi: core: Protect host state changes with the host lock ---
commit 89fe98af4d7aa30cc00f86bd2b550ec8e90c44ff
Author: Bart Van Assche <bvanassche@acm.org>

scsi: core: Protect host state changes with the host lock

This commit annotates the SCSI host state with __guarded_by(host_lock)
and protects state changes with the host lock. It also updates state
accesses to use READ_ONCE() and WRITE_ONCE() to prevent data races.

Closes: https://lore.kernel.org/all/36d59d0e.6db0.19cdbeee01b.Coremail.luckd0g@163.com/

>     Some but not all SCSI host state changes are protected with the SCSI
>     host lock. Annotate the SCSI host state with __guarded_by(host_lock),
>     protect all SCSI host state changes with the SCSI host lock and use
>     READ_ONCE() for all SCSI host state reads. This patch prevents that
>     KCSAN complains about data races when accessing the SCSI host state.

[Severity: Low]
This isn't a bug, but the commit message states that READ_ONCE() is used for
all SCSI host state reads. However, looking at scsi_host_set_state() in
drivers/scsi/hosts.c, the initial state read still uses a plain access:

int scsi_host_set_state(struct Scsi_Host *shost, enum scsi_host_state state)
{
	enum scsi_host_state oldstate = shost->shost_state;

While this is safe since the host lock is held here, should this read also be
updated to match the commit message, or should the commit message be
adjusted to reflect this exception?

> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index 08ec144f7bf77..9ea161478a10a 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -145,7 +145,7 @@ int scsi_host_set_state(struct Scsi_Host *shost, enum scsi_host_state state)
>  		}
>  		break;
>  	}
> -	shost->shost_state = state;
> +	WRITE_ONCE(shost->shost_state, state);
>  	return 0;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/68f3346cab2be570839350831f82b6c9233832ed.1785534721.git.bvanassche@acm.org?part=5