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: <20230628045925.5261-1-dg573847474@gmail.com> <20230704114849.GA6455@unreal>
In-Reply-To: <20230704114849.GA6455@unreal>
From:   Chengfeng Ye <dg573847474@gmail.com>
Date:   Wed, 5 Jul 2023 01:42:31 +0800
Message-ID: <CAAo+4rXkMM87OJzim=8dACdV=kWK_1yXeD=W5GZzHoJ2Gz6rtw@mail.gmail.com>
Subject: Re: [PATCH] IB/hfi1: Fix potential deadlock on &sde->flushlist_lock
To:     Leon Romanovsky <leon@kernel.org>
Cc:     dennis.dalessandro@cornelisnetworks.com, jgg@ziepe.ca,
        linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

> Plus, we already in context where interrupts are stopped.

Indeed they can be called from .ndo_start_xmit callback and
the document said it is with bh disabled.

But I found some call chain from the user process that seems could
be called from irq disabled context. For sdma_send_txlist(),
there is a call chain.

-> hfi1_write_iter()  (.write_iter callback)
-> hfi1_user_sdma_process_request()
-> user_sdma_send_pkts()
-> sdma_send_txlist()

The .write_iter seems not to disable irq by default, as mentioned by
https://www.kernel.org/doc/Documentation/filesystems/vfs.txt
And I didn't find any explicit disabling or bh or irq along the call path,
and also see several  copy_from_usr() which cannot be invoked under
irq context.


For sdma_send_txreq(), there is a call chain.

-> qp_priv_alloc()
-> iowait_init() (register _hfi1_do_tid_send() as a work queue)
-> _hfi1_do_tid_send() (workqueue)
-> hfi1_do_tid_send()
-> hfi1_verbs_send()
-> sr(qp, ps, 0) (sr could points to hfi1_verbs_send_dm())
-> hfi1_verbs_send_dma()
-> sdma_send_txreq()

_hfi1_do_tid_send() is a work queue without irq disabled by default,
I also check the remaining call path and also found that there is no explic=
it
irq disable, instead the call site of hfi1_verbs_send() is exactly after
spin_lock_irq_restore(), seems like a hint that it is probably called withi=
rq
enable.

Another hint is that the lock acquisition of
spin_lock_irqsave(&sde->tail_lock, flags);
just before my patch in the same function also disable irq, seems like anot=
her
hint that this function could be called with interrupt disable,
otherwise the lock/unlock
for sde->tail_lock does not need to disable irq?

Would be appreciated if you could further check with this.


> It can't work as exactly after "ret =3D -ECOMM;" line, there is "goto unl=
ock"
> and there hfi1 calls to spin_unlock_irqrestore(..) with same "flags".

Yeah, that's my negligence, and sorry for this. Once you confirm that
there should
be some fixes, I would like to provide with v2 patch with the correct fix.

Best Regards,
Chengfeng


Leon Romanovsky <leon@kernel.org> =E4=BA=8E2023=E5=B9=B47=E6=9C=884=E6=97=
=A5=E5=91=A8=E4=BA=8C 19:48=E5=86=99=E9=81=93=EF=BC=9A
>
> On Wed, Jun 28, 2023 at 04:59:25AM +0000, Chengfeng Ye wrote:
> > As &sde->flushlist_lock is acquired by timer sdma_err_progress_check()
> > through layer of calls under softirq context, other process
> > context code acquiring the lock should disable irq.
> >
> > Possible deadlock scenario
> > sdma_send_txreq()
> >     -> spin_lock(&sde->flushlist_lock)
> >         <timer interrupt>
> >         -> sdma_err_progress_check()
> >         -> __sdma_process_event()
> >         -> sdma_set_state()
> >         -> sdma_flush()
> >         -> spin_lock_irqsave(&sde->flushlist_lock, flags) (deadlock her=
e)
> >
> > This flaw was found using an experimental static analysis tool we are
> > developing for irq-related deadlock.
> >
> > The tentative patch fix the potential deadlock by spin_lock_irqsave().
> >
> > Signed-off-by: Chengfeng Ye <dg573847474@gmail.com>
> > ---
> >  drivers/infiniband/hw/hfi1/sdma.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/infiniband/hw/hfi1/sdma.c b/drivers/infiniband/hw/=
hfi1/sdma.c
> > index bb2552dd29c1..0431f575c861 100644
> > --- a/drivers/infiniband/hw/hfi1/sdma.c
> > +++ b/drivers/infiniband/hw/hfi1/sdma.c
> > @@ -2371,9 +2371,9 @@ int sdma_send_txreq(struct sdma_engine *sde,
> >       tx->sn =3D sde->tail_sn++;
> >       trace_hfi1_sdma_in_sn(sde, tx->sn);
> >  #endif
> > -     spin_lock(&sde->flushlist_lock);
> > +     spin_lock_irqsave(&sde->flushlist_lock, flags);
> >       list_add_tail(&tx->list, &sde->flushlist);
> > -     spin_unlock(&sde->flushlist_lock);
> > +     spin_unlock_irqrestore(&sde->flushlist_lock, flags);
> >       iowait_inc_wait_count(wait, tx->num_desc);
> >       queue_work_on(sde->cpu, system_highpri_wq, &sde->flush_worker);
> >       ret =3D -ECOMM;
>
> It can't work as exactly after "ret =3D -ECOMM;" line, there is "goto unl=
ock"
> and there hfi1 calls to spin_unlock_irqrestore(..) with same "flags".
>
> Plus, we already in context where interrupts are stopped.
>
> Thanks
>
> > @@ -2459,7 +2459,7 @@ int sdma_send_txlist(struct sdma_engine *sde, str=
uct iowait_work *wait,
> >       *count_out =3D total_count;
> >       return ret;
> >  unlock_noconn:
> > -     spin_lock(&sde->flushlist_lock);
> > +     spin_lock_irqsave(&sde->flushlist_lock, flags);
> >       list_for_each_entry_safe(tx, tx_next, tx_list, list) {
> >               tx->wait =3D iowait_ioww_to_iow(wait);
> >               list_del_init(&tx->list);
> > @@ -2472,7 +2472,7 @@ int sdma_send_txlist(struct sdma_engine *sde, str=
uct iowait_work *wait,
> >               flush_count++;
> >               iowait_inc_wait_count(wait, tx->num_desc);
> >       }
> > -     spin_unlock(&sde->flushlist_lock);
> > +     spin_unlock_irqrestore(&sde->flushlist_lock, flags);
> >       queue_work_on(sde->cpu, system_highpri_wq, &sde->flush_worker);
> >       ret =3D -ECOMM;
> >       goto update_tail;
> > --
> > 2.17.1
> >