Re: [PATCH net-next v4 04/10] NTB: ntb_transport: Keep local QP link requests separate

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] A cleared QP up-request can be re-asserted by the QP's own
  link_work, leaving a closed QP advertised as 'up' in the peer's…

--

NTB: ntb_transport: Keep local QP link requests separate

This moves the local QP up-request bitmap out of the local scratchpad into
a new nt->up_request field, serializing the read-modify-write and the peer
QP_LINKS write under a new nt->up_request_lock. The bitmap is cleared in
ntb_transport_link_cleanup() so each QP re-advertises after reconnecting.

[ ... ]

[Severity: Medium]
The up/down decision is still made by the caller outside up_request_lock,
so the last writer wins.  Can a closed QP end up advertised as up again
here?

ntb_qp_link_work() calls ntb_qp_up_request(qp, true) unconditionally, with
no test of qp->client_ready, while ntb_transport_link_down() clears the bit
first and only cancels the worker in the qp->link_is_up == false branch:

drivers/ntb/ntb_transport.c:ntb_transport_link_down() {
	qp->client_ready = false;

	ntb_qp_up_request(qp, false);

	if (qp->link_is_up)
		ntb_send_link_down(qp);
	else
		cancel_delayed_work_sync(&qp->link_work);
}

So with qp->link_is_up true, a queued or in-flight qp->link_work can run
after the clear and re-set BIT(qp->qp_num) in nt->up_request, republishing
it to the peer's QP_LINKS.

There is also a repeating path through the cleanup worker:

drivers/ntb/ntb_transport.c:ntb_qp_link_cleanup_work() {
	ntb_qp_link_cleanup(qp);

	if (nt->link_is_up)
		schedule_delayed_work(&qp->link_work,
				      msecs_to_jiffies(NTB_LINK_DOWN_TIMEOUT));
}

nt->link_is_up is set false only in ntb_transport_probe(), so after the
peer's LINK_DOWN payload is consumed (ntb_process_rxc() -> ntb_qp_link_down()
-> schedule_work(&qp->link_cleanup)) the QP's link_work keeps re-arming every
NTB_LINK_DOWN_TIMEOUT and keeps re-advertising a QP whose client already went
down.  Once that happens, is there any remaining publisher that would clear
the bit again short of a transport-wide cleanup?

The peer would then declare its side of the QP up and transmit into a queue
whose client is closed, and those frames get dropped.

Would it be more robust to exclude qp->link_work unconditionally (cancel or
flush before clearing), or to evaluate qp->client_ready inside
ntb_qp_up_request() under up_request_lock, rather than only making the
read-modify-write atomic?

For reference, the later patch in this series ("NTB: ntb_transport: Exchange
client capabilities at link-up") adds

	if (!smp_load_acquire(&qp->client_ready))
		return;

before the ntb_qp_up_request(qp, true) call and moves the
cancel_delayed_work_sync() ahead of the clear, which removes the repeating
cleanup-worker path.  A worker that already passed that client_ready load
can still publish the bit after ntb_transport_link_down() cleared it when
qp->link_is_up is true, since that branch skips the cancel.

 	/* query remote spad for qp ready bits */
 	dev_dbg_ratelimited(&pdev->dev, "Remote QP link status = %x\n", val);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914084838.2158249-1-den%40valinux.co.jp