Re: [PATCH v3 13/14] lpfc: Refactor calls on fc_disctmo to lpfc_set_disctmo in RSCN handler

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing memory barrier in open-coded lock causes NULL pointer dereference.
--

--- Patch [13]: [PATCH v3 13/14] lpfc: Refactor calls on fc_disctmo to lpfc_set_disctmo in RSCN handler ---
commit 1b8710fdf36e956919fff9a52e4a58021de35371
Author: Justin Tee <justintee8345@gmail.com>

lpfc: Refactor calls on fc_disctmo to lpfc_set_disctmo in RSCN handler

This commit refactors discovery timeout timer handling in the RSCN handler
to use lpfc_set_disctmo() instead of modifying the timer directly. This
ensures discovery timer actions are properly logged and visible.

> diff --git a/drivers/scsi/lpfc/lpfc_els.c b/drivers/scsi/lpfc/lpfc_els.c
> index 0a7e69dcb2dbe..0c5524c613e57 100644
> --- a/drivers/scsi/lpfc/lpfc_els.c
> +++ b/drivers/scsi/lpfc/lpfc_els.c

[ ... ]

> @@ -8497,11 +8494,9 @@ lpfc_els_rcv_rscn(struct lpfc_vport *vport, struct lpfc_iocbq *cmdiocb,
>  		set_bit(FC_RSCN_DEFERRED, &vport->fc_flag);
>  
>  		/* Restart disctmo if its already running */
> -		if (test_bit(FC_DISC_TMO, &vport->fc_flag)) {
> -			tmo = ((phba->fc_ratov * 3) + 3);
> -			mod_timer(&vport->fc_disctmo,
> -				  jiffies + secs_to_jiffies(tmo));
> -		}
> +		if (test_bit(FC_DISC_TMO, &vport->fc_flag))
> +			lpfc_set_disctmo(vport);
> +
>  		if ((rscn_cnt < FC_MAX_HOLD_RSCN) &&
>  		    !test_bit(FC_RSCN_DISCOVERY, &vport->fc_flag)) {
>  			set_bit(FC_RSCN_MODE, &vport->fc_flag);

[Severity: High]
This is a pre-existing issue, but could the lockless updates to
fc_rscn_id_cnt and fc_rscn_flush result in a null pointer dereference?

In lpfc_els_rcv_rscn(), new rscn events are added to the list and the flush
flag is cleared without memory barriers. This happens in both the defer path:

vport->fc_rscn_id_list[rscn_cnt] = pcmd;
vport->fc_rscn_id_cnt++;
...
vport->fc_rscn_flush = 0;

And at the end of the function:

vport->fc_rscn_id_list[vport->fc_rscn_id_cnt++] = pcmd;
...
vport->fc_rscn_flush = 0;

If the compiler or cpu reorders these stores, the updated fc_rscn_id_cnt and
fc_rscn_flush = 0 could become visible to other threads before the new
pointer is stored in fc_rscn_id_list.

A concurrent thread running lpfc_rscn_payload_check() might then see the
incremented count and cleared flush flag, but read a stale null pointer from
the array:

for (i = 0; i < vport->fc_rscn_id_cnt; i++) {
        lp = vport->fc_rscn_id_list[i]->virt;

Would it be safer to use smp_store_release() when clearing fc_rscn_flush,
or add an smp_wmb() before updating the flag?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714011812.106753-1-justintee8345@gmail.com?part=13