diff mbox

nfs: check a crash in nfs_lookup_revalidate

Message ID 1305147805-5756-1-git-send-email-shawn.p.huang@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Peng Huang May 11, 2011, 9:03 p.m. UTC 
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(-)

Comments

Trond Myklebust May 11, 2011, 9:08 p.m. UTC | #1 
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;
Peng Huang May 11, 2011, 9:35 p.m. UTC | #2 
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
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Trond Myklebust May 11, 2011, 10:08 p.m. UTC | #3 
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.
Tyler Hicks May 11, 2011, 10:17 p.m. UTC | #4 
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/
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Trond Myklebust May 11, 2011, 10:33 p.m. UTC | #5 
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.
Tyler Hicks May 11, 2011, 11:07 p.m. UTC | #6 
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/
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Hellwig May 13, 2011, 10:32 a.m. UTC | #7 
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.

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peng Huang May 13, 2011, 2:31 p.m. UTC | #8 
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.
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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);