Re: [PATCH 06/11] NTB: ntb_transport: Publish link state after QP setup

[sashiko-bot@kernel.org]

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

New issues:
- [High] Store-Load memory ordering race between `nt->link_is_up` and `qp->client_ready` allows both threads to miss link scheduling, potentially deadlocking link establishment.

Pre-existing issues:
- [High] Workqueue teardown race leading to Use-After-Free during `ntb_transport_free()`.
- [High] Missing minimum size validation for memory windows causes an integer underflow when calculating tx_size and rx_size, leading to massive out-of-bounds MMIO writes and kernel heap memory corruption.
--

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

NTB: ntb_transport: Publish link state after QP setup

This commit delays publishing the transport link state until all QPs' MW and
peer MSI states are set up. It uses a release store for link state publication
and acquire loads before queuing QP link work to avoid premature execution.

Link: https://lore.kernel.org/r/anyKbq3mpLG4y7rb@SMW015318

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index c8e7fc774b4fd..411017873a83b 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c

[ ... ]

> @@ -1102,16 +1109,19 @@ static void ntb_transport_link_work(struct work_struct *work)
>  			goto out1;
>  	}
>  
>  	for (i = 0; i < nt->qp_count; i++) {
>  		ntb_transport_setup_qp_mw(nt, i);
>  		ntb_transport_setup_qp_peer_msi(nt, i);
> +	}
> +
> +	/* Publish the link only after every QP has been set up. */
> +	smp_store_release(&nt->link_is_up, true);
> +
> +	for (i = 0; i < nt->qp_count; i++) {
> +		struct ntb_transport_qp *qp = &nt->qp_vec[i];
>  
>  		if (qp->client_ready)
> -			schedule_delayed_work(&qp->link_work, 0);
> +			ntb_transport_schedule_qp_link(qp, 0);

[Severity: High]
Does this code have a Store-Load memory ordering race that could lead to a
deadlock?

In this thread, ntb_transport_link_work() performs a release store to
nt->link_is_up, followed by a plain load of qp->client_ready.

Meanwhile, ntb_transport_link_up() performs a plain store to qp->client_ready,
followed by an acquire load of nt->link_is_up via
ntb_transport_schedule_qp_link():

drivers/ntb/ntb_transport.c:ntb_transport_link_up() {
    ...
    qp->client_ready = true;
    ntb_transport_schedule_qp_link(qp, 0);
    ...
}

Since neither smp_store_release() nor smp_load_acquire() provides a full
memory barrier (Store-Load ordering), is it possible for both threads to delay
their stores in the CPU store buffer? If this happens, both threads could read
false and miss the wakeup, leaving the link permanently inactive.

[ ... ]

[Severity: High]
This is a pre-existing issue, but is there a use-after-free race during
workqueue teardown?

Looking at ntb_transport_free(), the delayed work is canceled before the
hardware events are neutralized:

drivers/ntb/ntb_transport.c:ntb_transport_free() {
    ...
    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);
    ...
}

If a hardware link event fires between cancel_delayed_work_sync() and
ntb_clear_ctx(), the interrupt handler could requeue the work item. After
ntb_transport_free() finishes freeing the context, couldn't the requeued work
execute and dereference the freed memory?

[Severity: High]
This is a pre-existing issue, but does this code lack minimum size validation
for memory windows, leading to a potential integer underflow?

In ntb_transport_init_queue(), the tx_size calculation unconditionally subtracts
the size of the struct:

drivers/ntb/ntb_transport.c:ntb_transport_init_queue() {
    ...
    tx_size -= sizeof(struct ntb_rx_info);
    ...
}

Similarly, in ntb_transport_setup_qp_mw():

drivers/ntb/ntb_transport.c:ntb_transport_setup_qp_mw() {
    ...
    rx_size -= sizeof(struct ntb_rx_info);
    ...
}

If tx_size or rx_size is less than 4 bytes, this will underflow. For tx_size,
setting the max_mw_size module parameter to a small value could trigger an
underflow. For rx_size, if a remote peer advertises a window size that
truncates to 0, could it underflow and lead to out-of-bounds MMIO writes
or memory corruption?

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