[PATCH 00/10] crypto: omap fixes towards 5.5

[PATCH 00/10] crypto: omap fixes towards 5.5

[Tero Kristo]

Hi,

This series fixes a number of bugs with omap crypto implementation.
These have become evident with the changes to the cryptomanager, where
it adds some new test cases and modifies some existing, namely the split
update tests. Also, while fixing the cryptomanager induced bugs, some
other surfaced with tcrypt/IPSec tests, so fixed them aswell.

Patch #9 is against crypto core modifying the crypto_wait_req
common API to have a timeout for it also, currently it waits forever
and it is kind of difficult to see what test fails with crypto manager.
This is not really needed for anything, but it is kind of nice to have
(makes debugging easier.)

This series has been tested on top of 5.4-rc2, with following setups,
on AM57xx-beagle-x15 board:

- crypto manager self tests
- tcrypt performance test
- ipsec test with strongswan

This series depends on the skcipher API switch patch from Ard Biesheuvel
[1].

-Tero

[1] https://patchwork.kernel.org/patch/11188595/


--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 01/10] crypto: omap-sham: split up data to multiple sg elements with huge data

[Tero Kristo]

When using huge data amount, allocating free pages fails as the kernel
isn't able to process get_free_page requests larger than MAX_ORDER.
Also, the DMA subsystem has an inherent limitation that data size
larger than some 2MB can't be handled properly. In these cases,
split up the data instead to smaller requests so that the kernel
can allocate the data, and also so that the DMA driver can handle
the separate SG elements.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
Tested-by: Bin Liu <b-liu@ti.com>
---
 drivers/crypto/omap-sham.c | 81 ++++++++++++++++++++++++++++++--------
 1 file changed, 64 insertions(+), 17 deletions(-)

diff --git a/drivers/crypto/omap-sham.c b/drivers/crypto/omap-sham.c
index ac80bc6af093..2e9435577cea 100644
--- a/drivers/crypto/omap-sham.c
+++ b/drivers/crypto/omap-sham.c
@@ -112,6 +112,8 @@
 #define FLAGS_BE32_SHA1		8
 #define FLAGS_SGS_COPIED	9
 #define FLAGS_SGS_ALLOCED	10
+#define FLAGS_HUGE		11
+
 /* context flags */
 #define FLAGS_FINUP		16
 
@@ -136,6 +138,8 @@
 #define BUFLEN			SHA512_BLOCK_SIZE
 #define OMAP_SHA_DMA_THRESHOLD	256
 
+#define OMAP_SHA_MAX_DMA_LEN	(1024 * 2048)
+
 struct omap_sham_dev;
 
 struct omap_sham_reqctx {
@@ -689,21 +693,20 @@ static int omap_sham_copy_sg_lists(struct omap_sham_reqctx *ctx,
 
 	set_bit(FLAGS_SGS_ALLOCED, &ctx->dd->flags);
 
+	ctx->offset += new_len - ctx->bufcnt;
 	ctx->bufcnt = 0;
 
 	return 0;
 }
 
 static int omap_sham_copy_sgs(struct omap_sham_reqctx *ctx,
-			      struct scatterlist *sg, int bs, int new_len)
+			      struct scatterlist *sg, int bs,
+			      unsigned int new_len)
 {
 	int pages;
 	void *buf;
-	int len;
-
-	len = new_len + ctx->bufcnt;
 
-	pages = get_order(ctx->total);
+	pages = get_order(new_len);
 
 	buf = (void *)__get_free_pages(GFP_ATOMIC, pages);
 	if (!buf) {
@@ -715,14 +718,14 @@ static int omap_sham_copy_sgs(struct omap_sham_reqctx *ctx,
 		memcpy(buf, ctx->dd->xmit_buf, ctx->bufcnt);
 
 	scatterwalk_map_and_copy(buf + ctx->bufcnt, sg, ctx->offset,
-				 ctx->total - ctx->bufcnt, 0);
+				 min(new_len, ctx->total) - ctx->bufcnt, 0);
 	sg_init_table(ctx->sgl, 1);
-	sg_set_buf(ctx->sgl, buf, len);
+	sg_set_buf(ctx->sgl, buf, new_len);
 	ctx->sg = ctx->sgl;
 	set_bit(FLAGS_SGS_COPIED, &ctx->dd->flags);
 	ctx->sg_len = 1;
+	ctx->offset += new_len - ctx->bufcnt;
 	ctx->bufcnt = 0;
-	ctx->offset = 0;
 
 	return 0;
 }
@@ -741,7 +744,7 @@ static int omap_sham_align_sgs(struct scatterlist *sg,
 	if (!sg || !sg->length || !nbytes)
 		return 0;
 
-	new_len = nbytes;
+	new_len = nbytes - offset;
 
 	if (offset)
 		list_ok = false;
@@ -751,6 +754,9 @@ static int omap_sham_align_sgs(struct scatterlist *sg,
 	else
 		new_len = (new_len - 1) / bs * bs;
 
+	if (!new_len)
+		return 0;
+
 	if (nbytes != new_len)
 		list_ok = false;
 
@@ -794,10 +800,17 @@ static int omap_sham_align_sgs(struct scatterlist *sg,
 		}
 	}
 
+	if (new_len > OMAP_SHA_MAX_DMA_LEN) {
+		new_len = OMAP_SHA_MAX_DMA_LEN;
+		aligned = false;
+	}
+
 	if (!aligned)
 		return omap_sham_copy_sgs(rctx, sg, bs, new_len);
 	else if (!list_ok)
 		return omap_sham_copy_sg_lists(rctx, sg, bs, new_len);
+	else
+		rctx->offset += new_len;
 
 	rctx->sg_len = n;
 	rctx->sg = sg;
@@ -821,7 +834,12 @@ static int omap_sham_prepare_request(struct ahash_request *req, bool update)
 	else
 		nbytes = 0;
 
-	rctx->total = nbytes + rctx->bufcnt;
+	rctx->total = nbytes + rctx->bufcnt - rctx->offset;
+
+	dev_dbg(rctx->dd->dev,
+		"%s: nbytes=%d, bs=%d, total=%d, offset=%d, bufcnt=%d\n",
+		__func__, nbytes, bs, rctx->total, rctx->offset,
+		rctx->bufcnt);
 
 	if (!rctx->total)
 		return 0;
@@ -847,12 +865,15 @@ static int omap_sham_prepare_request(struct ahash_request *req, bool update)
 
 	xmit_len = rctx->total;
 
+	if (xmit_len > OMAP_SHA_MAX_DMA_LEN)
+		xmit_len = OMAP_SHA_MAX_DMA_LEN;
+
 	if (!IS_ALIGNED(xmit_len, bs)) {
 		if (final)
 			xmit_len = DIV_ROUND_UP(xmit_len, bs) * bs;
 		else
 			xmit_len = xmit_len / bs * bs;
-	} else if (!final) {
+	} else if (!final && rctx->total == xmit_len) {
 		xmit_len -= bs;
 	}
 
@@ -880,7 +901,7 @@ static int omap_sham_prepare_request(struct ahash_request *req, bool update)
 		rctx->sg_len = 1;
 	}
 
-	if (hash_later) {
+	if (hash_later && hash_later <= rctx->buflen) {
 		int offset = 0;
 
 		if (hash_later > req->nbytes) {
@@ -901,6 +922,9 @@ static int omap_sham_prepare_request(struct ahash_request *req, bool update)
 		rctx->bufcnt = 0;
 	}
 
+	if (hash_later > rctx->buflen)
+		set_bit(FLAGS_HUGE, &rctx->dd->flags);
+
 	if (!final)
 		rctx->total = xmit_len;
 
@@ -998,10 +1022,11 @@ static int omap_sham_update_req(struct omap_sham_dev *dd)
 	struct ahash_request *req = dd->req;
 	struct omap_sham_reqctx *ctx = ahash_request_ctx(req);
 	int err;
-	bool final = ctx->flags & BIT(FLAGS_FINUP);
+	bool final = (ctx->flags & BIT(FLAGS_FINUP)) &&
+			!(dd->flags & BIT(FLAGS_HUGE));
 
-	dev_dbg(dd->dev, "update_req: total: %u, digcnt: %d, finup: %d\n",
-		 ctx->total, ctx->digcnt, (ctx->flags & BIT(FLAGS_FINUP)) != 0);
+	dev_dbg(dd->dev, "update_req: total: %u, digcnt: %d, final: %d",
+		ctx->total, ctx->digcnt, final);
 
 	if (ctx->total < get_block_size(ctx) ||
 	    ctx->total < dd->fallback_sz)
@@ -1024,6 +1049,9 @@ static int omap_sham_final_req(struct omap_sham_dev *dd)
 	struct omap_sham_reqctx *ctx = ahash_request_ctx(req);
 	int err = 0, use_dma = 1;
 
+	if (dd->flags & BIT(FLAGS_HUGE))
+		return 0;
+
 	if ((ctx->total <= get_block_size(ctx)) || dd->polling_mode)
 		/*
 		 * faster to handle last block with cpu or
@@ -1083,7 +1111,7 @@ static void omap_sham_finish_req(struct ahash_request *req, int err)
 
 	if (test_bit(FLAGS_SGS_COPIED, &dd->flags))
 		free_pages((unsigned long)sg_virt(ctx->sg),
-			   get_order(ctx->sg->length + ctx->bufcnt));
+			   get_order(ctx->sg->length));
 
 	if (test_bit(FLAGS_SGS_ALLOCED, &dd->flags))
 		kfree(ctx->sg);
@@ -1092,6 +1120,21 @@ static void omap_sham_finish_req(struct ahash_request *req, int err)
 
 	dd->flags &= ~(BIT(FLAGS_SGS_ALLOCED) | BIT(FLAGS_SGS_COPIED));
 
+	if (dd->flags & BIT(FLAGS_HUGE)) {
+		dd->flags &= ~(BIT(FLAGS_CPU) | BIT(FLAGS_DMA_READY) |
+				BIT(FLAGS_OUTPUT_READY) | BIT(FLAGS_HUGE));
+		omap_sham_prepare_request(req, ctx->op == OP_UPDATE);
+		if (ctx->op == OP_UPDATE || (dd->flags & BIT(FLAGS_HUGE))) {
+			err = omap_sham_update_req(dd);
+			if (err != -EINPROGRESS &&
+			    (ctx->flags & BIT(FLAGS_FINUP)))
+				err = omap_sham_final_req(dd);
+		} else if (ctx->op == OP_FINAL) {
+			omap_sham_final_req(dd);
+		}
+		return;
+	}
+
 	if (!err) {
 		dd->pdata->copy_hash(req, 1);
 		if (test_bit(FLAGS_FINAL, &dd->flags))
@@ -1107,6 +1150,8 @@ static void omap_sham_finish_req(struct ahash_request *req, int err)
 	pm_runtime_mark_last_busy(dd->dev);
 	pm_runtime_put_autosuspend(dd->dev);
 
+	ctx->offset = 0;
+
 	if (req->base.complete)
 		req->base.complete(&req->base, err);
 }
@@ -1158,7 +1203,7 @@ static int omap_sham_handle_queue(struct omap_sham_dev *dd,
 		/* request has changed - restore hash */
 		dd->pdata->copy_hash(req, 0);
 
-	if (ctx->op == OP_UPDATE) {
+	if (ctx->op == OP_UPDATE || (dd->flags & BIT(FLAGS_HUGE))) {
 		err = omap_sham_update_req(dd);
 		if (err != -EINPROGRESS && (ctx->flags & BIT(FLAGS_FINUP)))
 			/* no final() after finup() */
@@ -1730,6 +1775,8 @@ static void omap_sham_done_task(unsigned long data)
 	struct omap_sham_dev *dd = (struct omap_sham_dev *)data;
 	int err = 0;
 
+	dev_dbg(dd->dev, "%s: flags=%lx\n", __func__, dd->flags);
+
 	if (!test_bit(FLAGS_BUSY, &dd->flags)) {
 		omap_sham_handle_queue(dd, NULL);
 		return;
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 02/10] crypto: omap-sham: remove the sysfs group during driver removal

[Tero Kristo]

The driver removal should also cleanup the created sysfs group. If not,
the driver fails the subsequent probe as the files exist already.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/crypto/omap-sham.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/crypto/omap-sham.c b/drivers/crypto/omap-sham.c
index 2e9435577cea..0bf07a7c060b 100644
--- a/drivers/crypto/omap-sham.c
+++ b/drivers/crypto/omap-sham.c
@@ -2270,6 +2270,8 @@ static int omap_sham_remove(struct platform_device *pdev)
 	if (!dd->polling_mode)
 		dma_release_channel(dd->dma_lch);
 
+	sysfs_remove_group(&dd->dev->kobj, &omap_sham_attr_group);
+
 	return 0;
 }
 
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 03/10] crypto: omap-aes: remove the sysfs group during driver removal

[Tero Kristo]

The driver removal should also cleanup the created sysfs group. If not,
the driver fails the subsequent probe as the files exist already. Also,
drop a completely unnecessary pointer assignment from the removal
function at the same time.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/crypto/omap-aes.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/omap-aes.c b/drivers/crypto/omap-aes.c
index a1fc03ed01f3..38c750e83dbe 100644
--- a/drivers/crypto/omap-aes.c
+++ b/drivers/crypto/omap-aes.c
@@ -1296,7 +1296,8 @@ static int omap_aes_remove(struct platform_device *pdev)
 	tasklet_kill(&dd->done_task);
 	omap_aes_dma_cleanup(dd);
 	pm_runtime_disable(dd->dev);
-	dd = NULL;
+
+	sysfs_remove_group(&dd->dev->kobj, &omap_aes_attr_group);
 
 	return 0;
 }
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 04/10] crypto: omap-des: add IV output handling

[Tero Kristo]

Currently omap-des driver does not copy end result IV out at all. This
is evident with the additional checks done at the crypto test manager.
Fix by copying out the IV values from HW.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/crypto/omap-des.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/crypto/omap-des.c b/drivers/crypto/omap-des.c
index 4c4dbc2b377e..ea82d55ea8c3 100644
--- a/drivers/crypto/omap-des.c
+++ b/drivers/crypto/omap-des.c
@@ -597,6 +597,7 @@ static int omap_des_crypt_req(struct crypto_engine *engine,
 static void omap_des_done_task(unsigned long data)
 {
 	struct omap_des_dev *dd = (struct omap_des_dev *)data;
+	int i;
 
 	pr_debug("enter done_task\n");
 
@@ -615,6 +616,11 @@ static void omap_des_done_task(unsigned long data)
 	omap_crypto_cleanup(&dd->out_sgl, dd->orig_out, 0, dd->total_save,
 			    FLAGS_OUT_DATA_ST_SHIFT, dd->flags);
 
+	if ((dd->flags & FLAGS_CBC) && dd->req->iv)
+		for (i = 0; i < 2; i++)
+			((u32 *)dd->req->iv)[i] =
+				omap_des_read(dd, DES_REG_IV(dd, i));
+
 	omap_des_finish_req(dd, 0);
 
 	pr_debug("exit\n");
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 05/10] crypto: omap-aes: add IV output handling

[Tero Kristo]

Currently omap-aes driver does not copy end result IV out at all. This
is evident with the additional checks done at the crypto test manager.
Fix by copying out the IV values from HW.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/crypto/omap-aes.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/crypto/omap-aes.c b/drivers/crypto/omap-aes.c
index 38c750e83dbe..c40876353b19 100644
--- a/drivers/crypto/omap-aes.c
+++ b/drivers/crypto/omap-aes.c
@@ -479,6 +479,14 @@ static int omap_aes_crypt_req(struct crypto_engine *engine,
 	return omap_aes_crypt_dma_start(dd);
 }
 
+static void omap_aes_copy_ivout(struct omap_aes_dev *dd, u8 *ivbuf)
+{
+	int i;
+
+	for (i = 0; i < 4; i++)
+		((u32 *)ivbuf)[i] = omap_aes_read(dd, AES_REG_IV(dd, i));
+}
+
 static void omap_aes_done_task(unsigned long data)
 {
 	struct omap_aes_dev *dd = (struct omap_aes_dev *)data;
@@ -500,6 +508,10 @@ static void omap_aes_done_task(unsigned long data)
 	omap_crypto_cleanup(&dd->out_sgl, dd->orig_out, 0, dd->total_save,
 			    FLAGS_OUT_DATA_ST_SHIFT, dd->flags);
 
+	/* Update IV output */
+	if (dd->flags & (FLAGS_CBC | FLAGS_CTR))
+		omap_aes_copy_ivout(dd, dd->req->iv);
+
 	omap_aes_finish_req(dd, 0);
 
 	pr_debug("exit\n");
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 06/10] crypto: omap-sham: fix buffer handling for split test cases

[Tero Kristo]

Current buffer handling logic fails in a case where the buffer contains
existing data from previous update which is divisible by block size.
This results in a block size of data to be left missing from the sg
list going out to the hw accelerator, ending up in stalling the
crypto accelerator driver (the last request never completes fully due
to missing data.)

Fix this by passing the total size of the data instead of the data size
of current request, and also parsing the buffer contents within the
prepare request handling.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/crypto/omap-sham.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/omap-sham.c b/drivers/crypto/omap-sham.c
index 0bf07a7c060b..e71cd977b621 100644
--- a/drivers/crypto/omap-sham.c
+++ b/drivers/crypto/omap-sham.c
@@ -740,11 +740,12 @@ static int omap_sham_align_sgs(struct scatterlist *sg,
 	struct scatterlist *sg_tmp = sg;
 	int new_len;
 	int offset = rctx->offset;
+	int bufcnt = rctx->bufcnt;
 
 	if (!sg || !sg->length || !nbytes)
 		return 0;
 
-	new_len = nbytes - offset;
+	new_len = nbytes;
 
 	if (offset)
 		list_ok = false;
@@ -763,6 +764,16 @@ static int omap_sham_align_sgs(struct scatterlist *sg,
 	while (nbytes > 0 && sg_tmp) {
 		n++;
 
+		if (bufcnt) {
+			if (!IS_ALIGNED(bufcnt, bs)) {
+				aligned = false;
+				break;
+			}
+			nbytes -= bufcnt;
+			bufcnt = 0;
+			continue;
+		}
+
 #ifdef CONFIG_ZONE_DMA
 		if (page_zonenum(sg_page(sg_tmp)) != ZONE_DMA) {
 			aligned = false;
@@ -859,7 +870,7 @@ static int omap_sham_prepare_request(struct ahash_request *req, bool update)
 	if (rctx->bufcnt)
 		memcpy(rctx->dd->xmit_buf, rctx->buffer, rctx->bufcnt);
 
-	ret = omap_sham_align_sgs(req->src, nbytes, bs, final, rctx);
+	ret = omap_sham_align_sgs(req->src, rctx->total, bs, final, rctx);
 	if (ret)
 		return ret;
 
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 07/10] crypto: omap-aes-gcm: fix corner case with only auth data

[Tero Kristo]

Fix a corner case where only authdata is generated, without any provided
assocdata / cryptdata. Passing the empty scatterlists to OMAP AES core driver
in this case would confuse it, failing to map DMAs.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/crypto/omap-aes-gcm.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/crypto/omap-aes-gcm.c b/drivers/crypto/omap-aes-gcm.c
index 9bbedbccfadf..dfd4d1cac421 100644
--- a/drivers/crypto/omap-aes-gcm.c
+++ b/drivers/crypto/omap-aes-gcm.c
@@ -148,12 +148,14 @@ static int omap_aes_gcm_copy_buffers(struct omap_aes_dev *dd,
 	if (req->src == req->dst || dd->out_sg == sg_arr)
 		flags |= OMAP_CRYPTO_FORCE_COPY;
 
-	ret = omap_crypto_align_sg(&dd->out_sg, cryptlen,
-				   AES_BLOCK_SIZE, &dd->out_sgl,
-				   flags,
-				   FLAGS_OUT_DATA_ST_SHIFT, &dd->flags);
-	if (ret)
-		return ret;
+	if (cryptlen) {
+		ret = omap_crypto_align_sg(&dd->out_sg, cryptlen,
+					   AES_BLOCK_SIZE, &dd->out_sgl,
+					   flags,
+					   FLAGS_OUT_DATA_ST_SHIFT, &dd->flags);
+		if (ret)
+			return ret;
+	}
 
 	dd->in_sg_len = sg_nents_for_len(dd->in_sg, alen + clen);
 	dd->out_sg_len = sg_nents_for_len(dd->out_sg, clen);
@@ -287,8 +289,12 @@ static int omap_aes_gcm_handle_queue(struct omap_aes_dev *dd,
 		return err;
 
 	err = omap_aes_write_ctrl(dd);
-	if (!err)
-		err = omap_aes_crypt_dma_start(dd);
+	if (!err) {
+		if (dd->in_sg_len && dd->out_sg_len)
+			err = omap_aes_crypt_dma_start(dd);
+		else
+			omap_aes_gcm_dma_out_callback(dd);
+	}
 
 	if (err) {
 		omap_aes_gcm_finish_req(dd, err);
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 08/10] crypto: omap-sham: fix split update cases with cryptomgr tests

[Tero Kristo]

The updated crypto manager finds a couple of new bugs from the omap-sham
driver. Basically the split update cases fail to calculate the amount of
data to be sent properly, leading into failed results and hangs with the
hw accelerator.

To fix these, the buffer handling needs to be fixed, but we do some cleanup
for the code at the same time to cut away some unnecessary code so that
it is easier to fix.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/crypto/omap-sham.c | 102 ++++++++++++-------------------------
 1 file changed, 33 insertions(+), 69 deletions(-)

diff --git a/drivers/crypto/omap-sham.c b/drivers/crypto/omap-sham.c
index e71cd977b621..33a58ebf652c 100644
--- a/drivers/crypto/omap-sham.c
+++ b/drivers/crypto/omap-sham.c
@@ -648,6 +648,8 @@ static int omap_sham_copy_sg_lists(struct omap_sham_reqctx *ctx,
 	struct scatterlist *tmp;
 	int offset = ctx->offset;
 
+	ctx->total = new_len;
+
 	if (ctx->bufcnt)
 		n++;
 
@@ -665,6 +667,7 @@ static int omap_sham_copy_sg_lists(struct omap_sham_reqctx *ctx,
 		sg_set_buf(tmp, ctx->dd->xmit_buf, ctx->bufcnt);
 		tmp = sg_next(tmp);
 		ctx->sg_len++;
+		new_len -= ctx->bufcnt;
 	}
 
 	while (sg && new_len) {
@@ -682,15 +685,18 @@ static int omap_sham_copy_sg_lists(struct omap_sham_reqctx *ctx,
 		if (len > 0) {
 			new_len -= len;
 			sg_set_page(tmp, sg_page(sg), len, sg->offset);
+			ctx->sg_len++;
 			if (new_len <= 0)
-				sg_mark_end(tmp);
+				break;
 			tmp = sg_next(tmp);
-			ctx->sg_len++;
 		}
 
 		sg = sg_next(sg);
 	}
 
+	if (tmp)
+		sg_mark_end(tmp);
+
 	set_bit(FLAGS_SGS_ALLOCED, &ctx->dd->flags);
 
 	ctx->offset += new_len - ctx->bufcnt;
@@ -726,6 +732,7 @@ static int omap_sham_copy_sgs(struct omap_sham_reqctx *ctx,
 	ctx->sg_len = 1;
 	ctx->offset += new_len - ctx->bufcnt;
 	ctx->bufcnt = 0;
+	ctx->total = new_len;
 
 	return 0;
 }
@@ -771,6 +778,9 @@ static int omap_sham_align_sgs(struct scatterlist *sg,
 			}
 			nbytes -= bufcnt;
 			bufcnt = 0;
+			if (!nbytes)
+				list_ok = false;
+
 			continue;
 		}
 
@@ -820,9 +830,9 @@ static int omap_sham_align_sgs(struct scatterlist *sg,
 		return omap_sham_copy_sgs(rctx, sg, bs, new_len);
 	else if (!list_ok)
 		return omap_sham_copy_sg_lists(rctx, sg, bs, new_len);
-	else
-		rctx->offset += new_len;
 
+	rctx->total = new_len;
+	rctx->offset += new_len;
 	rctx->sg_len = n;
 	rctx->sg = sg;
 
@@ -834,99 +844,54 @@ static int omap_sham_prepare_request(struct ahash_request *req, bool update)
 	struct omap_sham_reqctx *rctx = ahash_request_ctx(req);
 	int bs;
 	int ret;
-	int nbytes;
+	unsigned int nbytes;
 	bool final = rctx->flags & BIT(FLAGS_FINUP);
-	int xmit_len, hash_later;
+	int hash_later;
 
 	bs = get_block_size(rctx);
 
-	if (update)
-		nbytes = req->nbytes;
-	else
-		nbytes = 0;
+	nbytes = rctx->bufcnt;
 
-	rctx->total = nbytes + rctx->bufcnt - rctx->offset;
+	if (update)
+		nbytes += req->nbytes - rctx->offset;
 
 	dev_dbg(rctx->dd->dev,
 		"%s: nbytes=%d, bs=%d, total=%d, offset=%d, bufcnt=%d\n",
 		__func__, nbytes, bs, rctx->total, rctx->offset,
 		rctx->bufcnt);
 
-	if (!rctx->total)
+	if (!nbytes)
 		return 0;
 
-	if (nbytes && (!IS_ALIGNED(rctx->bufcnt, bs))) {
+	rctx->total = nbytes;
+
+	if (update && req->nbytes && (!IS_ALIGNED(rctx->bufcnt, bs))) {
 		int len = bs - rctx->bufcnt % bs;
 
-		if (len > nbytes)
-			len = nbytes;
+		if (len > req->nbytes)
+			len = req->nbytes;
 		scatterwalk_map_and_copy(rctx->buffer + rctx->bufcnt, req->src,
 					 0, len, 0);
 		rctx->bufcnt += len;
-		nbytes -= len;
 		rctx->offset = len;
 	}
 
 	if (rctx->bufcnt)
 		memcpy(rctx->dd->xmit_buf, rctx->buffer, rctx->bufcnt);
 
-	ret = omap_sham_align_sgs(req->src, rctx->total, bs, final, rctx);
+	ret = omap_sham_align_sgs(req->src, nbytes, bs, final, rctx);
 	if (ret)
 		return ret;
 
-	xmit_len = rctx->total;
-
-	if (xmit_len > OMAP_SHA_MAX_DMA_LEN)
-		xmit_len = OMAP_SHA_MAX_DMA_LEN;
-
-	if (!IS_ALIGNED(xmit_len, bs)) {
-		if (final)
-			xmit_len = DIV_ROUND_UP(xmit_len, bs) * bs;
-		else
-			xmit_len = xmit_len / bs * bs;
-	} else if (!final && rctx->total == xmit_len) {
-		xmit_len -= bs;
-	}
-
-	hash_later = rctx->total - xmit_len;
+	hash_later = nbytes - rctx->total;
 	if (hash_later < 0)
 		hash_later = 0;
 
-	if (rctx->bufcnt && nbytes) {
-		/* have data from previous operation and current */
-		sg_init_table(rctx->sgl, 2);
-		sg_set_buf(rctx->sgl, rctx->dd->xmit_buf, rctx->bufcnt);
-
-		sg_chain(rctx->sgl, 2, req->src);
-
-		rctx->sg = rctx->sgl;
-
-		rctx->sg_len++;
-	} else if (rctx->bufcnt) {
-		/* have buffered data only */
-		sg_init_table(rctx->sgl, 1);
-		sg_set_buf(rctx->sgl, rctx->dd->xmit_buf, xmit_len);
-
-		rctx->sg = rctx->sgl;
-
-		rctx->sg_len = 1;
-	}
-
-	if (hash_later && hash_later <= rctx->buflen) {
-		int offset = 0;
-
-		if (hash_later > req->nbytes) {
-			memcpy(rctx->buffer, rctx->buffer + xmit_len,
-			       hash_later - req->nbytes);
-			offset = hash_later - req->nbytes;
-		}
-
-		if (req->nbytes) {
-			scatterwalk_map_and_copy(rctx->buffer + offset,
-						 req->src,
-						 offset + req->nbytes -
-						 hash_later, hash_later, 0);
-		}
+	if (hash_later) {
+		scatterwalk_map_and_copy(rctx->buffer,
+					 req->src,
+					 req->nbytes - hash_later,
+					 hash_later, 0);
 
 		rctx->bufcnt = hash_later;
 	} else {
@@ -936,8 +901,7 @@ static int omap_sham_prepare_request(struct ahash_request *req, bool update)
 	if (hash_later > rctx->buflen)
 		set_bit(FLAGS_HUGE, &rctx->dd->flags);
 
-	if (!final)
-		rctx->total = xmit_len;
+	rctx->total = min(nbytes, rctx->total);
 
 	return 0;
 }
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 09/10] crypto: add timeout to crypto_wait_req

[Tero Kristo]

Currently crypto_wait_req waits indefinitely for an async crypto request
to complete. This is bad as it can cause for example the crypto test
manager to hang without any notification as to why it has happened.
Instead of waiting indefinitely, add a 1 second timeout to the call,
and provide a warning print if a timeout happens.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 include/linux/crypto.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/linux/crypto.h b/include/linux/crypto.h
index 19ea3a371d7b..b8f0e5c3cc0c 100644
--- a/include/linux/crypto.h
+++ b/include/linux/crypto.h
@@ -682,8 +682,15 @@ static inline int crypto_wait_req(int err, struct crypto_wait *wait)
 	switch (err) {
 	case -EINPROGRESS:
 	case -EBUSY:
-		wait_for_completion(&wait->completion);
+		err = wait_for_completion_timeout(&wait->completion,
+						  msecs_to_jiffies(1000));
 		reinit_completion(&wait->completion);
+		if (!err) {
+			pr_err("%s: timeout for %p\n", __func__, wait);
+			err = -ETIMEDOUT;
+			break;
+		}
+
 		err = wait->err;
 		break;
 	};
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 10/10] crypto: omap-aes: fixup aligned data cleanup

[Tero Kristo]

Aligned data cleanup is using wrong pointers in the cleanup calls. Most
of the time these are right, but can cause mysterious problems in some
cases. Fix to use the same pointers that were used with the align call.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/crypto/omap-aes.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/omap-aes.c b/drivers/crypto/omap-aes.c
index c40876353b19..649abbc92fd4 100644
--- a/drivers/crypto/omap-aes.c
+++ b/drivers/crypto/omap-aes.c
@@ -502,10 +502,10 @@ static void omap_aes_done_task(unsigned long data)
 		omap_aes_crypt_dma_stop(dd);
 	}
 
-	omap_crypto_cleanup(dd->in_sgl, NULL, 0, dd->total_save,
+	omap_crypto_cleanup(dd->in_sg, NULL, 0, dd->total_save,
 			    FLAGS_IN_DATA_ST_SHIFT, dd->flags);
 
-	omap_crypto_cleanup(&dd->out_sgl, dd->orig_out, 0, dd->total_save,
+	omap_crypto_cleanup(dd->out_sg, dd->orig_out, 0, dd->total_save,
 			    FLAGS_OUT_DATA_ST_SHIFT, dd->flags);
 
 	/* Update IV output */
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 00/10] crypto: omap fixes towards 5.5

[Ard Biesheuvel]

On Thu, 17 Oct 2019 at 14:26, Tero Kristo <t-kristo@ti.com> wrote:
>
> Hi,
>
> This series fixes a number of bugs with omap crypto implementation.
> These have become evident with the changes to the cryptomanager, where
> it adds some new test cases and modifies some existing, namely the split
> update tests. Also, while fixing the cryptomanager induced bugs, some
> other surfaced with tcrypt/IPSec tests, so fixed them aswell.
>
> Patch #9 is against crypto core modifying the crypto_wait_req
> common API to have a timeout for it also, currently it waits forever
> and it is kind of difficult to see what test fails with crypto manager.
> This is not really needed for anything, but it is kind of nice to have
> (makes debugging easier.)
>
> This series has been tested on top of 5.4-rc2, with following setups,
> on AM57xx-beagle-x15 board:
>
> - crypto manager self tests
> - tcrypt performance test
> - ipsec test with strongswan
>
> This series depends on the skcipher API switch patch from Ard Biesheuvel
> [1].
>

Hi Tero,

On my BeagleBone White, I am hitting the following issues after
applying these patches:

[    7.493903] alg: skcipher: ecb-aes-omap encryption unexpectedly
succeeded on test vector "random: len=531 klen=32";
expected_error=-22, cfg="random: inplace may_sleep use_finup
src_divs=[44.72%@+4028, <flush>14.70%@alignmask+3, 19.45%@+4070,
21.13%@+2728]"
[    7.651103] alg: skcipher: cbc-aes-omap encryption unexpectedly
succeeded on test vector "random: len=1118 klen=32";
expected_error=-22, cfg="random: may_sleep use_final
src_divs=[<reimport>41.87%@+31, <flush>58.13%@+2510]"

These are simply a result of the ECB and CBC implementations not
returning -EINVAL when the input is not a multiple of the block size.

[    7.845527] alg: skcipher: blocksize for ctr-aes-omap (16) doesn't
match generic impl (1)

This means cra_blocksize is not set to 1 as it should. If your driver
uses the skcipher walk API, it should set the walksize to
AES_BLOCK_SIZE to ensure that the input is handled correctly. If you
don't, then you can disregard that part.

[    8.306491] alg: aead: gcm-aes-omap setauthsize unexpectedly
succeeded on test vector "random: alen=3 plen=31 authsize=6 klen=9";
expected_error=-22

Another missing sanity check. GCM only permits certain authsizes.

[    9.074703] omap_crypto_copy_sgs: Couldn't allocate pages for
unaligned cases.

This is not a bug, but I'm not sure if the below is related or not.

I'll preserve the binaries, in case you need me to objdump anything.

-- 
Ard.



[    9.082178] 8<--- cut here ---
[    9.085258] Unable to handle kernel NULL pointer dereference at
virtual address 00000008
[    9.093442] pgd = (ptrval)
[    9.096165] [00000008] *pgd=00000000
[    9.099773] Internal error: Oops: 5 [#1] SMP ARM
[    9.104415] Modules linked in:
[    9.107494] CPU: 0 PID: 134 Comm: cryptomgr_test Tainted: G
W         5.4.0-rc1-00140-g2e186dcd60ce #17
[    9.117539] Hardware name: Generic AM33XX (Flattened Device Tree)
[    9.123676] PC is at scatterwalk_ffwd+0x24/0xd4
[    9.128232] LR is at scatterwalk_ffwd+0x3c/0xd4
[    9.132785] pc : [<c0494a68>]    lr : [<c0494a80>]    psr: a0070013
[    9.139082] sp : cce839a8  ip : cce79b90  fp : cce75eb0
[    9.144331] r10: cce75e58  r9 : c0d05348  r8 : cce839dc
[    9.149582] r7 : 00000b8b  r6 : cce839dc  r5 : 00000000  r4 : fffff45d
[    9.156142] r3 : cfd820c2  r2 : fffffff8  r1 : cce77420  r0 : 00000000
[    9.162705] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
[    9.169875] Control: 10c5387d  Table: 8cec0019  DAC: 00000051
[    9.175648] Process cryptomgr_test (pid: 134, stack limit = 0x(ptrval))
[    9.182294] Stack: (0xcce839a8 to 0xcce84000)
[    9.186679] 39a0:                   cce75e40 cce73c80 ffffff8d
c0739198 0000000d 00000008
[    9.194904] 39c0: cce75e58 cce73cd8 00000010 cce839dc 00000000
00000b90 cce75ec4 00000000
[    9.203127] 39e0: cce6c480 00000000 00000000 d72ad1e4 00000000
cce62f80 a0070013 c073948c
[    9.211351] 3a00: ccc2b400 d72ad1e4 cce83a70 cce62f80 cce73c80
00000b83 ccc2b400 cce73cd8
[    9.219575] 3a20: cce83a70 c0d05348 cce83a5c c07394c8 0eec3000
d72ad1e4 00000080 cce73cc8
[    9.227800] 3a40: 00000019 00000010 cfd8182e 00000cc8 00000010
00000000 00000000 cfd8182e
[    9.236023] 3a60: 00000cd8 00000010 00000000 00000000 00000000
00000000 cce83a78 cce83a78
[    9.244247] 3a80: cce77400 d72ad1e4 0000000f cce83c74 cce73c80
cce77400 c0491fd4 cce83ae8
[    9.252470] 3aa0: cce75600 ccc2b3c0 00000000 c04a00ec 00000b9b
cce83afc 00000002 00000010
[    9.260695] 3ac0: 006b875a 00000001 cce83b8d 00000600 cce75a00
cce83cec c0a84898 00000200
[    9.268918] 3ae0: 0000000d 00000000 00000000 00000000 cce83af0
cce83af0 00000000 cce84000
[    9.277142] 3b00: 00000000 cce86000 00000b8b 00000000 cce83dfc
cce83e6c cce83b4c cce83bf8
[    9.285366] 3b20: 0000000d 00000000 c0a84c0f 00000000 00000000
00000000 ffffffff c08ebda8
[    9.293590] 3b40: c0a84c0f ffffff04 ffff0a00 ffff3133 00000092
cce83dda 00000000 ffffff04
[    9.301813] 3b60: ffff0a00 d72ad1e4 00000000 cce83dfc c0a84e35
c0a84e35 cce83bf8 00000002
[    9.310038] 3b80: cce83ba4 cce83e6c c09b03d8 da3b963c 8beab4c9
ffff0a7e 0000007b cce83df1
[    9.318261] 3ba0: 00002710 ffffff0f ffff0a00 d72ad1e4 00000064
c0d05348 0000007b 00002710
[    9.326486] 3bc0: 00000000 a5b2b7e6 01fb625e cce75600 c0d05348
a5b2b7e6 01fb625e c049cfc0
[    9.334709] 3be0: 00000001 d72ad1e4 cce75600 d72ad1e4 cce75600
cce77400 cce7e400 cce83ccc
[    9.342933] 3c00: cce73c80 cce75600 00000000 cce75a00 c0d05348
c04a2804 cce75600 cce73c80
[    9.351157] 3c20: cce77400 cce75a00 00000024 cce75a00 cce77400
ccc2b240 ccc2b280 cce75600
[    9.359381] 3c40: cce73c80 00000024 00000200 cfd7ac52 00000000
00000b9b 00000000 00000000
[    9.367604] 3c60: 00000000 00000000 cce83c68 cce83c68 00000000
cce62fc0 cce62100 cce86000
[    9.375828] 3c80: cce84000 ccb74000 00240000 00000b8b 00000b9b
00000000 00000000 00000000
[    9.384052] 3ca0: ffffffea cfd81ad8 00000000 00000b8b 00000000
00000000 00000002 00000000
[    9.392275] 3cc0: 00000000 00000000 00000000 c9da3b96 7e8beab4
00000000 00000000 00000000
[    9.400499] 3ce0: 00000000 00000000 00000000 6e617222 3a6d6f64
656c6120 20303d6e 6e656c70
[    9.408723] 3d00: 3539323d 75612035 69736874 313d657a 6c6b2036
333d6e65 00002236 00000000
[    9.416946] 3d20: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[    9.425169] 3d40: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[    9.433392] 3d60: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[    9.441615] 3d80: 00000000 00000000 00000000 00000000 00000000
00000002 00000000 c0003232
[    9.449839] 3da0: 00000cc0 c04ee044 00000cc0 646e6172 203a6d6f
6c706e69 20656361 5f79616d
[    9.458063] 3dc0: 65656c73 73752070 69665f65 2070756e 5f637273
73766964 723c5b3d 706d6965
[    9.466287] 3de0: 3e74726f 2e303031 2b402530 39363131 7669205d
66666f5f 3d746573 75003331
[    9.474511] 3e00: 363e6873 2534322e 2c342b40 382e3320 2b402539
205d3232 5f747364 73766964
[    9.482735] 3e20: 30315b3d 25302e30 31332b40 005d3035 34353034
7669205d 66666f5f 3d746573
[    9.490959] 3e40: 3d003631 33003231 36312000 4025342e 005d372b
3034332b 64205d31 645f7473
[    9.499183] 3e60: 3d737669 2e33375b 00253434 d72ad1e4 cce77400
00000017 c092fa2c cce73c80
[    9.507408] 3e80: cce75a00 cce77400 00000000 ccc2b3c0 ccc2b400
c04a2d3c cce73c80 cce77400
[    9.515631] 3ea0: ce2b08c0 c04a2bf8 00001083 c092e66c ffffffff
cce75a00 c0d05348 00000400
[    9.523855] 3ec0: 00000000 c049d0f4 c0c922c0 cce75a80 ce03f7c0
cfb55300 ce03f7c0 cfb55300
[    9.532079] 3ee0: ce03f7c0 ce2b08c0 00000000 ffffffff c08f0e28
ce2b0840 ce08a018 cfb552c0
[    9.540303] 3f00: ce2b0840 ce03f740 00000000 ce03fa50 00000001
cce83f44 cce83f74 c08f0e28
[    9.548527] 3f20: c0167d1c ce08bce4 00000000 00000003 00000000
0eec3000 c08f138c 00000000
[    9.556750] 3f40: 00000000 ce08bce0 00000000 d72ad1e4 ffffe000
d72ad1e4 ffffe000 cce75a00
[    9.564974] 3f60: cce62080 cce62040 cce82000 cce75a00 c049bd44
ce08bcdc cce6205c c049bd84
[    9.573198] 3f80: 00000000 c015c56c 00000029 cce62080 c015c438
00000000 00000000 00000000
[    9.581421] 3fa0: 00000000 00000000 00000000 c01010e8 00000000
00000000 00000000 00000000
[    9.589644] 3fc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[    9.597866] 3fe0: 00000000 00000000 00000000 00000000 00000013
00000000 00000000 00000000
[    9.606099] [<c0494a68>] (scatterwalk_ffwd) from [<c0739198>]
(omap_aes_gcm_handle_queue+0x180/0x35c)
[    9.615372] [<c0739198>] (omap_aes_gcm_handle_queue) from
[<c07394c8>] (omap_aes_gcm_crypt+0x154/0x210)
[    9.624818] [<c07394c8>] (omap_aes_gcm_crypt) from [<c04a00ec>]
(test_aead_vec_cfg+0x260/0x95c)
[    9.633567] [<c04a00ec>] (test_aead_vec_cfg) from [<c04a2804>]
(test_aead_vs_generic_impl+0x350/0x744)
[    9.642926] [<c04a2804>] (test_aead_vs_generic_impl) from
[<c04a2d3c>] (alg_test_aead+0x144/0x1b8)
[    9.651935] [<c04a2d3c>] (alg_test_aead) from [<c049d0f4>]
(alg_test.part.9+0x9c/0x388)
[    9.659983] [<c049d0f4>] (alg_test.part.9) from [<c049bd84>]
(cryptomgr_test+0x40/0x48)
[    9.668033] [<c049bd84>] (cryptomgr_test) from [<c015c56c>]
(kthread+0x134/0x148)
[    9.675559] [<c015c56c>] (kthread) from [<c01010e8>]
(ret_from_fork+0x14/0x2c)
[    9.682815] Exception stack(0xcce83fb0 to 0xcce83ff8)
[    9.687895] 3fa0:                                     00000000
00000000 00000000 00000000
[    9.696118] 3fc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[    9.704338] 3fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[    9.710994] Code: e1a06000 e1540003 2a000003 ea00000a (e5953008)
[

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 00/10] crypto: omap fixes towards 5.5

[Tero Kristo]

On 25/10/2019 14:33, Ard Biesheuvel wrote:
> On Thu, 17 Oct 2019 at 14:26, Tero Kristo <t-kristo@ti.com> wrote:
>>
>> Hi,
>>
>> This series fixes a number of bugs with omap crypto implementation.
>> These have become evident with the changes to the cryptomanager, where
>> it adds some new test cases and modifies some existing, namely the split
>> update tests. Also, while fixing the cryptomanager induced bugs, some
>> other surfaced with tcrypt/IPSec tests, so fixed them aswell.
>>
>> Patch #9 is against crypto core modifying the crypto_wait_req
>> common API to have a timeout for it also, currently it waits forever
>> and it is kind of difficult to see what test fails with crypto manager.
>> This is not really needed for anything, but it is kind of nice to have
>> (makes debugging easier.)
>>
>> This series has been tested on top of 5.4-rc2, with following setups,
>> on AM57xx-beagle-x15 board:
>>
>> - crypto manager self tests
>> - tcrypt performance test
>> - ipsec test with strongswan
>>
>> This series depends on the skcipher API switch patch from Ard Biesheuvel
>> [1].
>>
> 
> Hi Tero,
> 
> On my BeagleBone White, I am hitting the following issues after
> applying these patches:
> 
> [    7.493903] alg: skcipher: ecb-aes-omap encryption unexpectedly
> succeeded on test vector "random: len=531 klen=32";
> expected_error=-22, cfg="random: inplace may_sleep use_finup
> src_divs=[44.72%@+4028, <flush>14.70%@alignmask+3, 19.45%@+4070,
> 21.13%@+2728]"
> [    7.651103] alg: skcipher: cbc-aes-omap encryption unexpectedly
> succeeded on test vector "random: len=1118 klen=32";
> expected_error=-22, cfg="random: may_sleep use_final
> src_divs=[<reimport>41.87%@+31, <flush>58.13%@+2510]"
> 
> These are simply a result of the ECB and CBC implementations not
> returning -EINVAL when the input is not a multiple of the block size.
> 
> [    7.845527] alg: skcipher: blocksize for ctr-aes-omap (16) doesn't
> match generic impl (1)
> 
> This means cra_blocksize is not set to 1 as it should. If your driver
> uses the skcipher walk API, it should set the walksize to
> AES_BLOCK_SIZE to ensure that the input is handled correctly. If you
> don't, then you can disregard that part.
> 
> [    8.306491] alg: aead: gcm-aes-omap setauthsize unexpectedly
> succeeded on test vector "random: alen=3 plen=31 authsize=6 klen=9";
> expected_error=-22
> 
> Another missing sanity check. GCM only permits certain authsizes.
> 
> [    9.074703] omap_crypto_copy_sgs: Couldn't allocate pages for
> unaligned cases.
> 
> This is not a bug, but I'm not sure if the below is related or not.
> 
> I'll preserve the binaries, in case you need me to objdump anything.

What are these tests you are executing? For me, the testmgr self test 
suite is passing just fine. Any extra tests you have enabled somehow?

I am also running full test on different board though (am57xx), I 
haven't been explicitly running anything on am335x.

-Tero


--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 00/10] crypto: omap fixes towards 5.5

[Tero Kristo]

On 25/10/2019 14:55, Tero Kristo wrote:
> On 25/10/2019 14:33, Ard Biesheuvel wrote:
>> On Thu, 17 Oct 2019 at 14:26, Tero Kristo <t-kristo@ti.com> wrote:
>>>
>>> Hi,
>>>
>>> This series fixes a number of bugs with omap crypto implementation.
>>> These have become evident with the changes to the cryptomanager, where
>>> it adds some new test cases and modifies some existing, namely the split
>>> update tests. Also, while fixing the cryptomanager induced bugs, some
>>> other surfaced with tcrypt/IPSec tests, so fixed them aswell.
>>>
>>> Patch #9 is against crypto core modifying the crypto_wait_req
>>> common API to have a timeout for it also, currently it waits forever
>>> and it is kind of difficult to see what test fails with crypto manager.
>>> This is not really needed for anything, but it is kind of nice to have
>>> (makes debugging easier.)
>>>
>>> This series has been tested on top of 5.4-rc2, with following setups,
>>> on AM57xx-beagle-x15 board:
>>>
>>> - crypto manager self tests
>>> - tcrypt performance test
>>> - ipsec test with strongswan
>>>
>>> This series depends on the skcipher API switch patch from Ard Biesheuvel
>>> [1].
>>>
>>
>> Hi Tero,
>>
>> On my BeagleBone White, I am hitting the following issues after
>> applying these patches:
>>
>> [    7.493903] alg: skcipher: ecb-aes-omap encryption unexpectedly
>> succeeded on test vector "random: len=531 klen=32";
>> expected_error=-22, cfg="random: inplace may_sleep use_finup
>> src_divs=[44.72%@+4028, <flush>14.70%@alignmask+3, 19.45%@+4070,
>> 21.13%@+2728]"
>> [    7.651103] alg: skcipher: cbc-aes-omap encryption unexpectedly
>> succeeded on test vector "random: len=1118 klen=32";
>> expected_error=-22, cfg="random: may_sleep use_final
>> src_divs=[<reimport>41.87%@+31, <flush>58.13%@+2510]"
>>
>> These are simply a result of the ECB and CBC implementations not
>> returning -EINVAL when the input is not a multiple of the block size.
>>
>> [    7.845527] alg: skcipher: blocksize for ctr-aes-omap (16) doesn't
>> match generic impl (1)
>>
>> This means cra_blocksize is not set to 1 as it should. If your driver
>> uses the skcipher walk API, it should set the walksize to
>> AES_BLOCK_SIZE to ensure that the input is handled correctly. If you
>> don't, then you can disregard that part.
>>
>> [    8.306491] alg: aead: gcm-aes-omap setauthsize unexpectedly
>> succeeded on test vector "random: alen=3 plen=31 authsize=6 klen=9";
>> expected_error=-22
>>
>> Another missing sanity check. GCM only permits certain authsizes.
>>
>> [    9.074703] omap_crypto_copy_sgs: Couldn't allocate pages for
>> unaligned cases.
>>
>> This is not a bug, but I'm not sure if the below is related or not.
>>
>> I'll preserve the binaries, in case you need me to objdump anything.
> 
> What are these tests you are executing? For me, the testmgr self test 
> suite is passing just fine. Any extra tests you have enabled somehow?
> 
> I am also running full test on different board though (am57xx), I 
> haven't been explicitly running anything on am335x.

Oh, and btw, did you try without my series? I think the selftests are 
failing rather miserably without them...

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 00/10] crypto: omap fixes towards 5.5

[Ard Biesheuvel]

On Fri, 25 Oct 2019 at 13:56, Tero Kristo <t-kristo@ti.com> wrote:
>
> On 25/10/2019 14:55, Tero Kristo wrote:
> > On 25/10/2019 14:33, Ard Biesheuvel wrote:
> >> On Thu, 17 Oct 2019 at 14:26, Tero Kristo <t-kristo@ti.com> wrote:
> >>>
> >>> Hi,
> >>>
> >>> This series fixes a number of bugs with omap crypto implementation.
> >>> These have become evident with the changes to the cryptomanager, where
> >>> it adds some new test cases and modifies some existing, namely the split
> >>> update tests. Also, while fixing the cryptomanager induced bugs, some
> >>> other surfaced with tcrypt/IPSec tests, so fixed them aswell.
> >>>
> >>> Patch #9 is against crypto core modifying the crypto_wait_req
> >>> common API to have a timeout for it also, currently it waits forever
> >>> and it is kind of difficult to see what test fails with crypto manager.
> >>> This is not really needed for anything, but it is kind of nice to have
> >>> (makes debugging easier.)
> >>>
> >>> This series has been tested on top of 5.4-rc2, with following setups,
> >>> on AM57xx-beagle-x15 board:
> >>>
> >>> - crypto manager self tests
> >>> - tcrypt performance test
> >>> - ipsec test with strongswan
> >>>
> >>> This series depends on the skcipher API switch patch from Ard Biesheuvel
> >>> [1].
> >>>
> >>
> >> Hi Tero,
> >>
> >> On my BeagleBone White, I am hitting the following issues after
> >> applying these patches:
> >>
> >> [    7.493903] alg: skcipher: ecb-aes-omap encryption unexpectedly
> >> succeeded on test vector "random: len=531 klen=32";
> >> expected_error=-22, cfg="random: inplace may_sleep use_finup
> >> src_divs=[44.72%@+4028, <flush>14.70%@alignmask+3, 19.45%@+4070,
> >> 21.13%@+2728]"
> >> [    7.651103] alg: skcipher: cbc-aes-omap encryption unexpectedly
> >> succeeded on test vector "random: len=1118 klen=32";
> >> expected_error=-22, cfg="random: may_sleep use_final
> >> src_divs=[<reimport>41.87%@+31, <flush>58.13%@+2510]"
> >>
> >> These are simply a result of the ECB and CBC implementations not
> >> returning -EINVAL when the input is not a multiple of the block size.
> >>
> >> [    7.845527] alg: skcipher: blocksize for ctr-aes-omap (16) doesn't
> >> match generic impl (1)
> >>
> >> This means cra_blocksize is not set to 1 as it should. If your driver
> >> uses the skcipher walk API, it should set the walksize to
> >> AES_BLOCK_SIZE to ensure that the input is handled correctly. If you
> >> don't, then you can disregard that part.
> >>
> >> [    8.306491] alg: aead: gcm-aes-omap setauthsize unexpectedly
> >> succeeded on test vector "random: alen=3 plen=31 authsize=6 klen=9";
> >> expected_error=-22
> >>
> >> Another missing sanity check. GCM only permits certain authsizes.
> >>
> >> [    9.074703] omap_crypto_copy_sgs: Couldn't allocate pages for
> >> unaligned cases.
> >>
> >> This is not a bug, but I'm not sure if the below is related or not.
> >>
> >> I'll preserve the binaries, in case you need me to objdump anything.
> >
> > What are these tests you are executing? For me, the testmgr self test
> > suite is passing just fine. Any extra tests you have enabled somehow?
> >

I enabled CONFIG_CRYPTO_MANAGER_EXTRA_TESTS, which enables a bunch of
fuzz tests of the offloaded algorithms against the generic
implementations.

> > I am also running full test on different board though (am57xx), I
> > haven't been explicitly running anything on am335x.
>
> Oh, and btw, did you try without my series? I think the selftests are
> failing rather miserably without them...
>

No, I just tried a branch with mine and your patches applied.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 00/10] crypto: omap fixes towards 5.5

[Tero Kristo]

On 25/10/2019 15:05, Ard Biesheuvel wrote:
> On Fri, 25 Oct 2019 at 13:56, Tero Kristo <t-kristo@ti.com> wrote:
>>
>> On 25/10/2019 14:55, Tero Kristo wrote:
>>> On 25/10/2019 14:33, Ard Biesheuvel wrote:
>>>> On Thu, 17 Oct 2019 at 14:26, Tero Kristo <t-kristo@ti.com> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> This series fixes a number of bugs with omap crypto implementation.
>>>>> These have become evident with the changes to the cryptomanager, where
>>>>> it adds some new test cases and modifies some existing, namely the split
>>>>> update tests. Also, while fixing the cryptomanager induced bugs, some
>>>>> other surfaced with tcrypt/IPSec tests, so fixed them aswell.
>>>>>
>>>>> Patch #9 is against crypto core modifying the crypto_wait_req
>>>>> common API to have a timeout for it also, currently it waits forever
>>>>> and it is kind of difficult to see what test fails with crypto manager.
>>>>> This is not really needed for anything, but it is kind of nice to have
>>>>> (makes debugging easier.)
>>>>>
>>>>> This series has been tested on top of 5.4-rc2, with following setups,
>>>>> on AM57xx-beagle-x15 board:
>>>>>
>>>>> - crypto manager self tests
>>>>> - tcrypt performance test
>>>>> - ipsec test with strongswan
>>>>>
>>>>> This series depends on the skcipher API switch patch from Ard Biesheuvel
>>>>> [1].
>>>>>
>>>>
>>>> Hi Tero,
>>>>
>>>> On my BeagleBone White, I am hitting the following issues after
>>>> applying these patches:
>>>>
>>>> [    7.493903] alg: skcipher: ecb-aes-omap encryption unexpectedly
>>>> succeeded on test vector "random: len=531 klen=32";
>>>> expected_error=-22, cfg="random: inplace may_sleep use_finup
>>>> src_divs=[44.72%@+4028, <flush>14.70%@alignmask+3, 19.45%@+4070,
>>>> 21.13%@+2728]"
>>>> [    7.651103] alg: skcipher: cbc-aes-omap encryption unexpectedly
>>>> succeeded on test vector "random: len=1118 klen=32";
>>>> expected_error=-22, cfg="random: may_sleep use_final
>>>> src_divs=[<reimport>41.87%@+31, <flush>58.13%@+2510]"
>>>>
>>>> These are simply a result of the ECB and CBC implementations not
>>>> returning -EINVAL when the input is not a multiple of the block size.
>>>>
>>>> [    7.845527] alg: skcipher: blocksize for ctr-aes-omap (16) doesn't
>>>> match generic impl (1)
>>>>
>>>> This means cra_blocksize is not set to 1 as it should. If your driver
>>>> uses the skcipher walk API, it should set the walksize to
>>>> AES_BLOCK_SIZE to ensure that the input is handled correctly. If you
>>>> don't, then you can disregard that part.
>>>>
>>>> [    8.306491] alg: aead: gcm-aes-omap setauthsize unexpectedly
>>>> succeeded on test vector "random: alen=3 plen=31 authsize=6 klen=9";
>>>> expected_error=-22
>>>>
>>>> Another missing sanity check. GCM only permits certain authsizes.
>>>>
>>>> [    9.074703] omap_crypto_copy_sgs: Couldn't allocate pages for
>>>> unaligned cases.
>>>>
>>>> This is not a bug, but I'm not sure if the below is related or not.
>>>>
>>>> I'll preserve the binaries, in case you need me to objdump anything.
>>>
>>> What are these tests you are executing? For me, the testmgr self test
>>> suite is passing just fine. Any extra tests you have enabled somehow?
>>>
> 
> I enabled CONFIG_CRYPTO_MANAGER_EXTRA_TESTS, which enables a bunch of
> fuzz tests of the offloaded algorithms against the generic
> implementations.

Ahha I see, let me give that a shot locally. I have so far only been 
testing with the standard suite.

> 
>>> I am also running full test on different board though (am57xx), I
>>> haven't been explicitly running anything on am335x.
>>
>> Oh, and btw, did you try without my series? I think the selftests are
>> failing rather miserably without them...
>>
> 
> No, I just tried a branch with mine and your patches applied.

Could you give it a shot without the CRYPTO_MANAGER_EXTRA_TESTS, that 
should pass with my series, and fail without?

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 07/10] crypto: omap-aes-gcm: fix corner case with only auth data

[Ard Biesheuvel]

On Thu, 17 Oct 2019 at 14:26, Tero Kristo <t-kristo@ti.com> wrote:
>
> Fix a corner case where only authdata is generated, without any provided
> assocdata / cryptdata. Passing the empty scatterlists to OMAP AES core driver
> in this case would confuse it, failing to map DMAs.
>

So this change appears to be the culprit for causing the remaining
issue that I reported in the cover letter of the followup series that
I sent out.

The logic below does not account for the case where only assocdata is
provided, which is a valid use of an AEAD.

> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> ---
>  drivers/crypto/omap-aes-gcm.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/crypto/omap-aes-gcm.c b/drivers/crypto/omap-aes-gcm.c
> index 9bbedbccfadf..dfd4d1cac421 100644
> --- a/drivers/crypto/omap-aes-gcm.c
> +++ b/drivers/crypto/omap-aes-gcm.c
> @@ -148,12 +148,14 @@ static int omap_aes_gcm_copy_buffers(struct omap_aes_dev *dd,
>         if (req->src == req->dst || dd->out_sg == sg_arr)
>                 flags |= OMAP_CRYPTO_FORCE_COPY;
>
> -       ret = omap_crypto_align_sg(&dd->out_sg, cryptlen,
> -                                  AES_BLOCK_SIZE, &dd->out_sgl,
> -                                  flags,
> -                                  FLAGS_OUT_DATA_ST_SHIFT, &dd->flags);
> -       if (ret)
> -               return ret;
> +       if (cryptlen) {
> +               ret = omap_crypto_align_sg(&dd->out_sg, cryptlen,
> +                                          AES_BLOCK_SIZE, &dd->out_sgl,
> +                                          flags,
> +                                          FLAGS_OUT_DATA_ST_SHIFT, &dd->flags);
> +               if (ret)
> +                       return ret;
> +       }
>
>         dd->in_sg_len = sg_nents_for_len(dd->in_sg, alen + clen);
>         dd->out_sg_len = sg_nents_for_len(dd->out_sg, clen);
> @@ -287,8 +289,12 @@ static int omap_aes_gcm_handle_queue(struct omap_aes_dev *dd,
>                 return err;
>
>         err = omap_aes_write_ctrl(dd);
> -       if (!err)
> -               err = omap_aes_crypt_dma_start(dd);
> +       if (!err) {
> +               if (dd->in_sg_len && dd->out_sg_len)
> +                       err = omap_aes_crypt_dma_start(dd);
> +               else
> +                       omap_aes_gcm_dma_out_callback(dd);
> +       }
>
>         if (err) {
>                 omap_aes_gcm_finish_req(dd, err);
> --
> 2.17.1
>
> --
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 00/10] crypto: omap fixes towards 5.5

[Ard Biesheuvel]

On Fri, 25 Oct 2019 at 14:18, Tero Kristo <t-kristo@ti.com> wrote:
>
> On 25/10/2019 15:05, Ard Biesheuvel wrote:
> > On Fri, 25 Oct 2019 at 13:56, Tero Kristo <t-kristo@ti.com> wrote:
> >>
> >> On 25/10/2019 14:55, Tero Kristo wrote:
> >>> On 25/10/2019 14:33, Ard Biesheuvel wrote:
> >>>> On Thu, 17 Oct 2019 at 14:26, Tero Kristo <t-kristo@ti.com> wrote:
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>> This series fixes a number of bugs with omap crypto implementation.
> >>>>> These have become evident with the changes to the cryptomanager, where
> >>>>> it adds some new test cases and modifies some existing, namely the split
> >>>>> update tests. Also, while fixing the cryptomanager induced bugs, some
> >>>>> other surfaced with tcrypt/IPSec tests, so fixed them aswell.
> >>>>>
> >>>>> Patch #9 is against crypto core modifying the crypto_wait_req
> >>>>> common API to have a timeout for it also, currently it waits forever
> >>>>> and it is kind of difficult to see what test fails with crypto manager.
> >>>>> This is not really needed for anything, but it is kind of nice to have
> >>>>> (makes debugging easier.)
> >>>>>
> >>>>> This series has been tested on top of 5.4-rc2, with following setups,
> >>>>> on AM57xx-beagle-x15 board:
> >>>>>
> >>>>> - crypto manager self tests
> >>>>> - tcrypt performance test
> >>>>> - ipsec test with strongswan
> >>>>>
> >>>>> This series depends on the skcipher API switch patch from Ard Biesheuvel
> >>>>> [1].
> >>>>>
> >>>>
> >>>> Hi Tero,
> >>>>
> >>>> On my BeagleBone White, I am hitting the following issues after
> >>>> applying these patches:
> >>>>
> >>>> [    7.493903] alg: skcipher: ecb-aes-omap encryption unexpectedly
> >>>> succeeded on test vector "random: len=531 klen=32";
> >>>> expected_error=-22, cfg="random: inplace may_sleep use_finup
> >>>> src_divs=[44.72%@+4028, <flush>14.70%@alignmask+3, 19.45%@+4070,
> >>>> 21.13%@+2728]"
> >>>> [    7.651103] alg: skcipher: cbc-aes-omap encryption unexpectedly
> >>>> succeeded on test vector "random: len=1118 klen=32";
> >>>> expected_error=-22, cfg="random: may_sleep use_final
> >>>> src_divs=[<reimport>41.87%@+31, <flush>58.13%@+2510]"
> >>>>
> >>>> These are simply a result of the ECB and CBC implementations not
> >>>> returning -EINVAL when the input is not a multiple of the block size.
> >>>>
> >>>> [    7.845527] alg: skcipher: blocksize for ctr-aes-omap (16) doesn't
> >>>> match generic impl (1)
> >>>>
> >>>> This means cra_blocksize is not set to 1 as it should. If your driver
> >>>> uses the skcipher walk API, it should set the walksize to
> >>>> AES_BLOCK_SIZE to ensure that the input is handled correctly. If you
> >>>> don't, then you can disregard that part.
> >>>>
> >>>> [    8.306491] alg: aead: gcm-aes-omap setauthsize unexpectedly
> >>>> succeeded on test vector "random: alen=3 plen=31 authsize=6 klen=9";
> >>>> expected_error=-22
> >>>>
> >>>> Another missing sanity check. GCM only permits certain authsizes.
> >>>>
> >>>> [    9.074703] omap_crypto_copy_sgs: Couldn't allocate pages for
> >>>> unaligned cases.
> >>>>
> >>>> This is not a bug, but I'm not sure if the below is related or not.
> >>>>
> >>>> I'll preserve the binaries, in case you need me to objdump anything.
> >>>
> >>> What are these tests you are executing? For me, the testmgr self test
> >>> suite is passing just fine. Any extra tests you have enabled somehow?
> >>>
> >
> > I enabled CONFIG_CRYPTO_MANAGER_EXTRA_TESTS, which enables a bunch of
> > fuzz tests of the offloaded algorithms against the generic
> > implementations.
>
> Ahha I see, let me give that a shot locally. I have so far only been
> testing with the standard suite.
>
> >
> >>> I am also running full test on different board though (am57xx), I
> >>> haven't been explicitly running anything on am335x.
> >>
> >> Oh, and btw, did you try without my series? I think the selftests are
> >> failing rather miserably without them...
> >>
> >
> > No, I just tried a branch with mine and your patches applied.
>
> Could you give it a shot without the CRYPTO_MANAGER_EXTRA_TESTS, that
> should pass with my series, and fail without?
>

The missing output IVs are fixed by this series, but it seems we need
some more work to get all the wrinkles ironed out. I sent some patches
on top that address a couple of them, but we still need a proper fix
for the situation where only assocdata is presented, and cryptlen == 0

Feel free to merge my patches into your series, or take bits and
pieces into your own patches where needed.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 09/10] crypto: add timeout to crypto_wait_req

[Eric Biggers]

On Thu, Oct 17, 2019 at 03:25:48PM +0300, Tero Kristo wrote:
> Currently crypto_wait_req waits indefinitely for an async crypto request
> to complete. This is bad as it can cause for example the crypto test
> manager to hang without any notification as to why it has happened.
> Instead of waiting indefinitely, add a 1 second timeout to the call,
> and provide a warning print if a timeout happens.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> ---
>  include/linux/crypto.h | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/crypto.h b/include/linux/crypto.h
> index 19ea3a371d7b..b8f0e5c3cc0c 100644
> --- a/include/linux/crypto.h
> +++ b/include/linux/crypto.h
> @@ -682,8 +682,15 @@ static inline int crypto_wait_req(int err, struct crypto_wait *wait)
>  	switch (err) {
>  	case -EINPROGRESS:
>  	case -EBUSY:
> -		wait_for_completion(&wait->completion);
> +		err = wait_for_completion_timeout(&wait->completion,
> +						  msecs_to_jiffies(1000));
>  		reinit_completion(&wait->completion);
> +		if (!err) {
> +			pr_err("%s: timeout for %p\n", __func__, wait);
> +			err = -ETIMEDOUT;
> +			break;
> +		}
> +
>  		err = wait->err;
>  		break;
>  	};

I'm not sure this is a good idea, because operations could legitimately take a
long time, e.g. if someone passes in a huge data buffer.  How do you know that X
amount of time is always going to be enough?

- Eric

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 09/10] crypto: add timeout to crypto_wait_req

[Gilad Ben-Yossef]

Hi,


On Thu, Oct 17, 2019 at 3:26 PM Tero Kristo <t-kristo@ti.com> wrote:
>
> Currently crypto_wait_req waits indefinitely for an async crypto request
> to complete. This is bad as it can cause for example the crypto test
> manager to hang without any notification as to why it has happened.
> Instead of waiting indefinitely, add a 1 second timeout to the call,
> and provide a warning print if a timeout happens.

While the incentive is clear and positive, this suggested solution
creates problems of its own.
In many (most?) cases where we are waiting here, we are waiting for a
DMA operation to finish from hardware.
Exiting while this pending DMA operation is not finished, even with a
proper error return value, is dangerous because
unless the calling code takes great care to not release the memory the
DMA is being done from/to, this can have disastrous effects.

As Eric has already mentioned, one second might seem like a long time,
but we don't really know if it is enough.

How about adding a second API (ig. crypto_wait_req_timeout) which
supports a calee specified timeout where
the calle knows how to correctly deal with timeout and port the
relevant call sites to use this?

Thanks!
Gilad


>
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> ---
>  include/linux/crypto.h | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/crypto.h b/include/linux/crypto.h
> index 19ea3a371d7b..b8f0e5c3cc0c 100644
> --- a/include/linux/crypto.h
> +++ b/include/linux/crypto.h
> @@ -682,8 +682,15 @@ static inline int crypto_wait_req(int err, struct crypto_wait *wait)
>         switch (err) {
>         case -EINPROGRESS:
>         case -EBUSY:
> -               wait_for_completion(&wait->completion);
> +               err = wait_for_completion_timeout(&wait->completion,
> +                                                 msecs_to_jiffies(1000));
>                 reinit_completion(&wait->completion);
> +               if (!err) {
> +                       pr_err("%s: timeout for %p\n", __func__, wait);
> +                       err = -ETIMEDOUT;
> +                       break;
> +               }
> +
>                 err = wait->err;
>                 break;
>         };
> --
> 2.17.1
>
> --
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki



--
Gilad Ben-Yossef
Chief Coffee Drinker

values of β will give rise to dom!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 09/10] crypto: add timeout to crypto_wait_req

[Tero Kristo]

On 06/11/2019 08:39, Gilad Ben-Yossef wrote:
> Hi,
> 
> 
> On Thu, Oct 17, 2019 at 3:26 PM Tero Kristo <t-kristo@ti.com> wrote:
>>
>> Currently crypto_wait_req waits indefinitely for an async crypto request
>> to complete. This is bad as it can cause for example the crypto test
>> manager to hang without any notification as to why it has happened.
>> Instead of waiting indefinitely, add a 1 second timeout to the call,
>> and provide a warning print if a timeout happens.
> 
> While the incentive is clear and positive, this suggested solution
> creates problems of its own.
> In many (most?) cases where we are waiting here, we are waiting for a
> DMA operation to finish from hardware.
> Exiting while this pending DMA operation is not finished, even with a
> proper error return value, is dangerous because
> unless the calling code takes great care to not release the memory the
> DMA is being done from/to, this can have disastrous effects.
> 
> As Eric has already mentioned, one second might seem like a long time,
> but we don't really know if it is enough.
> 
> How about adding a second API (ig. crypto_wait_req_timeout) which
> supports a calee specified timeout where
> the calle knows how to correctly deal with timeout and port the
> relevant call sites to use this?

Yeah, that would work for me. I guess we could just swap the testmgr to 
use this timeout API, as it is quite clear it should timeout rather than 
wait indefinitely, and afaics, the data buffers it uses are limited 
size. It doesn't really matter for it whether the timeout is 1 second or 
10 seconds, as long as it eventually times out.

-Tero

> 
> Thanks!
> Gilad
> 
> 
>>
>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>> ---
>>   include/linux/crypto.h | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/crypto.h b/include/linux/crypto.h
>> index 19ea3a371d7b..b8f0e5c3cc0c 100644
>> --- a/include/linux/crypto.h
>> +++ b/include/linux/crypto.h
>> @@ -682,8 +682,15 @@ static inline int crypto_wait_req(int err, struct crypto_wait *wait)
>>          switch (err) {
>>          case -EINPROGRESS:
>>          case -EBUSY:
>> -               wait_for_completion(&wait->completion);
>> +               err = wait_for_completion_timeout(&wait->completion,
>> +                                                 msecs_to_jiffies(1000));
>>                  reinit_completion(&wait->completion);
>> +               if (!err) {
>> +                       pr_err("%s: timeout for %p\n", __func__, wait);
>> +                       err = -ETIMEDOUT;
>> +                       break;
>> +               }
>> +
>>                  err = wait->err;
>>                  break;
>>          };
>> --
>> 2.17.1
>>
>> --
> 
> 
> 
> --
> Gilad Ben-Yossef
> Chief Coffee Drinker
> 
> values of β will give rise to dom!
> 

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 09/10] crypto: add timeout to crypto_wait_req

[Gilad Ben-Yossef]

On Wed, Nov 6, 2019 at 9:25 AM Tero Kristo <t-kristo@ti.com> wrote:
>
> On 06/11/2019 08:39, Gilad Ben-Yossef wrote:
> > Hi,
> >
> >
> > On Thu, Oct 17, 2019 at 3:26 PM Tero Kristo <t-kristo@ti.com> wrote:
> >>
> >> Currently crypto_wait_req waits indefinitely for an async crypto request
> >> to complete. This is bad as it can cause for example the crypto test
> >> manager to hang without any notification as to why it has happened.
> >> Instead of waiting indefinitely, add a 1 second timeout to the call,
> >> and provide a warning print if a timeout happens.
> >
> > While the incentive is clear and positive, this suggested solution
> > creates problems of its own.
> > In many (most?) cases where we are waiting here, we are waiting for a
> > DMA operation to finish from hardware.
> > Exiting while this pending DMA operation is not finished, even with a
> > proper error return value, is dangerous because
> > unless the calling code takes great care to not release the memory the
> > DMA is being done from/to, this can have disastrous effects.
> >
> > As Eric has already mentioned, one second might seem like a long time,
> > but we don't really know if it is enough.
> >
> > How about adding a second API (ig. crypto_wait_req_timeout) which
> > supports a calee specified timeout where
> > the calle knows how to correctly deal with timeout and port the
> > relevant call sites to use this?
>
> Yeah, that would work for me. I guess we could just swap the testmgr to
> use this timeout API, as it is quite clear it should timeout rather than
> wait indefinitely, and afaics, the data buffers it uses are limited
> size. It doesn't really matter for it whether the timeout is 1 second or
> 10 seconds, as long as it eventually times out.


As long as you avoid releasing the memory used on timeout, that should
work well, I think.

Gilad

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 09/10] crypto: add timeout to crypto_wait_req

[Eric Biggers]

On Wed, Nov 06, 2019 at 09:33:20AM +0200, Gilad Ben-Yossef wrote:
> On Wed, Nov 6, 2019 at 9:25 AM Tero Kristo <t-kristo@ti.com> wrote:
> >
> > On 06/11/2019 08:39, Gilad Ben-Yossef wrote:
> > > Hi,
> > >
> > >
> > > On Thu, Oct 17, 2019 at 3:26 PM Tero Kristo <t-kristo@ti.com> wrote:
> > >>
> > >> Currently crypto_wait_req waits indefinitely for an async crypto request
> > >> to complete. This is bad as it can cause for example the crypto test
> > >> manager to hang without any notification as to why it has happened.
> > >> Instead of waiting indefinitely, add a 1 second timeout to the call,
> > >> and provide a warning print if a timeout happens.
> > >
> > > While the incentive is clear and positive, this suggested solution
> > > creates problems of its own.
> > > In many (most?) cases where we are waiting here, we are waiting for a
> > > DMA operation to finish from hardware.
> > > Exiting while this pending DMA operation is not finished, even with a
> > > proper error return value, is dangerous because
> > > unless the calling code takes great care to not release the memory the
> > > DMA is being done from/to, this can have disastrous effects.
> > >
> > > As Eric has already mentioned, one second might seem like a long time,
> > > but we don't really know if it is enough.
> > >
> > > How about adding a second API (ig. crypto_wait_req_timeout) which
> > > supports a calee specified timeout where
> > > the calle knows how to correctly deal with timeout and port the
> > > relevant call sites to use this?
> >
> > Yeah, that would work for me. I guess we could just swap the testmgr to
> > use this timeout API, as it is quite clear it should timeout rather than
> > wait indefinitely, and afaics, the data buffers it uses are limited
> > size. It doesn't really matter for it whether the timeout is 1 second or
> > 10 seconds, as long as it eventually times out.
> 
> 
> As long as you avoid releasing the memory used on timeout, that should
> work well, I think.
> 

The memory is always going to be freed eventually, though.  Although the crypto
tests currently reuse the input/output buffers and the request structure from
one test to the next, they're freed at the end of the tests.  Also, it's unsafe
for one request structure to be used for multiple requests concurrently anyway.

I think crypto_wait_req_timeout() would just be fundamentally unsafe.

Couldn't you just use CONFIG_DETECT_HUNG_TASK=y instead?  It should report if
any thread is blocked for too long.

- Eric

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 09/10] crypto: add timeout to crypto_wait_req

[Tero Kristo]

On 08/11/2019 04:27, Eric Biggers wrote:
> On Wed, Nov 06, 2019 at 09:33:20AM +0200, Gilad Ben-Yossef wrote:
>> On Wed, Nov 6, 2019 at 9:25 AM Tero Kristo <t-kristo@ti.com> wrote:
>>>
>>> On 06/11/2019 08:39, Gilad Ben-Yossef wrote:
>>>> Hi,
>>>>
>>>>
>>>> On Thu, Oct 17, 2019 at 3:26 PM Tero Kristo <t-kristo@ti.com> wrote:
>>>>>
>>>>> Currently crypto_wait_req waits indefinitely for an async crypto request
>>>>> to complete. This is bad as it can cause for example the crypto test
>>>>> manager to hang without any notification as to why it has happened.
>>>>> Instead of waiting indefinitely, add a 1 second timeout to the call,
>>>>> and provide a warning print if a timeout happens.
>>>>
>>>> While the incentive is clear and positive, this suggested solution
>>>> creates problems of its own.
>>>> In many (most?) cases where we are waiting here, we are waiting for a
>>>> DMA operation to finish from hardware.
>>>> Exiting while this pending DMA operation is not finished, even with a
>>>> proper error return value, is dangerous because
>>>> unless the calling code takes great care to not release the memory the
>>>> DMA is being done from/to, this can have disastrous effects.
>>>>
>>>> As Eric has already mentioned, one second might seem like a long time,
>>>> but we don't really know if it is enough.
>>>>
>>>> How about adding a second API (ig. crypto_wait_req_timeout) which
>>>> supports a calee specified timeout where
>>>> the calle knows how to correctly deal with timeout and port the
>>>> relevant call sites to use this?
>>>
>>> Yeah, that would work for me. I guess we could just swap the testmgr to
>>> use this timeout API, as it is quite clear it should timeout rather than
>>> wait indefinitely, and afaics, the data buffers it uses are limited
>>> size. It doesn't really matter for it whether the timeout is 1 second or
>>> 10 seconds, as long as it eventually times out.
>>
>>
>> As long as you avoid releasing the memory used on timeout, that should
>> work well, I think.
>>
> 
> The memory is always going to be freed eventually, though.  Although the crypto
> tests currently reuse the input/output buffers and the request structure from
> one test to the next, they're freed at the end of the tests.  Also, it's unsafe
> for one request structure to be used for multiple requests concurrently anyway.
> 
> I think crypto_wait_req_timeout() would just be fundamentally unsafe.
> 
> Couldn't you just use CONFIG_DETECT_HUNG_TASK=y instead?  It should report if
> any thread is blocked for too long.

The problem is not detecting a hung task, the problem is determining 
what caused the hang. Personally I don't care if the system dies if a 
crypto accelerator self test has failed, as long as I get reported about 
the exact nature of the failure. The failures are expected to happen 
only in development phase of a crypto driver.

With the timeout patch in place, I get reported what exact crypto test 
case failed and I can focus my debug efforts on that one.

Anyways, as said this is just a nice to have patch, and can be dropped 
no issues there. I was just thinking some other people might find it 
useful also.

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 09/10] crypto: add timeout to crypto_wait_req

[Herbert Xu]

On Fri, Nov 08, 2019 at 09:40:57AM +0200, Tero Kristo wrote:
>
> The problem is not detecting a hung task, the problem is determining what
> caused the hang. Personally I don't care if the system dies if a crypto
> accelerator self test has failed, as long as I get reported about the exact
> nature of the failure. The failures are expected to happen only in
> development phase of a crypto driver.
> 
> With the timeout patch in place, I get reported what exact crypto test case
> failed and I can focus my debug efforts on that one.

If that's all you need then how about just making the wait killable?

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 09/10] crypto: add timeout to crypto_wait_req

[Tero Kristo]

On 08/11/2019 11:16, Herbert Xu wrote:
> On Fri, Nov 08, 2019 at 09:40:57AM +0200, Tero Kristo wrote:
>>
>> The problem is not detecting a hung task, the problem is determining what
>> caused the hang. Personally I don't care if the system dies if a crypto
>> accelerator self test has failed, as long as I get reported about the exact
>> nature of the failure. The failures are expected to happen only in
>> development phase of a crypto driver.
>>
>> With the timeout patch in place, I get reported what exact crypto test case
>> failed and I can focus my debug efforts on that one.
> 
> If that's all you need then how about just making the wait killable?

Yeah, that would be an alternative.

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 09/10] crypto: add timeout to crypto_wait_req

[Eric Biggers]

On Fri, Nov 08, 2019 at 11:22:48AM +0200, Tero Kristo wrote:
> On 08/11/2019 11:16, Herbert Xu wrote:
> > On Fri, Nov 08, 2019 at 09:40:57AM +0200, Tero Kristo wrote:
> > > 
> > > The problem is not detecting a hung task, the problem is determining what
> > > caused the hang. Personally I don't care if the system dies if a crypto
> > > accelerator self test has failed, as long as I get reported about the exact
> > > nature of the failure. The failures are expected to happen only in
> > > development phase of a crypto driver.
> > > 
> > > With the timeout patch in place, I get reported what exact crypto test case
> > > failed and I can focus my debug efforts on that one.
> > 
> > If that's all you need then how about just making the wait killable?
> 
> Yeah, that would be an alternative.
> 

I don't see how making crypto_wait_req killable would be any better than adding
a timeout, since in both cases the crypto operation would still be proceeding in
the background while things are being freed.

Would it help if the crypto self-tests printed a pr_debug() message when
starting each test vector?  These wouldn't be shown by default, but it would be
possible to enable them using dynamic-debug or by adding '#define DEBUG' to the
top of the source file.

- Eric

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 09/10] crypto: add timeout to crypto_wait_req

[Herbert Xu]

On Fri, Nov 08, 2019 at 06:27:49PM -0800, Eric Biggers wrote:
> 
> I don't see how making crypto_wait_req killable would be any better than adding
> a timeout, since in both cases the crypto operation would still be proceeding in
> the background while things are being freed.

Right, you would need to modify the caller to actually distinguish
between the killed case vs. actual completion.

> Would it help if the crypto self-tests printed a pr_debug() message when
> starting each test vector?  These wouldn't be shown by default, but it would be
> possible to enable them using dynamic-debug or by adding '#define DEBUG' to the
> top of the source file.

This should be simpler to implement.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel