[PATCH] fuse: make fuse_permission() RCU aware

[PATCH] fuse: make fuse_permission() RCU aware

[Miklos Szeredi]

From: Miklos Szeredi <mszeredi@suse.cz>

Only bail out of fuse_permission() on IPERM_FLAG_RCU when it is
actually necessary.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/fuse/dir.c |   30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

Index: linux-2.6/fs/fuse/dir.c
===================================================================
--- linux-2.6.orig/fs/fuse/dir.c	2011-01-11 11:30:03.000000000 +0100
+++ linux-2.6/fs/fuse/dir.c	2011-01-11 12:38:59.000000000 +0100
@@ -971,6 +971,14 @@ static int fuse_access(struct inode *ino
 	return err;
 }
 
+static int fuse_perm_getattr(struct inode *inode, int flags)
+{
+	if (flags & IPERM_FLAG_RCU)
+		return -ECHILD;
+
+	return fuse_do_getattr(inode, NULL, NULL);
+}
+
 /*
  * Check permission.  The two basic access models of FUSE are:
  *
@@ -990,9 +998,6 @@ static int fuse_permission(struct inode
 	bool refreshed = false;
 	int err = 0;
 
-	if (flags & IPERM_FLAG_RCU)
-		return -ECHILD;
-
 	if (!fuse_allow_task(fc, current))
 		return -EACCES;
 
@@ -1001,9 +1006,15 @@ static int fuse_permission(struct inode
 	 */
 	if ((fc->flags & FUSE_DEFAULT_PERMISSIONS) ||
 	    ((mask & MAY_EXEC) && S_ISREG(inode->i_mode))) {
-		err = fuse_update_attributes(inode, NULL, NULL, &refreshed);
-		if (err)
-			return err;
+		struct fuse_inode *fi = get_fuse_inode(inode);
+
+		if (fi->i_time < get_jiffies_64()) {
+			refreshed = true;
+
+			err = fuse_perm_getattr(inode, flags);
+			if (err)
+				return err;
+		}
 	}
 
 	if (fc->flags & FUSE_DEFAULT_PERMISSIONS) {
@@ -1013,7 +1024,7 @@ static int fuse_permission(struct inode
 		   attributes.  This is also needed, because the root
 		   node will at first have no permissions */
 		if (err == -EACCES && !refreshed) {
-			err = fuse_do_getattr(inode, NULL, NULL);
+			err = fuse_perm_getattr(inode, flags);
 			if (!err)
 				err = generic_permission(inode, mask,
 							flags, NULL);
@@ -1024,13 +1035,16 @@ static int fuse_permission(struct inode
 		   noticed immediately, only after the attribute
 		   timeout has expired */
 	} else if (mask & (MAY_ACCESS | MAY_CHDIR)) {
+		if (flags & IPERM_FLAG_RCU)
+			return -ECHILD;
+
 		err = fuse_access(inode, mask);
 	} else if ((mask & MAY_EXEC) && S_ISREG(inode->i_mode)) {
 		if (!(inode->i_mode & S_IXUGO)) {
 			if (refreshed)
 				return -EACCES;
 
-			err = fuse_do_getattr(inode, NULL, NULL);
+			err = fuse_perm_getattr(inode, flags);
 			if (!err && !(inode->i_mode & S_IXUGO))
 				return -EACCES;
 		}

Re: [PATCH] fuse: make fuse_permission() RCU aware

[Nick Piggin]

On Tue, Jan 11, 2011 at 11:14 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
>
> Only bail out of fuse_permission() on IPERM_FLAG_RCU when it is
> actually necessary.

Great, thanks for taking a look... How about d_revalidate?

Re: [PATCH] fuse: make fuse_permission() RCU aware

[Miklos Szeredi]

On Wed, 12 Jan 2011, Nick Piggin wrote:
> On Tue, Jan 11, 2011 at 11:14 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> > From: Miklos Szeredi <mszeredi@suse.cz>
> >
> > Only bail out of fuse_permission() on IPERM_FLAG_RCU when it is
> > actually necessary.
> 
> Great, thanks for taking a look... How about d_revalidate?

Yeah, here's the patch

Do you want to collect these patches from fs maintainers, or should I
submit to Linus directly?

Thanks,
Miklos
---

From: Miklos Szeredi <mszeredi@suse.cz>
Subject: fuse: make fuse_dentry_revalidate() RCU aware

Only bail out of fuse_dentry_revalidate() on LOOKUP_RCU when blocking
is actually necessary.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/fuse/dir.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Index: linux-2.6/fs/fuse/dir.c
===================================================================
--- linux-2.6.orig/fs/fuse/dir.c	2011-01-12 13:06:04.000000000 +0100
+++ linux-2.6/fs/fuse/dir.c	2011-01-12 13:07:30.000000000 +0100
@@ -158,9 +158,6 @@ static int fuse_dentry_revalidate(struct
 {
 	struct inode *inode;
 
-	if (nd->flags & LOOKUP_RCU)
-		return -ECHILD;
-
 	inode = entry->d_inode;
 	if (inode && is_bad_inode(inode))
 		return 0;
@@ -177,6 +174,9 @@ static int fuse_dentry_revalidate(struct
 		if (!inode)
 			return 0;
 
+		if (nd->flags & LOOKUP_RCU)
+			return -ECHILD;
+
 		fc = get_fuse_conn(inode);
 		req = fuse_get_req(fc);
 		if (IS_ERR(req))

Re: [PATCH] fuse: make fuse_permission() RCU aware

[Nick Piggin]

On Thu, Jan 13, 2011 at 1:26 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Wed, 12 Jan 2011, Nick Piggin wrote:
>> On Tue, Jan 11, 2011 at 11:14 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> > From: Miklos Szeredi <mszeredi@suse.cz>
>> >
>> > Only bail out of fuse_permission() on IPERM_FLAG_RCU when it is
>> > actually necessary.
>>
>> Great, thanks for taking a look... How about d_revalidate?
>
> Yeah, here's the patch
>
> Do you want to collect these patches from fs maintainers, or should I
> submit to Linus directly?

I would like to have a look over them and ack them before they go to Linus,
but I think the most logical and merge friendly approach is just going via
filesystems trees.


> From: Miklos Szeredi <mszeredi@suse.cz>
> Subject: fuse: make fuse_dentry_revalidate() RCU aware
>
> Only bail out of fuse_dentry_revalidate() on LOOKUP_RCU when blocking
> is actually necessary.
>
> Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
> ---
>  fs/fuse/dir.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> Index: linux-2.6/fs/fuse/dir.c
> ===================================================================
> --- linux-2.6.orig/fs/fuse/dir.c        2011-01-12 13:06:04.000000000 +0100
> +++ linux-2.6/fs/fuse/dir.c     2011-01-12 13:07:30.000000000 +0100
> @@ -158,9 +158,6 @@ static int fuse_dentry_revalidate(struct
>  {
>        struct inode *inode;
>
> -       if (nd->flags & LOOKUP_RCU)
> -               return -ECHILD;
> -
>        inode = entry->d_inode;
>        if (inode && is_bad_inode(inode))
>                return 0;

Now it can be the case that entry->d_inode is not stable -- it can
go away or even flip between inodes in the case of concurrent
unlink/creat activity. And you may be using a different inode than
the namei path walk is using!

This isn't as scary as it sounds actually, because any such changes
do get detected and the path walk restarted in that case.

You might be OK becuse you do test for NULL (although it really
wants an ACCESS_ONCE() so it doesn't load a NULL or different
inodes, but that's quite theoretical).

But this is a little unfriendly for filesystems, and I do want to impress
the rule to not touch ->d_parent or ->d_inode in rcu walk mode
(just to avoid any surprises).

So what I have done in such cases is to update the API to provide
what the callers want. In this case, we could consider adding an
inode parameter to .d_revalidate, which callers can be sure matches
the inode used by vfs, and will not change.

Aside from that detail, the changes seem good. Thanks for looking
at it.


> @@ -177,6 +174,9 @@ static int fuse_dentry_revalidate(struct
>                if (!inode)
>                        return 0;
>
> +               if (nd->flags & LOOKUP_RCU)
> +                       return -ECHILD;
> +
>                fc = get_fuse_conn(inode);
>                req = fuse_get_req(fc);
>                if (IS_ERR(req))
>

Re: [PATCH] fuse: make fuse_permission() RCU aware

[Miklos Szeredi]

On Thu, 13 Jan 2011, Nick Piggin wrote:
> >        inode = entry->d_inode;
> >        if (inode && is_bad_inode(inode))
> >                return 0;
> 
> Now it can be the case that entry->d_inode is not stable -- it can
> go away or even flip between inodes in the case of concurrent
> unlink/creat activity. And you may be using a different inode than
> the namei path walk is using!
> This isn't as scary as it sounds actually, because any such changes
> do get detected and the path walk restarted in that case.
> 
> You might be OK becuse you do test for NULL (although it really
> wants an ACCESS_ONCE() so it doesn't load a NULL or different
> inodes, but that's quite theoretical).
> 
> But this is a little unfriendly for filesystems, and I do want to impress
> the rule to not touch ->d_parent or ->d_inode in rcu walk mode
> (just to avoid any surprises).
> 
> So what I have done in such cases is to update the API to provide
> what the callers want. In this case, we could consider adding an
> inode parameter to .d_revalidate, which callers can be sure matches
> the inode used by vfs, and will not change.

Yes, I think supplying an inode to ->d_revalidate() would be the right
thing.

I see that there's nd->inode, and it's documented in vfs.txt, but IMO
we shouln'd be encouraging new uses of 'nd' inside filesystem
callbacks, rather the opposite.  A 'flags' parameter too might make
sense which, for the moment, would only have LOOKUP_RCU instead of all
the lookup flags.

Thanks,
Miklos

Re: [PATCH] fuse: make fuse_permission() RCU aware

[Nick Piggin]

On Thu, Jan 13, 2011 at 10:09 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Thu, 13 Jan 2011, Nick Piggin wrote:
>> >        inode = entry->d_inode;
>> >        if (inode && is_bad_inode(inode))
>> >                return 0;
>>
>> Now it can be the case that entry->d_inode is not stable -- it can
>> go away or even flip between inodes in the case of concurrent
>> unlink/creat activity. And you may be using a different inode than
>> the namei path walk is using!
>> This isn't as scary as it sounds actually, because any such changes
>> do get detected and the path walk restarted in that case.
>>
>> You might be OK becuse you do test for NULL (although it really
>> wants an ACCESS_ONCE() so it doesn't load a NULL or different
>> inodes, but that's quite theoretical).
>>
>> But this is a little unfriendly for filesystems, and I do want to impress
>> the rule to not touch ->d_parent or ->d_inode in rcu walk mode
>> (just to avoid any surprises).
>>
>> So what I have done in such cases is to update the API to provide
>> what the callers want. In this case, we could consider adding an
>> inode parameter to .d_revalidate, which callers can be sure matches
>> the inode used by vfs, and will not change.
>
> Yes, I think supplying an inode to ->d_revalidate() would be the right
> thing.

OK I'll work something up.


> I see that there's nd->inode, and it's documented in vfs.txt, but IMO
> we shouln'd be encouraging new uses of 'nd' inside filesystem
> callbacks, rather the opposite.  A 'flags' parameter too might make
> sense which, for the moment, would only have LOOKUP_RCU instead of all
> the lookup flags.

I have tried to avoid filesystems poking in nd (except for this case I
guess), so yes an inode * parameter should work.

Thanks,
Nick