CIFS fails to automount DFS shares

CIFS fails to automount DFS shares

[Gerlando Falauto]

Hi,

I am new here, so please forgive me if I am abusing something or being
too verbose...
I found a problem with samba/CIFS when the server uses DFS.
The same problem (or at least a similar one) has also been reported in:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/740062
https://bugs.launchpad.net/ubuntu/+source/cifs-utils/+bug/809243

What happens is that when you mount a SAMBA share which has DFS folders
(i.e. folders which are physically located at another server, sort-of a
remote symlink), you get the following error when you try to access that
directory. 

$ mount -t cifs //server/dfs /mnt/dfs
$ cd /mnt/dfs
$ ls -l
total 0
drwx------ 0 user     root 0 May  4  2010 test
$ cd test
bash: cd: test: Is a directory

What should happen instead is that the remote share should get
automagically mounted under "/mnt/dfs/test".

Apparently it all started here:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=01c64feac45cea1317263eabc4f7ee1b240f297f

which introduced automount for CIFS mounts as well (as opposed to
simulating a symlink), and was admittedly untested.
So apparently this problem has been lying around since 2.6.38-rc1.

I did some futher investigation and found that this behaviour only
appears when the name of the directory in question has been cached
somehow.
So, for instance, if you do not "ls" on "/mnt/dfs" before tryint to "cd
test", the remote share gets happily mounted.
Everything also works when trying to cd into "test" spelled with a
different case: "cd Test", "cd TEST", cd "TeSt" would always succeed.
Also in that case the direntry would not be present.
The missing point in the broken case is that follow_automount() is not
called beacuse DCACHE_NEED_AUTOMOUNT is not set, and apparently
d_instantiate() is the only way to go.

This patch seems to fix the problem by marking as no-longer-valid a
dentry whose inode turned out to be of automount type; I am no expert in
this area so I am not quite sure this is the best way to approach this.

Particularly, according to Documentation/filesystems/vfs.txt:
<<<
d_revalidate may be called in rcu-walk mode (nd->flags & LOOKUP_RCU).
If in rcu-walk mode, the filesystem must revalidate the dentry without
blocking or storing to the dentry, d_parent and d_inode should not be
used without care (because they can go NULL), instead nd->inode should
be used.
>>>

but nd->inode is definitely something different than direntry->d_inode.

Thank you.
Gerlando

P.S. I just noticed how nfs_lookup_verify_inode() does pretty much the
same thing, so maybe my proposal is not that foolish after all.

Gerlando Falauto (1):
  CIFS: fix automount for DFS shares

 fs/cifs/dir.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

[PATCH] CIFS: fix automount for DFS shares

[Gerlando Falauto]

Automounting directories are now invalidated by .d_revalidate()
so to be d_instantiate()d again with the right DCACHE_NEED_AUTOMOUNT
flag

Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>
---
 fs/cifs/dir.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index 9ea65cf..67f54d3 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -637,8 +637,13 @@ cifs_d_revalidate(struct dentry *direntry, struct nameidata *nd)
 	if (direntry->d_inode) {
 		if (cifs_revalidate_dentry(direntry))
 			return 0;
-		else
+		else {
+			/* We want automonting inodes to be
+			 * considered invalid or so */
+			if (IS_AUTOMOUNT(direntry->d_inode))
+				return 0;
 			return 1;
+		}
 	}
 
 	/*
-- 
1.7.1

Re: [PATCH] CIFS: fix automount for DFS shares

[Ian Kent]

On Fri, 2011-10-07 at 13:09 +0200, Gerlando Falauto wrote:
> Automounting directories are now invalidated by .d_revalidate()
> so to be d_instantiate()d again with the right DCACHE_NEED_AUTOMOUNT
> flag

But why doesn't CIFS know this is a DFS inode the first time around, it
appears to do a truck load of work looking that stuff up?

> 
> Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>
> ---
>  fs/cifs/dir.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> index 9ea65cf..67f54d3 100644
> --- a/fs/cifs/dir.c
> +++ b/fs/cifs/dir.c
> @@ -637,8 +637,13 @@ cifs_d_revalidate(struct dentry *direntry, struct nameidata *nd)
>  	if (direntry->d_inode) {
>  		if (cifs_revalidate_dentry(direntry))
>  			return 0;
> -		else
> +		else {
> +			/* We want automonting inodes to be
> +			 * considered invalid or so */
> +			if (IS_AUTOMOUNT(direntry->d_inode))
> +				return 0;

I'd be inclined to set DCACHE_NEED_AUTOMOUNT here but are we certain
that cifs_revalidate_dentry() will always return 0 for a DFS inode or at
least ones that don't yet have DCACHE_NEED_AUTOMOUNT set and why?

>  			return 1;
> +		}
>  	}
>  
>  	/*

Re: [PATCH] CIFS: fix automount for DFS shares

[Jeff Layton]

On Sun, 23 Oct 2011 19:14:34 +0800
Ian Kent <raven@themaw.net> wrote:

> On Fri, 2011-10-07 at 13:09 +0200, Gerlando Falauto wrote:
> > Automounting directories are now invalidated by .d_revalidate()
> > so to be d_instantiate()d again with the right DCACHE_NEED_AUTOMOUNT
> > flag
> 
> But why doesn't CIFS know this is a DFS inode the first time around, it
> appears to do a truck load of work looking that stuff up?
> 

This area needs some work...

The readdir codepath in cifs uses the FIND_FIRST/NEXT calls on the
wire. Those return both filenames and attributes for the particular SMB
call infolevel. That in turn is used to instantiate dentries and inodes
for those entries.

Unfortunately though, we have not found a way to determine whether a
particular inode is a DFS referral from within this codepath. The only
way to know for sure (AFAIK) is to try an operation on a specific
pathname and then look for a NT_STATUS_PATH_NOT_COVERED return code.

> > 
> > Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>
> > ---
> >  fs/cifs/dir.c |    7 ++++++-
> >  1 files changed, 6 insertions(+), 1 deletions(-)
> > 
> > diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> > index 9ea65cf..67f54d3 100644
> > --- a/fs/cifs/dir.c
> > +++ b/fs/cifs/dir.c
> > @@ -637,8 +637,13 @@ cifs_d_revalidate(struct dentry *direntry, struct nameidata *nd)
> >  	if (direntry->d_inode) {
> >  		if (cifs_revalidate_dentry(direntry))
> >  			return 0;
> > -		else
> > +		else {
> > +			/* We want automonting inodes to be
> > +			 * considered invalid or so */
> > +			if (IS_AUTOMOUNT(direntry->d_inode))
> > +				return 0;
> 
> I'd be inclined to set DCACHE_NEED_AUTOMOUNT here but are we certain
> that cifs_revalidate_dentry() will always return 0 for a DFS inode or at
> least ones that don't yet have DCACHE_NEED_AUTOMOUNT set and why?
> 

You mean you'd set that in the "return 1" case here? That doesn't sound
quite right if so. This inode could be entirely unrelated to DFS.

Currently, cifs_revalidate_dentry checks to see if the attributes on
the inode are "too old" (past the actimeo setting). If they are then it
will issue a QUERY_PATH_INFO call on the wire to try and update those
attributes. If it's a DFS inode then you'll get an error back and that
function should return 0.

It's possible however that the inode has already had its attributes
updated recently, and was found to be an automount point. That is, it
got S_AUTOMOUNT set but no referral chasing happened. At that point I'm
a little fuzzy as to what should happen...

The safest thing would seem to be to return 0 in that case, to force an
invalidation and lookup on this dentry again, but maybe there's a better
way to handle that?

> >  			return 1;
> > +		}
> >  	}
> >  
> >  	/*
> 
> 


-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] CIFS: fix automount for DFS shares

[Ian Kent]

On Sun, 2011-10-23 at 07:46 -0400, Jeff Layton wrote:
> On Sun, 23 Oct 2011 19:14:34 +0800
> Ian Kent <raven@themaw.net> wrote:
> 
> > On Fri, 2011-10-07 at 13:09 +0200, Gerlando Falauto wrote:
> > > Automounting directories are now invalidated by .d_revalidate()
> > > so to be d_instantiate()d again with the right DCACHE_NEED_AUTOMOUNT
> > > flag
> > 
> > But why doesn't CIFS know this is a DFS inode the first time around, it
> > appears to do a truck load of work looking that stuff up?
> > 
> 
> This area needs some work...
> 
> The readdir codepath in cifs uses the FIND_FIRST/NEXT calls on the
> wire. Those return both filenames and attributes for the particular SMB
> call infolevel. That in turn is used to instantiate dentries and inodes
> for those entries.
> 
> Unfortunately though, we have not found a way to determine whether a
> particular inode is a DFS referral from within this codepath. The only
> way to know for sure (AFAIK) is to try an operation on a specific
> pathname and then look for a NT_STATUS_PATH_NOT_COVERED return code.
> 
> > > 
> > > Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>
> > > ---
> > >  fs/cifs/dir.c |    7 ++++++-
> > >  1 files changed, 6 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> > > index 9ea65cf..67f54d3 100644
> > > --- a/fs/cifs/dir.c
> > > +++ b/fs/cifs/dir.c
> > > @@ -637,8 +637,13 @@ cifs_d_revalidate(struct dentry *direntry, struct nameidata *nd)
> > >  	if (direntry->d_inode) {
> > >  		if (cifs_revalidate_dentry(direntry))
> > >  			return 0;
> > > -		else
> > > +		else {
> > > +			/* We want automonting inodes to be
> > > +			 * considered invalid or so */
> > > +			if (IS_AUTOMOUNT(direntry->d_inode))
> > > +				return 0;
> > 
> > I'd be inclined to set DCACHE_NEED_AUTOMOUNT here but are we certain
> > that cifs_revalidate_dentry() will always return 0 for a DFS inode or at
> > least ones that don't yet have DCACHE_NEED_AUTOMOUNT set and why?
> > 
> 
> You mean you'd set that in the "return 1" case here? That doesn't sound
> quite right if so. This inode could be entirely unrelated to DFS.

Actually, the return code might not matter. The current code returns 1
if cifs_revalidate_dentry() returns 0, so yes, that's what I'm saying.
Setting DCACHE_NEED_AUTOMOUNT in d_flags if S_AUTOMOUNT() is set is
enough to trigger the mount and S_AUTOMOUNT() will be set if it's been
seen as a DFS point.

If cifs_revalidate_dentry() returns 1 it returns false and the lookup
gets re-done so the other case looks like it handles the case where we
now know the thing is a DFS inode but haven't set the dcache flag since
the dentry wasn't actually instantiated.

> 
> Currently, cifs_revalidate_dentry checks to see if the attributes on
> the inode are "too old" (past the actimeo setting). If they are then it
> will issue a QUERY_PATH_INFO call on the wire to try and update those
> attributes. If it's a DFS inode then you'll get an error back and that
> function should return 0.
> 
> It's possible however that the inode has already had its attributes
> updated recently, and was found to be an automount point. That is, it
> got S_AUTOMOUNT set but no referral chasing happened. At that point I'm
> a little fuzzy as to what should happen...

Right, from the POV of CIFS .... the automounting bit is ....

The automounting happens during the follow after the lookup/revalidate
of the path component if the appropriate flag(s) is set. The follow will
be done for each mountpoint dentry in the stack, calling ->d_manage() if
needed, for each and once the last one is reached ->d_automount() is
called if it is needed. That's a bit simplified but is basically the way
it's done.

What I'm suggesting is that there's no need to allocate a new dentry
when the attributes have now been filled in and the dentry is seen as a
DFS point.

Not sure how that will go with multiple concurrent walks though.

> 
> The safest thing would seem to be to return 0 in that case, to force an
> invalidation and lookup on this dentry again, but maybe there's a better
> way to handle that?

Who else can we consult on this?

> 
> > >  			return 1;
> > > +		}
> > >  	}
> > >  
> > >  	/*
> > 
> > 
> 
> 

CIFS fails to automount DFS shares

[Gerlando Falauto]

Hi,

I am new here, so please forgive me if I am abusing something or being
too verbose...
I found a problem with samba/CIFS when the server uses DFS.
The same problem (or at least a similar one) has also been reported in:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/740062
https://bugs.launchpad.net/ubuntu/+source/cifs-utils/+bug/809243

What happens is that when you mount a SAMBA share which has DFS folders
(i.e. folders which are physically located at another server, sort-of a
remote symlink), you get the following error when you try to access that
directory. 

$ mount -t cifs //server/dfs /mnt/dfs
$ cd /mnt/dfs
$ ls -l
total 0
drwx------ 0 user     root 0 May  4  2010 test
$ cd test
bash: cd: test: Is a directory

What should happen instead is that the remote share should get
automagically mounted under "/mnt/dfs/test".

Apparently it all started here:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=01c64feac45cea1317263eabc4f7ee1b240f297f

which introduced automount for CIFS mounts as well (as opposed to
simulating a symlink), and was admittedly untested.
So apparently this problem has been lying around since 2.6.38-rc1.

I did some futher investigation and found that this behaviour only
appears when the name of the directory in question has been cached
somehow.
So, for instance, if you do not "ls" on "/mnt/dfs" before tryint to "cd
test", the remote share gets happily mounted.
Everything also works when trying to cd into "test" spelled with a
different case: "cd Test", "cd TEST", cd "TeSt" would always succeed.
Also in that case the direntry would not be present.
The missing point in the broken case is that follow_automount() is not
called beacuse DCACHE_NEED_AUTOMOUNT is not set, and apparently
d_instantiate() is the only way to go.

This patch seems to fix the problem by marking as no-longer-valid a
dentry whose inode turned out to be of automount type; I am no expert in
this area so I am not quite sure this is the best way to approach this.

Particularly, according to Documentation/filesystems/vfs.txt:
(((
d_revalidate may be called in rcu-walk mode (nd->flags & LOOKUP_RCU).
If in rcu-walk mode, the filesystem must revalidate the dentry without
blocking or storing to the dentry, d_parent and d_inode should not be
used without care (because they can go NULL), instead nd->inode should
be used.
)))

but nd->inode is definitely something different than direntry->d_inode.

Thank you.
Gerlando

P.S. I just noticed how nfs_lookup_verify_inode() does pretty much the
same thing, so maybe my proposal is not that foolish after all.

Gerlando Falauto (1):
  CIFS: fix automount for DFS shares

 fs/cifs/dir.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

[PATCH] CIFS: fix automount for DFS shares

[Gerlando Falauto]

Automounting directories are now invalidated by .d_revalidate()
so to be d_instantiate()d again with the right DCACHE_NEED_AUTOMOUNT
flag

Signed-off-by: Gerlando Falauto <gerlando.falauto-SkAbAL50j+5BDgjK7y7TUQ@public.gmane.org>
---
 fs/cifs/dir.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index 9ea65cf..67f54d3 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -637,8 +637,13 @@ cifs_d_revalidate(struct dentry *direntry, struct nameidata *nd)
 	if (direntry->d_inode) {
 		if (cifs_revalidate_dentry(direntry))
 			return 0;
-		else
+		else {
+			/* We want automonting inodes to be
+			 * considered invalid or so */
+			if (IS_AUTOMOUNT(direntry->d_inode))
+				return 0;
 			return 1;
+		}
 	}
 
 	/*
-- 
1.7.1

Re: [PATCH] CIFS: fix automount for DFS shares

[Jeff Layton]

On Mon, 17 Oct 2011 11:59:03 +0200
Gerlando Falauto <gerlando.falauto-SkAbAL50j+5BDgjK7y7TUQ@public.gmane.org> wrote:

> Automounting directories are now invalidated by .d_revalidate()
> so to be d_instantiate()d again with the right DCACHE_NEED_AUTOMOUNT
> flag
> 
> Signed-off-by: Gerlando Falauto <gerlando.falauto-SkAbAL50j+5BDgjK7y7TUQ@public.gmane.org>
> ---
>  fs/cifs/dir.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> index 9ea65cf..67f54d3 100644
> --- a/fs/cifs/dir.c
> +++ b/fs/cifs/dir.c
> @@ -637,8 +637,13 @@ cifs_d_revalidate(struct dentry *direntry, struct nameidata *nd)
>  	if (direntry->d_inode) {
>  		if (cifs_revalidate_dentry(direntry))
>  			return 0;
> -		else
> +		else {
> +			/* We want automonting inodes to be
> +			 * considered invalid or so */

The comment is a little unclear. A better comment might be -- "Forcibly
invalidate automounting inodes", or something along those lines...

> +			if (IS_AUTOMOUNT(direntry->d_inode))
> +				return 0;
>  			return 1;
> +		}
>  	}
>  
>  	/*

The patch itself looks reasonable though...

Reviewed-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH] CIFS: fix automount for DFS shares

[Steve French]

Would you respin to add the reviewed-by (and any more you got) and fix
the comment

On Mon, Oct 17, 2011 at 5:45 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> On Mon, 17 Oct 2011 11:59:03 +0200
> Gerlando Falauto <gerlando.falauto-SkAbAL50j+5BDgjK7y7TUQ@public.gmane.org> wrote:
>
>> Automounting directories are now invalidated by .d_revalidate()
>> so to be d_instantiate()d again with the right DCACHE_NEED_AUTOMOUNT
>> flag
>>
>> Signed-off-by: Gerlando Falauto <gerlando.falauto-SkAbAL50j+5BDgjK7y7TUQ@public.gmane.org>
>> ---
>>  fs/cifs/dir.c |    7 ++++++-
>>  1 files changed, 6 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
>> index 9ea65cf..67f54d3 100644
>> --- a/fs/cifs/dir.c
>> +++ b/fs/cifs/dir.c
>> @@ -637,8 +637,13 @@ cifs_d_revalidate(struct dentry *direntry, struct nameidata *nd)
>>       if (direntry->d_inode) {
>>               if (cifs_revalidate_dentry(direntry))
>>                       return 0;
>> -             else
>> +             else {
>> +                     /* We want automonting inodes to be
>> +                      * considered invalid or so */
>
> The comment is a little unclear. A better comment might be -- "Forcibly
> invalidate automounting inodes", or something along those lines...
>
>> +                     if (IS_AUTOMOUNT(direntry->d_inode))
>> +                             return 0;
>>                       return 1;
>> +             }
>>       }
>>
>>       /*
>
> The patch itself looks reasonable though...
>
> Reviewed-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
Thanks,

Steve

[PATCH] CIFS: fix automount for DFS shares

[Gerlando Falauto]

Automounting directories are now invalidated by .d_revalidate()
so to be d_instantiate()d again with the right DCACHE_NEED_AUTOMOUNT
flag

Reviewed-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
Signed-off-by: Gerlando Falauto <gerlando.falauto-SkAbAL50j+5BDgjK7y7TUQ@public.gmane.org>
---
 fs/cifs/dir.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index 9ea65cf..ca3b571 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -637,8 +637,16 @@ cifs_d_revalidate(struct dentry *direntry, struct nameidata *nd)
 	if (direntry->d_inode) {
 		if (cifs_revalidate_dentry(direntry))
 			return 0;
-		else
+		else {
+			/*
+			 * Forcibly invalidate automounting directory inodes
+			 * (remote DFS directories) so to have them
+			 * instantiated again for automount
+			 */
+			if (IS_AUTOMOUNT(direntry->d_inode))
+				return 0;
 			return 1;
+		}
 	}
 
 	/*
-- 
1.7.1

[PATCH] CIFS: fix automount for DFS shares

[Gerlando Falauto]

Automounting directories are now invalidated by .d_revalidate()
so to be d_instantiate()d again with the right DCACHE_NEED_AUTOMOUNT
flag

Reviewed-by: Jeff Layton <jlayton@samba.org>
Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>
---
 fs/cifs/dir.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index 9ea65cf..ca3b571 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -637,8 +637,16 @@ cifs_d_revalidate(struct dentry *direntry, struct nameidata *nd)
 	if (direntry->d_inode) {
 		if (cifs_revalidate_dentry(direntry))
 			return 0;
-		else
+		else {
+			/*
+			 * Forcibly invalidate automounting directory inodes
+			 * (remote DFS directories) so to have them
+			 * instantiated again for automount
+			 */
+			if (IS_AUTOMOUNT(direntry->d_inode))
+				return 0;
 			return 1;
+		}
 	}
 
 	/*
-- 
1.7.1