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

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

[Kyle Mahlkuch]

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@linux.ibm.com>
Signed-off-by: Kyle Mahlkuch <kmahlkuc@linux.ibm.com>
---
  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);
  	}
  }
-- 
2.52.0

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@linux.ibm.com>
> Signed-off-by: Kyle Mahlkuch <kmahlkuc@linux.ibm.com>
> ---
>   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@suse.de                                +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