linux-kernel - RE: [PATCH v5 6/6] swiotlb: Remove pointless stride adjustment for allocations >= PAGE

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:
 <SN6PR02MB41577D09E97B1D9645369D58D45F2@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Thu, 29 Feb 2024 07:36:05 +0000
From: Michael Kelley <mhklinux@...look.com>
To: Will Deacon <will@...nel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, Petr Tesarik
	<petr.tesarik1@...wei-partners.com>
CC: "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 v5 6/6] swiotlb: Remove pointless stride adjustment for
 allocations >= PAGE_SIZE

> From: Will Deacon <will@...nel.org> Sent: Wednesday, February 28, 2024 5:40 AM
> >
> > For swiotlb allocations >= PAGE_SIZE, the slab search historically
> > adjusted the stride to avoid checking unaligned slots. However, this is
> > no longer needed now that the code around it has evolved and the
> > stride is calculated from the required alignment.
> >
> > Either 'alloc_align_mask' is used to specify the allocation alignment or
> > the DMA 'min_align_mask' is used to align the allocation with 'orig_addr'.
> > At least one of these masks is always non-zero.
> 
> I think the patch is correct, but this justification is not.  alloc_align_mask
> and the DMA min_align_mask are often both zero.  While the NVMe
> PCI driver sets min_align_mask, SCSI disk drivers do not (except for the
> Hyper-V synthetic SCSI driver).   When both are zero, presumably
> there are no alignment requirements, so a stride of 1 is appropriate.
> 

I did additional checking into the history of page alignment for alloc
sizes of a page or greater. The swiotlb code probably made sense
prior to commit 0eee5ae10256. This commit has this change:

        slot_base = area_index * mem->area_nslabs;
-       index = wrap_area_index(mem, ALIGN(area->index, stride));
+       index = area->index;

        for (slots_checked = 0; slots_checked < mem->area_nslabs; ) {
                slot_index = slot_base + index;

In the old code, once the PAGE_SIZE was factored into the stride, the
stride was used to align the starting index, so that the first slot checked
would be page aligned. But commit 0eee5ae10256 drops that use
of the stride and puts the page alignment in iotlb_align_mask, for
reasons explained in the commit message. But in Will's Patch 1
when the page alignment is no longer incorporated into
iotlb_align_mask (for good reason), page aligning the stride then
doesn't do anything useful, resulting in this patch.

If there *is* a requirement for page alignment of page-size-or-greater
requests, even when alloc_align_mask and min_align_mask are zero,
we need to think about how to do that correctly, as that requirement
is no longer met after Patch 1 of this series.

Michael