[syzbot] WARNING in usb_tx_block/usb_submit_urb

[syzbot] WARNING in usb_tx_block/usb_submit_urb

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    f87b564686ee dt-bindings: usb: amlogic,meson-g12a-usb-ctrl..
git tree:       https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
console output: https://syzkaller.appspot.com/x/log.txt?x=119f3aaf480000
kernel config:  https://syzkaller.appspot.com/x/.config?x=6d59dd45f9349215
dashboard link: https://syzkaller.appspot.com/bug?extid=355c68b459d1d96c4d06
compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=17db7007480000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=1670f2b3480000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/162f005fbb8d/disk-f87b5646.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/868c38dbb85a/vmlinux-f87b5646.xz
kernel image: https://storage.googleapis.com/syzbot-assets/e560670dfb35/bzImage-f87b5646.xz

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

usb 1-1: Product: syz
usb 1-1: Manufacturer: syz
usb 1-1: SerialNumber: syz
usb 1-1: config 0 descriptor??
------------[ cut here ]------------
URB ffff888112baaf00 submitted while active
WARNING: CPU: 0 PID: 12 at drivers/usb/core/urb.c:379 usb_submit_urb+0x14ec/0x1880 drivers/usb/core/urb.c:379
Modules linked in:
CPU: 0 PID: 12 Comm: kworker/0:1 Not tainted 6.2.0-rc7-syzkaller-00232-gf87b564686ee #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/21/2023
Workqueue: events request_firmware_work_func
RIP: 0010:usb_submit_urb+0x14ec/0x1880 drivers/usb/core/urb.c:379
Code: 89 de e8 87 86 88 fd 84 db 0f 85 a3 f3 ff ff e8 0a 8a 88 fd 4c 89 fe 48 c7 c7 00 2d a8 86 c6 05 14 8a 14 05 01 e8 18 06 19 02 <0f> 0b e9 81 f3 ff ff 48 89 7c 24 40 e8 e3 89 88 fd 48 8b 7c 24 40
RSP: 0018:ffffc900000cfa00 EFLAGS: 00010282
RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
RDX: ffff8881002dd400 RSI: ffffffff812db84c RDI: fffff52000019f32
RBP: ffff888112baaf00 R08: 0000000000000005 R09: 0000000000000000
R10: 0000000080000000 R11: 0000000000000000 R12: 00000000c0028200
R13: 0000000000000010 R14: 00000000fffffff0 R15: ffff888112baaf00
FS:  0000000000000000(0000) GS:ffff8881f6800000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f165ac57130 CR3: 000000011215a000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 <TASK>
 usb_tx_block+0x241/0x2e0 drivers/net/wireless/marvell/libertas/if_usb.c:436
 if_usb_issue_boot_command drivers/net/wireless/marvell/libertas/if_usb.c:766 [inline]
 if_usb_prog_firmware+0x531/0xe30 drivers/net/wireless/marvell/libertas/if_usb.c:859
 lbs_fw_loaded drivers/net/wireless/marvell/libertas/firmware.c:23 [inline]
 helper_firmware_cb drivers/net/wireless/marvell/libertas/firmware.c:80 [inline]
 helper_firmware_cb+0x1e9/0x2c0 drivers/net/wireless/marvell/libertas/firmware.c:64
 request_firmware_work_func+0x130/0x240 drivers/base/firmware_loader/main.c:1107
 process_one_work+0x9bf/0x1710 kernel/workqueue.c:2289
 worker_thread+0x669/0x1090 kernel/workqueue.c:2436
 kthread+0x2ee/0x3a0 kernel/kthread.c:376
 ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:308
 </TASK>


---
This report 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 issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this issue, for details see:
https://goo.gl/tpsmEJ#testing-patches

Re: [syzbot] WARNING in usb_tx_block/usb_submit_urb

[Fabio M. De Francesco]

On giovedì 16 febbraio 2023 09:18:34 CET Hillf Danton wrote:
> Fabio!
> 
> On Thu, 16 Feb 2023 07:54:08 +0100 Fabio M. De Francesco
> <fmdefrancesco@gmail.com>
> > >  	do {
> > >  	
> > >  		int j =3D 0;
> > >  		i++;
> > > 
> > > -		if_usb_issue_boot_command(cardp, BOOT_CMD_FW_BY_USB);
> > 
> > Don't we need to call if_usb_issue_boot_command() in a loop in order to
> > retry
> > the command?
> 
> Nope certainly because of no sense made by sending it again, given no
> response this round.
> 

Your argument looks reasonable but...

For what regards subsystems/drivers whose I'm not expert I always assume that 
the authors know what they do despite bugs. I mean that looks more probable 
that they have reasons to issue several calls to if_usb_issue_boot_command() / 
usb_submit_urb in a loop. May be that those usb_submit_urb get lost in some 
particular conditions, since they decide to try if_usb_issue_boot_command() in 
a loop (but forget to kill the URB before next iteration).

I have no reasons to think you are wrong. However I don't understand the 
reason that made you leave the loops untouched (except the line with the call 
to if_usb_issue_boot_command().

I suppose that, if you confirm that we have no reasons to reiterate that call, 
you should also leave only one loop waiting for response.

Am I missing something?

Thanks,

Fabio

Re: [syzbot] WARNING in usb_tx_block/usb_submit_urb

[syzbot]

Hello,

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

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

Tested on:

commit:         f87b5646 dt-bindings: usb: amlogic,meson-g12a-usb-ctrl..
git tree:       https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
console output: https://syzkaller.appspot.com/x/log.txt?x=160ab7d0c80000
kernel config:  https://syzkaller.appspot.com/x/.config?x=6d59dd45f9349215
dashboard link: https://syzkaller.appspot.com/bug?extid=355c68b459d1d96c4d06
compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
patch:          https://syzkaller.appspot.com/x/patch.diff?x=16fe1f10c80000

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

Re: [syzbot] WARNING in usb_tx_block/usb_submit_urb

[Fabio M. De Francesco]

On giovedì 16 febbraio 2023 08:44:18 CET syzbot wrote:
> Hello,
> 
> syzbot tried to test the proposed patch but the build/boot failed:
> 
> drivers/net/wireless/marvell/libertas/if_usb.c:865:43: error: expected ';'
> before ':' token
> 
> 
> Tested on:
> 
> commit:         f87b5646 dt-bindings: usb: amlogic,meson-g12a-usb-ctrl..
> git tree:       https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/
usb.git
> dashboard link: https://syzkaller.appspot.com/bug?extid=355c68b459d1d96c4d06
> compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils
> for Debian) 2.35.2 patch:         
> https://syzkaller.appspot.com/x/patch.diff?x=1414acf0c80000

Sorry for this syntax error :-(

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git 
f87b564686ee

diff --git a/drivers/net/wireless/marvell/libertas/if_usb.c b/drivers/net/
wireless/marvell/libertas/if_usb.c
index 20436a289d5c..e03a5dcf6dab 100644
--- a/drivers/net/wireless/marvell/libertas/if_usb.c
+++ b/drivers/net/wireless/marvell/libertas/if_usb.c
@@ -859,6 +859,7 @@ static void if_usb_prog_firmware(struct lbs_private *priv, 
int ret,
                        j++;
                        msleep_interruptible(100);
                } while (cardp->bootcmdresp == 0 && j < 10);
+               usb_kill_urb(cardp->tx_urb);
        } while (cardp->bootcmdresp == 0 && i < 5);

        if (cardp->bootcmdresp == BOOT_CMD_RESP_NOT_SUPPORTED) {
--

Fabio

Re: [syzbot] WARNING in usb_tx_block/usb_submit_urb

[syzbot]

Hello,

syzbot tried to test the proposed patch but the build/boot failed:

drivers/net/wireless/marvell/libertas/if_usb.c:865:43: error: expected ';' before ':' token


Tested on:

commit:         f87b5646 dt-bindings: usb: amlogic,meson-g12a-usb-ctrl..
git tree:       https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
dashboard link: https://syzkaller.appspot.com/bug?extid=355c68b459d1d96c4d06
compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
patch:          https://syzkaller.appspot.com/x/patch.diff?x=1414acf0c80000

Re: [syzbot] WARNING in usb_tx_block/usb_submit_urb

[Fabio M. De Francesco]

On mercoledì 15 febbraio 2023 12:05:15 CET Hillf Danton wrote:
> On Tue, 14 Feb 2023 23:00:47 -0800
> 
> > syzbot found the following issue on:
> > 
> > HEAD commit:    f87b564686ee dt-bindings: usb: amlogic,meson-g12a-usb-
ctrl..
> > git tree:      
> > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
> > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=1670f2b3480000
> Kill urb in flight after submitting it.
> 
> #syz test https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git 
> f87b564686ee
> 
> --- x/drivers/net/wireless/marvell/libertas/if_usb.c
> +++ y/drivers/net/wireless/marvell/libertas/if_usb.c
> @@ -763,9 +763,7 @@ static int if_usb_issue_boot_command(str
>  	memset(bootcmd->pad, 0, sizeof(bootcmd->pad));
> 
>  	/* Issue command */
> -	usb_tx_block(cardp, cardp->ep_out_buf, sizeof(*bootcmd));
> -
> -	return 0;
> +	return usb_tx_block(cardp, cardp->ep_out_buf, sizeof(*bootcmd));
>  }
> 
> 
> @@ -853,10 +851,12 @@ restart:
>  	}

> 
>  	cardp->bootcmdresp = 0;
> +	ret = if_usb_issue_boot_command(cardp, BOOT_CMD_FW_BY_USB);
> +	if (ret)
> +		goto done;

I think that you are changing the logic here (please read below)...

>  	do {
>  		int j = 0;
>  		i++;
> -		if_usb_issue_boot_command(cardp, BOOT_CMD_FW_BY_USB);

Don't we need to call if_usb_issue_boot_command() in a loop in order to retry 
the command?

>  		/* wait for command response */
>  		do {
>  			j++;
> @@ -864,6 +864,8 @@ restart:
>  		} while (cardp->bootcmdresp == 0 && j < 10);
>  	} while (cardp->bootcmdresp == 0 && i < 5);
> 
> +	usb_kill_urb(cardp->tx_urb);
> +

I'm not an expert in the USB core, anyway calling usb_kill_urb() looks good to 
me, but I think we should call it after each call of 
if_usb_issue_boot_command() in the above outer loop.

>  	if (cardp->bootcmdresp == BOOT_CMD_RESP_NOT_SUPPORTED) {
>  		/* Return to normal operation */
>  		ret = -EOPNOTSUPP;
> --

Can the following work?

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git 
f87b564686ee

diff --git a/drivers/net/wireless/marvell/libertas/if_usb.c b/drivers/net/
wireless/marvell/libertas/if_usb.c
index 20436a289d5c..626357d0c7b0 100644
--- a/drivers/net/wireless/marvell/libertas/if_usb.c
+++ b/drivers/net/wireless/marvell/libertas/if_usb.c
@@ -859,6 +859,7 @@ static void if_usb_prog_firmware(struct lbs_private *priv, 
int ret,
                        j++;
                        msleep_interruptible(100);
                } while (cardp->bootcmdresp == 0 && j < 10);
+               usb_kill_urb(cardp->tx_urb):
        } while (cardp->bootcmdresp == 0 && i < 5);
 
        if (cardp->bootcmdresp == BOOT_CMD_RESP_NOT_SUPPORTED) {
--

Thanks,

Fabio

Re: [syzbot] WARNING in usb_tx_block/usb_submit_urb

[syzbot]

Hello,

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

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

Tested on:

commit:         f87b5646 dt-bindings: usb: amlogic,meson-g12a-usb-ctrl..
git tree:       https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
console output: https://syzkaller.appspot.com/x/log.txt?x=13c8d577480000
kernel config:  https://syzkaller.appspot.com/x/.config?x=6d59dd45f9349215
dashboard link: https://syzkaller.appspot.com/bug?extid=355c68b459d1d96c4d06
compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
patch:          https://syzkaller.appspot.com/x/patch.diff?x=113a83d0c80000

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