next-20090220: XFS, IMA: BUG: sleeping function called from invalid context at mm/slub.c:1613

next-20090220: XFS, IMA: BUG: sleeping function called from invalid context at mm/slub.c:1613

[Alexander Beregalov]

Hi

I have applied the following patch from Mimi Zohar
http://marc.info/?l=linux-next&m=123509665514552

That is why it is dirty.
The kernel can not boot without it when IMA is enabled.


BUG: sleeping function called from invalid context at mm/slub.c:1613
in_atomic(): 1, irqs_disabled(): 0, pid: 1514, name: mkdir
3 locks held by mkdir/1514:
 #0:  (&type->i_mutex_dir_key#2/1){+.+.+.}, at: [<ffffffff802d3460>]
lookup_create+0x30/0xd0
 #1:  (&(&ip->i_lock)->mr_lock/1){+.+.+.}, at: [<ffffffff803ca77f>]
xfs_ilock+0xdf/0x120
 #2:  (&pag->pag_ici_lock){++++.+}, at: [<ffffffff803caed6>]
xfs_iget+0x156/0x650
Pid: 1514, comm: mkdir Not tainted 2.6.29-rc5-next-20090220-dirty #1
Call Trace:
 [<ffffffff8026ad33>] ? __debug_show_held_locks+0x13/0x30
 [<ffffffff80234fe5>] __might_sleep+0x105/0x140
 [<ffffffff802c6191>] kmem_cache_alloc+0xd1/0x100
 [<ffffffff8045fb29>] ima_iint_insert+0x49/0xf0
 [<ffffffff8045fbed>] ima_inode_alloc+0x1d/0x30
 [<ffffffff802dfa5f>] inode_init_always+0xaf/0x250
 [<ffffffff803caf86>] xfs_iget+0x206/0x650
 [<ffffffff803eb8e8>] xfs_trans_iget+0x208/0x250
 [<ffffffff803ce571>] xfs_ialloc+0xc1/0x700
 [<ffffffff803ec6f9>] xfs_dir_ialloc+0xa9/0x340
 [<ffffffff8025e009>] ? down_write_nested+0x79/0x90
 [<ffffffff803ef0c1>] xfs_create+0x3e1/0x690
 [<ffffffff803fb813>] xfs_vn_mknod+0x63/0xf0
 [<ffffffff803fb8ae>] xfs_vn_mkdir+0xe/0x10
 [<ffffffff802d487c>] vfs_mkdir+0x8c/0xd0
 [<ffffffff802d6a56>] sys_mkdirat+0x106/0x120
 [<ffffffff8020bc0c>] ? sysret_check+0x27/0x62
 [<ffffffff8026c79d>] ? trace_hardirqs_on_caller+0x17d/0x1e0
 [<ffffffff802d6a83>] sys_mkdir+0x13/0x20
 [<ffffffff8020bbdb>] system_call_fastpath+0x16/0x1b
BUG: sleeping function called from invalid context at mm/slub.c:1613
in_atomic(): 1, irqs_disabled(): 0, pid: 1514, name: mkdir
3 locks held by mkdir/1514:
 #0:  (&type->i_mutex_dir_key#2/1){+.+.+.}, at: [<ffffffff802d3460>]
lookup_create+0x30/0xd0
 #1:  (&(&ip->i_lock)->mr_lock/1){+.+.+.}, at: [<ffffffff803ca77f>]
xfs_ilock+0xdf/0x120
 #2:  (&pag->pag_ici_lock){++++.+}, at: [<ffffffff803caed6>]
xfs_iget+0x156/0x650
Pid: 1514, comm: mkdir Not tainted 2.6.29-rc5-next-20090220-dirty #1
Call Trace:
 [<ffffffff8026ad33>] ? __debug_show_held_locks+0x13/0x30
 [<ffffffff80234fe5>] __might_sleep+0x105/0x140
 [<ffffffff802c6191>] kmem_cache_alloc+0xd1/0x100
 [<ffffffff8048131a>] radix_tree_preload+0x6a/0xf0
 [<ffffffff8045fb3b>] ima_iint_insert+0x5b/0xf0
 [<ffffffff8045fbed>] ima_inode_alloc+0x1d/0x30
 [<ffffffff802dfa5f>] inode_init_always+0xaf/0x250
 [<ffffffff803caf86>] xfs_iget+0x206/0x650
 [<ffffffff803eb8e8>] xfs_trans_iget+0x208/0x250
 [<ffffffff803ce571>] xfs_ialloc+0xc1/0x700
 [<ffffffff803ec6f9>] xfs_dir_ialloc+0xa9/0x340
 [<ffffffff8025e009>] ? down_write_nested+0x79/0x90
 [<ffffffff803ef0c1>] xfs_create+0x3e1/0x690
 [<ffffffff803fb813>] xfs_vn_mknod+0x63/0xf0
 [<ffffffff803fb8ae>] xfs_vn_mkdir+0xe/0x10
 [<ffffffff802d487c>] vfs_mkdir+0x8c/0xd0
 [<ffffffff802d6a56>] sys_mkdirat+0x106/0x120
 [<ffffffff8020bc0c>] ? sysret_check+0x27/0x62
 [<ffffffff8026c79d>] ? trace_hardirqs_on_caller+0x17d/0x1e0
 [<ffffffff802d6a83>] sys_mkdir+0x13/0x20
 [<ffffffff8020bbdb>] system_call_fastpath+0x16/0x1b

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: next-20090220: XFS, IMA: BUG: sleeping function called from invalid context at mm/slub.c:1613

[Andrew Morton]

On Fri, 20 Feb 2009 14:00:21 +0300
Alexander Beregalov <a.beregalov@gmail.com> wrote:

> Hi
> 
> I have applied the following patch from Mimi Zohar
> http://marc.info/?l=linux-next&m=123509665514552
> 
> That is why it is dirty.
> The kernel can not boot without it when IMA is enabled.
> 
> 
> BUG: sleeping function called from invalid context at mm/slub.c:1613
> in_atomic(): 1, irqs_disabled(): 0, pid: 1514, name: mkdir
> 3 locks held by mkdir/1514:
>  #0:  (&type->i_mutex_dir_key#2/1){+.+.+.}, at: [<ffffffff802d3460>]
> lookup_create+0x30/0xd0
>  #1:  (&(&ip->i_lock)->mr_lock/1){+.+.+.}, at: [<ffffffff803ca77f>]
> xfs_ilock+0xdf/0x120
>  #2:  (&pag->pag_ici_lock){++++.+}, at: [<ffffffff803caed6>]
> xfs_iget+0x156/0x650
> Pid: 1514, comm: mkdir Not tainted 2.6.29-rc5-next-20090220-dirty #1
> Call Trace:
>  [<ffffffff8026ad33>] ? __debug_show_held_locks+0x13/0x30
>  [<ffffffff80234fe5>] __might_sleep+0x105/0x140
>  [<ffffffff802c6191>] kmem_cache_alloc+0xd1/0x100
>  [<ffffffff8045fb29>] ima_iint_insert+0x49/0xf0
>  [<ffffffff8045fbed>] ima_inode_alloc+0x1d/0x30
>  [<ffffffff802dfa5f>] inode_init_always+0xaf/0x250
>  [<ffffffff803caf86>] xfs_iget+0x206/0x650
>  [<ffffffff803eb8e8>] xfs_trans_iget+0x208/0x250
>  [<ffffffff803ce571>] xfs_ialloc+0xc1/0x700
>  [<ffffffff803ec6f9>] xfs_dir_ialloc+0xa9/0x340
>  [<ffffffff8025e009>] ? down_write_nested+0x79/0x90
>  [<ffffffff803ef0c1>] xfs_create+0x3e1/0x690
>  [<ffffffff803fb813>] xfs_vn_mknod+0x63/0xf0
>  [<ffffffff803fb8ae>] xfs_vn_mkdir+0xe/0x10
>  [<ffffffff802d487c>] vfs_mkdir+0x8c/0xd0
>  [<ffffffff802d6a56>] sys_mkdirat+0x106/0x120
>  [<ffffffff8020bc0c>] ? sysret_check+0x27/0x62
>  [<ffffffff8026c79d>] ? trace_hardirqs_on_caller+0x17d/0x1e0
>  [<ffffffff802d6a83>] sys_mkdir+0x13/0x20
>  [<ffffffff8020bbdb>] system_call_fastpath+0x16/0x1b
> BUG: sleeping function called from invalid context at mm/slub.c:1613

Please do not be tempted to switch ima_iint_insert() to use GFP_ATOMIC
to "fix" this.

Arguably, ima_iint_insert() should requite that the caller pass in the
gfp_t rather than assuming that GFP_KERNEL can be used.  But that is
not a suitable fix for this bug.

We may need to make that change to ima_iint_insert() anyway, as there's
a good chance that callers will require GFP_NOFS.


But to fix this bug, xfs needs to stop calling inode_init_always()
under read_lock().  Because inode_alloc_security() also can sleep (see
new_inode_smack()).

Also, ima_iint_insert() does a radix_tree_lookup() without holding
ima_iint_lock, which appears to be a bug.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: next-20090220: XFS, IMA: BUG: sleeping function called from invalid context at mm/slub.c:1613

[Mimi Zohar]

On Fri, 2009-02-20 at 12:22 -0800, Andrew Morton wrote: 
> On Fri, 20 Feb 2009 14:00:21 +0300
> Alexander Beregalov <a.beregalov@gmail.com> wrote:
> 
> > Hi
> > 
> > I have applied the following patch from Mimi Zohar
> > http://marc.info/?l=linux-next&m=123509665514552
> > 
> > That is why it is dirty.
> > The kernel can not boot without it when IMA is enabled.

With CONFIG_DEBUG_SG and IMA defined, it couldn't boot.

> > BUG: sleeping function called from invalid context at mm/slub.c:1613
> > in_atomic(): 1, irqs_disabled(): 0, pid: 1514, name: mkdir
> > 3 locks held by mkdir/1514:
> >  #0:  (&type->i_mutex_dir_key#2/1){+.+.+.}, at: [<ffffffff802d3460>]
> > lookup_create+0x30/0xd0
> >  #1:  (&(&ip->i_lock)->mr_lock/1){+.+.+.}, at: [<ffffffff803ca77f>]
> > xfs_ilock+0xdf/0x120
> >  #2:  (&pag->pag_ici_lock){++++.+}, at: [<ffffffff803caed6>]
> > xfs_iget+0x156/0x650
> > Pid: 1514, comm: mkdir Not tainted 2.6.29-rc5-next-20090220-dirty #1
> > Call Trace:
> >  [<ffffffff8026ad33>] ? __debug_show_held_locks+0x13/0x30
> >  [<ffffffff80234fe5>] __might_sleep+0x105/0x140
> >  [<ffffffff802c6191>] kmem_cache_alloc+0xd1/0x100
> >  [<ffffffff8045fb29>] ima_iint_insert+0x49/0xf0
> >  [<ffffffff8045fbed>] ima_inode_alloc+0x1d/0x30
> >  [<ffffffff802dfa5f>] inode_init_always+0xaf/0x250
> >  [<ffffffff803caf86>] xfs_iget+0x206/0x650
> >  [<ffffffff803eb8e8>] xfs_trans_iget+0x208/0x250
> >  [<ffffffff803ce571>] xfs_ialloc+0xc1/0x700
> >  [<ffffffff803ec6f9>] xfs_dir_ialloc+0xa9/0x340
> >  [<ffffffff8025e009>] ? down_write_nested+0x79/0x90
> >  [<ffffffff803ef0c1>] xfs_create+0x3e1/0x690
> >  [<ffffffff803fb813>] xfs_vn_mknod+0x63/0xf0
> >  [<ffffffff803fb8ae>] xfs_vn_mkdir+0xe/0x10
> >  [<ffffffff802d487c>] vfs_mkdir+0x8c/0xd0
> >  [<ffffffff802d6a56>] sys_mkdirat+0x106/0x120
> >  [<ffffffff8020bc0c>] ? sysret_check+0x27/0x62
> >  [<ffffffff8026c79d>] ? trace_hardirqs_on_caller+0x17d/0x1e0
> >  [<ffffffff802d6a83>] sys_mkdir+0x13/0x20
> >  [<ffffffff8020bbdb>] system_call_fastpath+0x16/0x1b
> > BUG: sleeping function called from invalid context at mm/slub.c:1613
> 
> Please do not be tempted to switch ima_iint_insert() to use GFP_ATOMIC
> to "fix" this.
> 
> Arguably, ima_iint_insert() should requite that the caller pass in the
> gfp_t rather than assuming that GFP_KERNEL can be used.  But that is
> not a suitable fix for this bug.
> 
> We may need to make that change to ima_iint_insert() anyway, as there's
> a good chance that callers will require GFP_NOFS.
> 
> 
> But to fix this bug, xfs needs to stop calling inode_init_always()
> under read_lock().  Because inode_alloc_security() also can sleep (see
> new_inode_smack()).
> 
> Also, ima_iint_insert() does a radix_tree_lookup() without holding
> ima_iint_lock, which appears to be a bug.

Thank you. For now, here's a patch to add locking around the
radix_tree_lookup(). I'll look into passing gfp_t.

Mimi

integrity: ima iint radix_tree_lookup locking fix

Based on Andrew Morton's comments:
- add missing locks around radix_tree_lookup in ima_iint_insert()

Signed-off-by: Mimi Zohar <zohar@us.ibm.com>

Index: security-testing-2.6/security/integrity/ima/ima_iint.c
===================================================================
--- security-testing-2.6.orig/security/integrity/ima/ima_iint.c
+++ security-testing-2.6/security/integrity/ima/ima_iint.c
@@ -73,8 +73,10 @@ out:
 	if (rc < 0) {
 		kmem_cache_free(iint_cache, iint);
 		if (rc == -EEXIST) {
+			spin_lock(&ima_iint_lock);
 			iint = radix_tree_lookup(&ima_iint_store,
 						 (unsigned long)inode);
+			spin_unlock(&ima_iint_lock);
 		} else
 			iint = NULL;
 	}




_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: next-20090220: XFS, IMA: BUG: sleeping function called from invalid context at mm/slub.c:1613

[Andrew Morton]

On Fri, 20 Feb 2009 17:16:59 -0500
Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:

> integrity: ima iint radix_tree_lookup locking fix
> 
> Based on Andrew Morton's comments:
> - add missing locks around radix_tree_lookup in ima_iint_insert()
> 
> Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
> 
> Index: security-testing-2.6/security/integrity/ima/ima_iint.c
> ===================================================================
> --- security-testing-2.6.orig/security/integrity/ima/ima_iint.c
> +++ security-testing-2.6/security/integrity/ima/ima_iint.c
> @@ -73,8 +73,10 @@ out:
>  	if (rc < 0) {
>  		kmem_cache_free(iint_cache, iint);
>  		if (rc == -EEXIST) {
> +			spin_lock(&ima_iint_lock);
>  			iint = radix_tree_lookup(&ima_iint_store,
>  						 (unsigned long)inode);
> +			spin_unlock(&ima_iint_lock);
>  		} else
>  			iint = NULL;
>  	}

Can the -EEXIST ever actually happen?

On the inode_init_always() path (at least), I don't think that any
other thread of control can have access to this inode*, so there is no
way in which a race can result in someone else adding this inode
first?


Also, idle question: why does the radix tree exist at all?  Would it
have been possible to just add a `struct ima_iint_cache *' field to the
inode instead?

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: next-20090220: XFS, IMA: BUG: sleeping function called from invalid context at mm/slub.c:1613

[Mimi Zohar]

On Fri, 2009-02-20 at 14:28 -0800, Andrew Morton wrote: 
> On Fri, 20 Feb 2009 17:16:59 -0500
> Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> 
> > integrity: ima iint radix_tree_lookup locking fix
> > 
> > Based on Andrew Morton's comments:
> > - add missing locks around radix_tree_lookup in ima_iint_insert()
> > 
> > Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
> > 
> > Index: security-testing-2.6/security/integrity/ima/ima_iint.c
> > ===================================================================
> > --- security-testing-2.6.orig/security/integrity/ima/ima_iint.c
> > +++ security-testing-2.6/security/integrity/ima/ima_iint.c
> > @@ -73,8 +73,10 @@ out:
> >  	if (rc < 0) {
> >  		kmem_cache_free(iint_cache, iint);
> >  		if (rc == -EEXIST) {
> > +			spin_lock(&ima_iint_lock);
> >  			iint = radix_tree_lookup(&ima_iint_store,
> >  						 (unsigned long)inode);
> > +			spin_unlock(&ima_iint_lock);
> >  		} else
> >  			iint = NULL;
> >  	}
> 
> Can the -EEXIST ever actually happen?

> On the inode_init_always() path (at least), I don't think that any
> other thread of control can have access to this inode*, so there is no
> way in which a race can result in someone else adding this inode
> first?

True, but for those inodes which were allocated before IMA was enabled
and are being allocated in ima_iint_find_insert_get(), it could be an
issue.

> Also, idle question: why does the radix tree exist at all?  Would it
> have been possible to just add a `struct ima_iint_cache *' field to the
> inode instead?

Up until November the iint was defined directly in the inode. This
changed based on Christoph Hellwig's posting
http://lkml.org/lkml/2008/10/14/170 where he said, "bloating the inode
for this is not an option".

Mimi Zohar

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: next-20090220: XFS, IMA: BUG: sleeping function called from invalid context at mm/slub.c:1613

[Christoph Hellwig]

On Fri, Feb 20, 2009 at 12:22:42PM -0800, Andrew Morton wrote:
> But to fix this bug, xfs needs to stop calling inode_init_always()
> under read_lock().  Because inode_alloc_security() also can sleep (see
> new_inode_smack()).

The normal inode initialization path is fine as it's not in atomic
context, but if we relcaim a partially torn down inode in
xfs_iget_cache_hit we call inode_init_always under pag_ici_lock.

I wonder if we should just give up on trying to "recover" such
inodes.  Instead call finish_reclaim on them and restart the inode
cache lookup.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: next-20090220: XFS, IMA: BUG: sleeping function called from invalid context at mm/slub.c:1613

[Dave Chinner]

On Tue, Feb 24, 2009 at 02:14:29PM -0500, Christoph Hellwig wrote:
> On Fri, Feb 20, 2009 at 12:22:42PM -0800, Andrew Morton wrote:
> > But to fix this bug, xfs needs to stop calling inode_init_always()
> > under read_lock().  Because inode_alloc_security() also can sleep (see
> > new_inode_smack()).
> 
> The normal inode initialization path is fine as it's not in atomic
> context, but if we relcaim a partially torn down inode in
> xfs_iget_cache_hit we call inode_init_always under pag_ici_lock.
> 
> I wonder if we should just give up on trying to "recover" such
> inodes.  Instead call finish_reclaim on them and restart the inode
> cache lookup.

The reason for this code is that journalling allows us to reuse the
inode without flushing it to disk. If we reclaim it, then we force
it to be flushed to disk before it can be reused. That will greatly
slow down workloads that create and delete temporary files
repeatedly.

I had a brief look at the code, and I think it can be reworked to
avoid re-initialisation under the pag_ici_lock. I should have some
time this weekend to test a fix....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: next-20090220: XFS, IMA: BUG: sleeping function called from invalid context at mm/slub.c:1613

[Mimi Zohar]

On Fri, 2009-02-20 at 12:22 -0800, Andrew Morton wrote:

> But to fix this bug, xfs needs to stop calling inode_init_always()
> under read_lock().  Because inode_alloc_security() also can sleep (see
> new_inode_smack()).

Yes, even without IMA enabled, with SELINUX, PREEMPT, and DEBUG_PREEMPT
enabled, I'm seeing the problem:

Feb 24 17:37:52 iicu10 kernel: BUG: sleeping function called from invalid context at mm/slub.c:1613
Feb 24 17:37:52 iicu10 kernel: in_atomic(): 1, irqs_disabled(): 0, pid: 3209, name: vi
Feb 24 17:37:52 iicu10 kernel: Pid: 3209, comm: vi Tainted: P           2.6.29-rc6-next-20090224 #5
Feb 24 17:37:52 iicu10 kernel: Call Trace:
Feb 24 17:37:52 iicu10 kernel: [<ffffffff8103d2c1>] __might_sleep+0x120/0x122
Feb 24 17:37:52 iicu10 kernel: [<ffffffff810c9a96>] kmem_cache_alloc+0x32/0xd1
Feb 24 17:37:52 iicu10 kernel: [<ffffffff811563ca>] selinux_inode_alloc_security+0x39/0x90
Feb 24 17:37:52 iicu10 kernel: [<ffffffff8114ef12>] security_inode_alloc+0x1c/0x1e
Feb 24 17:37:52 iicu10 kernel: [<ffffffff810e15b3>] inode_init_always+0xc3/0x1dc
Feb 24 17:37:52 iicu10 kernel: [<ffffffffa0031a34>] xfs_iget+0x14e/0x49e [xfs]
Feb 24 17:37:52 iicu10 kernel: [<ffffffffa00462c2>] xfs_trans_iget+0xa6/0xd4 [xfs]
Feb 24 17:37:52 iicu10 kernel: [<ffffffffa003463c>] xfs_ialloc+0x96/0x517 [xfs]
Feb 24 17:37:52 iicu10 kernel: [<ffffffff8118b5e2>] ? _raw_spin_lock+0x6c/0x112
Feb 24 17:37:52 iicu10 kernel: [<ffffffff8118b5e2>] ? _raw_spin_lock+0x6c/0x112
Feb 24 17:37:52 iicu10 kernel: [<ffffffffa0046bb1>] xfs_dir_ialloc+0x73/0x26a [x

Mimi Zohar

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs