| Message ID | 20180724130155.20641-1-mszeredi@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] vfs: don't evict uninitialized inode | expand |
On Tue, Jul 24, 2018 at 10:01 AM, Miklos Szeredi <mszeredi@redhat.com> wrote: > iput() ends up calling ->evict() on new inode, which is not yet initialized > by owning fs. So use destroy_inode() instead. > > Add to sb->s_inodes list only if inode is not in I_CREATING state (meaning > that it wasn't allocated with new_inode(), which already does the > insertion). > > Reported-by: Al Viro <viro@zeniv.linux.org.uk> > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > Fixes: 80ea09a002bf ("vfs: factor out inode_insert5()") > --- > fs/inode.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/fs/inode.c b/fs/inode.c > index 04dd7e0d5142..0aa5b29b6f87 100644 > --- a/fs/inode.c > +++ b/fs/inode.c > @@ -1050,6 +1050,7 @@ struct inode *inode_insert5(struct inode *inode, unsigned long hashval, > { > struct hlist_head *head = inode_hashtable + hash(inode->i_sb, hashval); > struct inode *old; > + bool creating = inode->i_state & I_CREATING; > > again: > spin_lock(&inode_hash_lock); > @@ -1083,6 +1084,8 @@ struct inode *inode_insert5(struct inode *inode, unsigned long hashval, > inode->i_state |= I_NEW; > hlist_add_head(&inode->i_hash, head); > spin_unlock(&inode->i_lock); > + if (!creating) > + inode_sb_list_add(inode); > unlock: > spin_unlock(&inode_hash_lock); > > @@ -1117,12 +1120,13 @@ struct inode *iget5_locked(struct super_block *sb, unsigned long hashval, > struct inode *inode = ilookup5(sb, hashval, test, data); > > if (!inode) { > - struct inode *new = new_inode(sb); > + struct inode *new = alloc_inode(sb); > > if (new) { > + new->i_state = 0; > inode = inode_insert5(new, hashval, test, set, data); > if (unlikely(inode != new)) > - iput(new); > + destroy_inode(new); > } > } > return inode; > -- > 2.14.3 > FYI with this patch (now merged) I'm seeing warnings whenever an object is created in an overlayfs mount: [ 842.152673] list_add double add: new=ffff88017efe03d8, prev=ffff88015c07ad88, next=ffff88017efe03d8. [ 842.152687] WARNING: CPU: 4 PID: 7592 at lib/list_debug.c:31 __list_add_valid+0x6e/0x80 The call stack looks like this, for the file creation case: [ 842.152746] inode_sb_list_add+0x4e/0x90 [ 842.152753] ? ovl_inode_test+0x20/0x20 [overlay] [ 842.152757] ? ovl_get_redirect_xattr+0x140/0x140 [overlay] [ 842.152759] inode_insert5+0x13e/0x1f0 [ 842.152764] ovl_get_inode.cold.16+0x38/0x44 [overlay] [ 842.152768] ovl_instantiate+0x75/0x130 [overlay] [ 842.152773] ovl_create_or_link+0x1c9/0x7d0 [overlay] [ 842.152776] ? ovl_alloc_inode+0x1b/0x80 [overlay] [ 842.152778] ? inode_sb_list_add+0x4e/0x90 [ 842.152782] ? ovl_fill_inode+0xd8/0x150 [overlay] [ 842.152787] ovl_create_object+0xa1/0xd0 [overlay] [ 842.152791] ovl_create+0x23/0x30 [overlay] .. where the inode passed to inode_insert5 originally comes from new_inode (ovl_create_object -> ovl_new_inode -> new_inode), was already added to the sb list, and doesn't have I_CREATING set. Originally seen from docker's use of overlayfs, but easily reproducible by creating a simple overlay of 2 ext4 directories and creating a new file or directory in the overlay. Marc
On Wed, Aug 22, 2018 at 4:37 PM, Marc Dionne <marc.c.dionne@gmail.com> wrote: > > FYI with this patch (now merged) I'm seeing warnings whenever an > object is created in an overlayfs mount: > > [ 842.152673] list_add double add: new=ffff88017efe03d8, > prev=ffff88015c07ad88, next=ffff88017efe03d8. > [ 842.152687] WARNING: CPU: 4 PID: 7592 at lib/list_debug.c:31 > __list_add_valid+0x6e/0x80 > And I see that's now fixed by 6faf05c2b2b4 ("ovl: set I_CREATING on inode being created"). Marc
diff --git a/fs/inode.c b/fs/inode.c index 04dd7e0d5142..0aa5b29b6f87 100644 --- a/fs/inode.c +++ b/fs/inode.c @@ -1050,6 +1050,7 @@ struct inode *inode_insert5(struct inode *inode, unsigned long hashval, { struct hlist_head *head = inode_hashtable + hash(inode->i_sb, hashval); struct inode *old; + bool creating = inode->i_state & I_CREATING; again: spin_lock(&inode_hash_lock); @@ -1083,6 +1084,8 @@ struct inode *inode_insert5(struct inode *inode, unsigned long hashval, inode->i_state |= I_NEW; hlist_add_head(&inode->i_hash, head); spin_unlock(&inode->i_lock); + if (!creating) + inode_sb_list_add(inode); unlock: spin_unlock(&inode_hash_lock); @@ -1117,12 +1120,13 @@ struct inode *iget5_locked(struct super_block *sb, unsigned long hashval, struct inode *inode = ilookup5(sb, hashval, test, data); if (!inode) { - struct inode *new = new_inode(sb); + struct inode *new = alloc_inode(sb); if (new) { + new->i_state = 0; inode = inode_insert5(new, hashval, test, set, data); if (unlikely(inode != new)) - iput(new); + destroy_inode(new); } } return inode;
iput() ends up calling ->evict() on new inode, which is not yet initialized by owning fs. So use destroy_inode() instead. Add to sb->s_inodes list only if inode is not in I_CREATING state (meaning that it wasn't allocated with new_inode(), which already does the insertion). Reported-by: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> Fixes: 80ea09a002bf ("vfs: factor out inode_insert5()") --- fs/inode.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)