On Wed, 20 Nov 2019, Pete Zaitcev wrote: > On Wed, 20 Nov 2019 11:14:05 -0500 (EST) > Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> 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
