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

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Uninitialized physical addresses cause severe DMA memory corruption during RX buffer refill.

Pre-existing issues:
- [High] The ndo_xdp_xmit callback violates API contract by not repacking arrays on failure, causing UAF and memory leaks.
- [High] Malformed RX packets exceeding MAX_SKB_FRAGS cause a permanent infinite NAPI polling loop.
- [High] Aborting XDP assembly on oversized packets leaves trailing fragments uncleaned, leading to packet injection.
- [High] Updating u64_stats without disabling preemption causes a system livelock on 32-bit architectures.
--

--- Patch [2]: [PATCH net-next 2/2] net: atlantic: convert RX path to page_pool ---
commit d3fa6dc4118e01fdb55de72ea89c2b574594ac08
Author: Yangyu Chen <cyy@cyyself.name>

net: atlantic: convert RX path to page_pool

This commit converts the atlantic driver's RX path to use page_pool for
fragment management to reduce DMA map/unmap overhead. It also registers the
PTP RX ring's xdp_rxq and cleans up deprecated ethtool counters.

> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> index c59b6c0b37f1e..e06c502b59f95 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c

[ ... ]

> -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;
> -	u16 page_offset = self->page_offset;
> -	u16 frame_max = self->frame_max;
> -	u16 tail_size = self->tail_size;
> -	int ret;
> -
> -	if (rxbuf->rxdata.page) {
> -		/* One means ring is the only user and can reuse */
> -		if (page_ref_count(rxbuf->rxdata.page) > 1) {
> -			/* Try reuse buffer */
> -			rxbuf->rxdata.pg_off += frame_max + page_offset +
> -						tail_size;
> -			if (rxbuf->rxdata.pg_off + frame_max + tail_size <=
> -			    (PAGE_SIZE << order)) {
> -				u64_stats_update_begin(&self->stats.rx.syncp);
> -				self->stats.rx.pg_flips++;
> -				u64_stats_update_end(&self->stats.rx.syncp);
> -
> -			} else {
> -				/* Buffer exhausted. We have other users and
> -				 * should release this page and realloc
> -				 */
> -				aq_free_rxpage(&rxbuf->rxdata,
> -					       aq_nic_get_dev(self->aq_nic));
> -				u64_stats_update_begin(&self->stats.rx.syncp);
> -				self->stats.rx.pg_losts++;
> -				u64_stats_update_end(&self->stats.rx.syncp);
> -			}
> -		} else {
> -			rxbuf->rxdata.pg_off = page_offset;
> -			u64_stats_update_begin(&self->stats.rx.syncp);
> -			self->stats.rx.pg_reuses++;
> -			u64_stats_update_end(&self->stats.rx.syncp);
> -		}
> +	unsigned int size = self->page_offset + self->frame_max +
> +			    self->tail_size;
> +	unsigned int pg_off;
>  	struct page *page;
> -	int ret = -ENOMEM;
> -	dma_addr_t daddr;
> -
> -	page = dev_alloc_pages(order);
> -	if (unlikely(!page))
> -		goto err_exit;
>  
> -	daddr = dma_map_page(dev, page, 0, PAGE_SIZE << order,
> -			     DMA_FROM_DEVICE);
> +	/* Buffers whose page was not passed up the stack are reposted
> +	 * with the data they already carry discarded.
> +	 */
> +	if (rxbuf->rxdata.page)
> +		return 0;
>  
> -	if (unlikely(dma_mapping_error(dev, daddr)))
> -		goto free_page;
> -
> -	rxpage->page = page;
> -	rxpage->daddr = daddr;
> -	rxpage->order = order;
> -	rxpage->pg_off = rx_ring->page_offset;
> -
> -	return 0;
> -
> -free_page:
> -	__free_pages(page, order);
> -
> -err_exit:
> -	return ret;
> -}
> -
> -static int aq_get_rxpages(struct aq_ring_s *self, struct aq_ring_buff_s *rxbuf)
> -{
> -	unsigned int order = self->page_order;
> -	u16 page_offset = self->page_offset;
> -	u16 frame_max = self->frame_max;
> -	u16 tail_size = self->tail_size;
> -	int ret;
> -
> -	if (rxbuf->rxdata.page) {
> -		/* One means ring is the only user and can reuse */
> -		if (page_ref_count(rxbuf->rxdata.page) > 1) {
> -			/* Try reuse buffer */
> -			rxbuf->rxdata.pg_off += frame_max + page_offset +
> -						tail_size;
> -			if (rxbuf->rxdata.pg_off + frame_max + tail_size <=
> -			    (PAGE_SIZE << order)) {
> -				u64_stats_update_begin(&self->stats.rx.syncp);
> -				self->stats.rx.pg_flips++;
> -				u64_stats_update_end(&self->stats.rx.syncp);
> -
> -			} else {
> -				/* Buffer exhausted. We have other users and
> -				 * should release this page and realloc
> -				 */
> -				aq_free_rxpage(&rxbuf->rxdata,
> -					       aq_nic_get_dev(self->aq_nic));
> -				u64_stats_update_begin(&self->stats.rx.syncp);
> -				self->stats.rx.pg_losts++;
> -				u64_stats_update_end(&self->stats.rx.syncp);
> -			}
> -		} else {
> -			rxbuf->rxdata.pg_off = page_offset;
> -			u64_stats_update_begin(&self->stats.rx.syncp);
> -			self->stats.rx.pg_reuses++;
> -			u64_stats_update_end(&self->stats.rx.syncp);
> -		}
> +	page = page_pool_dev_alloc_frag(self->pg_pool, &pg_off, size);
> +	if (unlikely(!page)) {
> +		u64_stats_update_begin(&self->stats.rx.syncp);
> +		self->stats.rx.alloc_fails++;
> +		u64_stats_update_end(&self->stats.rx.syncp);

[Severity: High]
This is a pre-existing issue, but can this cause a system livelock on 32-bit
architectures?

The u64_stats_update_begin() function is called here in process context
(e.g., during aq_ring_rx_fill() via aq_vec_start()) without disabling
preemption.

If a writer is preempted by a reader (such as ndo_get_stats64() invoked from
a different CPU), the reader will spin forever in the
u64_stats_fetch_retry() loop on 32-bit systems because the sequence counter
will never become even.

> +		return -ENOMEM;
> +	}
>  
> -	if (!rxbuf->rxdata.page) {
> -		ret = aq_alloc_rxpages(&rxbuf->rxdata, self);
> -		if (ret) {
> -			u64_stats_update_begin(&self->stats.rx.syncp);
> -			self->stats.rx.alloc_fails++;
> -			u64_stats_update_end(&self->stats.rx.syncp);
> -		}
> -		return ret;
> -	}
> +	rxbuf->rxdata.page = page;
> +	rxbuf->rxdata.daddr = page_pool_get_dma_addr(page);
> +	rxbuf->rxdata.pg_off = pg_off + self->page_offset;

[Severity: Critical]
Can this cause severe DMA memory corruption during RX buffer refill?

The initialization of buff->pa appears to be missing or was removed here,
but the hardware callback still executes immediately after and
unconditionally overwrites the descriptor's physical address:

hw_atl_b0_hw_ring_rx_fill() {
    ...
    rxd->buf_addr = buff->pa;
    rxd->hdr_addr = 0U;
    ...
}

Because buff->pa is no longer initialized for RX, it contains garbage from
the previous packet (specifically rss_hash and next, due to the union in
the aq_ring_buff_s struct).

This programs the NIC with arbitrary physical addresses, causing it to DMA
incoming network packets into random physical memory.

[ ... ]

> @@ -438,22 +380,15 @@ int aq_xdp_xmit(struct net_device *dev, int num_frames,
>  }

[Severity: High]
This is a pre-existing issue, but does the error handling in aq_xdp_xmit()
violate the ndo_xdp_xmit API contract?

When transmitting a batch of XDP frames, if a frame in the middle fails
(returning NETDEV_TX_BUSY), the function increments a drop counter and
returns num_frames - drop:

aq_xdp_xmit() {
    ...
    if (aq_nic_xmit_xdpf(aq_nic, ring, xdpf) == NETDEV_TX_BUSY)
        drop++;
    ...
    return num_frames - drop;
}

The loop drops frames but does not repack the frames array. The network
stack (e.g., devmap) assumes that a partial success count N means the first
N frames succeeded and the rest failed.

It will free the frames at the end of the array, some of which were actually
successfully queued for transmission by the driver. This leads to freeing
memory that the NIC is actively trying to DMA, while leaking the memory of
the frame that actually failed.

[ ... ]

> @@ -547,8 +490,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;

[Severity: High]
This is a pre-existing issue, but does aborting XDP assembly on oversized
packets leave trailing fragments uncleaned?

When an RX packet exceeds MAX_SKB_FRAGS during XDP processing, this early
return aborts but fails to set buff_->is_cleaned = 1 for the remaining
descriptors of the malformed packet.

The outer loop in __aq_ring_xdp_clean() handles the abort by advancing
sw_head by 1. Because the trailing fragments were never marked as cleaned,
subsequent iterations of the loop will encounter them and incorrectly parse
them as the Start-Of-Packet (SOP) for new frames.

This forces the driver to interpret raw middle-of-packet payload data as
Ethernet/IP headers, leading to arbitrary packet injection.

> +		}
>  
>  		frag = &sinfo->frags[sinfo->nr_frags++];
>  		buff_ = &ring->buff_ring[buff_->next];

[ ... ]

> @@ -675,6 +626,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 does the bailout logic in
__aq_ring_rx_clean() create an unrecoverable infinite loop?

If a remote attacker sends a highly fragmented or malformed packet that
spans more than MAX_SKB_FRAGS descriptors, the driver detects this and
jumps to err_exit:

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

Because it bails out without advancing the sw_head ring pointer or dropping
the packet, the driver will process the exact same malformed packet on the
next NAPI poll, hit the exact same condition, and bail out again.

This creates a permanent infinite loop in softirq context, leading to a
complete denial of service for the CPU core processing RX queues.

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

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