linux-kernel - Re: [PATCH] iommu/iova: Separate out rcache init

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:   Wed, 26 Jan 2022 17:58:04 +0000
From:   John Garry <john.garry@...wei.com>
To:     Robin Murphy <robin.murphy@....com>,
        "joro@...tes.org" <joro@...tes.org>,
        "will@...nel.org" <will@...nel.org>,
        "mst@...hat.com" <mst@...hat.com>,
        "jasowang@...hat.com" <jasowang@...hat.com>
CC:     "xieyongji@...edance.com" <xieyongji@...edance.com>,
        "iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "virtualization@...ts.linux-foundation.org" 
        <virtualization@...ts.linux-foundation.org>,
        Linuxarm <linuxarm@...wei.com>
Subject: Re: [PATCH] iommu/iova: Separate out rcache init

Hi Robin,

>>
>> Signed-off-by: John Garry<john.garry@...wei.com>
> Mangled patch? (no "---" separator here)

hmm... not sure. As an experiment, I just downloaded this patch from 
lore.kernel.org and it applies ok.

> 
> Overall this looks great, just a few comments further down...
> 

...

>> +}
>> +EXPORT_SYMBOL_GPL(iova_domain_init_rcaches);
>> +
>> +void iova_domain_free_rcaches(struct iova_domain *iovad)
>> +{
>> +	cpuhp_state_remove_instance_nocalls(CPUHP_IOMMU_IOVA_DEAD,
>> +					    &iovad->cpuhp_dead);
>> +	free_iova_rcaches(iovad);
>>    }
>> +EXPORT_SYMBOL_GPL(iova_domain_free_rcaches);
> I think we should continue to expect external callers to clean up with
> put_iova_domain(). 

ok, fine, makes sense

> If they aren't doing that already they have a bug
> (albeit minor), and we don't want to give the impression that it's OK to
> free the caches at any point*other*  than tearing down the whole
> iova_domain, since the implementation really wouldn't expect that.
> 
>>    /*
>>     * Try inserting IOVA range starting with 'iova_pfn' into 'rcache', and
>> @@ -831,7 +872,7 @@ static unsigned long iova_rcache_get(struct iova_domain *iovad,
>>    {
>>    	unsigned int log_size = order_base_2(size);
>>    
>> -	if (log_size >= IOVA_RANGE_CACHE_MAX_SIZE)
>> +	if (log_size >= IOVA_RANGE_CACHE_MAX_SIZE || !iovad->rcaches)
>>    		return 0;
>>    

..

>> @@ -102,6 +92,8 @@ struct iova *reserve_iova(struct iova_domain *iovad, unsigned long pfn_lo,
>>    	unsigned long pfn_hi);
>>    void init_iova_domain(struct iova_domain *iovad, unsigned long granule,
>>    	unsigned long start_pfn);
>> +int iova_domain_init_rcaches(struct iova_domain *iovad);
>> +void iova_domain_free_rcaches(struct iova_domain *iovad);
> As above, I vote for just forward-declaring the free routine in iova.c
> and keeping it entirely private.

ok

> 
>>    struct iova *find_iova(struct iova_domain *iovad, unsigned long pfn);
>>    void put_iova_domain(struct iova_domain *iovad);
>>    #else
>> @@ -157,6 +149,15 @@ static inline void init_iova_domain(struct iova_domain *iovad,
>>    {
>>    }
>>    
>> +static inline int iova_domain_init_rcaches(struct iova_domain *iovad)
>> +{
>> +	return -ENOTSUPP;
>> +}
>> +
>> +static inline void iova_domain_free_rcaches(struct iova_domain *iovad)
>> +{
>> +}
>> +
> I'd be inclined not to add stubs at all - I think it's a reasonable
> assumption that anyone involved enough to care about rcaches has a hard
> dependency on IOMMU_IOVA already.

So iova_domain_free_rcaches() would disappear from here as a result of 
the changes discussed above.

As for iova_domain_init_rcaches(), I was just following the IOMMU/IOVA 
coding practice - that is, always stub.

> It's certainly the case today, and I'd
> hardly want to encourage more users anyway.

I think that stronger deterrents would be needed :)

Anyway, I can remove it.

Thanks,
John