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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Encountering an RX packet with more than MAX_SKB_FRAGS fragments causes an infinite NAPI polling loop.
- [High] In the XDP path, if a packet exceeds MAX_SKB_FRAGS, the remainder of its fragments are erroneously parsed as independent new packets.
- [High] NULL pointer dereference when an XDP program returns XDP_TX for a packet received on the PTP ring.
- [High] Incorrect reporting of dropped XDP frames in `ndo_xdp_xmit` causes dropped frames to be leaked and successfully transmitted frames to be double-freed.
--

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

net: atlantic: convert RX path to page_pool

The driver currently allocates RX buffers with dev_alloc_pages(), maps
them with dma_map_page(), and uses a hand-rolled page-flip scheme to
subdivide high-order pages. Behind an IOMMU, the map/unmap churn is a
major RX cost. This patch converts RX buffers to page_pool fragments.

> 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

[ ... ]

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

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for
aq_xdp_xmit() to mislead the caller into freeing the wrong frames?

When a transmission fails, the loop increments drop but does not break
or reorganize the frames array:

        for (i = 0; i < num_frames; i++) {
                struct xdp_frame *xdpf = frames[i];

                if (aq_nic_xmit_xdpf(aq_nic, ring, xdpf) == NETDEV_TX_BUSY)
                        drop++;
        }

        return num_frames - drop;

If the failure occurs in the middle of the array, the kernel's XDP stack will
assume the last drop frames failed and free them. Could this result in a
use-after-free of successfully transmitted frames while they are still being
accessed by hardware, and leak the actual dropped frames?

>  static struct sk_buff *aq_xdp_build_skb(struct xdp_buff *xdp,

[ ... ]

> @@ -503,7 +446,6 @@ static struct sk_buff *aq_xdp_run_prog(struct aq_nic_s *aq_nic,
>  		u64_stats_update_begin(&rx_ring->stats.rx.syncp);
>  		++rx_ring->stats.rx.xdp_tx;
>  		u64_stats_update_end(&rx_ring->stats.rx.syncp);
> -		aq_get_rxpages_xdp(buff, xdp);
>  		break;

[Severity: High]
This is a pre-existing issue, but could this dereference a NULL pointer
if an XDP program returns XDP_TX for a packet on the PTP ring?

The PTP RX ring shares the RX clean path, meaning __aq_ring_xdp_clean() and
aq_xdp_run_prog() are executed for it. If an XDP program returns XDP_TX:

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

Since the PTP TX ring is never placed into the aq_ring_tx array during
allocation, tx_ring would be NULL here.

>  	case XDP_REDIRECT:

[ ... ]

> @@ -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;
> +		}

[Severity: High]
This isn't a bug introduced by this patch, but does the early return in
aq_add_rx_fragment() leave remaining descriptors uncleaned?

If a packet exceeds MAX_SKB_FRAGS, it seems aq_add_rx_fragment() aborts
and returns true without setting buff_->is_cleaned = 1 on the rest of the
fragments.

Then, in __aq_ring_xdp_clean(), the outer loop continues, increments sw_head,
and encounters those uncleaned fragments:

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

Since their is_cleaned flag is false, could they be treated as the start of a
brand new packet and erroneously passed to the XDP program?

>  		frag = &sinfo->frags[sinfo->nr_frags++];

[ ... ]

> @@ -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 I have a question about the error handling
in __aq_ring_rx_clean().

If hardware aggregates a packet resulting in more than MAX_SKB_FRAGS fragments
(for example, via LRO), the code takes this error path before getting here:

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

Does this exit the function without advancing sw_head or consuming the
malformed multi-fragment packet? On the next NAPI poll, it seems the exact
same state would be encountered, permanently stalling the queue in an infinite
loop.

>  		if (is_ptp_ring)
>  			buff->len -=

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