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

[Pete Zaitcev <zaitcev@redhat.com>]

On Fri, 29 Dec 2017 16:24:20 +0300
"Kirill A. Shutemov" <kirill@shutemov.name> wrote:

> Looks like MON_IOCT_RING_SIZE reallocates ring buffer without any
> serialization wrt mon_bin_vma_fault(). By the time of get_page() the page
> may be freed.

Okay. Who knew that you could fork while holding an open descriptor. :-)

> The patch below seems help the crash to go away, but I *think* more work
> is required. For instance, after ring buffer reallocation the old pages
> will stay mapped. Nothing pulls them.

You know, this bothered me all these years too, but I was assured
back in the day (as much as I can remember), that doing get_page()
in the .fault() is just the right thing. In my defense, you can
see other drivers doing it, such as:

./drivers/char/agp/alpha-agp.c
./drivers/hsi/clients/cmt_speech.c

I'd appreciate insight from someone who knows how VM subsystem works.

Now, about the code:

> diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
> index f6ae753ab99b..ac168fecf04f 100644
> --- a/drivers/usb/mon/mon_bin.c
> +++ 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. The ->b_lock is designed for things that are accessed
from interrupts that Host Controller Driver serves -- mostly
various pointers. By defintion it's not covering things that
are related to re-allocation. Now, the re-allocation itself
grabs it, because it resets indexes into the new buffer, but
does not appear to apply here, does it now?

-- 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>