linux-kernel - Re: [PATCH v2 0/4] Fix restricted DMA vs swiotlb

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]
Date:   Fri, 23 Jul 2021 10:50:57 +0200
From:   Christian Borntraeger <borntraeger@...ibm.com>
To:     Halil Pasic <pasic@...ux.ibm.com>
Cc:     Will Deacon <will@...nel.org>, iommu@...ts.linux-foundation.org,
        linux-kernel@...r.kernel.org, Guenter Roeck <linux@...ck-us.net>,
        Claire Chang <tientzu@...omium.org>,
        Christoph Hellwig <hch@....de>,
        Robin Murphy <robin.murphy@....com>,
        Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
        Nathan Chancellor <nathan@...nel.org>,
        linux-s390 <linux-s390@...r.kernel.org>,
        Vasily Gorbik <gor@...ux.ibm.com>,
        Heiko Carstens <hca@...ux.ibm.com>
Subject: Re: [PATCH v2 0/4] Fix restricted DMA vs swiotlb_exit()



On 23.07.21 10:47, Halil Pasic wrote:
> On Fri, 23 Jul 2021 08:14:19 +0200
> Christian Borntraeger <borntraeger@...ibm.com> wrote:
> 
>> Resending with the correct email of Heiko....
>>
>> On 23.07.21 03:12, Halil Pasic wrote:
>>> On Thu, 22 Jul 2021 21:22:58 +0200
>>> Christian Borntraeger <borntraeger@...ibm.com> wrote:
>>>    
>>>> On 20.07.21 15:38, Will Deacon wrote:
>>>>> Hi again, folks,
>>>>>
>>>>> This is version two of the patch series I posted yesterday:
>>>>>
>>>>>      https://lore.kernel.org/r/20210719123054.6844-1-will@kernel.org
>>>>>
>>>>> The only changes since v1 are:
>>>>>
>>>>>      * Squash patches 2 and 3, amending the commit message accordingly
>>>>>      * Add Reviewed-by and Tested-by tags from Christoph and Claire (thanks!)
>>>>>
>>>>> I'd usually leave it a bit longer between postings, but since this fixes
>>>>> issues with patches in -next I thought I'd spin a new version immediately.
>>>>>
>>>>> Cheers,
>>>>
>>>> FWIW, I just bisected virtio-errors with secure execution mode
>>>> qemu-system-s390x: virtio-serial-bus: Unexpected port id 4205794771 for device virtio-serial0.0
>>>>
>>>> to
>>>> commit 903cd0f315fe426c6a64c54ed389de0becb663dc
>>>> Author: Claire Chang <tientzu@...omium.org>
>>>> Date:   Thu Jun 24 23:55:20 2021 +0800
>>>>
>>>>         swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing
>>>>
>>>> Unfortunately this patch series does NOT fix this issue, so it seems that even more
>>>> things are broken.
>>>>
>>>> Any idea what else might be broken?
>>>
>>> I've done some debugging, and I think I know what is going on. Since
>>> that commit we need to set force_swiotlb before the swiotlb itself is
>>> initialized. So the patch below should fix the problem.
>>>
>>> --------------------8<-------------------------------------
>>>
>>> From: Halil Pasic <pasic@...ux.ibm.com>
>>> Date: Fri, 23 Jul 2021 02:57:06 +0200
>>> Subject: [PATCH 1/1] s390/pv: fix the forcing of the swiotlb
>>>
>>> Since commit 903cd0f315fe ("swiotlb: Use is_swiotlb_force_bounce for
>>> swiotlb data bouncing") if code sets swiotlb_force it needs to do so
>>> before the swiotlb is initialised. Otherwise
>>> io_tlb_default_mem->force_bounce will not get set to true, and devices
>>> that use (the default) swiotlb will not bounce  despite switolb_force
>>> having the value of SWIOTLB_FORCE.
>>>
>>> Let us restore swiotlb functionality for PV by fulfilling this new
>>> requirement.
>>>    
>> I would add:
>> Fixes: 903cd0f315fe ("swiotlb: Use is_swiotlb_force_bounce for swiotlb data bouncing")
>> as this patch breaks things
>> and
>> Fixes: 64e1f0c531d1 ("s390/mm: force swiotlb for protected virtualization")
>>
>> to make the s390 init code more robust in case people start backporting things.
> 
> I agree. Do we want this backported to the stable releases that have
> 64e1f0c531d1  (i.e. do we need a cc stable) or should the fixes tag just
> serve as metadata? My guess is, it's the former. In that sense should I
> add the tags along with an explanation for the second fixes respin with
> cc stable?
> 
> (BTW I don't think this formally qualifies for the stable backports, but
> I hope we can make an exception...)

I think it makes sense for stable as it is cleaner to set the flags before
calling the init function. cc stable would be better and the right way
according to process, but the Fixes tag is mostly enough.

> 
>>
>>> Signed-off-by: Halil Pasic <pasic@...ux.ibm.com>
>>
>> I can confirm that this fixes the problem. This also makes sense codewise.
>>
>> Tested-by: Christian Borntraeger <borntraeger@...ibm.com>
>> Reviewed-by: Christian Borntraeger <borntraeger@...ibm.com>
> 
> Thanks!
> 
> Regards,
> Halil
>>
>> Konrad, Heiko, Vasily, any preference which tree this goes? I think s390
>> would be easiest, but that requires that the patches in the swiotlb tree have
>> fixed commit IDs.
>>
>>> ---
>>>    arch/s390/mm/init.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
>>> index 8ac710de1ab1..07bbee9b7320 100644
>>> --- a/arch/s390/mm/init.c
>>> +++ b/arch/s390/mm/init.c
>>> @@ -186,9 +186,9 @@ static void pv_init(void)
>>>    		return;
>>>    
>>>    	/* make sure bounce buffers are shared */
>>> +	swiotlb_force = SWIOTLB_FORCE;
>>>    	swiotlb_init(1);
>>>    	swiotlb_update_mem_attributes();
>>> -	swiotlb_force = SWIOTLB_FORCE;
>>>    }
>>>    
>>>    void __init mem_init(void)
>>>    
>