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

[Maram Srimannarayana Murthy]

On 09/04/26 8:42 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>
> ---


Tested-by: Maram Srimannarayana Murthy <msmurthy@xxxxxxxxxxxxx>

Tested the complete 3-patch series on an IBM Power11 (ppc64le) server 
equipped with an Emulex FC HBA.
The patches applied cleanly, and FC driver parameter validation testing 
was executed continuously for 36 hours.
No crashes, hangs, or functional issues were observed during the test 
period.

Thanks,
Maram Srimannarayana Murthy

>  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);
>  }
>
>  /**
> @@ -2793,7 +2795,9 @@ fc_queue_work(struct Scsi_Host *shost, struct 
> work_struct *work)
>  static void
>  fc_flush_work(struct Scsi_Host *shost)
>  {
> -    if (!fc_host_work_q(shost)) {
> +    struct workqueue_struct *wq = READ_ONCE(fc_host_work_q(shost));
> +
> +    if (!wq) {
>          printk(KERN_ERR
>              "ERROR: FC host '%s' attempted to flush work, "
>              "when no workqueue created.\n", shost->hostt->name);
> @@ -2801,7 +2805,8 @@ fc_flush_work(struct Scsi_Host *shost)
>          return;
>      }
>
> -    flush_workqueue(fc_host_work_q(shost));
> +    /* Use local copy to prevent TOCTOU race */
> +    flush_workqueue(wq);
>  }
>
>  /**
> @@ -2818,16 +2823,18 @@ static int
>  fc_queue_devloss_work(struct Scsi_Host *shost, struct fc_rport *rport,
>                struct delayed_work *work, unsigned long delay)
>  {
> -    if (unlikely(!rport->devloss_work_q)) {
> +    struct workqueue_struct *wq = READ_ONCE(rport->devloss_work_q);
> +
> +    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_delayed_work(rport->devloss_work_q, work, delay);
> +    /* Use local copy to prevent TOCTOU race */
> +    return queue_delayed_work(wq, work, delay);
>  }
>
>  /**
> @@ -2838,7 +2845,9 @@ fc_queue_devloss_work(struct Scsi_Host *shost, 
> struct fc_rport *rport,
>  static void
>  fc_flush_devloss(struct Scsi_Host *shost, struct fc_rport *rport)
>  {
> -    if (unlikely(!rport->devloss_work_q)) {
> +    struct workqueue_struct *wq = READ_ONCE(rport->devloss_work_q);
> +
> +    if (unlikely(!wq)) {
>          printk(KERN_ERR
>              "ERROR: FC host '%s' attempted to flush work, "
>              "when no workqueue created.\n", shost->hostt->name);
> @@ -2846,7 +2855,7 @@ fc_flush_devloss(struct Scsi_Host *shost, struct 
> fc_rport *rport)
>          return;
>      }
>
> -    flush_workqueue(rport->devloss_work_q);
> +    flush_workqueue(wq);
>  }
>
>
> @@ -2905,7 +2914,12 @@ fc_remove_host(struct Scsi_Host *shost)
>      /* flush all stgt delete, and rport delete work items, then kill 
> it  */
>      if (fc_host->work_q) {
>          work_q = fc_host->work_q;
> -        fc_host->work_q = NULL;
> +        /* Prevent new work from being queued by setting work_q to 
> NULL */
> +        WRITE_ONCE(fc_host->work_q, NULL);
> +        /* Ensures NULL is visible to other CPUs before flush */
> +        smp_mb();
> +        /* Flush any work that was queued before NULL assignment */
> +        flush_workqueue(work_q);
>          destroy_workqueue(work_q);
>      }
>  }