Re: [PATCH net-next v2 1/3] net: atlantic: free stranded TX buffers on ring deinit

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incomplete fix: PTP TX ring still uses `aq_ring_tx_clean()` on teardown, leading to memory/DMA leaks and a locking violation.
--

--- Patch [1]: [PATCH net-next v2 1/3] net: atlantic: free stranded TX buffers on ring deinit ---
commit d506a87f11eebfd3878d7ddde798e451d9fd8627
Author: Yangyu Chen <cyy@cyyself.name>

net: atlantic: free stranded TX buffers on ring deinit

This commit brings back aq_ring_tx_deinit() to properly clean up TX rings 
on teardown by iterating up to sw_tail. It replaces the call to 
aq_ring_tx_clean() in aq_vec_deinit() to fix a memory and DMA mapping leak 
that occurs during interface shutdown.

Cc: stable@vger.kernel.org # v4.11+

> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_vec.c b/drivers/net/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)
>  
>  	for (i = 0U; self->tx_rings > i; ++i) {
>  		ring = self->ring[i];
> -		aq_ring_tx_clean(&ring[AQ_VEC_TX_ID]);
> +		aq_ring_tx_deinit(&ring[AQ_VEC_TX_ID]);
>  		aq_ring_rx_deinit(&ring[AQ_VEC_RX_ID]);
>  	}

[Severity: High]
This is a pre-existing issue, but should the PTP TX ring be updated to use 
aq_ring_tx_deinit() as well?

Looking at the PTP TX ring teardown path:

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

Since aq_ring_tx_clean() only cleans up to hw_head, won't any PTP descriptors
between hw_head and sw_tail be leaked when the interface is stopped while
PTP TX packets are in flight? 

Also, since aq_ring_tx_clean() updates u64_stats without disabling preemption,
does keeping it here leave the teardown path vulnerable to a lockdep splat on
32-bit systems?

Does it make sense to update aq_ptp_ring_deinit() to use the newly restored
aq_ring_tx_deinit() for the PTP TX ring to fully close this leak?

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