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

[Maram Srimannarayana Murthy <msmurthy@linux.ibm.com>]

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


Tested-by: Maram Srimannarayana Murthy <msmurthy@linux.ibm.com>

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 | 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);
>      }
>