[linux-cifs-client] Re: [PATCH 2/3] [CIFS] Remote DFS root support.

[Jeff Layton]

On Tue, 17 Mar 2009 19:41:15 +0300
Igor Mammedov <niallain at gmail.com> wrote:

Sorry for the delay in reviewing this.

> Subject: [PATCH 2/3] [CIFS] Remote DFS root support.
> 
>  Allows to mount share on a server that returns -EREMOTE
>  at the tree connect stage or at the check on a full path
>  accessibility.
> 
> Signed-off-by: Igor Mammedov <niallain at gmail.com>
> ---
>  fs/cifs/connect.c |  159 ++++++++++++++++++++++++++++++++++++++++++-----------
>  1 files changed, 127 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index cd4ccc8..abe54d2 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -2214,9 +2214,63 @@ is_path_accessible(int xid, struct cifsTconInfo *tcon,
>  	return rc;
>  }
>  
> +static void
> +cleanup_volume_info(struct smb_vol **pvolume_info)
> +{
> +	struct smb_vol *volume_info;
> +
> +	if (!pvolume_info)
> +		return;
> +	volume_info = *pvolume_info;
> +
> +	if (!volume_info)
> +		return;
> +
> +	if (volume_info->password != NULL) {
> +		memset(volume_info->password, 0,
> +				strlen(volume_info->password));
> +		kfree(volume_info->password);
> +	}

^^^ this should probably turned into a kzfree() instead.

> +	kfree(volume_info->UNC);
> +	kfree(volume_info->prepath);
> +	kfree(volume_info);
> +	*pvolume_info = NULL;
> +	return;
> +}
> +
> +/* build_path_to_root returns full path to root when
> + * we do not have an exiting connection (tcon) */
> +static char *
> +build_path_to_root(const struct smb_vol *volume_info,
> +		const struct cifs_sb_info *cifs_sb)
> +{
> +	char *full_path;
> +
> +	int unc_len = strnlen(volume_info->UNC, MAX_TREE_SIZE + 1);
> +	full_path = kmalloc(unc_len + cifs_sb->prepathlen + 1, GFP_KERNEL);
> +	if (full_path == NULL)
> +		return ERR_PTR(-ENOMEM);
> +
> +	strncpy(full_path, volume_info->UNC, unc_len);
> +	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS) {
> +		int i;
> +		for (i = 0; i < unc_len; i++) {
> +			if (full_path[i] == '\\')
> +				full_path[i] = '/';
> +		}
> +	}
> +
> +	if (cifs_sb->prepathlen)
> +		strncpy(full_path + unc_len, cifs_sb->prepath,
> +				cifs_sb->prepathlen);
> +
> +	full_path[unc_len + cifs_sb->prepathlen] = 0; /* add trailing null */
> +	return full_path;
> +}
> +
>  int
>  cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
> -	   char *mount_data, const char *devname)
> +		char *mount_data_global, const char *devname)
>  {
>  	int rc = 0;
>  	int xid;
> @@ -2225,6 +2279,13 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
>  	struct cifsTconInfo *tcon = NULL;
>  	struct TCP_Server_Info *srvTcp = NULL;
>  	char   *full_path;
> +	struct dfs_info3_param *referrals = NULL;
> +	unsigned int num_referrals = 0;
> +
> +	char *mount_data = mount_data_global;
> +
> +try_mount_again:
> +	full_path = NULL;
>  
>  	xid = GetXid();
>  
> @@ -2371,11 +2432,9 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
>  				}
>  			}
>  
> -			/* check for null share name ie connect to dfs root */
>  			if ((strchr(volume_info->UNC + 3, '\\') == NULL)
>  			    && (strchr(volume_info->UNC + 3, '/') == NULL)) {
> -				/* rc = connect_to_dfs_path(...) */
> -				cFYI(1, ("DFS root not supported"));
> +				cERROR(1, ("Missing share name"));
>  				rc = -ENODEV;
>  				goto mount_fail_check;
>  			} else {
> @@ -2392,7 +2451,7 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
>  				}
>  			}
>  			if (rc)
> -				goto mount_fail_check;
> +				goto remote_path_check;
>  			tcon->seal = volume_info->seal;
>  			write_lock(&cifs_tcp_ses_lock);
>  			list_add(&tcon->tcon_list, &pSesInfo->tcon_list);
> @@ -2417,19 +2476,9 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
>  	/* BB FIXME fix time_gran to be larger for LANMAN sessions */
>  	sb->s_time_gran = 100;
>  
> -mount_fail_check:
> -	/* on error free sesinfo and tcon struct if needed */
> -	if (rc) {
> -		/* If find_unc succeeded then rc == 0 so we can not end */
> -		/* up accidently freeing someone elses tcon struct */
> -		if (tcon)
> -			cifs_put_tcon(tcon);
> -		else if (pSesInfo)
> -			cifs_put_smb_ses(pSesInfo);
> -		else
> -			cifs_put_tcp_session(srvTcp);
> -		goto out;
> -	}
> +	if (rc)
> +		goto remote_path_check;
> +
>  	cifs_sb->tcon = tcon;
>  
>  	/* do not care if following two calls succeed - informational */
> @@ -2461,7 +2510,9 @@ mount_fail_check:
>  		cifs_sb->rsize = min(cifs_sb->rsize,
>  			       (tcon->ses->server->maxBuf - MAX_CIFS_HDR_SIZE));
>  
> -	if (!rc && cifs_sb->prepathlen) {
> +remote_path_check:
> +	/* check if a whole path (including prepath) is not remote */
> +	if (!rc && cifs_sb->prepathlen && tcon) {
>  		/* build_path_to_root works only when we have a valid tcon */
>  		full_path = cifs_build_path_to_root(cifs_sb);
>  		if (full_path == NULL) {
> @@ -2469,31 +2520,75 @@ mount_fail_check:
>  			goto mount_fail_check;
>  		}
>  		rc = is_path_accessible(xid, tcon, cifs_sb, full_path);
> -		if (rc) {
> -			cERROR(1, ("Path %s in not accessible: %d",
> -						full_path, rc));
> +		if (rc != -EREMOTE) {
>  			kfree(full_path);
>  			goto mount_fail_check;
>  		}
>  		kfree(full_path);
>  	}
>  
> +	/* get referral if needed */
> +	if (rc == -EREMOTE) {
> +		/* convert forward to back slashes in prepath here if needed */
> +		if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS) == 0)
> +			convert_delimiter(cifs_sb->prepath,
> +					CIFS_DIR_SEP(cifs_sb));
> +		full_path = build_path_to_root(volume_info, cifs_sb);
> +		if (IS_ERR(full_path)) {
> +			rc = PTR_ERR(full_path);
> +			goto mount_fail_check;
> +		}
> +
> +		cFYI(1, ("Getting referral for: %s", full_path));
> +		rc = get_dfs_path(xid, pSesInfo , full_path + 1,
> +			cifs_sb->local_nls, &num_referrals, &referrals,
> +			cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR);
> +		if (!rc && num_referrals > 0) {
> +			char *fake_devname = NULL;
> +
> +			if (mount_data != mount_data_global)
> +				kfree(mount_data);
> +			mount_data = cifs_compose_mount_options(
> +					cifs_sb->mountdata, full_path + 1,
> +					referrals, &fake_devname);
> +			kfree(fake_devname);
> +			free_dfs_info_array(referrals, num_referrals);
> +
> +			if (tcon)
> +				cifs_put_tcon(tcon);
> +			else if (pSesInfo)
> +				cifs_put_smb_ses(pSesInfo);
> +

^^^

This is not a high-performance codepath, but I wonder whether it would
be more efficient to not put the references to the tcon or session
here. Consider the case where the DFS root and the target server are
the same. You'll end up having to reconnect the socket and start the
mount all over again even though that really isn't necessary. It seems
like it would be best to somehow track the extra references we've
incurred while mounting and then put them all at the end of the function.


> +			cleanup_volume_info(&volume_info);
> +			FreeXid(xid);
> +			kfree(full_path);
> +			goto try_mount_again;
> +		}
> +	}
> +
> +mount_fail_check:
> +	/* on error free sesinfo and tcon struct if needed */
> +	if (rc) {
> +		if (mount_data != mount_data_global)
> +			kfree(mount_data);
> +		/* If find_unc succeeded then rc == 0 so we can not end */
> +		/* up accidently freeing someone elses tcon struct */
> +		if (tcon)
> +			cifs_put_tcon(tcon);
> +		else if (pSesInfo)
> +			cifs_put_smb_ses(pSesInfo);
> +		else
> +			cifs_put_tcp_session(srvTcp);
> +		goto out;
> +	}
> +
>  	/* volume_info->password is freed above when existing session found
>  	(in which case it is not needed anymore) but when new sesion is created
>  	the password ptr is put in the new session structure (in which case the
>  	password will be freed at unmount time) */
>  out:
>  	/* zero out password before freeing */
> -	if (volume_info) {
> -		if (volume_info->password != NULL) {
> -			memset(volume_info->password, 0,
> -				strlen(volume_info->password));
> -			kfree(volume_info->password);
> -		}
> -		kfree(volume_info->UNC);
> -		kfree(volume_info->prepath);
> -		kfree(volume_info);
> -	}
> +	cleanup_volume_info(&volume_info);
>  	FreeXid(xid);
>  	return rc;
>  }


-- 
Jeff Layton <jlayton at redhat.com>