Re: [PATCH net-next v3] net: atlantic: convert RX path to page_pool

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing `dma_sync_single_for_device` on locally recycled RX buffers.

Pre-existing issues:
- [Critical] Kernel crash when `XDP_TX` is returned for a packet on the PTP RX ring due to a missing PTP TX ring mapping.
- [Critical] Garbage packet injection and state machine desynchronization when an XDP packet exceeds the MAX_SKB_FRAGS limit.
- [High] Infinite NAPI polling loop when receiving an oversized packet.
- [High] Concurrent execution of `u64_stats_update_begin` on `tx.syncp` corrupts seqcount.
- [High] Lockless concurrent access to TX ring descriptors in `aq_xdp_xmit`.
--

--- Patch [1]: [PATCH net-next v3] net: atlantic: convert RX path to page_pool ---
commit 6897c00e6a396ede4252845d65592374630adc51
Author: Yangyu Chen <cyy@cyyself.name>

net: atlantic: convert RX path to page_pool

The driver is converted to use page_pool fragments for RX buffers instead of
dev_alloc_pages() and a hand-rolled page-flip scheme. The PTP RX ring's
xdp_rxq is also registered to use the MEM_TYPE_PAGE_POOL memory model.

Link: https://lore.kernel.org/lkml/tencent_1F173E0FC1606D2AC704DC9C98AF10984607@qq.com/
Link: https://lore.kernel.org/lkml/tencent_7DB01BE7F8FA056BB5F11D3570CF636C4309@qq.com/

> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c b/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
> index 558ac9237f759..3a40d986cd678 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
> @@ -1192,12 +1193,23 @@ int aq_ptp_ring_alloc(struct aq_nic_s *aq_nic)
>  	if (err)
>  		goto err_exit_ptp_tx;
>  

[Severity: Critical]
This is a pre-existing issue, but could this cause a kernel panic from a
NULL pointer dereference? In aq_ptp_ring_alloc(), the PTP TX ring is
allocated but not registered in the global aq_nic->aq_ring_tx array:

aq_ptp_ring_alloc()
    err = aq_ring_tx_alloc(&aq_ptp->ptp_tx, aq_nic, ...);

If an XDP program is attached and returns XDP_TX for a packet received on
the PTP RX ring, the tx_ring lookup evaluates to NULL:

aq_xdp_run_prog()
    tx_ring = aq_nic->aq_ring_tx[rx_ring->idx];
    result = aq_nic_xmit_xdpf(aq_nic, tx_ring, xdpf);

> +	err = xdp_rxq_info_reg(&aq_ptp->ptp_rx.xdp_rxq, aq_nic->ndev,
> +			       rx_ring_idx, aq_ptp->napi.napi_id);
[ ... ]
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> index e1193c6719d9d..9dd8817105942 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> @@ -14,120 +14,37 @@
> -static int aq_get_rxpages(struct aq_ring_s *self, struct aq_ring_buff_s *rxbuf)
> +static int aq_get_rxpages(struct aq_ring_s *self, struct aq_ring_buff_s *rxbuf)
>  {
> -	unsigned int order = self->page_order;
[ ... ]
> +	unsigned int size = self->page_offset + self->frame_max +
> +			    self->tail_size;
> +	unsigned int pg_off;
>  	struct page *page;
>  
> -	if (rxbuf->rxdata.page) {
[ ... ]
> +	/* Buffers whose page was not passed up the stack are reposted
> +	 * with the data they already carry discarded.
> +	 */
> +	if (rxbuf->rxdata.page)
> +		return 0;

[Severity: High]
Are we missing a dma_sync_single_for_device() call for the locally
recycled buffer?

When a packet is smaller than the header copy threshold, its payload is
copied to a newly allocated SKB and its page is recycled directly. The CPU
reads the data after a dma_sync_single_range_for_cpu() call, but when the
page is passed back to the hardware, the lack of
dma_sync_single_for_device() might allow the CPU cache to overwrite DMA
memory while the NIC writes the next packet.

> +	page = page_pool_dev_alloc_frag(self->pg_pool, &pg_off, size);
[ ... ]
> @@ -346,7 +284,11 @@ bool aq_ring_tx_clean(struct aq_ring_s *self)
>  			++self->stats.tx.packets;
>  			self->stats.tx.bytes += xdp_get_frame_len(buff->xdpf);
>  			u64_stats_update_end(&self->stats.tx.syncp);
> -			xdp_return_frame_rx_napi(buff->xdpf);
> +			/* Frames queued via ndo_xdp_xmit() may come from a

[Severity: High]
This is a pre-existing issue, but might this lead to a corrupted seqcount
and infinite loops for readers on 32-bit systems?

Writers to a u64_stats_sync block must be mutually exclusive and disable
preemption. However, this code updates tx.syncp locklessly from the NAPI
poll context in aq_ring_tx_clean():

aq_ring_tx_clean()
    if (buff->skb) {
        u64_stats_update_begin(&self->stats.tx.syncp);

While aq_ptp_xmit() updates the exact same syncp block holding a spinlock
in process or softirq context:

aq_ptp_xmit()
    spin_lock_irqsave(&aq_nic->aq_ptp->ptp_ring_lock, irq_flags);
    ...
    u64_stats_update_begin(&ring->stats.tx.syncp);

> +			 * page pool owned by another NAPI context: no direct
> +			 * recycling.
> +			 */
> +			xdp_return_frame(buff->xdpf);
>  		}
>  
>  out:
[ ... ]
> @@ -437,22 +379,15 @@ int aq_xdp_xmit(struct net_device *dev, int num_frames,
>  }

[Severity: High]
This is a pre-existing issue, but could this cause lockless concurrent
access to TX ring descriptors?

If the number of online CPUs exceeds the number of hardware queues, multiple
CPUs will map to the same ring and invoke aq_nic_xmit_xdpf() simultaneously
without any locks:

aq_xdp_xmit()
    vec = cpu % aq_cfg->vecs;
    ring = aq_nic->aq_ring_tx[AQ_NIC_CFG_TCVEC2RING(aq_cfg, 0, vec)];
    for (i = 0; i < num_frames; i++) {
        ...
        if (aq_nic_xmit_xdpf(aq_nic, ring, xdpf) == NETDEV_TX_BUSY)

>  
>  static struct sk_buff *aq_xdp_build_skb(struct xdp_buff *xdp,
> -					struct net_device *dev,
> -					struct aq_ring_buff_s *buff)
[ ... ]
> @@ -546,8 +489,11 @@ static bool aq_add_rx_fragment(struct device *dev,
>  	do {
>  		skb_frag_t *frag;
>  
> -		if (unlikely(sinfo->nr_frags >= MAX_SKB_FRAGS))
> +		if (unlikely(sinfo->nr_frags >= MAX_SKB_FRAGS)) {
> +			/* Attached frags must reach xdp_return_buff() */
> +			xdp_buff_set_frags_flag(xdp);
>  			return true;
> +		}
>  
>  		frag = &sinfo->frags[sinfo->nr_frags++];
>  		buff_ = &ring->buff_ring[buff_->next];

[Severity: Critical]
This is a pre-existing issue, but does this abandon trailing fragments
without marking them as cleaned?

When a packet exceeds MAX_SKB_FRAGS, aq_add_rx_fragment() returns true
early:

aq_add_rx_fragment()
    if (unlikely(sinfo->nr_frags >= MAX_SKB_FRAGS)) {
        ...
        return true;
    }

Then __aq_ring_xdp_clean() skips further processing of those fragments:

__aq_ring_xdp_clean()
    if (!buff->is_eop) {
        if (aq_add_rx_fragment(dev, rx_ring, buff, &xdp)) {
            ...
            xdp_return_buff(&xdp);
            continue;
        }
    }

The uncleaned hardware descriptors are left behind. On the next loop
iteration, the driver will treat these uncleaned payload fragments as the
start of a completely new packet, potentially injecting garbage or bypassing
header checks.

> @@ -674,6 +625,7 @@ static int __aq_ring_rx_clean(struct aq_ring_s *self, struct napi_struct *napi,
>  			err = -ENOMEM;
>  			goto err_exit;
>  		}
> +		skb_mark_for_recycle(skb);

[Severity: High]
This is a pre-existing issue, but can this create an infinite NAPI polling
loop when receiving an oversized packet?

In __aq_ring_rx_clean(), when a packet spanning more than MAX_SKB_FRAGS is
received:

__aq_ring_rx_clean()
    if (unlikely(!is_rsc_completed) ||
        frag_cnt > MAX_SKB_FRAGS) {
        err = 0;
        goto err_exit;
    }

The error handler jumps to err_exit without advancing self->sw_head. This
would trap the driver into processing the exact same failing packet
continuously on every poll iteration, leading to a denial of service for
the affected RX queue.

>  		if (is_ptp_ring)
>  			buff->len -=
>  				aq_ptp_extract_ts(self->aq_nic, skb_hwtstamps(skb),

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_F771B81731B921458D6F7CBF82ECDA888E08@qq.com?part=1