Re: [PATCH 2/3] scsi: fc_transport: Fix TOCTOU races and workqueue

[Hannes Reinecke]

On 4/9/26 5:12 PM, Kyle Mahlkuch wrote:
> Fix the TOCTOU races in workqueue access, use READ_ONCE() in
> fc_queue_work(), fc_flush_work(), fc_queue_devloss_work(), and
> fc_flush_devloss().
>
> The workqueue destruction in fc_remove_host() uses WRITE_ONCE() to set
> the pointer to NULL to prevents new work, flushing the work queued
> before NULL, then safely destroying it.
>
> Signed-off-by: Thinh Tran <thinhtr@xxxxxxxxxxxxx>
> Signed-off-by: Kyle Mahlkuch <kmahlkuc@xxxxxxxxxxxxx>
> ---
>   drivers/scsi/scsi_transport_fc.c | 36 ++++++++++++++++++++++----------
>   1 file changed, 25 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/ 
> scsi_transport_fc.c
> index 3a821afee9bc..123b22b52640 100644
> --- a/drivers/scsi/scsi_transport_fc.c
> +++ b/drivers/scsi/scsi_transport_fc.c
> @@ -2774,16 +2774,18 @@ EXPORT_SYMBOL(fc_release_transport);
>   static int
>   fc_queue_work(struct Scsi_Host *shost, struct work_struct *work)
>   {
> -    if (unlikely(!fc_host_work_q(shost))) {
> +    struct workqueue_struct *wq = READ_ONCE(fc_host_work_q(shost));
> +
> +    if (unlikely(!wq)) {
>           printk(KERN_ERR
>               "ERROR: FC host '%s' attempted to queue work, "
>               "when no workqueue created.\n", shost->hostt->name);
>           dump_stack();
> -
>           return -EINVAL;
>       }
>
> -    return queue_work(fc_host_work_q(shost), work);
> +    /* Use local copy to prevent TOCTOU race */
> +    return queue_work(wq, work);
>   }
This is wrong. The only way when this will become an issue is
if someone calls fc_queue_work() after the queue has been deleted,
ie when the Scsi Host is in the process of being shutdown.

And I would argue that it's a programming error if some driver would
scheduled workqueue elements at that time; the driver should know that
the host is about to be terminated, and should not try to schedule
anything here.

Seeing that every driver is using this sequence:

fc_remove_host()
scsi_remove_host()

and scsi_remove_host() is setting the 'SHOST_CANCEL' state
it would be far better to set the 'SHOST_CANCEL' state first and
then call fc_remove_host().
Then we can have an easy check in fc_queue_work() to disallow any
new elements when the host state is SHOST_CANCEL (or SHOST_DELETE).

I'll send a patch.

Cheers,

Hannes
--
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@xxxxxxx                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich