| Message ID | 1393957854-10961-1-git-send-email-joelf@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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,
> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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, > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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;
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(-)