Re: [PATCH v4 2/3] iommu/iova: Avoid double-negatives in magazine helpers

[Robin Murphy <robin.murphy@arm.com>]

On 2021-01-18 10:55, John Garry wrote:
> On 18/01/2021 10:08, Jean-Philippe Brucker wrote:
>>>> Any idea why that's happening?  This fix seems ok but if we're 
>>>> expecting
>>>> allocation failures for the loaded magazine then we could easily get it
>>>> for cpu_rcaches too, and get a similar abort at runtime.
>>> It's not specifically that we expect them (allocation failures for the
>>> loaded magazine), rather we should make safe against it.
>>>
>>> So could you be more specific in your concern for the cpu_rcache 
>>> failure?
>>> cpu_rcache magazine assignment comes from this logic.
>> If this fails:
>>
>> drivers/iommu/iova.c:847: rcache->cpu_rcaches = 
>> __alloc_percpu(sizeof(*cpu_rcache), cache_line_size());
>>
>> then we'll get an Oops in __iova_rcache_get(). So if we're making the
>> module safer against magazine allocation failure, shouldn't we also
>> protect against cpu_rcaches allocation failure?
> 
> Ah, gotcha. So we have the WARN there, but that's not much use as this 
> would still crash, as you say.
> 
> So maybe we can embed the cpu rcaches in iova_domain struct, to avoid 
> the separate (failable) cpu rcache allocation.

Is that even possible? The size of percpu data isn't known at compile 
time, so at best it would add ugly runtime complexity to any allocation 
of a struct iova_domain by itself, but worse than that it means that 
embedding iova_domain in any other structure becomes completely broken, no?

Robin.

> Alternatively, we could add NULL checks __iova_rcache_get() et al for 
> this allocation failure but that's not preferable as it's fastpath.
> 
> Finally so we could pass back an error code from init_iova_rcache() to 
> its only caller, init_iova_domain(); but that has multiple callers and 
> would need to be fixed up.
> 
> Not sure which is best or on other options.
> 
> Thanks,
> John