RE: [PATCH] wifi: rtw88: usb: fix memory leaks on USB write failures

[Ping-Ke Shih]

luka.gejak@xxxxxxxxx <luka.gejak@xxxxxxxxx> wrote:
> From: Luka Gejak <luka.gejak@xxxxxxxxx>
> 
> When rtw_usb_write_port() fails to submit a USB Request Block (URB)
> (e.g., due to device disconnect or ENOMEM), the completion callback is
> never executed.
> 
> Currently, the driver ignores the return value of rtw_usb_write_port()
> in rtw_usb_write_data() and rtw_usb_tx_agg_skb(). Because these
> functions rely on the completion callback to free the socket buffers
> (skbs) and the transaction control block (txcb), a submission failure
> results in:
> 1. A memory leak of the allocated skb in rtw_usb_write_data().
> 2. A memory leak of the txcb structure and all aggregated skbs in
>    rtw_usb_tx_agg_skb().
> 
> Fix this by checking the return value of rtw_usb_write_port(). If it
> fails, explicitly free the skb in rtw_usb_write_data(), and properly
> purge the tx_ack_queue and free the txcb in rtw_usb_tx_agg_skb().
> 
> Fixes: e3037485c68e ("rtw88: new Realtek 802.11ac driver")

This commit doesn't introduce USB support.

> Cc: stable@xxxxxxxxxxxxxxx
> Tested-by: Luka Gejak <luka.gejak@xxxxxxxxx>

How did you test this patch? Have you really encountered leak problem and
fixed it by this patch? Or make this patch by analysis, and test it
without regression?

> Signed-off-by: Luka Gejak <luka.gejak@xxxxxxxxx>
> ---
>  drivers/net/wireless/realtek/rtw88/usb.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
> index 718940ebba31..d430645a3ef3 100644
> --- a/drivers/net/wireless/realtek/rtw88/usb.c
> +++ b/drivers/net/wireless/realtek/rtw88/usb.c
> @@ -456,7 +456,13 @@ static bool rtw_usb_tx_agg_skb(struct rtw_usb *rtwusb, struct sk_buff_head *list
>         tx_desc = (struct rtw_tx_desc *)skb_head->data;
>         qsel = le32_get_bits(tx_desc->w1, RTW_TX_DESC_W1_QSEL);
> 
> -       rtw_usb_write_port(rtwdev, qsel, skb_head, rtw_usb_write_port_tx_complete, txcb);
> +       if (rtw_usb_write_port(rtwdev, qsel, skb_head,
> +                              rtw_usb_write_port_tx_complete, txcb)) {

Can we use 'ret = rtw_usb_write_port(...);' style, and check by next line?
Like the other one modified by this patch.

> +               /* URB submission failed, completion won't run, free the queue */

I think this comment is not necessary. 

> +               skb_queue_purge(&txcb->tx_ack_queue);

Should use ieee80211_purge_tx_queue() since this is TX? 

> +               kfree(txcb);
> +               return false;
> +       }
> 
>         return true;
>  }
> @@ -518,8 +524,10 @@ static int rtw_usb_write_data(struct rtw_dev *rtwdev,
> 
>         ret = rtw_usb_write_port(rtwdev, qsel, skb,
>                                  rtw_usb_write_port_complete, skb);
> -       if (unlikely(ret))
> +       if (unlikely(ret)) {
>                 rtw_err(rtwdev, "failed to do USB write, ret=%d\n", ret);
> +               dev_kfree_skb_any(skb);
> +       }
> 
>         return ret;
>  }
> --
> 2.54.0