Re: possible deadlock in mon_bin_vma_fault

From: Pete Zaitcev <zaitcev@redhat.com>
To: Alan Stern <stern@rowland.harvard.edu>
Cc: syzbot <syzbot+56f9673bb4cdcbeb0e92@syzkaller.appspotmail.com>,
	arnd@arndb.de, <gregkh@linuxfoundation.org>,
	<jrdr.linux@gmail.com>, <keescook@chromium.org>,
	<kstewart@linuxfoundation.org>,
	Kernel development list <linux-kernel@vger.kernel.org>,
	USB list <linux-usb@vger.kernel.org>,
	<syzkaller-bugs@googlegroups.com>, <tglx@linutronix.de>,
	<viro@zeniv.linux.org.uk>,
	zaitcev@redhat.com
Subject: Re: possible deadlock in mon_bin_vma_fault
Date: Wed, 20 Nov 2019 11:12:35 -0600	[thread overview]
Message-ID: <20191120111235.7d306f23@suzdal.zaitcev.lan> (raw)
In-Reply-To: <Pine.LNX.4.44L0.1911201109500.1498-100000@iolanthe.rowland.org>

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;
 

