| Message ID | 20180729220453.13431-3-viro@ZenIV.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/16] nfs_instantiate(): prevent multiple aliases for directory inode | expand |
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.
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
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.
> 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.
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
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
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;