[03/16] vfs: don't evict uninitialized inode
diff mbox series

Message ID 20180729220453.13431-3-viro@ZenIV.linux.org.uk
State New
Headers show
Series
  • [01/16] nfs_instantiate(): prevent multiple aliases for directory inode
Related show

Commit Message

Al Viro July 29, 2018, 10:04 p.m. UTC
From: Miklos Szeredi <mszeredi@redhat.com>

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()")
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/inode.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Amir Goldstein July 30, 2018, 5:09 a.m. UTC | #1
On Mon, Jul 30, 2018 at 1:04 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> From: Miklos Szeredi <mszeredi@redhat.com>
>
> 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()")

Backport hint: this patch depends on the patch ("new primitive:
discard_new_inode()") currently commit 22dc9a168272 in Al's for-next.

Still trying to figure out the best format to channel this information to
stable maintainers...

Thanks,
Amir.
Miklos Szeredi July 30, 2018, 7:41 a.m. UTC | #2
On Mon, Jul 30, 2018 at 7:09 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Mon, Jul 30, 2018 at 1:04 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>> From: Miklos Szeredi <mszeredi@redhat.com>
>>
>> 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()")
>
> Backport hint: this patch depends on the patch ("new primitive:
> discard_new_inode()") currently commit 22dc9a168272 in Al's for-next.
>
> Still trying to figure out the best format to channel this information to
> stable maintainers...

Why are we talking about stable?  This regression was introduced in
4.18-rc1, spotted by Al *and* reported by testers.  It needs to be
fixed in one way or other in 4.18.

I've nothing against applying "new primitive: discard_new_inode() now
+ this patch, but if it is deemed too risky at this point, we could
just revert the buggy commit 80ea09a002bf ("vfs: factor out
inode_insert5()") and its dependencies.

Thanks,
Miklos
Amir Goldstein Aug. 16, 2018, 4:29 p.m. UTC | #3
On Mon, Jul 30, 2018 at 10:41 AM Miklos Szeredi <mszeredi@redhat.com> wrote:
>
> On Mon, Jul 30, 2018 at 7:09 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> > On Mon, Jul 30, 2018 at 1:04 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >> From: Miklos Szeredi <mszeredi@redhat.com>
> >>
> >> 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()")
> >
> > Backport hint: this patch depends on the patch ("new primitive:
> > discard_new_inode()") currently commit 22dc9a168272 in Al's for-next.
> >
> > Still trying to figure out the best format to channel this information to
> > stable maintainers...
>
> Why are we talking about stable?  This regression was introduced in
> 4.18-rc1, spotted by Al *and* reported by testers.  It needs to be
> fixed in one way or other in 4.18.
>

Miklos,

Seeing that it wasn't fixed in 4.18..

> I've nothing against applying "new primitive: discard_new_inode() now
> + this patch, but if it is deemed too risky at this point, we could
> just revert the buggy commit 80ea09a002bf ("vfs: factor out
> inode_insert5()") and its dependencies.
>

Should we propose for stable the upstream commits:
e950564b97fd vfs: don't evict uninitialized inode
c2b6d621c4ff new primitive: discard_new_inode()

Or should we go with the independent v1 patch:
https://patchwork.kernel.org/patch/10511969/

Thanks,
Amir.
Amir Goldstein Aug. 24, 2018, 6:47 a.m. UTC | #4
> Miklos,
>
> Seeing that it wasn't fixed in 4.18..
>
> > I've nothing against applying "new primitive: discard_new_inode() now
> > + this patch, but if it is deemed too risky at this point, we could
> > just revert the buggy commit 80ea09a002bf ("vfs: factor out
> > inode_insert5()") and its dependencies.
> >
>
> Should we propose for stable the upstream commits:
> e950564b97fd vfs: don't evict uninitialized inode
> c2b6d621c4ff new primitive: discard_new_inode()
>
> Or should we go with the independent v1 patch:
> https://patchwork.kernel.org/patch/10511969/
>

Greg,

To fix a 4.18 overlayfs regression please apply the following
3 upstream commits (in apply order):

c2b6d621c4ff new primitive: discard_new_inode()
e950564b97fd vfs: don't evict uninitialized inode
6faf05c2b2b4 ovl: set I_CREATING on inode being created

Thanks,
Amir.
Miklos Szeredi Oct. 8, 2018, 6:41 a.m. UTC | #5
On Fri, Aug 24, 2018 at 8:47 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>> Miklos,
>>
>> Seeing that it wasn't fixed in 4.18..
>>
>> > I've nothing against applying "new primitive: discard_new_inode() now
>> > + this patch, but if it is deemed too risky at this point, we could
>> > just revert the buggy commit 80ea09a002bf ("vfs: factor out
>> > inode_insert5()") and its dependencies.
>> >
>>
>> Should we propose for stable the upstream commits:
>> e950564b97fd vfs: don't evict uninitialized inode
>> c2b6d621c4ff new primitive: discard_new_inode()
>>
>> Or should we go with the independent v1 patch:
>> https://patchwork.kernel.org/patch/10511969/
>>
>
> Greg,
>
> To fix a 4.18 overlayfs regression please apply the following
> 3 upstream commits (in apply order):
>
> c2b6d621c4ff new primitive: discard_new_inode()
> e950564b97fd vfs: don't evict uninitialized inode
> 6faf05c2b2b4 ovl: set I_CREATING on inode being created

Is this fixed in 4.18.z yet?

Thanks,
Miklos
Greg Kroah-Hartman Oct. 8, 2018, 1:23 p.m. UTC | #6
On Mon, Oct 08, 2018 at 08:41:13AM +0200, Miklos Szeredi wrote:
> On Fri, Aug 24, 2018 at 8:47 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> >> Miklos,
> >>
> >> Seeing that it wasn't fixed in 4.18..
> >>
> >> > I've nothing against applying "new primitive: discard_new_inode() now
> >> > + this patch, but if it is deemed too risky at this point, we could
> >> > just revert the buggy commit 80ea09a002bf ("vfs: factor out
> >> > inode_insert5()") and its dependencies.
> >> >
> >>
> >> Should we propose for stable the upstream commits:
> >> e950564b97fd vfs: don't evict uninitialized inode
> >> c2b6d621c4ff new primitive: discard_new_inode()
> >>
> >> Or should we go with the independent v1 patch:
> >> https://patchwork.kernel.org/patch/10511969/
> >>
> >
> > Greg,
> >
> > To fix a 4.18 overlayfs regression please apply the following
> > 3 upstream commits (in apply order):
> >
> > c2b6d621c4ff new primitive: discard_new_inode()
> > e950564b97fd vfs: don't evict uninitialized inode
> > 6faf05c2b2b4 ovl: set I_CREATING on inode being created
> 
> Is this fixed in 4.18.z yet?

Not yet, sorry, will try to get to that today...

greg k-h

Patch
diff mbox series

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;