slash appended to filesystem when mtab symlinked to proc

slash appended to filesystem when mtab symlinked to proc

[Luk Claes]

Hi

A Debian user reported a bug that a slash gets appended to the
filesystem when he mounts a share. This only happens when mtab is
symlinked to proc which I can reproduce:

$ grep cifs /etc/fstab
//<host>/<path>	/mnt	cifs	user,user=<username>	0	0

$ mount /mnt

$ grep cifs /etc/mtab
//<host>/<path>/ /mnt cifs
rw,nosuid,nodev,relatime,unc=\\<host>\<path>,username=<username>,uid=1000,forceuid,gid=1000,forcegid,addr=<ip>,file_mode=0755,dir_mode=0755,serverino,rsize=16384,wsize=57344
0 0

$ umount /mnt
umount: /mnt mount disagrees with the fstab

There does not seem to be any other filesystems that append a slash to
the filesystem in proc. Does cifsfs do that intentionally (in which case
the client should cope with it: by means of umount.cifs?) or should it
get fixed in the kernel (maybe the client should still cope with it)?

Can you give me a hand in identifying the code that would need to be
updated to fix this so I can prepare a patch?

Cheers

Luk

Re: slash appended to filesystem when mtab symlinked to proc

[Jeff Layton]

On Mon, 04 Apr 2011 16:33:05 +0200
Luk Claes <luk-8fiUuRrzOP0dnm+yROfE0A@public.gmane.org> wrote:

> Hi
> 
> A Debian user reported a bug that a slash gets appended to the
> filesystem when he mounts a share. This only happens when mtab is
> symlinked to proc which I can reproduce:
> 
> $ grep cifs /etc/fstab
> //<host>/<path>	/mnt	cifs	user,user=<username>	0	0
> 
> $ mount /mnt
> 
> $ grep cifs /etc/mtab
> //<host>/<path>/ /mnt cifs
> rw,nosuid,nodev,relatime,unc=\\<host>\<path>,username=<username>,uid=1000,forceuid,gid=1000,forcegid,addr=<ip>,file_mode=0755,dir_mode=0755,serverino,rsize=16384,wsize=57344
> 0 0
> 
> $ umount /mnt
> umount: /mnt mount disagrees with the fstab
> 
> There does not seem to be any other filesystems that append a slash to
> the filesystem in proc. Does cifsfs do that intentionally (in which case
> the client should cope with it: by means of umount.cifs?) or should it
> get fixed in the kernel (maybe the client should still cope with it)?
> 
> Can you give me a hand in identifying the code that would need to be
> updated to fix this so I can prepare a patch?
> 
> Cheers
> 
> Luk

mount.cifs will convert the delimiters in the "device string" to
forward slashes (in case someone specifies a UNC with backslashes) and
then passes it down to the kernel. When it does this, it adds an
unnecessary '/' to the end. Probably what we need to do is make it pass
down the "original" device string to the kernel mount() call.

The reason mount.cifs does this though is because /proc/mounts turns
'\\' into an escaped number string. So, in addition to changing
mount.cifs to fix this, the kernel routines that format /proc/mounts
should also be fixed to handle backslashes correctly.

Cheers,
-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

[PATCH] mount.cifs: only convert '\' to '/' for dev_name

[Luk Claes]

When dev_name is constructed by combining slashes with the different
parsed_mount_info fields, there can be an unnecessary appended slash
compared to the original device string. So only convert backslashes
to slashes to get the device name from the original device string.

Signed-off-by: Luk Claes <luk-8fiUuRrzOP0dnm+yROfE0A@public.gmane.org>
---
 mount.cifs.c |   15 +++++----------
 1 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/mount.cifs.c b/mount.cifs.c
index 8e1e32b..2cec7e4 100644
--- a/mount.cifs.c
+++ b/mount.cifs.c
@@ -1824,10 +1824,7 @@ int main(int argc, char **argv)
 	}
 
 	/* lengths of different strings + slashes + trailing \0 */
-	dev_len = strnlen(parsed_info->host, sizeof(parsed_info->host)) +
-	    strnlen(parsed_info->share, sizeof(parsed_info->share)) +
-	    strnlen(parsed_info->prefix, sizeof(parsed_info->prefix)) +
-	    2 + 1 + 1 + 1;
+	dev_len = strlen(orig_dev) + 1;
 	dev_name = calloc(dev_len, 1);
 	if (!dev_name) {
 		rc = EX_SYSERR;
@@ -1835,12 +1832,10 @@ int main(int argc, char **argv)
 	}
 
 	/* rebuild device name with forward slashes */
-	strlcpy(dev_name, "//", dev_len);
-	strlcat(dev_name, parsed_info->host, dev_len);
-	strlcat(dev_name, "/", dev_len);
-	strlcat(dev_name, parsed_info->share, dev_len);
-	strlcat(dev_name, "/", dev_len);
-	strlcat(dev_name, parsed_info->prefix, dev_len);
+	strlcpy(dev_name, orig_dev, dev_len);
+	while (c = strcspn(dev_name, "\\") < dev_len - 1) {
+		dev_name[c] = '/';
+	}
 
 	currentaddress = parsed_info->addrlist;
 	nextaddress = strchr(currentaddress, ',');
-- 
1.7.4.1

Re: [PATCH] mount.cifs: only convert '\' to '/' for dev_name

[Jeff Layton]

On Wed,  6 Apr 2011 00:26:48 +0200
Luk Claes <luk-8fiUuRrzOP0dnm+yROfE0A@public.gmane.org> wrote:

> When dev_name is constructed by combining slashes with the different
> parsed_mount_info fields, there can be an unnecessary appended slash
> compared to the original device string. So only convert backslashes
> to slashes to get the device name from the original device string.
> 
> Signed-off-by: Luk Claes <luk-8fiUuRrzOP0dnm+yROfE0A@public.gmane.org>
> ---
>  mount.cifs.c |   15 +++++----------
>  1 files changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/mount.cifs.c b/mount.cifs.c
> index 8e1e32b..2cec7e4 100644
> --- a/mount.cifs.c
> +++ b/mount.cifs.c
> @@ -1824,10 +1824,7 @@ int main(int argc, char **argv)
>  	}
>  
>  	/* lengths of different strings + slashes + trailing \0 */
> -	dev_len = strnlen(parsed_info->host, sizeof(parsed_info->host)) +
> -	    strnlen(parsed_info->share, sizeof(parsed_info->share)) +
> -	    strnlen(parsed_info->prefix, sizeof(parsed_info->prefix)) +
> -	    2 + 1 + 1 + 1;
> +	dev_len = strlen(orig_dev) + 1;
>  	dev_name = calloc(dev_len, 1);
>  	if (!dev_name) {
>  		rc = EX_SYSERR;
> @@ -1835,12 +1832,10 @@ int main(int argc, char **argv)
>  	}
>  
>  	/* rebuild device name with forward slashes */
> -	strlcpy(dev_name, "//", dev_len);
> -	strlcat(dev_name, parsed_info->host, dev_len);
> -	strlcat(dev_name, "/", dev_len);
> -	strlcat(dev_name, parsed_info->share, dev_len);
> -	strlcat(dev_name, "/", dev_len);
> -	strlcat(dev_name, parsed_info->prefix, dev_len);
> +	strlcpy(dev_name, orig_dev, dev_len);
> +	while (c = strcspn(dev_name, "\\") < dev_len - 1) {
> +		dev_name[c] = '/';
> +	}
>  
>  	currentaddress = parsed_info->addrlist;
>  	nextaddress = strchr(currentaddress, ',');

Uh, no. That doesn't solve your problem. Imagine your fstab contains a
UNC line this:

    \\\\server\\share

Then you're back to the same issue -- the devname in /proc/mounts
doesn't match the one in fstab. 

I think you just want to pass in orig_dev to mount() as-is (and maybe
get rid of dev_name altogether). The fact that the kernel doesn't print
'\\' sanely in /proc/mounts is a separate (but related) issue, but
passing in the raw devname directly is the most future-proof solution
and shouldn't make the current situation any worse.
-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

[PATCH] mount.cifs: Use original device string all the way

[Luk Claes]

Don't construct a device name, but use the original device string
to mount so the device name in /proc/mounts matches the one in
/etc/fstab.

Signed-off-by: Luk Claes <luk-8fiUuRrzOP0dnm+yROfE0A@public.gmane.org>
---
 mount.cifs.c |   27 +++------------------------
 1 files changed, 3 insertions(+), 24 deletions(-)

diff --git a/mount.cifs.c b/mount.cifs.c
index 8e1e32b..29b0d4c 100644
--- a/mount.cifs.c
+++ b/mount.cifs.c
@@ -1673,12 +1673,11 @@ int main(int argc, char **argv)
 	char *orgoptions = NULL;
 	char *mountpoint = NULL;
 	char *options = NULL;
-	char *dev_name = NULL, *orig_dev = NULL;
+	char *orig_dev = NULL;
 	char *currentaddress, *nextaddress;
 	int rc = 0;
 	int already_uppercased = 0;
 	size_t options_size = MAX_OPTIONS_LEN;
-	size_t dev_len;
 	struct parsed_mount_info *parsed_info = NULL;
 	pid_t pid;
 	const char *fstype;
@@ -1823,25 +1822,6 @@ int main(int argc, char **argv)
 		goto mount_exit;
 	}
 
-	/* lengths of different strings + slashes + trailing \0 */
-	dev_len = strnlen(parsed_info->host, sizeof(parsed_info->host)) +
-	    strnlen(parsed_info->share, sizeof(parsed_info->share)) +
-	    strnlen(parsed_info->prefix, sizeof(parsed_info->prefix)) +
-	    2 + 1 + 1 + 1;
-	dev_name = calloc(dev_len, 1);
-	if (!dev_name) {
-		rc = EX_SYSERR;
-		goto mount_exit;
-	}
-
-	/* rebuild device name with forward slashes */
-	strlcpy(dev_name, "//", dev_len);
-	strlcat(dev_name, parsed_info->host, dev_len);
-	strlcat(dev_name, "/", dev_len);
-	strlcat(dev_name, parsed_info->share, dev_len);
-	strlcat(dev_name, "/", dev_len);
-	strlcat(dev_name, parsed_info->prefix, dev_len);
-
 	currentaddress = parsed_info->addrlist;
 	nextaddress = strchr(currentaddress, ',');
 	if (nextaddress)
@@ -1889,7 +1869,7 @@ mount_retry:
 	if (parsed_info->verboseflag)
 		fprintf(stderr, "\n");
 
-	rc = check_mtab(thisprogram, dev_name, mountpoint);
+	rc = check_mtab(thisprogram, orig_dev, mountpoint);
 	if (rc)
 		goto mount_exit;
 
@@ -1900,7 +1880,7 @@ mount_retry:
 
 	if (!parsed_info->fakemnt) {
 		toggle_dac_capability(0, 1);
-		rc = mount(dev_name, ".", fstype, parsed_info->flags, options);
+		rc = mount(orig_dev, ".", fstype, parsed_info->flags, options);
 		toggle_dac_capability(0, 0);
 		if (rc == 0)
 			goto do_mtab;
@@ -1948,7 +1928,6 @@ mount_exit:
 		memset(parsed_info->password, 0, sizeof(parsed_info->password));
 		munmap(parsed_info, sizeof(*parsed_info));
 	}
-	SAFE_FREE(dev_name);
 	SAFE_FREE(options);
 	SAFE_FREE(orgoptions);
 	return rc;
-- 
1.7.4.1

Re: [PATCH] mount.cifs: Use original device string all the way

[Jeff Layton]

On Wed,  6 Apr 2011 10:46:55 +0200
Luk Claes <luk-8fiUuRrzOP0dnm+yROfE0A@public.gmane.org> wrote:

> Don't construct a device name, but use the original device string
> to mount so the device name in /proc/mounts matches the one in
> /etc/fstab.
> 
> Signed-off-by: Luk Claes <luk-8fiUuRrzOP0dnm+yROfE0A@public.gmane.org>
> ---
>  mount.cifs.c |   27 +++------------------------
>  1 files changed, 3 insertions(+), 24 deletions(-)
> 
> diff --git a/mount.cifs.c b/mount.cifs.c
> index 8e1e32b..29b0d4c 100644
> --- a/mount.cifs.c
> +++ b/mount.cifs.c
> @@ -1673,12 +1673,11 @@ int main(int argc, char **argv)
>  	char *orgoptions = NULL;
>  	char *mountpoint = NULL;
>  	char *options = NULL;
> -	char *dev_name = NULL, *orig_dev = NULL;
> +	char *orig_dev = NULL;
>  	char *currentaddress, *nextaddress;
>  	int rc = 0;
>  	int already_uppercased = 0;
>  	size_t options_size = MAX_OPTIONS_LEN;
> -	size_t dev_len;
>  	struct parsed_mount_info *parsed_info = NULL;
>  	pid_t pid;
>  	const char *fstype;
> @@ -1823,25 +1822,6 @@ int main(int argc, char **argv)
>  		goto mount_exit;
>  	}
>  
> -	/* lengths of different strings + slashes + trailing \0 */
> -	dev_len = strnlen(parsed_info->host, sizeof(parsed_info->host)) +
> -	    strnlen(parsed_info->share, sizeof(parsed_info->share)) +
> -	    strnlen(parsed_info->prefix, sizeof(parsed_info->prefix)) +
> -	    2 + 1 + 1 + 1;
> -	dev_name = calloc(dev_len, 1);
> -	if (!dev_name) {
> -		rc = EX_SYSERR;
> -		goto mount_exit;
> -	}
> -
> -	/* rebuild device name with forward slashes */
> -	strlcpy(dev_name, "//", dev_len);
> -	strlcat(dev_name, parsed_info->host, dev_len);
> -	strlcat(dev_name, "/", dev_len);
> -	strlcat(dev_name, parsed_info->share, dev_len);
> -	strlcat(dev_name, "/", dev_len);
> -	strlcat(dev_name, parsed_info->prefix, dev_len);
> -
>  	currentaddress = parsed_info->addrlist;
>  	nextaddress = strchr(currentaddress, ',');
>  	if (nextaddress)
> @@ -1889,7 +1869,7 @@ mount_retry:
>  	if (parsed_info->verboseflag)
>  		fprintf(stderr, "\n");
>  
> -	rc = check_mtab(thisprogram, dev_name, mountpoint);
> +	rc = check_mtab(thisprogram, orig_dev, mountpoint);
>  	if (rc)
>  		goto mount_exit;
>  
> @@ -1900,7 +1880,7 @@ mount_retry:
>  
>  	if (!parsed_info->fakemnt) {
>  		toggle_dac_capability(0, 1);
> -		rc = mount(dev_name, ".", fstype, parsed_info->flags, options);
> +		rc = mount(orig_dev, ".", fstype, parsed_info->flags, options);
>  		toggle_dac_capability(0, 0);
>  		if (rc == 0)
>  			goto do_mtab;
> @@ -1948,7 +1928,6 @@ mount_exit:
>  		memset(parsed_info->password, 0, sizeof(parsed_info->password));
>  		munmap(parsed_info, sizeof(*parsed_info));
>  	}
> -	SAFE_FREE(dev_name);
>  	SAFE_FREE(options);
>  	SAFE_FREE(orgoptions);
>  	return rc;

Looks good to me, thanks.

Committed. Should make 4.10 (or whatever we'll call the next release).

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>