Re: [PATCH v2 07/21] crypto: shash, caam: Make use of the new sg_map helper function

[Logan Gunthorpe]

On 28/04/17 11:51 AM, Herbert Xu wrote:
> On Fri, Apr 28, 2017 at 10:53:45AM -0600, Logan Gunthorpe wrote:
>>
>>
>> On 28/04/17 12:30 AM, Herbert Xu wrote:
>>> You are right.  Indeed the existing code looks buggy as they
>>> don't take sg->offset into account when doing the kmap.  Could
>>> you send me some patches that fix these problems first so that
>>> they can be easily backported?
>>
>> Ok, I think the only buggy one in crypto is hifn_795x. Shash and caam
>> both do have the sg->offset accounted for. I'll send a patch for the
>> buggy one shortly.
> 
> I think they're all buggy when sg->offset is greater than PAGE_SIZE.

Yes, technically. But that's a _very_ common mistake. Pretty nearly
every case I looked at did not take that into account. I don't think
sg's that point to more than one continuous page are all that common.

Fixing all those cases without making a common function is a waste of
time IMO.

Logan

Re: [PATCH v2 07/21] crypto: shash, caam: Make use of the new sg_map helper function

[Herbert Xu]

On Fri, Apr 28, 2017 at 10:53:45AM -0600, Logan Gunthorpe wrote:
> 
> 
> On 28/04/17 12:30 AM, Herbert Xu wrote:
> > You are right.  Indeed the existing code looks buggy as they
> > don't take sg->offset into account when doing the kmap.  Could
> > you send me some patches that fix these problems first so that
> > they can be easily backported?
> 
> Ok, I think the only buggy one in crypto is hifn_795x. Shash and caam
> both do have the sg->offset accounted for. I'll send a patch for the
> buggy one shortly.

I think they're all buggy when sg->offset is greater than PAGE_SIZE.

Thanks,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 07/21] crypto: shash, caam: Make use of the new sg_map helper function

[Logan Gunthorpe]

On 28/04/17 12:30 AM, Herbert Xu wrote:
> You are right.  Indeed the existing code looks buggy as they
> don't take sg->offset into account when doing the kmap.  Could
> you send me some patches that fix these problems first so that
> they can be easily backported?

Ok, I think the only buggy one in crypto is hifn_795x. Shash and caam
both do have the sg->offset accounted for. I'll send a patch for the
buggy one shortly.

Logan

Re: [PATCH v2 07/21] crypto: shash, caam: Make use of the new sg_map helper function

[Herbert Xu]

On Thu, Apr 27, 2017 at 09:45:57AM -0600, Logan Gunthorpe wrote:
> 
> 
> On 26/04/17 09:56 PM, Herbert Xu wrote:
> > On Tue, Apr 25, 2017 at 12:20:54PM -0600, Logan Gunthorpe wrote:
> >> Very straightforward conversion to the new function in the caam driver
> >> and shash library.
> >>
> >> Signed-off-by: Logan Gunthorpe 
> >> Cc: Herbert Xu 
> >> Cc: "David S. Miller" 
> >> ---
> >>  crypto/shash.c| 9 ++---
> >>  drivers/crypto/caam/caamalg.c | 8 +++-
> >>  2 files changed, 9 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/crypto/shash.c b/crypto/shash.c
> >> index 5e31c8d..5914881 100644
> >> --- a/crypto/shash.c
> >> +++ b/crypto/shash.c
> >> @@ -283,10 +283,13 @@ int shash_ahash_digest(struct ahash_request *req, 
> >> struct shash_desc *desc)
> >>if (nbytes < min(sg->length, ((unsigned int)(PAGE_SIZE)) - offset)) {
> >>void *data;
> >>  
> >> -  data = kmap_atomic(sg_page(sg));
> >> -  err = crypto_shash_digest(desc, data + offset, nbytes,
> >> +  data = sg_map(sg, 0, SG_KMAP_ATOMIC);
> >> +  if (IS_ERR(data))
> >> +  return PTR_ERR(data);
> >> +
> >> +  err = crypto_shash_digest(desc, data, nbytes,
> >>  req->result);
> >> -  kunmap_atomic(data);
> >> +  sg_unmap(sg, data, 0, SG_KMAP_ATOMIC);
> >>crypto_yield(desc->flags);
> >>} else
> >>err = crypto_shash_init(desc) ?:
> > 
> > Nack.  This is an optimisation for the special case of a single
> > SG list entry.  In fact in the common case the kmap_atomic should
> > disappear altogether in the no-highmem case.  So replacing it
> > with sg_map is not acceptable.
> 
> What you seem to have missed is that sg_map is just a thin wrapper
> around kmap_atomic. Perhaps with a future check for a mappable page.
> This change should have zero impact on performance.

You are right.  Indeed the existing code looks buggy as they
don't take sg->offset into account when doing the kmap.  Could
you send me some patches that fix these problems first so that
they can be easily backported?

Thanks,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 07/21] crypto: shash, caam: Make use of the new sg_map helper function

[Logan Gunthorpe]

On 26/04/17 09:56 PM, Herbert Xu wrote:
> On Tue, Apr 25, 2017 at 12:20:54PM -0600, Logan Gunthorpe wrote:
>> Very straightforward conversion to the new function in the caam driver
>> and shash library.
>>
>> Signed-off-by: Logan Gunthorpe 
>> Cc: Herbert Xu 
>> Cc: "David S. Miller" 
>> ---
>>  crypto/shash.c| 9 ++---
>>  drivers/crypto/caam/caamalg.c | 8 +++-
>>  2 files changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/crypto/shash.c b/crypto/shash.c
>> index 5e31c8d..5914881 100644
>> --- a/crypto/shash.c
>> +++ b/crypto/shash.c
>> @@ -283,10 +283,13 @@ int shash_ahash_digest(struct ahash_request *req, 
>> struct shash_desc *desc)
>>  if (nbytes < min(sg->length, ((unsigned int)(PAGE_SIZE)) - offset)) {
>>  void *data;
>>  
>> -data = kmap_atomic(sg_page(sg));
>> -err = crypto_shash_digest(desc, data + offset, nbytes,
>> +data = sg_map(sg, 0, SG_KMAP_ATOMIC);
>> +if (IS_ERR(data))
>> +return PTR_ERR(data);
>> +
>> +err = crypto_shash_digest(desc, data, nbytes,
>>req->result);
>> -kunmap_atomic(data);
>> +sg_unmap(sg, data, 0, SG_KMAP_ATOMIC);
>>  crypto_yield(desc->flags);
>>  } else
>>  err = crypto_shash_init(desc) ?:
> 
> Nack.  This is an optimisation for the special case of a single
> SG list entry.  In fact in the common case the kmap_atomic should
> disappear altogether in the no-highmem case.  So replacing it
> with sg_map is not acceptable.

What you seem to have missed is that sg_map is just a thin wrapper
around kmap_atomic. Perhaps with a future check for a mappable page.
This change should have zero impact on performance.

Logan

Re: [PATCH v2 07/21] crypto: shash, caam: Make use of the new sg_map helper function

[Herbert Xu]

On Tue, Apr 25, 2017 at 12:20:54PM -0600, Logan Gunthorpe wrote:
> Very straightforward conversion to the new function in the caam driver
> and shash library.
> 
> Signed-off-by: Logan Gunthorpe 
> Cc: Herbert Xu 
> Cc: "David S. Miller" 
> ---
>  crypto/shash.c| 9 ++---
>  drivers/crypto/caam/caamalg.c | 8 +++-
>  2 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/crypto/shash.c b/crypto/shash.c
> index 5e31c8d..5914881 100644
> --- a/crypto/shash.c
> +++ b/crypto/shash.c
> @@ -283,10 +283,13 @@ int shash_ahash_digest(struct ahash_request *req, 
> struct shash_desc *desc)
>   if (nbytes < min(sg->length, ((unsigned int)(PAGE_SIZE)) - offset)) {
>   void *data;
>  
> - data = kmap_atomic(sg_page(sg));
> - err = crypto_shash_digest(desc, data + offset, nbytes,
> + data = sg_map(sg, 0, SG_KMAP_ATOMIC);
> + if (IS_ERR(data))
> + return PTR_ERR(data);
> +
> + err = crypto_shash_digest(desc, data, nbytes,
> req->result);
> - kunmap_atomic(data);
> + sg_unmap(sg, data, 0, SG_KMAP_ATOMIC);
>   crypto_yield(desc->flags);
>   } else
>   err = crypto_shash_init(desc) ?:

Nack.  This is an optimisation for the special case of a single
SG list entry.  In fact in the common case the kmap_atomic should
disappear altogether in the no-highmem case.  So replacing it
with sg_map is not acceptable.

Cheers,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt