Re: [PATCH net 1/2] net: atlantic: free stranded TX buffers on ring deinit

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

Pre-existing issues:
- [Critical] `aq_xdp_xmit()` does not break on transmission failures or com=
pact the frames array, leading to double-frees and memory leaks.
- [High] `aq_ptp_ring_deinit()` uses `aq_ring_tx_clean()` instead of `aq_ri=
ng_tx_deinit()`, leaving the PTP TX ring vulnerable to the same stranded bu=
ffer memory leak.
--

--- Patch [1]: [PATCH net 1/2] net: atlantic: free stranded TX buffers on r=
ing deinit ---
commit 80ef845583443e3da7aeeb279e31f6845d6e1812
Author: Yangyu Chen <[email protected]>
net: atlantic: free stranded TX buffers on ring deinit

This commit brings back aq_ring_tx_deinit() to replace aq_ring_tx_clean()
during TX ring deinitialization. This ensures that completed descriptors
beyond the clean budget and frames stuck between hw_head and sw_tail are
properly freed, resolving a memory leak when an interface goes down.

> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/n=
et/ethernet/aquantia/atlantic/aq_ring.c
> index 8ff07de2bd524..81685a4dc5a6d 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> @@ -360,6 +360,35 @@ bool aq_ring_tx_clean(struct aq_ring_s *self)
>  	return !!budget;
>  }
> =20
> +void aq_ring_tx_deinit(struct aq_ring_s *self)
> +{
> +	if (!self)
> +		return;
> +
> +	for (; self->sw_head !=3D self->sw_tail;
> +		self->sw_head =3D aq_ring_next_dx(self, self->sw_head)) {
> +		struct aq_ring_buff_s *buff =3D &self->buff_ring[self->sw_head];
> +		struct device *ndev =3D aq_nic_get_dev(self->aq_nic);
> +
> +		if (buff->is_mapped) {
> +			if (buff->is_sop) {
> +				dma_unmap_single(ndev, buff->pa, buff->len,
> +						 DMA_TO_DEVICE);
> +			} else {
> +				dma_unmap_page(ndev, buff->pa, buff->len,
> +					       DMA_TO_DEVICE);
> +			}
> +		}
> +
> +		if (buff->is_eop) {
> +			if (buff->skb)
> +				dev_kfree_skb_any(buff->skb);
> +			else if (buff->xdpf)
> +				xdp_return_frame(buff->xdpf);
> +		}
> +	}
> +}

[Severity: Critical]
This is a pre-existing issue, but while we are looking at XDP frame handling
in this file, does aq_xdp_xmit() handle transmission failures correctly?

In aq_xdp_xmit(), if aq_nic_xmit_xdpf() fails, it just increments a drop
counter and continues:

drivers/net/ethernet/aquantia/atlantic/aq_ring.c:aq_xdp_xmit() {
    ...
	for (i =3D 0; i < num_frames; i++) {
		struct xdp_frame *xdpf =3D frames[i];

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

	return num_frames - drop;
}

Callers of ndo_xdp_xmit (like bq_xmit_all() in kernel/bpf/devmap.c) assume a
return value of N means the first N contiguous frames in the array were
successfully transmitted, and they free the remaining frames starting at
index N.

If aq_xdp_xmit() processes 3 frames, fails on the first, and succeeds on the
next two, it returns 2.

Will the caller then free frames[2] which was successfully queued to the NI=
C,
causing a double-free, while leaking the failed frames[0]?

> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_vec.c b/drivers/ne=
t/ethernet/aquantia/atlantic/aq_vec.c
> index 2f9033ceed8ce..05814fea0f5f7 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_vec.c
> @@ -275,7 +275,7 @@ void aq_vec_deinit(struct aq_vec_s *self)
> =20
>  	for (i =3D 0U; self->tx_rings > i; ++i) {
>  		ring =3D self->ring[i];
> -		aq_ring_tx_clean(&ring[AQ_VEC_TX_ID]);
> +		aq_ring_tx_deinit(&ring[AQ_VEC_TX_ID]);

[Severity: High]
This isn't a bug introduced by this patch, but does the PTP TX ring need the
same fix?

Looking at aq_ptp_ring_deinit(), the PTP TX ring is still deinitialized
using aq_ring_tx_clean():

drivers/net/ethernet/aquantia/atlantic/aq_ptp.c:aq_ptp_ring_deinit() {
    ...
	aq_ring_tx_clean(&aq_ptp->ptp_tx);
	aq_ring_rx_deinit(&aq_ptp->ptp_rx);
}

If the interface is brought down while PTP traffic is in flight, will this
lead to the exact same memory leak of SKBs and DMA mappings for the PTP ring
when it is subsequently freed?

>  		aq_ring_rx_deinit(&ring[AQ_VEC_RX_ID]);
>  	}

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/tencent_29B86031792=
[email protected]?part=3D1