[v2] crypto: tcrypt - Fix memory leaks/crashes in multibuffer hash speed test
diff mbox series

Message ID 20160628123352.GA17844@gondor.apana.org.au
State New, archived
Headers show
Series
  • [v2] crypto: tcrypt - Fix memory leaks/crashes in multibuffer hash speed test
Related show

Commit Message

Herbert Xu June 28, 2016, 12:33 p.m. UTC 
On Tue, Jun 28, 2016 at 12:15:43PM +0200, Krzysztof Kozlowski wrote:
> Oops:

Thanks, there was a typo where it said k instead of j in the second
loop.

---8<---
This patch resolves a number of issues with the mb speed test
function:

* The tfm is never freed.
* Memory is allocated even when we're not using mb.
* When an error occurs we don't wait for completion for other requests.
* When an error occurs during allocation we may leak memory.
* The test function ignores plen but still runs for plen != blen.
* The backlog flag is incorrectly used (may crash).

This patch tries to resolve all these issues as well as making
the code consistent with the existing hash speed testing function.

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

Comments

Krzysztof Kozlowski June 29, 2016, 8:16 a.m. UTC | #1 
On 06/28/2016 02:33 PM, Herbert Xu wrote:
> On Tue, Jun 28, 2016 at 12:15:43PM +0200, Krzysztof Kozlowski wrote:
>> Oops:
> 
> Thanks, there was a typo where it said k instead of j in the second
> loop.
> 
> ---8<---
> This patch resolves a number of issues with the mb speed test
> function:
> 
> * The tfm is never freed.
> * Memory is allocated even when we're not using mb.
> * When an error occurs we don't wait for completion for other requests.
> * When an error occurs during allocation we may leak memory.
> * The test function ignores plen but still runs for plen != blen.
> * The backlog flag is incorrectly used (may crash).
> 
> This patch tries to resolve all these issues as well as making
> the code consistent with the existing hash speed testing function.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Seems to work fine except:
1. The updates are always 1.
2. For bigger blocks it reports always 1 or 3 cycles per byte:

[root@odroidxu3 ~]# modprobe tcrypt mode=423
[   70.129206]
[   70.129206] testing speed of multibuffer sha256 (sha256-neon)
[   70.135511] test  0 (   16 byte blocks,   16 bytes per update,   1 updates):   4597 cycles/operation,   35 cycles/byte
[   70.145826] test  2 (   64 byte blocks,   64 bytes per update,   1 updates):   5762 cycles/operation,   11 cycles/byte
[   70.156469] test  5 (  256 byte blocks,  256 bytes per update,   1 updates):  10687 cycles/operation,    5 cycles/byte
[   70.167125] test  8 ( 1024 byte blocks, 1024 bytes per update,   1 updates):  31485 cycles/operation,    3 cycles/byte
[   70.177919] test 12 ( 2048 byte blocks, 2048 bytes per update,   1 updates):  59281 cycles/operation,    3 cycles/byte
[   70.189780] test 16 ( 4096 byte blocks, 4096 bytes per update,   1 updates): 118097 cycles/operation,    3 cycles/byte
[   70.204117] test 21 ( 8192 byte blocks, 8192 bytes per update,   1 updates): 232309 cycles/operation,    3 cycles/byte

[root@odroidxu3 ~]# modprobe tcrypt mode=422
[   71.988248]
[   71.988248] testing speed of multibuffer sha1 (sha1-neon)
[   71.994097] test  0 (   16 byte blocks,   16 bytes per update,   1 updates):   2506 cycles/operation,   19 cycles/byte
[   72.004547] test  2 (   64 byte blocks,   64 bytes per update,   1 updates):   2299 cycles/operation,    4 cycles/byte
[   72.015152] test  5 (  256 byte blocks,  256 bytes per update,   1 updates):   3535 cycles/operation,    1 cycles/byte
[   72.025807] test  8 ( 1024 byte blocks, 1024 bytes per update,   1 updates):   9403 cycles/operation,    1 cycles/byte
[   72.036401] test 12 ( 2048 byte blocks, 2048 bytes per update,   1 updates):  17142 cycles/operation,    1 cycles/byte
[   72.047058] test 16 ( 4096 byte blocks, 4096 bytes per update,   1 updates):  33109 cycles/operation,    1 cycles/byte
[   72.057821] test 21 ( 8192 byte blocks, 8192 bytes per update,   1 updates):  67750 cycles/operation,    1 cycles/byte
modprobe: ERROR: could not insert 'tcrypt': Resource temporarily unavailable

Is it expected?

Best regards,
Krzysztof
Herbert Xu June 29, 2016, 8:19 a.m. UTC | #2 
On Wed, Jun 29, 2016 at 10:16:10AM +0200, Krzysztof Kozlowski wrote:
> 
> Seems to work fine except:
> 1. The updates are always 1.

Yes the test function only does digest so it's always one update.

> 2. For bigger blocks it reports always 1 or 3 cycles per byte:

Yes the average cycles per-byte should reach an asymptotic value.

Thanks,
Krzysztof Kozlowski June 29, 2016, 8:28 a.m. UTC | #3 
On 06/29/2016 10:19 AM, Herbert Xu wrote:
> On Wed, Jun 29, 2016 at 10:16:10AM +0200, Krzysztof Kozlowski wrote:
>>
>> Seems to work fine except:
>> 1. The updates are always 1.
> 
> Yes the test function only does digest so it's always one update.
> 
>> 2. For bigger blocks it reports always 1 or 3 cycles per byte:
> 
> Yes the average cycles per-byte should reach an asymptotic value.

Then:
Tested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof
Dey, Megha June 29, 2016, 5:45 p.m. UTC | #4 
I tested the latest cryptodev tree on my haswell machine and this is
what I see:
[   40.402834] modprobe tcrypt mode=422
[   40.403105] testing speed of multibuffer sha1 (sha1_mb)
[   40.403108] test  0 (   16 byte blocks,   16 bytes per update,   1
updates):  32271 cycles/operation,  252 cycles/byte
[   40.403118] At least one hashing failed ret=-115
[   43.218712] modprobe tcrypt mode=423
[   43.218712] testing speed of multibuffer sha256 (sha256_mb)
[   43.218715] test  0 (   16 byte blocks,   16 bytes per update,   1
updates): 106965 cycles/operation,  835 cycles/byte
[   43.218747] At least one hashing failed ret=-115
[   45.346657] modprobe tcrypt mode=424
[   45.346657] testing speed of multibuffer sha512 (sha512_mb)
[   45.346660] test  0 (   16 byte blocks,   16 bytes per update,   1
updates):  43179 cycles/operation,  337 cycles/byte
[   45.346673] At least one hashing failed ret=-115

Don't think this is expected, is it?

This is the patch which might have an issue?
72259deb3a9f2c07d18d71d7c9356754e7d88369

Thanks,
Megha

On Wed, 2016-06-29 at 10:28 +0200, Krzysztof Kozlowski wrote:
> On 06/29/2016 10:19 AM, Herbert Xu wrote:
> > On Wed, Jun 29, 2016 at 10:16:10AM +0200, Krzysztof Kozlowski wrote:
> >>
> >> Seems to work fine except:
> >> 1. The updates are always 1.
> > 
> > Yes the test function only does digest so it's always one update.
> > 
> >> 2. For bigger blocks it reports always 1 or 3 cycles per byte:
> > 
> > Yes the average cycles per-byte should reach an asymptotic value.
> 
> Then:
> Tested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> 
> Best regards,
> Krzysztof
>

Patch
diff mbox series 

diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
index 1537a1c..68064fc 100644
--- a/crypto/tcrypt.c
+++ b/crypto/tcrypt.c
@@ -409,54 +409,61 @@  static inline int do_one_ahash_op(struct ahash_request *req, int ret)
 	return ret;
 }
 
-char ptext[4096];
-struct scatterlist sg[8][8];
-char result[8][64];
-struct ahash_request *req[8];
-struct tcrypt_result tresult[8];
-char *xbuf[8][XBUFSIZE];
-unsigned long start[8], end[8], mid;
+struct test_mb_ahash_data {
+	struct scatterlist sg[TVMEMSIZE];
+	char result[64];
+	struct ahash_request *req;
+	struct tcrypt_result tresult;
+	char *xbuf[XBUFSIZE];
+};
 
 static void test_mb_ahash_speed(const char *algo, unsigned int sec,
-					struct hash_speed *speed)
+				struct hash_speed *speed)
 {
-	unsigned int i, j, k;
-	void *hash_buff;
-	int ret = -ENOMEM;
+	struct test_mb_ahash_data *data;
 	struct crypto_ahash *tfm;
+	unsigned long start, end;
 	unsigned long cycles;
+	unsigned int i, j, k;
+	int ret;
+
+	data = kzalloc(sizeof(*data) * 8, GFP_KERNEL);
+	if (!data)
+		return;
 
 	tfm = crypto_alloc_ahash(algo, 0, 0);
 	if (IS_ERR(tfm)) {
 		pr_err("failed to load transform for %s: %ld\n",
 			algo, PTR_ERR(tfm));
-		return;
+		goto free_data;
 	}
+
 	for (i = 0; i < 8; ++i) {
-		if (testmgr_alloc_buf(xbuf[i]))
-			goto out_nobuf;
+		if (testmgr_alloc_buf(data[i].xbuf))
+			goto out;
 
-		init_completion(&tresult[i].completion);
+		init_completion(&data[i].tresult.completion);
 
-		req[i] = ahash_request_alloc(tfm, GFP_KERNEL);
-		if (!req[i]) {
+		data[i].req = ahash_request_alloc(tfm, GFP_KERNEL);
+		if (!data[i].req) {
 			pr_err("alg: hash: Failed to allocate request for %s\n",
 			       algo);
-			goto out_noreq;
+			goto out;
 		}
-		ahash_request_set_callback(req[i], CRYPTO_TFM_REQ_MAY_BACKLOG,
-					   tcrypt_complete, &tresult[i]);
 
-		hash_buff = xbuf[i][0];
-		memcpy(hash_buff, ptext, 4096);
+		ahash_request_set_callback(data[i].req, 0,
+					   tcrypt_complete, &data[i].tresult);
+		test_hash_sg_init(data[i].sg);
 	}
 
-	j = 0;
-
-	pr_err("\ntesting speed of %s (%s)\n", algo,
-	       get_driver_name(crypto_ahash, tfm));
+	pr_info("\ntesting speed of multibuffer %s (%s)\n", algo,
+		get_driver_name(crypto_ahash, tfm));
 
 	for (i = 0; speed[i].blen != 0; i++) {
+		/* For some reason this only tests digests. */
+		if (speed[i].blen != speed[i].plen)
+			continue;
+
 		if (speed[i].blen > TVMEMSIZE * PAGE_SIZE) {
 			pr_err("template (%u) too big for tvmem (%lu)\n",
 			       speed[i].blen, TVMEMSIZE * PAGE_SIZE);
@@ -466,53 +473,59 @@  static void test_mb_ahash_speed(const char *algo, unsigned int sec,
 		if (speed[i].klen)
 			crypto_ahash_setkey(tfm, tvmem[0], speed[i].klen);
 
-		for (k = 0; k < 8; ++k) {
-			sg_init_one(&sg[k][0], (void *) xbuf[k][0],
-				    speed[i].blen);
-			ahash_request_set_crypt(req[k], sg[k],
-						result[k], speed[i].blen);
-		}
+		for (k = 0; k < 8; k++)
+			ahash_request_set_crypt(data[k].req, data[k].sg,
+						data[k].result, speed[i].blen);
 
-		pr_err("test%3u (%5u byte blocks,%5u bytes per update,%4u updates): ",
+		pr_info("test%3u "
+			"(%5u byte blocks,%5u bytes per update,%4u updates): ",
 			i, speed[i].blen, speed[i].plen,
 			speed[i].blen / speed[i].plen);
 
-		for (k = 0; k < 8; ++k) {
-			start[k] = get_cycles();
-			ret = crypto_ahash_digest(req[k]);
-			if (ret == -EBUSY || ret == -EINPROGRESS)
+		start = get_cycles();
+
+		for (k = 0; k < 8; k++) {
+			ret = crypto_ahash_digest(data[k].req);
+			if (ret == -EINPROGRESS)
 				continue;
-			if (ret) {
-				pr_err("alg (%s) something wrong, ret = %d ...\n",
-				       algo, ret);
-				goto out;
-			}
+
+			if (ret)
+				break;
+
+			complete(&data[k].tresult.completion);
+			data[k].tresult.err = 0;
 		}
-		mid = get_cycles();
 
-		for (k = 0; k < 8; ++k) {
-			struct tcrypt_result *tr = &tresult[k];
+		for (j = 0; j < k; j++) {
+			struct tcrypt_result *tr = &data[j].tresult;
 
-			ret = wait_for_completion_interruptible(&tr->completion);
-			if (ret)
-				pr_err("alg(%s): hash: digest failed\n", algo);
-			end[k] = get_cycles();
+			wait_for_completion(&tr->completion);
+			if (tr->err)
+				ret = tr->err;
 		}
 
-		cycles = end[7] - start[0];
-		printk("\nBlock: %6lu cycles (%4lu cycles/byte)\n",
-		       cycles, cycles / (8 * speed[i].blen));
+		end = get_cycles();
+		cycles = end - start;
+		pr_cont("%6lu cycles/operation, %4lu cycles/byte\n",
+			cycles, cycles / (8 * speed[i].blen));
+
+		if (ret) {
+			pr_err("At least one hashing failed ret=%d\n", ret);
+			break;
+		}
 	}
-	ret = 0;
 
 out:
 	for (k = 0; k < 8; ++k)
-		ahash_request_free(req[k]);
-out_noreq:
+		ahash_request_free(data[k].req);
+
 	for (k = 0; k < 8; ++k)
-		testmgr_free_buf(xbuf[k]);
-out_nobuf:
-	return;
+		testmgr_free_buf(data[k].xbuf);
+
+	crypto_free_ahash(tfm);
+
+free_data:
+	kfree(data);
 }
 
 static int test_ahash_jiffies_digest(struct ahash_request *req, int blen,