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]>
Signed-off-by: Roman Borisov <[email protected]>
---
fs/namespace.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/fs/namespace.c b/fs/namespace.c
index 60813f0..f219060 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -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
On Thu, 21 Apr 2011 16:37:28 +0400
Roman Borisov <[email protected]> wrote:
> 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]>
>
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.
Signed-off-by: Roman Borisov <[email protected]>
Reported-by: Denys Vlasenko <[email protected]>
Cc: Chuck Ebbert <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
---
fs/namespace.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff -puN fs/namespace.c~fs-bound-mount-propagation-fix fs/namespace.c
--- a/fs/namespace.c~fs-bound-mount-propagation-fix
+++ a/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))
_
On 04/22/2011 12:04 AM, ext Andrew Morton wrote:
> On Thu, 21 Apr 2011 16:37:28 +0400
> Roman Borisov<[email protected]> wrote:
>
>> 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]>
>>
>
> 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.
>
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);
> I've pieced together a fix for the first problem but then got lazy.
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
#
> Please can someone provide an analysis of what was causing this bug?
>
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.
> And has this v3 patch been tested with Denys's reproducer?
>
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.
Signed-off-by: Roman Borisov <[email protected]>
---
fs/namespace.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/fs/namespace.c b/fs/namespace.c
index 7dba2ed..e7c479e 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -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