linux-kernel - Re: [PATCH 5/5] iommufd: Flush CPU caches on DMA pages in non-coherent domains

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 20 May 2024 10:45:56 +0800
From: Yan Zhao <yan.y.zhao@...el.com>
To: Jason Gunthorpe <jgg@...dia.com>
CC: <kvm@...r.kernel.org>, <linux-kernel@...r.kernel.org>, <x86@...nel.org>,
	<alex.williamson@...hat.com>, <kevin.tian@...el.com>,
	<iommu@...ts.linux.dev>, <pbonzini@...hat.com>, <seanjc@...gle.com>,
	<dave.hansen@...ux.intel.com>, <luto@...nel.org>, <peterz@...radead.org>,
	<tglx@...utronix.de>, <mingo@...hat.com>, <bp@...en8.de>, <hpa@...or.com>,
	<corbet@....net>, <joro@...tes.org>, <will@...nel.org>,
	<robin.murphy@....com>, <baolu.lu@...ux.intel.com>, <yi.l.liu@...el.com>
Subject: Re: [PATCH 5/5] iommufd: Flush CPU caches on DMA pages in
 non-coherent domains

On Fri, May 17, 2024 at 02:04:18PM -0300, Jason Gunthorpe wrote:
> On Thu, May 16, 2024 at 10:32:43AM +0800, Yan Zhao wrote:
> > On Wed, May 15, 2024 at 05:43:04PM -0300, Jason Gunthorpe wrote:
> > > On Wed, May 15, 2024 at 03:06:36PM +0800, Yan Zhao wrote:
> > > 
> > > > > So it has to be calculated on closer to a page by page basis (really a
> > > > > span by span basis) if flushing of that span is needed based on where
> > > > > the pages came from. Only pages that came from a hwpt that is
> > > > > non-coherent can skip the flushing.
> > > > Is area by area basis also good?
> > > > Isn't an area either not mapped to any domain or mapped into all domains?
> > > 
> > > Yes, this is what the span iterator turns into in the background, it
> > > goes area by area to cover things.
> > > 
> > > > But, yes, considering the limited number of non-coherent domains, it appears
> > > > more robust and clean to always flush for non-coherent domain in
> > > > iopt_area_fill_domain().
> > > > It eliminates the need to decide whether to retain the area flag during a split.
> > > 
> > > And flush for pin user pages, so you basically always flush because
> > > you can't tell where the pages came from.
> > As a summary, do you think it's good to flush in below way?
> > 
> > 1. in iopt_area_fill_domains(), flush before mapping a page into domains when
> >    iopt->noncoherent_domain_cnt > 0, no matter where the page is from.
> >    Record cache_flush_required in pages for unpin.
> > 2. in iopt_area_fill_domain(), pass in hwpt to check domain non-coherency.
> >    flush before mapping a page into a non-coherent domain, no matter where the
> >    page is from.
> >    Record cache_flush_required in pages for unpin.
> > 3. in batch_unpin(), flush if pages->cache_flush_required before
> >    unpin_user_pages.
> 
> It does not quite sound right, there should be no tracking in the
> pages of this stuff.
What's the downside of having tracking in the pages?

Lazily flush pages right before unpin pages is not only to save flush count
for performance, but also for some real problem we encountered. see below.

> 
> If pfn_reader_fill_span() does batch_from_domain() and
> the source domain's storage_domain is non-coherent then you can skip
> the flush. This is not pedantically perfect in skipping all flushes, but
> in practice it is probably good enough.
We don't know whether the source storage_domain is non-coherent since
area->storage_domain is of "struct iommu_domain".

Do you want to add a flag in "area", e.g. area->storage_domain_is_noncoherent,
and set this flag along side setting storage_domain?
(But looks this is not easy in iopt_area_fill_domains() as we don't have hwpt
there.)

> __iopt_area_unfill_domain() (and children) must flush after
> iopt_area_unmap_domain_range() if the area's domain is
> non-coherent. This is also not perfect, but probably good enough.
Do you mean flush after each iopt_area_unmap_domain_range() if the domain is
non-coherent?
The problem is that iopt_area_unmap_domain_range() knows only IOVA, the
IOVA->PFN relationship is not available without iommu_iova_to_phys() and
iommu_domain contains no coherency info.
Besides, when the non-coherent domain is a storage domain, we still need to do
the flush in batch_unpin(), right?
Then, with a more complex case, if the non-coherent domain is a storage domain,
and if some pages are still held in pages->access_itree when unfilling the
domain, should we get PFNs from pages->pinned_pfns and do the flush in
__iopt_area_unfill_domain()?
> 
> Doing better in both cases would require inspecting the areas under
> the used span to see what is there. This is not so easy.
My feeling is that checking non-coherency of target domain and save non-coherency
in pages might be the easiest way with least code change.