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

[Igor Mammedov]

Jeff Layton wrote:
> 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.

will fix.

> 
>> +	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.

I've thought about this case. Considering that it is not a high-performance
codepath, I'd rather leave it as is for the sake of simplicity than adding
extra reference tracking and jumping into a pretty large cifs_mount.

PS:
Possible reuse of session will only hit once (until the first session
is created.), following attempts to mount will reuse existing session if
I read code correctly. The same possible for tcon, However crazy enough 
admin can create a 100+ of shares on root server and cross reference them
to each other, just to trigger this case.

PS2:
Referencing could be added later if people really will need it.
I'm sorry but I can't promise to do it in the near future,
I'm quite short on free time lately.

> 
> 
>> +			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;
>>  }
> 
> 

-- 

Best regards,

-------------------------
Igor Mammedov,
niallain "at" gmail.com