[PATCH v2 (v4.18 regression fix)] vfs: don't evict uninitialized inode

[PATCH v2 (v4.18 regression fix)] vfs: don't evict uninitialized inode

[Miklos Szeredi]

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 | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index 04dd7e0d5142..e7de38c1d9d8 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,12 @@ 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) {
 			inode = inode_insert5(new, hashval, test, set, data);
 			if (unlikely(inode != new))
-				iput(new);
+				destroy_inode(new);
 		}
 	}
 	return inode;
-- 
2.14.3

Re: [PATCH v2 (v4.18 regression fix)] vfs: don't evict uninitialized inode

[Amir Goldstein]

On Fri, Jul 6, 2018 at 11:45 PM, 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()")
> ---

Al,

Did you loose track of this 4.18 regression fix?

Thanks,
Amir.

>  fs/inode.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/fs/inode.c b/fs/inode.c
> index 04dd7e0d5142..e7de38c1d9d8 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,12 @@ 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) {
>                         inode = inode_insert5(new, hashval, test, set, data);
>                         if (unlikely(inode != new))
> -                               iput(new);
> +                               destroy_inode(new);
>                 }
>         }
>         return inode;
> --
> 2.14.3
>

Re: [PATCH v2 (v4.18 regression fix)] vfs: don't evict uninitialized inode

[Miklos Szeredi]

Ping?

This (or some variant) needs to go into 4.18.

Thanks,
Miklos

On Fri, Jul 6, 2018 at 10:45 PM, 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 | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/fs/inode.c b/fs/inode.c
> index 04dd7e0d5142..e7de38c1d9d8 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,12 @@ 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) {
>                         inode = inode_insert5(new, hashval, test, set, data);
>                         if (unlikely(inode != new))
> -                               iput(new);
> +                               destroy_inode(new);
>                 }
>         }
>         return inode;
> --
> 2.14.3
>

Re: [PATCH v2 (v4.18 regression fix)] vfs: don't evict uninitialized inode

[Al Viro]

On Wed, Jul 18, 2018 at 01:40:08PM +0200, Miklos Szeredi wrote:
> Ping?
> 
> This (or some variant) needs to go into 4.18.

Yes - icache series is on the top of my list, now that -open stuff
is (hopefully) sorted out (in for-next, anyway).

I'm putting together a queue with that stuff at the moment.

BTW, why have you left generic_readlink() sitting around?  AFAICS,
it could've been folded into the only remaining caller just as
you've made it static in late 2016...  I'll fold it in;
just curious what was the reason for not doing that back then...

Re: [PATCH v2 (v4.18 regression fix)] vfs: don't evict uninitialized inode

[Miklos Szeredi]

On Wed, Jul 18, 2018 at 2:18 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:

> BTW, why have you left generic_readlink() sitting around?  AFAICS,
> it could've been folded into the only remaining caller just as
> you've made it static in late 2016...  I'll fold it in;
> just curious what was the reason for not doing that back then...

I don't remember any particular reason.

Thanks,
Miklos

Re: [PATCH v2 (v4.18 regression fix)] vfs: don't evict uninitialized inode

[Al Viro]

On Wed, Jul 18, 2018 at 01:18:33PM +0100, Al Viro wrote:

> BTW, why have you left generic_readlink() sitting around?  AFAICS,
> it could've been folded into the only remaining caller just as
> you've made it static in late 2016...  I'll fold it in;
> just curious what was the reason for not doing that back then...

BTW^2:
const char *vfs_get_link(struct dentry *dentry, struct delayed_call *done)
{
        const char *res = ERR_PTR(-EINVAL);
        struct inode *inode = d_inode(dentry);

        if (d_is_symlink(dentry)) {
                res = ERR_PTR(security_inode_readlink(dentry));
                if (!res)
                        res = inode->i_op->get_link(dentry, inode, done);
        }
        return res;
}
hits a method call that is not needed in the majority of cases.  Is there
any subtle reason why it shouldn't be

const char *vfs_get_link(struct dentry *dentry, struct delayed_call *done)
{
        const char *res = ERR_PTR(-EINVAL);
        struct inode *inode = d_inode(dentry);

        if (d_is_symlink(dentry)) {
                res = ERR_PTR(security_inode_readlink(dentry));
                if (!res)
			res = inode->i_link;
		if (!res)
                        res = inode->i_op->get_link(dentry, inode, done);
        }
        return res;
}
instead?

Re: [PATCH v2 (v4.18 regression fix)] vfs: don't evict uninitialized inode

[Miklos Szeredi]

On Thu, Jul 19, 2018 at 11:45 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Wed, Jul 18, 2018 at 01:18:33PM +0100, Al Viro wrote:
>
>> BTW, why have you left generic_readlink() sitting around?  AFAICS,
>> it could've been folded into the only remaining caller just as
>> you've made it static in late 2016...  I'll fold it in;
>> just curious what was the reason for not doing that back then...
>
> BTW^2:
> const char *vfs_get_link(struct dentry *dentry, struct delayed_call *done)
> {
>         const char *res = ERR_PTR(-EINVAL);
>         struct inode *inode = d_inode(dentry);
>
>         if (d_is_symlink(dentry)) {
>                 res = ERR_PTR(security_inode_readlink(dentry));
>                 if (!res)
>                         res = inode->i_op->get_link(dentry, inode, done);
>         }
>         return res;
> }
> hits a method call that is not needed in the majority of cases.  Is there
> any subtle reason why it shouldn't be
>
> const char *vfs_get_link(struct dentry *dentry, struct delayed_call *done)
> {
>         const char *res = ERR_PTR(-EINVAL);
>         struct inode *inode = d_inode(dentry);
>
>         if (d_is_symlink(dentry)) {
>                 res = ERR_PTR(security_inode_readlink(dentry));
>                 if (!res)
>                         res = inode->i_link;
>                 if (!res)
>                         res = inode->i_op->get_link(dentry, inode, done);
>         }
>         return res;
> }
> instead?

Can't see any issues.  But I also don't think any of the callers are
seriously performance sensitive, so I guess it basically doesn't
matter.

Thanks,
Miklos