Skip to contentSkip to conversation

scsi: lpfc: Fix a possible data race in lpfc_unregister_fcf_rescan()

PATCH In review Tuo Li 3 participants 3 messages Jun 30, 2023 – Jul 5, 2023

Subsystems (2)
SCSI Emulex/Broadcom lpfc FC/FCoE SCSI driver
lore
Share & tools
lore

Tuo Li

author
#0 lore raw

PATCH 1/1 scsi: lpfc: Fix a possible data race in lpfc_unregister_fcf_rescan()

The variable phba->fcf.fcf_flag is often protected by the lock phba->hbalock() when is accessed. Here is an example in lpfc_unregister_fcf_rescan():

  spin_lock_irq(&phba->hbalock);
  phba->fcf.fcf_flag |= FCF_INIT_DISC;
  spin_unlock_irq(&phba->hbalock);

However, in the same function, phba->fcf.fcf_flag is assigned with 0 without holding the lock, and thus can cause a data race:

  phba->fcf.fcf_flag = 0;

To fix this possible data race, a lock and unlock pair is added when accessing the variable phba->fcf.fcf_flag.

Reported-by BassCheck <[email protected]>
Signed-off-by Tuo Li <[email protected]>
---
 drivers/scsi/lpfc/lpfc_hbadisc.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/scsi/lpfc/lpfc_hbadisc.c +2 -0
index 5ba3a9ad9501..9d2feb69cae7 100644
@@ -6961,7 +6961,9 @@ lpfc_unregister_fcf_rescan(struct lpfc_hba *phba)
 	if (rc)
 		return;
 	/* Reset HBA FCF states after successful unregister FCF */
+	spin_lock_irq(&phba->hbalock);
 	phba->fcf.fcf_flag = 0;
+	spin_unlock_irq(&phba->hbalock);
 	phba->fcf.current_rec.flag = 0;
 
 	/*
Signature · 1 line
2.34.1
Reviewed-by on v1 1/1
#1 lore raw

Hi Tuo,

Looks good.

Reviewed-by Justin Tee <[email protected]>

Thanks,
Justin

Quoted 40 lines · Tuo Li

The variable phba->fcf.fcf_flag is often protected by the lock phba->hbalock() when is accessed. Here is an example in lpfc_unregister_fcf_rescan():

  spin_lock_irq(&phba->hbalock);
  phba->fcf.fcf_flag |= FCF_INIT_DISC;
  spin_unlock_irq(&phba->hbalock);

However, in the same function, phba->fcf.fcf_flag is assigned with 0 without holding the lock, and thus can cause a data race:

  phba->fcf.fcf_flag = 0;

To fix this possible data race, a lock and unlock pair is added when accessing the variable phba->fcf.fcf_flag.

Reported-by BassCheck <[email protected]>
Signed-off-by Tuo Li <[email protected]>
---
 drivers/scsi/lpfc/lpfc_hbadisc.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/scsi/lpfc/lpfc_hbadisc.c +2 -1
index 5ba3a9ad9501..9d2feb69cae7 100644
@@ -6961,7 +6961,9 @@ lpfc_unregister_fcf_rescan(struct lpfc_hba *phba)
        if (rc)
                return;
        /* Reset HBA FCF states after successful unregister FCF */
+       spin_lock_irq(&phba->hbalock);
        phba->fcf.fcf_flag = 0;
+       spin_unlock_irq(&phba->hbalock);
        phba->fcf.current_rec.flag = 0;
 
        /*
--

2.34.1

Reviewed-by on v1 1/1
#2 lore raw
In reply to Tuo Li
Quoted 36 lines · Tuo Li

The variable phba->fcf.fcf_flag is often protected by the lock phba->hbalock() when is accessed. Here is an example in lpfc_unregister_fcf_rescan():

  spin_lock_irq(&phba->hbalock);
  phba->fcf.fcf_flag |= FCF_INIT_DISC;
  spin_unlock_irq(&phba->hbalock);

However, in the same function, phba->fcf.fcf_flag is assigned with 0 without holding the lock, and thus can cause a data race:

  phba->fcf.fcf_flag = 0;

To fix this possible data race, a lock and unlock pair is added when accessing the variable phba->fcf.fcf_flag.

Reported-by BassCheck <[email protected]>
Signed-off-by Tuo Li <[email protected]>

---
 drivers/scsi/lpfc/lpfc_hbadisc.c | 2 ++

 1 file changed, 2 insertions(+)
drivers/scsi/lpfc/lpfc_hbadisc.c +2 -0
index 5ba3a9ad9501..9d2feb69cae7 100644
@@ -6961,7 +6961,9 @@ lpfc_unregister_fcf_rescan(struct lpfc_hba
 *phba)
         if (rc)
                 return;
         /* Reset HBA FCF states after successful unregister FCF */
+       spin_lock_irq(&phba->hbalock);
         phba->fcf.fcf_flag = 0;
+       spin_unlock_irq(&phba->hbalock);
         phba->fcf.current_rec.flag = 0;
Quoted 2 lines


        /*

This makes sense and looks good to me

Reviewed-by Laurence Oberman <[email protected]>