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 <[email protected]>
Signed-off-by: Miklos Szeredi <[email protected]>
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
On Fri, Jul 6, 2018 at 11:45 PM, Miklos Szeredi <[email protected]> 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 <[email protected]>
> Signed-off-by: Miklos Szeredi <[email protected]>
> 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
>
Ping?
This (or some variant) needs to go into 4.18.
Thanks,
Miklos
On Fri, Jul 6, 2018 at 10:45 PM, Miklos Szeredi <[email protected]> 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 <[email protected]>
> Signed-off-by: Miklos Szeredi <[email protected]>
> 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
>
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...
On Wed, Jul 18, 2018 at 2:18 PM, Al Viro <[email protected]> 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
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?
On Thu, Jul 19, 2018 at 11:45 PM, Al Viro <[email protected]> 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