Re: [PATCH 09/19] mm: New follow_pfnmap API

Jason Gunthorpe
2024-08-15T17:24:51Z

On Thu, Aug 15, 2024 at 01:21:01PM -0400, Peter Xu wrote:
> > Why? Either the function only returns PFN map no-struct page things or
> > it returns struct page stuff too, in which case why bother to check
> > the VMA flags if the caller already has to be correct for struct page
> > backed results?
> >
> > This function is only safe to use under the proper locking, and under
> > those rules it doesn't matter at all what the result is..
>
> Do you mean we should drop the PFNMAP|IO check?

Yeah

> I didn't see all the
> callers to say that they won't rely on proper failing of !PFNMAP&&!IO vmas
> to work alright. So I assume we should definitely keep them around.

But as before, if we care about this we should be using vm_normal_page
as that is sort of abusing the PFNMAP flags.

> > Any physical address obtained through this API is only valid while
> > the @follow_pfnmap_args. Continuing to use the address after end(),
> > without some other means to synchronize with page table updates
> > will create a security bug.
>
> Some misuse on wordings here (e.g. we don't return PA but PFN), and some
> sentence doesn't seem to be complete.. but I think I get the "scary" part
> of it. How about this, appending the scary part to the end?
>
> * During the start() and end() calls, the results in @args will be valid
> * as proper locks will be held. After the end() is called, all the fields
> * in @follow_pfnmap_args will be invalid to be further accessed. Further
> * use of such information after end() may require proper synchronizations
> * by the caller with page table updates, otherwise it can create a
> * security bug.

I would specifically emphasis that the pfn may not be used after
end. That is the primary mistake people have made.

They think it is a PFN so it is safe.

> It sounds like we need some mmu notifiers when mapping the IOMMU pgtables,
> as long as there's MMIO-region / P2P involved. It'll make sure when
> tearing down the BAR mappings, the devices will at least see the same view
> as the processors.

I think the mmu notifiers can trigger too often for this to be
practical for DMA :(

Jason