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 here)
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 <[email protected]>
---
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 = 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 = -ECOMM;
@@ -2459,7 +2459,7 @@ int sdma_send_txlist(struct sdma_engine *sde, struct iowait_work *wait,
*count_out = 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 = iowait_ioww_to_iow(wait);
list_del_init(&tx->list);
@@ -2472,7 +2472,7 @@ int sdma_send_txlist(struct sdma_engine *sde, struct 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 = -ECOMM;
goto update_tail;
--
2.17.1
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 here)
>
> 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 <[email protected]>
> ---
> 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 = 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 = -ECOMM;
It can't work as exactly after "ret = -ECOMM;" line, there is "goto unlock"
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, struct iowait_work *wait,
> *count_out = 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 = iowait_ioww_to_iow(wait);
> list_del_init(&tx->list);
> @@ -2472,7 +2472,7 @@ int sdma_send_txlist(struct sdma_engine *sde, struct 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 = -ECOMM;
> goto update_tail;
> --
> 2.17.1
>
> 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 explicit
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 withirq
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 another
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 = -ECOMM;" line, there is "goto unlock"
> 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 <[email protected]> 于2023年7月4日周二 19:48写道：
>
> 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 here)
> >
> > 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 <[email protected]>
> > ---
> > 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 = 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 = -ECOMM;
>
> It can't work as exactly after "ret = -ECOMM;" line, there is "goto unlock"
> 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, struct iowait_work *wait,
> > *count_out = 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 = iowait_ioww_to_iow(wait);
> > list_del_init(&tx->list);
> > @@ -2472,7 +2472,7 @@ int sdma_send_txlist(struct sdma_engine *sde, struct 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 = -ECOMM;
> > goto update_tail;
> > --
> > 2.17.1
> >
On Wed, Jul 05, 2023 at 01:42:31AM +0800, Chengfeng Ye wrote:
> > 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 explicit
> 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 withirq
> enable.
Right, that path is called in process context and can sleep, there is no
need in irq disabled variant there.
>
> 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 another
> hint that this function could be called with interrupt disable,
Exactly, we already called to spin_lock_irqsave(), there is no value in
doing it twice.
void f() {
spin_lock_irqsave(...)
spin_lock_irqsave(...)
....
spin_unlock_irqrestore(...)
spin_unlock_irqrestore(...)
}
is exactly the same as
void f() {
spin_lock_irqsave(...)
spin_lock(...)
....
spin_unlock(...)
spin_unlock_irqrestore(...)
}
Thanks
> Exactly, we already called to spin_lock_irqsave(), there is no value in
> doing it twice.
Oh yeah, I just notice that the lock acquisition of &sde->flushlist_lock
is always nested inside &sde->tail_lock due to the goto. Then it is true
that no need for irq invariant lock/unlock on &sde->flushlist_lock.
Thanks much for your reply and your time.
Best Regards,
Chengfeng
On 7/5/23 2:47 AM, Chengfeng Ye wrote:
>> Exactly, we already called to spin_lock_irqsave(), there is no value in
>> doing it twice.
>
> Oh yeah, I just notice that the lock acquisition of &sde->flushlist_lock
> is always nested inside &sde->tail_lock due to the goto. Then it is true
> that no need for irq invariant lock/unlock on &sde->flushlist_lock.
>
> Thanks much for your reply and your time.
Agree. Thanks Leon for looking at this. I was out of the office and just now
seen it.
-Denny