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

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 stat=
e reads, but scsi_host_set_state() still performs a plain read.
--

--- Patch [5]: [PATCH v4 5/6] scsi: core: Protect host state changes with t=
he host lock ---
commit 89fe98af4d7aa30cc00f86bd2b550ec8e90c44ff
Author: Bart Van Assche <[email protected]>

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.luck=
[email protected]/

>     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 =3D 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 =3D state;
> +	WRITE_ONCE(shost->shost_state, state);
>  	return 0;

[ ... ]

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/68f3346cab2be570839=
[email protected]?part=3D5