Re: [PATCH v3 26/57] scsi: fnic: Enable lock context analysis

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] IRQ state corruption due to mixed use of shared and local variables for spinlock IRQ flags.
- [High] Use-After-Free when iterating over tport lists with list_for_each_entry_safe() and dropping the lock.
--

--- Patch [26]: [PATCH v3 26/57] scsi: fnic: Enable lock context analysis ---
commit b5089a03ad2ecce02a0412b046877b7375670d2d
Author: Bart Van Assche <bvanassche@acm.org>

scsi: fnic: Enable lock context analysis

This commit documents locking requirements across the fnic driver by adding
__must_hold(), __acquire(), and __release() annotations. This allows static
analysis tools to verify that spinlocks and mutexes are held in the required
contexts, suppressing false positives.

> diff --git a/drivers/scsi/fnic/fdls_disc.c b/drivers/scsi/fnic/fdls_disc.c
> --- a/drivers/scsi/fnic/fdls_disc.c
> +++ b/drivers/scsi/fnic/fdls_disc.c
> @@ -425,8 +426,8 @@ void fnic_del_fabric_timer_sync(struct fnic *fnic)
>  	fnic->iport.fabric.del_timer_inprogress = 0;
>  }
>  
> -void fnic_del_tport_timer_sync(struct fnic *fnic,
> -						struct fnic_tport_s *tport)
> +void fnic_del_tport_timer_sync(struct fnic *fnic, struct fnic_tport_s *tport)
> +	__must_hold(&fnic->fnic_lock)
>  {
>  	tport->del_timer_inprogress = 1;
>  	spin_unlock_irqrestore(&fnic->fnic_lock, fnic->lock_flags);

[Severity: High]
This is a pre-existing issue, but does this pattern risk corrupting the IRQ
state?

Functions like fdls_tport_timer_callback() and fnic_tport_event_handler()
acquire the lock using a local stack variable:

    spin_lock_irqsave(&fnic->fnic_lock, flags);

If they subsequently call down into fnic_del_tport_timer_sync() (which drops
the lock using the shared fnic->lock_flags), it restores a stale IRQ state
belonging to a completely different thread, since the local acquisition never
updated the shared fnic->lock_flags. Could this erroneously enable or
disable IRQs prematurely?

[ ... ]

> @@ -1322,6 +1334,7 @@ fdls_send_tgt_adisc(struct fnic_iport_s *iport, struct fnic_tport_s *tport)
>  }
>  
>  bool fdls_delete_tport(struct fnic_iport_s *iport, struct fnic_tport_s *tport)
> +	__must_hold(&iport->fnic->fnic_lock)
>  {
>  	struct fnic_tport_event_s *tport_del_evt;
>  	struct fnic *fnic = iport->fnic;

[Severity: High]
This isn't a bug introduced by this patch, but does dropping the lock in this
function expose the driver to a Use-After-Free during list iteration?

Functions such as fdls_process_gpn_ft_tgt_list() iterate over the
iport->tport_list using list_for_each_entry_safe(tport, next, ...):

    list_for_each_entry_safe(tport, next, &iport->tport_list, links) {
        ...
        fdls_delete_tport(iport, tport);
    }

Inside fdls_delete_tport(), the fnic_lock is temporarily dropped to call
functions like fnic_rport_exch_reset(). While the lock is dropped, a
concurrent thread (like an incoming ELS frame reject or a workqueue event)
can acquire the lock and free the exact tport pointed to by the cached
next pointer. When the original loop reacquires the lock and proceeds to
the next iteration, wouldn't it dereference the freed next pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1785451174.git.bvanassche@acm.org?part=26