On Fri, 29 Dec 2017 16:24:20 +0300
"Kirill A. Shutemov" <kirill@xxxxxxxxxxxxx> 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 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
