Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <CAD-N9QUSMmsCtM3CcbQ4jKwHVuG+woxqGpz3h9ygKvuy73DC0Q@mail.gmail.com>
In-Reply-To: <CAD-N9QUSMmsCtM3CcbQ4jKwHVuG+woxqGpz3h9ygKvuy73DC0Q@mail.gmail.com>
From:   Dmitry Vyukov <dvyukov@google.com>
Date:   Mon, 7 Jun 2021 07:25:09 +0200
Message-ID: <CACT4Y+Yk614rYkVBF2k8toGM5fOuoMjBqGMtNjnKfsBxCVS6XA@mail.gmail.com>
Subject: Re: Comment on the patch of "memory leak in ath9k_hif_usb_firmware_cb"
To:     Dongliang Mu <mudongliangabcd@gmail.com>
Cc:     syzbot <syzbot+6692c72009680f7c4eb2@syzkaller.appspotmail.com>,
        ath9k-devel@qca.qualcomm.com,
        linux-wireless <linux-wireless@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Jun 7, 2021 at 5:58 AM Dongliang Mu <mudongliangabcd@gmail.com> 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