Re: [PATCH] drivers/media/rc: Ensure usb_submit_urb() is not called if write is in progress

[Greg KH <gregkh@linuxfoundation.org>]

On Sat, Sep 03, 2022 at 03:08:23PM +0530, Gautam Menghani wrote:
> On Sat, Sep 03, 2022 at 08:49:56AM +0100, Sean Young wrote:
> > On Fri, Sep 02, 2022 at 11:42:41PM +0530, Gautam Menghani 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;
> > > >  	/* Check if we need to use control or interrupt urb */
> > > >  	if (!ictx->tx_control) {
> > > >  		pipe = usb_sndintpipe(ictx->usbdev_intf0,
> > > > @@ -654,6 +656,7 @@ static int send_packet(struct imon_context *ictx)
> > > >  			pr_err_ratelimited("task interrupted\n");
> > > >  		}
> > > >  		mutex_lock(&ictx->lock);
> > > > +		ictx->tx.busy = false;
> > > >  
> > > >  		retval = ictx->tx.status;
> > > >  		if (retval)
> > > > -- 
> > > > 2.34.1
> > > > 
> > > Hi,
> > > 
> > > Please review the above fix and let me know if any changes are needed.
> > 
> > Greg has pointed out issues with this patch: there is no locking.
> > 
> > Thanks,
> > 
> > Sean
> 
> Hi,
> 
> I am a bit confused about the locking part. All the calls to send_packet() are
> made with ictx->lock held. So will the following be sufficient?
> 
> lockdep_assert_held(&ictx->lock);
> if (ictx->tx.busy)
> 	return -EBUSY;
> 
> Or do we need to add a mutex/spin lock inside ictx->tx structure? 

If a lock is held, how can this function be called at the same time
through different ways?  That should not happen.

> Or should we instead wait for the the tx to be completed as follows:
> wait_for_completion(&ictx->tx.finished);

It depends on what you are trying to protect from here.  What is the
real issue that is allowing multiple submissions of the same urb to
happen?  Fix that issue instead of trying to paper over it in this lower
level function please.

thanks,

greg k-h