linux-kernel - Re: [PATCH v2 1/2] xen/swiotlb: add alignment check for dma buffers

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: <2d4fd45a-2461-441e-a116-3b6cff18ee9e@suse.com>
Date: Mon, 2 Dec 2024 09:27:50 +0100
From: Jürgen Groß <jgross@...e.com>
To: Thierry Escande <thierry.escande@...es.tech>,
 linux-kernel@...r.kernel.org, iommu@...ts.linux.dev
Cc: Stefano Stabellini <sstabellini@...nel.org>,
 Oleksandr Tyshchenko <oleksandr_tyshchenko@...m.com>,
 xen-devel@...ts.xenproject.org, jbeulich@...e.com
Subject: Re: [PATCH v2 1/2] xen/swiotlb: add alignment check for dma buffers

On 29.11.24 18:36, Thierry Escande wrote:
> Hi,
> 
> On 16/09/2024 08:47, Juergen Gross wrote:
>> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
>> index 35155258a7e2..ddf5b1df632e 100644
>> --- a/drivers/xen/swiotlb-xen.c
>> +++ b/drivers/xen/swiotlb-xen.c
>> @@ -78,9 +78,15 @@ static inline int range_straddles_page_boundary(phys_addr_t p, size_t size)
>>   {
>>   	unsigned long next_bfn, xen_pfn = XEN_PFN_DOWN(p);
>>   	unsigned int i, nr_pages = XEN_PFN_UP(xen_offset_in_page(p) + size);
>> +	phys_addr_t algn = 1ULL << (get_order(size) + PAGE_SHIFT);
>>   
>>   	next_bfn = pfn_to_bfn(xen_pfn);
>>   
>> +	/* If buffer is physically aligned, ensure DMA alignment. */
>> +	if (IS_ALIGNED(p, algn) &&
>> +	    !IS_ALIGNED(next_bfn << XEN_PAGE_SHIFT, algn))
>> +		return 1;
> 
> There is a regression in the mpt3sas driver because of this change.
> When, in a dom0, this driver creates its DMA pool at probe time and
> calls dma_pool_zalloc() (see [1]), the call to
> range_straddles_page_boundary() (from xen_swiotlb_alloc_coherent())
> returns 1 because of the alignment checks added by this patch. Then the
> call to xen_create_contiguous_region() in xen_swiotlb_alloc_coherent()
> fails because the passed size order is too big (> MAX_CONTIG_ORDER).
> This driver sets the pool allocation block size to 2.3+ MBytes.
> 
>  From previous discussions on the v1 patch, these checks are not
> necessary from xen_swiotlb_alloc_coherent() that already ensures
> alignment, right?

It ensures alignment regarding guest physical memory, but it doesn't do
so for machine memory.

For DMA machine memory proper alignment might be needed, too, so the
check is required. As an example, some crypto drivers seem to rely on
proper machine memory alignment, which was the reason for those checks
to be added.

Possible solutions include:

- rising the MAX_CONTIG_ORDER limit (to which value?)
- adding a way to allocate large DMA buffers with relaxed alignment
   requirements (this will impact the whole DMA infrastructure plus
   drivers like mp3sas which would need to use the new interface)
- modify the mpt3sas driver to stay within the current limits
- only guarantee proper machine memory alignment up to MAX_CONTIG_ORDER
   (risking to introduce hard to diagnose bugs for the rare use cases of
   such large buffers)


Juergen

Download attachment "OpenPGP_0xB0DE9DD628BF132F.asc" of type "application/pgp-keys" (3684 bytes)

Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (496 bytes)