Re: [PATCH v2 03/14] NTB: ntb_transport: Avoid deadlock when cancelling link work

[Koichiro Den]

On Fri, Sep 11, 2026 at 11:21:43AM -0500, Frank Li wrote:
> On Thu, Sep 10, 2026 at 01:08:25PM +0900, Koichiro Den wrote:
> > During initial link setup, ntb_transport_link_work() can retry with
> > nt->link_is_up still false. A retry can block on link_event_lock
> > while cleanup holds it and waits in cancel_delayed_work_sync(),
> > leading to deadlock.
> >
> > Move the conditional cancellation outside link_event_lock, before
> > QP cleanup. Keep QP cleanup and MW release under the lock so link
> > work cannot restart QPs between them. Put the locking in
> > ntb_transport_link_cleanup() to cover both worker and remove paths.
> >
> > Fixes: 3db835dd8f9a ("ntb: Add mutex to make link_event_callback executed linearly.")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Koichiro Den <den@xxxxxxxxxxxxx>
> > ---
> > Changes in v2:
> >   - No changes.
> >
> >  drivers/ntb/ntb_transport.c | 13 +++++++++----
> >  1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> > index 55a20ae9a85e..c77b173dca01 100644
> > --- a/drivers/ntb/ntb_transport.c
> > +++ b/drivers/ntb/ntb_transport.c
> > @@ -960,6 +960,15 @@ static void ntb_transport_link_cleanup(struct ntb_transport_ctx *nt)
> >  	struct ntb_transport_qp *qp;
> >  	u64 qp_bitmap_alloc;
> >  	unsigned int i, count;
> > +	bool cancel_link_work;
> > +
> > +	scoped_guard(mutex, &nt->link_event_lock)
> > +		cancel_link_work = !nt->link_is_up;
> > +
> > +	if (cancel_link_work)
> > +		cancel_delayed_work_sync(&nt->link_work);
> 
> what happen if schedule new work here?

Both link_work and link_cleanup can race to take link_event_lock.

- If link_work wins and completes setup, cleanup can tear the link down again.
  The old code also allowed setup to run before a pending cleanup though. In
  that sense, although Sashiko flagged it as a new issue, I believe the ordering
  issue itself is pre-existing.
- If link_cleanup wins, link_work just waits for cleanup to finish before
  starting setup.

This patch fixes the deadlock, not the event ordering. I would prefer to handle
the ordering issue separately.

Best regards,
Koichiro

> 
> Frank
> > +
> > +	guard(mutex)(&nt->link_event_lock);
> >
> >  	qp_bitmap_alloc = nt->qp_bitmap & ~nt->qp_bitmap_free;
> >
> > @@ -972,9 +981,6 @@ static void ntb_transport_link_cleanup(struct ntb_transport_ctx *nt)
> >  			cancel_delayed_work_sync(&qp->link_work);
> >  		}
> >
> > -	if (!nt->link_is_up)
> > -		cancel_delayed_work_sync(&nt->link_work);
> > -
> >  	for (i = 0; i < nt->mw_count; i++)
> >  		ntb_free_mw(nt, i);
> >
> > @@ -992,7 +998,6 @@ static void ntb_transport_link_cleanup_work(struct work_struct *work)
> >  	struct ntb_transport_ctx *nt =
> >  		container_of(work, struct ntb_transport_ctx, link_cleanup);
> >
> > -	guard(mutex)(&nt->link_event_lock);
> >  	ntb_transport_link_cleanup(nt);
> >  }
> >
> > --
> > 2.51.0
> >