linux-kernel - Re: [PATCH 4.19 17/32] iommu/vt-d: Dont queue

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: <dc639510-5d88-5b05-a973-5f4b7c720f76@arista.com>
Date:   Tue, 6 Aug 2019 23:47:44 +0100
From:   Dmitry Safonov <dima@...sta.com>
To:     Pavel Machek <pavel@....cz>
Cc:     linux-kernel@...r.kernel.org,
        David Woodhouse <dwmw2@...radead.org>,
        Joerg Roedel <joro@...tes.org>,
        Lu Baolu <baolu.lu@...ux.intel.com>,
        iommu@...ts.linux-foundation.org, Joerg Roedel <jroedel@...e.de>
Subject: Re: [PATCH 4.19 17/32] iommu/vt-d: Dont queue_iova() if there is no
 flush queue

Hi Pavel,

On 8/3/19 10:34 PM, Pavel Machek wrote:
> Hi!
> 
>> --- a/drivers/iommu/intel-iommu.c
>> +++ b/drivers/iommu/intel-iommu.c
>> @@ -3721,7 +3721,7 @@ static void intel_unmap(struct device *d
>>  
>>  	freelist = domain_unmap(domain, start_pfn, last_pfn);
>>  
>> -	if (intel_iommu_strict) {
>> +	if (intel_iommu_strict || !has_iova_flush_queue(&domain->iovad)) {
>>  		iommu_flush_iotlb_psi(iommu, domain, start_pfn,
>>  				      nrpages, !freelist, 0);
>>  		/* free iova */
>> --- a/drivers/iommu/iova.c
>> +++ b/drivers/iommu/iova.c
>> @@ -65,9 +65,14 @@ init_iova_domain(struct iova_domain *iov
>>  }
>>  EXPORT_SYMBOL_GPL(init_iova_domain);
>>  
>> +bool has_iova_flush_queue(struct iova_domain *iovad)
>> +{
>> +	return !!iovad->fq;
> 
> Should this be READ_ONCE()?

Why? Compiler can't anyhow assume that it's always true/false and there
is a clear data dependency between this and:
:	queue_iova(&domain->iovad, iova_pfn, nrpages,
:			   (unsigned long)freelist);

> 
>> @@ -100,13 +106,17 @@ int init_iova_flush_queue(struct iova_do
>>  	for_each_possible_cpu(cpu) {
>>  		struct iova_fq *fq;
>>  
>> -		fq = per_cpu_ptr(iovad->fq, cpu);
>> +		fq = per_cpu_ptr(queue, cpu);
>>  		fq->head = 0;
>>  		fq->tail = 0;
>>  
>>  		spin_lock_init(&fq->lock);
>>  	}
>>  
>> +	smp_wmb();
>> +
>> +	iovad->fq = queue;
>> +
> 
> Could we have a comment why the barrier is needed,

I'm up for the comment if you feel like it - in my POV it's quite
obvious that we want finish initializing the queue's internals before
assigning the queue. I didn't put the comment exactly because I felt
like it would state something evident [in my POV].

> and perhaps there
> should be oposing smp_rmb() somewhere? Does this need to be
> WRITE_ONCE() as it is racing against reader?

I feel confused. I might have forgotten everything about barriers, but
again if I'm not mistaken, one doesn't need a barrier in:
: if (A->a != NULL)
:     use(A); /* dereferences A->a */
: else
:     /* don't use `a' */

Thanks,
          Dmitry