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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Dropping `link_event_lock` around `cancel_delayed_work_sync()` introduces a race condition that can permanently tear down a successfully established link.

Pre-existing issues:
- [High] Separate work items for link-up and link-down events can execute out of order, leading to a permanent link failure.
--

commit d67dbc659b034b7a703fc35d72b81082c61a2ecd
Author: Koichiro Den <den@valinux.co.jp>

NTB: ntb_transport: Avoid deadlock when cancelling link work

This commit changes ntb_transport_link_cleanup() to cancel the link_work
delayed work item outside of the link_event_lock. This avoids a deadlock
where a retry in link_work blocks on the lock held by the cleanup function.

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index 55a20ae9a85e6..c77b173dca01f 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);
> +
> +	guard(mutex)(&nt->link_event_lock);

[Severity: High]
Does dropping the link_event_lock around cancel_delayed_work_sync() create a
race condition that could permanently tear down the link?

If a hardware link flap occurs, ntb_transport_event_callback() can queue both
link_work and link_cleanup. If link_cleanup executes, it evaluates
!nt->link_is_up under the scoped_guard, drops the lock, and calls
cancel_delayed_work_sync().

During this unlocked window, a pending link_work can acquire the lock,
successfully establish the NTB link, set nt->link_is_up = true, and complete.

When link_cleanup reacquires the lock, it unconditionally destroys the QPs
and MWs that were just successfully established, without re-evaluating the
link state.

Can we re-check the link state after reacquiring the lock to prevent this
permanent link failure?

[ ... ]

As an additional observation regarding the event handling:

drivers/ntb/ntb_transport.c:ntb_transport_event_callback() {
...
	if (ntb_link_is_up(nt->ndev, NULL, NULL) == 1)
		schedule_delayed_work(&nt->link_work, 0);
	else
		schedule_work(&nt->link_cleanup);
...
}

[Severity: High]
This isn't a bug introduced by this patch, but does using separate work items
for link-up and link-down events allow them to execute out of order?

Since link_work and link_cleanup are queued to the global workqueue as
distinct work items, rapid hardware flapping (DOWN then UP) can queue both
simultaneously.

If they execute out of order, or if link_cleanup executes last and cancels an
un-run link_work, the software state could be torn down permanently even
though the physical hardware state is UP.

Could a single work item or a state machine handle both events to enforce
ordering?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910040836.3792333-1-den@valinux.co.jp?part=3