Re: [PATCH v22 net-next 04/12] net/nebula-matrix: add channel layer

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
--- Patch [4]: [PATCH v22 net-next 04/12] net/nebula-matrix: add channel layer ---
commit 52547754390ebdc264a4b6a5c891b4ecfbcd56a7
Author: illusion wang <illusion.wang@xxxxxxxxxxxxxxxxx>

net/nebula-matrix: add channel layer

This patch introduces a channel management layer for the Nebula Matrix
driver to handle communication between different components and drivers.
It adds support for mailbox channels, queue initialization, hardware
abstraction, and interrupt-driven receive paths with waitqueues for
transmission.

> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_channel/nbl_channel.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_channel/nbl_channel.c
> new file mode 100644
> index 0000000000000..85218f826f59d
> --- /dev/null
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_channel/nbl_channel.c
[ ... ]
> +static void nbl_chan_clean_queue(struct nbl_channel_mgt *chan_mgt,
> +				 struct nbl_chan_info *chan_info)
> +{
[ ... ]
> +	rxq->next_to_clean = next_to_clean;
> +	/* add READ_ONCE check to avoid UAF when clean_task is null*/
> +	if (more_work && READ_ONCE(chan_info->clean_task))
> +		schedule_work(chan_info->clean_task);
> +}

[Severity: High]
Can this result in a NULL pointer dereference if chan_info->clean_task is
cleared concurrently?

When calling schedule_work(chan_info->clean_task), the compiler fetches the
pointer from memory a second time. If nbl_chan_teardown_queue() sets
chan_info->clean_task = NULL right after the READ_ONCE() check but before
the schedule_work() call, wouldn't a NULL pointer be passed to the workqueue
API? Could a local variable be used to cache the pointer to avoid this?

[ ... ]
> +static int nbl_chan_teardown_queue(struct nbl_channel_mgt *chan_mgt,
> +				   u8 chan_type)
> +{
> +	struct nbl_chan_info *chan_info = chan_mgt->chan_info[chan_type];
> +	struct nbl_chan_waitqueue_head *wait_head;
> +	u16 i;
> +
> +	/* Step 1: Mark shutdown flag, reject all new send requests */
> +	WRITE_ONCE(chan_info->shutdown, true);
> +
> +	/* Stop hardware queues */
> +	nbl_chan_stop_queue(chan_mgt);
> +
> +	/* Cancel any pending cleanup work */
> +	if (chan_info->clean_task) {
> +		cancel_work_sync(chan_info->clean_task);
> +		chan_info->clean_task = NULL;
> +	}
> +	for (i = 0; i < chan_info->num_txq_entries; i++) {
[ ... ]
> +	}
> +
> +	/* Step 2: Wait all in-flight send_msg threads exit via counter */
> +	while (atomic_read(&chan_info->inflight_tx_cnt) != 0) {
> +		/* synchronize atomic counter load with other CPUs */
> +		smp_rmb();
> +		usleep_range(100, 1000);
> +	}
> +	return 0;
> +}

[Severity: High]
Is cancel_work_sync() called too early here?

Since cancel_work_sync() is called before waiting for inflight_tx_cnt to
drain, could an in-flight sender thread currently executing its polling loop
call queue_work() concurrently after the cancellation is finished? This
might leave pending work in the queue that executes after the device has been
torn down, causing a use-after-free regression.

[ ... ]
> +static int nbl_chan_send_msg(struct nbl_channel_mgt *chan_mgt,
> +			     struct nbl_chan_send_info *chan_send)
> +{
[ ... ]
> +	/* Reject new send if shutdown already triggered */
> +	if (READ_ONCE(chan_info->shutdown))
> +		return -ESHUTDOWN;
> +	/* Ensure shutdown load is ordered before inflight counter increment */
> +	smp_rmb();
> +	atomic_inc(&chan_info->inflight_tx_cnt);

[Severity: High]
Does this sequence create a TOCTOU race with nbl_chan_teardown_queue()?

If a sender thread checks READ_ONCE(chan_info->shutdown) and it is false,
but the thread is then preempted before incrementing inflight_tx_cnt,
could nbl_chan_teardown_queue() set shutdown = true, observe the counter
is 0, and proceed to free resources? When the sender resumes, wouldn't it
increment the counter and access destroyed queues?

[ ... ]
> +	if (test_bit(NBL_CHAN_INTERRUPT_READY, chan_info->state)) {
> +		spin_lock_irq(&wait_head->status_lock);
> +		while (!READ_ONCE(wait_head->acked)) {
> +			spin_unlock_irq(&wait_head->status_lock);
> +			ret = wait_event_timeout(wait_head->wait_queue,
> +						 READ_ONCE(wait_head->acked),
> +						 NBL_CHAN_ACK_WAIT_TIME);
> +			spin_lock_irq(&wait_head->status_lock);
> +
> +			if (READ_ONCE(chan_info->shutdown)) {
> +				ret = -ESHUTDOWN;
> +				goto inflight_dec_out;
> +			}

[Severity: Critical]
Does this error path leak the spinlock and leave interrupts disabled?

Since spin_lock_irq(&wait_head->status_lock) was called right before this
check, and the inflight_dec_out label simply decrements the counter and
returns, shouldn't spin_unlock_irq() be called before taking this goto?

> +			if (ret == 0) {
> +				if (READ_ONCE(wait_head->status) ==
> +				    NBL_MBX_STATUS_WAITING) {
> +					wait_head->status =
> +						NBL_MBX_STATUS_TIMEOUT;
> +					wait_head->acked = 0;
> +					wait_head->ack_data = NULL;
> +					wait_head->ack_data_len = 0;
> +				}
> +				spin_unlock_irq(&wait_head->status_lock);
> +				dev_err(dev,
> +					"Channel waiting ack failed, message type: %d, msg id: %u\n",
> +					chan_send->msg_type, msgid);
> +				goto inflight_dec_out;
> +			}

[Severity: High]
When wait_event_timeout() times out, it returns 0. If this path is taken,
will the function incorrectly return 0 (success) instead of -ETIMEDOUT to
the caller? This might lead callers to believe the transmission succeeded and
read uninitialized response buffers.

[ ... ]
> +	/* Polling: only init/deinit, add shutdown detect to avoid deadlock */
> +	while (i--) {
> +		if (READ_ONCE(chan_info->shutdown)) {
> +			ret = -ESHUTDOWN;
> +			goto inflight_dec_out;
> +		}
> +		/* Order shutdown read before accessing clean_task/workqueue */
> +		smp_rmb();
> +		if (READ_ONCE(chan_info->clean_task) && common->wq)
> +			queue_work(common->wq, chan_info->clean_task);

[Severity: High]
Could this suffer from the same double-read TOCTOU race as in
nbl_chan_clean_queue()?

If chan_info->clean_task is set to NULL after the READ_ONCE() check but
before queue_work() evaluates its arguments, could this pass a NULL pointer
to the workqueue core?