[PATCH] crypto: omap-sham: Check for HIGHMEM before mapping SG pages

[PATCH] crypto: omap-sham: Check for HIGHMEM before mapping SG pages

[Lokesh Vutla]

Commit 26a05489ee0e ("crypto: omap-sham - Map SG pages if they are HIGHMEM before accessing")
says that HIGHMEM pages may not be mapped so we must
kmap them before accessing, but it doesn't check whether the
corresponding page is in highmem or not. Because of this all
the crypto tests are failing.

00000000: d9 a1 1b 7c aa 90 3b aa 11 ab cb 25 00 b8 ac bf
[    2.338169] 00000010: c1 39 cd ff 48 d0 a8 e2 2b fa 33 a1
[    2.344008] alg: hash: Chunking test 1 failed for omap-sha256

So Checking for HIGHMEM before mapping SG pages.

Fixes: 26a05489ee0 ("crypto: omap-sham - Map SG pages if they are HIGHMEM before accessing")
Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
 drivers/crypto/omap-sham.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/omap-sham.c b/drivers/crypto/omap-sham.c
index 3c76696..ace5852 100644
--- a/drivers/crypto/omap-sham.c
+++ b/drivers/crypto/omap-sham.c
@@ -639,13 +639,17 @@ static size_t omap_sham_append_sg(struct omap_sham_reqctx *ctx)
 	const u8 *vaddr;
 
 	while (ctx->sg) {
-		vaddr = kmap_atomic(sg_page(ctx->sg));
+		if (PageHighMem(sg_page(ctx->sg)))
+			vaddr = kmap_atomic(sg_page(ctx->sg));
+		else
+			vaddr = sg_virt(ctx->sg);
 
 		count = omap_sham_append_buffer(ctx,
 				vaddr + ctx->offset,
 				ctx->sg->length - ctx->offset);
 
-		kunmap_atomic((void *)vaddr);
+		if (PageHighMem(sg_page(ctx->sg)))
+			kunmap_atomic((void *)vaddr);
 
 		if (!count)
 			break;
-- 
1.9.1

[PATCH] crypto: omap-sham: Check for HIGHMEM before mapping SG pages

[Lokesh Vutla]

Commit 26a05489ee0e ("crypto: omap-sham - Map SG pages if they are HIGHMEM before accessing")
says that HIGHMEM pages may not be mapped so we must
kmap them before accessing, but it doesn't check whether the
corresponding page is in highmem or not. Because of this all
the crypto tests are failing.

00000000: d9 a1 1b 7c aa 90 3b aa 11 ab cb 25 00 b8 ac bf
[    2.338169] 00000010: c1 39 cd ff 48 d0 a8 e2 2b fa 33 a1
[    2.344008] alg: hash: Chunking test 1 failed for omap-sha256

So Checking for HIGHMEM before mapping SG pages.

Fixes: 26a05489ee0 ("crypto: omap-sham - Map SG pages if they are HIGHMEM before accessing")
Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
 drivers/crypto/omap-sham.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/omap-sham.c b/drivers/crypto/omap-sham.c
index 3c76696..ace5852 100644
--- a/drivers/crypto/omap-sham.c
+++ b/drivers/crypto/omap-sham.c
@@ -639,13 +639,17 @@ static size_t omap_sham_append_sg(struct omap_sham_reqctx *ctx)
 	const u8 *vaddr;
 
 	while (ctx->sg) {
-		vaddr = kmap_atomic(sg_page(ctx->sg));
+		if (PageHighMem(sg_page(ctx->sg)))
+			vaddr = kmap_atomic(sg_page(ctx->sg));
+		else
+			vaddr = sg_virt(ctx->sg);
 
 		count = omap_sham_append_buffer(ctx,
 				vaddr + ctx->offset,
 				ctx->sg->length - ctx->offset);
 
-		kunmap_atomic((void *)vaddr);
+		if (PageHighMem(sg_page(ctx->sg)))
+			kunmap_atomic((void *)vaddr);
 
 		if (!count)
 			break;
-- 
1.9.1

[PATCH] crypto: omap-sham: Use pm_runtime_irq_safe()

[Lokesh Vutla]

omap_sham_handle_queue() can be called as part of done_task tasklet.
During this its atomic and any calls to pm functions cannot sleep.

But there is a call to pm_runtime_get_sync() (which can sleep) in
omap_sham_handle_queue(), because of which the following appears:
" [  116.169969] BUG: scheduling while atomic: kworker/0:2/2676/0x00000100"

Add pm_runtime_irq_safe() to avoid this.

Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
 drivers/crypto/omap-sham.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/crypto/omap-sham.c b/drivers/crypto/omap-sham.c
index ace5852..81ed511 100644
--- a/drivers/crypto/omap-sham.c
+++ b/drivers/crypto/omap-sham.c
@@ -1949,6 +1949,7 @@ static int omap_sham_probe(struct platform_device *pdev)
 	dd->flags |= dd->pdata->flags;
 
 	pm_runtime_enable(dev);
+	pm_runtime_irq_safe(dev);
 	pm_runtime_get_sync(dev);
 	rev = omap_sham_read(dd, SHA_REG_REV(dd));
 	pm_runtime_put_sync(&pdev->dev);
-- 
1.9.1

[PATCH] crypto: omap-sham: Use pm_runtime_irq_safe()

[Lokesh Vutla]

omap_sham_handle_queue() can be called as part of done_task tasklet.
During this its atomic and any calls to pm functions cannot sleep.

But there is a call to pm_runtime_get_sync() (which can sleep) in
omap_sham_handle_queue(), because of which the following appears:
" [  116.169969] BUG: scheduling while atomic: kworker/0:2/2676/0x00000100"

Add pm_runtime_irq_safe() to avoid this.

Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
 drivers/crypto/omap-sham.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/crypto/omap-sham.c b/drivers/crypto/omap-sham.c
index ace5852..81ed511 100644
--- a/drivers/crypto/omap-sham.c
+++ b/drivers/crypto/omap-sham.c
@@ -1949,6 +1949,7 @@ static int omap_sham_probe(struct platform_device *pdev)
 	dd->flags |= dd->pdata->flags;
 
 	pm_runtime_enable(dev);
+	pm_runtime_irq_safe(dev);
 	pm_runtime_get_sync(dev);
 	rev = omap_sham_read(dd, SHA_REG_REV(dd));
 	pm_runtime_put_sync(&pdev->dev);
-- 
1.9.1

Re: [PATCH] crypto: omap-sham: Use pm_runtime_irq_safe()

[Herbert Xu]

On Tue, Mar 31, 2015 at 09:52:24AM +0530, Lokesh Vutla wrote:
> omap_sham_handle_queue() can be called as part of done_task tasklet.
> During this its atomic and any calls to pm functions cannot sleep.
> 
> But there is a call to pm_runtime_get_sync() (which can sleep) in
> omap_sham_handle_queue(), because of which the following appears:
> " [  116.169969] BUG: scheduling while atomic: kworker/0:2/2676/0x00000100"
> 
> Add pm_runtime_irq_safe() to avoid this.
> 
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>

Applied.
-- 
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

[PATCH] crypto: omap-aes: Fix support for unequal lengths

[Lokesh Vutla]

For cases where total length of an input SGs is not same as
length of the input data for encryption, omap-aes driver
crashes. This happens in the case when IPsec is trying to use
omap-aes driver.

To avoid this, we copy all the pages from the input SG list
into a contiguous buffer and prepare a single element SG list
for this buffer with length as the total bytes to crypt, which is
similar thing that is done in case of unaligned lengths.

Fixes: 6242332ff2f3 ("crypto: omap-aes - Add support for cases of unaligned lengths")
Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
 drivers/crypto/omap-aes.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/crypto/omap-aes.c b/drivers/crypto/omap-aes.c
index 42f95a4..9a28b7e 100644
--- a/drivers/crypto/omap-aes.c
+++ b/drivers/crypto/omap-aes.c
@@ -554,15 +554,23 @@ static int omap_aes_crypt_dma_stop(struct omap_aes_dev *dd)
 	return err;
 }
 
-static int omap_aes_check_aligned(struct scatterlist *sg)
+static int omap_aes_check_aligned(struct scatterlist *sg, int total)
 {
+	int len = 0;
+
 	while (sg) {
 		if (!IS_ALIGNED(sg->offset, 4))
 			return -1;
 		if (!IS_ALIGNED(sg->length, AES_BLOCK_SIZE))
 			return -1;
+
+		len += sg->length;
 		sg = sg_next(sg);
 	}
+
+	if (len != total)
+		return -1;
+
 	return 0;
 }
 
@@ -633,8 +641,8 @@ static int omap_aes_handle_queue(struct omap_aes_dev *dd,
 	dd->in_sg = req->src;
 	dd->out_sg = req->dst;
 
-	if (omap_aes_check_aligned(dd->in_sg) ||
-	    omap_aes_check_aligned(dd->out_sg)) {
+	if (omap_aes_check_aligned(dd->in_sg, dd->total) ||
+	    omap_aes_check_aligned(dd->out_sg, dd->total)) {
 		if (omap_aes_copy_sgs(dd))
 			pr_err("Failed to copy SGs for unaligned cases\n");
 		dd->sgs_copied = 1;
-- 
1.9.1

[PATCH] crypto: omap-aes: Fix support for unequal lengths

[Lokesh Vutla]

For cases where total length of an input SGs is not same as
length of the input data for encryption, omap-aes driver
crashes. This happens in the case when IPsec is trying to use
omap-aes driver.

To avoid this, we copy all the pages from the input SG list
into a contiguous buffer and prepare a single element SG list
for this buffer with length as the total bytes to crypt, which is
similar thing that is done in case of unaligned lengths.

Fixes: 6242332ff2f3 ("crypto: omap-aes - Add support for cases of unaligned lengths")
Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
 drivers/crypto/omap-aes.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/crypto/omap-aes.c b/drivers/crypto/omap-aes.c
index 42f95a4..9a28b7e 100644
--- a/drivers/crypto/omap-aes.c
+++ b/drivers/crypto/omap-aes.c
@@ -554,15 +554,23 @@ static int omap_aes_crypt_dma_stop(struct omap_aes_dev *dd)
 	return err;
 }
 
-static int omap_aes_check_aligned(struct scatterlist *sg)
+static int omap_aes_check_aligned(struct scatterlist *sg, int total)
 {
+	int len = 0;
+
 	while (sg) {
 		if (!IS_ALIGNED(sg->offset, 4))
 			return -1;
 		if (!IS_ALIGNED(sg->length, AES_BLOCK_SIZE))
 			return -1;
+
+		len += sg->length;
 		sg = sg_next(sg);
 	}
+
+	if (len != total)
+		return -1;
+
 	return 0;
 }
 
@@ -633,8 +641,8 @@ static int omap_aes_handle_queue(struct omap_aes_dev *dd,
 	dd->in_sg = req->src;
 	dd->out_sg = req->dst;
 
-	if (omap_aes_check_aligned(dd->in_sg) ||
-	    omap_aes_check_aligned(dd->out_sg)) {
+	if (omap_aes_check_aligned(dd->in_sg, dd->total) ||
+	    omap_aes_check_aligned(dd->out_sg, dd->total)) {
 		if (omap_aes_copy_sgs(dd))
 			pr_err("Failed to copy SGs for unaligned cases\n");
 		dd->sgs_copied = 1;
-- 
1.9.1

Re: [PATCH] crypto: omap-aes: Fix support for unequal lengths

[Herbert Xu]

On Tue, Mar 31, 2015 at 09:52:25AM +0530, Lokesh Vutla wrote:
> For cases where total length of an input SGs is not same as
> length of the input data for encryption, omap-aes driver
> crashes. This happens in the case when IPsec is trying to use
> omap-aes driver.
> 
> To avoid this, we copy all the pages from the input SG list
> into a contiguous buffer and prepare a single element SG list
> for this buffer with length as the total bytes to crypt, which is
> similar thing that is done in case of unaligned lengths.
> 
> Fixes: 6242332ff2f3 ("crypto: omap-aes - Add support for cases of unaligned lengths")
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>

Applied.
-- 
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

Re: [PATCH] crypto: omap-sham: Check for HIGHMEM before mapping SG pages

[Herbert Xu]

On Tue, Mar 31, 2015 at 09:52:23AM +0530, Lokesh Vutla wrote:
> Commit 26a05489ee0e ("crypto: omap-sham - Map SG pages if they are HIGHMEM before accessing")
> says that HIGHMEM pages may not be mapped so we must
> kmap them before accessing, but it doesn't check whether the
> corresponding page is in highmem or not. Because of this all
> the crypto tests are failing.
> 
> 00000000: d9 a1 1b 7c aa 90 3b aa 11 ab cb 25 00 b8 ac bf
> [    2.338169] 00000010: c1 39 cd ff 48 d0 a8 e2 2b fa 33 a1
> [    2.344008] alg: hash: Chunking test 1 failed for omap-sha256
> 
> So Checking for HIGHMEM before mapping SG pages.
> 
> Fixes: 26a05489ee0 ("crypto: omap-sham - Map SG pages if they are HIGHMEM before accessing")
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> ---
>  drivers/crypto/omap-sham.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/crypto/omap-sham.c b/drivers/crypto/omap-sham.c
> index 3c76696..ace5852 100644
> --- a/drivers/crypto/omap-sham.c
> +++ b/drivers/crypto/omap-sham.c
> @@ -639,13 +639,17 @@ static size_t omap_sham_append_sg(struct omap_sham_reqctx *ctx)
>  	const u8 *vaddr;
>  
>  	while (ctx->sg) {
> -		vaddr = kmap_atomic(sg_page(ctx->sg));
> +		if (PageHighMem(sg_page(ctx->sg)))
> +			vaddr = kmap_atomic(sg_page(ctx->sg));
> +		else
> +			vaddr = sg_virt(ctx->sg);

This is completely bogus.  kmap_atomic should be identical to
sg_virt(sg_page()) for the lowmem case.

So either your architecture is broken (because the same problem
would obviously affect the core crypto code which does exactly
the same thing), or there is some other bug causing the selftest
to fail.

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

Re: [PATCH] crypto: omap-sham: Check for HIGHMEM before mapping SG pages

[Lokesh Vutla]

Hi Herbert,
On Wednesday 01 April 2015 07:48 PM, Herbert Xu wrote:
> On Tue, Mar 31, 2015 at 09:52:23AM +0530, Lokesh Vutla wrote:
>> Commit 26a05489ee0e ("crypto: omap-sham - Map SG pages if they are HIGHMEM before accessing")
>> says that HIGHMEM pages may not be mapped so we must
>> kmap them before accessing, but it doesn't check whether the
>> corresponding page is in highmem or not. Because of this all
>> the crypto tests are failing.
>>
>> 00000000: d9 a1 1b 7c aa 90 3b aa 11 ab cb 25 00 b8 ac bf
>> [    2.338169] 00000010: c1 39 cd ff 48 d0 a8 e2 2b fa 33 a1
>> [    2.344008] alg: hash: Chunking test 1 failed for omap-sha256
>>
>> So Checking for HIGHMEM before mapping SG pages.
>>
>> Fixes: 26a05489ee0 ("crypto: omap-sham - Map SG pages if they are HIGHMEM before accessing")
>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>> ---
>>  drivers/crypto/omap-sham.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/crypto/omap-sham.c b/drivers/crypto/omap-sham.c
>> index 3c76696..ace5852 100644
>> --- a/drivers/crypto/omap-sham.c
>> +++ b/drivers/crypto/omap-sham.c
>> @@ -639,13 +639,17 @@ static size_t omap_sham_append_sg(struct omap_sham_reqctx *ctx)
>>  	const u8 *vaddr;
>>  
>>  	while (ctx->sg) {
>> -		vaddr = kmap_atomic(sg_page(ctx->sg));
>> +		if (PageHighMem(sg_page(ctx->sg)))
>> +			vaddr = kmap_atomic(sg_page(ctx->sg));
>> +		else
>> +			vaddr = sg_virt(ctx->sg);
> 
> This is completely bogus.  kmap_atomic should be identical to
> sg_virt(sg_page()) for the lowmem case.
> 
> So either your architecture is broken (because the same problem
> would obviously affect the core crypto code which does exactly
> the same thing), or there is some other bug causing the selftest
> to fail.
Oops my bad. You are right.
Here the problem is sg->offset is not being added to vaddr.
sg_virt gives page address + sg->offset but
kmap_atomic gives only the page address.

Ill update and repost the patch.

Thanks and regards,
Lokesh
> 
> Cheers,
> 

Re: [PATCH] crypto: omap-sham: Check for HIGHMEM before mapping SG pages

[Lokesh Vutla]

Hi Herbert,
On Wednesday 01 April 2015 07:48 PM, Herbert Xu wrote:
> On Tue, Mar 31, 2015 at 09:52:23AM +0530, Lokesh Vutla wrote:
>> Commit 26a05489ee0e ("crypto: omap-sham - Map SG pages if they are HIGHMEM before accessing")
>> says that HIGHMEM pages may not be mapped so we must
>> kmap them before accessing, but it doesn't check whether the
>> corresponding page is in highmem or not. Because of this all
>> the crypto tests are failing.
>>
>> 00000000: d9 a1 1b 7c aa 90 3b aa 11 ab cb 25 00 b8 ac bf
>> [    2.338169] 00000010: c1 39 cd ff 48 d0 a8 e2 2b fa 33 a1
>> [    2.344008] alg: hash: Chunking test 1 failed for omap-sha256
>>
>> So Checking for HIGHMEM before mapping SG pages.
>>
>> Fixes: 26a05489ee0 ("crypto: omap-sham - Map SG pages if they are HIGHMEM before accessing")
>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>> ---
>>  drivers/crypto/omap-sham.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/crypto/omap-sham.c b/drivers/crypto/omap-sham.c
>> index 3c76696..ace5852 100644
>> --- a/drivers/crypto/omap-sham.c
>> +++ b/drivers/crypto/omap-sham.c
>> @@ -639,13 +639,17 @@ static size_t omap_sham_append_sg(struct omap_sham_reqctx *ctx)
>>  	const u8 *vaddr;
>>  
>>  	while (ctx->sg) {
>> -		vaddr = kmap_atomic(sg_page(ctx->sg));
>> +		if (PageHighMem(sg_page(ctx->sg)))
>> +			vaddr = kmap_atomic(sg_page(ctx->sg));
>> +		else
>> +			vaddr = sg_virt(ctx->sg);
> 
> This is completely bogus.  kmap_atomic should be identical to
> sg_virt(sg_page()) for the lowmem case.
> 
> So either your architecture is broken (because the same problem
> would obviously affect the core crypto code which does exactly
> the same thing), or there is some other bug causing the selftest
> to fail.
Oops my bad. You are right.
Here the problem is sg->offset is not being added to vaddr.
sg_virt gives page address + sg->offset but
kmap_atomic gives only the page address.

Ill update and repost the patch.

Thanks and regards,
Lokesh
> 
> Cheers,
>