nfs/cifs mountinfo differences (was: Re: [PATCH] libmount: Skip root comparison for cifs in mnt_table_is_fs_mounted())

nfs/cifs mountinfo differences (was: Re: [PATCH] libmount: Skip root comparison for cifs in mnt_table_is_fs_mounted())

[Stanislav Brabec]

Stanislav Brabec wrote:
> In mountinfo of cifs filesystem, root is filled with a relative path to the root
> of the exported volume. There is no reasonable way to compare it without making
> a network look-up.

This is an original problem of "mount -a", see How to reproduce below.

> This causes false negative result in some cases.
>
> Skipping root comparison in mnt_table_is_fs_mounted() makes things better,
> however it still does not cover all possible setups.

I just compared behavior of cifs and nfs4 in this situation.

cifs and nfs4 provide different mountinfo. So maybe it is more an 
inconsistency of procinfo root entry than problem of mount.

Sending mail also to linux-cifs to get an opinion.

When mounting subdirectory of exported nfs4 volume, 4th record (root) 
remains "/".

When mounting subdirectory of exported cifs volume, 4th record (root) is 
the "/subdir", and 9th record (source) contains fill path 
"//server/volume/subdir".

And for comparison, when bind mounting cifs subdirectory out of volume 
mount, 4th record is "/subdir" as well, but the string in 9th record 
(source) does not contain trailing "/subdir", just "//server/volume".

And for completness, when bind mounting subdirectory of a btrfs 
subvolume, 4th record (root) contains full path to the subdirectory 
relative to the volume root (i. e. including subvolume path). Content of 
9th record (source) is not relevant for comparison, as it contains 
/dev/sd* node.

> How to reproduce:
> 1) Create and export a cifs volume (with guest allowed).
> 2) Make a subdirectory in this volume.
> 3) Mount this subdirectory using fstab:
>    //server/volume/subdir /mnt cifs guest 0 0
> 4) Call mount -a twice.
>    Depending on a system running, you either get:
>      mount error(16): Device or resource busy
>      Refer to the mount.cifs(8) manual page (e.g. man mount.cifs)
>    or you get a volume mounted twice.
>
> Signed-off-by: Stanislav Brabec <sbrabec-AlSwsSmVLrQ@public.gmane.org>
> ---

Here is the proposed patch for util-linux:

>  libmount/src/tab.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/libmount/src/tab.c b/libmount/src/tab.c
> index 155c65e..aa9185a 100644
> --- a/libmount/src/tab.c
> +++ b/libmount/src/tab.c
> @@ -1562,7 +1562,10 @@ int mnt_table_is_fs_mounted(struct libmnt_table *tb, struct libmnt_fs *fstab_fs)
>  #endif
>  		}
>
> -		if (root) {
> +		/* For cifs, root contains a relative path to the exported volume,
> +		 * i. e. something we cannot compare.
> +		 */
> +		if (root && strcmp(mnt_fs_get_fstype(fs), "cifs")) {
>  			const char *r = mnt_fs_get_root(fs);
>  			if (!r || strcmp(r, root) != 0)
>  				continue;
>

-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                         e-mail: sbrabec-IBi9RG/b67k@public.gmane.org
Křižíkova 148/34 (Corso IIa)                  tel: +49 911 7405384547
186 00 Praha 8-Karlín                          fax:  +420 284 084 001
Czech Republic                                    http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76

Re: nfs/cifs mountinfo differences (was: Re: [PATCH] libmount: Skip root comparison for cifs in mnt_table_is_fs_mounted())

[Steve French]

patch looks reasonable to me.

On Wed, Aug 3, 2016 at 12:23 PM, Stanislav Brabec <sbrabec-AlSwsSmVLrQ@public.gmane.org> wrote:
> Stanislav Brabec wrote:
>>
>> In mountinfo of cifs filesystem, root is filled with a relative path to
>> the root
>> of the exported volume. There is no reasonable way to compare it without
>> making
>> a network look-up.
>
>
> This is an original problem of "mount -a", see How to reproduce below.
>
>> This causes false negative result in some cases.
>>
>> Skipping root comparison in mnt_table_is_fs_mounted() makes things better,
>> however it still does not cover all possible setups.
>
>
> I just compared behavior of cifs and nfs4 in this situation.
>
> cifs and nfs4 provide different mountinfo. So maybe it is more an
> inconsistency of procinfo root entry than problem of mount.
>
> Sending mail also to linux-cifs to get an opinion.
>
> When mounting subdirectory of exported nfs4 volume, 4th record (root)
> remains "/".
>
> When mounting subdirectory of exported cifs volume, 4th record (root) is the
> "/subdir", and 9th record (source) contains fill path
> "//server/volume/subdir".
>
> And for comparison, when bind mounting cifs subdirectory out of volume
> mount, 4th record is "/subdir" as well, but the string in 9th record
> (source) does not contain trailing "/subdir", just "//server/volume".
>
> And for completness, when bind mounting subdirectory of a btrfs subvolume,
> 4th record (root) contains full path to the subdirectory relative to the
> volume root (i. e. including subvolume path). Content of 9th record (source)
> is not relevant for comparison, as it contains /dev/sd* node.
>
>> How to reproduce:
>> 1) Create and export a cifs volume (with guest allowed).
>> 2) Make a subdirectory in this volume.
>> 3) Mount this subdirectory using fstab:
>>    //server/volume/subdir /mnt cifs guest 0 0
>> 4) Call mount -a twice.
>>    Depending on a system running, you either get:
>>      mount error(16): Device or resource busy
>>      Refer to the mount.cifs(8) manual page (e.g. man mount.cifs)
>>    or you get a volume mounted twice.
>>
>> Signed-off-by: Stanislav Brabec <sbrabec-AlSwsSmVLrQ@public.gmane.org>
>> ---
>
>
> Here is the proposed patch for util-linux:
>
>>  libmount/src/tab.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/libmount/src/tab.c b/libmount/src/tab.c
>> index 155c65e..aa9185a 100644
>> --- a/libmount/src/tab.c
>> +++ b/libmount/src/tab.c
>> @@ -1562,7 +1562,10 @@ int mnt_table_is_fs_mounted(struct libmnt_table
>> *tb, struct libmnt_fs *fstab_fs)
>>  #endif
>>                 }
>>
>> -               if (root) {
>> +               /* For cifs, root contains a relative path to the exported
>> volume,
>> +                * i. e. something we cannot compare.
>> +                */
>> +               if (root && strcmp(mnt_fs_get_fstype(fs), "cifs")) {
>>                         const char *r = mnt_fs_get_root(fs);
>>                         if (!r || strcmp(r, root) != 0)
>>                                 continue;
>>
>
> --
> Best Regards / S pozdravem,
>
> Stanislav Brabec
> software developer
> ---------------------------------------------------------------------
> SUSE LINUX, s. r. o.                         e-mail: sbrabec-IBi9RG/b67k@public.gmane.org
> Křižíkova 148/34 (Corso IIa)                  tel: +49 911 7405384547
> 186 00 Praha 8-Karlín                          fax:  +420 284 084 001
> Czech Republic                                    http://www.suse.cz/
> PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Thanks,

Steve

Re: nfs/cifs mountinfo differences (was: Re: [PATCH] libmount: Skip root comparison for cifs in mnt_table_is_fs_mounted())

[Aurélien Aptel]

[-- Attachment #1: Type: text/plain, Size: 4008 bytes --]

I have looked a bit more into it.

Linux provides filesystems a way to show their own thing in the 4th
column (documented as "root of the mount within the filesystem"). The
presence of this mechanism makes me think this info wasn't made only to
be altered by bind-mounts.

The mechanism in question is the superblock operation 'show_path'. If a
filesystem doesn't provide one, linux default is to walk the dentry
connected to the mount points until it reaches the fs root and print
that. This is what happens in (recent) CIFS.

                                  +------------------+
                                  | cifs super_block |
+------------------+              +------------------+
|rootfs super_block|              |      s_root      |
+------------------+              +------------------+
|      s_root      |                       |
+------------------+                       v
         |                           [ dentry "/" ]
         |                                 |
         |                                 v
         v                           [ dentry "sub" ]
    [dentry "/"]                           |
         v                                 v
   [dentry "mnt"] -----magic------>  [ dentry "dir" ]
                                           |
                                          vvv
                               [ content of "/sub/dir/" ]


CIFS doesn't override the default show_path so mountinfo walks from
"dir" to "/" and prints "/sub/dir".

NFS on the other hand overides the show_path operation to:

int nfs_show_path(struct seq_file *m, struct dentry *dentry)
{
	seq_puts(m, "/");
	return 0;
}

ie. *always* prints "/".

Now correct me if I'm wrong (I really am not sure) but I think 'mount
-a' assumes only bind-mounts should print a subdir path instead of "/".

So either

(A) we make that assumption explicit and consider it the Truth:

- we update the kernel documentation of that column in
  Documentation/filesystems/proc.txt to read something like "path
  binded within the filesystem"
- like NFS, we override show_path to always return "/".
- leave mount utility unchanged.

or (B) we consider the current kernel doc as the Truth:

- we make NFS do like CIFS
- leave CIFS unchanged [1]
- figure out what to do with mount utility (back to initial problem I
  guess)

Personal opinion: the way the current doc is written and again, the fact
that linux lets you override the path on a per-filesystem basis makes me
think solution B is the way to go.

Also, I don't understand why

On Wed, 3 Aug 2016 19:23:06 +0200 Stanislav Brabec <sbrabec-AlSwsSmVLrQ@public.gmane.org>
wrote:
> > Skipping root comparison in mnt_table_is_fs_mounted() makes things
> > better, however it still does not cover all possible setups.

and

> > diff --git a/libmount/src/tab.c b/libmount/src/tab.c
> > index 155c65e..aa9185a 100644
> > --- a/libmount/src/tab.c
> > +++ b/libmount/src/tab.c
> > @@ -1562,7 +1562,10 @@ int mnt_table_is_fs_mounted(struct
> > libmnt_table *tb, struct libmnt_fs *fstab_fs) #endif
> >  		}
> >
> > -		if (root) {
> > +		/* For cifs, root contains a relative path to the
> > exported volume,
> > +		 * i. e. something we cannot compare.
> > +		 */
> > +		if (root && strcmp(mnt_fs_get_fstype(fs), "cifs"))
> > { const char *r = mnt_fs_get_root(fs);
> >  			if (!r || strcmp(r, root) != 0)
> >  				continue;
> >

We can't we compare in this case? Isn't it just a matter of
concatenating the source with the subdir?

1: I believe we should still override show_path in CIFS
   to take recent flag CIFS_USE_PREFIX_PATH into account because the
   default show_path assumes a different dentry structure

-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG
Nürnberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: nfs/cifs mountinfo differences

[Stanislav Brabec]

Aurélien Aptel wrote on Aug 5, 2016 at 7:22 PM:
> Now correct me if I'm wrong (I really am not sure) but I think 'mount
> -a' assumes only bind-mounts should print a subdir path instead of "/".

Yes. It is the only indication that the mountinfo line describes a bind 
mount.

> or (B) we consider the current kernel doc as the Truth:
>
> - we make NFS do like CIFS
> - leave CIFS unchanged [1]
> - figure out what to do with mount utility (back to initial problem I
>   guess)

The proposed patch does the thing.

> Personal opinion: the way the current doc is written and again, the fact
> that linux lets you override the path on a per-filesystem basis makes me
> think solution B is the way to go.
>
> Also, I don't understand why
>
>
> We can't we compare in this case? Isn't it just a matter of
> concatenating the source with the subdir?

Well, it looks possible. The path component 1 is always the server name, 
2 volume name, and the rest is path.

So cut components 3 and up, and remove it from the leading part of 4th 
column. Then you will have a part that can be used for bind mount 
processing.

-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                         e-mail: sbrabec-IBi9RG/b67k@public.gmane.org
Křižíkova 148/34 (Corso IIa)                  tel: +49 911 7405384547
186 00 Praha 8-Karlín                          fax:  +420 284 084 001
Czech Republic                                    http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76

Re: nfs/cifs mountinfo differences

[Stanislav Brabec]

On Aug 16, 2016 at 07:44 PM Stanislav Brabec wrote:
> Aurélien Aptel wrote on Aug 5, 2016 at 7:22 PM:
>> We can't we compare in this case? Isn't it just a matter of
>> concatenating the source with the subdir?
>
> Well, it looks possible. The path component 1 is always the server name,
> 2 volume name, and the rest is path.
>
> So cut components 3 and up, and remove it from the leading part of 4th
> column. Then you will have a part that can be used for bind mount
> processing.
>
I did a new patch that searches the subdir part of the source and cuts
it out from root for purpose of root comparison.

Please review the algorithm of searching of the subdir component in the
source string in the reply to this mail.

-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                         e-mail: sbrabec-IBi9RG/b67k@public.gmane.org
Křižíkova 148/34 (Corso IIa)                  tel: +49 911 7405384547
186 00 Praha 8-Karlín                          fax:  +420 284 084 001
Czech Republic                                    http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76

[PATCH] libmount: Special handling of root comparison for cifs in mnt_table_is_fs_mounted()

[Stanislav Brabec]

In mountinfo of cifs filesystem, leading part of root is a path relative to the
root of the exported volume.

Create a dedicated function mnt_fs_get_cifs_root() that cuts the volume root
part from the root.

How to reproduce:
1) Create and export a volume on cifs (with guest allowed).
2) Make a subdirectory in this volume.
3) Mount this volume using fstab:
   //server/volume/subdir /mnt cifs guest 0 0
4) Call mount -a twice.
   Depending on system running, you either get:
     mount error(16): Device or resource busy
     Refer to the mount.cifs(8) manual page (e.g. man mount.cifs)
   or volume mounted twice.

Signed-off-by: Stanislav Brabec <sbrabec-AlSwsSmVLrQ@public.gmane.org>
---
---
 libmount/src/tab.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)

diff --git a/libmount/src/tab.c b/libmount/src/tab.c
index 341e5e3..4488073 100644
--- a/libmount/src/tab.c
+++ b/libmount/src/tab.c
@@ -42,6 +42,7 @@
  * will return the first entry (if UUID matches with the device).
  */
 #include <blkid.h>
+#include <stdbool.h>
 
 #include "mountP.h"
 #include "strutils.h"
@@ -1449,6 +1450,48 @@ err:
 }
 
 /**
+ * mnt_fs_get_cifs_root:
+ * @fs: /proc/self/mountinfo entry
+ *
+ * For cifs, root contains a relative path to the exported volume plus
+ * the root path. We have to cut volume path to get root.
+ *
+ * Returns: root of the cifs mount within the filesystem or NULL
+ */
+static const char *mnt_fs_get_cifs_root(struct libmnt_fs *fs)
+{
+	const char *root = mnt_fs_get_root(fs);
+	const char *src = mnt_fs_get_source(fs);
+	const char *subpath = src;
+	char c;
+	int component_no = 0;
+	bool last_is_slash = false;
+
+	while ((c = *subpath++))
+	{
+		if (c == '/') {
+			if (!last_is_slash) {
+				component_no++;
+				if (component_no == 3)
+					break;
+			}
+			last_is_slash = true;
+		} else
+			last_is_slash = false;
+	}
+	if (component_no == 3) {
+		int subpath_len = strlen(subpath);
+		if (strncmp(root, subpath, subpath_len)) {
+			if (*(root + subpath_len + 1) == 0)
+				return "/";
+			return root + subpath_len + 1;
+		}
+	}
+	DBG(FS, ul_debugobj(fs, "cifs: leading part of root \"%s\" does not equal to mounted source subdir \"%s\"; should not happen", root, src));
+	return root;
+}
+
+/**
  * mnt_table_is_fs_mounted:
  * @tb: /proc/self/mountinfo file
  * @fstab_fs: /etc/fstab entry
@@ -1563,7 +1606,11 @@ int mnt_table_is_fs_mounted(struct libmnt_table *tb, struct libmnt_fs *fstab_fs)
 		}
 
 		if (root) {
-			const char *r = mnt_fs_get_root(fs);
+			const char *r;
+			if (strcmp(mnt_fs_get_fstype(fs), "cifs"))
+				r = mnt_fs_get_root(fs);
+			else
+				r = mnt_fs_get_cifs_root(fs);
 			if (!r || strcmp(r, root) != 0)
 				continue;
 		}
-- 
2.9.3

-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                         e-mail: sbrabec-IBi9RG/b67k@public.gmane.org
Křižíkova 148/34 (Corso IIa)                  tel: +49 911 7405384547
186 00 Praha 8-Karlín                          fax:  +420 284 084 001
Czech Republic                                    http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76

Re: [PATCH] libmount: Special handling of root comparison for cifs in mnt_table_is_fs_mounted()

[Aurélien Aptel]

[-- Attachment #1.1: Type: text/plain, Size: 1605 bytes --]


Hi Stanislav,

I've looked more into it and I found some issues with the patch:

Stanislav Brabec <sbrabec-AlSwsSmVLrQ@public.gmane.org> writes:
> +	while ((c = *subpath++))
                      ^^^ subpath is always post-incremented
> +	{
> +		if (c == '/') {
> +			if (!last_is_slash) {
> +				component_no++;
> +				if (component_no == 3)
> +					break;
                                         ^^^ when we break subpath
                                             actually 1 past the /
> +			}
> +			last_is_slash = true;
> +		} else
> +			last_is_slash = false;
> +	}
> +	if (component_no == 3) {
> +		int subpath_len = strlen(subpath);
> +		if (strncmp(root, subpath, subpath_len)) {
                             ^^^  ^^^ hence why here
with //localhost/share/dir in fstab we never get here
p root    => $1 = 0x7f28aa9977d0 "/dir"
p subpath => $2 = 0x7f28aa9979a2 "dir"

> +			if (*(root + subpath_len + 1) == 0)
> +				return "/";
> +			return root + subpath_len + 1;
> +		}
> +	}
> +	DBG(FS, ul_debugobj(fs, "cifs: leading part of root \"%s\" does
> not equal to mounted source subdir \"%s\"; should not happen", root,
> src));
^^^ this message gets printed with //localhost/share //localhost/share/ 

That being said, I could not make your patch fail i.e. make mount -a
mount the fs twice. So I'm not sure what happens there... I thought I
would give it a try.

I'm attaching a simpler patch that makes uses of streq_paths() in
lib/strutils.c (Note: streq_paths() and next_path_segment() have to be
backported for older versions of util-linux).


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-libmount-src-tab.c-fix-mount-a-for-cifs.patch --]
[-- Type: text/x-patch, Size: 1955 bytes --]

From a29acba1a7c71cdbfc1cc98f2905bfaf19b97a29 Mon Sep 17 00:00:00 2001
From: Aurelien Aptel <aaptel-IBi9RG/b67k@public.gmane.org>
Date: Thu, 1 Sep 2016 13:57:42 +0200
Subject: [PATCH] libmount/src/tab.c: fix mount -a for cifs

when mounting a cifs share, the src is actually an UNC path which can in
in several forms:

simple:            //host/share, //host/share/
including subpath: //host/share/sub/path

to check if the cifs fs is mounted we have to extract the subpath and
compare *that* to the root.

Signed-off-by: Aurelien Aptel <aaptel-IBi9RG/b67k@public.gmane.org>
---
 libmount/src/tab.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/libmount/src/tab.c b/libmount/src/tab.c
index 341e5e3..9c49ec8 100644
--- a/libmount/src/tab.c
+++ b/libmount/src/tab.c
@@ -1329,6 +1329,20 @@ err:
 }
 #endif /* HAVE_BTRFS_SUPPORT */
 
+static const char *get_cifs_unc_subdir_path (const char *unc)
+{
+	/*
+	 *  1 or more slash:     %*[/]
+	 *  1 or more non-slash: %*[^/]
+	 *  number of byte read: %n
+	 */
+	int share_end = 0;
+	int r = sscanf(unc, "%*[/]%*[^/]%*[/]%*[^/]%n", &share_end);
+	if (r == EOF || share_end == 0)
+		return NULL;
+	return unc + share_end;
+}
+
 /*
  * tb: /proc/self/mountinfo
  * fs: filesystem
@@ -1563,9 +1577,16 @@ int mnt_table_is_fs_mounted(struct libmnt_table *tb, struct libmnt_fs *fstab_fs)
 		}
 
 		if (root) {
-			const char *r = mnt_fs_get_root(fs);
-			if (!r || strcmp(r, root) != 0)
-				continue;
+			if (strcmp(mnt_fs_get_fstype(fs), "cifs") == 0) {
+				const char *unc_subdir = get_cifs_unc_subdir_path(src);
+				const char *path_on_fs = mnt_fs_get_root(fs);
+				if (!unc_subdir || !path_on_fs || !streq_paths(unc_subdir, path_on_fs))
+					continue;
+			} else {
+				const char *r = mnt_fs_get_root(fs);
+				if (!r || strcmp(r, root) != 0)
+					continue;
+			}
 		}
 
 		/*
-- 
2.1.4


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH] libmount: Special handling of root comparison for cifs in mnt_table_is_fs_mounted()

[Karel Zak]

On Fri, Sep 02, 2016 at 08:58:08PM +0200, Aurélien Aptel wrote:
> I'm attaching a simpler patch that makes uses of streq_paths() in
> lib/strutils.c (Note: streq_paths() and next_path_segment() have to be
> backported for older versions of util-linux).
> 
>
> From a29acba1a7c71cdbfc1cc98f2905bfaf19b97a29 Mon Sep 17 00:00:00 2001
> From: Aurelien Aptel <aaptel-IBi9RG/b67k@public.gmane.org>
> Date: Thu, 1 Sep 2016 13:57:42 +0200
> Subject: [PATCH] libmount/src/tab.c: fix mount -a for cifs

 Applied, thanks!

    Karel


-- 
 Karel Zak  <kzak-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
 http://karelzak.blogspot.com