Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 14 Aug 2022 22:14:14 +0530
From:   Gautam Menghani <gautammenghani201@gmail.com>
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     sean@mess.org, mchehab@kernel.org, hdanton@sina.com,
        linux-kernel@vger.kernel.org,
        syzbot+0c3cb6dc05fbbdc3ad66@syzkaller.appspotmail.com,
        linux-kernel-mentees@lists.linuxfoundation.org,
        linux-media@vger.kernel.org
Subject: Re: [PATCH] drivers/media/rc: Ensure usb_submit_urb() is not called
 if write is in progress
Message-ID: <YvkmXiZ+8zM9uh+Q@biggie>
References: <20220814142543.24910-1-gautammenghani201@gmail.com>
 <YvkKdIL8UVhKrI0Q@kroah.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YvkKdIL8UVhKrI0Q@kroah.com>
Precedence: bulk

On Sun, Aug 14, 2022 at 04:45:08PM +0200, Greg KH wrote:
> On Sun, Aug 14, 2022 at 07:55:42PM +0530, Gautam Menghani wrote:
> > The warning "URB submitted while active" is reported if the function
> > send_packet() in imon.c is called if a write is already is in progress.
> > Add a check to return -EBUSY in case a write is already is in progress.
> > Also, mark tx.busy as false after transmission is completed.
> > 
> > Fixes: 21677cfc562a ("V4L/DVB: ir-core: add imon driver")
> > Cc: hdanton@sina.com
> > Suggested-by: hdanton@sina.com
> > Link: https://syzkaller.appspot.com/bug?id=e378e6a51fbe6c5cc43e34f131cc9a315ef0337e
> > Reported-by: syzbot+0c3cb6dc05fbbdc3ad66@syzkaller.appspotmail.com
> > Signed-off-by: Gautam Menghani <gautammenghani201@gmail.com>
> > ---
> >  drivers/media/rc/imon.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c
> > index 735b925da998..a5b997c2c7e2 100644
> > --- a/drivers/media/rc/imon.c
> > +++ b/drivers/media/rc/imon.c
> > @@ -598,6 +598,8 @@ static int send_packet(struct imon_context *ictx)
> >  	int retval = 0;
> >  	struct usb_ctrlrequest *control_req = NULL;
> >  
> > +	if (ictx->tx.busy)
> > +		return -EBUSY;
> 
> What happens if we go busy right after this check?  Where is the locking
> here to protect this?

All calls to send_packet() are protected with ictx->lock() held. Are you referring 
to something else?

Also, if we return busy, the task is interrupted and the packet transaction fails, 
just like the current behaviour. With the above patch, warning is not triggered.
Here's the log from running the reproducer (with patch applied).

imon 1-1:0.0: Looks like you're trying to use an IR protocol this device does not support
imon 1-1:0.0: Unsupported IR protocol specified, overriding to iMON IR protocol
rc rc0: iMON Remote (15c2:0040) as /devices/platform/dummy_hcd.0/usb1/1-1/1-1:0.0/rc/rc0
input: iMON Remote (15c2:0040) as /devices/platform/dummy_hcd.0/usb1/1-1/1-1:0.0/rc/rc0/input5
imon 1-1:0.0: iMON device (15c2:0040, intf0) on usb<1:2> initialized
imon:vfd_write: send packet #0 failed
imon:send_packet: task interrupted
imon:send_packet: packet tx failed (-2)
imon:vfd_write: send packet #0 failed
usb 1-1: USB disconnect, device number 2

Thanks,
Gautam