cifs_get_root shouldn't use path with tree name
diff mbox

Message ID 1473260003-5271-1-git-send-email-sprabhu@redhat.com
State New
Headers show

Commit Message

Sachin Prabhu Sept. 7, 2016, 2:53 p.m. UTC
When a server returns the optional flag SMB_SHARE_IS_IN_DFS in response
to a tree connect, cifs_build_path_to_root() will return a pathname
which includes the tree name. This causes problems with cifs_get_root()
which separates each component and does a lookup for each component of
the path.

We encountered a problem with dfs shares hosted by a Netapp. when
connecting to nodes pointed to by the DFS share. The tree connect for
these nodes return SMB_SHARE_IS_IN_DFS even after the DFS node is
resolved and the node returned doesn't contain a DFS share. Other
servers(tested with MS Win2012 R2) do not and hence do not face this
problem. A workaround for Netapp servers is to mount with the
nodfs flag.
The return flag results in cifs_build_path_to_root() using the tree name
in building the path which is then split into its components and
individual lookup done for each component in cifs_get_root(). The first
component of the path in this case is the hostname which results in
object not found errors.

We have tested this patch both on our internal test servers as well as
the user's own servers and can confirm that it fixes the problem.

Signed-off-by: Sachin Prabhu <sprabhu@redhat.com>
Reported-by: Pierguido Lambri <plambri@redhat.com>
---
 fs/cifs/cifsfs.c    | 2 +-
 fs/cifs/cifsproto.h | 3 ++-
 fs/cifs/connect.c   | 3 ++-
 fs/cifs/dir.c       | 4 ++--
 4 files changed, 7 insertions(+), 5 deletions(-)

Comments

Aurelien Aptel Sept. 8, 2016, 10:49 a.m. UTC | #1
Hi Sachin,

As you said, patch is similar to what I proposed few month ago. There's
only one thing that bothers me. cifs_build_path_to_root() has this bit
before the DFS test:

	/* if no prefix path, simply set path to the root of share to "" */
	if (pplen == 0) {
		full_path = kzalloc(1, GFP_KERNEL);
		return full_path;
	}


I think it should be dropped. It was already inconsistent because if the
share was in a DFS without a prepath it would have returned an empty
string instead of a path with the tree name prefix.

I can send a patch after yours to correct that you think it should be in
a separate commit.

Cheers,
Sachin Prabhu Sept. 12, 2016, 4:40 p.m. UTC | #2
On Thu, 2016-09-08 at 12:49 +0200, Aurélien Aptel wrote:
> Hi Sachin,
> 
> As you said, patch is similar to what I proposed few month ago.
> There's
> only one thing that bothers me. cifs_build_path_to_root() has this
> bit
> before the DFS test:
> 
> 	/* if no prefix path, simply set path to the root of share to
> "" */
> 	if (pplen == 0) {
> 		full_path = kzalloc(1, GFP_KERNEL);
> 		return full_path;
> 	}
> 
> 
> I think it should be dropped. It was already inconsistent because if
> the
> share was in a DFS without a prepath it would have returned an empty
> string instead of a path with the tree name prefix.
> 
> I can send a patch after yours to correct that you think it should be
> in
> a separate commit.
> 
> Cheers,
> 

I suspect that is because the root of the share cannot be a DFS mount.
Maybe Steve can confirm it. 

Sachin Prabhu
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Aurelien Aptel Oct. 6, 2016, 12:15 p.m. UTC | #3
What is the status on this?

If we explicitely ask for a tree name prefix via the new parameter, we
should always get it. It should not return an empty string if there is
no prepath.

Sachin Prabhu <sprabhu@redhat.com> writes:
> I suspect that is because the root of the share cannot be a DFS mount.
> Maybe Steve can confirm it.

So that means we can safely remove that early return, IIUC.
Sachin Prabhu Oct. 6, 2016, 12:20 p.m. UTC | #4
On Thu, 2016-10-06 at 14:15 +0200, Aurélien Aptel wrote:
> What is the status on this?
> 
> If we explicitely ask for a tree name prefix via the new parameter,
> we
> should always get it. It should not return an empty string if there
> is
> no prepath.
> 
> Sachin Prabhu <sprabhu@redhat.com> writes:
> > 
> > I suspect that is because the root of the share cannot be a DFS
> > mount.
> > Maybe Steve can confirm it.
> 
> So that means we can safely remove that early return, IIUC.
> 

Aurelien,

I think Steve had requested for a reproducer for this issue. The only
place I've encountered this was with Netapp servers. Do you have a
reproder which works with Samba or Windows servers?

Sachin Prabhu
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Aurelien Aptel Oct. 11, 2016, 6:18 a.m. UTC | #5
Sachin Prabhu <sprabhu@redhat.com> writes:
> I think Steve had requested for a reproducer for this issue. The only
> place I've encountered this was with Netapp servers. Do you have a
> reproder which works with Samba or Windows servers?

The reproducer was for Samba but the root cause was something different
(later fixed by your last DFS fix). The thing I used to reproduce it
doesn't reproduce it anymore.

I have found that the tree name prefix is needed, at least when
mounting. I guess Steve already knew this but it wasn't obvious for me
so I'm sharing it.

When mounting //server/dfsroot/target with 'target' linking to
//server/dfstarget

With tree name prefixed:

=> Trans2 Request, QUERY_PATH_INFO, Query File All Info, Path: //10.160.64.222/dfsroot/target
<= Trans2 Response, QUERY_PATH_INFO, Error: STATUS_PATH_NOT_COVERED

Without tree name prefixed:

=> Trans2 Request, QUERY_PATH_INFO, Query File All Info, Path: /target
<= Trans2 Response, QUERY_PATH_INFO, Error: STATUS_OBJECT_NAME_NOT_FOUND

PATH_NOT_COVERED is mapped to EREMOTE whereas OBJECT_NAME_NOT_FOUND is
mapped to ENOENT.

So *not* prefixing with the tree name actually breaks mounting a path
containing a DFS link in its components. Which is the *opposite* of what
I initially thought. I did not analyze the problem and traces enough,
I'm sorry.

So if we merge this patch, we have to be careful about when to not
prefix the tree name.
Sachin Prabhu Nov. 4, 2016, 5:45 p.m. UTC | #6
On Tue, 2016-10-11 at 08:18 +0200, Aurélien Aptel wrote:
> Sachin Prabhu <sprabhu@redhat.com> writes:
> > 
> > I think Steve had requested for a reproducer for this issue. The
> > only
> > place I've encountered this was with Netapp servers. Do you have a
> > reproder which works with Samba or Windows servers?
> 
> The reproducer was for Samba but the root cause was something
> different
> (later fixed by your last DFS fix). The thing I used to reproduce it
> doesn't reproduce it anymore.
> 
> I have found that the tree name prefix is needed, at least when
> mounting. I guess Steve already knew this but it wasn't obvious for
> me
> so I'm sharing it.
> 
> When mounting //server/dfsroot/target with 'target' linking to
> //server/dfstarget
> 
> With tree name prefixed:
> 
> => Trans2 Request, QUERY_PATH_INFO, Query File All Info, Path:
> //10.160.64.222/dfsroot/target
> <= Trans2 Response, QUERY_PATH_INFO, Error: STATUS_PATH_NOT_COVERED
> 
> Without tree name prefixed:
> 
> => Trans2 Request, QUERY_PATH_INFO, Query File All Info, Path:
> /target
> <= Trans2 Response, QUERY_PATH_INFO, Error:
> STATUS_OBJECT_NAME_NOT_FOUND
> 
> PATH_NOT_COVERED is mapped to EREMOTE whereas OBJECT_NAME_NOT_FOUND
> is
> mapped to ENOENT.
> 
> So *not* prefixing with the tree name actually breaks mounting a path
> containing a DFS link in its components. Which is the *opposite* of
> what
> I initially thought. I did not analyze the problem and traces enough,
> I'm sorry.
> 
> So if we merge this patch, we have to be careful about when to not
> prefix the tree name.
> 

Hello Aurelien,

The patch maintains current behaviour except for the call to
cifs_build_path_to_root in cifs_get_root() where the logic expects the
full path to the file/directory without the hostname as it iterates
over each component of the path. This fixes a behaviour seen with
Netapp where the tcon connect returns a SMB_SHARE_IS_IN_DFS even though
there doesn't isn't a DFS share in the path.

Sachin Prabhu
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Layton Dec. 13, 2016, 12:33 p.m. UTC | #7
On Wed, 2016-09-07 at 15:53 +0100, Sachin Prabhu wrote:
> When a server returns the optional flag SMB_SHARE_IS_IN_DFS in response
> to a tree connect, cifs_build_path_to_root() will return a pathname
> which includes the tree name. This causes problems with cifs_get_root()
> which separates each component and does a lookup for each component of
> the path.
> 
> We encountered a problem with dfs shares hosted by a Netapp. when
> connecting to nodes pointed to by the DFS share. The tree connect for
> these nodes return SMB_SHARE_IS_IN_DFS even after the DFS node is
> resolved and the node returned doesn't contain a DFS share. Other
> servers(tested with MS Win2012 R2) do not and hence do not face this
> problem. A workaround for Netapp servers is to mount with the
> nodfs flag.
> The return flag results in cifs_build_path_to_root() using the tree name
> in building the path which is then split into its components and
> individual lookup done for each component in cifs_get_root(). The first
> component of the path in this case is the hostname which results in
> object not found errors.
> 
> We have tested this patch both on our internal test servers as well as
> the user's own servers and can confirm that it fixes the problem.
> 
> Signed-off-by: Sachin Prabhu <sprabhu@redhat.com>
> Reported-by: Pierguido Lambri <plambri@redhat.com>
> ---
>  fs/cifs/cifsfs.c    | 2 +-
>  fs/cifs/cifsproto.h | 3 ++-
>  fs/cifs/connect.c   | 3 ++-
>  fs/cifs/dir.c       | 4 ++--
>  4 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 6bbec5e..b6f57dd 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -610,7 +610,7 @@ cifs_get_root(struct smb_vol *vol, struct super_block *sb)
>  	char sep;
>  
>  	full_path = cifs_build_path_to_root(vol, cifs_sb,
> -					    cifs_sb_master_tcon(cifs_sb));
> +				cifs_sb_master_tcon(cifs_sb), 0);

Ok, sorry if I'm being dense here, but I haven't been following the
various patches in this area closely...

cifs_get_root basically does a lookup of the dentry that we want to be
the root of the mount on the client.

You're saying here that we need to _not_ send a DFS style path query
when doing that lookup, even though the server is setting
the SMB_SHARE_IS_IN_DFS flag in the tcon response?

Is this just a workaround for misbehaving servers, or is the client
actually doing this wrong?

>  	if (full_path == NULL)
>  		return ERR_PTR(-ENOMEM);
>  
> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> index 1243bd3..52c307b 100644
> --- a/fs/cifs/cifsproto.h
> +++ b/fs/cifs/cifsproto.h
> @@ -63,7 +63,8 @@ extern void exit_cifs_spnego(void);
>  extern char *build_path_from_dentry(struct dentry *);
>  extern char *cifs_build_path_to_root(struct smb_vol *vol,
>  				     struct cifs_sb_info *cifs_sb,
> -				     struct cifs_tcon *tcon);
> +				     struct cifs_tcon *tcon,
> +				     int add_treename);
>  extern char *build_wildcard_path_from_dentry(struct dentry *direntry);
>  extern char *cifs_compose_mount_options(const char *sb_mountdata,
>  		const char *fullpath, const struct dfs_info3_param *ref,
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 9878943..afca9ee 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -3649,7 +3649,8 @@ remote_path_check:
>  		/*
>  		 * cifs_build_path_to_root works only when we have a valid tcon
>  		 */
> -		full_path = cifs_build_path_to_root(volume_info, cifs_sb, tcon);
> +		full_path = cifs_build_path_to_root(volume_info, cifs_sb, tcon,
> +					tcon->Flags & SMB_SHARE_IS_IN_DFS);
>  		if (full_path == NULL) {
>  			rc = -ENOMEM;
>  			goto mount_fail_check;
> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> index 4716c54..4c0c62c 100644
> --- a/fs/cifs/dir.c
> +++ b/fs/cifs/dir.c
> @@ -47,7 +47,7 @@ renew_parental_timestamps(struct dentry *direntry)
>  
>  char *
>  cifs_build_path_to_root(struct smb_vol *vol, struct cifs_sb_info *cifs_sb,
> -			struct cifs_tcon *tcon)
> +			struct cifs_tcon *tcon, int add_treename)
>  {
>  	int pplen = vol->prepath ? strlen(vol->prepath) + 1 : 0;
>  	int dfsplen;
> @@ -59,7 +59,7 @@ cifs_build_path_to_root(struct smb_vol *vol, struct cifs_sb_info *cifs_sb,
>  		return full_path;
>  	}
>  
> -	if (tcon->Flags & SMB_SHARE_IS_IN_DFS)
> +	if (add_treename)
>  		dfsplen = strnlen(tcon->treeName, MAX_TREE_SIZE + 1);
>  	else
>  		dfsplen = 0;
Jeff Layton Dec. 13, 2016, 1:06 p.m. UTC | #8
On Tue, 2016-12-13 at 07:33 -0500, Jeff Layton wrote:
> On Wed, 2016-09-07 at 15:53 +0100, Sachin Prabhu wrote:
> > 
> > When a server returns the optional flag SMB_SHARE_IS_IN_DFS in response
> > to a tree connect, cifs_build_path_to_root() will return a pathname
> > which includes the tree name. This causes problems with cifs_get_root()
> > which separates each component and does a lookup for each component of
> > the path.
> > 
> > We encountered a problem with dfs shares hosted by a Netapp. when
> > connecting to nodes pointed to by the DFS share. The tree connect for
> > these nodes return SMB_SHARE_IS_IN_DFS even after the DFS node is
> > resolved and the node returned doesn't contain a DFS share. Other
> > servers(tested with MS Win2012 R2) do not and hence do not face this
> > problem. A workaround for Netapp servers is to mount with the
> > nodfs flag.
> > The return flag results in cifs_build_path_to_root() using the tree name
> > in building the path which is then split into its components and
> > individual lookup done for each component in cifs_get_root(). The first
> > component of the path in this case is the hostname which results in
> > object not found errors.
> > 
> > We have tested this patch both on our internal test servers as well as
> > the user's own servers and can confirm that it fixes the problem.
> > 
> > Signed-off-by: Sachin Prabhu <sprabhu@redhat.com>
> > Reported-by: Pierguido Lambri <plambri@redhat.com>
> > ---
> >  fs/cifs/cifsfs.c    | 2 +-
> >  fs/cifs/cifsproto.h | 3 ++-
> >  fs/cifs/connect.c   | 3 ++-
> >  fs/cifs/dir.c       | 4 ++--
> >  4 files changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > index 6bbec5e..b6f57dd 100644
> > --- a/fs/cifs/cifsfs.c
> > +++ b/fs/cifs/cifsfs.c
> > @@ -610,7 +610,7 @@ cifs_get_root(struct smb_vol *vol, struct super_block *sb)
> >  	char sep;
> >  
> >  	full_path = cifs_build_path_to_root(vol, cifs_sb,
> > -					    cifs_sb_master_tcon(cifs_sb));
> > +				cifs_sb_master_tcon(cifs_sb), 0);
> 
> Ok, sorry if I'm being dense here, but I haven't been following the
> various patches in this area closely...
> 
> cifs_get_root basically does a lookup of the dentry that we want to be
> the root of the mount on the client.
> 
> You're saying here that we need to _not_ send a DFS style path query
> when doing that lookup, even though the server is setting
> the SMB_SHARE_IS_IN_DFS flag in the tcon response?
> 
> Is this just a workaround for misbehaving servers, or is the client
> actually doing this wrong?
> 

Doh! I just needed another cup of coffee...

I get it now. We build the path to root, and then try to issue a lookup
on each component (a'la lookup_one_len_unlocked). That fails in this
case because we're trying to lookup the DFS hostname component like we
would a normal path component.

I think is this a reasonable fix for now. That said, this whole Rube
Goldberg contraption could really use a serious re-think, IMO...

You can add:

Reviewed-by: Jeff Layton <jlayton@redhat.com>

> > 
> >  	if (full_path == NULL)
> >  		return ERR_PTR(-ENOMEM);
> >  
> > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> > index 1243bd3..52c307b 100644
> > --- a/fs/cifs/cifsproto.h
> > +++ b/fs/cifs/cifsproto.h
> > @@ -63,7 +63,8 @@ extern void exit_cifs_spnego(void);
> >  extern char *build_path_from_dentry(struct dentry *);
> >  extern char *cifs_build_path_to_root(struct smb_vol *vol,
> >  				     struct cifs_sb_info *cifs_sb,
> > -				     struct cifs_tcon *tcon);
> > +				     struct cifs_tcon *tcon,
> > +				     int add_treename);
> >  extern char *build_wildcard_path_from_dentry(struct dentry *direntry);
> >  extern char *cifs_compose_mount_options(const char *sb_mountdata,
> >  		const char *fullpath, const struct dfs_info3_param *ref,
> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> > index 9878943..afca9ee 100644
> > --- a/fs/cifs/connect.c
> > +++ b/fs/cifs/connect.c
> > @@ -3649,7 +3649,8 @@ remote_path_check:
> >  		/*
> >  		 * cifs_build_path_to_root works only when we have a valid tcon
> >  		 */
> > -		full_path = cifs_build_path_to_root(volume_info, cifs_sb, tcon);
> > +		full_path = cifs_build_path_to_root(volume_info, cifs_sb, tcon,
> > +					tcon->Flags & SMB_SHARE_IS_IN_DFS);
> >  		if (full_path == NULL) {
> >  			rc = -ENOMEM;
> >  			goto mount_fail_check;
> > diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> > index 4716c54..4c0c62c 100644
> > --- a/fs/cifs/dir.c
> > +++ b/fs/cifs/dir.c
> > @@ -47,7 +47,7 @@ renew_parental_timestamps(struct dentry *direntry)
> >  
> >  char *
> >  cifs_build_path_to_root(struct smb_vol *vol, struct cifs_sb_info *cifs_sb,
> > -			struct cifs_tcon *tcon)
> > +			struct cifs_tcon *tcon, int add_treename)
> >  {
> >  	int pplen = vol->prepath ? strlen(vol->prepath) + 1 : 0;
> >  	int dfsplen;
> > @@ -59,7 +59,7 @@ cifs_build_path_to_root(struct smb_vol *vol, struct cifs_sb_info *cifs_sb,
> >  		return full_path;
> >  	}
> >  
> > -	if (tcon->Flags & SMB_SHARE_IS_IN_DFS)
> > +	if (add_treename)
> >  		dfsplen = strnlen(tcon->treeName, MAX_TREE_SIZE + 1);
> >  	else
> >  		dfsplen = 0;
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Aurélien Aptel Dec. 14, 2016, 9:55 a.m. UTC | #9
Jeff Layton <jlayton@redhat.com> writes:
> On Tue, 2016-12-13 at 07:33 -0500, Jeff Layton wrote:
> I think is this a reasonable fix for now. That said, this whole Rube
> Goldberg contraption could really use a serious re-think, IMO...

Indeed. I find cifs_mount() in particular really hard to follow.

Patch
diff mbox

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 6bbec5e..b6f57dd 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -610,7 +610,7 @@  cifs_get_root(struct smb_vol *vol, struct super_block *sb)
 	char sep;
 
 	full_path = cifs_build_path_to_root(vol, cifs_sb,
-					    cifs_sb_master_tcon(cifs_sb));
+				cifs_sb_master_tcon(cifs_sb), 0);
 	if (full_path == NULL)
 		return ERR_PTR(-ENOMEM);
 
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index 1243bd3..52c307b 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -63,7 +63,8 @@  extern void exit_cifs_spnego(void);
 extern char *build_path_from_dentry(struct dentry *);
 extern char *cifs_build_path_to_root(struct smb_vol *vol,
 				     struct cifs_sb_info *cifs_sb,
-				     struct cifs_tcon *tcon);
+				     struct cifs_tcon *tcon,
+				     int add_treename);
 extern char *build_wildcard_path_from_dentry(struct dentry *direntry);
 extern char *cifs_compose_mount_options(const char *sb_mountdata,
 		const char *fullpath, const struct dfs_info3_param *ref,
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 9878943..afca9ee 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -3649,7 +3649,8 @@  remote_path_check:
 		/*
 		 * cifs_build_path_to_root works only when we have a valid tcon
 		 */
-		full_path = cifs_build_path_to_root(volume_info, cifs_sb, tcon);
+		full_path = cifs_build_path_to_root(volume_info, cifs_sb, tcon,
+					tcon->Flags & SMB_SHARE_IS_IN_DFS);
 		if (full_path == NULL) {
 			rc = -ENOMEM;
 			goto mount_fail_check;
diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index 4716c54..4c0c62c 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -47,7 +47,7 @@  renew_parental_timestamps(struct dentry *direntry)
 
 char *
 cifs_build_path_to_root(struct smb_vol *vol, struct cifs_sb_info *cifs_sb,
-			struct cifs_tcon *tcon)
+			struct cifs_tcon *tcon, int add_treename)
 {
 	int pplen = vol->prepath ? strlen(vol->prepath) + 1 : 0;
 	int dfsplen;
@@ -59,7 +59,7 @@  cifs_build_path_to_root(struct smb_vol *vol, struct cifs_sb_info *cifs_sb,
 		return full_path;
 	}
 
-	if (tcon->Flags & SMB_SHARE_IS_IN_DFS)
+	if (add_treename)
 		dfsplen = strnlen(tcon->treeName, MAX_TREE_SIZE + 1);
 	else
 		dfsplen = 0;