diff mbox series

[RFC,60/62] orangefs: make use of ->free_inode()

Message ID 20190416175340.21068-60-viro@ZenIV.linux.org.uk (mailing list archive)
State New, archived
Headers show
Series [RFC,01/62] securityfs: fix use-after-free on symlink traversal | expand

Commit Message

Al Viro April 16, 2019, 5:53 p.m. UTC 
From: Al Viro <viro@zeniv.linux.org.uk>

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/orangefs/super.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

Comments

Mike Marshall April 22, 2019, 9:14 p.m. UTC | #1 
Hi Al...

I applied your "new inode method: ->free_inode()" and
"orangefs: make use of ->free_inode()" to our pagecache
branch (I hope to get it pulled in the next merge window).

I had to modify your "orangefs: make use of ->free_inode()" a
little, since Martin Brandenburg had already modified orangefs_i_callback
for the pagecache branch. I don't know for sure that my modifications
aren't nonsense :-) but I do know for sure that everything runs
with no xfstests regressions. I'll see what Martin thinks about
my changes...

orangefs_destroy_inode is pretty much a no-op now, so I guess
we'll get rid of it...

Acked-by: Mike Marshall <hubcap@omnibond.com>

Thanks...

-Mike


[root@vm1 linux]# git diff HEAD^ fs/orangefs/super.c
diff --git a/fs/orangefs/super.c b/fs/orangefs/super.c
index 8fa30c13b7ed..f82ac9373443 100644
--- a/fs/orangefs/super.c
+++ b/fs/orangefs/super.c
@@ -125,20 +125,18 @@ static struct inode *orangefs_alloc_inode(struct
super_block *sb)
        return &orangefs_inode->vfs_inode;
 }

-static void orangefs_i_callback(struct rcu_head *head)
+static void orangefs_free_inode(struct inode *inode)
 {
-       struct inode *inode = container_of(head, struct inode, i_rcu);
-       struct orangefs_inode_s *orangefs_inode = ORANGEFS_I(inode);
        struct orangefs_cached_xattr *cx;
        struct hlist_node *tmp;
        int i;

-       hash_for_each_safe(orangefs_inode->xattr_cache, i, tmp, cx, node) {
+       hash_for_each_safe(ORANGEFS_I(inode)->xattr_cache, i, tmp, cx, node) {
                hlist_del(&cx->node);
                kfree(cx);
        }

-       kmem_cache_free(orangefs_inode_cache, orangefs_inode);
+       kmem_cache_free(orangefs_inode_cache, ORANGEFS_I(inode));
 }

 static void orangefs_destroy_inode(struct inode *inode)
@@ -148,8 +146,6 @@ static void orangefs_destroy_inode(struct inode *inode)
        gossip_debug(GOSSIP_SUPER_DEBUG,
                        "%s: deallocated %p destroying inode %pU\n",
                        __func__, orangefs_inode, get_khandle_from_ino(inode));
-
-       call_rcu(&inode->i_rcu, orangefs_i_callback);
 }

 static int orangefs_write_inode(struct inode *inode,
@@ -316,6 +312,7 @@ void fsid_key_table_finalize(void)

 static const struct super_operations orangefs_s_ops = {
        .alloc_inode = orangefs_alloc_inode,
+       .free_inode = orangefs_free_inode,
        .destroy_inode = orangefs_destroy_inode,
        .write_inode = orangefs_write_inode,
        .drop_inode = generic_delete_inode,

On Tue, Apr 16, 2019 at 1:55 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> From: Al Viro <viro@zeniv.linux.org.uk>
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  fs/orangefs/super.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/fs/orangefs/super.c b/fs/orangefs/super.c
> index dfaee90d30bd..3784f7e8b603 100644
> --- a/fs/orangefs/super.c
> +++ b/fs/orangefs/super.c
> @@ -124,11 +124,9 @@ static struct inode *orangefs_alloc_inode(struct super_block *sb)
>         return &orangefs_inode->vfs_inode;
>  }
>
> -static void orangefs_i_callback(struct rcu_head *head)
> +static void orangefs_free_inode(struct inode *inode)
>  {
> -       struct inode *inode = container_of(head, struct inode, i_rcu);
> -       struct orangefs_inode_s *orangefs_inode = ORANGEFS_I(inode);
> -       kmem_cache_free(orangefs_inode_cache, orangefs_inode);
> +       kmem_cache_free(orangefs_inode_cache, ORANGEFS_I(inode));
>  }
>
>  static void orangefs_destroy_inode(struct inode *inode)
> @@ -138,8 +136,6 @@ static void orangefs_destroy_inode(struct inode *inode)
>         gossip_debug(GOSSIP_SUPER_DEBUG,
>                         "%s: deallocated %p destroying inode %pU\n",
>                         __func__, orangefs_inode, get_khandle_from_ino(inode));
> -
> -       call_rcu(&inode->i_rcu, orangefs_i_callback);
>  }
>
>  /*
> @@ -299,6 +295,7 @@ void fsid_key_table_finalize(void)
>
>  static const struct super_operations orangefs_s_ops = {
>         .alloc_inode = orangefs_alloc_inode,
> +       .free_inode = orangefs_free_inode,
>         .destroy_inode = orangefs_destroy_inode,
>         .drop_inode = generic_delete_inode,
>         .statfs = orangefs_statfs,
> --
> 2.11.0
>
Linus Torvalds April 22, 2019, 9:56 p.m. UTC | #2 
On Mon, Apr 22, 2019 at 2:14 PM Mike Marshall <hubcap@omnibond.com> wrote:
>
> I applied your "new inode method: ->free_inode()" and
> "orangefs: make use of ->free_inode()" to our pagecache
> branch (I hope to get it pulled in the next merge window).

Actually, please don't.

Exactly because this needs that common vfs patch, I'd really prefer to
get it all through Al's tree, rather than have individual filesystems
apply their own copies of the common infrastructure commit, and then
apply their changes on top of that.

I can easily handle any trivial conflicts this causes, so that's not a
reason to have each filesystem do it either.

So if this is at the top of your tree, can you just "git reset" it
away and I'll get all the filesystems (and the common infrastructure
commit) all together from Al.

                 Linus
Al Viro April 22, 2019, 11:10 p.m. UTC | #3 
On Mon, Apr 22, 2019 at 02:56:57PM -0700, Linus Torvalds wrote:
> On Mon, Apr 22, 2019 at 2:14 PM Mike Marshall <hubcap@omnibond.com> wrote:
> >
> > I applied your "new inode method: ->free_inode()" and
> > "orangefs: make use of ->free_inode()" to our pagecache
> > branch (I hope to get it pulled in the next merge window).
> 
> Actually, please don't.
> 
> Exactly because this needs that common vfs patch, I'd really prefer to
> get it all through Al's tree, rather than have individual filesystems
> apply their own copies of the common infrastructure commit, and then
> apply their changes on top of that.
> 
> I can easily handle any trivial conflicts this causes, so that's not a
> reason to have each filesystem do it either.
> 
> So if this is at the top of your tree, can you just "git reset" it
> away and I'll get all the filesystems (and the common infrastructure
> commit) all together from Al.

What's more, seeing the changes in orangefs tree I would rather have
static void orangefs_free_inode(struct inode *inode)
{
	struct orangefs_inode_s *orangefs_inode = ORANGEFS_I(inode);
	kmem_cache_free(orangefs_inode_cache, orangefs_inode);
}

in that series; not only less noise on merge, but with additional
uses of orangefs_inode in the body from orangefs tree changes
keeping the local variable clearly makes sense...
Mike Marshall April 22, 2019, 11:17 p.m. UTC | #4 
Hi Linus and Al...

I just wanted Al to know I tested his patch and acked it and that it
there would be
a conflict if our pagecache code got pulled... I wasn't suggesting that I
should get that one part of Al's patch pulled...

>> I can easily handle any trivial conflicts this causes...

Thanks :-)

-Mike

On Mon, Apr 22, 2019 at 7:10 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Mon, Apr 22, 2019 at 02:56:57PM -0700, Linus Torvalds wrote:
> > On Mon, Apr 22, 2019 at 2:14 PM Mike Marshall <hubcap@omnibond.com> wrote:
> > >
> > > I applied your "new inode method: ->free_inode()" and
> > > "orangefs: make use of ->free_inode()" to our pagecache
> > > branch (I hope to get it pulled in the next merge window).
> >
> > Actually, please don't.
> >
> > Exactly because this needs that common vfs patch, I'd really prefer to
> > get it all through Al's tree, rather than have individual filesystems
> > apply their own copies of the common infrastructure commit, and then
> > apply their changes on top of that.
> >
> > I can easily handle any trivial conflicts this causes, so that's not a
> > reason to have each filesystem do it either.
> >
> > So if this is at the top of your tree, can you just "git reset" it
> > away and I'll get all the filesystems (and the common infrastructure
> > commit) all together from Al.
>
> What's more, seeing the changes in orangefs tree I would rather have
> static void orangefs_free_inode(struct inode *inode)
> {
>         struct orangefs_inode_s *orangefs_inode = ORANGEFS_I(inode);
>         kmem_cache_free(orangefs_inode_cache, orangefs_inode);
> }
>
> in that series; not only less noise on merge, but with additional
> uses of orangefs_inode in the body from orangefs tree changes
> keeping the local variable clearly makes sense...
diff mbox series

Patch

diff --git a/fs/orangefs/super.c b/fs/orangefs/super.c
index dfaee90d30bd..3784f7e8b603 100644
--- a/fs/orangefs/super.c
+++ b/fs/orangefs/super.c
@@ -124,11 +124,9 @@  static struct inode *orangefs_alloc_inode(struct super_block *sb)
 	return &orangefs_inode->vfs_inode;
 }
 
-static void orangefs_i_callback(struct rcu_head *head)
+static void orangefs_free_inode(struct inode *inode)
 {
-	struct inode *inode = container_of(head, struct inode, i_rcu);
-	struct orangefs_inode_s *orangefs_inode = ORANGEFS_I(inode);
-	kmem_cache_free(orangefs_inode_cache, orangefs_inode);
+	kmem_cache_free(orangefs_inode_cache, ORANGEFS_I(inode));
 }
 
 static void orangefs_destroy_inode(struct inode *inode)
@@ -138,8 +136,6 @@  static void orangefs_destroy_inode(struct inode *inode)
 	gossip_debug(GOSSIP_SUPER_DEBUG,
 			"%s: deallocated %p destroying inode %pU\n",
 			__func__, orangefs_inode, get_khandle_from_ino(inode));
-
-	call_rcu(&inode->i_rcu, orangefs_i_callback);
 }
 
 /*
@@ -299,6 +295,7 @@  void fsid_key_table_finalize(void)
 
 static const struct super_operations orangefs_s_ops = {
 	.alloc_inode = orangefs_alloc_inode,
+	.free_inode = orangefs_free_inode,
 	.destroy_inode = orangefs_destroy_inode,
 	.drop_inode = generic_delete_inode,
 	.statfs = orangefs_statfs,