Re: [PATCH v2 0/2] crypto: removing various VLAs
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
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
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 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
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