| Message ID | 20230630024748.1035993-1-islituo@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: lpfc: Fix a possible data race in lpfc_unregister_fcf_rescan() | expand |
Hi Tuo, Looks good. Reviewed-by: Justin Tee <justin.tee@broadcom.com> Thanks, Justin On Thu, Jun 29, 2023 at 8:03 PM Tuo Li <islituo@gmail.com> wrote: > > 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 <bass@buaa.edu.cn> > Signed-off-by: Tuo Li <islituo@gmail.com> > --- > drivers/scsi/lpfc/lpfc_hbadisc.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/scsi/lpfc/lpfc_hbadisc.c b/drivers/scsi/lpfc/lpfc_hbadisc.c > index 5ba3a9ad9501..9d2feb69cae7 100644 > --- a/drivers/scsi/lpfc/lpfc_hbadisc.c > +++ b/drivers/scsi/lpfc/lpfc_hbadisc.c > @@ -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 >
On Fri, 2023-06-30 at 10:47 +0800, Tuo Li wrote: > 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 <bass@buaa.edu.cn> > Signed-off-by: Tuo Li <islituo@gmail.com> > --- > drivers/scsi/lpfc/lpfc_hbadisc.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/scsi/lpfc/lpfc_hbadisc.c > b/drivers/scsi/lpfc/lpfc_hbadisc.c > index 5ba3a9ad9501..9d2feb69cae7 100644 > --- a/drivers/scsi/lpfc/lpfc_hbadisc.c > +++ b/drivers/scsi/lpfc/lpfc_hbadisc.c > @@ -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; > > /* This makes sense and looks good to me Reviewed-by: Laurence Oberman <loberman@redhat.com>
diff --git a/drivers/scsi/lpfc/lpfc_hbadisc.c b/drivers/scsi/lpfc/lpfc_hbadisc.c index 5ba3a9ad9501..9d2feb69cae7 100644 --- a/drivers/scsi/lpfc/lpfc_hbadisc.c +++ b/drivers/scsi/lpfc/lpfc_hbadisc.c @@ -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; /*
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 <bass@buaa.edu.cn> Signed-off-by: Tuo Li <islituo@gmail.com> --- drivers/scsi/lpfc/lpfc_hbadisc.c | 2 ++ 1 file changed, 2 insertions(+)