On Wed, 3 Jan 2018 12:26:04 +0300
"Kirill A. Shutemov" <kirill@xxxxxxxxxxxxx> wrote:
> > > +++ b/drivers/usb/mon/mon_bin.c
> > > @@ -1228,15 +1228,24 @@ static void mon_bin_vma_close(struct vm_area_struct *vma)
> > > static int mon_bin_vma_fault(struct vm_fault *vmf)
> > > {
> > > struct mon_reader_bin *rp = vmf->vma->vm_private_data;
> > > - unsigned long offset, chunk_idx;
> > > + unsigned long offset, chunk_idx, flags;
> > > struct page *pageptr;
> > >
> > > + mutex_lock(&rp->fetch_lock);
> > > + spin_lock_irqsave(&rp->b_lock, flags);
> > > offset = vmf->pgoff << PAGE_SHIFT;
> > > - if (offset >= rp->b_size)
> > > + if (offset >= rp->b_size) {
> > > + spin_unlock_irqrestore(&rp->b_lock, flags);
> > > + mutex_unlock(&rp->fetch_lock);
> > > return VM_FAULT_SIGBUS;
> > > + }
> > > chunk_idx = offset / CHUNK_SIZE;
> > > +
> > > pageptr = rp->b_vec[chunk_idx].pg;
> > > get_page(pageptr);
> > > + spin_unlock_irqrestore(&rp->b_lock, flags);
> > > + mutex_unlock(&rp->fetch_lock);
> > > +
> > > vmf->page = pageptr;
> > > return 0;
> > > }
> >
> > I think that grabbing the spinlock is not really necessary in
> > this case. [...]
>
> Please, double check everything. I remember that the mutex wasn't enough
> to stop bug from triggering. But I didn't spend much time understanding
> the code.
I just don't understand why. The only two fields that are used
in the fault routine are rp->b_vec and rp->b_size. They are
protected by the mutex rp->fetch_lock. I don't see anything else
can spill into these fields by dirtying adjacent words in memory,
either.... except this:
case MON_IOCQ_RING_SIZE:
ret = rp->b_size;
break;
In the old days, this was safe, but who knows what CPUs do today.
It needs the same mutex taken around the read-only reference too.
How about this:
diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
index f6ae753ab99b..cb3612f28804 100644
--- a/drivers/usb/mon/mon_bin.c
+++ b/drivers/usb/mon/mon_bin.c
@@ -1004,7 +1004,9 @@ static long mon_bin_ioctl(struct file *file, unsigned int cmd, unsigned long arg
break;
case MON_IOCQ_RING_SIZE:
+ mutex_lock(&rp->fetch_lock);
ret = rp->b_size;
+ mutex_unlock(&rp->fetch_lock);
break;
case MON_IOCT_RING_SIZE:
@@ -1231,12 +1233,15 @@ static int 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);
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;
}
-- Pete
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
