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;
MIME-Version: 1.0
References: <20230628153010.57705-1-dg573847474@gmail.com> <eb9476ed-8dc3-a7b6-7478-b7fba3d8e33b@hisilicon.com>
In-Reply-To: <eb9476ed-8dc3-a7b6-7478-b7fba3d8e33b@hisilicon.com>
From:   Chengfeng Ye <dg573847474@gmail.com>
Date:   Wed, 5 Jul 2023 18:06:18 +0800
Message-ID: <CAAo+4rXdvk5NKCi84-7M=vBRSq21iV=6KQn2Te8EdEsr8GOTOw@mail.gmail.com>
Subject: Re: [PATCH] scsi: hisi_sas: Fix potential deadlock on &hisi_hba->lock
To:     "chenxiang (M)" <chenxiang66@hisilicon.com>
Cc:     jejb@linux.ibm.com, martin.petersen@oracle.com,
        linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Thanks for the notice!

Yes, removing the lock acquisition inside hisi_sas_port_notify_formed()
can avoid the deadlock problem by the way.

Best Regards,
Chengfeng

chenxiang (M) <chenxiang66@hisilicon.com> =E4=BA=8E2023=E5=B9=B47=E6=9C=884=
=E6=97=A5=E5=91=A8=E4=BA=8C 13:55=E5=86=99=E9=81=93=EF=BC=9A
>
> Hi,
>
>
> =E5=9C=A8 2023/6/28 =E6=98=9F=E6=9C=9F=E4=B8=89 23:30, Chengfeng Ye =E5=
=86=99=E9=81=93:
> > As &hisi_hba->lock is acquired by hard irq int_abnormal_v1_hw(),
> > other acquisition of the same lock under process context should
> > disable irq, otherwise deadlock could happen if the
> > irq preempt the execution while the lock is held in process context
> > on the same CPU.
> >
> > [Interrupt]: int_abnormal_v1_hw()
> >      -->/root/linux/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c:1447
> >      -->/root/linux/drivers/scsi/hisi_sas/hisi_sas_main.c:2050
> >      -->/root/linux/drivers/scsi/hisi_sas/hisi_sas_main.c:1079
> >      -->spin_lock_irqsave(&hisi_hba->lock, flags);
> >
> > [Process Context]: hisi_sas_clear_nexus_ha()
> >      -->/root/linux/drivers/scsi/hisi_sas/hisi_sas_main.c:1932
> >      -->/root/linux/drivers/scsi/hisi_sas/hisi_sas_main.c:1135
> >      -->/root/linux/drivers/scsi/hisi_sas/hisi_sas_main.c:1116
> >      -->/root/linux/drivers/scsi/hisi_sas/hisi_sas_main.c:1105
> >      -->/root/linux/drivers/scsi/hisi_sas/hisi_sas_main.c:166
> >      -->spin_lock(&hisi_hba->lock);
> >
> > [Process Context]: hisi_sas_dev_found()
> >      -->/root/linux/drivers/scsi/hisi_sas/hisi_sas_main.c:665
> >      -->spin_lock(&hisi_hba->lock);
> >
> > [Process Context]: hisi_sas_queue_command()
> >      -->/root/linux/drivers/scsi/hisi_sas/hisi_sas_main.c:188
> >      -->spin_lock(&hisi_hba->lock);
> >
> > This flaw was found by an experimental static analysis tool I am
> > developing for irq-related deadlock, which reported the above
> > warning when analyzing the linux kernel 6.4-rc7 release.
> >
> > The tentative patch fix the potential deadlock by spin_lock_irqsave().
>
> Thank you for reporting it.
> But we consider about removing  hisi_hba->lock in function
> hisi_sas_port_notify_formed()
> which is called by int_abnormal_v1_hw(), as we think it is not necessary
> to add hisi_hba->lock in this function.
> So please ignore it and still thank you for pointing out the issue.
>
> Thanks,
> Shawn
>
> >
> > Signed-off-by: Chengfeng Ye <dg573847474@gmail.com>
> > ---
> >   drivers/scsi/hisi_sas/hisi_sas_main.c | 17 ++++++++++-------
> >   1 file changed, 10 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_=
sas/hisi_sas_main.c
> > index 412431c901a7..47c5062a732e 100644
> > --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
> > +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
> > @@ -161,11 +161,12 @@ static void hisi_sas_slot_index_clear(struct hisi=
_hba *hisi_hba, int slot_idx)
> >
> >   static void hisi_sas_slot_index_free(struct hisi_hba *hisi_hba, int s=
lot_idx)
> >   {
> > +     unsigned long flags;
> >       if (hisi_hba->hw->slot_index_alloc ||
> >           slot_idx < HISI_SAS_RESERVED_IPTT) {
> > -             spin_lock(&hisi_hba->lock);
> > +             spin_lock_irqsave(&hisi_hba->lock, flags);
> >               hisi_sas_slot_index_clear(hisi_hba, slot_idx);
> > -             spin_unlock(&hisi_hba->lock);
> > +             spin_unlock_irqrestore(&hisi_hba->lock, flags);
> >       }
> >   }
> >
> > @@ -181,11 +182,12 @@ static int hisi_sas_slot_index_alloc(struct hisi_=
hba *hisi_hba,
> >   {
> >       int index;
> >       void *bitmap =3D hisi_hba->slot_index_tags;
> > +     unsigned long flags;
> >
> >       if (rq)
> >               return rq->tag + HISI_SAS_RESERVED_IPTT;
> >
> > -     spin_lock(&hisi_hba->lock);
> > +     spin_lock_irqsave(&hisi_hba->lock, flags);
> >       index =3D find_next_zero_bit(bitmap, HISI_SAS_RESERVED_IPTT,
> >                                  hisi_hba->last_slot_index + 1);
> >       if (index >=3D HISI_SAS_RESERVED_IPTT) {
> > @@ -193,13 +195,13 @@ static int hisi_sas_slot_index_alloc(struct hisi_=
hba *hisi_hba,
> >                               HISI_SAS_RESERVED_IPTT,
> >                               0);
> >               if (index >=3D HISI_SAS_RESERVED_IPTT) {
> > -                     spin_unlock(&hisi_hba->lock);
> > +                     spin_unlock_irqrestore(&hisi_hba->lock, flags);
> >                       return -SAS_QUEUE_FULL;
> >               }
> >       }
> >       hisi_sas_slot_index_set(hisi_hba, index);
> >       hisi_hba->last_slot_index =3D index;
> > -     spin_unlock(&hisi_hba->lock);
> > +     spin_unlock_irqrestore(&hisi_hba->lock, flags);
> >
> >       return index;
> >   }
> > @@ -658,11 +660,12 @@ static struct hisi_sas_device *hisi_sas_alloc_dev=
(struct domain_device *device)
> >   {
> >       struct hisi_hba *hisi_hba =3D dev_to_hisi_hba(device);
> >       struct hisi_sas_device *sas_dev =3D NULL;
> > +     unsigned long flags;
> >       int last =3D hisi_hba->last_dev_id;
> >       int first =3D (hisi_hba->last_dev_id + 1) % HISI_SAS_MAX_DEVICES;
> >       int i;
> >
> > -     spin_lock(&hisi_hba->lock);
> > +     spin_lock_irqsave(&hisi_hba->lock, flags);
> >       for (i =3D first; i !=3D last; i %=3D HISI_SAS_MAX_DEVICES) {
> >               if (hisi_hba->devices[i].dev_type =3D=3D SAS_PHY_UNUSED) =
{
> >                       int queue =3D i % hisi_hba->queue_count;
> > @@ -682,7 +685,7 @@ static struct hisi_sas_device *hisi_sas_alloc_dev(s=
truct domain_device *device)
> >               i++;
> >       }
> >       hisi_hba->last_dev_id =3D i;
> > -     spin_unlock(&hisi_hba->lock);
> > +     spin_unlock_irqrestore(&hisi_hba->lock, flags);
> >
> >       return sas_dev;
> >   }
>