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: <d7bd7dce-ead2-4c9b-bb47-d4029c3ce6f7@arm.com>
Date: Mon, 4 Mar 2024 17:11:23 +0000
From: Robin Murphy <robin.murphy@....com>
To: Michael Kelley <mhklinux@...look.com>, Petr Tesařík
 <petr@...arici.cz>
Cc: Christoph Hellwig <hch@....de>, Will Deacon <will@...nel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 Petr Tesarik <petr.tesarik1@...wei-partners.com>,
 "kernel-team@...roid.com" <kernel-team@...roid.com>,
 "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
 Marek Szyprowski <m.szyprowski@...sung.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

On 04/03/2024 4:04 pm, Michael Kelley wrote:
> From: Petr Tesařík <petr@...arici.cz> Sent: Monday, March 4, 2024 7:55 AM
>>
>> On Mon, 4 Mar 2024 13:37:56 +0000
>> Robin Murphy <robin.murphy@....com> wrote:
>>
>>> On 04/03/2024 11:00 am, Petr Tesařík wrote:
>>> [...]
>>>>> Here's my take on tying all the threads together. There are
>>>>> four alignment combinations:
>>>>>
>>>>> 1. alloc_align_mask: zero; min_align_mask: zero
>>>>> 2. alloc_align_mask: zero; min_align_mask: non-zero
>>>>> 3. alloc_align_mask: non-zero; min_align_mask: zero/ignored
>>>>> 4. alloc_align_mask: non-zero; min_align_mask: non-zero
>>>>
>>>> What does "min_align_mask: zero/ignored" mean? Under which
>>>> circumstances should be a non-zero min_align_mask ignored?
> 
> "Ignored" was my short-hand for the swiotlb_alloc() case where
> orig_addr is zero.  Even if min_align_mask is set for the device, it
> doesn't have any effect when orig_addr is zero.
> 
>>>>
>>>>> xen_swiotlb_map_page() and dma_direct_map_page() are #1 or #2
>>>>> via swiotlb_map() and swiotlb_tbl_map_single()
>>>>>
>>>>> iommu_dma_map_page() is #3 and #4 via swiotlb_tbl_map_single()
>>>>>
>>>>> swiotlb_alloc() is #3, directly to swiotlb_find_slots()
>>>>>
>>>>> For #1, the returned physical address has no constraints if
>>>>> the requested size is less than a page. For page size or
>>>>> greater, the discussed historical requirement for page
>>>>> alignment applies.
>>>>>
>>>>> For #2, min_align_mask governs the bits of the returned
>>>>> physical address that must match the original address. When
>>>>> needed, swiotlb must also allocate pre-padding aligned to
>>>>> IO_TLB_SIZE that precedes the returned physical address.  A
>>>>> request size <= swiotlb_max_mapping_size() will not exceed
>>>>> IO_TLB_SEGSIZE even with the padding. The historical
>>>>> requirement for page alignment does not apply because the
>>>>> driver has explicitly used the newer min_align_mask feature.
>>>>
>>>> What is the idea here? Is it the assumption that only old drivers rely
>>>> on page alignment, so if they use min_align_mask, it proves that they
>>>> are new and must not rely on page alignment?
>>>
>>> Yes, if a driver goes out of its way to set a min_align_mask which is
>>> smaller than its actual alignment constraint, that is clearly the
>>> driver's own bug. Strictly we only need to be sympathetic to drivers
>>> which predate min_align_mask, when implicitly relying on page alignment
>>> was all they had.
>>>
>>>>> For #3, alloc_align_mask specifies the required alignment. No
>>>>> pre-padding is needed. Per earlier comments from Robin[1],
>>>>> it's reasonable to assume alloc_align_mask (i.e., the granule)
>>>>> is >= IO_TLB_SIZE. The original address is not relevant in
>>>>> determining the alignment, and the historical page alignment
>>>>> requirement does not apply since alloc_align_mask explicitly
>>>>> states the alignment.
>>>
>>> FWIW I'm also starting to wonder about getting rid of the alloc_size
>>> argument and just have SWIOTLB round the end address up to
>>> alloc_align_mask itself as part of all these calculations. Seems like it
>>> could potentially end up a little simpler, maybe?
> 
> Yes, I was thinking exactly this.  But my reasoning was to solve the
> bug in #4 that I previously pointed out.  If iommu_dma_map_page()
> does *not* do
> 
> 	aligned_size = iova_align(iovad, size);
> 
> but swiotlb_tbl_map_single() rounds up the size based on
> alloc_align_mask *after* adding the offset modulo
> min_align_mask, then the rounded-up size won't exceed IO_TLB_SIZE,
> regardless of which bits are set in orig_addr.

Ah, neat, I had a gut feeling that something like that might also fall 
out, I just didn't feel like working through the details to see if 
"simpler" could lead to "objectively better" :)

I guess at worst we might also need to pass an alloc_align_mask to 
swiotlb_max_mapping_size() as well, but even that's not necessarily a 
bad thing if it keeps the equivalent calculations close together within 
SWIOTLB and makes things more robust overall.

Cheers,
Robin.