[PATCH v2] crypto: omap-sham: kmap SG pages before appending

[PATCH v2] crypto: omap-sham: kmap SG pages before appending

[Joel Fernandes]

HIGHMEM pages may not be mapped so we must kmap them before accessing.
This resolves a random OOPs error that was showing up during OpenSSL SHA tests.

Signed-off-by: Joel Fernandes <joelf@ti.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
---
v2 changes:
	- don't check for HIGHMEM, kmap/kunmap do them anyway (Thanks Herbert).

 drivers/crypto/omap-sham.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/omap-sham.c b/drivers/crypto/omap-sham.c
index e45aaaf..207eac1 100644
--- a/drivers/crypto/omap-sham.c
+++ b/drivers/crypto/omap-sham.c
@@ -636,11 +636,17 @@ static size_t omap_sham_append_buffer(struct omap_sham_reqctx *ctx,
 static size_t omap_sham_append_sg(struct omap_sham_reqctx *ctx)
 {
 	size_t count;
+	const u8 *vaddr;
 
 	while (ctx->sg) {
+		vaddr = kmap(sg_page(ctx->sg));
+
 		count = omap_sham_append_buffer(ctx,
-				sg_virt(ctx->sg) + ctx->offset,
+				vaddr + ctx->offset,
 				ctx->sg->length - ctx->offset);
+
+		kunmap((void *)vaddr);
+
 		if (!count)
 			break;
 		ctx->offset += count;
-- 
1.7.9.5

[PATCH v2] crypto: omap-sham: kmap SG pages before appending

[Joel Fernandes]

HIGHMEM pages may not be mapped so we must kmap them before accessing.
This resolves a random OOPs error that was showing up during OpenSSL SHA tests.

Signed-off-by: Joel Fernandes <joelf@ti.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
---
v2 changes:
	- don't check for HIGHMEM, kmap/kunmap do them anyway (Thanks Herbert).

 drivers/crypto/omap-sham.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/omap-sham.c b/drivers/crypto/omap-sham.c
index e45aaaf..207eac1 100644
--- a/drivers/crypto/omap-sham.c
+++ b/drivers/crypto/omap-sham.c
@@ -636,11 +636,17 @@ static size_t omap_sham_append_buffer(struct omap_sham_reqctx *ctx,
 static size_t omap_sham_append_sg(struct omap_sham_reqctx *ctx)
 {
 	size_t count;
+	const u8 *vaddr;
 
 	while (ctx->sg) {
+		vaddr = kmap(sg_page(ctx->sg));
+
 		count = omap_sham_append_buffer(ctx,
-				sg_virt(ctx->sg) + ctx->offset,
+				vaddr + ctx->offset,
 				ctx->sg->length - ctx->offset);
+
+		kunmap((void *)vaddr);
+
 		if (!count)
 			break;
 		ctx->offset += count;
-- 
1.7.9.5

Re: [PATCH v2] crypto: omap-sham: kmap SG pages before appending

[Herbert Xu]

On Tue, Mar 04, 2014 at 12:30:54PM -0600, Joel Fernandes wrote:
> HIGHMEM pages may not be mapped so we must kmap them before accessing.
> This resolves a random OOPs error that was showing up during OpenSSL SHA tests.
> 
> Signed-off-by: Joel Fernandes <joelf@ti.com>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> ---
> v2 changes:
> 	- don't check for HIGHMEM, kmap/kunmap do them anyway (Thanks Herbert).
> 
>  drivers/crypto/omap-sham.c |    8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/crypto/omap-sham.c b/drivers/crypto/omap-sham.c
> index e45aaaf..207eac1 100644
> --- a/drivers/crypto/omap-sham.c
> +++ b/drivers/crypto/omap-sham.c
> @@ -636,11 +636,17 @@ static size_t omap_sham_append_buffer(struct omap_sham_reqctx *ctx,
>  static size_t omap_sham_append_sg(struct omap_sham_reqctx *ctx)
>  {
>  	size_t count;
> +	const u8 *vaddr;
>  
>  	while (ctx->sg) {
> +		vaddr = kmap(sg_page(ctx->sg));

Are you sure you can safely use kmap here as opposed to kmap_atomic?

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

Re: [PATCH v2] crypto: omap-sham: kmap SG pages before appending

[Fernandes, Joel]

> On Mar 4, 2014, at 8:00 PM, "Herbert Xu" <herbert@gondor.apana.org.au> wrote:
> 
>> On Tue, Mar 04, 2014 at 12:30:54PM -0600, Joel Fernandes wrote:
>> HIGHMEM pages may not be mapped so we must kmap them before accessing.
>> This resolves a random OOPs error that was showing up during OpenSSL SHA tests.
>> 
>> Signed-off-by: Joel Fernandes <joelf@ti.com>
>> Cc: Herbert Xu <herbert@gondor.apana.org.au>
>> ---
>> v2 changes:
>>    - don't check for HIGHMEM, kmap/kunmap do them anyway (Thanks Herbert).
>> 
>> drivers/crypto/omap-sham.c |    8 +++++++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/crypto/omap-sham.c b/drivers/crypto/omap-sham.c
>> index e45aaaf..207eac1 100644
>> --- a/drivers/crypto/omap-sham.c
>> +++ b/drivers/crypto/omap-sham.c
>> @@ -636,11 +636,17 @@ static size_t omap_sham_append_buffer(struct omap_sham_reqctx *ctx,
>> static size_t omap_sham_append_sg(struct omap_sham_reqctx *ctx)
>> {
>>    size_t count;
>> +    const u8 *vaddr;
>> 
>>    while (ctx->sg) {
>> +        vaddr = kmap(sg_page(ctx->sg));
> 
> Are you sure you can safely use kmap here as opposed to kmap_atomic?

Yes I made sure it is not called in interrupt context and also tested the patch.

Regards,
-Joel

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

Re: [PATCH v2] crypto: omap-sham: kmap SG pages before appending

[Herbert Xu]

On Wed, Mar 05, 2014 at 04:18:12AM +0000, Fernandes, Joel wrote:
> 
> 
> > On Mar 4, 2014, at 8:00 PM, "Herbert Xu" <herbert@gondor.apana.org.au> wrote:
> > 
> >> On Tue, Mar 04, 2014 at 12:30:54PM -0600, Joel Fernandes wrote:
> >> HIGHMEM pages may not be mapped so we must kmap them before accessing.
> >> This resolves a random OOPs error that was showing up during OpenSSL SHA tests.
> >> 
> >> Signed-off-by: Joel Fernandes <joelf@ti.com>
> >> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> >> ---
> >> v2 changes:
> >>    - don't check for HIGHMEM, kmap/kunmap do them anyway (Thanks Herbert).
> >> 
> >> drivers/crypto/omap-sham.c |    8 +++++++-
> >> 1 file changed, 7 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/crypto/omap-sham.c b/drivers/crypto/omap-sham.c
> >> index e45aaaf..207eac1 100644
> >> --- a/drivers/crypto/omap-sham.c
> >> +++ b/drivers/crypto/omap-sham.c
> >> @@ -636,11 +636,17 @@ static size_t omap_sham_append_buffer(struct omap_sham_reqctx *ctx,
> >> static size_t omap_sham_append_sg(struct omap_sham_reqctx *ctx)
> >> {
> >>    size_t count;
> >> +    const u8 *vaddr;
> >> 
> >>    while (ctx->sg) {
> >> +        vaddr = kmap(sg_page(ctx->sg));
> > 
> > Are you sure you can safely use kmap here as opposed to kmap_atomic?
> 
> Yes I made sure it is not called in interrupt context and also tested the patch.

What about omap_sham_update => omap_sham_append_sg? As the former
can be called in softirq context what is preventing it from calling
kmap?

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 v2] crypto: omap-sham: kmap SG pages before appending

[Joel Fernandes]

On 03/05/2014 01:11 AM, Herbert Xu wrote:
> On Wed, Mar 05, 2014 at 04:18:12AM +0000, Fernandes, Joel wrote:
>>
>>
>>> On Mar 4, 2014, at 8:00 PM, "Herbert Xu" <herbert@gondor.apana.org.au> wrote:
>>>
>>>> On Tue, Mar 04, 2014 at 12:30:54PM -0600, Joel Fernandes wrote:
>>>> HIGHMEM pages may not be mapped so we must kmap them before accessing.
>>>> This resolves a random OOPs error that was showing up during OpenSSL SHA tests.
>>>>
>>>> Signed-off-by: Joel Fernandes <joelf@ti.com>
>>>> Cc: Herbert Xu <herbert@gondor.apana.org.au>
>>>> ---
>>>> v2 changes:
>>>>    - don't check for HIGHMEM, kmap/kunmap do them anyway (Thanks Herbert).
>>>>
>>>> drivers/crypto/omap-sham.c |    8 +++++++-
>>>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/crypto/omap-sham.c b/drivers/crypto/omap-sham.c
>>>> index e45aaaf..207eac1 100644
>>>> --- a/drivers/crypto/omap-sham.c
>>>> +++ b/drivers/crypto/omap-sham.c
>>>> @@ -636,11 +636,17 @@ static size_t omap_sham_append_buffer(struct omap_sham_reqctx *ctx,
>>>> static size_t omap_sham_append_sg(struct omap_sham_reqctx *ctx)
>>>> {
>>>>    size_t count;
>>>> +    const u8 *vaddr;
>>>>
>>>>    while (ctx->sg) {
>>>> +        vaddr = kmap(sg_page(ctx->sg));
>>>
>>> Are you sure you can safely use kmap here as opposed to kmap_atomic?
>>
>> Yes I made sure it is not called in interrupt context and also tested the patch.
> 
> What about omap_sham_update => omap_sham_append_sg? As the former
> can be called in softirq context what is preventing it from calling
> kmap?

In my testing it wasn't called from softirq, I'm using cryptodev which is
called from openssl through ioctl, here is a traceback (sorry about wrap):

[<bf8010bb>] (cryptodev_ioctl+0x282/0x59c [cryptodev])
[  220.015390] [<bf8010bb>] (cryptodev_ioctl+0x282/0x59c [cryptodev]) from
[<c00cdaa5>] (do_vfs_ioctl+0x61/0x41c)
[  220.025909] [<c00cdaa5>] (do_vfs_ioctl+0x61/0x41c) from [<c00cdeab>]
(SyS_ioctl+0x4b/0x50)
[  220.034602] [<c00cdeab>] (SyS_ioctl+0x4b/0x50) from [<c000cf01>]
(ret_fast_syscall+0x1/0x46)
[  220.045254] CPU: 0 PID: 1798 Comm: openssl Tainted: G           O
3.12.9-00581-g5fa084c-dirty #32
[  220.054636] [<c0012d05>] (unwind_backtrace+0x1/0x9c) from [<c000fa6d>]
(show_stack+0x11/0x14)
[  220.063619] [<c000fa6d>] (show_stack+0x11/0x14) from [<c0342edb>]
(dump_stack+0x4b/0x60)
[  220.072144] [<c0342edb>] (dump_stack+0x4b/0x60) from [<c02a1b3f>]
(omap_sham_update+0x1f/0xa4)
[  220.081214] [<c02a1b3f>] (omap_sham_update+0x1f/0xa4) from [<bf801f67>]
(cryptodev_hash_update+0x26/0x80 [cryptodev])


Could you suggest, what other place is update() called in softirq context?
I may be missing something.

Thanks,
-Joel

> 
> Cheers,
> 

Re: [PATCH v2] crypto: omap-sham: kmap SG pages before appending

[Herbert Xu]

On Wed, Mar 05, 2014 at 09:42:38AM -0600, Joel Fernandes wrote:
>
> In my testing it wasn't called from softirq, I'm using cryptodev which is
> called from openssl through ioctl, here is a traceback (sorry about wrap):
> 
> [<bf8010bb>] (cryptodev_ioctl+0x282/0x59c [cryptodev])
> [  220.015390] [<bf8010bb>] (cryptodev_ioctl+0x282/0x59c [cryptodev]) from
> [<c00cdaa5>] (do_vfs_ioctl+0x61/0x41c)
> [  220.025909] [<c00cdaa5>] (do_vfs_ioctl+0x61/0x41c) from [<c00cdeab>]
> (SyS_ioctl+0x4b/0x50)
> [  220.034602] [<c00cdeab>] (SyS_ioctl+0x4b/0x50) from [<c000cf01>]
> (ret_fast_syscall+0x1/0x46)
> [  220.045254] CPU: 0 PID: 1798 Comm: openssl Tainted: G           O
> 3.12.9-00581-g5fa084c-dirty #32
> [  220.054636] [<c0012d05>] (unwind_backtrace+0x1/0x9c) from [<c000fa6d>]
> (show_stack+0x11/0x14)
> [  220.063619] [<c000fa6d>] (show_stack+0x11/0x14) from [<c0342edb>]
> (dump_stack+0x4b/0x60)
> [  220.072144] [<c0342edb>] (dump_stack+0x4b/0x60) from [<c02a1b3f>]
> (omap_sham_update+0x1f/0xa4)
> [  220.081214] [<c02a1b3f>] (omap_sham_update+0x1f/0xa4) from [<bf801f67>]
> (cryptodev_hash_update+0x26/0x80 [cryptodev])
> 
> 
> Could you suggest, what other place is update() called in softirq context?
> I may be missing something.

IPsec may call this from softirq context.

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