Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6105b8bf9d35ca2906094a0f9a58b5351bc741da.camel@redhat.com>
Subject: Re: [PATCH] scsi: lpfc: Fix a possible data race in
 lpfc_unregister_fcf_rescan()
From:   Laurence Oberman <loberman@redhat.com>
To:     Tuo Li <islituo@gmail.com>, james.smart@broadcom.com,
        dick.kennedy@broadcom.com, jejb@linux.ibm.com,
        martin.petersen@oracle.com
Cc:     linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org,
        baijiaju1990@outlook.com, BassCheck <bass@buaa.edu.cn>
Date:   Wed, 05 Jul 2023 10:35:08 -0400
In-Reply-To: <20230630024748.1035993-1-islituo@gmail.com>
References: <20230630024748.1035993-1-islituo@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4 (3.46.4-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Fri, 2023-06-30 at 10:47 +0800, Tuo Li wrote:
> The variable phba->fcf.fcf_flag is often protected by the lock=20
> phba->hbalock() when is accessed. Here is an example in=20
> lpfc_unregister_fcf_rescan():
>=20
> =C2=A0 spin_lock_irq(&phba->hbalock);
> =C2=A0 phba->fcf.fcf_flag |=3D FCF_INIT_DISC;
> =C2=A0 spin_unlock_irq(&phba->hbalock);
>=20
> However, in the same function, phba->fcf.fcf_flag is assigned with 0=20
> without holding the lock, and thus can cause a data race:
>=20
> =C2=A0 phba->fcf.fcf_flag =3D 0;
>=20
> To fix this possible data race, a lock and unlock pair is added when=20
> accessing the variable phba->fcf.fcf_flag.
>=20
> Reported-by: BassCheck <bass@buaa.edu.cn>
> Signed-off-by: Tuo Li <islituo@gmail.com>
> ---
> =C2=A0drivers/scsi/lpfc/lpfc_hbadisc.c | 2 ++
> =C2=A01 file changed, 2 insertions(+)
>=20
> 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)
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (rc)
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0return;
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* Reset HBA FCF states a=
fter successful unregister FCF */
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0spin_lock_irq(&phba->hbalock);
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0phba->fcf.fcf_flag =3D 0;
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0spin_unlock_irq(&phba->hbalock=
);
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0phba->fcf.current_rec.fla=
g =3D 0;
> =C2=A0
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/*

This makes sense and looks good to me
Reviewed-by: Laurence Oberman <loberman@redhat.com>