[Fwd: [Bug 3003] New: might_sleep warning when setting up IPSec with IPCOMP]

[Fwd: [Bug 3003] New: might_sleep warning when setting up IPSec with IPCOMP]

[Nivedita Singhvi]

James, Andrew,

Looks like deflate_comp_init() is not calling
__vmalloc in a kosher way.

We are not in softirq, so deflate_gfp() is going
to return GFP_KERNEL, so this is essentially
__vmalloc() with GFP_KERNEL|__GFP_HIGHMEM, PAGE_KERNEL.

Should we be using __vmalloc here, or is this a vm issue?

thanks,
Nivedita

-------- Original Message --------
Subject: [Bug 3003] New: might_sleep warning when setting up IPSec with IPCOMP
Date: Fri, 2 Jul 2004 10:32:44 -0700
From: bugme-daemon@osdl.org
To: niv@us.ibm.com

http://bugme.osdl.org/show_bug.cgi?id=3003

            Summary: might_sleep warning when setting up IPSec with IPCOMP
     Kernel Version: 2.6.7
             Status: NEW
           Severity: normal
              Owner: niv@us.ibm.com
          Submitter: christophe@saout.de
                 CC: christophe@saout.de


Linux 2.6.7 on x86, UP preempt

Using OpenSWAN for IPSec tunnel with IP-Compression enabled.

Often gives the following warning shortly after a connection has been set up:

I'm not exactly sure whether this is cryptoapi or the compression module or
IPCOMP's fault.

Jul  2 18:39:04 websrv Debug: sleeping function called from invalid context at
mm/slab.c:1994
Jul  2 18:39:04 websrv in_atomic():1, irqs_disabled():0
Jul  2 18:39:04 websrv [<c0106c37>] dump_stack+0x17/0x20
Jul  2 18:39:04 websrv [<c0118fb4>] __might_sleep+0xb4/0xe0
Jul  2 18:39:04 websrv [<c0139fbc>] kmem_cache_alloc+0x5c/0x60
Jul  2 18:39:04 websrv [<c0147bc0>] __get_vm_area+0x20/0xf0
Jul  2 18:39:04 websrv [<c0147cb4>] get_vm_area+0x24/0x30
Jul  2 18:39:04 websrv [<c0147f1c>] __vmalloc+0x3c/0x100
Jul  2 18:39:04 websrv [<c01ecdca>] deflate_comp_init+0x4a/0xf0
Jul  2 18:39:04 websrv [<c01ecf44>] deflate_compress+0x24/0x80
Jul  2 18:39:04 websrv [<c01e8a84>] crypto_compress+0x24/0x30
Jul  2 18:39:04 websrv [<c031744c>] ipcomp_compress+0x6c/0x110
Jul  2 18:39:04 websrv [<c0317681>] ipcomp_output+0xc1/0x370
Jul  2 18:39:04 websrv [<c02e684e>] dst_output+0xe/0x30
Jul  2 18:39:04 websrv [<c02d684a>] nf_hook_slow+0xfa/0x170
Jul  2 18:39:04 websrv [<c02e4d12>] ip_queue_xmit+0x452/0x540
Jul  2 18:39:04 websrv [<c02f48af>] tcp_transmit_skb+0x3ef/0x660
Jul  2 18:39:04 websrv [<c02f558b>] tcp_write_xmit+0x15b/0x2c0
Jul  2 18:39:04 websrv [<c02ea255>] tcp_sendmsg+0x4e5/0x1030
Jul  2 18:39:04 websrv [<c0309ec7>] inet_sendmsg+0x47/0x60
Jul  2 18:39:04 websrv [<c02c5fee>] sock_aio_write+0xae/0xd0
Jul  2 18:39:04 websrv [<c014d1b6>] do_sync_write+0x76/0xb0
Jul  2 18:39:04 websrv [<c014d2bb>] vfs_write+0xcb/0xf0
Jul  2 18:39:04 websrv [<c014d375>] sys_write+0x35/0x60
Jul  2 18:39:04 websrv [<c0105e3d>] sysenter_past_esp+0x52/0x71

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

Re: [Fwd: [Bug 3003] New: might_sleep warning when setting up IPSec with IPCOMP]

[Christophe Saout]

[-- Attachment #1: Type: text/plain, Size: 571 bytes --]

Am Fr, den 02.07.2004 um 10:56 Uhr -0700 schrieb Nivedita Singhvi:

> Looks like deflate_comp_init() is not calling
> __vmalloc in a kosher way.
>
> [...]
> Jul  2 18:39:04 websrv [<c0139fbc>] kmem_cache_alloc+0x5c/0x60
> Jul  2 18:39:04 websrv [<c0147bc0>] __get_vm_area+0x20/0xf0
> Jul  2 18:39:04 websrv [<c0147cb4>] get_vm_area+0x24/0x30
> Jul  2 18:39:04 websrv [<c0147f1c>] __vmalloc+0x3c/0x100
> [...]

__get_vm_area does a

area = kmalloc(sizeof(*area), GFP_KERNEL);

So it will produce a warning no matter what flags are passed to
__vmalloc.


[-- Attachment #2: Dies ist ein digital signierter Nachrichtenteil --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [Fwd: [Bug 3003] New: might_sleep warning when setting up IPSec with IPCOMP]

[Nivedita Singhvi]

Nivedita Singhvi wrote:

> James, Andrew,
> 
> Looks like deflate_comp_init() is not calling
> __vmalloc in a kosher way.

> Jul  2 18:39:04 websrv Debug: sleeping function called from invalid 
> context at
> mm/slab.c:1994
> Jul  2 18:39:04 websrv in_atomic():1, irqs_disabled():0
> Jul  2 18:39:04 websrv [<c0106c37>] dump_stack+0x17/0x20
> Jul  2 18:39:04 websrv [<c0118fb4>] __might_sleep+0xb4/0xe0
> Jul  2 18:39:04 websrv [<c0139fbc>] kmem_cache_alloc+0x5c/0x60
> Jul  2 18:39:04 websrv [<c0147bc0>] __get_vm_area+0x20/0xf0
> Jul  2 18:39:04 websrv [<c0147cb4>] get_vm_area+0x24/0x30
> Jul  2 18:39:04 websrv [<c0147f1c>] __vmalloc+0x3c/0x100
> Jul  2 18:39:04 websrv [<c01ecdca>] deflate_comp_init+0x4a/0xf0
> Jul  2 18:39:04 websrv [<c01ecf44>] deflate_compress+0x24/0x80
> Jul  2 18:39:04 websrv [<c01e8a84>] crypto_compress+0x24/0x30
> Jul  2 18:39:04 websrv [<c031744c>] ipcomp_compress+0x6c/0x110
> Jul  2 18:39:04 websrv [<c0317681>] ipcomp_output+0xc1/0x370

We are grabbing dst->xfrm lock in ipcomp_output(),
and have it held when we call ipcomp_compress().

Is that the issue? I don't have the crypto module
code, but in_atomic() will be true.


thanks,
Nivedita

Re: [Fwd: [Bug 3003] New: might_sleep warning when setting up IPSec with IPCOMP]

[Christophe Saout]

[-- Attachment #1: Type: text/plain, Size: 799 bytes --]

Am Fr, den 02.07.2004 um 14:27 Uhr -0700 schrieb Nivedita Singhvi:

> We are grabbing dst->xfrm lock in ipcomp_output(),
> and have it held when we call ipcomp_compress().
> 
> Is that the issue? I don't have the crypto module
> code, but in_atomic() will be true.

Yes. But the code might also be called from softirq context, when a
packed from the NIC gets handled or when a slot in the queue becomes
free. (I've also got warnings from those two cases in my logs)

The compress/decompress calls should be able to be run from softirq
(atomic) context just like encrypt/decrypt.

I'm just wondering, why does deflate_compress call deflate_comp_init
when it is called the first time, but deflate_init is a noop? Shouldn't
the deflate_comp_init call just be moved to deflate_init?


[-- Attachment #2: Dies ist ein digital signierter Nachrichtenteil --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [Fwd: [Bug 3003] New: might_sleep warning when setting up IPSec with IPCOMP]

[Andrew Morton]

Nivedita Singhvi <niv@us.ibm.com> wrote:
>
> Nivedita Singhvi wrote:
> 
> > James, Andrew,
> > 
> > Looks like deflate_comp_init() is not calling
> > __vmalloc in a kosher way.
> 
> > Jul  2 18:39:04 websrv Debug: sleeping function called from invalid 
> > context at
> > mm/slab.c:1994
> > Jul  2 18:39:04 websrv in_atomic():1, irqs_disabled():0
> > Jul  2 18:39:04 websrv [<c0106c37>] dump_stack+0x17/0x20
> > Jul  2 18:39:04 websrv [<c0118fb4>] __might_sleep+0xb4/0xe0
> > Jul  2 18:39:04 websrv [<c0139fbc>] kmem_cache_alloc+0x5c/0x60
> > Jul  2 18:39:04 websrv [<c0147bc0>] __get_vm_area+0x20/0xf0
> > Jul  2 18:39:04 websrv [<c0147cb4>] get_vm_area+0x24/0x30
> > Jul  2 18:39:04 websrv [<c0147f1c>] __vmalloc+0x3c/0x100
> > Jul  2 18:39:04 websrv [<c01ecdca>] deflate_comp_init+0x4a/0xf0
> > Jul  2 18:39:04 websrv [<c01ecf44>] deflate_compress+0x24/0x80
> > Jul  2 18:39:04 websrv [<c01e8a84>] crypto_compress+0x24/0x30
> > Jul  2 18:39:04 websrv [<c031744c>] ipcomp_compress+0x6c/0x110
> > Jul  2 18:39:04 websrv [<c0317681>] ipcomp_output+0xc1/0x370
> 
> We are grabbing dst->xfrm lock in ipcomp_output(),
> and have it held when we call ipcomp_compress().
> 
> Is that the issue? I don't have the crypto module
> code, but in_atomic() will be true.
> 

Yes, that is the issue.

>From a design point-of-view, it is not idiomatic for deflate_compress() to
be doing this magical lazy initialisation.  It would be better if the
client of the deflate.c code were to explicitly initialise the context
before diving in and using it.

	/*
	 * Lazy initialization to make interface simple without allocating
	 * un-needed workspaces.  Thus can be called in softirq context.
	 */
	static int deflate_comp_init(struct deflate_ctx *ctx)

Well no.  Those games with deflate_gfp() really need to go away. 
in_atomic() works OK if CONFIG_PREEMPT is enabled.  But with
CONFIG_PREEMPT=n, in_atomic() returns false inside spinlock.  And
in_atomic()'s return value is unaffected by local_irq_disable().

This all needs to be redesigned, sorry.

Re: [Fwd: [Bug 3003] New: might_sleep warning when setting up IPSec with IPCOMP]

[James Morris]

On Fri, 2 Jul 2004, Andrew Morton wrote:

> Well no.  Those games with deflate_gfp() really need to go away. 
> in_atomic() works OK if CONFIG_PREEMPT is enabled.  But with
> CONFIG_PREEMPT=n, in_atomic() returns false inside spinlock.  And
> in_atomic()'s return value is unaffected by local_irq_disable().
> 
> This all needs to be redesigned, sorry.

Ok, fixed.  Lazy allocation is gone.

Please apply.

Signed-off-by: James Morris <jmorris@redhat.com>


diff -urN -X dontdiff linux-2.6.7-bk20.o/crypto/deflate.c linux-2.6.7-bk20.w/crypto/deflate.c
--- linux-2.6.7-bk20.o/crypto/deflate.c	2004-06-16 01:20:04.000000000 -0400
+++ linux-2.6.7-bk20.w/crypto/deflate.c	2004-07-09 00:38:43.137189624 -0400
@@ -39,44 +39,16 @@
 #define DEFLATE_DEF_MEMLEVEL		MAX_MEM_LEVEL
 
 struct deflate_ctx {
-	int comp_initialized;
-	int decomp_initialized;
 	struct z_stream_s comp_stream;
 	struct z_stream_s decomp_stream;
 };
 
-static inline int deflate_gfp(void)
-{
-	return in_softirq() ? GFP_ATOMIC : GFP_KERNEL;
-}
-
-static int deflate_init(void *ctx)
-{
-	return 0;
-}
-
-static void deflate_exit(void *ctx)
-{
-	struct deflate_ctx *dctx = ctx;
-
-	if (dctx->comp_initialized)
-		vfree(dctx->comp_stream.workspace);
-	if (dctx->decomp_initialized)
-		kfree(dctx->decomp_stream.workspace);
-}
-
-/*
- * Lazy initialization to make interface simple without allocating
- * un-needed workspaces.  Thus can be called in softirq context.
- */
 static int deflate_comp_init(struct deflate_ctx *ctx)
 {
 	int ret = 0;
 	struct z_stream_s *stream = &ctx->comp_stream;
 
-	stream->workspace = __vmalloc(zlib_deflate_workspacesize(),
-	                              deflate_gfp()|__GFP_HIGHMEM,
-	                              PAGE_KERNEL);
+	stream->workspace = vmalloc(zlib_deflate_workspacesize());
 	if (!stream->workspace ) {
 		ret = -ENOMEM;
 		goto out;
@@ -89,7 +61,6 @@
 		ret = -EINVAL;
 		goto out_free;
 	}
-	ctx->comp_initialized = 1;
 out:	
 	return ret;
 out_free:
@@ -102,8 +73,7 @@
 	int ret = 0;
 	struct z_stream_s *stream = &ctx->decomp_stream;
 
-	stream->workspace = kmalloc(zlib_inflate_workspacesize(),
-	                            deflate_gfp());
+	stream->workspace = kmalloc(zlib_inflate_workspacesize(), GFP_KERNEL);
 	if (!stream->workspace ) {
 		ret = -ENOMEM;
 		goto out;
@@ -114,7 +84,6 @@
 		ret = -EINVAL;
 		goto out_free;
 	}
-	ctx->decomp_initialized = 1;
 out:
 	return ret;
 out_free:
@@ -122,6 +91,36 @@
 	goto out;
 }
 
+static void deflate_comp_exit(struct deflate_ctx *ctx)
+{
+	vfree(ctx->comp_stream.workspace);
+}
+
+static void deflate_decomp_exit(struct deflate_ctx *ctx)
+{
+	kfree(ctx->decomp_stream.workspace);
+}
+
+static int deflate_init(void *ctx)
+{
+	int ret;
+	
+	ret = deflate_comp_init(ctx);
+	if (ret)
+		goto out;
+	ret = deflate_decomp_init(ctx);
+	if (ret)
+		deflate_comp_exit(ctx);
+out:
+	return ret;
+}
+
+static void deflate_exit(void *ctx)
+{
+	deflate_comp_exit(ctx);
+	deflate_decomp_exit(ctx);
+}
+
 static int deflate_compress(void *ctx, const u8 *src, unsigned int slen,
 	                    u8 *dst, unsigned int *dlen)
 {
@@ -129,12 +128,6 @@
 	struct deflate_ctx *dctx = ctx;
 	struct z_stream_s *stream = &dctx->comp_stream;
 
-	if (!dctx->comp_initialized) {
-		ret = deflate_comp_init(dctx);
-		if (ret)
-			goto out;
-	}
-
 	ret = zlib_deflateReset(stream);
 	if (ret != Z_OK) {
 		ret = -EINVAL;
@@ -165,12 +158,6 @@
 	struct deflate_ctx *dctx = ctx;
 	struct z_stream_s *stream = &dctx->decomp_stream;
 
-	if (!dctx->decomp_initialized) {
-		ret = deflate_decomp_init(dctx);
-		if (ret)
-			goto out;
-	}
-
 	ret = zlib_inflateReset(stream);
 	if (ret != Z_OK) {
 		ret = -EINVAL;

Re: [Fwd: [Bug 3003] New: might_sleep warning when setting up IPSec with IPCOMP]

[David S. Miller]

On Fri, 9 Jul 2004 00:20:56 -0400 (EDT)
James Morris <jmorris@redhat.com> wrote:

> On Fri, 2 Jul 2004, Andrew Morton wrote:
> 
> > Well no.  Those games with deflate_gfp() really need to go away. 
> > in_atomic() works OK if CONFIG_PREEMPT is enabled.  But with
> > CONFIG_PREEMPT=n, in_atomic() returns false inside spinlock.  And
> > in_atomic()'s return value is unaffected by local_irq_disable().
> > 
> > This all needs to be redesigned, sorry.
> 
> Ok, fixed.  Lazy allocation is gone.
> 
> Please apply.

Applied.