* [PATCH] fuse: make fuse_permission() RCU aware
@ 2011-01-11 12:14 Miklos Szeredi
2011-01-12 4:37 ` Nick Piggin
0 siblings, 1 reply; 6+ messages in thread
From: Miklos Szeredi @ 2011-01-11 12:14 UTC (permalink / raw)
To: linux-fsdevel; +Cc: npiggin, akpm, linux-kernel
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;
}
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] fuse: make fuse_permission() RCU aware
2011-01-11 12:14 [PATCH] fuse: make fuse_permission() RCU aware Miklos Szeredi
@ 2011-01-12 4:37 ` Nick Piggin
2011-01-12 14:26 ` Miklos Szeredi
0 siblings, 1 reply; 6+ messages in thread
From: Nick Piggin @ 2011-01-12 4:37 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: linux-fsdevel, akpm, linux-kernel
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?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fuse: make fuse_permission() RCU aware
2011-01-12 4:37 ` Nick Piggin
@ 2011-01-12 14:26 ` Miklos Szeredi
2011-01-13 1:49 ` Nick Piggin
0 siblings, 1 reply; 6+ messages in thread
From: Miklos Szeredi @ 2011-01-12 14:26 UTC (permalink / raw)
To: Nick Piggin; +Cc: miklos, linux-fsdevel, akpm, linux-kernel
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))
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] fuse: make fuse_permission() RCU aware
2011-01-12 14:26 ` Miklos Szeredi
@ 2011-01-13 1:49 ` Nick Piggin
2011-01-13 11:09 ` Miklos Szeredi
0 siblings, 1 reply; 6+ messages in thread
From: Nick Piggin @ 2011-01-13 1:49 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: linux-fsdevel, akpm, linux-kernel
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))
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] fuse: make fuse_permission() RCU aware
2011-01-13 1:49 ` Nick Piggin
@ 2011-01-13 11:09 ` Miklos Szeredi
2011-01-13 11:16 ` Nick Piggin
0 siblings, 1 reply; 6+ messages in thread
From: Miklos Szeredi @ 2011-01-13 11:09 UTC (permalink / raw)
To: Nick Piggin; +Cc: miklos, linux-fsdevel, akpm, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fuse: make fuse_permission() RCU aware
2011-01-13 11:09 ` Miklos Szeredi
@ 2011-01-13 11:16 ` Nick Piggin
0 siblings, 0 replies; 6+ messages in thread
From: Nick Piggin @ 2011-01-13 11:16 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: linux-fsdevel, akpm, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-01-13 11:16 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-01-11 12:14 [PATCH] fuse: make fuse_permission() RCU aware Miklos Szeredi
2011-01-12 4:37 ` Nick Piggin
2011-01-12 14:26 ` Miklos Szeredi
2011-01-13 1:49 ` Nick Piggin
2011-01-13 11:09 ` Miklos Szeredi
2011-01-13 11:16 ` Nick Piggin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).