Re: [PATCH RFC] iommu/dma: Validate page before accessing P2PDMA state

[Pranjal Shrivastava <praan@google.com>]

On Fri, Feb 27, 2026 at 11:16:02AM +0530, Ashish Mhetre wrote:
> 
> 
> On 2/26/2026 1:28 PM, Leon Romanovsky wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Wed, Feb 25, 2026 at 08:11:29PM +0000, Pranjal Shrivastava wrote:
> > > On Wed, Feb 25, 2026 at 09:56:09AM +0200, Leon Romanovsky wrote:
> > > > On Wed, Feb 25, 2026 at 10:19:41AM +0530, Ashish Mhetre wrote:
> > > > > 
> > > > > On 2/25/2026 2:27 AM, Pranjal Shrivastava wrote:
> > > > > > External email: Use caution opening links or attachments
> > > > > > 
> > > > > > 
> > > > > > On Tue, Feb 24, 2026 at 02:32:21PM +0200, Leon Romanovsky wrote:
> > > > > > > On Tue, Feb 24, 2026 at 10:42:57AM +0000, Ashish Mhetre wrote:
> > > > > > > > When mapping scatter-gather entries that reference reserved
> > > > > > > > memory regions without struct page backing (e.g., bootloader created
> > > > > > > > carveouts), is_pci_p2pdma_page() dereferences the page pointer
> > > > > > > > returned by sg_page() without first verifying its validity.
> > > > > > > I believe this behavior started after commit 88df6ab2f34b
> > > > > > > ("mm: add folio_is_pci_p2pdma()"). Prior to that change, the
> > > > > > > is_zone_device_page(page) check would return false when given a
> > > > > > > non‑existent page pointer.
> > > > > > > 
> > > > > Thanks Leon for the review. This crash started after commit 30280eee2db1
> > > > > ("iommu/dma: support PCI P2PDMA pages in dma-iommu map_sg").
> > > > > 
> > > > > > Doesn't folio_is_pci_p2pdma() also check for zone device?
> > > > > > I see[1] that it does:
> > > > > > 
> > > > > > static inline bool folio_is_pci_p2pdma(const struct folio *folio)
> > > > > > {
> > > > > >           return IS_ENABLED(CONFIG_PCI_P2PDMA) &&
> > > > > >                   folio_is_zone_device(folio) &&
> > > > > >                   folio->pgmap->type == MEMORY_DEVICE_PCI_P2PDMA;
> > > > > > }
> > > > > > 
> > > > > > I believe the problem arises due to the page_folio() call in
> > > > > > folio_is_pci_p2pdma(page_folio(page)); within is_pci_p2pdma_page().
> > > > > > page_folio() assumes it has a valid struct page to work with. For these
> > > > > > carveouts, that isn't true.
> > > > > > 
> > > > > > Potentially something like the following would stop the crash:
> > > > > > 
> > > > > > diff --git a/include/linux/memremap.h b/include/linux/memremap.h
> > > > > > index e3c2ccf872a8..e47876021afa 100644
> > > > > > --- a/include/linux/memremap.h
> > > > > > +++ b/include/linux/memremap.h
> > > > > > @@ -197,7 +197,8 @@ static inline void folio_set_zone_device_data(struct folio *folio, void *data)
> > > > > > 
> > > > > >    static inline bool is_pci_p2pdma_page(const struct page *page)
> > > > > >    {
> > > > > > -       return IS_ENABLED(CONFIG_PCI_P2PDMA) &&
> > > > > > +       return IS_ENABLED(CONFIG_PCI_P2PDMA) && page &&
> > > > > > +               pfn_valid(page_to_pfn(page)) &&
> > > > > >                   folio_is_pci_p2pdma(page_folio(page));
> > > > > >    }
> > > > > > 
> > > > > Yes, this will also fix the crash.
> > > > > 
> > > > > > But my broader question is: why are we calling a page-based API like
> > > > > > is_pci_p2pdma_page() on non-struct-page memory in the first place?
> > > > > > Could we instead add a helper to verify if the sg_page() return value
> > > > > > is actually backed by a struct page? If it isn't, we should arguably
> > > > > > skip the P2PDMA logic entirely and fall back to a dma_map_phys style
> > > > > > path. Isn't handling these "pageless" physical ranges the primary reason
> > > > > > dma_map_phys exists?
> > > > > Thanks for the feedback, Pranjal.
> > > > > 
> > > > > To clarify: are you suggesting we handle non-page-backed mappings inside
> > > > > iommu_dma_map_sg (within dma-iommu), or that callers should detect
> > > > > non-page-backed memory and use dma_map_phys instead of dma_map_sg?
> > > > The latter one.
> > > > 
> > > Yup, I meant the latter.
> > > 
> > > > > Former approach sounds better so that existing iommu_dma_map_sg callers
> > > > > don't need changes, but I'd like to confirm your preference.
> > > > The bug is in callers which used wrong API, they need to be adapted.
> > > Yes, the thing is, if the caller already knows that the region to be
> > > mapped is NOT struct page-backed, then why does it use dma_map_sg
> > > variants?
> > Before dma_map_phys() was added, there was no reliable way to DMA‑map
> > such memory, and using dma_map_sg() was a workaround that happened to

Ack.

> > work. I'm not sure whether it worked by design or by accident, but the
> > correct approach now is to use dma_map_phys().
> 
> Thanks Leon and Pranjal for the detailed feedback. I'll update our callers
> to use
> dma_map_phys() for non-page-backed buffers.
> 
> One question: would it make sense to add a check in iommu_dma_map_sg to
> fail gracefully when non-page-backed buffers are passed, instead of crashing
> the kernel?

In my opinion, the answer is no, since this is almost like the "should
the kernel protect developers from themselves" debate.. we should be a
little dramatic to make sure the developer doesn't call the wrong API.
Sure, we could return a DMA_MAPPING_ERROR or something but a silent 
DMA_MAPPING_ERROR can be ignored by a lazy driver resulting in a much
harder-to-debug scenario than a straight-forward crash.

The question is, are we sure to use scatterlists to represent non-paged
memory? 

If no, then why are we even calling the dma_map_sg* API?
struct scatterlist has a field "page_link" [1] which is literally the
struct page with a few bits representing something else.

If yes, then we could maybe encode some information (similar to
SG_CHAIN) representing if the sg is backed by a struct page. And then in
the *sg_map APIs, we could fallback to the dma_phys API if it isn't
struct paged-backed. (This would be quite some re-work and not limited
to the DMA API alone).

But as Leon pointed out that the use of sg for non-paged memory started
as a "work-around" since there was no equivalent API to dma_map_phys
earlier. Since that's the status quo, I'm leaning towards no.

But I think this gives us a nice opportunity to discuss if we really
*need* to have scatterlists to represent non-paged memory. I remember
some similar discussion happened during tcp_devmem reviews [2].

Adding Jason for his thoughts as well..

Thanks,
Praan

[1] https://elixir.bootlin.com/linux/v6.19.3/source/include/linux/scatterlist.h#L12
[2] https://lore.kernel.org/netdev/20241115015912.GA559636@ziepe.ca/