* 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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 related [flat|nested] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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 2020-08-21 8:27 ` [PATCH v2] " Tetsuo Handa 0 siblings, 2 replies; 20+ 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] 20+ 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 2020-08-21 8:27 ` [PATCH v2] " Tetsuo Handa 1 sibling, 2 replies; 20+ 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 related [flat|nested] 20+ 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; 20+ 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] 20+ 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 2020-08-17 13:06 ` Tetsuo Handa 1 sibling, 1 reply; 20+ 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] 20+ messages in thread
* Re: [PATCH] mwifiex: don't call del_timer_sync() on uninitialized timer 2020-07-28 18:45 ` Brian Norris @ 2020-08-17 13:06 ` Tetsuo Handa 0 siblings, 0 replies; 20+ messages in thread From: Tetsuo Handa @ 2020-08-17 13:06 UTC (permalink / raw) To: Ganapathi Bhat Cc: Brian Norris, amit karwar, andreyknvl, David S. Miller, Dmitry Vyukov, Xinming Hu, Kalle Valo, Linux Kernel, Linux USB Mailing List, linux-wireless, netdev, Nishant Sarmukadam, syzbot+dc4127f950da51639216, syzkaller-bugs, syzbot Ganapathi, how do you want to fix this bug? On 2020/07/29 3:45, Brian Norris 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.) ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2] mwifiex: don't call del_timer_sync() on uninitialized timer 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-08-21 8:27 ` Tetsuo Handa 2020-08-24 18:52 ` Brian Norris 2020-08-27 10:00 ` Kalle Valo 1 sibling, 2 replies; 20+ messages in thread From: Tetsuo Handa @ 2020-08-21 8:27 UTC (permalink / raw) To: Ganapathi Bhat, Brian Norris Cc: amitkarwar, andreyknvl, davem, dvyukov, huxinming820, kvalo, linux-kernel, linux-usb, linux-wireless, netdev, nishants, syzkaller-bugs, syzbot, 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]. Ganapathi Bhat proposed a possibly cleaner fix, but it seems that that fix was forgotten [2]. "grep -FrB1 'del_timer' drivers/ | grep -FA1 '.function)'" says that currently there are 28 locations which call del_timer[_sync]() only if that timer's function field was initialized (because timer_setup() sets that timer's function field). Therefore, let's use same approach here. [1] https://syzkaller.appspot.com/bug?id=26525f643f454dd7be0078423e3cdb0d57744959 [2] https://lkml.kernel.org/r/CA+ASDXMHt2gq9Hy+iP_BYkWXsSreWdp3_bAfMkNcuqJ3K+-jbQ@mail.gmail.com Reported-by: syzbot <syzbot+dc4127f950da51639216@syzkaller.appspotmail.com> Cc: Ganapathi Bhat <ganapathi.bhat@nxp.com> Cc: Brian Norris <briannorris@chromium.org> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- 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 6f3cfde4654c..426e39d4ccf0 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.hold_timer.function) + 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; } -- 2.18.4 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2] mwifiex: don't call del_timer_sync() on uninitialized timer 2020-08-21 8:27 ` [PATCH v2] " Tetsuo Handa @ 2020-08-24 18:52 ` Brian Norris 2020-08-27 4:50 ` [EXT] " Ganapathi Bhat 2020-08-27 10:00 ` Kalle Valo 1 sibling, 1 reply; 20+ messages in thread From: Brian Norris @ 2020-08-24 18:52 UTC (permalink / raw) To: Tetsuo Handa Cc: Ganapathi Bhat, amit karwar, Andrey Konovalov, David S. Miller, Dmitry Vyukov, Xinming Hu, Kalle Valo, Linux Kernel, Linux USB Mailing List, linux-wireless, <netdev@vger.kernel.org>, Nishant Sarmukadam, syzkaller-bugs, syzbot, syzbot On Fri, Aug 21, 2020 at 1:28 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]. > > Ganapathi Bhat proposed a possibly cleaner fix, but it seems that > that fix was forgotten [2]. > > "grep -FrB1 'del_timer' drivers/ | grep -FA1 '.function)'" says that > currently there are 28 locations which call del_timer[_sync]() only if > that timer's function field was initialized (because timer_setup() sets > that timer's function field). Therefore, let's use same approach here. > > [1] https://syzkaller.appspot.com/bug?id=26525f643f454dd7be0078423e3cdb0d57744959 > [2] https://lkml.kernel.org/r/CA+ASDXMHt2gq9Hy+iP_BYkWXsSreWdp3_bAfMkNcuqJ3K+-jbQ@mail.gmail.com > > Reported-by: syzbot <syzbot+dc4127f950da51639216@syzkaller.appspotmail.com> > Cc: Ganapathi Bhat <ganapathi.bhat@nxp.com> > Cc: Brian Norris <briannorris@chromium.org> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> This seems good to me: Reviewed-by: Brian Norris <briannorris@chromium.org> > --- > 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 6f3cfde4654c..426e39d4ccf0 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.hold_timer.function) > + 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; > } > -- > 2.18.4 > ^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [EXT] Re: [PATCH v2] mwifiex: don't call del_timer_sync() on uninitialized timer 2020-08-24 18:52 ` Brian Norris @ 2020-08-27 4:50 ` Ganapathi Bhat 0 siblings, 0 replies; 20+ messages in thread From: Ganapathi Bhat @ 2020-08-27 4:50 UTC (permalink / raw) To: Brian Norris, Tetsuo Handa Cc: amit karwar, Andrey Konovalov, David S. Miller, Dmitry Vyukov, Xinming Hu, Kalle Valo, Linux Kernel, Linux USB Mailing List, linux-wireless, <netdev@vger.kernel.org>, Nishant Sarmukadam, syzkaller-bugs, syzbot, syzbot Hi Tetsuo, > > "grep -FrB1 'del_timer' drivers/ | grep -FA1 '.function)'" says that > > currently there are 28 locations which call del_timer[_sync]() only if > > that timer's function field was initialized (because timer_setup() > > sets that timer's function field). Therefore, let's use same approach here. Thanks for the change, it look cleaner than my re-work; Acked-by: Ganapathi Bhat <ganapathi.bhat@nxp.com> Regards, Ganapathi ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] mwifiex: don't call del_timer_sync() on uninitialized timer 2020-08-21 8:27 ` [PATCH v2] " Tetsuo Handa 2020-08-24 18:52 ` Brian Norris @ 2020-08-27 10:00 ` Kalle Valo 1 sibling, 0 replies; 20+ messages in thread From: Kalle Valo @ 2020-08-27 10:00 UTC (permalink / raw) To: Tetsuo Handa Cc: Ganapathi Bhat, Brian Norris, amitkarwar, andreyknvl, davem, dvyukov, huxinming820, linux-kernel, linux-usb, linux-wireless, netdev, nishants, syzkaller-bugs, syzbot, Tetsuo Handa, syzbot 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]. > > Ganapathi Bhat proposed a possibly cleaner fix, but it seems that > that fix was forgotten [2]. > > "grep -FrB1 'del_timer' drivers/ | grep -FA1 '.function)'" says that > currently there are 28 locations which call del_timer[_sync]() only if > that timer's function field was initialized (because timer_setup() sets > that timer's function field). Therefore, let's use same approach here. > > [1] https://syzkaller.appspot.com/bug?id=26525f643f454dd7be0078423e3cdb0d57744959 > [2] https://lkml.kernel.org/r/CA+ASDXMHt2gq9Hy+iP_BYkWXsSreWdp3_bAfMkNcuqJ3K+-jbQ@mail.gmail.com > > Reported-by: syzbot <syzbot+dc4127f950da51639216@syzkaller.appspotmail.com> > Cc: Ganapathi Bhat <ganapathi.bhat@nxp.com> > Cc: Brian Norris <briannorris@chromium.org> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Reviewed-by: Brian Norris <briannorris@chromium.org> > Acked-by: Ganapathi Bhat <ganapathi.bhat@nxp.com> Patch applied to wireless-drivers-next.git, thanks. 621a3a8b1c0e mwifiex: don't call del_timer_sync() on uninitialized timer -- https://patchwork.kernel.org/patch/11728607/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-08-27 10:01 UTC | newest] Thread overview: 20+ 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 2020-08-17 13:06 ` Tetsuo Handa 2020-08-21 8:27 ` [PATCH v2] " Tetsuo Handa 2020-08-24 18:52 ` Brian Norris 2020-08-27 4:50 ` [EXT] " Ganapathi Bhat 2020-08-27 10:00 ` Kalle Valo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).