Re: [PATCH 07/15] yunsilicon: add ethernet TX path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider:
- [Critical] net: stale wi->skb pointer causes infinite loop and double-free on spurious CQE
- [High] net: unpopped error CQE causes infinite NAPI polling loop
- [High] net: inverted return logic in xsc_poll_tx_cq causes NAPI interrupt storms
- [High] net: TX queue can permanently stall due to missing smp_mb() after netif_tx_stop_queue
- [High] net: NAPI instances are deleted without being disabled in xsc_eth_open_channels error path
- [High] net: NULL pointer dereference of aff_mask in xsc_channel_no_affinity_change
- [High] yunsilicon: memory and DMA leak on TX queue teardown
- [Medium] net: unconditional memory leak of mailbox buffer in xsc_eth_set_cq
- [Medium] net: SQ DB arrays leaked on error path in xsc_eth_open_qp_sq
- [Medium] net: firmware teardown failures cause software memory leaks
- [Medium] net: TX packets are double-counted on retry and miscounted on drop
- [Medium] net: missing u64_stats_sync for 64-bit per-queue network statistics
--

commit 68f71bfadd8545a6a72866ea79110d42b134cfcb
Author: Xin Tian <tianx@yunsilicon.com>
Subject: yunsilicon: add ethernet TX path

This commit adds the transmit path for the yunsilicon ethernet driver.
It introduces WQE construction, completion handling, and ndo_start_xmit.

[Severity: High]
In drivers/net/ethernet/yunsilicon/xsc/net/xsc_eth_txrx.c, inside the
xsc_poll_tx_cq function around line 62, the code detects an error CQE
and returns false without calling xsc_cqwq_pop. Will this cause the NAPI
polling loop to wedge permanently as subsequent polls re-fetch the exact
same CQE? Additionally, the CQEs fetched inside the do-while loop around
line 99 bypass the error check entirely, which might result in error
CQEs being processed as successful completions.

[Severity: High]
Also in xsc_poll_tx_cq around line 99, the polling loop uses the condition
++i <= napi_budget. If the budget is fully exhausted, the loop breaks with
i equal to napi_budget + 1. The function then returns i == napi_budget,
which evaluates to false. Can this inverted return logic cause NAPI to
prematurely end the polling cycle while the queue is still full, leading
to interrupt storms?

[Severity: Critical]
Further down in xsc_poll_tx_cq around lines 84 and 96, napi_consume_skb
frees the SKB but leaves the wi->skb pointer intact. If the cyclic buffer
wraps around and a spurious CQE is fetched, this dangling pointer will
falsely pass the !skb condition check, resulting in a double-free of the
SKB and double DMA unmap. Furthermore, the handling for spurious CQEs uses
continue to skip the rest of the loop without advancing sqcc and
dma_fifo_cc. Does this result in an infinite loop by re-processing the
same SW WQE?

[Severity: High]
In drivers/net/ethernet/yunsilicon/xsc/net/xsc_eth_tx.c, within the
xsc_txwqe_complete function around line 185, netif_tx_stop_queue is
called if the queue lacks room. Should there be an smp_mb barrier and a
space re-check immediately after stopping the queue? Without an explicit
barrier, concurrent NAPI updates could race with the TX path, permanently
stalling the TX queue.

[Severity: Medium]
In drivers/net/ethernet/yunsilicon/xsc/net/main.c, inside the
xsc_eth_set_cq function around line 427, the mailbox buffer in is
allocated with kvzalloc. On successful completion of xsc_core_eth_create_cq,
the function returns 0 but skips freeing the buffer. Can we add a kvfree
for the buffer on the success path to avoid permanently leaking this memory?

[Severity: Medium]
Similarly, in xsc_eth_open_qp_sq around lines 560 and 586, if a setup step
like modify_in allocation fails, the error path jumps to
err_sq_common_destroy. This path destroys the WQ and QP but does not call
xsc_free_qp_sq_db. Does this leak the dma_fifo and wqe_info arrays
allocated earlier in the function?

[Severity: High]
When tearing down channels in xsc_eth_open_channels (error path around line
639) and xsc_eth_close_channel (around line 605), netif_napi_del is
called directly. Because napi_enable was already called, should
napi_disable be executed prior to deletion to prevent a use-after-free if
the NAPI instance is scheduled concurrently?

[Severity: Medium]
In xsc_eth_close_cq and xsc_eth_close_qp_sq around lines 349 and 492, the
functions return early if xsc_core_eth_destroy_cq or
xsc_core_eth_destroy_qp report a firmware error. Bypassing xsc_eth_free_cq
and xsc_free_qp_sq on these errors appears to indefinitely leak the OS
level queue memory if the firmware is unresponsive. Is it intentional to
skip freeing software memory on a firmware timeout?

[Severity: Medium]
In xsc_eth_xmit_frame around lines 234 and 262, stats->packets is
incremented preemptively. If the packet exceeds a single WQE and is passed
to skb_linearize, a successful linearization jumps back to retry_send,
causing the packet to be double-counted. If linearization fails and jumps
to err_drop, the counter is not reverted. Should the statistics update be
moved after we guarantee the packet will not be dropped or retried?

[Severity: High]
In drivers/net/ethernet/yunsilicon/xsc/net/xsc_eth_txrx.c, the function
xsc_channel_no_affinity_change around line 23 directly dereferences
c->aff_mask in cpumask_test_cpu. Since c->aff_mask is only assigned when
xdev->caps.msix_enable is true, and irq_get_affinity_mask can return
NULL, will this cause a NULL pointer dereference during NAPI polling on
systems without MSI-X?

[Severity: Medium]
In xsc_eth_xmit_frame around line 290, 64-bit statistic counters like
stats->bytes and stats->packets are updated directly. To prevent torn
reads on 32-bit architectures, should these per-queue statistics be updated
using the u64_stats_update_begin and u64_stats_update_end macros?

[Severity: High]
In xsc_free_qp_sq_db around line 436, the tracking arrays sq->db.wqe_info
and sq->db.dma_fifo are freed. It appears the function does not drain and
unmap active elements between sq->cc and sq->pc first. Can this lead to
a silent leak of SKBs and DMA mappings when the interface is stopped or
reconfigured while traffic is active?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012114352.2471317-1-tianx@yunsilicon.com?part=7