On Wed, 6 Apr 2011 16:26:42 +0200
Sean Finney <seanius@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
