Re: Comment on the patch of "memory leak in ath9k_hif_usb_firmware_cb"

On Mon, Jun 7, 2021 at 5:58 AM Dongliang Mu [off-list ref] wrote:

Hi Dmitry,

I saw you have tested one patch [1] for "memory leak in
ath9k_hif_usb_firmware_cb". And it does not trigger any bugs on the
patched version. This is great. However, I find there are other
similar code patterns in the same file below. Shall we fix other sites
as well in the same patch?

Hi Dongliang,

Sure, if there are more bugs, it would be good to fix them.
It's always a good idea to check for similar code around when a bug is fixed.

static void ath9k_hif_usb_dealloc_tx_urbs(struct hif_device_usb *hif_dev) [2]

list_for_each_entry_safe(tx_buf, tx_buf_tmp,
&hif_dev->tx.tx_buf, list) {
usb_get_urb(tx_buf->urb);
spin_unlock_irqrestore(&hif_dev->tx.tx_lock, flags);
usb_kill_urb(tx_buf->urb);
list_del(&tx_buf->list);
usb_free_urb(tx_buf->urb);
kfree(tx_buf->buf);
kfree(tx_buf);
spin_lock_irqsave(&hif_dev->tx.tx_lock, flags);
}

list_for_each_entry_safe(tx_buf, tx_buf_tmp,
&hif_dev->tx.tx_pending, list) {
usb_get_urb(tx_buf->urb);
spin_unlock_irqrestore(&hif_dev->tx.tx_lock, flags);
usb_kill_urb(tx_buf->urb);
list_del(&tx_buf->list);
usb_free_urb(tx_buf->urb);
kfree(tx_buf->buf);
kfree(tx_buf);
spin_lock_irqsave(&hif_dev->tx.tx_lock, flags);
}

static void hif_usb_stop(void *hif_handle) [3]

list_for_each_entry_safe(tx_buf, tx_buf_tmp,
&hif_dev->tx.tx_pending, list) {
usb_get_urb(tx_buf->urb);
spin_unlock_irqrestore(&hif_dev->tx.tx_lock, flags);
usb_kill_urb(tx_buf->urb);
list_del(&tx_buf->list);
usb_free_urb(tx_buf->urb);
kfree(tx_buf->buf);
kfree(tx_buf);
spin_lock_irqsave(&hif_dev->tx.tx_lock, flags);
}

[1] https://syzkaller.appspot.com/text?tag=Patch&x=14107bbdd00000
[2] https://elixir.bootlin.com/linux/latest/source/drivers/net/wireless/ath/ath9k/hif_usb.c#L769
[3] https://elixir.bootlin.com/linux/latest/source/drivers/net/wireless/ath/ath9k/hif_usb.c#L439

--
My best regards to you.

     No System Is Safe!
     Dongliang Mu