On Mon, Jun 15, 2026 at 03:27:01PM +0100, Matt Evans wrote:
Hi Matt,
>
> On 11/06/2026 21:30, Pranjal Shrivastava wrote:
> > On Wed, Jun 10, 2026 at 04:43:16PM +0100, Matt Evans wrote:
> >> Add vfio_pci_dma_buf_find_pfn(), which a VMA fault handler can use to
> >> find a PFN.
> >>
> >> This supports multi-range DMABUFs, which typically would be used to
> >> represent scattered spans but might even represent overlapping or
> >> aliasing spans of PFNs.
> >>
> >> Because this is intended to be used in vfio_pci_core.c, we also need
> >> to expose the struct vfio_pci_dma_buf in the vfio_pci_priv.h header.
> >>
> >> Signed-off-by: Matt Evans <[email protected]>
[...]
> >> +
> >> + const unsigned long pagesize = PAGE_SIZE << order;
> >> + unsigned long vma_off = ((vma->vm_pgoff - priv->vma_pgoff_adjust) <<
> >> + PAGE_SHIFT) & VFIO_PCI_OFFSET_MASK;
> >> + unsigned long rounded_page_addr = ALIGN_DOWN(address, pagesize);
> >> + unsigned long rounded_page_end = rounded_page_addr + pagesize;
> >> + unsigned long page_buf_offset;
> >> + unsigned long page_buf_offset_end;
> >> + unsigned long range_buf_offset = 0;
> >> + unsigned int i;
> >> +
> >> + if (rounded_page_addr < vma->vm_start || rounded_page_end >
> >> vma->vm_end) {
> >> + if (order > 0)
> >> + return -EAGAIN;
> >> +
> >> + /* A fault address outside of the VMA is absurd. */
> >> + WARN(1, "Fault addr 0x%lx outside VMA 0x%lx-0x%lx\n",
> >> + address, vma->vm_start, vma->vm_end);
> >
> > This could flood dmesg if triggered repeatedly by userspace :(
> > Since a fault outside the VMA is an invalid access that already results
> > in a SIGBUS, we could probably avoid the WARN here?
> > Perhaps pr_warn_ratelimited() should suffice?
>
> I'm OK moving to a pr_warn_ratelimited(). Note though that this case is
> "genuinely impossible" currently and the check exists in case something
> changes elsewhere. (Re your flood comment, am I missing a way for
> userspace to trigger this? The scenario is a faulthandler for a VMA
> getting a VA outside the bounds of that VMA; such a fault address
> wouldn't match that VMA.)
I should've been explicit, I guess I worded it wrong, my bad.
I didn't mean that a user-space could hit this on it's own. However,
I meant to say if there's a bug in core mm during some future work,
dmesg being flooded by stackdumps gets messy (especially during dev) as
the underlying reason might get missed in the flood. Hence, I prefer
moving to pr_warn_ratelimited.
>
> >> + return -EFAULT;
> >> + }
> >> +
> >> + /*
> >> + * page_buff_offset[_end] is the span of DMABUF offsets
> >> + * corresponding to the faulting page:
> >> + */
> >> + if (unlikely(check_add_overflow(rounded_page_addr - vma->vm_start,
> >> + vma_off, &page_buf_offset) ||
> >> + check_add_overflow(page_buf_offset, pagesize,
> >> + &page_buf_offset_end)))
> >> + return -EFAULT;
> >> +
> >> + for (i = 0; i < priv->nr_ranges; i++) {
> >> + size_t range_len = priv->phys_vec[i].len;
> >> + phys_addr_t range_start = priv->phys_vec[i].paddr;
> >> +
> >> + /*
> >> + * If the current range starts after the page's span,
> >> + * this and any future range won't match. Bail early.
> >> + */
> >> + if (page_buf_offset_end <= range_buf_offset)
> >> + break;
> >> +
> >> + if (page_buf_offset >= range_buf_offset &&
> >> + page_buf_offset_end <= range_buf_offset + range_len) {
> >> + /*
> >> + * The faulting page is wholly contained
> >> + * within the span represented by the range.
> >> + * Validate PFN alignment for the order:
> >> + */
> >> + unsigned long pfn = (range_start + page_buf_offset -
> >> + range_buf_offset) / PAGE_SIZE;
> >
> > Minor nit: I'm aware that decent compilers convert pow(2) divides to >>
> > However, we seem to be using `>> PAGE_SHIFT` across vfio-pci. E.g.:
> >
> > return (pci_resource_start(vdev->pdev, index) >> PAGE_SHIFT) + pgoff;
> > unsigned long pgoff = (addr - vma->vm_start) >> PAGE_SHIFT;
> >
> > Let's consider using the same pattern?
>
> (Do you know of a compiler that both builds the kernel and does NOT
> perform this transformation? I am confident that resulting object code
> will be OK here.)
>
I guess most of the modern compiler do, I was just referring to the
style across the file. I don't have any strong opinion.
> In an earlier revision I was using shifts but they were fairly messy
> compared to this expression, which arises from a request by Jason.
>
Yea, looking back at Jason's comment [1], I think he was mainly pointing out
that the common factor (PAGE_SIZE) could be grouped together. But again,
no strong feeling about this, just picked up a pattern across the file.
If it breaks on some compiler we can fix it later..
> >> +
> >> + if (IS_ALIGNED(pfn, 1 << order)) {
> >> + *out_pfn = pfn;
> >> + return 0;
> >> + }
> >> + /* Retry with smaller order */
> >> + return -EAGAIN;
> >> + }
> >> + range_buf_offset += range_len;
> >> + }
> >> +
> >> + /*
> >> + * A hugepage straddling a range boundary will fail to match a
> >> + * range, but the address will (eventually) match when retried
> >> + * with a smaller page.
> >> + */
> >> + if (order > 0)
> >> + return -EAGAIN;
> >> +
> >> + /*
> >> + * If we get here, the address fell outside of the span
> >> + * represented by the (concatenated) ranges. Setup of a
> >
> > Nit: double space before "Setup" and "But" below.
>
> I liked Alex's response :-) This is common practice for monospaced text
> since increasing inter-sentence spacing helps readability in paragraph
> blocks (see Documentation/ for many examples ...).
>
Ack. :)
> >> + * mapping must ensure that the VMA is <= the total size of
> >> + * the ranges, so this should never happen. But, if it does,
> >> + * force SIGBUS for the access and warn.
> >> + */
> >> + WARN_ONCE(1, "No range for addr 0x%lx, order %d: VMA 0x%lx-0x%lx pgoff
> >> 0x%lx, %u ranges, size 0x%zx\n",
> >> + address, order, vma->vm_start, vma->vm_end, vma->vm_pgoff,
> >> + priv->nr_ranges, priv->size);
> >> +
> >> + return -EFAULT;
> >
> > The fall-through logic at the end feels a bit redundant.
> >
> > If we've exhausted the phys_vec list without finding a match, returning
> > -EAGAIN for order > 0 seems like the correct fallback behavior.
>
> This path can happen (for order > 0) e.g. mis-alignment of VA versus the
> PFN, i.e. is likely...
>
> > However, the subsequent WARN_ONCE for the order == 0 seems unnecessary?
> > An out-of-bounds access is an error that should simply return -EFAULT
> > (converting to SIGBUS) without polluting the kernel log with stackdumps?
>
> ...but the only way this can happen, for order == 0, is if the VMA
> extends beyond the underlying resource. For example, if the VMA is
> larger than the DMABUF size (the total length of phys ranges set up
> inside the DMABUF). Both VFIO BAR mmap() and a DMABUF mmap() disallow
> mapping off the end of the underlying resource. That is, this also
> "cannot happen" but if logic changes elsewhere then we will really want
> to know about hitting this case -- the check is not redundant.
I didn't mean to imply that the path itself is impossible or won't
happen.. I just meant that the logic / structure felt a bit redundant at
the end of the function..
Instead of having the separate `if (order > 0)` block falling through to
the base case, I suggest it could be cleaner as:
ret = order ? -EAGAIN : -EFAULT;
if (ret == -EFAULT)
pr_warn_ratelimited(...);
return ret;
But again, that's a preference. I'd leave that to your judgement.
>
> Still, it doesn't need a regdump/backtrace (at least while this is only
> called from one spot), so a pr_warn_* is better.
>
Ack.
Thanks,
Praan
