Re: kernel BUG at ./include/linux/mm.h:LINE! (3)

[Pete Zaitcev <zaitcev@redhat.com>]

On Wed, 3 Jan 2018 12:26:04 +0300
"Kirill A. Shutemov" <kirill@shutemov.name> 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, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>