Re: [PATCH] crypto: gcm - fix cacheline sharing

[Horia Geanta]

On 5/30/2019 4:26 PM, Ard Biesheuvel wrote:
> On Thu, 30 May 2019 at 15:18, Horia Geanta <horia.geanta@xxxxxxx> wrote:
>>
>> On 5/30/2019 11:08 AM, Ard Biesheuvel wrote:
>>> On Thu, 30 May 2019 at 09:46, Horia Geanta <horia.geanta@xxxxxxx> wrote:
>>>>
>>>> On 5/30/2019 8:34 AM, Herbert Xu wrote:
>>>>> It would appear that Ard's latest suggestion should fix the problem
>>>>> and is the correct approach.
>>>>>
>>>> I disagree.
>>>> We shouldn't force crypto implementations to be aware of such inconsistencies in
>>>> the I/O data buffers (pointed to by src/dst scatterlists) that are supposed to
>>>> be safely DMA mapped.
>>>>
>>>
>>> I'm on the fence here. On the one hand, it is slightly dodgy for the
>>> GCM driver to pass a scatterlist referencing a buffer that shares a
>>> cacheline with another buffer passed by an ordinary pointer, and for
>>> which an explicit requirement exists that the callee should update it
>>> before returning.
>>>
>>> On the other hand, I think it is reasonable to require drivers not to
>>> perform such updates while the scatterlist is mapped for DMA, since
>>> fixing it in the callers puts a disproportionate burden on them, given
>>> that non-coherent DMA only represents a small minority of use cases.
>>>
>> The problem with this approach is that the buffers in the scatterlist could
>> hypothetically share cache lines with *any* other CPU-updated data, not just the
>> IV in the crypto request (as it happens here).
>> How could a non-coherent DMA implementation cope with this?
>>
> 
> I don't think the situation is that bad. We only support allocations
> in the linear map for DMA/scatterlists, and buffers in the linear map
> can only share a cacheline if they were allocated using the same
> kmalloc() invocation (on systems that support non-coherent DMA)
> 
> That does mean that it is actually more straightforward to deal with
> this at the level where the allocation occurs, and that would justify
> dealing with it in the callers.
> 
> However, from the callee's point of view, it simply means that you
> should not dereference any request struct pointers for writing while
> the 'dst' scatterlist is mapped for DMA.
> 
Is this the only restriction, or I might find out more in the future?

This kind of violation of an abstraction layer must be clearly documented.

In particular, if crypto API implementations cannot rely on DMA API guarantees,
then exceptions must be listed.

Thanks,
Horia