| Message ID | 20210709084351.2087311-1-mudongliangabcd@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | ath9k: hif_usb: fix memory leak in ath9k_hif_usb_firmware_cb | expand |
On Fri, Jul 9, 2021 at 4:44 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote: > > The commit 03fb92a432ea ("ath9k: hif_usb: fix race condition between > usb_get_urb() and usb_kill_anchored_urbs()") adds three usb_get_urb > in ath9k_hif_usb_dealloc_tx_urbs and usb_free_urb. > > Fix this bug by adding corresponding usb_free_urb in > ath9k_hif_usb_dealloc_tx_urbs other and hif_usb_stop. > Any idea about this patch? > Reported-by: syzbot+6692c72009680f7c4eb2@syzkaller.appspotmail.com > Fixes: 03fb92a432ea ("ath9k: hif_usb: fix race condition between usb_get_urb() and usb_kill_anchored_urbs()") > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > --- > drivers/net/wireless/ath/ath9k/hif_usb.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c > index 860da13bfb6a..bda91ff3289b 100644 > --- a/drivers/net/wireless/ath/ath9k/hif_usb.c > +++ b/drivers/net/wireless/ath/ath9k/hif_usb.c > @@ -457,6 +457,7 @@ static void hif_usb_stop(void *hif_handle) > usb_kill_urb(tx_buf->urb); > list_del(&tx_buf->list); > usb_free_urb(tx_buf->urb); > + usb_free_urb(tx_buf->urb); > kfree(tx_buf->buf); > kfree(tx_buf); > spin_lock_irqsave(&hif_dev->tx.tx_lock, flags); > @@ -779,6 +780,7 @@ static void ath9k_hif_usb_dealloc_tx_urbs(struct hif_device_usb *hif_dev) > usb_kill_urb(tx_buf->urb); > list_del(&tx_buf->list); > usb_free_urb(tx_buf->urb); > + usb_free_urb(tx_buf->urb); > kfree(tx_buf->buf); > kfree(tx_buf); > spin_lock_irqsave(&hif_dev->tx.tx_lock, flags); > @@ -797,6 +799,7 @@ static void ath9k_hif_usb_dealloc_tx_urbs(struct hif_device_usb *hif_dev) > usb_kill_urb(tx_buf->urb); > list_del(&tx_buf->list); > usb_free_urb(tx_buf->urb); > + usb_free_urb(tx_buf->urb); > kfree(tx_buf->buf); > kfree(tx_buf); > spin_lock_irqsave(&hif_dev->tx.tx_lock, flags); > -- > 2.25.1 >
Dongliang Mu <mudongliangabcd@gmail.com> writes: > The commit 03fb92a432ea ("ath9k: hif_usb: fix race condition between > usb_get_urb() and usb_kill_anchored_urbs()") adds three usb_get_urb > in ath9k_hif_usb_dealloc_tx_urbs and usb_free_urb. > > Fix this bug by adding corresponding usb_free_urb in > ath9k_hif_usb_dealloc_tx_urbs other and hif_usb_stop. > > Reported-by: syzbot+6692c72009680f7c4eb2@syzkaller.appspotmail.com > Fixes: 03fb92a432ea ("ath9k: hif_usb: fix race condition between usb_get_urb() and usb_kill_anchored_urbs()") > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> In the past there have been so many problems with ath9k syzbot fixes that I have now a hard time trusting them. Can someone review this in detail, please?
Hi Dongliang, (Drive-by review, I know almost nothing about the code in question) On Fri, Jul 9, 2021 at 6:47 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote: > > The commit 03fb92a432ea ("ath9k: hif_usb: fix race condition between > usb_get_urb() and usb_kill_anchored_urbs()") adds three usb_get_urb > in ath9k_hif_usb_dealloc_tx_urbs and usb_free_urb. > > Fix this bug by adding corresponding usb_free_urb in > ath9k_hif_usb_dealloc_tx_urbs other and hif_usb_stop. > > Reported-by: syzbot+6692c72009680f7c4eb2@syzkaller.appspotmail.com > Fixes: 03fb92a432ea ("ath9k: hif_usb: fix race condition between usb_get_urb() and usb_kill_anchored_urbs()") > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > --- > drivers/net/wireless/ath/ath9k/hif_usb.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c > index 860da13bfb6a..bda91ff3289b 100644 > --- a/drivers/net/wireless/ath/ath9k/hif_usb.c > +++ b/drivers/net/wireless/ath/ath9k/hif_usb.c > @@ -457,6 +457,7 @@ static void hif_usb_stop(void *hif_handle) > usb_kill_urb(tx_buf->urb); > list_del(&tx_buf->list); > usb_free_urb(tx_buf->urb); > + usb_free_urb(tx_buf->urb); Ok, so if I'm reading this correctly, before the first usb_free_urb() call, we have two references to the urb at tx_buf->urb. Why? Isn't the better fix here to detangle why there's more than one reference to it and resolve it that way? This looks like a hack to fix something much more fundamentally broken. Thanks,
Julian Calaby <julian.calaby@gmail.com> writes: > Hi Dongliang, > > (Drive-by review, I know almost nothing about the code in question) > > On Fri, Jul 9, 2021 at 6:47 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote: >> >> The commit 03fb92a432ea ("ath9k: hif_usb: fix race condition between >> usb_get_urb() and usb_kill_anchored_urbs()") adds three usb_get_urb >> in ath9k_hif_usb_dealloc_tx_urbs and usb_free_urb. >> >> Fix this bug by adding corresponding usb_free_urb in >> ath9k_hif_usb_dealloc_tx_urbs other and hif_usb_stop. >> >> Reported-by: syzbot+6692c72009680f7c4eb2@syzkaller.appspotmail.com >> Fixes: 03fb92a432ea ("ath9k: hif_usb: fix race condition between usb_get_urb() and usb_kill_anchored_urbs()") >> Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> >> --- >> drivers/net/wireless/ath/ath9k/hif_usb.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c >> index 860da13bfb6a..bda91ff3289b 100644 >> --- a/drivers/net/wireless/ath/ath9k/hif_usb.c >> +++ b/drivers/net/wireless/ath/ath9k/hif_usb.c >> @@ -457,6 +457,7 @@ static void hif_usb_stop(void *hif_handle) >> usb_kill_urb(tx_buf->urb); >> list_del(&tx_buf->list); >> usb_free_urb(tx_buf->urb); >> + usb_free_urb(tx_buf->urb); > > Ok, so if I'm reading this correctly, before the first usb_free_urb() > call, we have two references to the urb at tx_buf->urb. > > Why? > > Isn't the better fix here to detangle why there's more than one > reference to it and resolve it that way? This looks like a hack to fix > something much more fundamentally broken. Yeah, this looks very suspicious. One more thing: also the patch should be tested with real hardware. I'm worried that people are just trying to fix a syzbot warning and not really considering how it works in real life. That's why I'm extra careful with syzbot patches for wireless drivers.
diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c index 860da13bfb6a..bda91ff3289b 100644 --- a/drivers/net/wireless/ath/ath9k/hif_usb.c +++ b/drivers/net/wireless/ath/ath9k/hif_usb.c @@ -457,6 +457,7 @@ static void hif_usb_stop(void *hif_handle) usb_kill_urb(tx_buf->urb); list_del(&tx_buf->list); usb_free_urb(tx_buf->urb); + usb_free_urb(tx_buf->urb); kfree(tx_buf->buf); kfree(tx_buf); spin_lock_irqsave(&hif_dev->tx.tx_lock, flags); @@ -779,6 +780,7 @@ static void ath9k_hif_usb_dealloc_tx_urbs(struct hif_device_usb *hif_dev) usb_kill_urb(tx_buf->urb); list_del(&tx_buf->list); usb_free_urb(tx_buf->urb); + usb_free_urb(tx_buf->urb); kfree(tx_buf->buf); kfree(tx_buf); spin_lock_irqsave(&hif_dev->tx.tx_lock, flags); @@ -797,6 +799,7 @@ static void ath9k_hif_usb_dealloc_tx_urbs(struct hif_device_usb *hif_dev) usb_kill_urb(tx_buf->urb); list_del(&tx_buf->list); usb_free_urb(tx_buf->urb); + usb_free_urb(tx_buf->urb); kfree(tx_buf->buf); kfree(tx_buf); spin_lock_irqsave(&hif_dev->tx.tx_lock, flags);
The commit 03fb92a432ea ("ath9k: hif_usb: fix race condition between usb_get_urb() and usb_kill_anchored_urbs()") adds three usb_get_urb in ath9k_hif_usb_dealloc_tx_urbs and usb_free_urb. Fix this bug by adding corresponding usb_free_urb in ath9k_hif_usb_dealloc_tx_urbs other and hif_usb_stop. Reported-by: syzbot+6692c72009680f7c4eb2@syzkaller.appspotmail.com Fixes: 03fb92a432ea ("ath9k: hif_usb: fix race condition between usb_get_urb() and usb_kill_anchored_urbs()") Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> --- drivers/net/wireless/ath/ath9k/hif_usb.c | 3 +++ 1 file changed, 3 insertions(+)