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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
[email protected]
http://www.netapp.com
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.
On Wed May 11, 2011 at 05:08:45PM -0400, Trond Myklebust <[email protected]> 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 <[email protected]>
> > ---
> > 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
> [email protected]
> http://www.netapp.com
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
On Wed May 11, 2011 at 06:33:57PM -0400, Trond Myklebust <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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
> [email protected]
> http://www.netapp.com
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
On Wed, May 11, 2011 at 5:08 PM, Trond Myklebust
<[email protected]> 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 <[email protected]>
>> ---
>> 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
> [email protected]
> http://www.netapp.com
>
>
On Wed, 2011-05-11 at 17:17 -0500, Tyler Hicks wrote:
> On Wed May 11, 2011 at 05:08:45PM -0400, Trond Myklebust <[email protected]> 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 <[email protected]>
> > > ---
> > > 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
[email protected]
http://www.netapp.com
On Wed, 2011-05-11 at 17:35 -0400, Peng Huang wrote:
> On Wed, May 11, 2011 at 5:08 PM, Trond Myklebust
> <[email protected]> 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 <[email protected]>
> >> ---
> >> 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
[email protected]
http://www.netapp.com
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 <[email protected]> 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.
>
>