[PATCH ath-next] wifi: ath9k: unmap DMA and free skb for stale TX buffers on queue drain

[PATCH ath-next] wifi: ath9k: unmap DMA and free skb for stale TX buffers on queue drain

[Rosen Penev]

When draining a TX queue (e.g. on device removal or reset), stale
ath_buf descriptors were returned directly to the free pool via
ath_tx_return_buffer without unmapping their DMA buffers or freeing
their associated skbs. This leaks both DMA API resources and socket
buffer memory.

Unmap the DMA buffer and free the skb before returning the buffer
to the pool.

Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/net/wireless/ath/ath9k/xmit.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index 89d8b3178784..89930eaf1fd3 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -1884,8 +1884,17 @@ static void ath_drain_txq_list(struct ath_softc *sc, struct ath_txq *txq,
 		bf = list_first_entry(list, struct ath_buf, list);
 
 		if (bf->bf_state.stale) {
-			list_del(&bf->list);
+			struct sk_buff *skb = bf->bf_mpdu;
 
+			list_del(&bf->list);
+			if (bf->bf_buf_addr) {
+				dma_unmap_single(sc->dev, bf->bf_buf_addr,
+						 skb ? skb->len : 0,
+						 DMA_TO_DEVICE);
+				bf->bf_buf_addr = 0;
+			}
+			bf->bf_mpdu = NULL;
+			dev_kfree_skb_any(skb);
 			ath_tx_return_buffer(sc, bf);
 			continue;
 		}
-- 
2.55.0

Re: [PATCH ath-next] wifi: ath9k: unmap DMA and free skb for stale TX buffers on queue drain

[Toke Høiland-Jørgensen]

Rosen Penev <rosenp@gmail.com> writes:

> When draining a TX queue (e.g. on device removal or reset), stale
> ath_buf descriptors were returned directly to the free pool via
> ath_tx_return_buffer without unmapping their DMA buffers or freeing
> their associated skbs. This leaks both DMA API resources and socket
> buffer memory.
>
> Unmap the DMA buffer and free the skb before returning the buffer
> to the pool.
>
> Assisted-by: opencode:big-pickle
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  drivers/net/wireless/ath/ath9k/xmit.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
> index 89d8b3178784..89930eaf1fd3 100644
> --- a/drivers/net/wireless/ath/ath9k/xmit.c
> +++ b/drivers/net/wireless/ath/ath9k/xmit.c
> @@ -1884,8 +1884,17 @@ static void ath_drain_txq_list(struct ath_softc *sc, struct ath_txq *txq,
>  		bf = list_first_entry(list, struct ath_buf, list);
>  
>  		if (bf->bf_state.stale) {
> -			list_del(&bf->list);
> +			struct sk_buff *skb = bf->bf_mpdu;
>  
> +			list_del(&bf->list);
> +			if (bf->bf_buf_addr) {
> +				dma_unmap_single(sc->dev, bf->bf_buf_addr,
> +						 skb ? skb->len : 0,
> +						 DMA_TO_DEVICE);

A zero-length unmap? That seems completely non-sensical. Is there even a
bug here? AFAICT, the only place where a buf is set as stale, the
aggregate subsequently goes through ath_tx_complete_buf(), which does
the unmapping.

Did you test this? Unless you can demonstrate a real bug here, I'm just
going to assume this is all LLM garbage output.

-Toke