Re: [PATCH v3 57/57] scsi: core: Protect host state changes with the host lock

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The patch missed converting a direct read of `shost_state` in the `mpi3mr` driver to the new `scsi_get_host_state()` API.
- [Medium] Double initialization of `host_lock` via the `spinlock_init` guard overwrites the correct lockdep class name.
- [Medium] `scsi_host_set_state()` performs a plain write to `shost_state`, which races with the newly introduced `READ_ONCE()` lockless readers.
--

--- Patch [57]: [PATCH v3 57/57] scsi: core: Protect host state changes with the host lock ---
commit b02669cdb0e82751599147d300ebcdc8db8d8734
Author: Bart Van Assche <[email protected]>

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

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.

Reported-by: Jianzhou Zhao <[email protected]>
Closes: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Bart Van Assche <[email protected]>

> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index 08ec144f7bf77..222baa7e10680 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
[ ... ]
> @@ -413,7 +415,8 @@ struct Scsi_Host *scsi_host_alloc(const struct scsi_host_template *sht, int priv
>  
>  	shost->host_lock = &shost->default_lock;
>  	spin_lock_init(shost->host_lock);
> -	shost->shost_state = SHOST_CREATED;
> +	scoped_guard(spinlock_init, shost->host_lock)
> +		shost->shost_state = SHOST_CREATED;

[Severity: Medium]
Will this double-initialize the host_lock and overwrite the lockdep class name?

The previous line explicitly calls spin_lock_init(shost->host_lock) which
assigns the correct static lockdep key. Using scoped_guard(spinlock_init, ...)
immediately after calls the initialization macro again internally, overwriting
the lockdep class with a generic macro variable name (_T->lock).

[ ... ]
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index c5f85af4ba072..d0eefefdc0be8 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
[ ... ]
> @@ -790,11 +790,18 @@ static inline struct Scsi_Host *dev_to_shost(struct device *dev)
>  	return container_of(dev, struct Scsi_Host, shost_gendev);
>  }
>  
> +static inline enum scsi_host_state scsi_get_host_state(struct Scsi_Host *shost)
> +{
> +	return context_unsafe(READ_ONCE(shost->shost_state));
> +}
> +

[Severity: Medium]
Does the plain write in scsi_host_set_state() need to be updated to
WRITE_ONCE() to match this new READ_ONCE()?

Since scsi_host_set_state() performs a plain write to shost->shost_state,
concurrent lockless READ_ONCE() readers could still cause KCSAN to flag data
races due to compiler tearing on the writer side. The LKMM requires
WRITE_ONCE() on the writer side when there are concurrent lockless
READ_ONCE() readers.

[ ... ]
> @@ -945,6 +953,7 @@ static inline unsigned char scsi_host_get_guard(struct Scsi_Host *shost)
>  	return shost->prot_guard_type;
>  }
>  
> -extern int scsi_host_set_state(struct Scsi_Host *, enum scsi_host_state);
> +int scsi_host_set_state(struct Scsi_Host *shost, enum scsi_host_state state)
> +	__must_hold(shost->host_lock);
>  
>  #endif /* _SCSI_SCSI_HOST_H */

[Severity: Medium]
Was the mpi3mr driver missed during the tree-wide conversion to the new
scsi_get_host_state() API?

It looks like mpi3mr_qcmd() in drivers/scsi/mpi3mr/mpi3mr_os.c still checks
scmd->device->host->shost_state == SHOST_RECOVERY directly without using
scsi_get_host_state() or holding the host_lock.

Will this trigger a new Sparse warning due to the newly added
__guarded_by(host_lock) annotation on shost_state?

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