Re: [PATCH] cifs_get_root shouldn't use path with tree name

[Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Reported-by: Pierguido Lambri <plambri-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  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 <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>