Re: [PATCH v22 net-next 04/12] net/nebula-matrix: add channel layer
flat view
From: Jakub Kicinski <kuba@kernel.org>
Date: 2026-07-31 01:30:29
Also in:
linux-doc, lkml
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
+static void nbl_chan_clean_queue(struct nbl_channel_mgt *chan_mgt, + struct nbl_chan_info *chan_info) +{
[ ... ]
quoted hunk ↗ jump to hunk
+ 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? [ ... ]
quoted hunk ↗ jump to hunk
+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++) {
[ ... ]
quoted hunk ↗ jump to hunk
+ } + + /* 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. [ ... ]
quoted hunk ↗ jump to hunk
+static int nbl_chan_send_msg(struct nbl_channel_mgt *chan_mgt, + struct nbl_chan_send_info *chan_send) +{
[ ... ]
quoted hunk ↗ jump to hunk
+ /* 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? [ ... ]
quoted hunk ↗ jump to hunk
+ 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?
quoted hunk ↗ jump to hunk
+ 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. [ ... ]
quoted hunk ↗ jump to hunk
+ /* 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?