Re: [PATCH 05/11] NTB: ntb_transport: Avoid deadlock when cancelling link work

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Race condition in ntb_transport_free() leading to use-after-free if a link event fires during removal.
--

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

NTB: ntb_transport: Avoid deadlock when cancelling link work

This commit prevents a deadlock in ntb_transport_link_cleanup by
conditionally cancelling link work outside of the link_event_lock.

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -973,6 +973,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]
This isn't a bug introduced by this patch, but while reviewing this work
cancellation, I noticed a potential race condition in ntb_transport_free()
that could lead to a use-after-free.

In ntb_transport_free(), the background works are cancelled, but this happens
before the hardware is stopped from generating link events via
ntb_link_disable() and ntb_clear_ctx():

	ntb_transport_link_cleanup(nt);
	cancel_work_sync(&nt->link_cleanup);
	cancel_delayed_work_sync(&nt->link_work);

	qp_bitmap_alloc = nt->qp_bitmap & ~nt->qp_bitmap_free;

	/* verify that all the qp's are freed */
	for (i = 0; i < nt->qp_count; i++) {
		qp = &nt->qp_vec[i];
		if (qp_bitmap_alloc & BIT_ULL(i))
			ntb_transport_free_queue(qp);
	}

	ntb_link_disable(ndev);
	ntb_clear_ctx(ndev);

	ntb_bus_remove(nt);

	for (i = nt->mw_count; i--; ) {
		ntb_free_mw(nt, i);
		iounmap(nt->mw_vec[i].vbase);
	}

	kfree(nt->qp_vec);
	kfree(nt->mw_vec);
	kfree(nt);

If a hardware interrupt triggers a link event during this window, can the
callback ntb_transport_event_callback() reschedule the just-cancelled works?

If so, would ntb_transport_free() then proceed to free the nt context via
kfree(nt), leaving the requeued work to execute against freed memory?

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