Re: [PATCH net-next 4/8] eth: mpnic: implement Tx queue allocation and cleanup

["Daniel Zahka" <daniel.zahka@gmail.com>]

On Wed Sep 23, 2026 at 10:05 PM EDT, netdev-bot+sashiko wrote:
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
>
> Low priority / nit picks for your consideration:
>
> - [Low] Cached doorbell pointers let queue-head MMIO writes reach BAR0
>   after the driver has marked the device as gone.
> - [Low] Concern (baseline: rejected): mpnic_alloc_ring_desc() stores
>   `count - 1` into the u16 ring->size_mask (line 334) without checking…
> - [Low] Concern (baseline: rejected): mpnic_clean_tcq() takes HEAD0 from
>   the device-written completion descriptor (line 163) and passes it…
>
> --
>
> eth: mpnic: implement Tx queue allocation and cleanup
>
> This adds the Tx ring structures and allocates queue triads per NAPI
> vector, along with their descriptor memory and interrupts. It also adds
> Tx completion processing, where one completion reports a work queue head
> and can release many packets. Nothing enables the queues in hardware yet.
>
>> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>> new file mode 100644
>> index 0000000000000..fe360a26a27be
>> --- /dev/null
>> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>> @@ -0,0 +1,496 @@
>
> [ ... ]
>
>> +static void mpnic_commit_cq_head(struct mpnic_ring *cmpl)
>> +{
>> +	u32 head = cmpl->head;
>> +
>> +	/* The tail shadows the last value written to the doorbell, so a
>> +	 * completion queue which has not moved costs no MMIO write.
>> +	 */
>> +	if (cmpl->tail != head) {
>> +		cmpl->tail = head;
>> +		writeq(head & cmpl->size_mask, cmpl->doorbell);
>> +	}
>> +}
>
> [Severity: Low]
> Can this doorbell write still reach BAR0 after the driver has marked the
> device as gone?
>
> mpnic_alloc_napi_vector() reads mpd->uc_addr0 once and stores raw BAR0
> addresses in the rings:
>
>     mpnic_ring_init(&nv->qt[0].sub0, &uc_addr[MPNIC_TWQ_TAIL(idx, 0)], idx);
>     mpnic_ring_init(&nv->qt[0].cmpl, &uc_addr[MPNIC_TCQ_HEAD(idx)], idx);
>
> When mpnic_rd64() sees all-ones reads, mpnic_mmio_err() in mpnic_pci.c
> clears only the accessor pointer:
>
>     WRITE_ONCE(mpd->uc_addr0, NULL);
>
> BAR0 stays mapped through pcim_iomap_region(). mpnic_wr64() checks for
> the NULL pointer, but mpnic_commit_cq_head() always writes through
> cmpl->doorbell.
>
> So suppose one mpnic_poll() run still has unprocessed completions that
> were posted before the failure. The TCQ head doorbell write goes out,
> while the mpnic_nv_irq_rearm() write through mpnic_wr64() is dropped.
>
> The scaffolding commit says that once the device is gone, no further
> accesses reach the bus. Should that also cover the doorbells?
>
> Later in the series, mpnic_tx_doorbell() and __mpnic_bdq_commit_tail()
> use the same cached-pointer pattern. Would a mpnic_present() check help
> in those places and here?
>
> The comment in mpnic_alloc_napi_vector() already says the doorbells
> cannot notice the window going away, and fbnic does the same thing. The
> practical effect is probably limited to posted writes to a dead link.
>

The observation is correct, but this is by design. Won't fix. If we lose
the device, we can let these writes get dropped by the bus instead of
trying to stop them.

>> +static void mpnic_clean_tcq(struct mpnic_napi_vector *nv,
>> +			    struct mpnic_q_triad *qt, int napi_budget)
>> +{
>
> [ ... ]
>
>> +	while ((*raw_tcd & cpu_to_le64(MPNIC_TCD_DONE)) == done) {
>> +		u64 tcd;
>> +
>> +		dma_rmb();
>> +
>> +		tcd = le64_to_cpu(*raw_tcd);
>> +		head0 = FIELD_GET(MPNIC_TCD_TYPE0_HEAD0, tcd);
>
> [ ... ]
>
>> +	cmpl->head = head;
>> +
>> +	if (head0 >= 0)
>> +		mpnic_clean_twq0(nv, napi_budget, &qt->sub0, false, head0);
>> +}
>
> [Severity: Low]
> This isn't a bug with a conforming device, but head0 comes straight from
> the completion descriptor the device writes. It goes to
> mpnic_clean_twq0() without any check against sub0.tail.
>

In this case, and in general, we do not perform this type of hardening
against completion fields written by the device. We trust its outputs
for things like offsets into buffers, rings, etc.

> If the device ever reported a head past the posted tail,
> mpnic_clean_twq0() would reach an empty slot and dereference NULL:
>
>     struct sk_buff *skb = ring->tx_buf[head];
>     ...
>     desc_cnt = MPNIC_XMIT_CB(skb)->desc_count;
>
> The completion walk loop is also not bounded by the NAPI budget. In
> practice the device cannot post past the CQ head, and that head is only
> committed after the loop.
>
> Stale entries from an earlier bringup do not look reachable. The
> descriptor memory is freshly zeroed by dma_alloc_coherent(), the head
> and tail are reset to 0 later in the series, and at head 0 the expected
> DONE bit is 1.
>
> Would a cheap sanity check on head0, or a NULL check on skb, be worth
> adding as hardening?
>
> [ ... ]
>
>> +static int mpnic_alloc_ring_desc(struct mpnic_net *mpn,
>> +				 struct mpnic_ring *ring, u32 count)
>> +{
>
> [ ... ]
>
>> +	ring->size_mask = count - 1;
>> +	ring->size = size;
>
> [Severity: Low]
> This isn't a bug with the current callers, but count is a u32 while
> ring->size_mask is a u16 in struct mpnic_ring. Nothing here checks that
> count is a non-zero power of two no larger than 65536.
>

Ack, but won't fix. When ethool is implemented, caller will have the
responsibility of ensuring these.

> This cannot be reached today. The only caller passes mpn->txq_size,
> which is only ever set to MPNIC_TXQ_SIZE_DEFAULT (1024), and there is
> no set_ringparam path.
>
> If a runtime ring size setter is added later, a count that is zero, not
> a power of two, or above 65536 would break the masked indexes in
> mpnic_clean_twq0() and mpnic_clean_tcq().
>
> Would a WARN_ON_ONCE() or an explicit check here be worthwhile?