* [PATCH] nfs: check a crash in nfs_lookup_revalidate
@ 2011-05-11 21:03 Peng Huang
2011-05-11 21:08 ` Trond Myklebust
2011-05-13 10:32 ` Christoph Hellwig
0 siblings, 2 replies; 9+ messages in thread
From: Peng Huang @ 2011-05-11 21:03 UTC (permalink / raw)
To: Trond Myklebust, linux-nfs, linux-kernel; +Cc: Peng Huang
lookup_one_len() may call nfs_loopup_revalidate() with nd == NULL
indirectly, that causes the kernel crash.
RIP: 0010:[<ffffffffa0ba3b41>] [<ffffffffa0ba3b41>]
nfs_lookup_revalidate+0x21/0x4a0 [nfs]
RSP: 0018:ffff88018f00fae8 EFLAGS: 00010286
Call Trace:
[<ffffffff81164a17>] do_revalidate+0x17/0x60
[<ffffffff81164e9b>] __lookup_hash+0xcb/0x140
[<ffffffff811653c4>] lookup_one_len+0x94/0xe0
[<ffffffff81241ef1>] ecryptfs_lookup+0x91/0x1d0
[<ffffffff81164d85>] d_alloc_and_lookup+0x45/0x90
[<ffffffff8116f7b5>] ? d_lookup+0x35/0x60
[<ffffffff811669b2>] do_lookup+0x192/0x2d0
[<ffffffff811763be>] ? vfsmount_lock_local_unlock+0x1e/0x30
[<ffffffff8126d09c>] ? security_inode_permission+0x1c/0x30
[<ffffffff81167d67>] link_path_walk+0x597/0xae0
[<ffffffff8117638e>] ? vfsmount_lock_local_lock+0x1e/0x30
[<ffffffff81165905>] ? path_init_rcu+0xa5/0x210
[<ffffffff8116858b>] do_path_lookup+0x5b/0x140
[<ffffffff811692f7>] user_path_at+0x57/0xa0
[<ffffffff8159fd08>] ? do_page_fault+0x1e8/0x4e0
[<ffffffff8115eb86>] vfs_fstatat+0x46/0x80
[<ffffffff8116b990>] ? filldir+0x0/0xe0
[<ffffffff8115ec2e>] vfs_lstat+0x1e/0x20
[<ffffffff8115ec54>] sys_newlstat+0x24/0x50
[<ffffffff8159c995>] ? page_fault+0x25/0x30
[<ffffffff8100bfc2>] system_call_fastpath+0x16/0x1b
Signed-off-by: Peng Huang <shawn.p.huang@gmail.com>
---
fs/nfs/dir.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 2c3eb33..9452aa5 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1028,7 +1028,7 @@ static int nfs_lookup_revalidate(struct dentry *dentry, struct nameidata *nd)
struct nfs_fattr *fattr = NULL;
int error;
- if (nd->flags & LOOKUP_RCU)
+ if (nd != NULL && nd->flags & LOOKUP_RCU)
return -ECHILD;
parent = dget_parent(dentry);
--
1.7.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] nfs: check a crash in nfs_lookup_revalidate
2011-05-11 21:03 [PATCH] nfs: check a crash in nfs_lookup_revalidate Peng Huang
@ 2011-05-11 21:08 ` Trond Myklebust
2011-05-11 21:35 ` Peng Huang
2011-05-11 22:17 ` Tyler Hicks
2011-05-13 10:32 ` Christoph Hellwig
1 sibling, 2 replies; 9+ messages in thread
From: Trond Myklebust @ 2011-05-11 21:08 UTC (permalink / raw)
To: Peng Huang; +Cc: linux-nfs, linux-kernel
On Wed, 2011-05-11 at 17:03 -0400, Peng Huang wrote:
> lookup_one_len() may call nfs_loopup_revalidate() with nd == NULL
> indirectly, that causes the kernel crash.
>
> RIP: 0010:[<ffffffffa0ba3b41>] [<ffffffffa0ba3b41>]
> nfs_lookup_revalidate+0x21/0x4a0 [nfs]
> RSP: 0018:ffff88018f00fae8 EFLAGS: 00010286
>
> Call Trace:
> [<ffffffff81164a17>] do_revalidate+0x17/0x60
> [<ffffffff81164e9b>] __lookup_hash+0xcb/0x140
> [<ffffffff811653c4>] lookup_one_len+0x94/0xe0
> [<ffffffff81241ef1>] ecryptfs_lookup+0x91/0x1d0
> [<ffffffff81164d85>] d_alloc_and_lookup+0x45/0x90
> [<ffffffff8116f7b5>] ? d_lookup+0x35/0x60
> [<ffffffff811669b2>] do_lookup+0x192/0x2d0
> [<ffffffff811763be>] ? vfsmount_lock_local_unlock+0x1e/0x30
> [<ffffffff8126d09c>] ? security_inode_permission+0x1c/0x30
> [<ffffffff81167d67>] link_path_walk+0x597/0xae0
> [<ffffffff8117638e>] ? vfsmount_lock_local_lock+0x1e/0x30
> [<ffffffff81165905>] ? path_init_rcu+0xa5/0x210
> [<ffffffff8116858b>] do_path_lookup+0x5b/0x140
> [<ffffffff811692f7>] user_path_at+0x57/0xa0
> [<ffffffff8159fd08>] ? do_page_fault+0x1e8/0x4e0
> [<ffffffff8115eb86>] vfs_fstatat+0x46/0x80
> [<ffffffff8116b990>] ? filldir+0x0/0xe0
> [<ffffffff8115ec2e>] vfs_lstat+0x1e/0x20
> [<ffffffff8115ec54>] sys_newlstat+0x24/0x50
> [<ffffffff8159c995>] ? page_fault+0x25/0x30
> [<ffffffff8100bfc2>] system_call_fastpath+0x16/0x1b
>
> Signed-off-by: Peng Huang <shawn.p.huang@gmail.com>
> ---
> fs/nfs/dir.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index 2c3eb33..9452aa5 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -1028,7 +1028,7 @@ static int nfs_lookup_revalidate(struct dentry *dentry, struct nameidata *nd)
> struct nfs_fattr *fattr = NULL;
> int error;
>
> - if (nd->flags & LOOKUP_RCU)
> + if (nd != NULL && nd->flags & LOOKUP_RCU)
> return -ECHILD;
>
> parent = dget_parent(dentry);
That's exactly what Tyler Hicks proposed last week and which was NACKed.
We simply won't support layered filesystems that don't do intents.
IOW: Feel free to change the above to.
if (nd == NULL)
return -EIO;
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust@netapp.com
www.netapp.com
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nfs: check a crash in nfs_lookup_revalidate
2011-05-11 21:08 ` Trond Myklebust
@ 2011-05-11 21:35 ` Peng Huang
2011-05-11 22:08 ` Trond Myklebust
2011-05-11 22:17 ` Tyler Hicks
1 sibling, 1 reply; 9+ messages in thread
From: Peng Huang @ 2011-05-11 21:35 UTC (permalink / raw)
To: Trond Myklebust; +Cc: linux-nfs, linux-kernel
On Wed, May 11, 2011 at 5:08 PM, Trond Myklebust
<Trond.Myklebust@netapp.com> wrote:
> On Wed, 2011-05-11 at 17:03 -0400, Peng Huang wrote:
>> lookup_one_len() may call nfs_loopup_revalidate() with nd == NULL
>> indirectly, that causes the kernel crash.
>>
>> RIP: 0010:[<ffffffffa0ba3b41>] [<ffffffffa0ba3b41>]
>> nfs_lookup_revalidate+0x21/0x4a0 [nfs]
>> RSP: 0018:ffff88018f00fae8 EFLAGS: 00010286
>>
>> Call Trace:
>> [<ffffffff81164a17>] do_revalidate+0x17/0x60
>> [<ffffffff81164e9b>] __lookup_hash+0xcb/0x140
>> [<ffffffff811653c4>] lookup_one_len+0x94/0xe0
>> [<ffffffff81241ef1>] ecryptfs_lookup+0x91/0x1d0
>> [<ffffffff81164d85>] d_alloc_and_lookup+0x45/0x90
>> [<ffffffff8116f7b5>] ? d_lookup+0x35/0x60
>> [<ffffffff811669b2>] do_lookup+0x192/0x2d0
>> [<ffffffff811763be>] ? vfsmount_lock_local_unlock+0x1e/0x30
>> [<ffffffff8126d09c>] ? security_inode_permission+0x1c/0x30
>> [<ffffffff81167d67>] link_path_walk+0x597/0xae0
>> [<ffffffff8117638e>] ? vfsmount_lock_local_lock+0x1e/0x30
>> [<ffffffff81165905>] ? path_init_rcu+0xa5/0x210
>> [<ffffffff8116858b>] do_path_lookup+0x5b/0x140
>> [<ffffffff811692f7>] user_path_at+0x57/0xa0
>> [<ffffffff8159fd08>] ? do_page_fault+0x1e8/0x4e0
>> [<ffffffff8115eb86>] vfs_fstatat+0x46/0x80
>> [<ffffffff8116b990>] ? filldir+0x0/0xe0
>> [<ffffffff8115ec2e>] vfs_lstat+0x1e/0x20
>> [<ffffffff8115ec54>] sys_newlstat+0x24/0x50
>> [<ffffffff8159c995>] ? page_fault+0x25/0x30
>> [<ffffffff8100bfc2>] system_call_fastpath+0x16/0x1b
>>
>> Signed-off-by: Peng Huang <shawn.p.huang@gmail.com>
>> ---
>> fs/nfs/dir.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
>> index 2c3eb33..9452aa5 100644
>> --- a/fs/nfs/dir.c
>> +++ b/fs/nfs/dir.c
>> @@ -1028,7 +1028,7 @@ static int nfs_lookup_revalidate(struct dentry *dentry, struct nameidata *nd)
>> struct nfs_fattr *fattr = NULL;
>> int error;
>>
>> - if (nd->flags & LOOKUP_RCU)
>> + if (nd != NULL && nd->flags & LOOKUP_RCU)
>> return -ECHILD;
>>
>> parent = dget_parent(dentry);
>
> That's exactly what Tyler Hicks proposed last week and which was NACKed.
> We simply won't support layered filesystems that don't do intents.
>
> IOW: Feel free to change the above to.
>
> if (nd == NULL)
> return -EIO;
>
I tested returning -EIO when nd is NULL. Kernel does not crash, but
ecryptfs can not work on nfs anymore.
Peng Huang
>
>
>
> --
> Trond Myklebust
> Linux NFS client maintainer
>
> NetApp
> Trond.Myklebust@netapp.com
> www.netapp.com
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nfs: check a crash in nfs_lookup_revalidate
2011-05-11 21:35 ` Peng Huang
@ 2011-05-11 22:08 ` Trond Myklebust
0 siblings, 0 replies; 9+ messages in thread
From: Trond Myklebust @ 2011-05-11 22:08 UTC (permalink / raw)
To: Peng Huang; +Cc: linux-nfs, linux-kernel
On Wed, 2011-05-11 at 17:35 -0400, Peng Huang wrote:
> On Wed, May 11, 2011 at 5:08 PM, Trond Myklebust
> <Trond.Myklebust@netapp.com> wrote:
> > On Wed, 2011-05-11 at 17:03 -0400, Peng Huang wrote:
> >> lookup_one_len() may call nfs_loopup_revalidate() with nd == NULL
> >> indirectly, that causes the kernel crash.
> >>
> >> RIP: 0010:[<ffffffffa0ba3b41>] [<ffffffffa0ba3b41>]
> >> nfs_lookup_revalidate+0x21/0x4a0 [nfs]
> >> RSP: 0018:ffff88018f00fae8 EFLAGS: 00010286
> >>
> >> Call Trace:
> >> [<ffffffff81164a17>] do_revalidate+0x17/0x60
> >> [<ffffffff81164e9b>] __lookup_hash+0xcb/0x140
> >> [<ffffffff811653c4>] lookup_one_len+0x94/0xe0
> >> [<ffffffff81241ef1>] ecryptfs_lookup+0x91/0x1d0
> >> [<ffffffff81164d85>] d_alloc_and_lookup+0x45/0x90
> >> [<ffffffff8116f7b5>] ? d_lookup+0x35/0x60
> >> [<ffffffff811669b2>] do_lookup+0x192/0x2d0
> >> [<ffffffff811763be>] ? vfsmount_lock_local_unlock+0x1e/0x30
> >> [<ffffffff8126d09c>] ? security_inode_permission+0x1c/0x30
> >> [<ffffffff81167d67>] link_path_walk+0x597/0xae0
> >> [<ffffffff8117638e>] ? vfsmount_lock_local_lock+0x1e/0x30
> >> [<ffffffff81165905>] ? path_init_rcu+0xa5/0x210
> >> [<ffffffff8116858b>] do_path_lookup+0x5b/0x140
> >> [<ffffffff811692f7>] user_path_at+0x57/0xa0
> >> [<ffffffff8159fd08>] ? do_page_fault+0x1e8/0x4e0
> >> [<ffffffff8115eb86>] vfs_fstatat+0x46/0x80
> >> [<ffffffff8116b990>] ? filldir+0x0/0xe0
> >> [<ffffffff8115ec2e>] vfs_lstat+0x1e/0x20
> >> [<ffffffff8115ec54>] sys_newlstat+0x24/0x50
> >> [<ffffffff8159c995>] ? page_fault+0x25/0x30
> >> [<ffffffff8100bfc2>] system_call_fastpath+0x16/0x1b
> >>
> >> Signed-off-by: Peng Huang <shawn.p.huang@gmail.com>
> >> ---
> >> fs/nfs/dir.c | 2 +-
> >> 1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> >> index 2c3eb33..9452aa5 100644
> >> --- a/fs/nfs/dir.c
> >> +++ b/fs/nfs/dir.c
> >> @@ -1028,7 +1028,7 @@ static int nfs_lookup_revalidate(struct dentry *dentry, struct nameidata *nd)
> >> struct nfs_fattr *fattr = NULL;
> >> int error;
> >>
> >> - if (nd->flags & LOOKUP_RCU)
> >> + if (nd != NULL && nd->flags & LOOKUP_RCU)
> >> return -ECHILD;
> >>
> >> parent = dget_parent(dentry);
> >
> > That's exactly what Tyler Hicks proposed last week and which was NACKed.
> > We simply won't support layered filesystems that don't do intents.
> >
> > IOW: Feel free to change the above to.
> >
> > if (nd == NULL)
> > return -EIO;
> >
>
> I tested returning -EIO when nd is NULL. Kernel does not crash, but
> ecryptfs can not work on nfs anymore.
It isn't going to work until ecryptfs gets fixed. Only blind luck made
it 'work' previously.
The above will at least ensure that if someone tries to use it over NFS,
then we won't Oops, and they will get a valid error message.
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust@netapp.com
www.netapp.com
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nfs: check a crash in nfs_lookup_revalidate
2011-05-11 21:08 ` Trond Myklebust
2011-05-11 21:35 ` Peng Huang
@ 2011-05-11 22:17 ` Tyler Hicks
2011-05-11 22:33 ` Trond Myklebust
1 sibling, 1 reply; 9+ messages in thread
From: Tyler Hicks @ 2011-05-11 22:17 UTC (permalink / raw)
To: Trond Myklebust; +Cc: Peng Huang, linux-nfs, linux-kernel
On Wed May 11, 2011 at 05:08:45PM -0400, Trond Myklebust <Trond.Myklebust@netapp.com> wrote:
> On Wed, 2011-05-11 at 17:03 -0400, Peng Huang wrote:
> > lookup_one_len() may call nfs_loopup_revalidate() with nd == NULL
> > indirectly, that causes the kernel crash.
> >
> > RIP: 0010:[<ffffffffa0ba3b41>] [<ffffffffa0ba3b41>]
> > nfs_lookup_revalidate+0x21/0x4a0 [nfs]
> > RSP: 0018:ffff88018f00fae8 EFLAGS: 00010286
> >
> > Call Trace:
> > [<ffffffff81164a17>] do_revalidate+0x17/0x60
> > [<ffffffff81164e9b>] __lookup_hash+0xcb/0x140
> > [<ffffffff811653c4>] lookup_one_len+0x94/0xe0
> > [<ffffffff81241ef1>] ecryptfs_lookup+0x91/0x1d0
> > [<ffffffff81164d85>] d_alloc_and_lookup+0x45/0x90
> > [<ffffffff8116f7b5>] ? d_lookup+0x35/0x60
> > [<ffffffff811669b2>] do_lookup+0x192/0x2d0
> > [<ffffffff811763be>] ? vfsmount_lock_local_unlock+0x1e/0x30
> > [<ffffffff8126d09c>] ? security_inode_permission+0x1c/0x30
> > [<ffffffff81167d67>] link_path_walk+0x597/0xae0
> > [<ffffffff8117638e>] ? vfsmount_lock_local_lock+0x1e/0x30
> > [<ffffffff81165905>] ? path_init_rcu+0xa5/0x210
> > [<ffffffff8116858b>] do_path_lookup+0x5b/0x140
> > [<ffffffff811692f7>] user_path_at+0x57/0xa0
> > [<ffffffff8159fd08>] ? do_page_fault+0x1e8/0x4e0
> > [<ffffffff8115eb86>] vfs_fstatat+0x46/0x80
> > [<ffffffff8116b990>] ? filldir+0x0/0xe0
> > [<ffffffff8115ec2e>] vfs_lstat+0x1e/0x20
> > [<ffffffff8115ec54>] sys_newlstat+0x24/0x50
> > [<ffffffff8159c995>] ? page_fault+0x25/0x30
> > [<ffffffff8100bfc2>] system_call_fastpath+0x16/0x1b
> >
> > Signed-off-by: Peng Huang <shawn.p.huang@gmail.com>
> > ---
> > fs/nfs/dir.c | 2 +-
> > 1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> > index 2c3eb33..9452aa5 100644
> > --- a/fs/nfs/dir.c
> > +++ b/fs/nfs/dir.c
> > @@ -1028,7 +1028,7 @@ static int nfs_lookup_revalidate(struct dentry *dentry, struct nameidata *nd)
> > struct nfs_fattr *fattr = NULL;
> > int error;
> >
> > - if (nd->flags & LOOKUP_RCU)
> > + if (nd != NULL && nd->flags & LOOKUP_RCU)
> > return -ECHILD;
> >
> > parent = dget_parent(dentry);
>
> That's exactly what Tyler Hicks proposed last week and which was NACKed.
> We simply won't support layered filesystems that don't do intents.
But you _did_ support it up until
34286d66 "fs: rcu-walk aware d_revalidate method"
I see why you wouldn't want NULL nameidata in the NFSv4 specific
functions, but don't quite understand the opposition against it in NFSv3
(nfs_lookup_revalidate). The one-liner above would allow users to begin
using eCryptfs on top of NFSv3 clients immediately, with no side effects
to NFS.
Tyler
>
> IOW: Feel free to change the above to.
>
> if (nd == NULL)
> return -EIO;
>
>
>
>
> --
> Trond Myklebust
> Linux NFS client maintainer
>
> NetApp
> Trond.Myklebust@netapp.com
> www.netapp.com
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nfs: check a crash in nfs_lookup_revalidate
2011-05-11 22:17 ` Tyler Hicks
@ 2011-05-11 22:33 ` Trond Myklebust
2011-05-11 23:07 ` Tyler Hicks
0 siblings, 1 reply; 9+ messages in thread
From: Trond Myklebust @ 2011-05-11 22:33 UTC (permalink / raw)
To: Tyler Hicks; +Cc: Peng Huang, linux-nfs, linux-kernel
On Wed, 2011-05-11 at 17:17 -0500, Tyler Hicks wrote:
> On Wed May 11, 2011 at 05:08:45PM -0400, Trond Myklebust <Trond.Myklebust@netapp.com> wrote:
> > On Wed, 2011-05-11 at 17:03 -0400, Peng Huang wrote:
> > > lookup_one_len() may call nfs_loopup_revalidate() with nd == NULL
> > > indirectly, that causes the kernel crash.
> > >
> > > RIP: 0010:[<ffffffffa0ba3b41>] [<ffffffffa0ba3b41>]
> > > nfs_lookup_revalidate+0x21/0x4a0 [nfs]
> > > RSP: 0018:ffff88018f00fae8 EFLAGS: 00010286
> > >
> > > Call Trace:
> > > [<ffffffff81164a17>] do_revalidate+0x17/0x60
> > > [<ffffffff81164e9b>] __lookup_hash+0xcb/0x140
> > > [<ffffffff811653c4>] lookup_one_len+0x94/0xe0
> > > [<ffffffff81241ef1>] ecryptfs_lookup+0x91/0x1d0
> > > [<ffffffff81164d85>] d_alloc_and_lookup+0x45/0x90
> > > [<ffffffff8116f7b5>] ? d_lookup+0x35/0x60
> > > [<ffffffff811669b2>] do_lookup+0x192/0x2d0
> > > [<ffffffff811763be>] ? vfsmount_lock_local_unlock+0x1e/0x30
> > > [<ffffffff8126d09c>] ? security_inode_permission+0x1c/0x30
> > > [<ffffffff81167d67>] link_path_walk+0x597/0xae0
> > > [<ffffffff8117638e>] ? vfsmount_lock_local_lock+0x1e/0x30
> > > [<ffffffff81165905>] ? path_init_rcu+0xa5/0x210
> > > [<ffffffff8116858b>] do_path_lookup+0x5b/0x140
> > > [<ffffffff811692f7>] user_path_at+0x57/0xa0
> > > [<ffffffff8159fd08>] ? do_page_fault+0x1e8/0x4e0
> > > [<ffffffff8115eb86>] vfs_fstatat+0x46/0x80
> > > [<ffffffff8116b990>] ? filldir+0x0/0xe0
> > > [<ffffffff8115ec2e>] vfs_lstat+0x1e/0x20
> > > [<ffffffff8115ec54>] sys_newlstat+0x24/0x50
> > > [<ffffffff8159c995>] ? page_fault+0x25/0x30
> > > [<ffffffff8100bfc2>] system_call_fastpath+0x16/0x1b
> > >
> > > Signed-off-by: Peng Huang <shawn.p.huang@gmail.com>
> > > ---
> > > fs/nfs/dir.c | 2 +-
> > > 1 files changed, 1 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> > > index 2c3eb33..9452aa5 100644
> > > --- a/fs/nfs/dir.c
> > > +++ b/fs/nfs/dir.c
> > > @@ -1028,7 +1028,7 @@ static int nfs_lookup_revalidate(struct dentry *dentry, struct nameidata *nd)
> > > struct nfs_fattr *fattr = NULL;
> > > int error;
> > >
> > > - if (nd->flags & LOOKUP_RCU)
> > > + if (nd != NULL && nd->flags & LOOKUP_RCU)
> > > return -ECHILD;
> > >
> > > parent = dget_parent(dentry);
> >
> > That's exactly what Tyler Hicks proposed last week and which was NACKed.
> > We simply won't support layered filesystems that don't do intents.
>
> But you _did_ support it up until
> 34286d66 "fs: rcu-walk aware d_revalidate method"
>
> I see why you wouldn't want NULL nameidata in the NFSv4 specific
> functions, but don't quite understand the opposition against it in NFSv3
> (nfs_lookup_revalidate). The one-liner above would allow users to begin
> using eCryptfs on top of NFSv3 clients immediately, with no side effects
> to NFS.
Because even on NFSv3 it breaks exclusive creates.
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust@netapp.com
www.netapp.com
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nfs: check a crash in nfs_lookup_revalidate
2011-05-11 22:33 ` Trond Myklebust
@ 2011-05-11 23:07 ` Tyler Hicks
0 siblings, 0 replies; 9+ messages in thread
From: Tyler Hicks @ 2011-05-11 23:07 UTC (permalink / raw)
To: Trond Myklebust; +Cc: Peng Huang, linux-nfs, linux-kernel
On Wed May 11, 2011 at 06:33:57PM -0400, Trond Myklebust <Trond.Myklebust@netapp.com> wrote:
> On Wed, 2011-05-11 at 17:17 -0500, Tyler Hicks wrote:
> > On Wed May 11, 2011 at 05:08:45PM -0400, Trond Myklebust <Trond.Myklebust@netapp.com> wrote:
> > > On Wed, 2011-05-11 at 17:03 -0400, Peng Huang wrote:
> > > > lookup_one_len() may call nfs_loopup_revalidate() with nd == NULL
> > > > indirectly, that causes the kernel crash.
> > > >
> > > > RIP: 0010:[<ffffffffa0ba3b41>] [<ffffffffa0ba3b41>]
> > > > nfs_lookup_revalidate+0x21/0x4a0 [nfs]
> > > > RSP: 0018:ffff88018f00fae8 EFLAGS: 00010286
> > > >
> > > > Call Trace:
> > > > [<ffffffff81164a17>] do_revalidate+0x17/0x60
> > > > [<ffffffff81164e9b>] __lookup_hash+0xcb/0x140
> > > > [<ffffffff811653c4>] lookup_one_len+0x94/0xe0
> > > > [<ffffffff81241ef1>] ecryptfs_lookup+0x91/0x1d0
> > > > [<ffffffff81164d85>] d_alloc_and_lookup+0x45/0x90
> > > > [<ffffffff8116f7b5>] ? d_lookup+0x35/0x60
> > > > [<ffffffff811669b2>] do_lookup+0x192/0x2d0
> > > > [<ffffffff811763be>] ? vfsmount_lock_local_unlock+0x1e/0x30
> > > > [<ffffffff8126d09c>] ? security_inode_permission+0x1c/0x30
> > > > [<ffffffff81167d67>] link_path_walk+0x597/0xae0
> > > > [<ffffffff8117638e>] ? vfsmount_lock_local_lock+0x1e/0x30
> > > > [<ffffffff81165905>] ? path_init_rcu+0xa5/0x210
> > > > [<ffffffff8116858b>] do_path_lookup+0x5b/0x140
> > > > [<ffffffff811692f7>] user_path_at+0x57/0xa0
> > > > [<ffffffff8159fd08>] ? do_page_fault+0x1e8/0x4e0
> > > > [<ffffffff8115eb86>] vfs_fstatat+0x46/0x80
> > > > [<ffffffff8116b990>] ? filldir+0x0/0xe0
> > > > [<ffffffff8115ec2e>] vfs_lstat+0x1e/0x20
> > > > [<ffffffff8115ec54>] sys_newlstat+0x24/0x50
> > > > [<ffffffff8159c995>] ? page_fault+0x25/0x30
> > > > [<ffffffff8100bfc2>] system_call_fastpath+0x16/0x1b
> > > >
> > > > Signed-off-by: Peng Huang <shawn.p.huang@gmail.com>
> > > > ---
> > > > fs/nfs/dir.c | 2 +-
> > > > 1 files changed, 1 insertions(+), 1 deletions(-)
> > > >
> > > > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> > > > index 2c3eb33..9452aa5 100644
> > > > --- a/fs/nfs/dir.c
> > > > +++ b/fs/nfs/dir.c
> > > > @@ -1028,7 +1028,7 @@ static int nfs_lookup_revalidate(struct dentry *dentry, struct nameidata *nd)
> > > > struct nfs_fattr *fattr = NULL;
> > > > int error;
> > > >
> > > > - if (nd->flags & LOOKUP_RCU)
> > > > + if (nd != NULL && nd->flags & LOOKUP_RCU)
> > > > return -ECHILD;
> > > >
> > > > parent = dget_parent(dentry);
> > >
> > > That's exactly what Tyler Hicks proposed last week and which was NACKed.
> > > We simply won't support layered filesystems that don't do intents.
> >
> > But you _did_ support it up until
> > 34286d66 "fs: rcu-walk aware d_revalidate method"
> >
> > I see why you wouldn't want NULL nameidata in the NFSv4 specific
> > functions, but don't quite understand the opposition against it in NFSv3
> > (nfs_lookup_revalidate). The one-liner above would allow users to begin
> > using eCryptfs on top of NFSv3 clients immediately, with no side effects
> > to NFS.
>
> Because even on NFSv3 it breaks exclusive creates.
I somehow missed that bit of code in nfs_lookup(). Thanks for the
pointer.
I'll start getting the eCryptfs lookup code straightened out.
Tyler
>
> --
> Trond Myklebust
> Linux NFS client maintainer
>
> NetApp
> Trond.Myklebust@netapp.com
> www.netapp.com
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nfs: check a crash in nfs_lookup_revalidate
2011-05-11 21:03 [PATCH] nfs: check a crash in nfs_lookup_revalidate Peng Huang
2011-05-11 21:08 ` Trond Myklebust
@ 2011-05-13 10:32 ` Christoph Hellwig
2011-05-13 14:31 ` Peng Huang
1 sibling, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2011-05-13 10:32 UTC (permalink / raw)
To: Peng Huang; +Cc: Trond Myklebust, linux-nfs, linux-kernel
On Wed, May 11, 2011 at 05:03:25PM -0400, Peng Huang wrote:
> lookup_one_len() may call nfs_loopup_revalidate() with nd == NULL
> indirectly, that causes the kernel crash.
lookup_one_len must only be called by a filesystem or a library function
called by the filesystem. You are not allowed to call it on a random
filesystem like nfs that doesn't support the underlying assumptions.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] nfs: check a crash in nfs_lookup_revalidate
2011-05-13 10:32 ` Christoph Hellwig
@ 2011-05-13 14:31 ` Peng Huang
0 siblings, 0 replies; 9+ messages in thread
From: Peng Huang @ 2011-05-13 14:31 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Trond Myklebust, linux-nfs, linux-kernel
I did not write any code to call lookup_one_len(). I just mounted an
ecryptfs on nfs, and then got the oops. So it should be a problem in
nfs or ecryptfs or both. At least it should not crash.
Peng
On Fri, May 13, 2011 at 6:32 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Wed, May 11, 2011 at 05:03:25PM -0400, Peng Huang wrote:
>> lookup_one_len() may call nfs_loopup_revalidate() with nd == NULL
>> indirectly, that causes the kernel crash.
>
> lookup_one_len must only be called by a filesystem or a library function
> called by the filesystem. You are not allowed to call it on a random
> filesystem like nfs that doesn't support the underlying assumptions.
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2011-05-13 14:31 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-05-11 21:03 [PATCH] nfs: check a crash in nfs_lookup_revalidate Peng Huang
2011-05-11 21:08 ` Trond Myklebust
2011-05-11 21:35 ` Peng Huang
2011-05-11 22:08 ` Trond Myklebust
2011-05-11 22:17 ` Tyler Hicks
2011-05-11 22:33 ` Trond Myklebust
2011-05-11 23:07 ` Tyler Hicks
2011-05-13 10:32 ` Christoph Hellwig
2011-05-13 14:31 ` Peng Huang
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).