[PATCH v4 1/6] cifs: Extract DFS referral expansion logic to separate function

[PATCH v4 1/6] cifs: Extract DFS referral expansion logic to separate function

[Sean Finney]

The logic behind the expansion of DFS referrals is now extracted from
cifs_mount into a new static function, expand_dfs_referral.  This will
reduce duplicate code in upcoming commits.

Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Sean Finney <seanius-ADwgVSpYHhHR7s880joybQ@public.gmane.org>
---
 fs/cifs/connect.c |  105 +++++++++++++++++++++++++++++++++++------------------
 1 files changed, 69 insertions(+), 36 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 94e60c5..c89d871 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -2742,6 +2742,57 @@ build_unc_path_to_root(const struct smb_vol *volume_info,
 	full_path[unc_len + cifs_sb->prepathlen] = 0; /* add trailing null */
 	return full_path;
 }
+
+/*
+ * Perform a dfs referral query for a share and (optionally) prefix
+ *
+ * If a referral is found, mount_data will be set to point at a newly
+ * allocated string containing updated options for the submount.
+ * Otherwise it will be left untouched.
+ *
+ * Returns the rc from get_dfs_path to the caller, which can be used to
+ * determine whether there were referrals.
+ */
+static int
+expand_dfs_referral(int xid, struct cifs_ses *pSesInfo,
+		    struct smb_vol *volume_info, struct cifs_sb_info *cifs_sb,
+		    char **mount_data, int check_prefix)
+{
+	int rc;
+	unsigned int num_referrals = 0;
+	struct dfs_info3_param *referrals = NULL;
+	char *full_path = NULL, *ref_path = NULL, *mdata = NULL;
+
+	full_path = build_unc_path_to_root(volume_info, cifs_sb);
+	if (IS_ERR(full_path))
+		return PTR_ERR(full_path);
+
+	/* For DFS paths, skip the first '\' of the UNC */
+	ref_path = check_prefix ? full_path + 1 : volume_info->UNC + 1;
+
+	rc = get_dfs_path(xid, pSesInfo , ref_path, 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;
+
+		mdata = cifs_compose_mount_options(cifs_sb->mountdata,
+						   full_path + 1, referrals,
+						   &fake_devname);
+
+		free_dfs_info_array(referrals, num_referrals);
+		kfree(fake_devname);
+
+		if (IS_ERR(mdata)) {
+			rc = PTR_ERR(mdata);
+			mdata = NULL;
+		}
+		*mount_data = mdata;
+	}
+	kfree(full_path);
+	return rc;
+}
 #endif
 
 int
@@ -2758,10 +2809,19 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
 	char *mount_data = mount_data_global;
 	struct tcon_link *tlink;
 #ifdef CONFIG_CIFS_DFS_UPCALL
-	struct dfs_info3_param *referrals = NULL;
-	unsigned int num_referrals = 0;
 	int referral_walks_count = 0;
 try_mount_again:
+
+	/* cleanup activities if we're chasing a referral */
+	if (referral_walks_count) {
+		if (tcon)
+			cifs_put_tcon(tcon);
+		else if (pSesInfo)
+			cifs_put_smb_ses(pSesInfo);
+
+		cleanup_volume_info(&volume_info);
+		FreeXid(xid);
+	}
 #endif
 	rc = 0;
 	tcon = NULL;
@@ -2907,46 +2967,19 @@ remote_path_check:
 		if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS) == 0)
 			convert_delimiter(cifs_sb->prepath,
 					CIFS_DIR_SEP(cifs_sb));
-		full_path = build_unc_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);
-
-			free_dfs_info_array(referrals, num_referrals);
-			kfree(fake_devname);
-			kfree(full_path);
-
-			if (IS_ERR(mount_data)) {
-				rc = PTR_ERR(mount_data);
-				mount_data = NULL;
-				goto mount_fail_check;
-			}
+		if (mount_data != mount_data_global)
+			kfree(mount_data);
 
-			if (tcon)
-				cifs_put_tcon(tcon);
-			else if (pSesInfo)
-				cifs_put_smb_ses(pSesInfo);
+		rc = expand_dfs_referral(xid, pSesInfo, volume_info, cifs_sb,
+					 &mount_data, true);
 
-			cleanup_volume_info(&volume_info);
+		if (!rc) {
 			referral_walks_count++;
-			FreeXid(xid);
 			goto try_mount_again;
 		}
+		mount_data = NULL;
+		goto mount_fail_check;
 #else /* No DFS support, return error on mount */
 		rc = -EOPNOTSUPP;
 #endif
-- 
1.7.4.1

[PATCH v4 2/6] cifs: Add support for mounting Windows 2008 DFS shares

[Sean Finney]

Windows 2008 CIFS servers do not always return PATH_NOT_COVERED when
attempting to access a DFS share.  Therefore, when checking for remote
shares, unconditionally ask for a DFS referral for the UNC (w/out prepath)
before continuing with previous behavior of attempting to access the UNC +
prepath and checking for PATH_NOT_COVERED.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=31092

Signed-off-by: Sean Finney <seanius-ADwgVSpYHhHR7s880joybQ@public.gmane.org>
---
 fs/cifs/connect.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index c89d871..4873bac 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -2934,6 +2934,24 @@ try_mount_again:
 			       (tcon->ses->server->maxBuf - MAX_CIFS_HDR_SIZE));
 
 remote_path_check:
+#ifdef CONFIG_CIFS_DFS_UPCALL
+	/*
+	 * Perform an unconditional check for whether there are DFS
+	 * referrals for this path without prefix, to provide support
+	 * for DFS referrals from w2k8 servers which don't seem to respond
+	 * with PATH_NOT_COVERED to requests that include the prefix.
+	 * Chase the referral if found, otherwise continue normally.
+	 */
+	if (referral_walks_count == 0) {
+		int refrc = expand_dfs_referral(xid, pSesInfo, volume_info,
+						cifs_sb, &mount_data, false);
+		if (!refrc) {
+			referral_walks_count++;
+			goto try_mount_again;
+		}
+	}
+#endif
+
 	/* check if a whole path (including prepath) is not remote */
 	if (!rc && tcon) {
 		/* build_path_to_root works only when we have a valid tcon */
-- 
1.7.4.1

Re: [PATCH v4 2/6] cifs: Add support for mounting Windows 2008 DFS shares

[Jeff Layton]

On Wed, 6 Apr 2011 16:26:41 +0200
Sean Finney <seanius-ADwgVSpYHhHR7s880joybQ@public.gmane.org> wrote:

> Windows 2008 CIFS servers do not always return PATH_NOT_COVERED when
> attempting to access a DFS share.  Therefore, when checking for remote
> shares, unconditionally ask for a DFS referral for the UNC (w/out prepath)
> before continuing with previous behavior of attempting to access the UNC +
> prepath and checking for PATH_NOT_COVERED.
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=31092
> 
> Signed-off-by: Sean Finney <seanius-ADwgVSpYHhHR7s880joybQ@public.gmane.org>
> ---
>  fs/cifs/connect.c |   18 ++++++++++++++++++
>  1 files changed, 18 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index c89d871..4873bac 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -2934,6 +2934,24 @@ try_mount_again:
>  			       (tcon->ses->server->maxBuf - MAX_CIFS_HDR_SIZE));
>  
>  remote_path_check:
> +#ifdef CONFIG_CIFS_DFS_UPCALL
> +	/*
> +	 * Perform an unconditional check for whether there are DFS
> +	 * referrals for this path without prefix, to provide support
> +	 * for DFS referrals from w2k8 servers which don't seem to respond
> +	 * with PATH_NOT_COVERED to requests that include the prefix.
> +	 * Chase the referral if found, otherwise continue normally.
> +	 */
> +	if (referral_walks_count == 0) {
> +		int refrc = expand_dfs_referral(xid, pSesInfo, volume_info,
> +						cifs_sb, &mount_data, false);
> +		if (!refrc) {
> +			referral_walks_count++;
> +			goto try_mount_again;
> +		}
> +	}
> +#endif
> +
>  	/* check if a whole path (including prepath) is not remote */
>  	if (!rc && tcon) {
>  		/* build_path_to_root works only when we have a valid tcon */

Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

[PATCH v4 3/6] cifs: cifs_parse_mount_options: do not tokenize mount options in-place

[Sean Finney]

To keep strings passed to cifs_parse_mount_options re-usable (which is
needed to clean up the DFS referral handling), tokenize a copy of the
mount options instead.  If values are needed from this tokenized string,
they too must be duplicated (previously, some options were copied and
others duplicated).

Since we are not on the critical path and any cleanup is relatively easy,
the extra memory usage shouldn't be a problem (and it is a bit simpler
than trying to implement something smarter).

Signed-off-by: Sean Finney <seanius-ADwgVSpYHhHR7s880joybQ@public.gmane.org>
---
 fs/cifs/connect.c |  109 ++++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 74 insertions(+), 35 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 4873bac..259fc64 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -807,11 +807,12 @@ extract_hostname(const char *unc)
 }
 
 static int
-cifs_parse_mount_options(char *options, const char *devname,
+cifs_parse_mount_options(const char *mountdata, const char *devname,
 			 struct smb_vol *vol)
 {
 	char *value;
 	char *data;
+	char *mountdata_copy, *options;
 	unsigned int  temp_len, i, j;
 	char separator[2];
 	short int override_uid = -1;
@@ -851,9 +852,14 @@ cifs_parse_mount_options(char *options, const char *devname,
 
 	vol->actimeo = CIFS_DEF_ACTIMEO;
 
-	if (!options)
-		return 1;
+	if (!mountdata)
+		goto cifs_parse_mount_err;
+
+	mountdata_copy = kstrndup(mountdata, PAGE_CACHE_SIZE, GFP_KERNEL);
+	if (!mountdata_copy)
+		goto cifs_parse_mount_err;
 
+	options = mountdata_copy;
 	if (strncmp(options, "sep=", 4) == 0) {
 		if (options[4] != 0) {
 			separator[0] = options[4];
@@ -878,17 +884,22 @@ cifs_parse_mount_options(char *options, const char *devname,
 			if (!value) {
 				printk(KERN_WARNING
 				       "CIFS: invalid or missing username\n");
-				return 1;	/* needs_arg; */
+				goto cifs_parse_mount_err;
 			} else if (!*value) {
 				/* null user, ie anonymous, authentication */
 				vol->nullauth = 1;
 			}
 			if (strnlen(value, MAX_USERNAME_SIZE) <
 						MAX_USERNAME_SIZE) {
-				vol->username = value;
+				vol->username = kstrdup(value, GFP_KERNEL);
+				if (!vol->username) {
+					printk(KERN_WARNING "CIFS: no memory "
+							    "for username\n");
+					goto cifs_parse_mount_err;
+				}
 			} else {
 				printk(KERN_WARNING "CIFS: username too long\n");
-				return 1;
+				goto cifs_parse_mount_err;
 			}
 		} else if (strnicmp(data, "pass", 4) == 0) {
 			if (!value) {
@@ -951,7 +962,7 @@ cifs_parse_mount_options(char *options, const char *devname,
 				if (vol->password == NULL) {
 					printk(KERN_WARNING "CIFS: no memory "
 							    "for password\n");
-					return 1;
+					goto cifs_parse_mount_err;
 				}
 				for (i = 0, j = 0; i < temp_len; i++, j++) {
 					vol->password[j] = value[i];
@@ -967,7 +978,7 @@ cifs_parse_mount_options(char *options, const char *devname,
 				if (vol->password == NULL) {
 					printk(KERN_WARNING "CIFS: no memory "
 							    "for password\n");
-					return 1;
+					goto cifs_parse_mount_err;
 				}
 				strcpy(vol->password, value);
 			}
@@ -977,11 +988,16 @@ cifs_parse_mount_options(char *options, const char *devname,
 				vol->UNCip = NULL;
 			} else if (strnlen(value, INET6_ADDRSTRLEN) <
 							INET6_ADDRSTRLEN) {
-				vol->UNCip = value;
+				vol->UNCip = kstrdup(value, GFP_KERNEL);
+				if (!vol->UNCip) {
+					printk(KERN_WARNING "CIFS: no memory "
+							    "for UNC IP\n");
+					goto cifs_parse_mount_err;
+				}
 			} else {
 				printk(KERN_WARNING "CIFS: ip address "
 						    "too long\n");
-				return 1;
+				goto cifs_parse_mount_err;
 			}
 		} else if (strnicmp(data, "sec", 3) == 0) {
 			if (!value || !*value) {
@@ -994,7 +1010,7 @@ cifs_parse_mount_options(char *options, const char *devname,
 				/* vol->secFlg |= CIFSSEC_MUST_SEAL |
 					CIFSSEC_MAY_KRB5; */
 				cERROR(1, "Krb5 cifs privacy not supported");
-				return 1;
+				goto cifs_parse_mount_err;
 			} else if (strnicmp(value, "krb5", 4) == 0) {
 				vol->secFlg |= CIFSSEC_MAY_KRB5;
 			} else if (strnicmp(value, "ntlmsspi", 8) == 0) {
@@ -1024,7 +1040,7 @@ cifs_parse_mount_options(char *options, const char *devname,
 				vol->nullauth = 1;
 			} else {
 				cERROR(1, "bad security option: %s", value);
-				return 1;
+				goto cifs_parse_mount_err;
 			}
 		} else if (strnicmp(data, "vers", 3) == 0) {
 			if (!value || !*value) {
@@ -1048,12 +1064,12 @@ cifs_parse_mount_options(char *options, const char *devname,
 			if (!value || !*value) {
 				printk(KERN_WARNING "CIFS: invalid path to "
 						    "network resource\n");
-				return 1;	/* needs_arg; */
+				goto cifs_parse_mount_err;
 			}
 			if ((temp_len = strnlen(value, 300)) < 300) {
 				vol->UNC = kmalloc(temp_len+1, GFP_KERNEL);
 				if (vol->UNC == NULL)
-					return 1;
+					goto cifs_parse_mount_err;
 				strcpy(vol->UNC, value);
 				if (strncmp(vol->UNC, "//", 2) == 0) {
 					vol->UNC[0] = '\\';
@@ -1062,27 +1078,32 @@ cifs_parse_mount_options(char *options, const char *devname,
 					printk(KERN_WARNING
 					       "CIFS: UNC Path does not begin "
 					       "with // or \\\\ \n");
-					return 1;
+					goto cifs_parse_mount_err;
 				}
 			} else {
 				printk(KERN_WARNING "CIFS: UNC name too long\n");
-				return 1;
+				goto cifs_parse_mount_err;
 			}
 		} else if ((strnicmp(data, "domain", 3) == 0)
 			   || (strnicmp(data, "workgroup", 5) == 0)) {
 			if (!value || !*value) {
 				printk(KERN_WARNING "CIFS: invalid domain name\n");
-				return 1;	/* needs_arg; */
+				goto cifs_parse_mount_err;
 			}
 			/* BB are there cases in which a comma can be valid in
 			a domain name and need special handling? */
 			if (strnlen(value, 256) < 256) {
-				vol->domainname = value;
+				vol->domainname = kstrdup(value, GFP_KERNEL);
+				if (!vol->domainname) {
+					printk(KERN_WARNING "CIFS: no memory "
+							    "for domainname\n");
+					goto cifs_parse_mount_err;
+				}
 				cFYI(1, "Domain name set");
 			} else {
 				printk(KERN_WARNING "CIFS: domain name too "
 						    "long\n");
-				return 1;
+				goto cifs_parse_mount_err;
 			}
 		} else if (strnicmp(data, "srcaddr", 7) == 0) {
 			vol->srcaddr.ss_family = AF_UNSPEC;
@@ -1090,7 +1111,7 @@ cifs_parse_mount_options(char *options, const char *devname,
 			if (!value || !*value) {
 				printk(KERN_WARNING "CIFS: srcaddr value"
 				       " not specified.\n");
-				return 1;	/* needs_arg; */
+				goto cifs_parse_mount_err;
 			}
 			i = cifs_convert_address((struct sockaddr *)&vol->srcaddr,
 						 value, strlen(value));
@@ -1098,20 +1119,20 @@ cifs_parse_mount_options(char *options, const char *devname,
 				printk(KERN_WARNING "CIFS:  Could not parse"
 				       " srcaddr: %s\n",
 				       value);
-				return 1;
+				goto cifs_parse_mount_err;
 			}
 		} else if (strnicmp(data, "prefixpath", 10) == 0) {
 			if (!value || !*value) {
 				printk(KERN_WARNING
 					"CIFS: invalid path prefix\n");
-				return 1;       /* needs_argument */
+				goto cifs_parse_mount_err;
 			}
 			if ((temp_len = strnlen(value, 1024)) < 1024) {
 				if (value[0] != '/')
 					temp_len++;  /* missing leading slash */
 				vol->prepath = kmalloc(temp_len+1, GFP_KERNEL);
 				if (vol->prepath == NULL)
-					return 1;
+					goto cifs_parse_mount_err;
 				if (value[0] != '/') {
 					vol->prepath[0] = '/';
 					strcpy(vol->prepath+1, value);
@@ -1120,24 +1141,33 @@ cifs_parse_mount_options(char *options, const char *devname,
 				cFYI(1, "prefix path %s", vol->prepath);
 			} else {
 				printk(KERN_WARNING "CIFS: prefix too long\n");
-				return 1;
+				goto cifs_parse_mount_err;
 			}
 		} else if (strnicmp(data, "iocharset", 9) == 0) {
 			if (!value || !*value) {
 				printk(KERN_WARNING "CIFS: invalid iocharset "
 						    "specified\n");
-				return 1;	/* needs_arg; */
+				goto cifs_parse_mount_err;
 			}
 			if (strnlen(value, 65) < 65) {
-				if (strnicmp(value, "default", 7))
-					vol->iocharset = value;
+				if (strnicmp(value, "default", 7)) {
+					vol->iocharset = kstrdup(value,
+								 GFP_KERNEL);
+
+					if (!vol->iocharset) {
+						printk(KERN_WARNING "CIFS: no "
+								   "memory for"
+								   "charset\n");
+						goto cifs_parse_mount_err;
+					}
+				}
 				/* if iocharset not set then load_nls_default
 				   is used by caller */
 				cFYI(1, "iocharset set to %s", value);
 			} else {
 				printk(KERN_WARNING "CIFS: iocharset name "
 						    "too long.\n");
-				return 1;
+				goto cifs_parse_mount_err;
 			}
 		} else if (!strnicmp(data, "uid", 3) && value && *value) {
 			vol->linux_uid = simple_strtoul(value, &value, 0);
@@ -1250,7 +1280,7 @@ cifs_parse_mount_options(char *options, const char *devname,
 				if (vol->actimeo > CIFS_MAX_ACTIMEO) {
 					cERROR(1, "CIFS: attribute cache"
 							"timeout too large");
-					return 1;
+					goto cifs_parse_mount_err;
 				}
 			}
 		} else if (strnicmp(data, "credentials", 4) == 0) {
@@ -1394,7 +1424,7 @@ cifs_parse_mount_options(char *options, const char *devname,
 #ifndef CONFIG_CIFS_FSCACHE
 			cERROR(1, "FS-Cache support needs CONFIG_CIFS_FSCACHE"
 				  "kernel config option set");
-			return 1;
+			goto cifs_parse_mount_err;
 #endif
 			vol->fsc = true;
 		} else if (strnicmp(data, "mfsymlinks", 10) == 0) {
@@ -1409,12 +1439,12 @@ cifs_parse_mount_options(char *options, const char *devname,
 		if (devname == NULL) {
 			printk(KERN_WARNING "CIFS: Missing UNC name for mount "
 						"target\n");
-			return 1;
+			goto cifs_parse_mount_err;
 		}
 		if ((temp_len = strnlen(devname, 300)) < 300) {
 			vol->UNC = kmalloc(temp_len+1, GFP_KERNEL);
 			if (vol->UNC == NULL)
-				return 1;
+				goto cifs_parse_mount_err;
 			strcpy(vol->UNC, devname);
 			if (strncmp(vol->UNC, "//", 2) == 0) {
 				vol->UNC[0] = '\\';
@@ -1422,21 +1452,21 @@ cifs_parse_mount_options(char *options, const char *devname,
 			} else if (strncmp(vol->UNC, "\\\\", 2) != 0) {
 				printk(KERN_WARNING "CIFS: UNC Path does not "
 						    "begin with // or \\\\ \n");
-				return 1;
+				goto cifs_parse_mount_err;
 			}
 			value = strpbrk(vol->UNC+2, "/\\");
 			if (value)
 				*value = '\\';
 		} else {
 			printk(KERN_WARNING "CIFS: UNC name too long\n");
-			return 1;
+			goto cifs_parse_mount_err;
 		}
 	}
 
 	if (vol->multiuser && !(vol->secFlg & CIFSSEC_MAY_KRB5)) {
 		cERROR(1, "Multiuser mounts currently require krb5 "
 			  "authentication!");
-		return 1;
+		goto cifs_parse_mount_err;
 	}
 
 	if (vol->UNCip == NULL)
@@ -1454,7 +1484,12 @@ cifs_parse_mount_options(char *options, const char *devname,
 		printk(KERN_NOTICE "CIFS: ignoring forcegid mount option "
 				   "specified with no gid= option.\n");
 
+	kfree(mountdata_copy);
 	return 0;
+
+cifs_parse_mount_err:
+	kfree(mountdata_copy);
+	return 1;
 }
 
 /** Returns true if srcaddr isn't specified and rhs isn't
@@ -2704,8 +2739,12 @@ cleanup_volume_info(struct smb_vol **pvolume_info)
 		return;
 
 	volume_info = *pvolume_info;
+	kfree(volume_info->username);
 	kzfree(volume_info->password);
 	kfree(volume_info->UNC);
+	kfree(volume_info->UNCip);
+	kfree(volume_info->domainname);
+	kfree(volume_info->iocharset);
 	kfree(volume_info->prepath);
 	kfree(volume_info);
 	*pvolume_info = NULL;
-- 
1.7.4.1

Re: [PATCH v4 3/6] cifs: cifs_parse_mount_options: do not tokenize mount options in-place

[Jeff Layton]

On Wed, 6 Apr 2011 16:26:42 +0200
Sean Finney <seanius-ADwgVSpYHhHR7s880joybQ@public.gmane.org> wrote:

> To keep strings passed to cifs_parse_mount_options re-usable (which is
> needed to clean up the DFS referral handling), tokenize a copy of the
> mount options instead.  If values are needed from this tokenized string,
> they too must be duplicated (previously, some options were copied and
> others duplicated).
> 
> Since we are not on the critical path and any cleanup is relatively easy,
> the extra memory usage shouldn't be a problem (and it is a bit simpler
> than trying to implement something smarter).
> 
> Signed-off-by: Sean Finney <seanius-ADwgVSpYHhHR7s880joybQ@public.gmane.org>
> ---
>  fs/cifs/connect.c |  109 ++++++++++++++++++++++++++++++++++++-----------------
>  1 files changed, 74 insertions(+), 35 deletions(-)
> 
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 4873bac..259fc64 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -807,11 +807,12 @@ extract_hostname(const char *unc)
>  }
>  
>  static int
> -cifs_parse_mount_options(char *options, const char *devname,
> +cifs_parse_mount_options(const char *mountdata, const char *devname,
>  			 struct smb_vol *vol)
>  {
>  	char *value;
>  	char *data;
> +	char *mountdata_copy, *options;
>  	unsigned int  temp_len, i, j;
>  	char separator[2];
>  	short int override_uid = -1;
> @@ -851,9 +852,14 @@ cifs_parse_mount_options(char *options, const char *devname,
>  
>  	vol->actimeo = CIFS_DEF_ACTIMEO;
>  
> -	if (!options)
> -		return 1;
> +	if (!mountdata)
> +		goto cifs_parse_mount_err;
> +
> +	mountdata_copy = kstrndup(mountdata, PAGE_CACHE_SIZE, GFP_KERNEL);
> +	if (!mountdata_copy)
> +		goto cifs_parse_mount_err;
>  
> +	options = mountdata_copy;
>  	if (strncmp(options, "sep=", 4) == 0) {
>  		if (options[4] != 0) {
>  			separator[0] = options[4];
> @@ -878,17 +884,22 @@ cifs_parse_mount_options(char *options, const char *devname,
>  			if (!value) {
>  				printk(KERN_WARNING
>  				       "CIFS: invalid or missing username\n");
> -				return 1;	/* needs_arg; */
> +				goto cifs_parse_mount_err;
>  			} else if (!*value) {
>  				/* null user, ie anonymous, authentication */
>  				vol->nullauth = 1;
>  			}
>  			if (strnlen(value, MAX_USERNAME_SIZE) <
>  						MAX_USERNAME_SIZE) {
> -				vol->username = value;
> +				vol->username = kstrdup(value, GFP_KERNEL);
> +				if (!vol->username) {
> +					printk(KERN_WARNING "CIFS: no memory "
> +							    "for username\n");
> +					goto cifs_parse_mount_err;
> +				}
>  			} else {
>  				printk(KERN_WARNING "CIFS: username too long\n");
> -				return 1;
> +				goto cifs_parse_mount_err;
>  			}
>  		} else if (strnicmp(data, "pass", 4) == 0) {
>  			if (!value) {
> @@ -951,7 +962,7 @@ cifs_parse_mount_options(char *options, const char *devname,
>  				if (vol->password == NULL) {
>  					printk(KERN_WARNING "CIFS: no memory "
>  							    "for password\n");
> -					return 1;
> +					goto cifs_parse_mount_err;
>  				}
>  				for (i = 0, j = 0; i < temp_len; i++, j++) {
>  					vol->password[j] = value[i];
> @@ -967,7 +978,7 @@ cifs_parse_mount_options(char *options, const char *devname,
>  				if (vol->password == NULL) {
>  					printk(KERN_WARNING "CIFS: no memory "
>  							    "for password\n");
> -					return 1;
> +					goto cifs_parse_mount_err;
>  				}
>  				strcpy(vol->password, value);
>  			}
> @@ -977,11 +988,16 @@ cifs_parse_mount_options(char *options, const char *devname,
>  				vol->UNCip = NULL;
>  			} else if (strnlen(value, INET6_ADDRSTRLEN) <
>  							INET6_ADDRSTRLEN) {
> -				vol->UNCip = value;
> +				vol->UNCip = kstrdup(value, GFP_KERNEL);
> +				if (!vol->UNCip) {
> +					printk(KERN_WARNING "CIFS: no memory "
> +							    "for UNC IP\n");
> +					goto cifs_parse_mount_err;
> +				}
>  			} else {
>  				printk(KERN_WARNING "CIFS: ip address "
>  						    "too long\n");
> -				return 1;
> +				goto cifs_parse_mount_err;
>  			}
>  		} else if (strnicmp(data, "sec", 3) == 0) {
>  			if (!value || !*value) {
> @@ -994,7 +1010,7 @@ cifs_parse_mount_options(char *options, const char *devname,
>  				/* vol->secFlg |= CIFSSEC_MUST_SEAL |
>  					CIFSSEC_MAY_KRB5; */
>  				cERROR(1, "Krb5 cifs privacy not supported");
> -				return 1;
> +				goto cifs_parse_mount_err;
>  			} else if (strnicmp(value, "krb5", 4) == 0) {
>  				vol->secFlg |= CIFSSEC_MAY_KRB5;
>  			} else if (strnicmp(value, "ntlmsspi", 8) == 0) {
> @@ -1024,7 +1040,7 @@ cifs_parse_mount_options(char *options, const char *devname,
>  				vol->nullauth = 1;
>  			} else {
>  				cERROR(1, "bad security option: %s", value);
> -				return 1;
> +				goto cifs_parse_mount_err;
>  			}
>  		} else if (strnicmp(data, "vers", 3) == 0) {
>  			if (!value || !*value) {
> @@ -1048,12 +1064,12 @@ cifs_parse_mount_options(char *options, const char *devname,
>  			if (!value || !*value) {
>  				printk(KERN_WARNING "CIFS: invalid path to "
>  						    "network resource\n");
> -				return 1;	/* needs_arg; */
> +				goto cifs_parse_mount_err;
>  			}
>  			if ((temp_len = strnlen(value, 300)) < 300) {
>  				vol->UNC = kmalloc(temp_len+1, GFP_KERNEL);
>  				if (vol->UNC == NULL)
> -					return 1;
> +					goto cifs_parse_mount_err;
>  				strcpy(vol->UNC, value);
>  				if (strncmp(vol->UNC, "//", 2) == 0) {
>  					vol->UNC[0] = '\\';
> @@ -1062,27 +1078,32 @@ cifs_parse_mount_options(char *options, const char *devname,
>  					printk(KERN_WARNING
>  					       "CIFS: UNC Path does not begin "
>  					       "with // or \\\\ \n");
> -					return 1;
> +					goto cifs_parse_mount_err;
>  				}
>  			} else {
>  				printk(KERN_WARNING "CIFS: UNC name too long\n");
> -				return 1;
> +				goto cifs_parse_mount_err;
>  			}
>  		} else if ((strnicmp(data, "domain", 3) == 0)
>  			   || (strnicmp(data, "workgroup", 5) == 0)) {
>  			if (!value || !*value) {
>  				printk(KERN_WARNING "CIFS: invalid domain name\n");
> -				return 1;	/* needs_arg; */
> +				goto cifs_parse_mount_err;
>  			}
>  			/* BB are there cases in which a comma can be valid in
>  			a domain name and need special handling? */
>  			if (strnlen(value, 256) < 256) {
> -				vol->domainname = value;
> +				vol->domainname = kstrdup(value, GFP_KERNEL);
> +				if (!vol->domainname) {
> +					printk(KERN_WARNING "CIFS: no memory "
> +							    "for domainname\n");
> +					goto cifs_parse_mount_err;
> +				}
>  				cFYI(1, "Domain name set");
>  			} else {
>  				printk(KERN_WARNING "CIFS: domain name too "
>  						    "long\n");
> -				return 1;
> +				goto cifs_parse_mount_err;
>  			}
>  		} else if (strnicmp(data, "srcaddr", 7) == 0) {
>  			vol->srcaddr.ss_family = AF_UNSPEC;
> @@ -1090,7 +1111,7 @@ cifs_parse_mount_options(char *options, const char *devname,
>  			if (!value || !*value) {
>  				printk(KERN_WARNING "CIFS: srcaddr value"
>  				       " not specified.\n");
> -				return 1;	/* needs_arg; */
> +				goto cifs_parse_mount_err;
>  			}
>  			i = cifs_convert_address((struct sockaddr *)&vol->srcaddr,
>  						 value, strlen(value));
> @@ -1098,20 +1119,20 @@ cifs_parse_mount_options(char *options, const char *devname,
>  				printk(KERN_WARNING "CIFS:  Could not parse"
>  				       " srcaddr: %s\n",
>  				       value);
> -				return 1;
> +				goto cifs_parse_mount_err;
>  			}
>  		} else if (strnicmp(data, "prefixpath", 10) == 0) {
>  			if (!value || !*value) {
>  				printk(KERN_WARNING
>  					"CIFS: invalid path prefix\n");
> -				return 1;       /* needs_argument */
> +				goto cifs_parse_mount_err;
>  			}
>  			if ((temp_len = strnlen(value, 1024)) < 1024) {
>  				if (value[0] != '/')
>  					temp_len++;  /* missing leading slash */
>  				vol->prepath = kmalloc(temp_len+1, GFP_KERNEL);
>  				if (vol->prepath == NULL)
> -					return 1;
> +					goto cifs_parse_mount_err;
>  				if (value[0] != '/') {
>  					vol->prepath[0] = '/';
>  					strcpy(vol->prepath+1, value);
> @@ -1120,24 +1141,33 @@ cifs_parse_mount_options(char *options, const char *devname,
>  				cFYI(1, "prefix path %s", vol->prepath);
>  			} else {
>  				printk(KERN_WARNING "CIFS: prefix too long\n");
> -				return 1;
> +				goto cifs_parse_mount_err;
>  			}
>  		} else if (strnicmp(data, "iocharset", 9) == 0) {
>  			if (!value || !*value) {
>  				printk(KERN_WARNING "CIFS: invalid iocharset "
>  						    "specified\n");
> -				return 1;	/* needs_arg; */
> +				goto cifs_parse_mount_err;
>  			}
>  			if (strnlen(value, 65) < 65) {
> -				if (strnicmp(value, "default", 7))
> -					vol->iocharset = value;
> +				if (strnicmp(value, "default", 7)) {
> +					vol->iocharset = kstrdup(value,
> +								 GFP_KERNEL);
> +
> +					if (!vol->iocharset) {
> +						printk(KERN_WARNING "CIFS: no "
> +								   "memory for"
> +								   "charset\n");
> +						goto cifs_parse_mount_err;
> +					}
> +				}
>  				/* if iocharset not set then load_nls_default
>  				   is used by caller */
>  				cFYI(1, "iocharset set to %s", value);
>  			} else {
>  				printk(KERN_WARNING "CIFS: iocharset name "
>  						    "too long.\n");
> -				return 1;
> +				goto cifs_parse_mount_err;
>  			}
>  		} else if (!strnicmp(data, "uid", 3) && value && *value) {
>  			vol->linux_uid = simple_strtoul(value, &value, 0);
> @@ -1250,7 +1280,7 @@ cifs_parse_mount_options(char *options, const char *devname,
>  				if (vol->actimeo > CIFS_MAX_ACTIMEO) {
>  					cERROR(1, "CIFS: attribute cache"
>  							"timeout too large");
> -					return 1;
> +					goto cifs_parse_mount_err;
>  				}
>  			}
>  		} else if (strnicmp(data, "credentials", 4) == 0) {
> @@ -1394,7 +1424,7 @@ cifs_parse_mount_options(char *options, const char *devname,
>  #ifndef CONFIG_CIFS_FSCACHE
>  			cERROR(1, "FS-Cache support needs CONFIG_CIFS_FSCACHE"
>  				  "kernel config option set");
> -			return 1;
> +			goto cifs_parse_mount_err;
>  #endif
>  			vol->fsc = true;
>  		} else if (strnicmp(data, "mfsymlinks", 10) == 0) {
> @@ -1409,12 +1439,12 @@ cifs_parse_mount_options(char *options, const char *devname,
>  		if (devname == NULL) {
>  			printk(KERN_WARNING "CIFS: Missing UNC name for mount "
>  						"target\n");
> -			return 1;
> +			goto cifs_parse_mount_err;
>  		}
>  		if ((temp_len = strnlen(devname, 300)) < 300) {
>  			vol->UNC = kmalloc(temp_len+1, GFP_KERNEL);
>  			if (vol->UNC == NULL)
> -				return 1;
> +				goto cifs_parse_mount_err;
>  			strcpy(vol->UNC, devname);
>  			if (strncmp(vol->UNC, "//", 2) == 0) {
>  				vol->UNC[0] = '\\';
> @@ -1422,21 +1452,21 @@ cifs_parse_mount_options(char *options, const char *devname,
>  			} else if (strncmp(vol->UNC, "\\\\", 2) != 0) {
>  				printk(KERN_WARNING "CIFS: UNC Path does not "
>  						    "begin with // or \\\\ \n");
> -				return 1;
> +				goto cifs_parse_mount_err;
>  			}
>  			value = strpbrk(vol->UNC+2, "/\\");
>  			if (value)
>  				*value = '\\';
>  		} else {
>  			printk(KERN_WARNING "CIFS: UNC name too long\n");
> -			return 1;
> +			goto cifs_parse_mount_err;
>  		}
>  	}
>  
>  	if (vol->multiuser && !(vol->secFlg & CIFSSEC_MAY_KRB5)) {
>  		cERROR(1, "Multiuser mounts currently require krb5 "
>  			  "authentication!");
> -		return 1;
> +		goto cifs_parse_mount_err;
>  	}
>  
>  	if (vol->UNCip == NULL)
> @@ -1454,7 +1484,12 @@ cifs_parse_mount_options(char *options, const char *devname,
>  		printk(KERN_NOTICE "CIFS: ignoring forcegid mount option "
>  				   "specified with no gid= option.\n");
>  
> +	kfree(mountdata_copy);
>  	return 0;
> +
> +cifs_parse_mount_err:
> +	kfree(mountdata_copy);
> +	return 1;
>  }
>  
>  /** Returns true if srcaddr isn't specified and rhs isn't
> @@ -2704,8 +2739,12 @@ cleanup_volume_info(struct smb_vol **pvolume_info)
>  		return;
>  
>  	volume_info = *pvolume_info;
> +	kfree(volume_info->username);
>  	kzfree(volume_info->password);
>  	kfree(volume_info->UNC);
> +	kfree(volume_info->UNCip);
> +	kfree(volume_info->domainname);
> +	kfree(volume_info->iocharset);
>  	kfree(volume_info->prepath);
>  	kfree(volume_info);
>  	*pvolume_info = NULL;

Looks reasonable. I think we could optimize this some by not saving the
iocharset and just setting the nls struct in here instead, but that's a
minor nit and could be fixed up later.

Reviwed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

[PATCH v4 4/6] cifs: Simplify handling of submount options in cifs_mount.

[Sean Finney]

With CONFIG_DFS_UPCALL enabled, maintain the submount options in
cifs_sb->mountdata, simplifying the code just a bit as well as making
corner-case allocation problems less likely.

Signed-off-by: Sean Finney <seanius-ADwgVSpYHhHR7s880joybQ@public.gmane.org>
---
 fs/cifs/connect.c |   24 +++++++++++-------------
 1 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 259fc64..8d2529c 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -2785,9 +2785,9 @@ build_unc_path_to_root(const struct smb_vol *volume_info,
 /*
  * Perform a dfs referral query for a share and (optionally) prefix
  *
- * If a referral is found, mount_data will be set to point at a newly
- * allocated string containing updated options for the submount.
- * Otherwise it will be left untouched.
+ * If a referral is found, cifs_sb->mountdata will be (re-)allocated
+ * to a string containing updated options for the submount.  Otherwise it
+ * will be left untouched.
  *
  * Returns the rc from get_dfs_path to the caller, which can be used to
  * determine whether there were referrals.
@@ -2795,7 +2795,7 @@ build_unc_path_to_root(const struct smb_vol *volume_info,
 static int
 expand_dfs_referral(int xid, struct cifs_ses *pSesInfo,
 		    struct smb_vol *volume_info, struct cifs_sb_info *cifs_sb,
-		    char **mount_data, int check_prefix)
+		    int check_prefix)
 {
 	int rc;
 	unsigned int num_referrals = 0;
@@ -2823,11 +2823,14 @@ expand_dfs_referral(int xid, struct cifs_ses *pSesInfo,
 		free_dfs_info_array(referrals, num_referrals);
 		kfree(fake_devname);
 
+		if (cifs_sb->mountdata != NULL)
+			kfree(cifs_sb->mountdata);
+
 		if (IS_ERR(mdata)) {
 			rc = PTR_ERR(mdata);
 			mdata = NULL;
 		}
-		*mount_data = mdata;
+		cifs_sb->mountdata = mdata;
 	}
 	kfree(full_path);
 	return rc;
@@ -2850,6 +2853,7 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
 #ifdef CONFIG_CIFS_DFS_UPCALL
 	int referral_walks_count = 0;
 try_mount_again:
+	mount_data = cifs_sb->mountdata;
 
 	/* cleanup activities if we're chasing a referral */
 	if (referral_walks_count) {
@@ -2983,7 +2987,7 @@ remote_path_check:
 	 */
 	if (referral_walks_count == 0) {
 		int refrc = expand_dfs_referral(xid, pSesInfo, volume_info,
-						cifs_sb, &mount_data, false);
+						cifs_sb, false);
 		if (!refrc) {
 			referral_walks_count++;
 			goto try_mount_again;
@@ -3025,17 +3029,13 @@ remote_path_check:
 			convert_delimiter(cifs_sb->prepath,
 					CIFS_DIR_SEP(cifs_sb));
 
-		if (mount_data != mount_data_global)
-			kfree(mount_data);
-
 		rc = expand_dfs_referral(xid, pSesInfo, volume_info, cifs_sb,
-					 &mount_data, true);
+					 true);
 
 		if (!rc) {
 			referral_walks_count++;
 			goto try_mount_again;
 		}
-		mount_data = NULL;
 		goto mount_fail_check;
 #else /* No DFS support, return error on mount */
 		rc = -EOPNOTSUPP;
@@ -3069,8 +3069,6 @@ remote_path_check:
 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)
-- 
1.7.4.1

Re: [PATCH v4 4/6] cifs: Simplify handling of submount options in cifs_mount.

[Jeff Layton]

On Wed, 6 Apr 2011 16:26:43 +0200
Sean Finney <seanius-ADwgVSpYHhHR7s880joybQ@public.gmane.org> wrote:

> With CONFIG_DFS_UPCALL enabled, maintain the submount options in
> cifs_sb->mountdata, simplifying the code just a bit as well as making
> corner-case allocation problems less likely.
> 
> Signed-off-by: Sean Finney <seanius-ADwgVSpYHhHR7s880joybQ@public.gmane.org>
> ---
>  fs/cifs/connect.c |   24 +++++++++++-------------
>  1 files changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 259fc64..8d2529c 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -2785,9 +2785,9 @@ build_unc_path_to_root(const struct smb_vol *volume_info,
>  /*
>   * Perform a dfs referral query for a share and (optionally) prefix
>   *
> - * If a referral is found, mount_data will be set to point at a newly
> - * allocated string containing updated options for the submount.
> - * Otherwise it will be left untouched.
> + * If a referral is found, cifs_sb->mountdata will be (re-)allocated
> + * to a string containing updated options for the submount.  Otherwise it
> + * will be left untouched.
>   *
>   * Returns the rc from get_dfs_path to the caller, which can be used to
>   * determine whether there were referrals.
> @@ -2795,7 +2795,7 @@ build_unc_path_to_root(const struct smb_vol *volume_info,
>  static int
>  expand_dfs_referral(int xid, struct cifs_ses *pSesInfo,
>  		    struct smb_vol *volume_info, struct cifs_sb_info *cifs_sb,
> -		    char **mount_data, int check_prefix)
> +		    int check_prefix)
>  {
>  	int rc;
>  	unsigned int num_referrals = 0;
> @@ -2823,11 +2823,14 @@ expand_dfs_referral(int xid, struct cifs_ses *pSesInfo,
>  		free_dfs_info_array(referrals, num_referrals);
>  		kfree(fake_devname);
>  
> +		if (cifs_sb->mountdata != NULL)
> +			kfree(cifs_sb->mountdata);
> +
>  		if (IS_ERR(mdata)) {
>  			rc = PTR_ERR(mdata);
>  			mdata = NULL;
>  		}
> -		*mount_data = mdata;
> +		cifs_sb->mountdata = mdata;
>  	}
>  	kfree(full_path);
>  	return rc;
> @@ -2850,6 +2853,7 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
>  #ifdef CONFIG_CIFS_DFS_UPCALL
>  	int referral_walks_count = 0;
>  try_mount_again:
> +	mount_data = cifs_sb->mountdata;
>  
>  	/* cleanup activities if we're chasing a referral */
>  	if (referral_walks_count) {
> @@ -2983,7 +2987,7 @@ remote_path_check:
>  	 */
>  	if (referral_walks_count == 0) {
>  		int refrc = expand_dfs_referral(xid, pSesInfo, volume_info,
> -						cifs_sb, &mount_data, false);
> +						cifs_sb, false);
>  		if (!refrc) {
>  			referral_walks_count++;
>  			goto try_mount_again;
> @@ -3025,17 +3029,13 @@ remote_path_check:
>  			convert_delimiter(cifs_sb->prepath,
>  					CIFS_DIR_SEP(cifs_sb));
>  
> -		if (mount_data != mount_data_global)
> -			kfree(mount_data);
> -
>  		rc = expand_dfs_referral(xid, pSesInfo, volume_info, cifs_sb,
> -					 &mount_data, true);
> +					 true);
>  
>  		if (!rc) {
>  			referral_walks_count++;
>  			goto try_mount_again;
>  		}
> -		mount_data = NULL;
>  		goto mount_fail_check;
>  #else /* No DFS support, return error on mount */
>  		rc = -EOPNOTSUPP;
> @@ -3069,8 +3069,6 @@ remote_path_check:
>  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)

Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

[PATCH v4 5/6] cifs: Use kstrndup for cifs_sb->mountdata

[Sean Finney]

A relatively minor nit, but also clarified the "consensus" from the
preceding comments that it is in fact better to try for the kstrdup
early and cleanup while cleaning up is still a simple thing to do.

Reviewed-By: Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Sean Finney <seanius-ADwgVSpYHhHR7s880joybQ@public.gmane.org>
---
 fs/cifs/cifsfs.c |   18 +++++++-----------
 1 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index fb6a2ad..cf8a610 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -134,24 +134,20 @@ cifs_read_super(struct super_block *sb, void *data,
 	cifs_sb->bdi.ra_pages = default_backing_dev_info.ra_pages;
 
 #ifdef CONFIG_CIFS_DFS_UPCALL
-	/* copy mount params to sb for use in submounts */
-	/* BB: should we move this after the mount so we
-	 * do not have to do the copy on failed mounts?
-	 * BB: May be it is better to do simple copy before
-	 * complex operation (mount), and in case of fail
-	 * just exit instead of doing mount and attempting
-	 * undo it if this copy fails?*/
+	/*
+	 * Copy mount params to sb for use in submounts. Better to do
+	 * the copy here and deal with the error before cleanup gets
+	 * complicated post-mount.
+	 */
 	if (data) {
-		int len = strlen(data);
-		cifs_sb->mountdata = kzalloc(len + 1, GFP_KERNEL);
+		cifs_sb->mountdata = kstrndup(data, PAGE_CACHE_SIZE,
+					      GFP_KERNEL);
 		if (cifs_sb->mountdata == NULL) {
 			bdi_destroy(&cifs_sb->bdi);
 			kfree(sb->s_fs_info);
 			sb->s_fs_info = NULL;
 			return -ENOMEM;
 		}
-		strncpy(cifs_sb->mountdata, data, len + 1);
-		cifs_sb->mountdata[len] = '\0';
 	}
 #endif
 
-- 
1.7.4.1

Re: [PATCH v4 5/6] cifs: Use kstrndup for cifs_sb->mountdata

[Jeff Layton]

On Wed, 6 Apr 2011 16:26:44 +0200
Sean Finney <seanius-ADwgVSpYHhHR7s880joybQ@public.gmane.org> wrote:

> A relatively minor nit, but also clarified the "consensus" from the
> preceding comments that it is in fact better to try for the kstrdup
> early and cleanup while cleaning up is still a simple thing to do.
> 
> Reviewed-By: Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Sean Finney <seanius-ADwgVSpYHhHR7s880joybQ@public.gmane.org>
> ---
>  fs/cifs/cifsfs.c |   18 +++++++-----------
>  1 files changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index fb6a2ad..cf8a610 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -134,24 +134,20 @@ cifs_read_super(struct super_block *sb, void *data,
>  	cifs_sb->bdi.ra_pages = default_backing_dev_info.ra_pages;
>  
>  #ifdef CONFIG_CIFS_DFS_UPCALL
> -	/* copy mount params to sb for use in submounts */
> -	/* BB: should we move this after the mount so we
> -	 * do not have to do the copy on failed mounts?
> -	 * BB: May be it is better to do simple copy before
> -	 * complex operation (mount), and in case of fail
> -	 * just exit instead of doing mount and attempting
> -	 * undo it if this copy fails?*/
> +	/*
> +	 * Copy mount params to sb for use in submounts. Better to do
> +	 * the copy here and deal with the error before cleanup gets
> +	 * complicated post-mount.
> +	 */
>  	if (data) {
> -		int len = strlen(data);
> -		cifs_sb->mountdata = kzalloc(len + 1, GFP_KERNEL);
> +		cifs_sb->mountdata = kstrndup(data, PAGE_CACHE_SIZE,
> +					      GFP_KERNEL);
>  		if (cifs_sb->mountdata == NULL) {
>  			bdi_destroy(&cifs_sb->bdi);
>  			kfree(sb->s_fs_info);
>  			sb->s_fs_info = NULL;
>  			return -ENOMEM;
>  		}
> -		strncpy(cifs_sb->mountdata, data, len + 1);
> -		cifs_sb->mountdata[len] = '\0';
>  	}
>  #endif
>  

I was wrong before, and we should be using PAGE_SIZE here instead of
PAGE_CACHE_SIZE. On all arches that I know of those are equivalent
currently, but that might not be the case in the future. It might not
hurt to change that in these patches though if you're respinning them
anyway.

Either way, you can add my:

Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH v4 5/6] cifs: Use kstrndup for cifs_sb->mountdata

[Sean Finney]

On Fri, Apr 08, 2011 at 09:53:33AM -0400, Jeff Layton wrote:
> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > index fb6a2ad..cf8a610 100644
> > --- a/fs/cifs/cifsfs.c
> > +++ b/fs/cifs/cifsfs.c
> > @@ -134,24 +134,20 @@ cifs_read_super(struct super_block *sb, void *data,
> >  	cifs_sb->bdi.ra_pages = default_backing_dev_info.ra_pages;
> >  
> >  #ifdef CONFIG_CIFS_DFS_UPCALL
> > -	/* copy mount params to sb for use in submounts */
> > -	/* BB: should we move this after the mount so we
> > -	 * do not have to do the copy on failed mounts?
> > -	 * BB: May be it is better to do simple copy before
> > -	 * complex operation (mount), and in case of fail
> > -	 * just exit instead of doing mount and attempting
> > -	 * undo it if this copy fails?*/
> > +	/*
> > +	 * Copy mount params to sb for use in submounts. Better to do
> > +	 * the copy here and deal with the error before cleanup gets
> > +	 * complicated post-mount.
> > +	 */
> >  	if (data) {
> > -		int len = strlen(data);
> > -		cifs_sb->mountdata = kzalloc(len + 1, GFP_KERNEL);
> > +		cifs_sb->mountdata = kstrndup(data, PAGE_CACHE_SIZE,
> > +					      GFP_KERNEL);
> >  		if (cifs_sb->mountdata == NULL) {
> >  			bdi_destroy(&cifs_sb->bdi);
> >  			kfree(sb->s_fs_info);
> >  			sb->s_fs_info = NULL;
> >  			return -ENOMEM;
> >  		}
> > -		strncpy(cifs_sb->mountdata, data, len + 1);
> > -		cifs_sb->mountdata[len] = '\0';
> >  	}
> >  #endif
> >  
> 
> I was wrong before, and we should be using PAGE_SIZE here instead of
> PAGE_CACHE_SIZE. On all arches that I know of those are equivalent
> currently, but that might not be the case in the future. It might not
> hurt to change that in these patches though if you're respinning them
> anyway.

Okay, no big deal to do so, will update it before I re-submit the patches
on monday.  Thanks for the review!


	sean

[PATCH v4 6/6] cifs: Unconditionally copy mount options to superblock info

[Sean Finney]

Previously mount options were copied and updated in the cifs_sb_info
struct only when CONFIG_CIFS_DFS_UPCALL was enabled.  Making this
information generally available allows us to remove a number of ifdefs,
extra function params, and temporary variables.

Signed-off-by: Sean Finney <seanius-ADwgVSpYHhHR7s880joybQ@public.gmane.org>
---
 fs/cifs/cifs_fs_sb.h |    4 +---
 fs/cifs/cifsfs.c     |    8 +-------
 fs/cifs/cifsproto.h  |    2 +-
 fs/cifs/connect.c    |    8 +++-----
 4 files changed, 6 insertions(+), 16 deletions(-)

diff --git a/fs/cifs/cifs_fs_sb.h b/fs/cifs/cifs_fs_sb.h
index 1ef54ab..adecd36 100644
--- a/fs/cifs/cifs_fs_sb.h
+++ b/fs/cifs/cifs_fs_sb.h
@@ -60,9 +60,7 @@ struct cifs_sb_info {
 	unsigned int mnt_cifs_flags;
 	int	prepathlen;
 	char   *prepath; /* relative path under the share to mount to */
-#ifdef CONFIG_CIFS_DFS_UPCALL
-	char   *mountdata; /* mount options received at mount time */
-#endif
+	char   *mountdata; /* options received at mount time or via DFS refs */
 	struct backing_dev_info bdi;
 	struct delayed_work prune_tlinks;
 };
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index cf8a610..c89a699 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -133,7 +133,6 @@ cifs_read_super(struct super_block *sb, void *data,
 	}
 	cifs_sb->bdi.ra_pages = default_backing_dev_info.ra_pages;
 
-#ifdef CONFIG_CIFS_DFS_UPCALL
 	/*
 	 * Copy mount params to sb for use in submounts. Better to do
 	 * the copy here and deal with the error before cleanup gets
@@ -149,9 +148,8 @@ cifs_read_super(struct super_block *sb, void *data,
 			return -ENOMEM;
 		}
 	}
-#endif
 
-	rc = cifs_mount(sb, cifs_sb, data, devname);
+	rc = cifs_mount(sb, cifs_sb, devname);
 
 	if (rc) {
 		if (!silent)
@@ -203,12 +201,10 @@ out_no_root:
 
 out_mount_failed:
 	if (cifs_sb) {
-#ifdef CONFIG_CIFS_DFS_UPCALL
 		if (cifs_sb->mountdata) {
 			kfree(cifs_sb->mountdata);
 			cifs_sb->mountdata = NULL;
 		}
-#endif
 		unload_nls(cifs_sb->local_nls);
 		bdi_destroy(&cifs_sb->bdi);
 		kfree(cifs_sb);
@@ -232,12 +228,10 @@ cifs_put_super(struct super_block *sb)
 	rc = cifs_umount(sb, cifs_sb);
 	if (rc)
 		cERROR(1, "cifs_umount failed with return code %d", rc);
-#ifdef CONFIG_CIFS_DFS_UPCALL
 	if (cifs_sb->mountdata) {
 		kfree(cifs_sb->mountdata);
 		cifs_sb->mountdata = NULL;
 	}
-#endif
 
 	unload_nls(cifs_sb->local_nls);
 	bdi_destroy(&cifs_sb->bdi);
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index 76c4dc7f..9985f99 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -146,7 +146,7 @@ extern struct cifs_ntsd *get_cifs_acl(struct cifs_sb_info *, struct inode *,
 extern int set_cifs_acl(struct cifs_ntsd *, __u32, struct inode *,
 				const char *);
 
-extern int cifs_mount(struct super_block *, struct cifs_sb_info *, char *,
+extern int cifs_mount(struct super_block *, struct cifs_sb_info *,
 			const char *);
 extern int cifs_umount(struct super_block *, struct cifs_sb_info *);
 extern void cifs_dfs_release_automount_timer(void);
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 8d2529c..3ab54a0 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -2839,7 +2839,7 @@ expand_dfs_referral(int xid, struct cifs_ses *pSesInfo,
 
 int
 cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
-		char *mount_data_global, const char *devname)
+		const char *devname)
 {
 	int rc;
 	int xid;
@@ -2848,13 +2848,10 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
 	struct cifs_tcon *tcon;
 	struct TCP_Server_Info *srvTcp;
 	char   *full_path;
-	char *mount_data = mount_data_global;
 	struct tcon_link *tlink;
 #ifdef CONFIG_CIFS_DFS_UPCALL
 	int referral_walks_count = 0;
 try_mount_again:
-	mount_data = cifs_sb->mountdata;
-
 	/* cleanup activities if we're chasing a referral */
 	if (referral_walks_count) {
 		if (tcon)
@@ -2881,7 +2878,8 @@ try_mount_again:
 		goto out;
 	}
 
-	if (cifs_parse_mount_options(mount_data, devname, volume_info)) {
+	if (cifs_parse_mount_options(cifs_sb->mountdata, devname,
+				     volume_info)) {
 		rc = -EINVAL;
 		goto out;
 	}
-- 
1.7.4.1

Re: [PATCH v4 6/6] cifs: Unconditionally copy mount options to superblock info

[Jeff Layton]

On Wed, 6 Apr 2011 16:26:45 +0200
Sean Finney <seanius-ADwgVSpYHhHR7s880joybQ@public.gmane.org> wrote:

> Previously mount options were copied and updated in the cifs_sb_info
> struct only when CONFIG_CIFS_DFS_UPCALL was enabled.  Making this
> information generally available allows us to remove a number of ifdefs,
> extra function params, and temporary variables.
> 
> Signed-off-by: Sean Finney <seanius-ADwgVSpYHhHR7s880joybQ@public.gmane.org>
> ---
>  fs/cifs/cifs_fs_sb.h |    4 +---
>  fs/cifs/cifsfs.c     |    8 +-------
>  fs/cifs/cifsproto.h  |    2 +-
>  fs/cifs/connect.c    |    8 +++-----
>  4 files changed, 6 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/cifs/cifs_fs_sb.h b/fs/cifs/cifs_fs_sb.h
> index 1ef54ab..adecd36 100644
> --- a/fs/cifs/cifs_fs_sb.h
> +++ b/fs/cifs/cifs_fs_sb.h
> @@ -60,9 +60,7 @@ struct cifs_sb_info {
>  	unsigned int mnt_cifs_flags;
>  	int	prepathlen;
>  	char   *prepath; /* relative path under the share to mount to */
> -#ifdef CONFIG_CIFS_DFS_UPCALL
> -	char   *mountdata; /* mount options received at mount time */
> -#endif
> +	char   *mountdata; /* options received at mount time or via DFS refs */
>  	struct backing_dev_info bdi;
>  	struct delayed_work prune_tlinks;
>  };
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index cf8a610..c89a699 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -133,7 +133,6 @@ cifs_read_super(struct super_block *sb, void *data,
>  	}
>  	cifs_sb->bdi.ra_pages = default_backing_dev_info.ra_pages;
>  
> -#ifdef CONFIG_CIFS_DFS_UPCALL
>  	/*
>  	 * Copy mount params to sb for use in submounts. Better to do
>  	 * the copy here and deal with the error before cleanup gets
> @@ -149,9 +148,8 @@ cifs_read_super(struct super_block *sb, void *data,
>  			return -ENOMEM;
>  		}
>  	}
> -#endif
>  
> -	rc = cifs_mount(sb, cifs_sb, data, devname);
> +	rc = cifs_mount(sb, cifs_sb, devname);
>  
>  	if (rc) {
>  		if (!silent)
> @@ -203,12 +201,10 @@ out_no_root:
>  
>  out_mount_failed:
>  	if (cifs_sb) {
> -#ifdef CONFIG_CIFS_DFS_UPCALL
>  		if (cifs_sb->mountdata) {
>  			kfree(cifs_sb->mountdata);
>  			cifs_sb->mountdata = NULL;
>  		}
> -#endif
>  		unload_nls(cifs_sb->local_nls);
>  		bdi_destroy(&cifs_sb->bdi);
>  		kfree(cifs_sb);
> @@ -232,12 +228,10 @@ cifs_put_super(struct super_block *sb)
>  	rc = cifs_umount(sb, cifs_sb);
>  	if (rc)
>  		cERROR(1, "cifs_umount failed with return code %d", rc);
> -#ifdef CONFIG_CIFS_DFS_UPCALL
>  	if (cifs_sb->mountdata) {
>  		kfree(cifs_sb->mountdata);
>  		cifs_sb->mountdata = NULL;
>  	}
> -#endif
>  
>  	unload_nls(cifs_sb->local_nls);
>  	bdi_destroy(&cifs_sb->bdi);
> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> index 76c4dc7f..9985f99 100644
> --- a/fs/cifs/cifsproto.h
> +++ b/fs/cifs/cifsproto.h
> @@ -146,7 +146,7 @@ extern struct cifs_ntsd *get_cifs_acl(struct cifs_sb_info *, struct inode *,
>  extern int set_cifs_acl(struct cifs_ntsd *, __u32, struct inode *,
>  				const char *);
>  
> -extern int cifs_mount(struct super_block *, struct cifs_sb_info *, char *,
> +extern int cifs_mount(struct super_block *, struct cifs_sb_info *,
>  			const char *);
>  extern int cifs_umount(struct super_block *, struct cifs_sb_info *);
>  extern void cifs_dfs_release_automount_timer(void);
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 8d2529c..3ab54a0 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -2839,7 +2839,7 @@ expand_dfs_referral(int xid, struct cifs_ses *pSesInfo,
>  
>  int
>  cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
> -		char *mount_data_global, const char *devname)
> +		const char *devname)
>  {
>  	int rc;
>  	int xid;
> @@ -2848,13 +2848,10 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
>  	struct cifs_tcon *tcon;
>  	struct TCP_Server_Info *srvTcp;
>  	char   *full_path;
> -	char *mount_data = mount_data_global;
>  	struct tcon_link *tlink;
>  #ifdef CONFIG_CIFS_DFS_UPCALL
>  	int referral_walks_count = 0;
>  try_mount_again:
> -	mount_data = cifs_sb->mountdata;
> -
>  	/* cleanup activities if we're chasing a referral */
>  	if (referral_walks_count) {
>  		if (tcon)
> @@ -2881,7 +2878,8 @@ try_mount_again:
>  		goto out;
>  	}
>  
> -	if (cifs_parse_mount_options(mount_data, devname, volume_info)) {
> +	if (cifs_parse_mount_options(cifs_sb->mountdata, devname,
> +				     volume_info)) {
>  		rc = -EINVAL;
>  		goto out;
>  	}

Nice cleanup. Always nice to get rid of nasty #ifdef-ery:

Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH v4 1/6] cifs: Extract DFS referral expansion logic to separate function

[Suresh Jayaraman]

On 04/06/2011 07:56 PM, Sean Finney wrote:
> The logic behind the expansion of DFS referrals is now extracted from
> cifs_mount into a new static function, expand_dfs_referral.  This will
> reduce duplicate code in upcoming commits.
>
> Reviewed-by: Jeff Layton<jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Sean Finney<seanius-ADwgVSpYHhHR7s880joybQ@public.gmane.org>
> ---
>   fs/cifs/connect.c |  105 +++++++++++++++++++++++++++++++++++------------------
>   1 files changed, 69 insertions(+), 36 deletions(-)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 94e60c5..c89d871 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -2742,6 +2742,57 @@ build_unc_path_to_root(const struct smb_vol *volume_info,
>   	full_path[unc_len + cifs_sb->prepathlen] = 0; /* add trailing null */
>   	return full_path;
>   }
> +
> +/*
> + * Perform a dfs referral query for a share and (optionally) prefix
> + *
> + * If a referral is found, mount_data will be set to point at a newly
> + * allocated string containing updated options for the submount.
> + * Otherwise it will be left untouched.
> + *
> + * Returns the rc from get_dfs_path to the caller, which can be used to
> + * determine whether there were referrals.
> + */
> +static int
> +expand_dfs_referral(int xid, struct cifs_ses *pSesInfo,
				^^^ Did you mean struct cifsSesInfo ?

I'm not seeing any other patch renaming cifsSesInfo to cifs_ses. Or Am I 
missing something. Sorry about pitching in so late..
			
The compiler gives a warning, but nevertheless it compiles... looks odd.

-- 
Suresh Jayaraman

Re: [PATCH v4 1/6] cifs: Extract DFS referral expansion logic to separate function

[Sean Finney]

Hi Suresh,

On Tue, Apr 26, 2011 at 11:13:16AM +0530, Suresh Jayaraman wrote:
>> +static int
>> +expand_dfs_referral(int xid, struct cifs_ses *pSesInfo,
> 				^^^ Did you mean struct cifsSesInfo ?
>
> I'm not seeing any other patch renaming cifsSesInfo to cifs_ses. Or Am I  
> missing something. Sorry about pitching in so late..
> 			
> The compiler gives a warning, but nevertheless it compiles... looks odd.

this is based on steve's master/for-next branch, which among other things
has commit 1c1c95b7b97ee797832c0f973419983faa4761ed, which renames
cifsSesInfo to cifs_ses.  IIRC that was the only thing different between
the two branches, so if you'd like to try it against the mainline kernel
you can probably just re-rename the struct back to struct cifsSesInfo.


	sean