diff mbox

[v2] crypto, dm: LLVMLinux: Remove VLAIS usage from dm-crypt

Message ID 1409958138-2727-1-git-send-email-behanw@converseincode.com
State New
Headers show

Commit Message

Behan Webster Sept. 5, 2014, 11:02 p.m. UTC 
From: Jan-Simon Möller <dl9pf@gmx.de>

The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
precludes the use of compilers which don't implement VLAIS (for instance the
Clang compiler). This patch instead allocates the appropriate amount of memory
using an char array.

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: Jan-Simon Möller <dl9pf@gmx.de>
Signed-off-by: Behan Webster <behanw@converseincode.com>
Cc: pageexec@freemail.hu
---
 drivers/md/dm-crypt.c | 38 ++++++++++++++++++--------------------
 1 file changed, 18 insertions(+), 20 deletions(-)

Comments

Milan Broz Sept. 6, 2014, 8:46 a.m. UTC | #1 
On 09/06/2014 01:02 AM, behanw@converseincode.com wrote:
> From: Jan-Simon Möller <dl9pf@gmx.de>
> 
> The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
> precludes the use of compilers which don't implement VLAIS (for instance the
> Clang compiler). This patch instead allocates the appropriate amount of memory
> using an char array.

Well, if clang (or C99 code) is now preferred for kernel, why not.

Just please commit the patch series en bloc (together with the patches
removing VLAIS from crypto code you posted to cryptoapi list).

Dmcrypt should always use the same coding practices as crypto part of the kernel
so we should not divert here.

> -	struct {
> -		struct shash_desc desc;
> -		char ctx[crypto_shash_descsize(lmk->hash_tfm)];
> -	} sdesc;
> +	char sdesc[sizeof(struct shash_desc) +
> +		crypto_shash_descsize(lmk->hash_tfm)] CRYPTO_MINALIGN_ATTR;
> +	struct shash_desc *desc = (struct shash_desc *)sdesc;

TBH, this looks even more uglier that the previous code :-)

(But tglx already complained on different patch and I fully agree that crypto code
should not use this kind of construction in the first place...
It would be very nice to introduce at least some macro hiding these crazy
stack allocations later.)

Thanks,
Milan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Behan Webster Sept. 7, 2014, 2:10 a.m. UTC | #2 
On 09/06/14 01:46, Milan Broz wrote:
> On 09/06/2014 01:02 AM, behanw@converseincode.com wrote:
>> From: Jan-Simon Möller <dl9pf@gmx.de>
>>
>> The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
>> precludes the use of compilers which don't implement VLAIS (for instance the
>> Clang compiler). This patch instead allocates the appropriate amount of memory
>> using an char array.
> Well, if clang (or C99 code) is now preferred for kernel, why not.
>
> Just please commit the patch series en bloc (together with the patches
> removing VLAIS from crypto code you posted to cryptoapi list).
They seemed more separate than that. However, happy to post them as a 
patch set.

>> -	struct {
>> -		struct shash_desc desc;
>> -		char ctx[crypto_shash_descsize(lmk->hash_tfm)];
>> -	} sdesc;
>> +	char sdesc[sizeof(struct shash_desc) +
>> +		crypto_shash_descsize(lmk->hash_tfm)] CRYPTO_MINALIGN_ATTR;
>> +	struct shash_desc *desc = (struct shash_desc *)sdesc;
> TBH, this looks even more uglier that the previous code :-)
I'm not claiming it's prettier. Merely C99. :)

> (But tglx already complained on different patch and I fully agree that crypto code
> should not use this kind of construction in the first place...
> It would be very nice to introduce at least some macro hiding these crazy
> stack allocations later.)
I actually agree with you and tglx. Will fix.

As I said to tglx, we were asked not to hide things with macro magic in 
some of our non-crypto VLAIS removal patches. Happy to use macros in 
this case.

Thanks,

Behan
diff mbox

Patch

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index cd15e08..ad696b8 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -526,29 +526,28 @@  static int crypt_iv_lmk_one(struct crypt_config *cc, u8 *iv,
 			    u8 *data)
 {
 	struct iv_lmk_private *lmk = &cc->iv_gen_private.lmk;
-	struct {
-		struct shash_desc desc;
-		char ctx[crypto_shash_descsize(lmk->hash_tfm)];
-	} sdesc;
+	char sdesc[sizeof(struct shash_desc) +
+		crypto_shash_descsize(lmk->hash_tfm)] CRYPTO_MINALIGN_ATTR;
+	struct shash_desc *desc = (struct shash_desc *)sdesc;
 	struct md5_state md5state;
 	__le32 buf[4];
 	int i, r;
 
-	sdesc.desc.tfm = lmk->hash_tfm;
-	sdesc.desc.flags = CRYPTO_TFM_REQ_MAY_SLEEP;
+	desc->tfm = lmk->hash_tfm;
+	desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
 
-	r = crypto_shash_init(&sdesc.desc);
+	r = crypto_shash_init(desc);
 	if (r)
 		return r;
 
 	if (lmk->seed) {
-		r = crypto_shash_update(&sdesc.desc, lmk->seed, LMK_SEED_SIZE);
+		r = crypto_shash_update(desc, lmk->seed, LMK_SEED_SIZE);
 		if (r)
 			return r;
 	}
 
 	/* Sector is always 512B, block size 16, add data of blocks 1-31 */
-	r = crypto_shash_update(&sdesc.desc, data + 16, 16 * 31);
+	r = crypto_shash_update(desc, data + 16, 16 * 31);
 	if (r)
 		return r;
 
@@ -557,12 +556,12 @@  static int crypt_iv_lmk_one(struct crypt_config *cc, u8 *iv,
 	buf[1] = cpu_to_le32((((u64)dmreq->iv_sector >> 32) & 0x00FFFFFF) | 0x80000000);
 	buf[2] = cpu_to_le32(4024);
 	buf[3] = 0;
-	r = crypto_shash_update(&sdesc.desc, (u8 *)buf, sizeof(buf));
+	r = crypto_shash_update(desc, (u8 *)buf, sizeof(buf));
 	if (r)
 		return r;
 
 	/* No MD5 padding here */
-	r = crypto_shash_export(&sdesc.desc, &md5state);
+	r = crypto_shash_export(desc, &md5state);
 	if (r)
 		return r;
 
@@ -679,10 +678,9 @@  static int crypt_iv_tcw_whitening(struct crypt_config *cc,
 	struct iv_tcw_private *tcw = &cc->iv_gen_private.tcw;
 	u64 sector = cpu_to_le64((u64)dmreq->iv_sector);
 	u8 buf[TCW_WHITENING_SIZE];
-	struct {
-		struct shash_desc desc;
-		char ctx[crypto_shash_descsize(tcw->crc32_tfm)];
-	} sdesc;
+	char sdesc[sizeof(struct shash_desc)
+		+ crypto_shash_descsize(tcw->crc32_tfm)] CRYPTO_MINALIGN_ATTR;
+	struct shash_desc *desc = (struct shash_desc *)sdesc;
 	int i, r;
 
 	/* xor whitening with sector number */
@@ -691,16 +689,16 @@  static int crypt_iv_tcw_whitening(struct crypt_config *cc,
 	crypto_xor(&buf[8], (u8 *)&sector, 8);
 
 	/* calculate crc32 for every 32bit part and xor it */
-	sdesc.desc.tfm = tcw->crc32_tfm;
-	sdesc.desc.flags = CRYPTO_TFM_REQ_MAY_SLEEP;
+	desc->tfm = tcw->crc32_tfm;
+	desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
 	for (i = 0; i < 4; i++) {
-		r = crypto_shash_init(&sdesc.desc);
+		r = crypto_shash_init(desc);
 		if (r)
 			goto out;
-		r = crypto_shash_update(&sdesc.desc, &buf[i * 4], 4);
+		r = crypto_shash_update(desc, &buf[i * 4], 4);
 		if (r)
 			goto out;
-		r = crypto_shash_final(&sdesc.desc, &buf[i * 4]);
+		r = crypto_shash_final(desc, &buf[i * 4]);
 		if (r)
 			goto out;
 	}