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

[Pranjal Shrivastava]

On Wed, Feb 25, 2026 at 09:50:00AM +0200, Leon Romanovsky wrote:
> On Tue, Feb 24, 2026 at 08:57:56PM +0000, Pranjal Shrivastava wrote:
> > 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.
> > > 
> > 
> > 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.
> 
> Yes, i came to the same conclusion, just explained why it worked before.
> 

Ack.

> > 
> > 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)) &&
> 
> pfn_valid() is a relatively expensive function [1] to invoke in the data path,
> and is_pci_p2pdma_page() ends up being called in these execution flows.
> 

Right, that makes sense. Ideally, it shouldn't be there at either of the
places (iommu_dma_map_sg or is_pci_p2pdma_page()).

> [1] https://elixir.bootlin.com/linux/v6.19.3/source/include/linux/mmzone.h#L2167
> 
> >                 folio_is_pci_p2pdma(page_folio(page));
> >  }
> > 
> > 
> > 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?
> 
> +1
> 
> > Could we instead add a helper to verify if the sg_page() return value
> > is actually backed by a struct page?
> 
> According to the SG design, callers should store only struct page pointers.
> There is one known user that violates this requirement: dmabuf, which is
> gradually being migrated away from this behavior [2].
> 
> [2] https://lore.kernel.org/all/0-v1-b5cab63049c0+191af-dmabuf_map_type_jgg@xxxxxxxxxx/
> 
> > 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?
> 
> Right. dma_map_sg() is indeed the wrong API to use for memory that is not
> backed by struct page pointers.
> 
> Thanks
> 

[--->8---]

Thanks,
Praan