linux-kernel - Re: [PATCH v4 1/5] swiotlb: Fix double-allocation of slots due to broken alignment handling

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]

Message-ID: <Zd4CANmJdW_t69S2@infradead.org>
Date: Tue, 27 Feb 2024 07:38:40 -0800
From: Christoph Hellwig <hch@...radead.org>
To: Will Deacon <will@...nel.org>
Cc: Michael Kelley <mhklinux@...look.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"kernel-team@...roid.com" <kernel-team@...roid.com>,
	"iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
	Christoph Hellwig <hch@....de>,
	Marek Szyprowski <m.szyprowski@...sung.com>,
	Robin Murphy <robin.murphy@....com>,
	Petr Tesarik <petr.tesarik1@...wei-partners.com>,
	Dexuan Cui <decui@...rosoft.com>,
	Nicolin Chen <nicolinc@...dia.com>
Subject: Re: [PATCH v4 1/5] swiotlb: Fix double-allocation of slots due to
 broken alignment handling

On Fri, Feb 23, 2024 at 12:47:43PM +0000, Will Deacon wrote:
> > >  	/*
> > >  	 * For allocations of PAGE_SIZE or larger only look for page aligned
> > >  	 * allocations.
> > >  	 */
> > >  	if (alloc_size >= PAGE_SIZE)
> > > -		iotlb_align_mask |= ~PAGE_MASK;
> > > -	iotlb_align_mask &= ~(IO_TLB_SIZE - 1);
> > > -
> > > -	/*
> > > -	 * For mappings with an alignment requirement don't bother looping to
> > > -	 * unaligned slots once we found an aligned one.
> > > -	 */
> > > -	stride = (iotlb_align_mask >> IO_TLB_SHIFT) + 1;
> > > +		stride = umax(stride, PAGE_SHIFT - IO_TLB_SHIFT + 1);
> > 
> > Is this special handling of alloc_size >= PAGE_SIZE really needed?
> 
> I've been wondering that as well, but please note that this code (and the
> comment) are in the upstream code, so I was erring in favour of keeping
> that while fixing the bugs. We could have an extra patch dropping it if
> we can convince ourselves that it's not adding anything, though.

This special casing goes back to before git history.  It obviously is not
needed, but it might have made sense back then.  If people come up with
a good argument I'm totally fine with removing it, but I also think we
need to get the fixes here in ASAP, so things that are just cleanups
probably aren't priority right now.

> > While the iommu_dma_map_page() case can already fail due to
> > "too large" requests because of not setting a max mapping size,
> > this patch can cause smaller requests to fail as well until Patch 4
> > gets applied.  That might be problem to avoid, perhaps by
> > merging the Patch 4 changes into this patch.
> 
> I'll leave this up to Christoph. Personally, I'm keen to avoid having
> a giant patch trying to fix all the SWIOTLB allocation issues in one go,
> as it will inevitably get reverted due to a corner case that we weren't
> able to test properly, breaking the common cases at the same time.

Let's keep it split.