Re: [PATCH 3/3] scsi: fc_transport: vport and rport cleanup synchronization

[Hannes Reinecke <hare@suse.de>]

On 4/9/26 5:12 PM, Kyle Mahlkuch wrote:
> Imporve synchronization and cleanup logic in the fc_remove_host() and
> fc_rport_final_delete() to prevent use-after-free conditions during
> host removal
> 
> Vport cleanup:
>    - Mark all vports with FC_VPORT_DELETING under lock
>    - Cancel all vport work synchronously before removing from the list
>    - Synchronous deletion with fc_vport_terminate()
> 
> Rport cleanup, applied to rports and rport_binding
>    - Mark all rport with FC_PORTSTATE_DELETED under lock
>    - Cancel all timers and work synchronously before removing from the list
>    - Call fc_rport_final_delete() synchronously instead of queuing
> 
> fc_rport_final_delete():
>    - Calling cancel_delayed_work_sync() for any outstanding delayed work
>    - Clear FC_RPORT_DEVLOSS_PENDING under lock
>    - Flushing all pending work completes before destruction
> 
> Signed-off-by: Thinh Tran <thinhtr@linux.ibm.com>
> Signed-off-by: Kyle Mahlkuch <kmahlkuc@linux.ibm.com>
> ---
>   drivers/scsi/scsi_transport_fc.c | 85 ++++++++++++++++++++++++--------
>   1 file changed, 64 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/ 
> scsi_transport_fc.c
> index 123b22b52640..0adb9330befc 100644
> --- a/drivers/scsi/scsi_transport_fc.c
> +++ b/drivers/scsi/scsi_transport_fc.c
> @@ -39,6 +39,7 @@ static void fc_li_stats_update(u16 event_type,
>   static void fc_delivery_stats_update(u32 reason_code,
>                        struct fc_fpin_stats *stats);
>   static void fc_cn_stats_update(u16 event_type, struct fc_fpin_stats 
> *stats);
> +static void fc_rport_final_delete(struct work_struct *work);
> 
>   /*
>    * Module Parameters
> @@ -2883,31 +2884,71 @@ fc_remove_host(struct Scsi_Host *shost)
>       struct fc_host_attrs *fc_host = shost_to_fc_host(shost);
>       unsigned long flags;
> 
> -    spin_lock_irqsave(shost->host_lock, flags);
> -
>       /* Remove any vports */
> +    /* Mark FC_VPORT_DELETING for now */
> +    spin_lock_irqsave(shost->host_lock, flags);
>       list_for_each_entry_safe(vport, next_vport, &fc_host->vports, 
> peers) {
>           vport->flags |= FC_VPORT_DELETING;
> -        fc_queue_work(shost, &vport->vport_delete_work);
> +    }
> +    spin_unlock_irqrestore(shost->host_lock, flags);
> +
> +    /*
> +     * remove all vport works synchronously BEFORE removing from list.
> +     * This prevents use-after-free when timers fire.
> +     */
> +    list_for_each_entry_safe(vport, next_vport, &fc_host->vports, peers) {
> +        /* Cancel any pending work/timers */
> +        cancel_work_sync(&vport->vport_delete_work);
> +        /* Now safe to do synchronous deletion */
> +        fc_vport_terminate(vport);
>       }
> 
>       /* Remove any remote ports */
> +    /* Mark rports and rport_bindings with FC_PORTSTATE_DELETED for now */
> +    spin_lock_irqsave(shost->host_lock, flags);
>       list_for_each_entry_safe(rport, next_rport,
>               &fc_host->rports, peers) {
> -        list_del(&rport->peers);
>           rport->port_state = FC_PORTSTATE_DELETED;
> -        fc_queue_work(shost, &rport->rport_delete_work);
>       }
> 
>       list_for_each_entry_safe(rport, next_rport,
>               &fc_host->rport_bindings, peers) {
> -        list_del(&rport->peers);
>           rport->port_state = FC_PORTSTATE_DELETED;
> -        fc_queue_work(shost, &rport->rport_delete_work);
>       }
> -
>       spin_unlock_irqrestore(shost->host_lock, flags);
> 
> +    list_for_each_entry_safe(rport, next_rport,
> +            &fc_host->rports, peers) {
> +        /* Cancel ALL timers and work before removing from list */
> +        cancel_delayed_work_sync(&rport->fail_io_work);
> +        cancel_delayed_work_sync(&rport->dev_loss_work);
> +        cancel_work_sync(&rport->scan_work);
> +        cancel_work_sync(&rport->stgt_delete_work);
> +
> +        spin_lock_irqsave(shost->host_lock, flags);
> +        list_del(&rport->peers);
> +        spin_unlock_irqrestore(shost->host_lock, flags);
> +
> +        /* Now safe to do final deletion synchronously */
> +        fc_rport_final_delete(&rport->rport_delete_work);
> +    }
> +
> +    list_for_each_entry_safe(rport, next_rport,
> +            &fc_host->rport_bindings, peers) {
> +        /* Cancel ALL timers and work before removing from list */
> +        cancel_delayed_work_sync(&rport->fail_io_work);
> +        cancel_delayed_work_sync(&rport->dev_loss_work);
> +        cancel_work_sync(&rport->scan_work);
> +        cancel_work_sync(&rport->stgt_delete_work);
> +
> +        spin_lock_irqsave(shost->host_lock, flags);
> +        list_del(&rport->peers);
> +        spin_unlock_irqrestore(shost->host_lock, flags);
> +
> +        /* Now safe to do final deletion synchronously */
> +        fc_rport_final_delete(&rport->rport_delete_work);
> +    }
> +
>       /* flush all scan work items */
>       scsi_flush_work(shost);
> 
> @@ -2983,21 +3024,22 @@ fc_rport_final_delete(struct work_struct *work)
>           scsi_flush_work(shost);
> 
>       /*
> -     * Cancel any outstanding timers. These should really exist
> -     * only when rmmod'ing the LLDD and we're asking for
> -     * immediate termination of the rports
> +     * Cancel any outstanding delayed work synchronously.
> +     * This must be done BEFORE taking spinlock and BEFORE
> +     * any state changes, as cancel_delayed_work_sync() can sleep.
> +     *
> +     * These timers should only exist when rmmod'ing the LLDD
> +     * and we're asking for immediate termination of rports.
> +     */
> +    cancel_delayed_work_sync(&rport->fail_io_work);
> +    cancel_delayed_work_sync(&rport->dev_loss_work);
> +    cancel_work_sync(&rport->scan_work);
> +    /*
> +     * Now safe to clear the flag under spinlock since all
> +     * async work has been cancelled.
>        */
>       spin_lock_irqsave(shost->host_lock, flags);
> -    if (rport->flags & FC_RPORT_DEVLOSS_PENDING) {
> -        spin_unlock_irqrestore(shost->host_lock, flags);
> -        if (!cancel_delayed_work(&rport->fail_io_work))
> -            fc_flush_devloss(shost, rport);
> -        if (!cancel_delayed_work(&rport->dev_loss_work))
> -            fc_flush_devloss(shost, rport);
> -        cancel_work_sync(&rport->scan_work);
> -        spin_lock_irqsave(shost->host_lock, flags);
> -        rport->flags &= ~FC_RPORT_DEVLOSS_PENDING;
> -    }
> +    rport->flags &= ~FC_RPORT_DEVLOSS_PENDING;
>       spin_unlock_irqrestore(shost->host_lock, flags);
> 
>       /* Delete SCSI target and sdevs */
> @@ -3027,6 +3069,7 @@ fc_rport_final_delete(struct work_struct *work)
>       if (rport->devloss_work_q) {
>           work_q = rport->devloss_work_q;
>           rport->devloss_work_q = NULL;
> +        flush_workqueue(work_q);
>           destroy_workqueue(work_q);
>       }
> 
Similar remarks to the previous patch.
If we set 'SHOST_CANCEL' after we scheduled all these
elements and before calling scsi_flush_work() we can
inhibit further calls to fc_queue_work() and we won't
need this.

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