possible deadlock in mon_bin_vma_fault

possible deadlock in mon_bin_vma_fault

[syzbot]

Hello,

syzbot found the following crash on:

HEAD commit:    58c3f14f86c9 Merge tag 'riscv-for-linus-4.19-rc2' of git:/..
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=12fc6f0e400000
kernel config:  https://syzkaller.appspot.com/x/.config?x=531a917630d2a492
dashboard link: https://syzkaller.appspot.com/bug?extid=56f9673bb4cdcbeb0e92
compiler:       gcc (GCC) 8.0.1 20180413 (experimental)
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=13dca13e400000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=17cbe492400000

IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+56f9673bb4cdcbeb0e92@syzkaller.appspotmail.com

vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0

======================================================
WARNING: possible circular locking dependency detected
4.19.0-rc1+ #215 Not tainted
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
------------------------------------------------------
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
syz-executor605/4648 is trying to acquire lock:
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
00000000a9171a30 (&rp->fetch_lock){+.+.}, at: mon_bin_vma_fault+0xdc/0x4a0  
drivers/usb/mon/mon_bin.c:1237

but task is already holding lock:
0000000069e9aac4 (&mm->mmap_sem){++++}, at: __mm_populate+0x31a/0x4d0  
mm/gup.c:1250
vhci_hcd: default hub control req: 0000 v0000 i0000 l0

which lock already depends on the new lock.

vhci_hcd: default hub control req: 0000 v0000 i0000 l0

the existing dependency chain (in reverse order) is:
vhci_hcd: default hub control req: 0000 v0000 i0000 l0

-> #1 (&mm->mmap_sem){++++}:
        __might_fault+0x155/0x1e0 mm/memory.c:4578
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
        _copy_to_user+0x30/0x110 lib/usercopy.c:25
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
        copy_to_user include/linux/uaccess.h:155 [inline]
        mon_bin_read+0x334/0x650 drivers/usb/mon/mon_bin.c:825
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
        __vfs_read+0x117/0x9b0 fs/read_write.c:416
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
        vfs_read+0x17f/0x3c0 fs/read_write.c:452
        ksys_read+0x101/0x260 fs/read_write.c:578
        __do_sys_read fs/read_write.c:588 [inline]
        __se_sys_read fs/read_write.c:586 [inline]
        __x64_sys_read+0x73/0xb0 fs/read_write.c:586
        do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
        entry_SYSCALL_64_after_hwframe+0x49/0xbe
vhci_hcd: default hub control req: 0000 v0000 i0000 l0

-> #0 (&rp->fetch_lock){+.+.}:
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
        lock_acquire+0x1e4/0x4f0 kernel/locking/lockdep.c:3901
        __mutex_lock_common kernel/locking/mutex.c:925 [inline]
        __mutex_lock+0x171/0x1700 kernel/locking/mutex.c:1073
        mutex_lock_nested+0x16/0x20 kernel/locking/mutex.c:1088
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
        mon_bin_vma_fault+0xdc/0x4a0 drivers/usb/mon/mon_bin.c:1237
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
        __do_fault+0xee/0x450 mm/memory.c:3240
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
        do_read_fault mm/memory.c:3652 [inline]
        do_fault mm/memory.c:3752 [inline]
        handle_pte_fault mm/memory.c:3983 [inline]
        __handle_mm_fault+0x2b4a/0x4350 mm/memory.c:4107
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
        handle_mm_fault+0x53e/0xc80 mm/memory.c:4144
        faultin_page mm/gup.c:518 [inline]
        __get_user_pages+0x823/0x1b50 mm/gup.c:718
        populate_vma_page_range+0x2db/0x3d0 mm/gup.c:1222
        __mm_populate+0x286/0x4d0 mm/gup.c:1270
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
        mm_populate include/linux/mm.h:2307 [inline]
        vm_mmap_pgoff+0x27f/0x2c0 mm/util.c:362
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
        ksys_mmap_pgoff+0x4da/0x660 mm/mmap.c:1585
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
        __do_sys_mmap arch/x86/kernel/sys_x86_64.c:100 [inline]
        __se_sys_mmap arch/x86/kernel/sys_x86_64.c:91 [inline]
        __x64_sys_mmap+0xe9/0x1b0 arch/x86/kernel/sys_x86_64.c:91
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
        do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
        entry_SYSCALL_64_after_hwframe+0x49/0xbe

other info that might help us debug this:

vhci_hcd: default hub control req: 0000 v0000 i0000 l0
  Possible unsafe locking scenario:

vhci_hcd: default hub control req: 0000 v0000 i0000 l0
        CPU0                    CPU1
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
        ----                    ----
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
   lock(&mm->mmap_sem);
                                lock(&rp->fetch_lock);
                                lock(&mm->mmap_sem);
   lock(&rp->fetch_lock);
vhci_hcd: default hub control req: 0000 v0000 i0000 l0

  *** DEADLOCK ***

vhci_hcd: default hub control req: 0000 v0000 i0000 l0
1 lock held by syz-executor605/4648:
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
  #0: 0000000069e9aac4 (&mm->mmap_sem){++++}, at: __mm_populate+0x31a/0x4d0  
mm/gup.c:1250
vhci_hcd: default hub control req: 0000 v0000 i0000 l0

stack backtrace:
CPU: 1 PID: 4648 Comm: syz-executor605 Not tainted 4.19.0-rc1+ #215
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS  
Google 01/01/2011
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
Call Trace:
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0x1c9/0x2b4 lib/dump_stack.c:113
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
  print_circular_bug.isra.34.cold.55+0x1bd/0x27d  
kernel/locking/lockdep.c:1222
  check_prev_add kernel/locking/lockdep.c:1862 [inline]
  check_prevs_add kernel/locking/lockdep.c:1975 [inline]
  validate_chain kernel/locking/lockdep.c:2416 [inline]
  __lock_acquire+0x3449/0x5020 kernel/locking/lockdep.c:3412
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
  lock_acquire+0x1e4/0x4f0 kernel/locking/lockdep.c:3901
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
  __mutex_lock_common kernel/locking/mutex.c:925 [inline]
  __mutex_lock+0x171/0x1700 kernel/locking/mutex.c:1073
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
  mutex_lock_nested+0x16/0x20 kernel/locking/mutex.c:1088
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
  mon_bin_vma_fault+0xdc/0x4a0 drivers/usb/mon/mon_bin.c:1237
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
  __do_fault+0xee/0x450 mm/memory.c:3240
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
  do_read_fault mm/memory.c:3652 [inline]
  do_fault mm/memory.c:3752 [inline]
  handle_pte_fault mm/memory.c:3983 [inline]
  __handle_mm_fault+0x2b4a/0x4350 mm/memory.c:4107
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
  handle_mm_fault+0x53e/0xc80 mm/memory.c:4144
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
  faultin_page mm/gup.c:518 [inline]
  __get_user_pages+0x823/0x1b50 mm/gup.c:718
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
  populate_vma_page_range+0x2db/0x3d0 mm/gup.c:1222
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
  __mm_populate+0x286/0x4d0 mm/gup.c:1270
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
  mm_populate include/linux/mm.h:2307 [inline]
  vm_mmap_pgoff+0x27f/0x2c0 mm/util.c:362
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
  ksys_mmap_pgoff+0x4da/0x660 mm/mmap.c:1585
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
  __do_sys_mmap arch/x86/kernel/sys_x86_64.c:100 [inline]
  __se_sys_mmap arch/x86/kernel/sys_x86_64.c:91 [inline]
  __x64_sys_mmap+0xe9/0x1b0 arch/x86/kernel/sys_x86_64.c:91
  do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
  entry_SYSCALL_64_after_hwframe+0x49/0xbe
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
RIP: 0033:0x444c09
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
Code: e8 ac e8 ff ff 48 83 c4 18 c3 0f 1f 80 00 00 00 00 48 89 f8 48 89 f7  
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff  
ff 0f 83 2b ce fb ff c3 66 2e 0f 1f 84 00 00 00 00
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
RSP: 002b:00007ffc31ed6738 EFLAGS: 00000216 ORIG_RAX: 0000000000000009
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 0000000000444c09
RDX: 0000000001fffffd RSI: 0000000000400000 RDI: 0000000020a05000
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
RBP: 0000000000000000 R08: 0000000000000005 R09: 0000000000000000
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
R10: 0000000000008011 R11: 0000000000000216 R12: 0000000000401ec0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
R13: 0000000000401f50 R14: 0000000000000000 R15: 0000000000000000
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0
vhci_hcd: default hub control req: 0000 v0000 i0000 l0


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

Re: possible deadlock in mon_bin_vma_fault

[syzbot]

syzbot has bisected this bug to:

commit 46eb14a6e1585d99c1b9f58d0e7389082a5f466b
Author: Pete Zaitcev <zaitcev@redhat.com>
Date:   Mon Jan 8 21:46:41 2018 +0000

     USB: fix usbmon BUG trigger

bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=14deb012e00000
start commit:   58c3f14f Merge tag 'riscv-for-linus-4.19-rc2' of git://git..
git tree:       upstream
final crash:    https://syzkaller.appspot.com/x/report.txt?x=16deb012e00000
console output: https://syzkaller.appspot.com/x/log.txt?x=12deb012e00000
kernel config:  https://syzkaller.appspot.com/x/.config?x=531a917630d2a492
dashboard link: https://syzkaller.appspot.com/bug?extid=56f9673bb4cdcbeb0e92
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=13dca13e400000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=17cbe492400000

Reported-by: syzbot+56f9673bb4cdcbeb0e92@syzkaller.appspotmail.com
Fixes: 46eb14a6e158 ("USB: fix usbmon BUG trigger")

For information about bisection process see: https://goo.gl/tpsmEJ#bisection

Re: possible deadlock in mon_bin_vma_fault

[Alan Stern]

On Wed, 20 Nov 2019, syzbot wrote:

> syzbot has bisected this bug to:
> 
> commit 46eb14a6e1585d99c1b9f58d0e7389082a5f466b
> Author: Pete Zaitcev <zaitcev@redhat.com>
> Date:   Mon Jan 8 21:46:41 2018 +0000
> 
>      USB: fix usbmon BUG trigger

Here's part of the commit description:

    USB: fix usbmon BUG trigger
    
    Automated tests triggered this by opening usbmon and accessing the
    mmap while simultaneously resizing the buffers. This bug was with
    us since 2006, because typically applications only size the buffers
    once and thus avoid racing. Reported by Kirill A. Shutemov.

As it happens, I spent a little time investigating this bug report just
yesterday.  It seems to me that the easiest fix would be to disallow
resizing the buffer while it is mapped by any users.  (Besides,
allowing that seems like a bad idea in any case.)

Pete, does that seem reasonable to you?

Alan Stern

Re: possible deadlock in mon_bin_vma_fault

[Pete Zaitcev]

On Wed, 20 Nov 2019 11:14:05 -0500 (EST)
Alan Stern <stern@rowland.harvard.edu> wrote:

> As it happens, I spent a little time investigating this bug report just
> yesterday.  It seems to me that the easiest fix would be to disallow
> resizing the buffer while it is mapped by any users.  (Besides,
> allowing that seems like a bad idea in any case.)
> 
> Pete, does that seem reasonable to you?

Yes, it does seem reasonable.

I think I understand it now. My fallacy was thinking that since everything
is nailed down as long as fetch_lock is held, it was okay to grab whatever
page from our pagemap. What happens later is an attempt to get pages of the
new buffer while looking at them through the old VMA, in mon_bin_vma_fault.

It seems to me that the use counter, mmap_active, is correct and sufficient
to check in the ioctl.

-- Pete

P.S. One thing that vaguely bothers me on this is that the bot
bisected to the commit that clearly fixed worse issues.

P.P.S. Like this?

diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
index ac2b4fcc265f..e27d99606adb 100644
--- a/drivers/usb/mon/mon_bin.c
+++ b/drivers/usb/mon/mon_bin.c
@@ -1020,6 +1020,9 @@ static long mon_bin_ioctl(struct file *file, unsigned int cmd, unsigned long arg
                int size;
                struct mon_pgmap *vec;
 
+               if (rp->mmap_active)
+                       return -EBUSY;
+
                if (arg < BUFF_MIN || arg > BUFF_MAX)
                        return -EINVAL;
 

Re: possible deadlock in mon_bin_vma_fault

[Pete Zaitcev]

On Wed, 20 Nov 2019 11:14:05 -0500 (EST)
Alan Stern <stern@rowland.harvard.edu> wrote:

> As it happens, I spent a little time investigating this bug report just
> yesterday.  It seems to me that the easiest fix would be to disallow
> resizing the buffer while it is mapped by any users.  (Besides,
> allowing that seems like a bad idea in any case.)
> 
> Pete, does that seem reasonable to you?

Actually, please hold on a little, I think to think more about this.
The deadlock is between mon_bin_read and mon_bin_vma_fault.
To disallow resizing isn't going to fix _that_.

-- Pete

Re: possible deadlock in mon_bin_vma_fault

[Alan Stern]

On Wed, 20 Nov 2019, Pete Zaitcev wrote:

> On Wed, 20 Nov 2019 11:14:05 -0500 (EST)
> Alan Stern <stern@rowland.harvard.edu> wrote:
> 
> > As it happens, I spent a little time investigating this bug report just
> > yesterday.  It seems to me that the easiest fix would be to disallow
> > resizing the buffer while it is mapped by any users.  (Besides,
> > allowing that seems like a bad idea in any case.)
> > 
> > Pete, does that seem reasonable to you?
> 
> Actually, please hold on a little, I think to think more about this.
> The deadlock is between mon_bin_read and mon_bin_vma_fault.
> To disallow resizing isn't going to fix _that_.

As I understand it (and my understanding is pretty limited, since I
only started to look seriously at the code one day ago), the reason why
mon_bin_vma_fault acquires fetch_lock is to prevent a resize from
happening while the fault is being handled.  Is there another reason?

If you disallow resizing while the buffer is mapped then 
mon_bin_vma_fault won't need to hold fetch_lock at all.  That would fix 
the deadlock, right?

Alan Stern

Re: possible deadlock in mon_bin_vma_fault

[Alan Stern]

On Wed, 20 Nov 2019, Pete Zaitcev wrote:

> On Wed, 20 Nov 2019 11:14:05 -0500 (EST)
> Alan Stern <stern@rowland.harvard.edu> wrote:
> 
> > As it happens, I spent a little time investigating this bug report just
> > yesterday.  It seems to me that the easiest fix would be to disallow
> > resizing the buffer while it is mapped by any users.  (Besides,
> > allowing that seems like a bad idea in any case.)
> > 
> > Pete, does that seem reasonable to you?
> 
> Yes, it does seem reasonable.
> 
> I think I understand it now. My fallacy was thinking that since everything
> is nailed down as long as fetch_lock is held, it was okay to grab whatever
> page from our pagemap. What happens later is an attempt to get pages of the
> new buffer while looking at them through the old VMA, in mon_bin_vma_fault.
> 
> It seems to me that the use counter, mmap_active, is correct and sufficient
> to check in the ioctl.
> 
> -- Pete
> 
> P.S. One thing that vaguely bothers me on this is that the bot
> bisected to the commit that clearly fixed worse issues.
> 
> P.P.S. Like this?
> 
> diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
> index ac2b4fcc265f..e27d99606adb 100644
> --- a/drivers/usb/mon/mon_bin.c
> +++ b/drivers/usb/mon/mon_bin.c
> @@ -1020,6 +1020,9 @@ static long mon_bin_ioctl(struct file *file, unsigned int cmd, unsigned long arg
>                 int size;
>                 struct mon_pgmap *vec;
>  
> +               if (rp->mmap_active)
> +                       return -EBUSY;
> +
>                 if (arg < BUFF_MIN || arg > BUFF_MAX)
>                         return -EINVAL;

Like that, yes, but the test has to be made while fetch_lock is held.  
Otherwise there's still a race: One thread could pass the test and then
do the resize, and in between another thread could map the buffer and
incur a fault.

Incidentally, the comment for fetch_lock says that it protects b_read 
and b_out, but mon_bin_vma_fault doesn't use either of those fields.

Alan Stern

Re: possible deadlock in mon_bin_vma_fault

[Pete Zaitcev]

On Wed, 20 Nov 2019 13:47:00 -0500 (EST)
Alan Stern <stern@rowland.harvard.edu> wrote:

> > +               if (rp->mmap_active)
> > +                       return -EBUSY;

> Like that, yes, but the test has to be made while fetch_lock is held.  

Certainly, thanks. I was rushing just to add a postscriptum.

> Incidentally, the comment for fetch_lock says that it protects b_read 
> and b_out, but mon_bin_vma_fault doesn't use either of those fields.

I probably should change that comment to "protect the integrity of the
circular buffer, such as b_out".

Anyway... If you are looking at it too, what do you think about not using
any locks in mon_bin_vma_fault() at all? Isn't it valid? I think I tried
to be "safe", but it only uses things that are constants unless we're
opening and closing; a process cannot make page faults unless it has
some thing mapped; and that is only possible if device is open and stays
open. Can you find a hole in this reasoning?

-- Pete

Re: possible deadlock in mon_bin_vma_fault

[Alan Stern]

On Thu, 21 Nov 2019, Pete Zaitcev wrote:

> Anyway... If you are looking at it too, what do you think about not using
> any locks in mon_bin_vma_fault() at all? Isn't it valid? I think I tried
> to be "safe", but it only uses things that are constants unless we're
> opening and closing; a process cannot make page faults unless it has
> some thing mapped; and that is only possible if device is open and stays
> open. Can you find a hole in this reasoning?

I think you're right.  But one thing concerns me: What happens if the 
same buffer is mapped by more than one process?  Do you allow that?  I 
haven't read the code in enough detail to see.

Alan Stern

Re: possible deadlock in mon_bin_vma_fault

[Pete Zaitcev]

On Thu, 21 Nov 2019 11:20:20 -0500 (EST)
Alan Stern <stern@rowland.harvard.edu> wrote:

> On Thu, 21 Nov 2019, Pete Zaitcev wrote:
> 
> > Anyway... If you are looking at it too, what do you think about not using
> > any locks in mon_bin_vma_fault() at all? Isn't it valid? I think I tried
> > to be "safe", but it only uses things that are constants unless we're
> > opening and closing; a process cannot make page faults unless it has
> > some thing mapped; and that is only possible if device is open and stays
> > open. Can you find a hole in this reasoning?  
> 
> I think you're right.  But one thing concerns me: What happens if the 
> same buffer is mapped by more than one process?  Do you allow that?

Yes, we allow 2 processes reading from mmap in the same time.
They may miss events, but there should be no issue to the internal
consistency of any pointers in usbmon, and no crashes or deadlocks.
Also, we cannot prohibit that. Imagine a process that does open(),
mmap(), fork()/clone().

-- Pete

Re: possible deadlock in mon_bin_vma_fault

[Pete Zaitcev]

On Thu, 21 Nov 2019 11:20:20 -0500 (EST)
Alan Stern <stern@rowland.harvard.edu> wrote:

> On Thu, 21 Nov 2019, Pete Zaitcev wrote:
> 
> > Anyway... If you are looking at it too, what do you think about not using
> > any locks in mon_bin_vma_fault() at all? Isn't it valid? I think I tried
> > to be "safe", but it only uses things that are constants unless we're
> > opening and closing; a process cannot make page faults unless it has
> > some thing mapped; and that is only possible if device is open and stays
> > open. Can you find a hole in this reasoning?  
> 
> I think you're right. [...]

How about the appended patch, then? You like?

Do you happen to know how to refer to a syzbot report in a commit message?

-- Pete

commit 628f3bbf37eee21cce4cfbfaa6a796b129d7736d
Author: Pete Zaitcev <zaitcev@kotori.zaitcev.us>
Date:   Thu Nov 21 17:24:00 2019 -0600

    usb: Fix a deadlock in usbmon between mmap and read
    
    Signed-off-by: Pete Zaitcev <zaitcev@redhat.com>

diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
index ac2b4fcc265f..fb7df9810bad 100644
--- a/drivers/usb/mon/mon_bin.c
+++ b/drivers/usb/mon/mon_bin.c
@@ -1039,12 +1039,18 @@ static long mon_bin_ioctl(struct file *file, unsigned int cmd, unsigned long arg
 
 		mutex_lock(&rp->fetch_lock);
 		spin_lock_irqsave(&rp->b_lock, flags);
-		mon_free_buff(rp->b_vec, rp->b_size/CHUNK_SIZE);
-		kfree(rp->b_vec);
-		rp->b_vec  = vec;
-		rp->b_size = size;
-		rp->b_read = rp->b_in = rp->b_out = rp->b_cnt = 0;
-		rp->cnt_lost = 0;
+		if (rp->mmap_active) {
+			mon_free_buff(vec, size/CHUNK_SIZE);
+			kfree(vec);
+			ret = -EBUSY;
+		} else {
+			mon_free_buff(rp->b_vec, rp->b_size/CHUNK_SIZE);
+			kfree(rp->b_vec);
+			rp->b_vec  = vec;
+			rp->b_size = size;
+			rp->b_read = rp->b_in = rp->b_out = rp->b_cnt = 0;
+			rp->cnt_lost = 0;
+		}
 		spin_unlock_irqrestore(&rp->b_lock, flags);
 		mutex_unlock(&rp->fetch_lock);
 		}
@@ -1093,11 +1099,11 @@ static long mon_bin_ioctl(struct file *file, unsigned int cmd, unsigned long arg
 			return ret;
 		if (put_user(ret, &uptr->nfetch))
 			return -EFAULT;
-		ret = 0;
 		}
 		break;
 
-	case MON_IOCG_STATS: {
+	case MON_IOCG_STATS:
+		{
 		struct mon_bin_stats __user *sp;
 		unsigned int nevents;
 		unsigned int ndropped;
@@ -1113,7 +1119,6 @@ static long mon_bin_ioctl(struct file *file, unsigned int cmd, unsigned long arg
 			return -EFAULT;
 		if (put_user(nevents, &sp->queued))
 			return -EFAULT;
-
 		}
 		break;
 
@@ -1216,13 +1221,21 @@ mon_bin_poll(struct file *file, struct poll_table_struct *wait)
 static void mon_bin_vma_open(struct vm_area_struct *vma)
 {
 	struct mon_reader_bin *rp = vma->vm_private_data;
+	unsigned long flags;
+
+	spin_lock_irqsave(&rp->b_lock, flags);
 	rp->mmap_active++;
+	spin_unlock_irqrestore(&rp->b_lock, flags);
 }
 
 static void mon_bin_vma_close(struct vm_area_struct *vma)
 {
+	unsigned long flags;
+
 	struct mon_reader_bin *rp = vma->vm_private_data;
+	spin_lock_irqsave(&rp->b_lock, flags);
 	rp->mmap_active--;
+	spin_unlock_irqrestore(&rp->b_lock, flags);
 }
 
 /*
@@ -1234,16 +1247,12 @@ static vm_fault_t mon_bin_vma_fault(struct vm_fault *vmf)
 	unsigned long offset, chunk_idx;
 	struct page *pageptr;
 
-	mutex_lock(&rp->fetch_lock);
 	offset = vmf->pgoff << PAGE_SHIFT;
-	if (offset >= rp->b_size) {
-		mutex_unlock(&rp->fetch_lock);
+	if (offset >= rp->b_size)
 		return VM_FAULT_SIGBUS;
-	}
 	chunk_idx = offset / CHUNK_SIZE;
 	pageptr = rp->b_vec[chunk_idx].pg;
 	get_page(pageptr);
-	mutex_unlock(&rp->fetch_lock);
 	vmf->page = pageptr;
 	return 0;
 }

Re: possible deadlock in mon_bin_vma_fault

[Dmitry Vyukov]

On Fri, Nov 22, 2019 at 12:38 AM Pete Zaitcev <zaitcev@redhat.com> wrote:
>
> On Thu, 21 Nov 2019 11:20:20 -0500 (EST)
> Alan Stern <stern@rowland.harvard.edu> wrote:
>
> > On Thu, 21 Nov 2019, Pete Zaitcev wrote:
> >
> > > Anyway... If you are looking at it too, what do you think about not using
> > > any locks in mon_bin_vma_fault() at all? Isn't it valid? I think I tried
> > > to be "safe", but it only uses things that are constants unless we're
> > > opening and closing; a process cannot make page faults unless it has
> > > some thing mapped; and that is only possible if device is open and stays
> > > open. Can you find a hole in this reasoning?
> >
> > I think you're right. [...]
>
> How about the appended patch, then? You like?
>
> Do you happen to know how to refer to a syzbot report in a commit message?
>
> -- Pete
>
> commit 628f3bbf37eee21cce4cfbfaa6a796b129d7736d
> Author: Pete Zaitcev <zaitcev@kotori.zaitcev.us>
> Date:   Thu Nov 21 17:24:00 2019 -0600
>
>     usb: Fix a deadlock in usbmon between mmap and read
>
>     Signed-off-by: Pete Zaitcev <zaitcev@redhat.com>

/\/\/\/\/\/\/\/\/\/\

Please don't forget the Reported-by

> diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
> index ac2b4fcc265f..fb7df9810bad 100644
> --- a/drivers/usb/mon/mon_bin.c
> +++ b/drivers/usb/mon/mon_bin.c
> @@ -1039,12 +1039,18 @@ static long mon_bin_ioctl(struct file *file, unsigned int cmd, unsigned long arg
>
>                 mutex_lock(&rp->fetch_lock);
>                 spin_lock_irqsave(&rp->b_lock, flags);
> -               mon_free_buff(rp->b_vec, rp->b_size/CHUNK_SIZE);
> -               kfree(rp->b_vec);
> -               rp->b_vec  = vec;
> -               rp->b_size = size;
> -               rp->b_read = rp->b_in = rp->b_out = rp->b_cnt = 0;
> -               rp->cnt_lost = 0;
> +               if (rp->mmap_active) {
> +                       mon_free_buff(vec, size/CHUNK_SIZE);
> +                       kfree(vec);
> +                       ret = -EBUSY;
> +               } else {
> +                       mon_free_buff(rp->b_vec, rp->b_size/CHUNK_SIZE);
> +                       kfree(rp->b_vec);
> +                       rp->b_vec  = vec;
> +                       rp->b_size = size;
> +                       rp->b_read = rp->b_in = rp->b_out = rp->b_cnt = 0;
> +                       rp->cnt_lost = 0;
> +               }
>                 spin_unlock_irqrestore(&rp->b_lock, flags);
>                 mutex_unlock(&rp->fetch_lock);
>                 }
> @@ -1093,11 +1099,11 @@ static long mon_bin_ioctl(struct file *file, unsigned int cmd, unsigned long arg
>                         return ret;
>                 if (put_user(ret, &uptr->nfetch))
>                         return -EFAULT;
> -               ret = 0;
>                 }
>                 break;
>
> -       case MON_IOCG_STATS: {
> +       case MON_IOCG_STATS:
> +               {
>                 struct mon_bin_stats __user *sp;
>                 unsigned int nevents;
>                 unsigned int ndropped;
> @@ -1113,7 +1119,6 @@ static long mon_bin_ioctl(struct file *file, unsigned int cmd, unsigned long arg
>                         return -EFAULT;
>                 if (put_user(nevents, &sp->queued))
>                         return -EFAULT;
> -
>                 }
>                 break;
>
> @@ -1216,13 +1221,21 @@ mon_bin_poll(struct file *file, struct poll_table_struct *wait)
>  static void mon_bin_vma_open(struct vm_area_struct *vma)
>  {
>         struct mon_reader_bin *rp = vma->vm_private_data;
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&rp->b_lock, flags);
>         rp->mmap_active++;
> +       spin_unlock_irqrestore(&rp->b_lock, flags);
>  }
>
>  static void mon_bin_vma_close(struct vm_area_struct *vma)
>  {
> +       unsigned long flags;
> +
>         struct mon_reader_bin *rp = vma->vm_private_data;
> +       spin_lock_irqsave(&rp->b_lock, flags);
>         rp->mmap_active--;
> +       spin_unlock_irqrestore(&rp->b_lock, flags);
>  }
>
>  /*
> @@ -1234,16 +1247,12 @@ static vm_fault_t mon_bin_vma_fault(struct vm_fault *vmf)
>         unsigned long offset, chunk_idx;
>         struct page *pageptr;
>
> -       mutex_lock(&rp->fetch_lock);
>         offset = vmf->pgoff << PAGE_SHIFT;
> -       if (offset >= rp->b_size) {
> -               mutex_unlock(&rp->fetch_lock);
> +       if (offset >= rp->b_size)
>                 return VM_FAULT_SIGBUS;
> -       }
>         chunk_idx = offset / CHUNK_SIZE;
>         pageptr = rp->b_vec[chunk_idx].pg;
>         get_page(pageptr);
> -       mutex_unlock(&rp->fetch_lock);
>         vmf->page = pageptr;
>         return 0;
>  }
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/20191121173825.1527c3a5%40suzdal.zaitcev.lan.

Re: possible deadlock in mon_bin_vma_fault

[Alan Stern]

On Thu, 21 Nov 2019, Pete Zaitcev wrote:

> On Thu, 21 Nov 2019 11:20:20 -0500 (EST)
> Alan Stern <stern@rowland.harvard.edu> wrote:
> 
> > On Thu, 21 Nov 2019, Pete Zaitcev wrote:
> > 
> > > Anyway... If you are looking at it too, what do you think about not using
> > > any locks in mon_bin_vma_fault() at all? Isn't it valid? I think I tried
> > > to be "safe", but it only uses things that are constants unless we're
> > > opening and closing; a process cannot make page faults unless it has
> > > some thing mapped; and that is only possible if device is open and stays
> > > open. Can you find a hole in this reasoning?  
> > 
> > I think you're right. [...]
> 
> How about the appended patch, then? You like?
> 
> Do you happen to know how to refer to a syzbot report in a commit message?

As Dmitry mentioned, you should put the Reported-by: line from the
original syzbot bug report (see
<https://marc.info/?l=linux-usb&m=153601206710985&w=2>) in the patch.

> -- Pete
> 
> commit 628f3bbf37eee21cce4cfbfaa6a796b129d7736d
> Author: Pete Zaitcev <zaitcev@kotori.zaitcev.us>
> Date:   Thu Nov 21 17:24:00 2019 -0600
> 
>     usb: Fix a deadlock in usbmon between mmap and read
>     
>     Signed-off-by: Pete Zaitcev <zaitcev@redhat.com>
> 
> diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
> index ac2b4fcc265f..fb7df9810bad 100644
> --- a/drivers/usb/mon/mon_bin.c
> +++ b/drivers/usb/mon/mon_bin.c
> @@ -1039,12 +1039,18 @@ static long mon_bin_ioctl(struct file *file, unsigned int cmd, unsigned long arg
>  
>  		mutex_lock(&rp->fetch_lock);
>  		spin_lock_irqsave(&rp->b_lock, flags);
> -		mon_free_buff(rp->b_vec, rp->b_size/CHUNK_SIZE);
> -		kfree(rp->b_vec);
> -		rp->b_vec  = vec;
> -		rp->b_size = size;
> -		rp->b_read = rp->b_in = rp->b_out = rp->b_cnt = 0;
> -		rp->cnt_lost = 0;
> +		if (rp->mmap_active) {
> +			mon_free_buff(vec, size/CHUNK_SIZE);
> +			kfree(vec);
> +			ret = -EBUSY;

It would be more elegant to do the rp->mmap_active test before calling
kcalloc and mon_alloc_buf.  But of course that's a pretty minor thing.

> +		} else {
> +			mon_free_buff(rp->b_vec, rp->b_size/CHUNK_SIZE);
> +			kfree(rp->b_vec);
> +			rp->b_vec  = vec;
> +			rp->b_size = size;
> +			rp->b_read = rp->b_in = rp->b_out = rp->b_cnt = 0;
> +			rp->cnt_lost = 0;
> +		}
>  		spin_unlock_irqrestore(&rp->b_lock, flags);
>  		mutex_unlock(&rp->fetch_lock);
>  		}
> @@ -1093,11 +1099,11 @@ static long mon_bin_ioctl(struct file *file, unsigned int cmd, unsigned long arg
>  			return ret;
>  		if (put_user(ret, &uptr->nfetch))
>  			return -EFAULT;
> -		ret = 0;

What's the reason for this change?

>  		}
>  		break;
>  
> -	case MON_IOCG_STATS: {
> +	case MON_IOCG_STATS:
> +		{

And this one?  This disagrees with the usual kernel style.

>  		struct mon_bin_stats __user *sp;
>  		unsigned int nevents;
>  		unsigned int ndropped;
> @@ -1113,7 +1119,6 @@ static long mon_bin_ioctl(struct file *file, unsigned int cmd, unsigned long arg
>  			return -EFAULT;
>  		if (put_user(nevents, &sp->queued))
>  			return -EFAULT;
> -
>  		}
>  		break;
>  
> @@ -1216,13 +1221,21 @@ mon_bin_poll(struct file *file, struct poll_table_struct *wait)
>  static void mon_bin_vma_open(struct vm_area_struct *vma)
>  {
>  	struct mon_reader_bin *rp = vma->vm_private_data;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&rp->b_lock, flags);
>  	rp->mmap_active++;
> +	spin_unlock_irqrestore(&rp->b_lock, flags);
>  }
>  
>  static void mon_bin_vma_close(struct vm_area_struct *vma)
>  {
> +	unsigned long flags;
> +
>  	struct mon_reader_bin *rp = vma->vm_private_data;
> +	spin_lock_irqsave(&rp->b_lock, flags);
>  	rp->mmap_active--;
> +	spin_unlock_irqrestore(&rp->b_lock, flags);
>  }
>  
>  /*
> @@ -1234,16 +1247,12 @@ static vm_fault_t mon_bin_vma_fault(struct vm_fault *vmf)
>  	unsigned long offset, chunk_idx;
>  	struct page *pageptr;
>  
> -	mutex_lock(&rp->fetch_lock);
>  	offset = vmf->pgoff << PAGE_SHIFT;
> -	if (offset >= rp->b_size) {
> -		mutex_unlock(&rp->fetch_lock);
> +	if (offset >= rp->b_size)
>  		return VM_FAULT_SIGBUS;
> -	}
>  	chunk_idx = offset / CHUNK_SIZE;
>  	pageptr = rp->b_vec[chunk_idx].pg;
>  	get_page(pageptr);
> -	mutex_unlock(&rp->fetch_lock);
>  	vmf->page = pageptr;
>  	return 0;
>  }

Apart from the items mentioned above, this looks right to me.

Alan Stern

Re: possible deadlock in mon_bin_vma_fault

[Pete Zaitcev]

On Fri, 22 Nov 2019 10:27:10 -0500 (EST)
Alan Stern <stern@rowland.harvard.edu> wrote:

> As Dmitry mentioned, you should put the Reported-by: line from the
> original syzbot bug report (see
> <https://marc.info/?l=linux-usb&m=153601206710985&w=2>) in the patch.

Thanks, got it. I also dropped all the cosmetic changes.

> >  		mutex_lock(&rp->fetch_lock);
> >  		spin_lock_irqsave(&rp->b_lock, flags);
> > -		mon_free_buff(rp->b_vec, rp->b_size/CHUNK_SIZE);
> > -		kfree(rp->b_vec);
> > -		rp->b_vec  = vec;
> > -		rp->b_size = size;
> > -		rp->b_read = rp->b_in = rp->b_out = rp->b_cnt = 0;
> > -		rp->cnt_lost = 0;
> > +		if (rp->mmap_active) {
> > +			mon_free_buff(vec, size/CHUNK_SIZE);
> > +			kfree(vec);
> > +			ret = -EBUSY;  
> 
> It would be more elegant to do the rp->mmap_active test before calling
> kcalloc and mon_alloc_buf.  But of course that's a pretty minor thing.

Indeed it feels wrong that so much work gets discarded. However, memory
allocations can block, right? In the same time, our main objective here is
to make sure that when a page fault happens, we fill in the page that VMA
is intended to refer, and not one that was re-allocated. Therefore, I'm
trying to avoid a situation where:

1. thread A checks mmap_active, finds it at zero and proceeds into the
reallocation ioctl
2. thread A sleeps in get_free_page()
3. thread B runs mmap() and succeeds
4. thread A obtains its pages and proceeds to substitute the buffer
5. thread B (or any other) pagefaults and ends with the new, unexpected page

The code is not pretty, but I don't see an alternative. Heck, I would
love you to find more races if you can.

-- Pete

commit 5252eb4c8297fedbf1c5f1e67da44efe00e6ef6b
Author: Pete Zaitcev <zaitcev@kotori.zaitcev.us>
Date:   Thu Nov 21 17:24:00 2019 -0600

    usb: Fix a deadlock in usbmon between mmap and read
    
    Signed-off-by: Pete Zaitcev <zaitcev@redhat.com>
    Reported-by: syzbot+56f9673bb4cdcbeb0e92@syzkaller.appspotmail.com

diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
index ac2b4fcc265f..f48a23adbc35 100644
--- a/drivers/usb/mon/mon_bin.c
+++ b/drivers/usb/mon/mon_bin.c
@@ -1039,12 +1039,18 @@ static long mon_bin_ioctl(struct file *file, unsigned int cmd, unsigned long arg
 
 		mutex_lock(&rp->fetch_lock);
 		spin_lock_irqsave(&rp->b_lock, flags);
-		mon_free_buff(rp->b_vec, rp->b_size/CHUNK_SIZE);
-		kfree(rp->b_vec);
-		rp->b_vec  = vec;
-		rp->b_size = size;
-		rp->b_read = rp->b_in = rp->b_out = rp->b_cnt = 0;
-		rp->cnt_lost = 0;
+		if (rp->mmap_active) {
+			mon_free_buff(vec, size/CHUNK_SIZE);
+			kfree(vec);
+			ret = -EBUSY;
+		} else {
+			mon_free_buff(rp->b_vec, rp->b_size/CHUNK_SIZE);
+			kfree(rp->b_vec);
+			rp->b_vec  = vec;
+			rp->b_size = size;
+			rp->b_read = rp->b_in = rp->b_out = rp->b_cnt = 0;
+			rp->cnt_lost = 0;
+		}
 		spin_unlock_irqrestore(&rp->b_lock, flags);
 		mutex_unlock(&rp->fetch_lock);
 		}
@@ -1216,13 +1222,21 @@ mon_bin_poll(struct file *file, struct poll_table_struct *wait)
 static void mon_bin_vma_open(struct vm_area_struct *vma)
 {
 	struct mon_reader_bin *rp = vma->vm_private_data;
+	unsigned long flags;
+
+	spin_lock_irqsave(&rp->b_lock, flags);
 	rp->mmap_active++;
+	spin_unlock_irqrestore(&rp->b_lock, flags);
 }
 
 static void mon_bin_vma_close(struct vm_area_struct *vma)
 {
+	unsigned long flags;
+
 	struct mon_reader_bin *rp = vma->vm_private_data;
+	spin_lock_irqsave(&rp->b_lock, flags);
 	rp->mmap_active--;
+	spin_unlock_irqrestore(&rp->b_lock, flags);
 }
 
 /*
@@ -1234,16 +1248,12 @@ static vm_fault_t mon_bin_vma_fault(struct vm_fault *vmf)
 	unsigned long offset, chunk_idx;
 	struct page *pageptr;
 
-	mutex_lock(&rp->fetch_lock);
 	offset = vmf->pgoff << PAGE_SHIFT;
-	if (offset >= rp->b_size) {
-		mutex_unlock(&rp->fetch_lock);
+	if (offset >= rp->b_size)
 		return VM_FAULT_SIGBUS;
-	}
 	chunk_idx = offset / CHUNK_SIZE;
 	pageptr = rp->b_vec[chunk_idx].pg;
 	get_page(pageptr);
-	mutex_unlock(&rp->fetch_lock);
 	vmf->page = pageptr;
 	return 0;
 }

Re: possible deadlock in mon_bin_vma_fault

[Alan Stern]

On Fri, 22 Nov 2019, Pete Zaitcev wrote:

> > It would be more elegant to do the rp->mmap_active test before calling
> > kcalloc and mon_alloc_buf.  But of course that's a pretty minor thing.
> 
> Indeed it feels wrong that so much work gets discarded. However, memory
> allocations can block, right? In the same time, our main objective here is
> to make sure that when a page fault happens, we fill in the page that VMA
> is intended to refer, and not one that was re-allocated. Therefore, I'm
> trying to avoid a situation where:
> 
> 1. thread A checks mmap_active, finds it at zero and proceeds into the
> reallocation ioctl
> 2. thread A sleeps in get_free_page()
> 3. thread B runs mmap() and succeeds
> 4. thread A obtains its pages and proceeds to substitute the buffer
> 5. thread B (or any other) pagefaults and ends with the new, unexpected page
> 
> The code is not pretty, but I don't see an alternative. Heck, I would
> love you to find more races if you can.

The alternative is to have the routines for mmap() hold fetch_lock
instead of b_lock.  mmap() is allowed to sleep, so that would be okay.  
Then you would also hold fetch_lock while checking mmap_active and
doing the memory allocations.  That would prevent any races -- in your
example above, thread A would acquire fetch_lock in step 1, so thread B
would block in step 3 until step 4 was finished.  Hence B would end up 
mapping the correct pages.

In practice, I don't see this being a routine problem.  How often do 
multiple threads independently try to mmap the same usbmon buffer?

Still, let's see syzbot reacts to your current patch.  The line below 
is how you ask syzbot to test a candidate patch.

Alan Stern

#syz test: linux-4.19.y f6e27dbb1afa

commit 5252eb4c8297fedbf1c5f1e67da44efe00e6ef6b
Author: Pete Zaitcev <zaitcev@kotori.zaitcev.us>
Date:   Thu Nov 21 17:24:00 2019 -0600

    usb: Fix a deadlock in usbmon between mmap and read
    
    Signed-off-by: Pete Zaitcev <zaitcev@redhat.com>
    Reported-by: syzbot+56f9673bb4cdcbeb0e92@syzkaller.appspotmail.com

diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
index ac2b4fcc265f..f48a23adbc35 100644
--- a/drivers/usb/mon/mon_bin.c
+++ b/drivers/usb/mon/mon_bin.c
@@ -1039,12 +1039,18 @@ static long mon_bin_ioctl(struct file *file, unsigned int cmd, unsigned long arg
 
 		mutex_lock(&rp->fetch_lock);
 		spin_lock_irqsave(&rp->b_lock, flags);
-		mon_free_buff(rp->b_vec, rp->b_size/CHUNK_SIZE);
-		kfree(rp->b_vec);
-		rp->b_vec  = vec;
-		rp->b_size = size;
-		rp->b_read = rp->b_in = rp->b_out = rp->b_cnt = 0;
-		rp->cnt_lost = 0;
+		if (rp->mmap_active) {
+			mon_free_buff(vec, size/CHUNK_SIZE);
+			kfree(vec);
+			ret = -EBUSY;
+		} else {
+			mon_free_buff(rp->b_vec, rp->b_size/CHUNK_SIZE);
+			kfree(rp->b_vec);
+			rp->b_vec  = vec;
+			rp->b_size = size;
+			rp->b_read = rp->b_in = rp->b_out = rp->b_cnt = 0;
+			rp->cnt_lost = 0;
+		}
 		spin_unlock_irqrestore(&rp->b_lock, flags);
 		mutex_unlock(&rp->fetch_lock);
 		}
@@ -1216,13 +1222,21 @@ mon_bin_poll(struct file *file, struct poll_table_struct *wait)
 static void mon_bin_vma_open(struct vm_area_struct *vma)
 {
 	struct mon_reader_bin *rp = vma->vm_private_data;
+	unsigned long flags;
+
+	spin_lock_irqsave(&rp->b_lock, flags);
 	rp->mmap_active++;
+	spin_unlock_irqrestore(&rp->b_lock, flags);
 }
 
 static void mon_bin_vma_close(struct vm_area_struct *vma)
 {
+	unsigned long flags;
+
 	struct mon_reader_bin *rp = vma->vm_private_data;
+	spin_lock_irqsave(&rp->b_lock, flags);
 	rp->mmap_active--;
+	spin_unlock_irqrestore(&rp->b_lock, flags);
 }
 
 /*
@@ -1234,16 +1248,12 @@ static vm_fault_t mon_bin_vma_fault(struct vm_fault *vmf)
 	unsigned long offset, chunk_idx;
 	struct page *pageptr;
 
-	mutex_lock(&rp->fetch_lock);
 	offset = vmf->pgoff << PAGE_SHIFT;
-	if (offset >= rp->b_size) {
-		mutex_unlock(&rp->fetch_lock);
+	if (offset >= rp->b_size)
 		return VM_FAULT_SIGBUS;
-	}
 	chunk_idx = offset / CHUNK_SIZE;
 	pageptr = rp->b_vec[chunk_idx].pg;
 	get_page(pageptr);
-	mutex_unlock(&rp->fetch_lock);
 	vmf->page = pageptr;
 	return 0;
 }

Re: Re: possible deadlock in mon_bin_vma_fault

[syzbot]

> On Fri, 22 Nov 2019, Pete Zaitcev wrote:

>> > It would be more elegant to do the rp->mmap_active test before calling
>> > kcalloc and mon_alloc_buf.  But of course that's a pretty minor thing.

>> Indeed it feels wrong that so much work gets discarded. However, memory
>> allocations can block, right? In the same time, our main objective here  
>> is
>> to make sure that when a page fault happens, we fill in the page that VMA
>> is intended to refer, and not one that was re-allocated. Therefore, I'm
>> trying to avoid a situation where:

>> 1. thread A checks mmap_active, finds it at zero and proceeds into the
>> reallocation ioctl
>> 2. thread A sleeps in get_free_page()
>> 3. thread B runs mmap() and succeeds
>> 4. thread A obtains its pages and proceeds to substitute the buffer
>> 5. thread B (or any other) pagefaults and ends with the new, unexpected  
>> page

>> The code is not pretty, but I don't see an alternative. Heck, I would
>> love you to find more races if you can.

> The alternative is to have the routines for mmap() hold fetch_lock
> instead of b_lock.  mmap() is allowed to sleep, so that would be okay.
> Then you would also hold fetch_lock while checking mmap_active and
> doing the memory allocations.  That would prevent any races -- in your
> example above, thread A would acquire fetch_lock in step 1, so thread B
> would block in step 3 until step 4 was finished.  Hence B would end up
> mapping the correct pages.

> In practice, I don't see this being a routine problem.  How often do
> multiple threads independently try to mmap the same usbmon buffer?

> Still, let's see syzbot reacts to your current patch.  The line below
> is how you ask syzbot to test a candidate patch.

> Alan Stern

> #syz test: linux-4.19.y f6e27dbb1afa

"linux-4.19.y" does not look like a valid git repo address.


> commit 5252eb4c8297fedbf1c5f1e67da44efe00e6ef6b
> Author: Pete Zaitcev <zaitcev@kotori.zaitcev.us>
> Date:   Thu Nov 21 17:24:00 2019 -0600

>      usb: Fix a deadlock in usbmon between mmap and read

>      Signed-off-by: Pete Zaitcev <zaitcev@redhat.com>
>      Reported-by: syzbot+56f9673bb4cdcbeb0e92@syzkaller.appspotmail.com

> diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
> index ac2b4fcc265f..f48a23adbc35 100644
> --- a/drivers/usb/mon/mon_bin.c
> +++ b/drivers/usb/mon/mon_bin.c
> @@ -1039,12 +1039,18 @@ static long mon_bin_ioctl(struct file *file,  
> unsigned int cmd, unsigned long arg

>   		mutex_lock(&rp->fetch_lock);
>   		spin_lock_irqsave(&rp->b_lock, flags);
> -		mon_free_buff(rp->b_vec, rp->b_size/CHUNK_SIZE);
> -		kfree(rp->b_vec);
> -		rp->b_vec  = vec;
> -		rp->b_size = size;
> -		rp->b_read = rp->b_in = rp->b_out = rp->b_cnt = 0;
> -		rp->cnt_lost = 0;
> +		if (rp->mmap_active) {
> +			mon_free_buff(vec, size/CHUNK_SIZE);
> +			kfree(vec);
> +			ret = -EBUSY;
> +		} else {
> +			mon_free_buff(rp->b_vec, rp->b_size/CHUNK_SIZE);
> +			kfree(rp->b_vec);
> +			rp->b_vec  = vec;
> +			rp->b_size = size;
> +			rp->b_read = rp->b_in = rp->b_out = rp->b_cnt = 0;
> +			rp->cnt_lost = 0;
> +		}
>   		spin_unlock_irqrestore(&rp->b_lock, flags);
>   		mutex_unlock(&rp->fetch_lock);
>   		}
> @@ -1216,13 +1222,21 @@ mon_bin_poll(struct file *file, struct  
> poll_table_struct *wait)
>   static void mon_bin_vma_open(struct vm_area_struct *vma)
>   {
>   	struct mon_reader_bin *rp = vma->vm_private_data;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&rp->b_lock, flags);
>   	rp->mmap_active++;
> +	spin_unlock_irqrestore(&rp->b_lock, flags);
>   }

>   static void mon_bin_vma_close(struct vm_area_struct *vma)
>   {
> +	unsigned long flags;
> +
>   	struct mon_reader_bin *rp = vma->vm_private_data;
> +	spin_lock_irqsave(&rp->b_lock, flags);
>   	rp->mmap_active--;
> +	spin_unlock_irqrestore(&rp->b_lock, flags);
>   }

>   /*
> @@ -1234,16 +1248,12 @@ static vm_fault_t mon_bin_vma_fault(struct  
> vm_fault *vmf)
>   	unsigned long offset, chunk_idx;
>   	struct page *pageptr;

> -	mutex_lock(&rp->fetch_lock);
>   	offset = vmf->pgoff << PAGE_SHIFT;
> -	if (offset >= rp->b_size) {
> -		mutex_unlock(&rp->fetch_lock);
> +	if (offset >= rp->b_size)
>   		return VM_FAULT_SIGBUS;
> -	}
>   	chunk_idx = offset / CHUNK_SIZE;
>   	pageptr = rp->b_vec[chunk_idx].pg;
>   	get_page(pageptr);
> -	mutex_unlock(&rp->fetch_lock);
>   	vmf->page = pageptr;
>   	return 0;
>   }

Re: Re: possible deadlock in mon_bin_vma_fault

[syzbot]

> On Fri, 22 Nov 2019, Pete Zaitcev wrote:

>> > It would be more elegant to do the rp->mmap_active test before calling
>> > kcalloc and mon_alloc_buf.  But of course that's a pretty minor thing.

>> Indeed it feels wrong that so much work gets discarded. However, memory
>> allocations can block, right? In the same time, our main objective here  
>> is
>> to make sure that when a page fault happens, we fill in the page that VMA
>> is intended to refer, and not one that was re-allocated. Therefore, I'm
>> trying to avoid a situation where:

>> 1. thread A checks mmap_active, finds it at zero and proceeds into the
>> reallocation ioctl
>> 2. thread A sleeps in get_free_page()
>> 3. thread B runs mmap() and succeeds
>> 4. thread A obtains its pages and proceeds to substitute the buffer
>> 5. thread B (or any other) pagefaults and ends with the new, unexpected  
>> page

>> The code is not pretty, but I don't see an alternative. Heck, I would
>> love you to find more races if you can.

> The alternative is to have the routines for mmap() hold fetch_lock
> instead of b_lock.  mmap() is allowed to sleep, so that would be okay.
> Then you would also hold fetch_lock while checking mmap_active and
> doing the memory allocations.  That would prevent any races -- in your
> example above, thread A would acquire fetch_lock in step 1, so thread B
> would block in step 3 until step 4 was finished.  Hence B would end up
> mapping the correct pages.

> In practice, I don't see this being a routine problem.  How often do
> multiple threads independently try to mmap the same usbmon buffer?

> Still, let's see syzbot reacts to your current patch.  The line below
> is how you ask syzbot to test a candidate patch.

> Alan Stern

> #syz test: linux-4.19.y f6e27dbb1afa

"linux-4.19.y" does not look like a valid git repo address.


> commit 5252eb4c8297fedbf1c5f1e67da44efe00e6ef6b
> Author: Pete Zaitcev <zaitcev@kotori.zaitcev.us>
> Date:   Thu Nov 21 17:24:00 2019 -0600

>      usb: Fix a deadlock in usbmon between mmap and read

>      Signed-off-by: Pete Zaitcev <zaitcev@redhat.com>
>      Reported-by: syzbot+56f9673bb4cdcbeb0e92@syzkaller.appspotmail.com

> diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
> index ac2b4fcc265f..f48a23adbc35 100644
> --- a/drivers/usb/mon/mon_bin.c
> +++ b/drivers/usb/mon/mon_bin.c
> @@ -1039,12 +1039,18 @@ static long mon_bin_ioctl(struct file *file,  
> unsigned int cmd, unsigned long arg

>   		mutex_lock(&rp->fetch_lock);
>   		spin_lock_irqsave(&rp->b_lock, flags);
> -		mon_free_buff(rp->b_vec, rp->b_size/CHUNK_SIZE);
> -		kfree(rp->b_vec);
> -		rp->b_vec  = vec;
> -		rp->b_size = size;
> -		rp->b_read = rp->b_in = rp->b_out = rp->b_cnt = 0;
> -		rp->cnt_lost = 0;
> +		if (rp->mmap_active) {
> +			mon_free_buff(vec, size/CHUNK_SIZE);
> +			kfree(vec);
> +			ret = -EBUSY;
> +		} else {
> +			mon_free_buff(rp->b_vec, rp->b_size/CHUNK_SIZE);
> +			kfree(rp->b_vec);
> +			rp->b_vec  = vec;
> +			rp->b_size = size;
> +			rp->b_read = rp->b_in = rp->b_out = rp->b_cnt = 0;
> +			rp->cnt_lost = 0;
> +		}
>   		spin_unlock_irqrestore(&rp->b_lock, flags);
>   		mutex_unlock(&rp->fetch_lock);
>   		}
> @@ -1216,13 +1222,21 @@ mon_bin_poll(struct file *file, struct  
> poll_table_struct *wait)
>   static void mon_bin_vma_open(struct vm_area_struct *vma)
>   {
>   	struct mon_reader_bin *rp = vma->vm_private_data;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&rp->b_lock, flags);
>   	rp->mmap_active++;
> +	spin_unlock_irqrestore(&rp->b_lock, flags);
>   }

>   static void mon_bin_vma_close(struct vm_area_struct *vma)
>   {
> +	unsigned long flags;
> +
>   	struct mon_reader_bin *rp = vma->vm_private_data;
> +	spin_lock_irqsave(&rp->b_lock, flags);
>   	rp->mmap_active--;
> +	spin_unlock_irqrestore(&rp->b_lock, flags);
>   }

>   /*
> @@ -1234,16 +1248,12 @@ static vm_fault_t mon_bin_vma_fault(struct  
> vm_fault *vmf)
>   	unsigned long offset, chunk_idx;
>   	struct page *pageptr;

> -	mutex_lock(&rp->fetch_lock);
>   	offset = vmf->pgoff << PAGE_SHIFT;
> -	if (offset >= rp->b_size) {
> -		mutex_unlock(&rp->fetch_lock);
> +	if (offset >= rp->b_size)
>   		return VM_FAULT_SIGBUS;
> -	}
>   	chunk_idx = offset / CHUNK_SIZE;
>   	pageptr = rp->b_vec[chunk_idx].pg;
>   	get_page(pageptr);
> -	mutex_unlock(&rp->fetch_lock);
>   	vmf->page = pageptr;
>   	return 0;
>   }

Re: Re: possible deadlock in mon_bin_vma_fault

[Alan Stern]

On Fri, 22 Nov 2019, syzbot wrote:

> > #syz test: linux-4.19.y f6e27dbb1afa
> 
> "linux-4.19.y" does not look like a valid git repo address.

Let's try again.  The "git tree" value in the original bug report was
"upstream", so I'll use that even though it doesn't look like a valid 
git repo address either.

Alan Stern

#syz test: upstream f6e27dbb1afa

commit 5252eb4c8297fedbf1c5f1e67da44efe00e6ef6b
Author: Pete Zaitcev <zaitcev@kotori.zaitcev.us>
Date:   Thu Nov 21 17:24:00 2019 -0600

     usb: Fix a deadlock in usbmon between mmap and read

     Signed-off-by: Pete Zaitcev <zaitcev@redhat.com>
     Reported-by: syzbot+56f9673bb4cdcbeb0e92@syzkaller.appspotmail.com

diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
index ac2b4fcc265f..f48a23adbc35 100644
--- a/drivers/usb/mon/mon_bin.c
+++ b/drivers/usb/mon/mon_bin.c
@@ -1039,12 +1039,18 @@ static long mon_bin_ioctl(struct file *file, unsigned int cmd, unsigned long arg

  		mutex_lock(&rp->fetch_lock);
  		spin_lock_irqsave(&rp->b_lock, flags);
-		mon_free_buff(rp->b_vec, rp->b_size/CHUNK_SIZE);
-		kfree(rp->b_vec);
-		rp->b_vec  = vec;
-		rp->b_size = size;
-		rp->b_read = rp->b_in = rp->b_out = rp->b_cnt = 0;
-		rp->cnt_lost = 0;
+		if (rp->mmap_active) {
+			mon_free_buff(vec, size/CHUNK_SIZE);
+			kfree(vec);
+			ret = -EBUSY;
+		} else {
+			mon_free_buff(rp->b_vec, rp->b_size/CHUNK_SIZE);
+			kfree(rp->b_vec);
+			rp->b_vec  = vec;
+			rp->b_size = size;
+			rp->b_read = rp->b_in = rp->b_out = rp->b_cnt = 0;
+			rp->cnt_lost = 0;
+		}
  		spin_unlock_irqrestore(&rp->b_lock, flags);
  		mutex_unlock(&rp->fetch_lock);
  		}
@@ -1216,13 +1222,21 @@ mon_bin_poll(struct file *file, struct poll_table_struct *wait)
  static void mon_bin_vma_open(struct vm_area_struct *vma)
  {
  	struct mon_reader_bin *rp = vma->vm_private_data;
+	unsigned long flags;
+
+	spin_lock_irqsave(&rp->b_lock, flags);
  	rp->mmap_active++;
+	spin_unlock_irqrestore(&rp->b_lock, flags);
  }

  static void mon_bin_vma_close(struct vm_area_struct *vma)
  {
+	unsigned long flags;
+
  	struct mon_reader_bin *rp = vma->vm_private_data;
+	spin_lock_irqsave(&rp->b_lock, flags);
  	rp->mmap_active--;
+	spin_unlock_irqrestore(&rp->b_lock, flags);
  }

  /*
@@ -1234,16 +1248,12 @@ static vm_fault_t mon_bin_vma_fault(struct vm_fault *vmf)
  	unsigned long offset, chunk_idx;
  	struct page *pageptr;

-	mutex_lock(&rp->fetch_lock);
  	offset = vmf->pgoff << PAGE_SHIFT;
-	if (offset >= rp->b_size) {
-		mutex_unlock(&rp->fetch_lock);
+	if (offset >= rp->b_size)
  		return VM_FAULT_SIGBUS;
-	}
  	chunk_idx = offset / CHUNK_SIZE;
  	pageptr = rp->b_vec[chunk_idx].pg;
  	get_page(pageptr);
-	mutex_unlock(&rp->fetch_lock);
  	vmf->page = pageptr;
  	return 0;
  }

Re: Re: Re: possible deadlock in mon_bin_vma_fault

[syzbot]

> On Fri, 22 Nov 2019, syzbot wrote:

>> > #syz test: linux-4.19.y f6e27dbb1afa

>> "linux-4.19.y" does not look like a valid git repo address.

> Let's try again.  The "git tree" value in the original bug report was
> "upstream", so I'll use that even though it doesn't look like a valid
> git repo address either.

> Alan Stern

> #syz test: upstream f6e27dbb1afa

"upstream" does not look like a valid git repo address.


> commit 5252eb4c8297fedbf1c5f1e67da44efe00e6ef6b
> Author: Pete Zaitcev <zaitcev@kotori.zaitcev.us>
> Date:   Thu Nov 21 17:24:00 2019 -0600

>       usb: Fix a deadlock in usbmon between mmap and read

>       Signed-off-by: Pete Zaitcev <zaitcev@redhat.com>
>       Reported-by: syzbot+56f9673bb4cdcbeb0e92@syzkaller.appspotmail.com

> diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
> index ac2b4fcc265f..f48a23adbc35 100644
> --- a/drivers/usb/mon/mon_bin.c
> +++ b/drivers/usb/mon/mon_bin.c
> @@ -1039,12 +1039,18 @@ static long mon_bin_ioctl(struct file *file,  
> unsigned int cmd, unsigned long arg

>    		mutex_lock(&rp->fetch_lock);
>    		spin_lock_irqsave(&rp->b_lock, flags);
> -		mon_free_buff(rp->b_vec, rp->b_size/CHUNK_SIZE);
> -		kfree(rp->b_vec);
> -		rp->b_vec  = vec;
> -		rp->b_size = size;
> -		rp->b_read = rp->b_in = rp->b_out = rp->b_cnt = 0;
> -		rp->cnt_lost = 0;
> +		if (rp->mmap_active) {
> +			mon_free_buff(vec, size/CHUNK_SIZE);
> +			kfree(vec);
> +			ret = -EBUSY;
> +		} else {
> +			mon_free_buff(rp->b_vec, rp->b_size/CHUNK_SIZE);
> +			kfree(rp->b_vec);
> +			rp->b_vec  = vec;
> +			rp->b_size = size;
> +			rp->b_read = rp->b_in = rp->b_out = rp->b_cnt = 0;
> +			rp->cnt_lost = 0;
> +		}
>    		spin_unlock_irqrestore(&rp->b_lock, flags);
>    		mutex_unlock(&rp->fetch_lock);
>    		}
> @@ -1216,13 +1222,21 @@ mon_bin_poll(struct file *file, struct  
> poll_table_struct *wait)
>    static void mon_bin_vma_open(struct vm_area_struct *vma)
>    {
>    	struct mon_reader_bin *rp = vma->vm_private_data;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&rp->b_lock, flags);
>    	rp->mmap_active++;
> +	spin_unlock_irqrestore(&rp->b_lock, flags);
>    }

>    static void mon_bin_vma_close(struct vm_area_struct *vma)
>    {
> +	unsigned long flags;
> +
>    	struct mon_reader_bin *rp = vma->vm_private_data;
> +	spin_lock_irqsave(&rp->b_lock, flags);
>    	rp->mmap_active--;
> +	spin_unlock_irqrestore(&rp->b_lock, flags);
>    }

>    /*
> @@ -1234,16 +1248,12 @@ static vm_fault_t mon_bin_vma_fault(struct  
> vm_fault *vmf)
>    	unsigned long offset, chunk_idx;
>    	struct page *pageptr;

> -	mutex_lock(&rp->fetch_lock);
>    	offset = vmf->pgoff << PAGE_SHIFT;
> -	if (offset >= rp->b_size) {
> -		mutex_unlock(&rp->fetch_lock);
> +	if (offset >= rp->b_size)
>    		return VM_FAULT_SIGBUS;
> -	}
>    	chunk_idx = offset / CHUNK_SIZE;
>    	pageptr = rp->b_vec[chunk_idx].pg;
>    	get_page(pageptr);
> -	mutex_unlock(&rp->fetch_lock);
>    	vmf->page = pageptr;
>    	return 0;
>    }

Re: Re: Re: possible deadlock in mon_bin_vma_fault

[syzbot]

> On Fri, 22 Nov 2019, syzbot wrote:

>> > #syz test: linux-4.19.y f6e27dbb1afa

>> "linux-4.19.y" does not look like a valid git repo address.

> Let's try again.  The "git tree" value in the original bug report was
> "upstream", so I'll use that even though it doesn't look like a valid
> git repo address either.

> Alan Stern

> #syz test: upstream f6e27dbb1afa

"upstream" does not look like a valid git repo address.


> commit 5252eb4c8297fedbf1c5f1e67da44efe00e6ef6b
> Author: Pete Zaitcev <zaitcev@kotori.zaitcev.us>
> Date:   Thu Nov 21 17:24:00 2019 -0600

>       usb: Fix a deadlock in usbmon between mmap and read

>       Signed-off-by: Pete Zaitcev <zaitcev@redhat.com>
>       Reported-by: syzbot+56f9673bb4cdcbeb0e92@syzkaller.appspotmail.com

> diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
> index ac2b4fcc265f..f48a23adbc35 100644
> --- a/drivers/usb/mon/mon_bin.c
> +++ b/drivers/usb/mon/mon_bin.c
> @@ -1039,12 +1039,18 @@ static long mon_bin_ioctl(struct file *file,  
> unsigned int cmd, unsigned long arg

>    		mutex_lock(&rp->fetch_lock);
>    		spin_lock_irqsave(&rp->b_lock, flags);
> -		mon_free_buff(rp->b_vec, rp->b_size/CHUNK_SIZE);
> -		kfree(rp->b_vec);
> -		rp->b_vec  = vec;
> -		rp->b_size = size;
> -		rp->b_read = rp->b_in = rp->b_out = rp->b_cnt = 0;
> -		rp->cnt_lost = 0;
> +		if (rp->mmap_active) {
> +			mon_free_buff(vec, size/CHUNK_SIZE);
> +			kfree(vec);
> +			ret = -EBUSY;
> +		} else {
> +			mon_free_buff(rp->b_vec, rp->b_size/CHUNK_SIZE);
> +			kfree(rp->b_vec);
> +			rp->b_vec  = vec;
> +			rp->b_size = size;
> +			rp->b_read = rp->b_in = rp->b_out = rp->b_cnt = 0;
> +			rp->cnt_lost = 0;
> +		}
>    		spin_unlock_irqrestore(&rp->b_lock, flags);
>    		mutex_unlock(&rp->fetch_lock);
>    		}
> @@ -1216,13 +1222,21 @@ mon_bin_poll(struct file *file, struct  
> poll_table_struct *wait)
>    static void mon_bin_vma_open(struct vm_area_struct *vma)
>    {
>    	struct mon_reader_bin *rp = vma->vm_private_data;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&rp->b_lock, flags);
>    	rp->mmap_active++;
> +	spin_unlock_irqrestore(&rp->b_lock, flags);
>    }

>    static void mon_bin_vma_close(struct vm_area_struct *vma)
>    {
> +	unsigned long flags;
> +
>    	struct mon_reader_bin *rp = vma->vm_private_data;
> +	spin_lock_irqsave(&rp->b_lock, flags);
>    	rp->mmap_active--;
> +	spin_unlock_irqrestore(&rp->b_lock, flags);
>    }

>    /*
> @@ -1234,16 +1248,12 @@ static vm_fault_t mon_bin_vma_fault(struct  
> vm_fault *vmf)
>    	unsigned long offset, chunk_idx;
>    	struct page *pageptr;

> -	mutex_lock(&rp->fetch_lock);
>    	offset = vmf->pgoff << PAGE_SHIFT;
> -	if (offset >= rp->b_size) {
> -		mutex_unlock(&rp->fetch_lock);
> +	if (offset >= rp->b_size)
>    		return VM_FAULT_SIGBUS;
> -	}
>    	chunk_idx = offset / CHUNK_SIZE;
>    	pageptr = rp->b_vec[chunk_idx].pg;
>    	get_page(pageptr);
> -	mutex_unlock(&rp->fetch_lock);
>    	vmf->page = pageptr;
>    	return 0;
>    }

Re: Re: Re: possible deadlock in mon_bin_vma_fault

[Alan Stern]

On Sat, 23 Nov 2019, syzbot wrote:

> > On Fri, 22 Nov 2019, syzbot wrote:
> 
> >> > #syz test: linux-4.19.y f6e27dbb1afa
> 
> >> "linux-4.19.y" does not look like a valid git repo address.
> 
> > Let's try again.  The "git tree" value in the original bug report was
> > "upstream", so I'll use that even though it doesn't look like a valid
> > git repo address either.
> 
> > Alan Stern
> 
> > #syz test: upstream f6e27dbb1afa
> 
> "upstream" does not look like a valid git repo address.

Andrey, can you do something about that?  It would be a lot nicer if 
_all_ the syzbot output and records included an actual git repo address 
in the appropriate places.

Alan Stern

#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git v5.3

commit 5252eb4c8297fedbf1c5f1e67da44efe00e6ef6b
Author: Pete Zaitcev <zaitcev@kotori.zaitcev.us>
Date:   Thu Nov 21 17:24:00 2019 -0600

     usb: Fix a deadlock in usbmon between mmap and read

     Signed-off-by: Pete Zaitcev <zaitcev@redhat.com>
     Reported-by: syzbot+56f9673bb4cdcbeb0e92@syzkaller.appspotmail.com

diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
index ac2b4fcc265f..f48a23adbc35 100644
--- a/drivers/usb/mon/mon_bin.c
+++ b/drivers/usb/mon/mon_bin.c
@@ -1039,12 +1039,18 @@ static long mon_bin_ioctl(struct file *file, unsigned int cmd, unsigned long arg

  		mutex_lock(&rp->fetch_lock);
  		spin_lock_irqsave(&rp->b_lock, flags);
-		mon_free_buff(rp->b_vec, rp->b_size/CHUNK_SIZE);
-		kfree(rp->b_vec);
-		rp->b_vec  = vec;
-		rp->b_size = size;
-		rp->b_read = rp->b_in = rp->b_out = rp->b_cnt = 0;
-		rp->cnt_lost = 0;
+		if (rp->mmap_active) {
+			mon_free_buff(vec, size/CHUNK_SIZE);
+			kfree(vec);
+			ret = -EBUSY;
+		} else {
+			mon_free_buff(rp->b_vec, rp->b_size/CHUNK_SIZE);
+			kfree(rp->b_vec);
+			rp->b_vec  = vec;
+			rp->b_size = size;
+			rp->b_read = rp->b_in = rp->b_out = rp->b_cnt = 0;
+			rp->cnt_lost = 0;
+		}
  		spin_unlock_irqrestore(&rp->b_lock, flags);
  		mutex_unlock(&rp->fetch_lock);
  		}
@@ -1216,13 +1222,21 @@ mon_bin_poll(struct file *file, struct poll_table_struct *wait)
  static void mon_bin_vma_open(struct vm_area_struct *vma)
  {
  	struct mon_reader_bin *rp = vma->vm_private_data;
+	unsigned long flags;
+
+	spin_lock_irqsave(&rp->b_lock, flags);
  	rp->mmap_active++;
+	spin_unlock_irqrestore(&rp->b_lock, flags);
  }

  static void mon_bin_vma_close(struct vm_area_struct *vma)
  {
+	unsigned long flags;
+
  	struct mon_reader_bin *rp = vma->vm_private_data;
+	spin_lock_irqsave(&rp->b_lock, flags);
  	rp->mmap_active--;
+	spin_unlock_irqrestore(&rp->b_lock, flags);
  }

  /*
@@ -1234,16 +1248,12 @@ static vm_fault_t mon_bin_vma_fault(struct vm_fault *vmf)
  	unsigned long offset, chunk_idx;
  	struct page *pageptr;

-	mutex_lock(&rp->fetch_lock);
  	offset = vmf->pgoff << PAGE_SHIFT;
-	if (offset >= rp->b_size) {
-		mutex_unlock(&rp->fetch_lock);
+	if (offset >= rp->b_size)
  		return VM_FAULT_SIGBUS;
-	}
  	chunk_idx = offset / CHUNK_SIZE;
  	pageptr = rp->b_vec[chunk_idx].pg;
  	get_page(pageptr);
-	mutex_unlock(&rp->fetch_lock);
  	vmf->page = pageptr;
  	return 0;
  }

Re: possible deadlock in mon_bin_vma_fault

[syzbot]

Hello,

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

failed to apply patch:
checking file drivers/usb/mon/mon_bin.c
patch: **** unexpected end of file in patch



Tested on:

commit:         4d856f72 Linux 5.3
git tree:        
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git v5.3
dashboard link: https://syzkaller.appspot.com/bug?extid=56f9673bb4cdcbeb0e92
compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
patch:          https://syzkaller.appspot.com/x/patch.diff?x=17a22f16e00000

Re: possible deadlock in mon_bin_vma_fault

[Alan Stern]

On Sun, 24 Nov 2019, syzbot wrote:

> Hello,
> 
> syzbot tried to test the proposed patch but build/boot failed:
> 
> failed to apply patch:
> checking file drivers/usb/mon/mon_bin.c
> patch: **** unexpected end of file in patch

One more try...

Alan Stern

#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git v5.3

commit 5252eb4c8297fedbf1c5f1e67da44efe00e6ef6b
Author: Pete Zaitcev <zaitcev@kotori.zaitcev.us>
Date:   Thu Nov 21 17:24:00 2019 -0600

     usb: Fix a deadlock in usbmon between mmap and read

     Signed-off-by: Pete Zaitcev <zaitcev@redhat.com>
     Reported-by: syzbot+56f9673bb4cdcbeb0e92@syzkaller.appspotmail.com

diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
index ac2b4fcc265f..f48a23adbc35 100644
--- a/drivers/usb/mon/mon_bin.c
+++ b/drivers/usb/mon/mon_bin.c
@@ -1039,12 +1039,18 @@ static long mon_bin_ioctl(struct file *file, unsigned int cmd, unsigned long arg
 
 		mutex_lock(&rp->fetch_lock);
 		spin_lock_irqsave(&rp->b_lock, flags);
-		mon_free_buff(rp->b_vec, rp->b_size/CHUNK_SIZE);
-		kfree(rp->b_vec);
-		rp->b_vec  = vec;
-		rp->b_size = size;
-		rp->b_read = rp->b_in = rp->b_out = rp->b_cnt = 0;
-		rp->cnt_lost = 0;
+		if (rp->mmap_active) {
+			mon_free_buff(vec, size/CHUNK_SIZE);
+			kfree(vec);
+			ret = -EBUSY;
+		} else {
+			mon_free_buff(rp->b_vec, rp->b_size/CHUNK_SIZE);
+			kfree(rp->b_vec);
+			rp->b_vec  = vec;
+			rp->b_size = size;
+			rp->b_read = rp->b_in = rp->b_out = rp->b_cnt = 0;
+			rp->cnt_lost = 0;
+		}
 		spin_unlock_irqrestore(&rp->b_lock, flags);
 		mutex_unlock(&rp->fetch_lock);
 		}
@@ -1216,13 +1222,21 @@ mon_bin_poll(struct file *file, struct poll_table_struct *wait)
 static void mon_bin_vma_open(struct vm_area_struct *vma)
 {
 	struct mon_reader_bin *rp = vma->vm_private_data;
+	unsigned long flags;
+
+	spin_lock_irqsave(&rp->b_lock, flags);
 	rp->mmap_active++;
+	spin_unlock_irqrestore(&rp->b_lock, flags);
 }
 
 static void mon_bin_vma_close(struct vm_area_struct *vma)
 {
+	unsigned long flags;
+
 	struct mon_reader_bin *rp = vma->vm_private_data;
+	spin_lock_irqsave(&rp->b_lock, flags);
 	rp->mmap_active--;
+	spin_unlock_irqrestore(&rp->b_lock, flags);
 }
 
 /*
@@ -1234,16 +1248,12 @@ static vm_fault_t mon_bin_vma_fault(struct vm_fault *vmf)
 	unsigned long offset, chunk_idx;
 	struct page *pageptr;
 
-	mutex_lock(&rp->fetch_lock);
 	offset = vmf->pgoff << PAGE_SHIFT;
-	if (offset >= rp->b_size) {
-		mutex_unlock(&rp->fetch_lock);
+	if (offset >= rp->b_size)
 		return VM_FAULT_SIGBUS;
-	}
 	chunk_idx = offset / CHUNK_SIZE;
 	pageptr = rp->b_vec[chunk_idx].pg;
 	get_page(pageptr);
-	mutex_unlock(&rp->fetch_lock);
 	vmf->page = pageptr;
 	return 0;
 }

Re: possible deadlock in mon_bin_vma_fault

[syzbot]

Hello,

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

Reported-and-tested-by:  
syzbot+56f9673bb4cdcbeb0e92@syzkaller.appspotmail.com

Tested on:

commit:         4d856f72 Linux 5.3
git tree:        
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git v5.3
kernel config:  https://syzkaller.appspot.com/x/.config?x=86071634b2594991
dashboard link: https://syzkaller.appspot.com/bug?extid=56f9673bb4cdcbeb0e92
compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
patch:          https://syzkaller.appspot.com/x/patch.diff?x=11ff3eeee00000

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

Re: possible deadlock in mon_bin_vma_fault

[Pete Zaitcev]

On Sun, 24 Nov 2019 15:24:00 -0800
syzbot <syzbot+56f9673bb4cdcbeb0e92@syzkaller.appspotmail.com> wrote:

> syzbot has tested the proposed patch and the reproducer did not trigger  
> crash:

Okay. Alan, what is the most appropriate tree for me to submit now?
Does Greg have one?

Do you want Reviewed-by or something?

-- Pete

Re: possible deadlock in mon_bin_vma_fault

[Alan Stern]

On Sun, 24 Nov 2019, Pete Zaitcev wrote:

> On Sun, 24 Nov 2019 15:24:00 -0800
> syzbot <syzbot+56f9673bb4cdcbeb0e92@syzkaller.appspotmail.com> wrote:
> 
> > syzbot has tested the proposed patch and the reproducer did not trigger  
> > crash:
> 
> Okay. Alan, what is the most appropriate tree for me to submit now?
> Does Greg have one?
> 
> Do you want Reviewed-by or something?

Send it to Greg.  Be sure to add a Fixes: line referencing the commit 
that syzbot found, and add a CC: <stable@vger.kernel.org> line.  You 
can also add:

Reviewed-by: Alan Stern <stern@rowland.harvard.edu>

Alan Stern