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

[Horia Geanta]

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:
>>> On Wed, May 29, 2019 at 01:27:28PM -0700, Eric Biggers wrote:
>>>>
>>>> So what about the other places that also pass an IV located next to the data,
>>>> like crypto/ccm.c and crypto/adiantum.c?  If we're actually going to make this a
>> Fix for ccm is WIP.
>> We were not aware of adiantum since our crypto engine does not accelerate it.
>>
>>>> new API requirement, then we need to add a debugging option that makes the API
>>>> detect this violation so that the other places can be fixed too.
>>>>
>> IMO this is not a new crypto API requirement.
>> crypto API and its users must follow DMA API rules, besides crypto-specific ones.
>>
>> In this particular case, crypto/gcm.c is both an implementation and a crypto API
>> user, since it uses underneath ctr(aes) (and ghash).
>> Currently generic gcm implementation is breaking DMA API, since part of the dst
>> buffer (auth_tag) provided to ctr(aes) is sharing a cache line with some other
>> data structure (iv).
>>
>> The DMA API rule is mentioned in Documentation/DMA-API.txt
>>
>> .. warning::
>>
>>         Memory coherency operates at a granularity called the cache
>>         line width.  In order for memory mapped by this API to operate
>>         correctly, the mapped region must begin exactly on a cache line
>>         boundary and end exactly on one (to prevent two separately mapped
>>         regions from sharing a single cache line).
>>
>>
> 
> This is overly restrictive, and not in line with reality. The whole
> networking stack operates on buffers shifted by 2 bytes if
> NET_IP_ALIGN is left at its default value of 2. There are numerous
> examples in other places as well.
> 
> Given that kmalloc() will take the cacheline granularity into account
> if necessary, the only way this issue can hit is when a single kmalloc
> buffer is written to by two different masters.
> 
I guess there are only two options:
-either cache line sharing is avoided OR
-users need to be *aware* they are sharing the cache line and some rules /
assumptions are in place on how to safely work on the data

What you are probably saying is that 2nd option is sometimes the way to go.

>>>> Also, did you consider whether there's any way to make the crypto API handle
>>>> this automatically, so that all the individual users don't have to?
>> That would probably work, but I guess it would come up with a big overhead.
>>
>> I am thinking crypto API would have to check each buffer used by src, dst
>> scatterlists is correctly aligned - starting and ending on cache line boundaries.
>>
>>>
>>> You're absolutely right Eric.
>>>
>>> What I suggested in the old thread is non-sense.  While you can
>>> force GCM to provide the right pointers you cannot force all the
>>> other crypto API users to do this.
>>>
>> Whose problem is that crypto API users don't follow the DMA API requirements?
>>
>>> 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?

Thanks,
Horia