LKML Archive on lore.kernel.org
 help / color / Atom feed
* INFO: trying to register non-static key in del_timer_sync (2)
@ 2019-04-12 14:26 syzbot
  2019-06-01 17:52 ` [EXT] " Ganapathi Bhat
  0 siblings, 1 reply; 15+ messages in thread
From: syzbot @ 2019-04-12 14:26 UTC (permalink / raw)
  To: amitkarwar, andreyknvl, davem, gbhat, huxinming820, kvalo,
	linux-kernel, linux-usb, linux-wireless, netdev, nishants,
	syzkaller-bugs

Hello,

syzbot found the following crash on:

HEAD commit:    9a33b369 usb-fuzzer: main usb gadget fuzzer driver
git tree:       https://github.com/google/kasan/tree/usb-fuzzer
console output: https://syzkaller.appspot.com/x/log.txt?x=14793fa7200000
kernel config:  https://syzkaller.appspot.com/x/.config?x=23e37f59d94ddd15
dashboard link: https://syzkaller.appspot.com/bug?extid=dc4127f950da51639216
compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=16f8c22d200000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=16eeadbb200000

IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+dc4127f950da51639216@syzkaller.appspotmail.com

usb 1-1: string descriptor 0 read error: -71
usb 1-1: USB disconnect, device number 2
usb 1-1: Direct firmware load for mrvl/usb8801_uapsta.bin failed with error  
-2
usb 1-1: Failed to get firmware mrvl/usb8801_uapsta.bin
usb 1-1: info: _mwifiex_fw_dpc: unregister device
INFO: trying to register non-static key.
the code is fine but needs lockdep annotation.
turning off the locking correctness validator.
CPU: 0 PID: 12 Comm: kworker/0:1 Not tainted 5.1.0-rc4-319354-g9a33b36 #3
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS  
Google 01/01/2011
Workqueue: events request_firmware_work_func
Call Trace:
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0xe8/0x16e lib/dump_stack.c:113
  assign_lock_key kernel/locking/lockdep.c:786 [inline]
  register_lock_class+0x11b8/0x1250 kernel/locking/lockdep.c:1095
  __lock_acquire+0xfb/0x37c0 kernel/locking/lockdep.c:3582
  lock_acquire+0x10d/0x2f0 kernel/locking/lockdep.c:4211
  del_timer_sync+0x4c/0x150 kernel/time/timer.c:1282
  mwifiex_usb_cleanup_tx_aggr  
drivers/net/wireless/marvell/mwifiex/usb.c:1358 [inline]
  mwifiex_unregister_dev+0x41b/0x690  
drivers/net/wireless/marvell/mwifiex/usb.c:1370
  _mwifiex_fw_dpc+0x711/0xdd0 drivers/net/wireless/marvell/mwifiex/main.c:651
  request_firmware_work_func+0x12d/0x249  
drivers/base/firmware_loader/main.c:785
  process_one_work+0x90f/0x1580 kernel/workqueue.c:2269
  worker_thread+0x9b/0xe20 kernel/workqueue.c:2415
  kthread+0x313/0x420 kernel/kthread.c:253
  ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:352
------------[ cut here ]------------
ODEBUG: assert_init not available (active state 0) object type: timer_list  
hint:           (null)
WARNING: CPU: 0 PID: 12 at lib/debugobjects.c:325  
debug_print_object+0x162/0x250 lib/debugobjects.c:325
Kernel panic - not syncing: panic_on_warn set ...
CPU: 0 PID: 12 Comm: kworker/0:1 Not tainted 5.1.0-rc4-319354-g9a33b36 #3
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS  
Google 01/01/2011
Workqueue: events request_firmware_work_func
Call Trace:
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0xe8/0x16e lib/dump_stack.c:113
  panic+0x29d/0x5f2 kernel/panic.c:214
  __warn.cold+0x20/0x48 kernel/panic.c:571
  report_bug+0x262/0x2a0 lib/bug.c:186
  fixup_bug arch/x86/kernel/traps.c:179 [inline]
  fixup_bug arch/x86/kernel/traps.c:174 [inline]
  do_error_trap+0x130/0x1f0 arch/x86/kernel/traps.c:272
  do_invalid_op+0x37/0x40 arch/x86/kernel/traps.c:291
  invalid_op+0x14/0x20 arch/x86/entry/entry_64.S:973
RIP: 0010:debug_print_object+0x162/0x250 lib/debugobjects.c:325
Code: dd e0 a1 b3 8e 48 89 fa 48 c1 ea 03 80 3c 02 00 0f 85 bf 00 00 00 48  
8b 14 dd e0 a1 b3 8e 48 c7 c7 60 96 b3 8e e8 8e 93 d2 fd <0f> 0b 83 05 e9  
d6 59 10 01 48 83 c4 20 5b 5d 41 5c 41 5d c3 48 89
RSP: 0018:ffff8880a84b78d8 EFLAGS: 00010286
RAX: 0000000000000000 RBX: 0000000000000005 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffffffff815b1e42 RDI: ffffed1015096f0d
RBP: 0000000000000001 R08: ffff8880a849b100 R09: fffffbfff22f95ed
R10: fffffbfff22f95ec R11: ffffffff917caf63 R12: ffffffff917e7780
R13: ffffffff8161ec90 R14: 1ffff11015096f28 R15: ffff88809fc893f8
  debug_object_assert_init lib/debugobjects.c:694 [inline]
  debug_object_assert_init+0x23d/0x2f0 lib/debugobjects.c:665
  debug_timer_assert_init kernel/time/timer.c:723 [inline]
  debug_assert_init kernel/time/timer.c:775 [inline]
  try_to_del_timer_sync+0x72/0x110 kernel/time/timer.c:1222
  del_timer_sync+0x112/0x150 kernel/time/timer.c:1292
  mwifiex_usb_cleanup_tx_aggr  
drivers/net/wireless/marvell/mwifiex/usb.c:1358 [inline]
  mwifiex_unregister_dev+0x41b/0x690  
drivers/net/wireless/marvell/mwifiex/usb.c:1370
  _mwifiex_fw_dpc+0x711/0xdd0 drivers/net/wireless/marvell/mwifiex/main.c:651
  request_firmware_work_func+0x12d/0x249  
drivers/base/firmware_loader/main.c:785
  process_one_work+0x90f/0x1580 kernel/workqueue.c:2269
  worker_thread+0x9b/0xe20 kernel/workqueue.c:2415
  kthread+0x313/0x420 kernel/kthread.c:253
  ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:352
Kernel Offset: disabled
Rebooting in 86400 seconds..


---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches

^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [EXT] INFO: trying to register non-static key in del_timer_sync (2)
  2019-04-12 14:26 INFO: trying to register non-static key in del_timer_sync (2) syzbot
@ 2019-06-01 17:52 ` Ganapathi Bhat
  2019-06-03  5:20   ` Dmitry Vyukov
  0 siblings, 1 reply; 15+ messages in thread
From: Ganapathi Bhat @ 2019-06-01 17:52 UTC (permalink / raw)
  To: syzbot, amitkarwar, andreyknvl, davem, huxinming820, kvalo,
	linux-kernel, linux-usb, linux-wireless, netdev, nishants,
	syzkaller-bugs

Hi syzbot,

> 
> syzbot found the following crash on:
> 
As per the link(https://syzkaller.appspot.com/bug?extid=dc4127f950da51639216), the issue is fixed; Is it OK? Let us know if we need to do something?

Regards,
Ganapathi

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [EXT] INFO: trying to register non-static key in del_timer_sync (2)
  2019-06-01 17:52 ` [EXT] " Ganapathi Bhat
@ 2019-06-03  5:20   ` Dmitry Vyukov
  2019-06-03  8:41     ` Ganapathi Bhat
  0 siblings, 1 reply; 15+ messages in thread
From: Dmitry Vyukov @ 2019-06-03  5:20 UTC (permalink / raw)
  To: Ganapathi Bhat
  Cc: syzbot, amitkarwar, andreyknvl, davem, huxinming820, kvalo,
	linux-kernel, linux-usb, linux-wireless, netdev, nishants,
	syzkaller-bugs

On Sat, Jun 1, 2019 at 7:52 PM Ganapathi Bhat <gbhat@marvell.com> wrote:
>
> Hi syzbot,
>
> >
> > syzbot found the following crash on:
> >
> As per the link(https://syzkaller.appspot.com/bug?extid=dc4127f950da51639216), the issue is fixed; Is it OK? Let us know if we need to do something?

Hi Ganapathi,

The "fixed" status relates to the similar past bug that was reported
and fixed more than a year ago:
https://groups.google.com/forum/#!msg/syzkaller-bugs/3YnGX1chF2w/jeQjeihtBAAJ
https://syzkaller.appspot.com/bug?id=b4b5c74c57c4b69f4fff86131abb799106182749

This one is still well alive and kicking, with 1200+ crashes and the
last one happened less then 30min ago.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [EXT] INFO: trying to register non-static key in del_timer_sync (2)
  2019-06-03  5:20   ` Dmitry Vyukov
@ 2019-06-03  8:41     ` Ganapathi Bhat
  2019-06-12 16:01       ` Ganapathi Bhat
  0 siblings, 1 reply; 15+ messages in thread
From: Ganapathi Bhat @ 2019-06-03  8:41 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: syzbot, amitkarwar, andreyknvl, davem, huxinming820, kvalo,
	linux-kernel, linux-usb, linux-wireless, netdev, nishants,
	syzkaller-bugs

Hi Dmitry,

> The "fixed" status relates to the similar past bug that was reported and fixed
> more than a year ago:
Oh OK; We understood the issue, working on a change to fix this;

Thanks,
Ganapathi

^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [EXT] INFO: trying to register non-static key in del_timer_sync (2)
  2019-06-03  8:41     ` Ganapathi Bhat
@ 2019-06-12 16:01       ` Ganapathi Bhat
  2019-06-12 16:13         ` Andrey Konovalov
  2019-08-13 13:36         ` [EXT] " Andrey Konovalov
  0 siblings, 2 replies; 15+ messages in thread
From: Ganapathi Bhat @ 2019-06-12 16:01 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: syzbot, amitkarwar, andreyknvl, davem, huxinming820, kvalo,
	linux-kernel, linux-usb, linux-wireless, netdev, nishants,
	syzkaller-bugs

Hi Dmitry,

We have a patch to fix this: https://patchwork.kernel.org/patch/10990275/

Regards,
Ganapathi

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [EXT] INFO: trying to register non-static key in del_timer_sync (2)
  2019-06-12 16:01       ` Ganapathi Bhat
@ 2019-06-12 16:13         ` Andrey Konovalov
  2019-06-12 16:59           ` syzbot
  2019-08-13 13:36         ` [EXT] " Andrey Konovalov
  1 sibling, 1 reply; 15+ messages in thread
From: Andrey Konovalov @ 2019-06-12 16:13 UTC (permalink / raw)
  To: Ganapathi Bhat
  Cc: Dmitry Vyukov, syzbot, amitkarwar, davem, huxinming820, kvalo,
	linux-kernel, linux-usb, linux-wireless, netdev, nishants,
	syzkaller-bugs


[-- Attachment #1: Type: text/plain, Size: 348 bytes --]

On Wed, Jun 12, 2019 at 6:03 PM Ganapathi Bhat <gbhat@marvell.com> wrote:
>
> Hi Dmitry,
>
> We have a patch to fix this: https://patchwork.kernel.org/patch/10990275/

Hi Ganapathi,

Great, thanks for working on this!

We can ask syzbot to test the fix:

#syz test: https://github.com/google/kasan.git usb-fuzzer

Thanks!

>
> Regards,
> Ganapathi

[-- Attachment #2: mwifiex-avoid-deleting-uninitialized-timer-during-USB-cleanup.diff --]
[-- Type: text/x-patch, Size: 2079 bytes --]

diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
index c2365ee..939f1e9 100644
--- a/drivers/net/wireless/marvell/mwifiex/usb.c
+++ b/drivers/net/wireless/marvell/mwifiex/usb.c
@@ -1348,6 +1348,8 @@ static void mwifiex_usb_cleanup_tx_aggr(struct mwifiex_adapter *adapter)
 
 	for (idx = 0; idx < MWIFIEX_TX_DATA_PORT; idx++) {
 		port = &card->port[idx];
+		if (!port->tx_data_ep)
+			continue;
 		if (adapter->bus_aggr.enable)
 			while ((skb_tmp =
 				skb_dequeue(&port->tx_aggr.aggr_list)))
@@ -1365,8 +1367,6 @@ static void mwifiex_unregister_dev(struct mwifiex_adapter *adapter)
 
 	mwifiex_usb_free(card);
 
-	mwifiex_usb_cleanup_tx_aggr(adapter);
-
 	card->adapter = NULL;
 }
 
@@ -1510,7 +1510,7 @@ static int mwifiex_prog_fw_w_helper(struct mwifiex_adapter *adapter,
 static int mwifiex_usb_dnld_fw(struct mwifiex_adapter *adapter,
 			struct mwifiex_fw_image *fw)
 {
-	int ret;
+	int ret = 0;
 	struct usb_card_rec *card = (struct usb_card_rec *)adapter->card;
 
 	if (card->usb_boot_state == USB8XXX_FW_DNLD) {
@@ -1523,10 +1523,6 @@ static int mwifiex_usb_dnld_fw(struct mwifiex_adapter *adapter,
 			return -1;
 	}
 
-	ret = mwifiex_usb_rx_init(adapter);
-	if (!ret)
-		ret = mwifiex_usb_tx_init(adapter);
-
 	return ret;
 }
 
@@ -1584,7 +1580,29 @@ static void mwifiex_usb_submit_rem_rx_urbs(struct mwifiex_adapter *adapter)
 	return 0;
 }
 
+static int mwifiex_init_usb(struct mwifiex_adapter *adapter)
+{
+	struct usb_card_rec *card = (struct usb_card_rec *)adapter->card;
+	int ret = 0;
+
+	if (card->usb_boot_state == USB8XXX_FW_DNLD)
+		return 0;
+
+	ret = mwifiex_usb_rx_init(adapter);
+	if (!ret)
+		ret = mwifiex_usb_tx_init(adapter);
+
+	return ret;
+}
+
+static void mwifiex_cleanup_usb(struct mwifiex_adapter *adapter)
+{
+	mwifiex_usb_cleanup_tx_aggr(adapter);
+}
+
 static struct mwifiex_if_ops usb_ops = {
+	.init_if =		mwifiex_init_usb,
+	.cleanup_if =		mwifiex_cleanup_usb,
 	.register_dev =		mwifiex_register_dev,
 	.unregister_dev =	mwifiex_unregister_dev,
 	.wakeup =		mwifiex_pm_wakeup_card,

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: INFO: trying to register non-static key in del_timer_sync (2)
  2019-06-12 16:13         ` Andrey Konovalov
@ 2019-06-12 16:59           ` syzbot
  0 siblings, 0 replies; 15+ messages in thread
From: syzbot @ 2019-06-12 16:59 UTC (permalink / raw)
  To: amitkarwar, andreyknvl, davem, dvyukov, gbhat, huxinming820,
	kvalo, linux-kernel, linux-usb, linux-wireless, netdev, nishants,
	syzkaller-bugs

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger  
crash:

Reported-and-tested-by:  
syzbot+dc4127f950da51639216@syzkaller.appspotmail.com

Tested on:

commit:         69bbe8c7 usb-fuzzer: main usb gadget fuzzer driver
git tree:       https://github.com/google/kasan.git usb-fuzzer
kernel config:  https://syzkaller.appspot.com/x/.config?x=39290eb0151bec39
compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
patch:          https://syzkaller.appspot.com/x/patch.diff?x=14171fd2a00000

Note: testing is done by a robot and is best-effort only.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [EXT] INFO: trying to register non-static key in del_timer_sync (2)
  2019-06-12 16:01       ` Ganapathi Bhat
  2019-06-12 16:13         ` Andrey Konovalov
@ 2019-08-13 13:36         ` Andrey Konovalov
  2019-08-13 13:58           ` Kalle Valo
  1 sibling, 1 reply; 15+ messages in thread
From: Andrey Konovalov @ 2019-08-13 13:36 UTC (permalink / raw)
  To: Ganapathi Bhat
  Cc: Dmitry Vyukov, syzbot, amitkarwar, davem, huxinming820, kvalo,
	linux-kernel, linux-usb, linux-wireless, netdev, nishants,
	syzkaller-bugs

On Wed, Jun 12, 2019 at 6:03 PM Ganapathi Bhat <gbhat@marvell.com> wrote:
>
> Hi Dmitry,
>
> We have a patch to fix this: https://patchwork.kernel.org/patch/10990275/

Hi Ganapathi,

Has this patch been accepted anywhere? This bug is still open on syzbot.

Thanks!

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [EXT] INFO: trying to register non-static key in del_timer_sync (2)
  2019-08-13 13:36         ` [EXT] " Andrey Konovalov
@ 2019-08-13 13:58           ` Kalle Valo
  2019-08-14 14:08             ` Ganapathi Bhat
  0 siblings, 1 reply; 15+ messages in thread
From: Kalle Valo @ 2019-08-13 13:58 UTC (permalink / raw)
  To: Andrey Konovalov
  Cc: Ganapathi Bhat, Dmitry Vyukov, syzbot, amitkarwar, davem,
	huxinming820, linux-kernel, linux-usb, linux-wireless, netdev,
	nishants, syzkaller-bugs

Andrey Konovalov <andreyknvl@google.com> writes:

> On Wed, Jun 12, 2019 at 6:03 PM Ganapathi Bhat <gbhat@marvell.com> wrote:
>>
>> Hi Dmitry,
>>
>> We have a patch to fix this: https://patchwork.kernel.org/patch/10990275/
>
> Hi Ganapathi,
>
> Has this patch been accepted anywhere? This bug is still open on syzbot.

The patch is in "Changes Requested" state which means that the author is
supposed to send a new version based on the review comments.

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [EXT] INFO: trying to register non-static key in del_timer_sync (2)
  2019-08-13 13:58           ` Kalle Valo
@ 2019-08-14 14:08             ` Ganapathi Bhat
  2019-10-01 16:40               ` Andrey Konovalov
  0 siblings, 1 reply; 15+ messages in thread
From: Ganapathi Bhat @ 2019-08-14 14:08 UTC (permalink / raw)
  To: Kalle Valo, Andrey Konovalov
  Cc: Dmitry Vyukov, syzbot, amitkarwar, davem, huxinming820,
	linux-kernel, linux-usb, linux-wireless, netdev, nishants,
	syzkaller-bugs

Hi Dmitry/Kalle,

> >>
> >> Hi Dmitry,
> >>
> >> We have a patch to fix this:
> >> https://patchwork.kernel.org/patch/10990275/
> >
> > Hi Ganapathi,
> >
> > Has this patch been accepted anywhere? This bug is still open on syzbot.
> 
> The patch is in "Changes Requested" state which means that the author is
> supposed to send a new version based on the review comments.
We will address the review comments and try to push the updated version soon;

Regards,
Ganapathi

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [EXT] INFO: trying to register non-static key in del_timer_sync (2)
  2019-08-14 14:08             ` Ganapathi Bhat
@ 2019-10-01 16:40               ` Andrey Konovalov
  2019-10-02 14:28                 ` Ganapathi Bhat
  0 siblings, 1 reply; 15+ messages in thread
From: Andrey Konovalov @ 2019-10-01 16:40 UTC (permalink / raw)
  To: Ganapathi Bhat
  Cc: Kalle Valo, Dmitry Vyukov, syzbot, amitkarwar, davem,
	huxinming820, linux-kernel, linux-usb, linux-wireless, netdev,
	nishants, syzkaller-bugs

On Wed, Aug 14, 2019 at 4:08 PM Ganapathi Bhat <gbhat@marvell.com> wrote:
>
> Hi Dmitry/Kalle,
>
> > >>
> > >> Hi Dmitry,
> > >>
> > >> We have a patch to fix this:
> > >> https://patchwork.kernel.org/patch/10990275/
> > >
> > > Hi Ganapathi,
> > >
> > > Has this patch been accepted anywhere? This bug is still open on syzbot.
> >
> > The patch is in "Changes Requested" state which means that the author is
> > supposed to send a new version based on the review comments.
> We will address the review comments and try to push the updated version soon;

Hi Ganapathi,

I was wondering if you've posted the updated version of the fix?

Thanks!

^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [EXT] INFO: trying to register non-static key in del_timer_sync (2)
  2019-10-01 16:40               ` Andrey Konovalov
@ 2019-10-02 14:28                 ` Ganapathi Bhat
  2020-07-28  1:44                   ` [PATCH] mwifiex: don't call del_timer_sync() on uninitialized timer Tetsuo Handa
  0 siblings, 1 reply; 15+ messages in thread
From: Ganapathi Bhat @ 2019-10-02 14:28 UTC (permalink / raw)
  To: Andrey Konovalov
  Cc: Kalle Valo, Dmitry Vyukov, syzbot, amitkarwar, davem,
	huxinming820, linux-kernel, linux-usb, linux-wireless, netdev,
	nishants, syzkaller-bugs

Hi Andy,

> I was wondering if you've posted the updated version of the fix?

Sorry for the delay; I have started addressing the comment from community; It should be done in couple of days;
Regards,
Ganapathi

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH] mwifiex: don't call del_timer_sync() on uninitialized timer
  2019-10-02 14:28                 ` Ganapathi Bhat
@ 2020-07-28  1:44                   ` Tetsuo Handa
  2020-07-28 17:29                     ` Andy Shevchenko
  2020-07-28 18:45                     ` Brian Norris
  0 siblings, 2 replies; 15+ messages in thread
From: Tetsuo Handa @ 2020-07-28  1:44 UTC (permalink / raw)
  To: gbhat
  Cc: amitkarwar, andreyknvl, davem, dvyukov, huxinming820, kvalo,
	linux-kernel, linux-usb, linux-wireless, netdev, nishants,
	syzbot+dc4127f950da51639216, syzkaller-bugs, Tetsuo Handa,
	syzbot

syzbot is reporting that del_timer_sync() is called from
mwifiex_usb_cleanup_tx_aggr() from mwifiex_unregister_dev() without
checking timer_setup() from mwifiex_usb_tx_init() was called [1].
Since mwifiex_usb_prepare_tx_aggr_skb() is calling del_timer() if
is_hold_timer_set == true, use the same condition for del_timer_sync().

[1] https://syzkaller.appspot.com/bug?id=fdeef9cf7348be8b8ab5b847f2ed993aba8ea7b6

Reported-by: syzbot <syzbot+373e6719b49912399d21@syzkaller.appspotmail.com>
Cc: Ganapathi Bhat <gbhat@marvell.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
A patch from Ganapathi Bhat ( https://patchwork.kernel.org/patch/10990275/ ) is stalling
at https://lore.kernel.org/linux-usb/MN2PR18MB2637D7C742BC235FE38367F0A09C0@MN2PR18MB2637.namprd18.prod.outlook.com/ .
syzbot by now got this report for 10000 times. Do we want to go with this simple patch?

 drivers/net/wireless/marvell/mwifiex/usb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
index 6f3cfde..04a1461 100644
--- a/drivers/net/wireless/marvell/mwifiex/usb.c
+++ b/drivers/net/wireless/marvell/mwifiex/usb.c
@@ -1353,7 +1353,8 @@ static void mwifiex_usb_cleanup_tx_aggr(struct mwifiex_adapter *adapter)
 				skb_dequeue(&port->tx_aggr.aggr_list)))
 				mwifiex_write_data_complete(adapter, skb_tmp,
 							    0, -1);
-		del_timer_sync(&port->tx_aggr.timer_cnxt.hold_timer);
+		if (port->tx_aggr.timer_cnxt.is_hold_timer_set)
+			del_timer_sync(&port->tx_aggr.timer_cnxt.hold_timer);
 		port->tx_aggr.timer_cnxt.is_hold_timer_set = false;
 		port->tx_aggr.timer_cnxt.hold_tmo_msecs = 0;
 	}
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] mwifiex: don't call del_timer_sync() on uninitialized timer
  2020-07-28  1:44                   ` [PATCH] mwifiex: don't call del_timer_sync() on uninitialized timer Tetsuo Handa
@ 2020-07-28 17:29                     ` Andy Shevchenko
  2020-07-28 18:45                     ` Brian Norris
  1 sibling, 0 replies; 15+ messages in thread
From: Andy Shevchenko @ 2020-07-28 17:29 UTC (permalink / raw)
  To: Tetsuo Handa
  Cc: gbhat, amitkarwar, andreyknvl, David S. Miller, Dmitry Vyukov,
	huxinming820, Kalle Valo, Linux Kernel Mailing List, USB,
	open list:TI WILINK WIRELES...,
	netdev, Nishant Sarmukadam, syzbot+dc4127f950da51639216,
	syzkaller-bugs, syzbot

On Tue, Jul 28, 2020 at 4:46 AM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> syzbot is reporting that del_timer_sync() is called from
> mwifiex_usb_cleanup_tx_aggr() from mwifiex_unregister_dev() without
> checking timer_setup() from mwifiex_usb_tx_init() was called [1].
> Since mwifiex_usb_prepare_tx_aggr_skb() is calling del_timer() if
> is_hold_timer_set == true, use the same condition for del_timer_sync().
>
> [1] https://syzkaller.appspot.com/bug?id=fdeef9cf7348be8b8ab5b847f2ed993aba8ea7b6
>

Can you use BugLink: tag for above?

> Reported-by: syzbot <syzbot+373e6719b49912399d21@syzkaller.appspotmail.com>
> Cc: Ganapathi Bhat <gbhat@marvell.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
> A patch from Ganapathi Bhat ( https://patchwork.kernel.org/patch/10990275/ ) is stalling
> at https://lore.kernel.org/linux-usb/MN2PR18MB2637D7C742BC235FE38367F0A09C0@MN2PR18MB2637.namprd18.prod.outlook.com/ .
> syzbot by now got this report for 10000 times. Do we want to go with this simple patch?
>
>  drivers/net/wireless/marvell/mwifiex/usb.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
> index 6f3cfde..04a1461 100644
> --- a/drivers/net/wireless/marvell/mwifiex/usb.c
> +++ b/drivers/net/wireless/marvell/mwifiex/usb.c
> @@ -1353,7 +1353,8 @@ static void mwifiex_usb_cleanup_tx_aggr(struct mwifiex_adapter *adapter)
>                                 skb_dequeue(&port->tx_aggr.aggr_list)))
>                                 mwifiex_write_data_complete(adapter, skb_tmp,
>                                                             0, -1);
> -               del_timer_sync(&port->tx_aggr.timer_cnxt.hold_timer);
> +               if (port->tx_aggr.timer_cnxt.is_hold_timer_set)
> +                       del_timer_sync(&port->tx_aggr.timer_cnxt.hold_timer);
>                 port->tx_aggr.timer_cnxt.is_hold_timer_set = false;
>                 port->tx_aggr.timer_cnxt.hold_tmo_msecs = 0;
>         }
> --
> 1.8.3.1
>


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] mwifiex: don't call del_timer_sync() on uninitialized timer
  2020-07-28  1:44                   ` [PATCH] mwifiex: don't call del_timer_sync() on uninitialized timer Tetsuo Handa
  2020-07-28 17:29                     ` Andy Shevchenko
@ 2020-07-28 18:45                     ` Brian Norris
  1 sibling, 0 replies; 15+ messages in thread
From: Brian Norris @ 2020-07-28 18:45 UTC (permalink / raw)
  To: Tetsuo Handa
  Cc: Ganapathi Bhat, amit karwar, andreyknvl, David S. Miller,
	Dmitry Vyukov, Xinming Hu, Kalle Valo, Linux Kernel,
	Linux USB Mailing List, linux-wireless,
	<netdev@vger.kernel.org>,
	Nishant Sarmukadam, syzbot+dc4127f950da51639216, syzkaller-bugs,
	syzbot

Hi,

On Mon, Jul 27, 2020 at 6:45 PM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> syzbot is reporting that del_timer_sync() is called from
> mwifiex_usb_cleanup_tx_aggr() from mwifiex_unregister_dev() without
> checking timer_setup() from mwifiex_usb_tx_init() was called [1].
> Since mwifiex_usb_prepare_tx_aggr_skb() is calling del_timer() if
> is_hold_timer_set == true, use the same condition for del_timer_sync().
>
> [1] https://syzkaller.appspot.com/bug?id=fdeef9cf7348be8b8ab5b847f2ed993aba8ea7b6
>
> Reported-by: syzbot <syzbot+373e6719b49912399d21@syzkaller.appspotmail.com>
> Cc: Ganapathi Bhat <gbhat@marvell.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
> A patch from Ganapathi Bhat ( https://patchwork.kernel.org/patch/10990275/ ) is stalling
> at https://lore.kernel.org/linux-usb/MN2PR18MB2637D7C742BC235FE38367F0A09C0@MN2PR18MB2637.namprd18.prod.outlook.com/ .
> syzbot by now got this report for 10000 times. Do we want to go with this simple patch?

Sorry, that stall is partly my fault, and partly Ganapathi's. It
doesn't help that it took him 4 months to reply to my questions, so I
completely lost even the tiny bit of context I had managed to build up
in my head at initial review time... and so it's still buried in the
dark corners of my inbox. (I think I'll go archive that now, because
it really deserves a better sell than it had initially, if Ganapathi
really wants to land it.)

>  drivers/net/wireless/marvell/mwifiex/usb.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
> index 6f3cfde..04a1461 100644
> --- a/drivers/net/wireless/marvell/mwifiex/usb.c
> +++ b/drivers/net/wireless/marvell/mwifiex/usb.c
> @@ -1353,7 +1353,8 @@ static void mwifiex_usb_cleanup_tx_aggr(struct mwifiex_adapter *adapter)
>                                 skb_dequeue(&port->tx_aggr.aggr_list)))
>                                 mwifiex_write_data_complete(adapter, skb_tmp,
>                                                             0, -1);
> -               del_timer_sync(&port->tx_aggr.timer_cnxt.hold_timer);
> +               if (port->tx_aggr.timer_cnxt.is_hold_timer_set)

I believe if we ever actually started aggregation, then the timer can
be active at this point, and thus, the access to 'is_hold_timer_set'
is racy.

This *probably* deserves a better refactor, but in absence of that
(and a better explanation than Ganapathi gave), I think you at least
need to hold port->tx_aggr_lock. So perhaps (totally untested):

  spin_lock_bh(&port->tx_aggr_lock);
  if (port->tx_aggr.timer_cnxt.is_hold_timer_set) {
    port->tx_aggr.timer_cnxt.is_hold_timer_set = false;
    spin_unlock_bh(&port->tx_aggr_lock);
    /* Timer could still be running, but it can't be restarted at this
point, so this is safe. */
    del_timer_sync(&port->tx_aggr.timer_cnxt.hold_timer);
  } else {
    spin_unlock_bh(&port->tx_aggr_lock);
  }

Otherwise, I think this is fine:

Reviewed-by: Brian Norris <briannorris@chromium.org>

I also believe mwifiex_usb_prepare_tx_aggr_skb() needs to stop using
del_timer() (without the _sync()), because otherwise we might have
deactivated the timer already but not ensured that it has completely
finished executing on other CPUs. But that is probably orthogonal to
the current patch. (Again, so much in this driver needs refactoring.)

Side note: this entire TX aggregation feature for USB has been hidden
behind the mwifiex.aggr_ctrl module param since its introduction,
which has always been disabled by default. I wonder whether anybody is
*really* testing it, or whether it's 100% broken, as with many things
in this driver...


Brian

> +                       del_timer_sync(&port->tx_aggr.timer_cnxt.hold_timer);
>                 port->tx_aggr.timer_cnxt.is_hold_timer_set = false;
>                 port->tx_aggr.timer_cnxt.hold_tmo_msecs = 0;
>         }
> --
> 1.8.3.1
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, back to index

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-12 14:26 INFO: trying to register non-static key in del_timer_sync (2) syzbot
2019-06-01 17:52 ` [EXT] " Ganapathi Bhat
2019-06-03  5:20   ` Dmitry Vyukov
2019-06-03  8:41     ` Ganapathi Bhat
2019-06-12 16:01       ` Ganapathi Bhat
2019-06-12 16:13         ` Andrey Konovalov
2019-06-12 16:59           ` syzbot
2019-08-13 13:36         ` [EXT] " Andrey Konovalov
2019-08-13 13:58           ` Kalle Valo
2019-08-14 14:08             ` Ganapathi Bhat
2019-10-01 16:40               ` Andrey Konovalov
2019-10-02 14:28                 ` Ganapathi Bhat
2020-07-28  1:44                   ` [PATCH] mwifiex: don't call del_timer_sync() on uninitialized timer Tetsuo Handa
2020-07-28 17:29                     ` Andy Shevchenko
2020-07-28 18:45                     ` Brian Norris

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lkml.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lkml.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lkml.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lkml.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lkml.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lkml.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lkml.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lkml.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lkml.kernel.org/lkml/8 lkml/git/8.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lkml.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git