RE: [PATCH v2] 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().
> 
> The issue was discovered in practice during device disconnect/reconnect
> scenarios and memory pressure conditions. Tested by verifying normal TX
> operation continues after the fix without regressions.

Did the memory pressure condition happen? and falls into the cases you are
adding? This is main thing I want to know.

> 
> Fixes: 87caeef032fc ("wifi: rtw88: Add rtw8723du chipset support")

I don't find this commit touching the code related to this patch.

> Cc: stable@xxxxxxxxxxxxxxx
> Tested-by: Luka Gejak <luka.gejak@xxxxxxxxx>
> Signed-off-by: Luka Gejak <luka.gejak@xxxxxxxxx>
> ---
> Changes in v2:
>  - Use ret = rtw_usb_write_port(...); style, and check by next line (in
>    rtw_usb_tx_agg_skb)
>  - Remove unnecessary comment
>  - Use ieee80211_purge_tx_queue() instead of skb_queue_purge()

If it falls into the case, you will see some warnings without this change.

Again, I'd like to know if OOM can happen in your test? If not, the test
you are doing will prove nothing, since your changes are executed only if OOM.

>  - Add testing details to commit message
>