2.6.35-rc4: mount results with and without MS_SILENT differ

This issue was discovered by users of busybox. Apparently, mount() calls with and without MS_SILENT

The following script was run in an empty test directory:

mkdir -p mount.dir mount.shared1 mount.shared2 touch mount.dir/a mount.dir/b
mount -vv --bind mount.shared1 mount.shared1
mount -vv --make-rshared mount.shared1
mount -vv --bind mount.shared2 mount.shared2
mount -vv --make-rshared mount.shared2
mount -vv --bind mount.shared2 mount.shared1
mount -vv --bind mount.dir mount.shared2
ls -R mount.dir mount.shared1 mount.shared2 umount mount.dir mount.shared1 mount.shared2 2>/dev/null umount mount.dir mount.shared1 mount.shared2 2>/dev/null umount mount.dir mount.shared1 mount.shared2 2>/dev/null rm -f mount.dir/a mount.dir/b mount.dir/c
rmdir mount.dir mount.shared1 mount.shared2

mount -vv was used to show the mount() call arguments and result. Output shows that flag argument has 0x00008000 = MS_SILENT bit:

mount: mount('mount.shared1','mount.shared1','(null)',0x00009000,'(null)'):0
mount: mount('','mount.shared1','',0x0010c000,''):0
mount: mount('mount.shared2','mount.shared2','(null)',0x00009000,'(null)'):0
mount: mount('','mount.shared2','',0x0010c000,''):0
mount: mount('mount.shared2','mount.shared1','(null)',0x00009000,'(null)'):0
mount: mount('mount.dir','mount.shared2','(null)',0x00009000,'(null)'):0 mount.dir:
a
b

mount.shared1:

mount.shared2:
a
b

After adding --loud option to remove MS_SILENT bit from just one mount cmd:

mkdir -p mount.dir mount.shared1 mount.shared2 touch mount.dir/a mount.dir/b
mount -vv --bind mount.shared1 mount.shared1 2>&1
mount -vv --make-rshared mount.shared1 2>&1
mount -vv --bind mount.shared2 mount.shared2 2>&1
mount -vv --loud --make-rshared mount.shared2 2>&1 # <-HERE
mount -vv --bind mount.shared2 mount.shared1 2>&1
mount -vv --bind mount.dir mount.shared2 2>&1
ls -R mount.dir mount.shared1 mount.shared2 2>&1
umount mount.dir mount.shared1 mount.shared2 2>/dev/null umount mount.dir mount.shared1 mount.shared2 2>/dev/null umount mount.dir mount.shared1 mount.shared2 2>/dev/null rm -f mount.dir/a mount.dir/b mount.dir/c
rmdir mount.dir mount.shared1 mount.shared2

The result is different now - look closely at mount.shared1 directory listing. Now it does show files 'a' and 'b':

mount: mount('mount.shared1','mount.shared1','(null)',0x00009000,'(null)'):0
mount: mount('','mount.shared1','',0x0010c000,''):0
mount: mount('mount.shared2','mount.shared2','(null)',0x00009000,'(null)'):0
mount: mount('','mount.shared2','',0x00104000,''):0
mount: mount('mount.shared2','mount.shared1','(null)',0x00009000,'(null)'):0
mount: mount('mount.dir','mount.shared2','(null)',0x00009000,'(null)'):0

mount.dir:
a
b

mount.shared1:
a
b

mount.shared2:
a
b

I am not asking whether mount command should or should not use MS_SILENT bit. It's an important question, but it doesn't belong to lkml.

My question is, intuitively, MS_SILENT should only affect (suppress) kernel messages, it should never affect the outcome of the mount() call, right?

Here it is obviously not the case - the behavior is different. Is it a bug?

vda

On Mon, 14 Mar 2011 05:20:08 +0100
Denys Vlasenko <[email protected]> wrote:

It looks like this bug has been there forever.

This fails:

This succeeds:

Failure case uses MS_REC | MS_SHARED | MS_SILENT

It succeeds with MS_REC | MS_SHARED

Looking at do_mount() we see some flags get filtered out before checking namespace flags:

flags &= ~(MS_NOSUID | MS_NOEXEC | MS_NODEV | MS_ACTIVE | MS_BORN |
MS_NOATIME | MS_NODIRATIME | MS_RELATIME| MS_KERNMOUNT | MS_STRICTATIME);

Then:

else if (flags & (MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE))
retval = do_change_type(&path, flags);

In do_change_type()

        type = flags_to_propagation_type(flag);
        if (!type)
                return -EINVAL;

And flags_to_propagation_type() filters out MS_REC before making sure one and only one of the propagation type flags is set:

        int type = flags & ~MS_REC;

        /* Fail if any non-propagation flags are set */
        if (type & ~(MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE))
                return 0;
        /* Only one propagation flag should be set */
        if (!is_power_of_2(type))
                return 0;

Looks like the is_power_of_2() test is failing because MS_SILENT is set. I'm not sure whether to filter MS_SILENT in the upper or lower function though.

We cannot slear out MS_SILENT here:

flags &= ~(MS_NOSUID | MS_NOEXEC | MS_NODEV | MS_ACTIVE | MS_BORN |
MS_NOATIME | MS_NODIRATIME | MS_RELATIME| MS_KERNMOUNT | MS_STRICTATIME);

Because...

if (flags & MS_REMOUNT)
retval = do_remount(&path, flags & ~MS_REMOUNT, mnt_flags,
data_page);
else if (flags & MS_BIND)
retval = do_loopback(&path, dev_name, flags & MS_REC);
else if (flags & (MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE))
retval = do_change_type(&path, flags);
else if (flags & MS_MOVE)
retval = do_move_mount(&path, dev_name);
else
...it needs to propagate into:
retval = do_new_mount(&path, type_page, flags, mnt_flags,
dev_name, data_page);

I think MS_SILENT can be cleared along with MS_REC in do_change_type() -> flags_to_propagation_type(), on this line:

- int type = flags & ~MS_REC;
+ int type = flags & ~(MS_REC | MS_SILENT);

vda

I think MS_SILENT shouldn't be cleared anywhere. I suppose the bug is in MS_SHARED options checking. Please see the patch below.

Fixed MS_SHARED, MS_SLAVE, MS_UNBINDABLE option handling; Existing options check doesn't allow to have any options combinations because of integer comparison (not bitwise).

---
 fs/namespace.c |    2 +-
 fs/pnode.c     |    6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

index 2beb0fb..e0cf263 100644
@@ -1434,7 +1434,7 @@ static int do_change_type(struct path *path, int flag)
 		return -EINVAL;
 
 	down_write(&namespace_sem);
-	if (type == MS_SHARED) {
+	if (type & MS_SHARED) {
 		err = invent_group_ids(mnt, recurse);
 		if (err)
 			goto out_unlock;

index 8d5f392..0c9dc54 100644
@@ -128,15 +128,15 @@ static int do_make_slave(struct vfsmount *mnt)
 
 void change_mnt_propagation(struct vfsmount *mnt, int type)
 {
-	if (type == MS_SHARED) {
+	if (type & MS_SHARED) {
 		set_mnt_shared(mnt);
 		return;
 	}
 	do_make_slave(mnt);
-	if (type != MS_SLAVE) {
+	if (!(type & MS_SLAVE)) {
 		list_del_init(&mnt->mnt_slave);
 		mnt->mnt_master = NULL;
-		if (type == MS_UNBINDABLE)
+		if (type & MS_UNBINDABLE)
 			mnt->mnt_flags |= MNT_UNBINDABLE;
 		else
 			mnt->mnt_flags &= ~MNT_UNBINDABLE;

1.7.0.4

(when fixing a bug, please include a *complete* description of that bug in the changelog. It should include a description of the user-visible misbehaviour and a description of the coding error).

The vfs code is pretty confusing about whether `type' is supposed to be a scalar or a bitfield.

flags_to_propagation_type() has that is_power_of-two() check in there to reject more-than-one-bit-set.

Thanks for comment.
I tested the patch on too old kernel which doesn't contain is_power_of_2 checking in do_change_type. I'll post patch_v2 recently.

--
Roman

MS_SILENT flag cleaning up added to flags_to_propagation_type function. It was reported that bound mount propagation doesn't work in busybox as by default busybox mount applet sets the MS_SILENT flag for any mount operation. Moreover recently added flags_to_propagation_type function doesn't allow to do such operations as --make-[r]private --make-[r]shared etc. when MS_SILENT is on.
The idea to clean MS_SILENT flag belongs to Denys Vlasenko <[email protected]>

---
 fs/namespace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

index 60813f0..f219060 100644
@@ -1423,7 +1423,7 @@ out_unlock:
 
 static int flags_to_propagation_type(int flags)
 {
-	int type = flags & ~MS_REC;
+	int type = flags & ~(MS_REC | MS_SILENT);
 
 	/* Fail if any non-propagation flags are set */
 	if (type & ~(MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE))

1.7.0.4

This is not an adequate changelog, IMO. It has almost no description of the errant user-visible kernel behaviour and almost no description of what was wrong with the kernel code - ie, why the kernel was doing the wrong thing.

I've pieced together a fix for the first problem but then got lazy. Please can someone provide an analysis of what was causing this bug?

And has this v3 patch been tested with Denys's reproducer?

From: Roman Borisov <[email protected]>

This issue was discovered by users of busybox. Apparently, mount is called with and without MS_SILENT, and this affects mount() behaviour. But MS_SILENT is only supposed to affect kernel logging verbosity.

The following script was run in an empty test directory:

mkdir -p mount.dir mount.shared1 mount.shared2 touch mount.dir/a mount.dir/b
mount -vv --bind mount.shared1 mount.shared1
mount -vv --make-rshared mount.shared1
mount -vv --bind mount.shared2 mount.shared2
mount -vv --make-rshared mount.shared2
mount -vv --bind mount.shared2 mount.shared1
mount -vv --bind mount.dir mount.shared2
ls -R mount.dir mount.shared1 mount.shared2 umount mount.dir mount.shared1 mount.shared2 2>/dev/null umount mount.dir mount.shared1 mount.shared2 2>/dev/null umount mount.dir mount.shared1 mount.shared2 2>/dev/null rm -f mount.dir/a mount.dir/b mount.dir/c
rmdir mount.dir mount.shared1 mount.shared2

mount -vv was used to show the mount() call arguments and result. Output shows that flag argument has 0x00008000 = MS_SILENT bit:

mount: mount('mount.shared1','mount.shared1','(null)',0x00009000,'(null)'):0
mount: mount('','mount.shared1','',0x0010c000,''):0
mount: mount('mount.shared2','mount.shared2','(null)',0x00009000,'(null)'):0
mount: mount('','mount.shared2','',0x0010c000,''):0
mount: mount('mount.shared2','mount.shared1','(null)',0x00009000,'(null)'):0
mount: mount('mount.dir','mount.shared2','(null)',0x00009000,'(null)'):0 mount.dir:
a
b

mount.shared1:

mount.shared2:
a
b

After adding --loud option to remove MS_SILENT bit from just one mount cmd:

mkdir -p mount.dir mount.shared1 mount.shared2 touch mount.dir/a mount.dir/b
mount -vv --bind mount.shared1 mount.shared1 2>&1
mount -vv --make-rshared mount.shared1 2>&1
mount -vv --bind mount.shared2 mount.shared2 2>&1
mount -vv --loud --make-rshared mount.shared2 2>&1 # <-HERE
mount -vv --bind mount.shared2 mount.shared1 2>&1
mount -vv --bind mount.dir mount.shared2 2>&1
ls -R mount.dir mount.shared1 mount.shared2 2>&1
umount mount.dir mount.shared1 mount.shared2 2>/dev/null umount mount.dir mount.shared1 mount.shared2 2>/dev/null umount mount.dir mount.shared1 mount.shared2 2>/dev/null rm -f mount.dir/a mount.dir/b mount.dir/c
rmdir mount.dir mount.shared1 mount.shared2

The result is different now - look closely at mount.shared1 directory listing. Now it does show files 'a' and 'b':

mount: mount('mount.shared1','mount.shared1','(null)',0x00009000,'(null)'):0
mount: mount('','mount.shared1','',0x0010c000,''):0
mount: mount('mount.shared2','mount.shared2','(null)',0x00009000,'(null)'):0
mount: mount('','mount.shared2','',0x00104000,''):0
mount: mount('mount.shared2','mount.shared1','(null)',0x00009000,'(null)'):0
mount: mount('mount.dir','mount.shared2','(null)',0x00009000,'(null)'):0

mount.dir:
a
b

mount.shared1:
a
b

mount.shared2:
a
b

Moreover, the recently added flags_to_propagation_type() function doesn't allow us to do such operations as --make-[r]private --make-[r]shared etc. when MS_SILENT is on. The idea or clearing the MS_SILENT flag came from to Denys Vlasenko.

---

 fs/namespace.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff -puN fs/namespace.c~fs-bound-mount-propagation-fix fs/namespace.c
@@ -1695,7 +1695,7 @@ static int graft_tree(struct vfsmount *m
 
 static int flags_to_propagation_type(int flags)
 {
-	int type = flags & ~MS_REC;
+	int type = flags & ~(MS_REC | MS_SILENT);
 
 	/* Fail if any non-propagation flags are set */
 	if (type & ~(MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE))

_

Hello,

I understand that description I've sent is not so clear for end-user. But there are two issues:
1. This is busybox related issue; only in busybox MS_SILENT is on for all mount operations; I don't know how to reproduce it in util-linux->mount without hardcoding;
2. initial scenario didn't consider "VFS: Sanity check mount flags passed to change_mnt_propagation()" patch (commit 7a2e8a8faab76386d8eaae9ded739ee5615be174);

Previously we used such script:

# mkdir -p mount.dir mount.shared1 mount.shared2
# touch mount.dir/a mount.dir/b
# mount -vv --bind mount.shared1 mount.shared1
# mount -vv --make-rshared mount.shared1
# mount -vv --bind mount.shared2 mount.shared2
# mount -vv --make-rshared mount.shared2
# mount -vv --bind mount.shared2 mount.shared1
# mount -vv --bind mount.dir     mount.shared2
# ls -R mount.dir mount.shared1 mount.shared2
mount.dir:
a  b
mount.shared1:
mount.shared2:
a  b
#

but expected result is:

# ls -R mount.dir mount.shared1 mount.shared2
mount.dir:
a  b
mount.shared1:
a  b
mount.shared2:
a  b
#

My "[PATCH] fs: bound mount propagation fix" fixed this issue but as I said I didn't consider Valerie's patch;

After I've applied the "VFS: Sanity check mount flags passed to change_mnt_propagation()" patch I have *another wrong result*:

# mkdir -p mount.dir mount.shared1 mount.shared2
# touch mount.dir/a mount.dir/b
# mount -vv --bind mount.shared1 mount.shared1
# mount -vv --make-rshared mount.shared1
mount: mount.shared1: Invalid argument
#

Now the debugging shows that MS_SILENT came to flags_to_propagation_type() and causes the error return after the is_power_of_2 checking; clearing the MS_SILENT flag fixes this issue.

Yes I've applied Valerie's and my patches and tested the scenario in busybox environment; the verdict is passed; I'll post [PATCH v4] with full description today.

--
Roman

This issue was discovered by users of busybox. And the bug is actual for busybox users, I don't know how it affects others. Apparently, mount is called with and without MS_SILENT, and this affects mount() behaviour. But MS_SILENT is only supposed to affect kernel logging verbosity.

There is the script to reproduce the issue in busybox environment:
mkdir -p mount.dir mount.shared1 mount.shared2
touch mount.dir/a mount.dir/b
mount -vv --bind mount.shared1 mount.shared1
mount -vv --make-rshared mount.shared1
mount -vv --bind mount.shared2 mount.shared2
mount -vv --make-rshared mount.shared2
mount -vv --bind mount.shared2 mount.shared1
mount -vv --bind mount.dir mount.shared2
ls -R mount.dir mount.shared1 mount.shared2

actual resul after the fourth command is error:
mount: mount.shared1: Invalid argument

but expected result is:
mount.dir:
a b
mount.shared1:
a b
mount.shared2:
a b

The analysis shows that MS_SILENT flag which is ON by default in any busybox-> mount operations cames to flags_to_propagation_type function and causes the error return while is_power_of_2 checking because the function expects only one bit set. This doesn't allow to do busybox->mount with
any --make-[r]shared, --make-[r]private etc options.

---
 fs/namespace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

index 7dba2ed..e7c479e 100644
@@ -1711,7 +1711,7 @@ static int graft_tree(struct vfsmount *mnt, struct path *path)
 
 static int flags_to_propagation_type(int flags)
 {
-	int type = flags & ~MS_REC;
+	int type = flags & ~(MS_REC | MS_SILENT);
 
 	/* Fail if any non-propagation flags are set */
 	if (type & ~(MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE))

1.7.0.4