| 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
| ||
|
Message-ID: <alpine.DEB.2.22.394.2502121310320.619090@ubuntu-linux-20-04-desktop>
Date: Wed, 12 Feb 2025 13:14:16 -0800 (PST)
From: Stefano Stabellini <sstabellini@...nel.org>
To: Jürgen Groß <jgross@...e.com>
cc: Stefano Stabellini <sstabellini@...nel.org>, linux-kernel@...r.kernel.org,
x86@...nel.org, iommu@...ts.linux.dev,
Boris Ostrovsky <boris.ostrovsky@...cle.com>,
Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
Borislav Petkov <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>,
"H. Peter Anvin" <hpa@...or.com>,
Oleksandr Tyshchenko <oleksandr_tyshchenko@...m.com>,
xen-devel@...ts.xenproject.org
Subject: Re: [PATCH 2/2] xen/swiotlb: don't destroy contiguous region in all
cases
On Wed, 12 Feb 2025, Jürgen Groß wrote:
> On 12.02.25 02:30, Stefano Stabellini wrote:
> > On Tue, 11 Feb 2025, Juergen Gross wrote:
> > > In case xen_swiotlb_alloc_coherent() needed to create a contiguous
> > > region only for other reason than the memory not being compliant with
> > > the device's DMA mask, there is no reason why this contiguous region
> > > should be destroyed by xen_swiotlb_free_coherent() later. Destroying
> > > this region should be done only, if the memory of the region was
> > > allocated with more stringent placement requirements than the memory
> > > it did replace.
> > >
> > > Signed-off-by: Juergen Gross <jgross@...e.com>
> > > ---
> > > arch/x86/include/asm/xen/swiotlb-xen.h | 5 +++--
> > > arch/x86/xen/mmu_pv.c | 18 ++++++++++++------
> > > drivers/xen/swiotlb-xen.c | 11 +++++++----
> > > 3 files changed, 22 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/arch/x86/include/asm/xen/swiotlb-xen.h
> > > b/arch/x86/include/asm/xen/swiotlb-xen.h
> > > index abde0f44df57..a353f20c7e79 100644
> > > --- a/arch/x86/include/asm/xen/swiotlb-xen.h
> > > +++ b/arch/x86/include/asm/xen/swiotlb-xen.h
> > > @@ -4,8 +4,9 @@
> > > int xen_swiotlb_fixup(void *buf, unsigned long nslabs);
> > > int xen_create_contiguous_region(phys_addr_t pstart, unsigned int order,
> > > - unsigned int address_bits,
> > > - dma_addr_t *dma_handle);
> > > + unsigned int address_bits,
> > > + dma_addr_t *dma_handle,
> > > + unsigned int *address_bits_in);
> > > void xen_destroy_contiguous_region(phys_addr_t pstart, unsigned int
> > > order);
> > > #endif /* _ASM_X86_SWIOTLB_XEN_H */
> > > diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
> > > index 2c70cd35e72c..fb586238f7c4 100644
> > > --- a/arch/x86/xen/mmu_pv.c
> > > +++ b/arch/x86/xen/mmu_pv.c
> > > @@ -2208,19 +2208,22 @@ void __init xen_init_mmu_ops(void)
> > > static unsigned long discontig_frames[1<<MAX_CONTIG_ORDER];
> > > #define VOID_PTE (mfn_pte(0, __pgprot(0)))
> > > -static void xen_zap_pfn_range(unsigned long vaddr, unsigned int order,
> > > - unsigned long *in_frames,
> > > - unsigned long *out_frames)
> > > +static int xen_zap_pfn_range(unsigned long vaddr, unsigned int order,
> > > + unsigned long *in_frames,
> > > + unsigned long *out_frames)
> > > {
> > > int i;
> > > + u64 address_bits = 0;
> > > struct multicall_space mcs;
> > > xen_mc_batch();
> > > for (i = 0; i < (1UL<<order); i++, vaddr += PAGE_SIZE) {
> > > mcs = __xen_mc_entry(0);
> > > - if (in_frames)
> > > + if (in_frames) {
> > > in_frames[i] = virt_to_mfn((void *)vaddr);
> > > + address_bits |= in_frames[i] << PAGE_SHIFT;
> > > + }
> > > MULTI_update_va_mapping(mcs.mc, vaddr, VOID_PTE, 0);
> > > __set_phys_to_machine(virt_to_pfn((void *)vaddr),
> > > INVALID_P2M_ENTRY);
> > > @@ -2229,6 +2232,8 @@ static void xen_zap_pfn_range(unsigned long vaddr,
> > > unsigned int order,
> > > out_frames[i] = virt_to_pfn((void *)vaddr);
> > > }
> > > xen_mc_issue(0);
> > > +
> > > + return fls64(address_bits);
> > > }
> > > /*
> > > @@ -2321,7 +2326,8 @@ static int xen_exchange_memory(unsigned long
> > > extents_in, unsigned int order_in,
> > > int xen_create_contiguous_region(phys_addr_t pstart, unsigned int
> > > order,
> > > unsigned int address_bits,
> > > - dma_addr_t *dma_handle)
> > > + dma_addr_t *dma_handle,
> > > + unsigned int *address_bits_in)
> > > {
> > > unsigned long *in_frames = discontig_frames, out_frame;
> > > unsigned long flags;
> > > @@ -2336,7 +2342,7 @@ int xen_create_contiguous_region(phys_addr_t pstart,
> > > unsigned int order,
> > > spin_lock_irqsave(&xen_reservation_lock, flags);
> > > /* 1. Zap current PTEs, remembering MFNs. */
> > > - xen_zap_pfn_range(vstart, order, in_frames, NULL);
> > > + *address_bits_in = xen_zap_pfn_range(vstart, order, in_frames, NULL);
> > > /* 2. Get a new contiguous memory extent. */
> > > out_frame = virt_to_pfn((void *)vstart);
> > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> > > index 26c62e0d34e9..3f3724f53914 100644
> > > --- a/drivers/xen/swiotlb-xen.c
> > > +++ b/drivers/xen/swiotlb-xen.c
> > > @@ -118,6 +118,7 @@ int xen_swiotlb_fixup(void *buf, unsigned long nslabs)
> > > int rc;
> > > unsigned int order = get_order(IO_TLB_SEGSIZE << IO_TLB_SHIFT);
> > > unsigned int i, dma_bits = order + PAGE_SHIFT;
> > > + unsigned int dummy;
> > > dma_addr_t dma_handle;
> > > phys_addr_t p = virt_to_phys(buf);
> > > @@ -129,7 +130,7 @@ int xen_swiotlb_fixup(void *buf, unsigned long
> > > nslabs)
> > > do {
> > > rc = xen_create_contiguous_region(
> > > p + (i << IO_TLB_SHIFT), order,
> > > - dma_bits, &dma_handle);
> > > + dma_bits, &dma_handle, &dummy);
> > > } while (rc && dma_bits++ < MAX_DMA_BITS);
> > > if (rc)
> > > return rc;
> > > @@ -144,6 +145,7 @@ xen_swiotlb_alloc_coherent(struct device *dev, size_t
> > > size,
> > > dma_addr_t *dma_handle, gfp_t flags, unsigned long attrs)
> > > {
> > > u64 dma_mask = dev->coherent_dma_mask;
> > > + unsigned int address_bits = fls64(dma_mask), address_bits_in;
> > > int order = get_order(size);
> > > phys_addr_t phys;
> > > void *ret;
> > > @@ -160,10 +162,11 @@ xen_swiotlb_alloc_coherent(struct device *dev,
> > > size_t size,
> > > if (*dma_handle + size - 1 > dma_mask ||
> > > range_straddles_page_boundary(phys, size) ||
> > > range_requires_alignment(phys, size)) {
> > > - if (xen_create_contiguous_region(phys, order, fls64(dma_mask),
> > > - dma_handle) != 0)
> > > + if (xen_create_contiguous_region(phys, order, address_bits,
> > > + dma_handle,
> > > &address_bits_in))
> > > goto out_free_pages;
> > > - SetPageXenRemapped(virt_to_page(ret));
> > > + if (address_bits_in > address_bits)
> > > + SetPageXenRemapped(virt_to_page(ret));
> >
> > This has the unfortunate side effect of making "PageXenRemapped"
> > unreliable as an indicator of whether a page has been remapped. A page
> > could still be remapped without the "PageXenRemapped" bit being set.
> >
> > I recommend adding an in-code comment to clarify this behavior.
>
> The PageXenRemapped bit is used only for determining whether
> xen_destroy_contiguous_region() should be called. And by not setting the bit
> I'm avoiding to call xen_destroy_contiguous_region() later. So I don't see any
> unfortunate side effect.
While the purpose of PageXenRemapped is to determine whether
xen_destroy_contiguous_region() should be called for the region, the
name "PageXenRemapped" suggests more generically that the region is
remapped.
Without this patch, all the regions that are remapped have
PageXenRemapped set. The name matches its meaning. (Also,
xen_destroy_contiguous_region() is called an them.)
With this patch, not all the regions that are remapped have
PageXenRemapped set. The name does not match its meaning.
Powered by blists - more mailing lists