possible deadlock in f_midi_transmit

possible deadlock in f_midi_transmit

[Rondreis]

Hello,

When fuzzing the Linux kernel driver v6.0-rc4, the following crash was
triggered.

HEAD commit: 7e18e42e4b280c85b76967a9106a13ca61c16179
git tree: upstream

kernel config: https://pastebin.com/raw/xtrgsXP3
C reproducer: https://pastebin.com/raw/C1xYEf7Q
console output: https://pastebin.com/raw/3RLhvQHE

Basically, in the c reproducer, we use the gadget module to emulate
attaching a USB device(vendor id: 0x403, product id: 0xff3d, with the
midi function) and executing some simple sequence of system calls.
To reproduce this crash, we utilize a third-party library to emulate
the attaching process: https://github.com/linux-usb-gadgets/libusbgx.
Just clone this repository, install it, and compile the c
reproducer with ``` gcc crash.c -lusbgx -lconfig -o crash ``` will do
the trick.

I would appreciate it if you have any idea how to solve this bug.

The crash report is as follows:


============================================
WARNING: possible recursive locking detected
6.0.0-rc4+ #20 Not tainted
--------------------------------------------
kworker/0:1H/9 is trying to acquire lock:
ffff888057ed9228 (&midi->transmit_lock){....}-{2:2}, at:
f_midi_transmit+0x18c/0x1460 drivers/usb/gadget/function/f_midi.c:683

but task is already holding lock:
ffff888057ed9228 (&midi->transmit_lock){....}-{2:2}, at:
f_midi_transmit+0x18c/0x1460 drivers/usb/gadget/function/f_midi.c:683

other info that might help us debug this:
 Possible unsafe locking scenario:

       CPU0
       ----
  lock(&midi->transmit_lock);
  lock(&midi->transmit_lock);

 *** DEADLOCK ***

 May be due to missing lock nesting notation

3 locks held by kworker/0:1H/9:
 #0: ffff888011c65138 ((wq_completion)events_highpri){+.+.}-{0:0}, at:
arch_atomic64_set arch/x86/include/asm/atomic64_64.h:34 [inline]
 #0: ffff888011c65138 ((wq_completion)events_highpri){+.+.}-{0:0}, at:
arch_atomic_long_set include/linux/atomic/atomic-long.h:41 [inline]
 #0: ffff888011c65138 ((wq_completion)events_highpri){+.+.}-{0:0}, at:
atomic_long_set include/linux/atomic/atomic-instrumented.h:1280
[inline]
 #0: ffff888011c65138 ((wq_completion)events_highpri){+.+.}-{0:0}, at:
set_work_data kernel/workqueue.c:636 [inline]
 #0: ffff888011c65138 ((wq_completion)events_highpri){+.+.}-{0:0}, at:
set_work_pool_and_clear_pending kernel/workqueue.c:663 [inline]
 #0: ffff888011c65138 ((wq_completion)events_highpri){+.+.}-{0:0}, at:
process_one_work+0x8b0/0x1650 kernel/workqueue.c:2260
 #1: ffffc900003afdb0 ((work_completion)(&midi->work)){+.+.}-{0:0},
at: process_one_work+0x8e4/0x1650 kernel/workqueue.c:2264
 #2: ffff888057ed9228 (&midi->transmit_lock){....}-{2:2}, at:
f_midi_transmit+0x18c/0x1460 drivers/usb/gadget/function/f_midi.c:683

stack backtrace:
CPU: 0 PID: 9 Comm: kworker/0:1H Not tainted 6.0.0-rc4+ #20
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
1.13.0-1ubuntu1.1 04/01/2014
Workqueue: events_highpri f_midi_in_work
Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:88 [inline]
 dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
 print_deadlock_bug kernel/locking/lockdep.c:2988 [inline]
 check_deadlock kernel/locking/lockdep.c:3031 [inline]
 validate_chain kernel/locking/lockdep.c:3816 [inline]
 __lock_acquire.cold+0x152/0x3c3 kernel/locking/lockdep.c:5053
 lock_acquire kernel/locking/lockdep.c:5666 [inline]
 lock_acquire+0x1ab/0x580 kernel/locking/lockdep.c:5631
 __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
 _raw_spin_lock_irqsave+0x39/0x50 kernel/locking/spinlock.c:162
 f_midi_transmit+0x18c/0x1460 drivers/usb/gadget/function/f_midi.c:683
 f_midi_complete+0x1bb/0x480 drivers/usb/gadget/function/f_midi.c:285
 dummy_queue+0x84a/0xb20 drivers/usb/gadget/udc/dummy_hcd.c:736
 usb_ep_queue+0xe8/0x3b0 drivers/usb/gadget/udc/core.c:288
 f_midi_do_transmit drivers/usb/gadget/function/f_midi.c:658 [inline]
 f_midi_transmit+0x7e4/0x1460 drivers/usb/gadget/function/f_midi.c:686
 process_one_work+0x9c7/0x1650 kernel/workqueue.c:2289
 worker_thread+0x623/0x1070 kernel/workqueue.c:2436
 kthread+0x2e9/0x3a0 kernel/kthread.c:376
 ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:306
 </TASK>

Re: possible deadlock in f_midi_transmit

[Greg KH]

On Wed, Sep 14, 2022 at 08:49:49PM +0800, Rondreis wrote:
> Hello,
> 
> When fuzzing the Linux kernel driver v6.0-rc4, the following crash was
> triggered.
> 
> HEAD commit: 7e18e42e4b280c85b76967a9106a13ca61c16179
> git tree: upstream
> 
> kernel config: https://pastebin.com/raw/xtrgsXP3
> C reproducer: https://pastebin.com/raw/C1xYEf7Q
> console output: https://pastebin.com/raw/3RLhvQHE
> 
> Basically, in the c reproducer, we use the gadget module to emulate
> attaching a USB device(vendor id: 0x403, product id: 0xff3d, with the
> midi function) and executing some simple sequence of system calls.
> To reproduce this crash, we utilize a third-party library to emulate
> the attaching process: https://github.com/linux-usb-gadgets/libusbgx.
> Just clone this repository, install it, and compile the c
> reproducer with ``` gcc crash.c -lusbgx -lconfig -o crash ``` will do
> the trick.
> 
> I would appreciate it if you have any idea how to solve this bug.
> 
> The crash report is as follows:
> 
> 
> ============================================
> WARNING: possible recursive locking detected
> 6.0.0-rc4+ #20 Not tainted
> --------------------------------------------
> kworker/0:1H/9 is trying to acquire lock:
> ffff888057ed9228 (&midi->transmit_lock){....}-{2:2}, at:
> f_midi_transmit+0x18c/0x1460 drivers/usb/gadget/function/f_midi.c:683
> 
> but task is already holding lock:
> ffff888057ed9228 (&midi->transmit_lock){....}-{2:2}, at:
> f_midi_transmit+0x18c/0x1460 drivers/usb/gadget/function/f_midi.c:683
> 
> other info that might help us debug this:
>  Possible unsafe locking scenario:
> 
>        CPU0
>        ----
>   lock(&midi->transmit_lock);
>   lock(&midi->transmit_lock);
> 
>  *** DEADLOCK ***
> 
>  May be due to missing lock nesting notation

That's a lockdep warning, is this really deadlocking?

If so, I'd recommend asking the midi developers...

thanks,

greg k-h

Re: possible deadlock in f_midi_transmit

[Rondreis]

Happy to receive your reply! I will double-check its validity. If
there is still a problem, I will contact the midi development team.

Best wishes!
Rondreis

On Wed, Sep 14, 2022 at 10:02 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Sep 14, 2022 at 08:49:49PM +0800, Rondreis wrote:
> > Hello,
> >
> > When fuzzing the Linux kernel driver v6.0-rc4, the following crash was
> > triggered.
> >
> > HEAD commit: 7e18e42e4b280c85b76967a9106a13ca61c16179
> > git tree: upstream
> >
> > kernel config: https://pastebin.com/raw/xtrgsXP3
> > C reproducer: https://pastebin.com/raw/C1xYEf7Q
> > console output: https://pastebin.com/raw/3RLhvQHE
> >
> > Basically, in the c reproducer, we use the gadget module to emulate
> > attaching a USB device(vendor id: 0x403, product id: 0xff3d, with the
> > midi function) and executing some simple sequence of system calls.
> > To reproduce this crash, we utilize a third-party library to emulate
> > the attaching process: https://github.com/linux-usb-gadgets/libusbgx.
> > Just clone this repository, install it, and compile the c
> > reproducer with ``` gcc crash.c -lusbgx -lconfig -o crash ``` will do
> > the trick.
> >
> > I would appreciate it if you have any idea how to solve this bug.
> >
> > The crash report is as follows:
> >
> >
> > ============================================
> > WARNING: possible recursive locking detected
> > 6.0.0-rc4+ #20 Not tainted
> > --------------------------------------------
> > kworker/0:1H/9 is trying to acquire lock:
> > ffff888057ed9228 (&midi->transmit_lock){....}-{2:2}, at:
> > f_midi_transmit+0x18c/0x1460 drivers/usb/gadget/function/f_midi.c:683
> >
> > but task is already holding lock:
> > ffff888057ed9228 (&midi->transmit_lock){....}-{2:2}, at:
> > f_midi_transmit+0x18c/0x1460 drivers/usb/gadget/function/f_midi.c:683
> >
> > other info that might help us debug this:
> >  Possible unsafe locking scenario:
> >
> >        CPU0
> >        ----
> >   lock(&midi->transmit_lock);
> >   lock(&midi->transmit_lock);
> >
> >  *** DEADLOCK ***
> >
> >  May be due to missing lock nesting notation
>
> That's a lockdep warning, is this really deadlocking?
>
> If so, I'd recommend asking the midi developers...
>
> thanks,
>
> greg k-h

Re: [PATCH] usb/gadget: Annotate midi lock nesting

[Greg KH]

On Sun, Sep 18, 2022 at 11:50:37AM +0800, eadavis@sina.com wrote:
> From: Edward Adam Davis <eadavis@sina.com>
> 
> ============================================
> WARNING: possible recursive locking detected
> 6.0.0-rc4+ #20 Not tainted
> --------------------------------------------
> kworker/0:1H/9 is trying to acquire lock:
> ffff888057ed9228 (&midi->transmit_lock){....}-{2:2}, at:
> f_midi_transmit+0x18c/0x1460 drivers/usb/gadget/function/f_midi.c:683
> 
> but task is already holding lock:
> ffff888057ed9228 (&midi->transmit_lock){....}-{2:2}, at:
> f_midi_transmit+0x18c/0x1460 drivers/usb/gadget/function/f_midi.c:683
> 
> other info that might help us debug this:
>  Possible unsafe locking scenario:
> 
>        CPU0
>        ----
>   lock(&midi->transmit_lock);
>   lock(&midi->transmit_lock);
> 
>  *** DEADLOCK ***
> 
>  May be due to missing lock nesting notation
> 
> 3 locks held by kworker/0:1H/9:
>  #0: ffff888011c65138 ((wq_completion)events_highpri){+.+.}-{0:0}, at:
> arch_atomic64_set arch/x86/include/asm/atomic64_64.h:34 [inline]
>  #0: ffff888011c65138 ((wq_completion)events_highpri){+.+.}-{0:0}, at:
> arch_atomic_long_set include/linux/atomic/atomic-long.h:41 [inline]
>  #0: ffff888011c65138 ((wq_completion)events_highpri){+.+.}-{0:0}, at:
> atomic_long_set include/linux/atomic/atomic-instrumented.h:1280
> [inline]
>  #0: ffff888011c65138 ((wq_completion)events_highpri){+.+.}-{0:0}, at:
> set_work_data kernel/workqueue.c:636 [inline]
>  #0: ffff888011c65138 ((wq_completion)events_highpri){+.+.}-{0:0}, at:
> set_work_pool_and_clear_pending kernel/workqueue.c:663 [inline]
>  #0: ffff888011c65138 ((wq_completion)events_highpri){+.+.}-{0:0}, at:
> process_one_work+0x8b0/0x1650 kernel/workqueue.c:2260
>  #1: ffffc900003afdb0 ((work_completion)(&midi->work)){+.+.}-{0:0},
> at: process_one_work+0x8e4/0x1650 kernel/workqueue.c:2264
>  #2: ffff888057ed9228 (&midi->transmit_lock){....}-{2:2}, at:
> f_midi_transmit+0x18c/0x1460 drivers/usb/gadget/function/f_midi.c:683
> 
> stack backtrace:
> CPU: 0 PID: 9 Comm: kworker/0:1H Not tainted 6.0.0-rc4+ #20
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> 1.13.0-1ubuntu1.1 04/01/2014
> Workqueue: events_highpri f_midi_in_work
> Call Trace:
>  <TASK>
>  __dump_stack lib/dump_stack.c:88 [inline]
>  dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
>  print_deadlock_bug kernel/locking/lockdep.c:2988 [inline]
>  check_deadlock kernel/locking/lockdep.c:3031 [inline]
>  validate_chain kernel/locking/lockdep.c:3816 [inline]
>  __lock_acquire.cold+0x152/0x3c3 kernel/locking/lockdep.c:5053
>  lock_acquire kernel/locking/lockdep.c:5666 [inline]
>  lock_acquire+0x1ab/0x580 kernel/locking/lockdep.c:5631
>  __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
>  _raw_spin_lock_irqsave+0x39/0x50 kernel/locking/spinlock.c:162
>  f_midi_transmit+0x18c/0x1460 drivers/usb/gadget/function/f_midi.c:683
>  f_midi_complete+0x1bb/0x480 drivers/usb/gadget/function/f_midi.c:285
>  dummy_queue+0x84a/0xb20 drivers/usb/gadget/udc/dummy_hcd.c:736
>  usb_ep_queue+0xe8/0x3b0 drivers/usb/gadget/udc/core.c:288
>  f_midi_do_transmit drivers/usb/gadget/function/f_midi.c:658 [inline]
>  f_midi_transmit+0x7e4/0x1460 drivers/usb/gadget/function/f_midi.c:686
>  process_one_work+0x9c7/0x1650 kernel/workqueue.c:2289
>  worker_thread+0x623/0x1070 kernel/workqueue.c:2436
>  kthread+0x2e9/0x3a0 kernel/kthread.c:376
>  ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:306
>  </TASK>
> Use nested notation for the spin_lock to avoid this warning.
> 
> Signed-off-by: Edward Adam Davis <eadavis@sina.com>
> ---
>  drivers/usb/gadget/function/f_midi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
> index fddf539008a9..ad745fbd549e 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -680,7 +680,8 @@ static void f_midi_transmit(struct f_midi *midi)
>  	if (!ep || !ep->enabled)
>  		goto drop_out;
>  
> -	spin_lock_irqsave(&midi->transmit_lock, flags);
> +	spin_lock_irqsave_nested(&midi->transmit_lock, flags, 
> +			SINGLE_DEPTH_NESTING);

This feels wrong (and you added a checkpatch warning at the same time.)

If this is correct, please document this really really well why this is
the correct solution and we just don't really have a lockdep issue here
with the code itself.

thanks,

greg k-h

Re: [PATCH] usb/gadget: Annotate midi lock nesting

[Greg KH]

On Thu, Sep 22, 2022 at 10:26:54PM +0800, eadavis@sina.com wrote:
> From: Edward Adam Davis <eadavis@sina.com>
> 
> On Thu, 22 Sep 2022 15:04:45 +0200, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Sun, Sep 18, 2022 at 11:50:37AM +0800, eadavis@sina.com wrote:
> > > From: Edward Adam Davis <eadavis@sina.com>
> > > 
> > > ============================================
> > > WARNING: possible recursive locking detected
> > > 6.0.0-rc4+ #20 Not tainted
> > > --------------------------------------------
> > > kworker/0:1H/9 is trying to acquire lock:
> > > ffff888057ed9228 (&midi->transmit_lock){....}-{2:2}, at:
> > > f_midi_transmit+0x18c/0x1460 drivers/usb/gadget/function/f_midi.c:683
> > > 
> > > but task is already holding lock:
> > > ffff888057ed9228 (&midi->transmit_lock){....}-{2:2}, at:
> > > f_midi_transmit+0x18c/0x1460 drivers/usb/gadget/function/f_midi.c:683
> > > 
> > > other info that might help us debug this:
> > >  Possible unsafe locking scenario:
> > > 
> > >        CPU0
> > >        ----
> > >   lock(&midi->transmit_lock);
> > >   lock(&midi->transmit_lock);
> > > 
> > >  *** DEADLOCK ***
> > > 
> > >  May be due to missing lock nesting notation
> > > 
> > > 3 locks held by kworker/0:1H/9:
> > >  #0: ffff888011c65138 ((wq_completion)events_highpri){+.+.}-{0:0}, at:
> > > arch_atomic64_set arch/x86/include/asm/atomic64_64.h:34 [inline]
> > >  #0: ffff888011c65138 ((wq_completion)events_highpri){+.+.}-{0:0}, at:
> > > arch_atomic_long_set include/linux/atomic/atomic-long.h:41 [inline]
> > >  #0: ffff888011c65138 ((wq_completion)events_highpri){+.+.}-{0:0}, at:
> > > atomic_long_set include/linux/atomic/atomic-instrumented.h:1280
> > > [inline]
> > >  #0: ffff888011c65138 ((wq_completion)events_highpri){+.+.}-{0:0}, at:
> > > set_work_data kernel/workqueue.c:636 [inline]
> > >  #0: ffff888011c65138 ((wq_completion)events_highpri){+.+.}-{0:0}, at:
> > > set_work_pool_and_clear_pending kernel/workqueue.c:663 [inline]
> > >  #0: ffff888011c65138 ((wq_completion)events_highpri){+.+.}-{0:0}, at:
> > > process_one_work+0x8b0/0x1650 kernel/workqueue.c:2260
> > >  #1: ffffc900003afdb0 ((work_completion)(&midi->work)){+.+.}-{0:0},
> > > at: process_one_work+0x8e4/0x1650 kernel/workqueue.c:2264
> > >  #2: ffff888057ed9228 (&midi->transmit_lock){....}-{2:2}, at:
> > > f_midi_transmit+0x18c/0x1460 drivers/usb/gadget/function/f_midi.c:683
> > > 
> > > stack backtrace:
> > > CPU: 0 PID: 9 Comm: kworker/0:1H Not tainted 6.0.0-rc4+ #20
> > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> > > 1.13.0-1ubuntu1.1 04/01/2014
> > > Workqueue: events_highpri f_midi_in_work
> > > Call Trace:
> > >  <TASK>
> > >  __dump_stack lib/dump_stack.c:88 [inline]
> > >  dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
> > >  print_deadlock_bug kernel/locking/lockdep.c:2988 [inline]
> > >  check_deadlock kernel/locking/lockdep.c:3031 [inline]
> > >  validate_chain kernel/locking/lockdep.c:3816 [inline]
> > >  __lock_acquire.cold+0x152/0x3c3 kernel/locking/lockdep.c:5053
> > >  lock_acquire kernel/locking/lockdep.c:5666 [inline]
> > >  lock_acquire+0x1ab/0x580 kernel/locking/lockdep.c:5631
> > >  __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
> > >  _raw_spin_lock_irqsave+0x39/0x50 kernel/locking/spinlock.c:162
> > >  f_midi_transmit+0x18c/0x1460 drivers/usb/gadget/function/f_midi.c:683
> > >  f_midi_complete+0x1bb/0x480 drivers/usb/gadget/function/f_midi.c:285
> > >  dummy_queue+0x84a/0xb20 drivers/usb/gadget/udc/dummy_hcd.c:736
> > >  usb_ep_queue+0xe8/0x3b0 drivers/usb/gadget/udc/core.c:288
> > >  f_midi_do_transmit drivers/usb/gadget/function/f_midi.c:658 [inline]
> > >  f_midi_transmit+0x7e4/0x1460 drivers/usb/gadget/function/f_midi.c:686
> > >  process_one_work+0x9c7/0x1650 kernel/workqueue.c:2289
> > >  worker_thread+0x623/0x1070 kernel/workqueue.c:2436
> > >  kthread+0x2e9/0x3a0 kernel/kthread.c:376
> > >  ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:306
> > >  </TASK>
> > > Use nested notation for the spin_lock to avoid this warning.
> > > 
> > > Signed-off-by: Edward Adam Davis <eadavis@sina.com>
> > > ---
> > >  drivers/usb/gadget/function/f_midi.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
> > > index fddf539008a9..ad745fbd549e 100644
> > > --- a/drivers/usb/gadget/function/f_midi.c
> > > +++ b/drivers/usb/gadget/function/f_midi.c
> > > @@ -680,7 +680,8 @@ static void f_midi_transmit(struct f_midi *midi)
> > >  	if (!ep || !ep->enabled)
> > >  		goto drop_out;
> > >  
> > > -	spin_lock_irqsave(&midi->transmit_lock, flags);
> > > +	spin_lock_irqsave_nested(&midi->transmit_lock, flags, 
> > > +			SINGLE_DEPTH_NESTING);
> > 
> > This feels wrong (and you added a checkpatch warning at the same time.)
> > 
> > If this is correct, please document this really really well why this is
> > the correct solution and we just don't really have a lockdep issue here
> > with the code itself.
> I want to assume the following scenario,
> 
>      	CPU1
>         ----
> spin_lock_irqsave(&midi->transmit_lock, f);          <----- Task A
> ...
> ...                                                  <----- raise NMI and call Task B 
>    spin_lock_irqsave(&midi->transmit_lock, f);       <----- Task B acquire same lock, OK?

Is that ok?  Can you nest a spin lock like this?  For some reason, I
didn't think you could, but I can't find anything in the documentation
about this, can you?

thanks,

greg k-h