Re: [PATCH v2 0/2] crypto: removing various VLAs

2018-04-26 Thread Salvatore Mesoraca 
2018-04-20 18:51 GMT+02:00 Herbert Xu :
> On Mon, Apr 09, 2018 at 03:54:45PM +0200, Salvatore Mesoraca wrote:
>> v2:
>>   As suggested by Herbert Xu, the blocksize and alignmask checks
>>   have been moved to crypto_check_alg.
>>   So, now, all the other separate checks are not necessary.
>>   Also, the defines have been moved to include/crypto/algapi.h.
>>
>> v1:
>>   As suggested by Laura Abbott[1], I'm resending my patch with
>>   MAX_BLOCKSIZE and MAX_ALIGNMASK defined in an header, so they
>>   can be used in other places.
>>   I took this opportunity to deal with some other VLAs not
>>   handled in the old patch.
>>
>> [1] http://lkml.kernel.org/r/4e536889-439a-49e6-dd95-2d4286913...@redhat.com
>>
>> Salvatore Mesoraca (2):
>>   crypto: api - laying defines and checks for statically allocated
>> buffers
>>   crypto: remove several VLAs
>>
>>  crypto/algapi.c | 10 ++
>>  crypto/cfb.c|  7 +++
>>  crypto/cipher.c |  3 ++-
>>  crypto/ctr.c|  4 ++--
>>  crypto/cts.c|  5 +++--
>>  crypto/pcbc.c   |  5 +++--
>>  include/crypto/algapi.h |  8 
>>  7 files changed, 31 insertions(+), 11 deletions(-)
>
> All applied.  Thanks.

Thank you very much.

Salvatore

Re: [PATCH v2 0/2] crypto: removing various VLAs

2018-04-26 Thread Salvatore Mesoraca 
2018-04-20 18:51 GMT+02:00 Herbert Xu :
> On Mon, Apr 09, 2018 at 03:54:45PM +0200, Salvatore Mesoraca wrote:
>> v2:
>>   As suggested by Herbert Xu, the blocksize and alignmask checks
>>   have been moved to crypto_check_alg.
>>   So, now, all the other separate checks are not necessary.
>>   Also, the defines have been moved to include/crypto/algapi.h.
>>
>> v1:
>>   As suggested by Laura Abbott[1], I'm resending my patch with
>>   MAX_BLOCKSIZE and MAX_ALIGNMASK defined in an header, so they
>>   can be used in other places.
>>   I took this opportunity to deal with some other VLAs not
>>   handled in the old patch.
>>
>> [1] http://lkml.kernel.org/r/4e536889-439a-49e6-dd95-2d4286913...@redhat.com
>>
>> Salvatore Mesoraca (2):
>>   crypto: api - laying defines and checks for statically allocated
>> buffers
>>   crypto: remove several VLAs
>>
>>  crypto/algapi.c | 10 ++
>>  crypto/cfb.c|  7 +++
>>  crypto/cipher.c |  3 ++-
>>  crypto/ctr.c|  4 ++--
>>  crypto/cts.c|  5 +++--
>>  crypto/pcbc.c   |  5 +++--
>>  include/crypto/algapi.h |  8 
>>  7 files changed, 31 insertions(+), 11 deletions(-)
>
> All applied.  Thanks.

Thank you very much.

Salvatore

Re: [PATCH v2 0/2] crypto: removing various VLAs

2018-04-20 Thread Herbert Xu 
On Mon, Apr 09, 2018 at 03:54:45PM +0200, Salvatore Mesoraca wrote:
> v2:
>   As suggested by Herbert Xu, the blocksize and alignmask checks
>   have been moved to crypto_check_alg.
>   So, now, all the other separate checks are not necessary.
>   Also, the defines have been moved to include/crypto/algapi.h.
> 
> v1:
>   As suggested by Laura Abbott[1], I'm resending my patch with
>   MAX_BLOCKSIZE and MAX_ALIGNMASK defined in an header, so they
>   can be used in other places.
>   I took this opportunity to deal with some other VLAs not
>   handled in the old patch.
> 
> [1] http://lkml.kernel.org/r/4e536889-439a-49e6-dd95-2d4286913...@redhat.com
> 
> Salvatore Mesoraca (2):
>   crypto: api - laying defines and checks for statically allocated
> buffers
>   crypto: remove several VLAs
> 
>  crypto/algapi.c | 10 ++
>  crypto/cfb.c|  7 +++
>  crypto/cipher.c |  3 ++-
>  crypto/ctr.c|  4 ++--
>  crypto/cts.c|  5 +++--
>  crypto/pcbc.c   |  5 +++--
>  include/crypto/algapi.h |  8 
>  7 files changed, 31 insertions(+), 11 deletions(-)

All applied.  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 0/2] crypto: removing various VLAs

2018-04-20 Thread Herbert Xu 
On Mon, Apr 09, 2018 at 03:54:45PM +0200, Salvatore Mesoraca wrote:
> v2:
>   As suggested by Herbert Xu, the blocksize and alignmask checks
>   have been moved to crypto_check_alg.
>   So, now, all the other separate checks are not necessary.
>   Also, the defines have been moved to include/crypto/algapi.h.
> 
> v1:
>   As suggested by Laura Abbott[1], I'm resending my patch with
>   MAX_BLOCKSIZE and MAX_ALIGNMASK defined in an header, so they
>   can be used in other places.
>   I took this opportunity to deal with some other VLAs not
>   handled in the old patch.
> 
> [1] http://lkml.kernel.org/r/4e536889-439a-49e6-dd95-2d4286913...@redhat.com
> 
> Salvatore Mesoraca (2):
>   crypto: api - laying defines and checks for statically allocated
> buffers
>   crypto: remove several VLAs
> 
>  crypto/algapi.c | 10 ++
>  crypto/cfb.c|  7 +++
>  crypto/cipher.c |  3 ++-
>  crypto/ctr.c|  4 ++--
>  crypto/cts.c|  5 +++--
>  crypto/pcbc.c   |  5 +++--
>  include/crypto/algapi.h |  8 
>  7 files changed, 31 insertions(+), 11 deletions(-)

All applied.  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 0/2] crypto: removing various VLAs

2018-04-11 Thread David Laight 
From: Salvatore Mesoraca
> Sent: 09 April 2018 17:38
...
> > You can also do much better than allocating MAX_BLOCKSIZE + MAX_ALIGNMASK
> > bytes by requesting 'long' aligned on-stack memory.
> > The easiest way is to define a union like:
> >
> > union crypto_tmp {
> > u8 buf[CRYPTO_MAX_TMP_BUF];
> > long buf_align;
> > };
> >
> > Then in each function:
> >
> > union tmp crypto_tmp;
> > u8 *keystream = PTR_ALIGN(tmp.buf, alignmask + 1);
> >
> > I think CRYPTO_MAX_TMP_BUF needs to be MAX_BLOCKSIZE + MAX_ALIGNMASK - 
> > sizeof (long).
> 
> Yeah, that would be nice, it might save us 4-8 bytes on the stack.
> But I was thinking, wouldn't it be even better to do something like:
> 
> u8 buf[CRYPTO_MAX_TMP_BUF] __aligned(__alignof__(long));
> u8 *keystream = PTR_ALIGN(buf, alignmask + 1);
> 
> In this case __aligned should work, if I'm not missing some other
> subtle GCC caveat.

Thinking further, there is no point aligning the buffer to less than
the maximum alignment allowed - it just adds code.

So you end up with:
#define MAX_STACK_ALIGN __alignof__(long)  /* Largest type the compiler can 
align on stack */
#define CRYPTO_MAX_TMP_BUF (MAX_BLOCKSIZE + MAX_ALIGNMASK + 1 - MAX_STACK_ALIGN)
u8 buf[CRYPTO_MAX_TMP_BUF] __aligned(MAX_STACK_ALIGN);
u8 *keystream = PTR_ALIGN(buf, MAX_ALIGNMASK + 1);

The last two lines could be put into a #define of their own so that the 'call 
sites'
don't need to know the gory details of how the buffer is defined.

In principle you could just have:
u8 keystream[MAX_BLOCKSIZE] __aligned(MAX_ALIGNMASK + 1);

But that will go wrong if the stack alignment has gone wrong somewhere
and generates a double stack frame if the requested alignment is larger
than the expected stack alignment.

IIRC there is a gcc command line option to enforce stack alignment on
some/all function entry prologues. The gory details are held in some
old brain cells somewhere.

David

RE: [PATCH v2 0/2] crypto: removing various VLAs

2018-04-11 Thread David Laight 
From: Salvatore Mesoraca
> Sent: 09 April 2018 17:38
...
> > You can also do much better than allocating MAX_BLOCKSIZE + MAX_ALIGNMASK
> > bytes by requesting 'long' aligned on-stack memory.
> > The easiest way is to define a union like:
> >
> > union crypto_tmp {
> > u8 buf[CRYPTO_MAX_TMP_BUF];
> > long buf_align;
> > };
> >
> > Then in each function:
> >
> > union tmp crypto_tmp;
> > u8 *keystream = PTR_ALIGN(tmp.buf, alignmask + 1);
> >
> > I think CRYPTO_MAX_TMP_BUF needs to be MAX_BLOCKSIZE + MAX_ALIGNMASK - 
> > sizeof (long).
> 
> Yeah, that would be nice, it might save us 4-8 bytes on the stack.
> But I was thinking, wouldn't it be even better to do something like:
> 
> u8 buf[CRYPTO_MAX_TMP_BUF] __aligned(__alignof__(long));
> u8 *keystream = PTR_ALIGN(buf, alignmask + 1);
> 
> In this case __aligned should work, if I'm not missing some other
> subtle GCC caveat.

Thinking further, there is no point aligning the buffer to less than
the maximum alignment allowed - it just adds code.

So you end up with:
#define MAX_STACK_ALIGN __alignof__(long)  /* Largest type the compiler can 
align on stack */
#define CRYPTO_MAX_TMP_BUF (MAX_BLOCKSIZE + MAX_ALIGNMASK + 1 - MAX_STACK_ALIGN)
u8 buf[CRYPTO_MAX_TMP_BUF] __aligned(MAX_STACK_ALIGN);
u8 *keystream = PTR_ALIGN(buf, MAX_ALIGNMASK + 1);

The last two lines could be put into a #define of their own so that the 'call 
sites'
don't need to know the gory details of how the buffer is defined.

In principle you could just have:
u8 keystream[MAX_BLOCKSIZE] __aligned(MAX_ALIGNMASK + 1);

But that will go wrong if the stack alignment has gone wrong somewhere
and generates a double stack frame if the requested alignment is larger
than the expected stack alignment.

IIRC there is a gcc command line option to enforce stack alignment on
some/all function entry prologues. The gory details are held in some
old brain cells somewhere.

David

Re: [PATCH v2 0/2] crypto: removing various VLAs

2018-04-09 Thread Salvatore Mesoraca 
2018-04-09 16:35 GMT+02:00 David Laight :
> From: Salvatore Mesoraca
>> Sent: 09 April 2018 14:55
>>
>> v2:
>>   As suggested by Herbert Xu, the blocksize and alignmask checks
>>   have been moved to crypto_check_alg.
>>   So, now, all the other separate checks are not necessary.
>>   Also, the defines have been moved to include/crypto/algapi.h.
>>
>> v1:
>>   As suggested by Laura Abbott[1], I'm resending my patch with
>>   MAX_BLOCKSIZE and MAX_ALIGNMASK defined in an header, so they
>>   can be used in other places.
>>   I took this opportunity to deal with some other VLAs not
>>   handled in the old patch.
>
> If the constants are visible they need better names.
> Maybe CRYPTO_MAX_xxx.

You are right, in fact I renamed them, but forget to write about this
in the change log.
The new names look like MAX_CIPHER_*.

> You can also do much better than allocating MAX_BLOCKSIZE + MAX_ALIGNMASK
> bytes by requesting 'long' aligned on-stack memory.
> The easiest way is to define a union like:
>
> union crypto_tmp {
> u8 buf[CRYPTO_MAX_TMP_BUF];
> long buf_align;
> };
>
> Then in each function:
>
> union tmp crypto_tmp;
> u8 *keystream = PTR_ALIGN(tmp.buf, alignmask + 1);
>
> I think CRYPTO_MAX_TMP_BUF needs to be MAX_BLOCKSIZE + MAX_ALIGNMASK - sizeof 
> (long).

Yeah, that would be nice, it might save us 4-8 bytes on the stack.
But I was thinking, wouldn't it be even better to do something like:

u8 buf[CRYPTO_MAX_TMP_BUF] __aligned(__alignof__(long));
u8 *keystream = PTR_ALIGN(buf, alignmask + 1);

In this case __aligned should work, if I'm not missing some other
subtle GCC caveat.

Thank you,

Salvatore

Re: [PATCH v2 0/2] crypto: removing various VLAs

2018-04-09 Thread Salvatore Mesoraca 
2018-04-09 16:35 GMT+02:00 David Laight :
> From: Salvatore Mesoraca
>> Sent: 09 April 2018 14:55
>>
>> v2:
>>   As suggested by Herbert Xu, the blocksize and alignmask checks
>>   have been moved to crypto_check_alg.
>>   So, now, all the other separate checks are not necessary.
>>   Also, the defines have been moved to include/crypto/algapi.h.
>>
>> v1:
>>   As suggested by Laura Abbott[1], I'm resending my patch with
>>   MAX_BLOCKSIZE and MAX_ALIGNMASK defined in an header, so they
>>   can be used in other places.
>>   I took this opportunity to deal with some other VLAs not
>>   handled in the old patch.
>
> If the constants are visible they need better names.
> Maybe CRYPTO_MAX_xxx.

You are right, in fact I renamed them, but forget to write about this
in the change log.
The new names look like MAX_CIPHER_*.

> You can also do much better than allocating MAX_BLOCKSIZE + MAX_ALIGNMASK
> bytes by requesting 'long' aligned on-stack memory.
> The easiest way is to define a union like:
>
> union crypto_tmp {
> u8 buf[CRYPTO_MAX_TMP_BUF];
> long buf_align;
> };
>
> Then in each function:
>
> union tmp crypto_tmp;
> u8 *keystream = PTR_ALIGN(tmp.buf, alignmask + 1);
>
> I think CRYPTO_MAX_TMP_BUF needs to be MAX_BLOCKSIZE + MAX_ALIGNMASK - sizeof 
> (long).

Yeah, that would be nice, it might save us 4-8 bytes on the stack.
But I was thinking, wouldn't it be even better to do something like:

u8 buf[CRYPTO_MAX_TMP_BUF] __aligned(__alignof__(long));
u8 *keystream = PTR_ALIGN(buf, alignmask + 1);

In this case __aligned should work, if I'm not missing some other
subtle GCC caveat.

Thank you,

Salvatore

RE: [PATCH v2 0/2] crypto: removing various VLAs

2018-04-09 Thread David Laight 
From: Salvatore Mesoraca
> Sent: 09 April 2018 14:55
> 
> v2:
>   As suggested by Herbert Xu, the blocksize and alignmask checks
>   have been moved to crypto_check_alg.
>   So, now, all the other separate checks are not necessary.
>   Also, the defines have been moved to include/crypto/algapi.h.
> 
> v1:
>   As suggested by Laura Abbott[1], I'm resending my patch with
>   MAX_BLOCKSIZE and MAX_ALIGNMASK defined in an header, so they
>   can be used in other places.
>   I took this opportunity to deal with some other VLAs not
>   handled in the old patch.

If the constants are visible they need better names.
Maybe CRYPTO_MAX_xxx.

You can also do much better than allocating MAX_BLOCKSIZE + MAX_ALIGNMASK
bytes by requesting 'long' aligned on-stack memory.
The easiest way is to define a union like:

union crypto_tmp {
u8 buf[CRYPTO_MAX_TMP_BUF];
long buf_align;
};

Then in each function:

union tmp crypto_tmp;
u8 *keystream = PTR_ALIGN(tmp.buf, alignmask + 1);

I think CRYPTO_MAX_TMP_BUF needs to be MAX_BLOCKSIZE + MAX_ALIGNMASK - sizeof 
(long).

David

RE: [PATCH v2 0/2] crypto: removing various VLAs

2018-04-09 Thread David Laight 
From: Salvatore Mesoraca
> Sent: 09 April 2018 14:55
> 
> v2:
>   As suggested by Herbert Xu, the blocksize and alignmask checks
>   have been moved to crypto_check_alg.
>   So, now, all the other separate checks are not necessary.
>   Also, the defines have been moved to include/crypto/algapi.h.
> 
> v1:
>   As suggested by Laura Abbott[1], I'm resending my patch with
>   MAX_BLOCKSIZE and MAX_ALIGNMASK defined in an header, so they
>   can be used in other places.
>   I took this opportunity to deal with some other VLAs not
>   handled in the old patch.

If the constants are visible they need better names.
Maybe CRYPTO_MAX_xxx.

You can also do much better than allocating MAX_BLOCKSIZE + MAX_ALIGNMASK
bytes by requesting 'long' aligned on-stack memory.
The easiest way is to define a union like:

union crypto_tmp {
u8 buf[CRYPTO_MAX_TMP_BUF];
long buf_align;
};

Then in each function:

union tmp crypto_tmp;
u8 *keystream = PTR_ALIGN(tmp.buf, alignmask + 1);

I think CRYPTO_MAX_TMP_BUF needs to be MAX_BLOCKSIZE + MAX_ALIGNMASK - sizeof 
(long).

David