diff mbox

[v3,01/12] crypto: LLVMLinux: Add macro to remove use of VLAIS in crypto code

Message ID 1410766234-1634-2-git-send-email-behanw@converseincode.com (mailing list archive)
State New, archived
Headers show

Commit Message

Behan Webster Sept. 15, 2014, 7:30 a.m. UTC 
From: Behan Webster <behanw@converseincode.com>

Add a macro which replaces the use of a Variable Length Array In Struct (VLAIS)
with a C99 compliant equivalent. This macro instead allocates the appropriate
amount of memory using an char array.

The new code can be compiled with both gcc and clang.

struct shash_desc contains a flexible array member member ctx declared with
CRYPTO_MINALIGN_ATTR, so sizeof(struct shash_desc) aligns the beginning
of the array declared after struct shash_desc with long long.

No trailing padding is required because it is not a struct type that can
be used in an array.

The CRYPTO_MINALIGN_ATTR is required so that desc is aligned with long long
as would be the case for a struct containing a member with
CRYPTO_MINALIGN_ATTR.

Signed-off-by: Behan Webster <behanw@converseincode.com>
---
 include/crypto/hash.h | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Michał Mirosław Sept. 15, 2014, 8:06 a.m. UTC | #1 
2014-09-15 9:30 GMT+02:00  <behanw@converseincode.com>:
[...]
> +#define SHASH_DESC_ON_STACK(shash, tfm)                                  \
> +       char __desc[sizeof(struct shash_desc) +                   \
> +               crypto_shash_descsize(tfm)] CRYPTO_MINALIGN_ATTR; \
> +       struct shash_desc *shash = (struct shash_desc *)__desc
> +

char shash##__desc[] or similar? Otherwise it won't work if you use
this macro twice in the same block.

Best Regards,
Micha? Miros?aw
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Behan Webster Sept. 15, 2014, 8:37 a.m. UTC | #2 
On 09/15/14 01:06, Micha? Miros?aw wrote:
> 2014-09-15 9:30 GMT+02:00  <behanw@converseincode.com>:
> [...]
>> +#define SHASH_DESC_ON_STACK(shash, tfm)                                  \
>> +       char __desc[sizeof(struct shash_desc) +                   \
>> +               crypto_shash_descsize(tfm)] CRYPTO_MINALIGN_ATTR; \
>> +       struct shash_desc *shash = (struct shash_desc *)__desc
>> +
> char shash##__desc[] or similar? Otherwise it won't work if you use
> this macro twice in the same block.
>
> Best Regards,
> Micha? Miros?aw
Good thinking. Will fix.

Behan
Herbert Xu Sept. 17, 2014, 9:22 a.m. UTC | #3 
On Mon, Sep 15, 2014 at 12:30:23AM -0700, behanw@converseincode.com wrote:
> From: Behan Webster <behanw@converseincode.com>
> 
> Add a macro which replaces the use of a Variable Length Array In Struct (VLAIS)
> with a C99 compliant equivalent. This macro instead allocates the appropriate
> amount of memory using an char array.
> 
> The new code can be compiled with both gcc and clang.
> 
> struct shash_desc contains a flexible array member member ctx declared with
> CRYPTO_MINALIGN_ATTR, so sizeof(struct shash_desc) aligns the beginning
> of the array declared after struct shash_desc with long long.
> 
> No trailing padding is required because it is not a struct type that can
> be used in an array.
> 
> The CRYPTO_MINALIGN_ATTR is required so that desc is aligned with long long
> as would be the case for a struct containing a member with
> CRYPTO_MINALIGN_ATTR.
> 
> Signed-off-by: Behan Webster <behanw@converseincode.com>

Acked-by: Herbert Xu <herbert@gondor.apana.org.au>

Thanks,
Dmitry Kasatkin Sept. 17, 2014, 11:15 a.m. UTC | #4 
On 17/09/14 12:22, Herbert Xu wrote:
> On Mon, Sep 15, 2014 at 12:30:23AM -0700, behanw@converseincode.com wrote:
>> From: Behan Webster <behanw@converseincode.com>
>>
>> Add a macro which replaces the use of a Variable Length Array In Struct (VLAIS)
>> with a C99 compliant equivalent. This macro instead allocates the appropriate
>> amount of memory using an char array.
>>
>> The new code can be compiled with both gcc and clang.
>>
>> struct shash_desc contains a flexible array member member ctx declared with
>> CRYPTO_MINALIGN_ATTR, so sizeof(struct shash_desc) aligns the beginning
>> of the array declared after struct shash_desc with long long.
>>
>> No trailing padding is required because it is not a struct type that can
>> be used in an array.
>>
>> The CRYPTO_MINALIGN_ATTR is required so that desc is aligned with long long
>> as would be the case for a struct containing a member with
>> CRYPTO_MINALIGN_ATTR.
>>
>> Signed-off-by: Behan Webster <behanw@converseincode.com>
> Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
>
> Thanks,

Just in case.
I would still follow advice from "Micha? Miros?aw" to use shash##__desc[]

- Dmitry


--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Herbert Xu Sept. 17, 2014, 11:30 a.m. UTC | #5 
On Wed, Sep 17, 2014 at 02:15:40PM +0300, Dmitry Kasatkin wrote:
> On 17/09/14 12:22, Herbert Xu wrote:
> > On Mon, Sep 15, 2014 at 12:30:23AM -0700, behanw@converseincode.com wrote:
> >> From: Behan Webster <behanw@converseincode.com>
> >>
> >> Add a macro which replaces the use of a Variable Length Array In Struct (VLAIS)
> >> with a C99 compliant equivalent. This macro instead allocates the appropriate
> >> amount of memory using an char array.
> >>
> >> The new code can be compiled with both gcc and clang.
> >>
> >> struct shash_desc contains a flexible array member member ctx declared with
> >> CRYPTO_MINALIGN_ATTR, so sizeof(struct shash_desc) aligns the beginning
> >> of the array declared after struct shash_desc with long long.
> >>
> >> No trailing padding is required because it is not a struct type that can
> >> be used in an array.
> >>
> >> The CRYPTO_MINALIGN_ATTR is required so that desc is aligned with long long
> >> as would be the case for a struct containing a member with
> >> CRYPTO_MINALIGN_ATTR.
> >>
> >> Signed-off-by: Behan Webster <behanw@converseincode.com>
> > Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
> >
> > Thanks,
> 
> Just in case.
> I would still follow advice from "Micha? Miros?aw" to use shash##__desc[]

Oh yes of course.  My ack is more about the approach.

Cheers,
Behan Webster Sept. 17, 2014, 3:26 p.m. UTC | #6 
On 09/17/14 04:30, Herbert Xu wrote:
> On Wed, Sep 17, 2014 at 02:15:40PM +0300, Dmitry Kasatkin wrote:
>> On 17/09/14 12:22, Herbert Xu wrote:
>>> On Mon, Sep 15, 2014 at 12:30:23AM -0700, behanw@converseincode.com wrote:
>>>> From: Behan Webster <behanw@converseincode.com>
>>>>
>>>> Add a macro which replaces the use of a Variable Length Array In Struct (VLAIS)
>>>> with a C99 compliant equivalent. This macro instead allocates the appropriate
>>>> amount of memory using an char array.
>>>>
>>>> The new code can be compiled with both gcc and clang.
>>>>
>>>> struct shash_desc contains a flexible array member member ctx declared with
>>>> CRYPTO_MINALIGN_ATTR, so sizeof(struct shash_desc) aligns the beginning
>>>> of the array declared after struct shash_desc with long long.
>>>>
>>>> No trailing padding is required because it is not a struct type that can
>>>> be used in an array.
>>>>
>>>> The CRYPTO_MINALIGN_ATTR is required so that desc is aligned with long long
>>>> as would be the case for a struct containing a member with
>>>> CRYPTO_MINALIGN_ATTR.
>>>>
>>>> Signed-off-by: Behan Webster <behanw@converseincode.com>
>>> Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
>>>
>>> Thanks,
>> Just in case.
>> I would still follow advice from "Micha? Miros?aw" to use shash##__desc[]
Absolutely. I will be posting a v4 patchset . Just waiting a bit more 
for more comments on v3.

The macro from v4 will look like this which I believe will satisfy the 
concern and indeed be safer than my previous version.

+#define SHASH_DESC_ON_STACK(shash, tfm)                                  \
+       char __##shash##_desc[sizeof(struct shash_desc) +        \
+               crypto_shash_descsize(tfm)] CRYPTO_MINALIGN_ATTR; \
+       struct shash_desc *shash = (struct shash_desc *)__##shash##_desc

Hmm. Is it worth adding a comment with this macro explaining the reason this works? Essentially much of what is in the commit message?
  

> Oh yes of course.  My ack is more about the approach.

Wonderful!

Indeed. I would have asked for you to wait for v4 anyways. :)

Thank you,

Behan
diff mbox

Patch

diff --git a/include/crypto/hash.h b/include/crypto/hash.h
index a391955..541125b 100644
--- a/include/crypto/hash.h
+++ b/include/crypto/hash.h
@@ -58,6 +58,11 @@  struct shash_desc {
 	void *__ctx[] CRYPTO_MINALIGN_ATTR;
 };
 
+#define SHASH_DESC_ON_STACK(shash, tfm)				  \
+	char __desc[sizeof(struct shash_desc) +                   \
+		crypto_shash_descsize(tfm)] CRYPTO_MINALIGN_ATTR; \
+	struct shash_desc *shash = (struct shash_desc *)__desc
+
 struct shash_alg {
 	int (*init)(struct shash_desc *desc);
 	int (*update)(struct shash_desc *desc, const u8 *data,