* 2.6.35-rc4: mount results with and without MS_SILENT differ
@ 2011-03-14 4:20 Denys Vlasenko
2011-03-25 3:47 ` Chuck Ebbert
0 siblings, 1 reply; 11+ messages in thread
From: Denys Vlasenko @ 2011-03-14 4:20 UTC (permalink / raw)
To: linux-kernel; +Cc: Alexander Viro, Roman Borisov, Alexander Shishkin
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: 2.6.35-rc4: mount results with and without MS_SILENT differ
2011-03-14 4:20 2.6.35-rc4: mount results with and without MS_SILENT differ Denys Vlasenko
@ 2011-03-25 3:47 ` Chuck Ebbert
2011-03-26 21:43 ` Denys Vlasenko
0 siblings, 1 reply; 11+ messages in thread
From: Chuck Ebbert @ 2011-03-25 3:47 UTC (permalink / raw)
To: Denys Vlasenko
Cc: linux-kernel, Alexander Viro, Roman Borisov, Alexander Shishkin
On Mon, 14 Mar 2011 05:20:08 +0100
Denys Vlasenko <vda.linux@googlemail.com> wrote:
It looks like this bug has been there forever.
This fails:
> mount -vv --make-rshared mount.shared1 2>&1
This succeeds:
> mount -vv --loud --make-rshared mount.shared2 2>&1 # <-HERE
Failure case uses MS_REC | MS_SHARED | MS_SILENT
> mount: mount('','mount.shared1','',0x0010c000,''):0
It succeeds with MS_REC | MS_SHARED
> mount: mount('','mount.shared2','',0x00104000,''):0
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: 2.6.35-rc4: mount results with and without MS_SILENT differ
2011-03-25 3:47 ` Chuck Ebbert
@ 2011-03-26 21:43 ` Denys Vlasenko
2011-04-01 14:48 ` [PATCH] fs: bound mount propagation fix Roman Borisov
0 siblings, 1 reply; 11+ messages in thread
From: Denys Vlasenko @ 2011-03-26 21:43 UTC (permalink / raw)
To: Chuck Ebbert
Cc: linux-kernel, Alexander Viro, Roman Borisov, Alexander Shishkin
On Friday 25 March 2011 04:47, Chuck Ebbert wrote:
> On Mon, 14 Mar 2011 05:20:08 +0100
> Denys Vlasenko <vda.linux@googlemail.com> wrote:
>
> It looks like this bug has been there forever.
>
> This fails:
> > mount -vv --make-rshared mount.shared1 2>&1
>
> This succeeds:
> > mount -vv --loud --make-rshared mount.shared2 2>&1 # <-HERE
>
> Failure case uses MS_REC | MS_SHARED | MS_SILENT
> > mount: mount('','mount.shared1','',0x0010c000,''):0
>
> It succeeds with MS_REC | MS_SHARED
> > mount: mount('','mount.shared2','',0x00104000,''):0
>
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] fs: bound mount propagation fix
2011-03-26 21:43 ` Denys Vlasenko
@ 2011-04-01 14:48 ` Roman Borisov
2011-04-12 10:28 ` Roman Borisov
2011-04-19 21:04 ` Andrew Morton
0 siblings, 2 replies; 11+ messages in thread
From: Roman Borisov @ 2011-04-01 14:48 UTC (permalink / raw)
To: viro; +Cc: linux-kernel, vda.linux, cebbert, virtuoso, Roman Borisov
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).
Signed-off-by: Roman Borisov <ext-roman.borisov@nokia.com>
---
fs/namespace.c | 2 +-
fs/pnode.c | 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/namespace.c b/fs/namespace.c
index 2beb0fb..e0cf263 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -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;
diff --git a/fs/pnode.c b/fs/pnode.c
index 8d5f392..0c9dc54 100644
--- a/fs/pnode.c
+++ b/fs/pnode.c
@@ -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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] fs: bound mount propagation fix
2011-04-01 14:48 ` [PATCH] fs: bound mount propagation fix Roman Borisov
@ 2011-04-12 10:28 ` Roman Borisov
2011-04-19 21:04 ` Andrew Morton
1 sibling, 0 replies; 11+ messages in thread
From: Roman Borisov @ 2011-04-12 10:28 UTC (permalink / raw)
To: viro; +Cc: Roman Borisov, linux-kernel, vda.linux, cebbert, virtuoso
On 04/01/2011 06:48 PM, Roman Borisov wrote:
> 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).
>
> Signed-off-by: Roman Borisov<ext-roman.borisov@nokia.com>
> ---
> fs/namespace.c | 2 +-
> fs/pnode.c | 6 +++---
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 2beb0fb..e0cf263 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -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;
> diff --git a/fs/pnode.c b/fs/pnode.c
> index 8d5f392..0c9dc54 100644
> --- a/fs/pnode.c
> +++ b/fs/pnode.c
> @@ -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;
Al could you comment please on which of the two approaches you prefer:
mine or previously Denis's
--
Roman
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] fs: bound mount propagation fix
2011-04-01 14:48 ` [PATCH] fs: bound mount propagation fix Roman Borisov
2011-04-12 10:28 ` Roman Borisov
@ 2011-04-19 21:04 ` Andrew Morton
2011-04-20 11:51 ` Roman Borisov
1 sibling, 1 reply; 11+ messages in thread
From: Andrew Morton @ 2011-04-19 21:04 UTC (permalink / raw)
To: Roman Borisov; +Cc: viro, linux-kernel, vda.linux, cebbert, virtuoso
On Fri, 1 Apr 2011 18:48:20 +0400
Roman Borisov <ext-roman.borisov@nokia.com> wrote:
> 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).
>
(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.
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 2beb0fb..e0cf263 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -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) {
So this change won't do anything, because do_change_type() has used
flags_to_propagation_type().
> err = invent_group_ids(mnt, recurse);
> if (err)
> goto out_unlock;
> diff --git a/fs/pnode.c b/fs/pnode.c
> index 8d5f392..0c9dc54 100644
> --- a/fs/pnode.c
> +++ b/fs/pnode.c
> @@ -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;
And afaict, no caller of change_mnt_propagation() will pass in a `type'
with more than a single bit set. umount_tree() passed MS_PRIVATE and
do_change_type() uses flags_to_propagation_type().
So as far as I can tell, this patch won't fix anything??
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] fs: bound mount propagation fix
2011-04-19 21:04 ` Andrew Morton
@ 2011-04-20 11:51 ` Roman Borisov
0 siblings, 0 replies; 11+ messages in thread
From: Roman Borisov @ 2011-04-20 11:51 UTC (permalink / raw)
To: ext Andrew Morton; +Cc: viro, linux-kernel, vda.linux, cebbert, virtuoso
On 04/20/2011 01:04 AM, ext Andrew Morton wrote:
> On Fri, 1 Apr 2011 18:48:20 +0400
> Roman Borisov<ext-roman.borisov@nokia.com> wrote:
>
>> 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).
>>
>
> (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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] fs: bound mount propagation fix
[not found] <cover.1303304011.git.ext-roman.borisov@nokia.com>
@ 2011-04-20 14:11 ` Roman Borisov
2011-04-21 20:04 ` [PATCH v3] " Andrew Morton
0 siblings, 1 reply; 11+ messages in thread
From: Roman Borisov @ 2011-04-20 14:11 UTC (permalink / raw)
To: linux-kernel; +Cc: linux-fsdevel, viro, Roman Borisov
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 <vda.linux@googlemail.com>
Signed-off-by: Roman Borisov <ext-roman.borisov@nokia.com>
---
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3] fs: bound mount propagation fix
2011-04-20 14:11 ` [PATCH v2] " Roman Borisov
@ 2011-04-21 20:04 ` Andrew Morton
2011-04-22 9:02 ` Roman Borisov
2011-04-22 11:05 ` [PATCH v4] fs: namespacec " Roman Borisov
0 siblings, 2 replies; 11+ messages in thread
From: Andrew Morton @ 2011-04-21 20:04 UTC (permalink / raw)
To: Roman Borisov; +Cc: viro, vda.linux, linux-kernel
On Thu, 21 Apr 2011 16:37:28 +0400
Roman Borisov <ext-roman.borisov@nokia.com> 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 <vda.linux@googlemail.com>
>
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 <ext-roman.borisov@nokia.com>
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 <ext-roman.borisov@nokia.com>
Reported-by: Denys Vlasenko <vda.linux@googlemail.com>
Cc: Chuck Ebbert <cebbert@redhat.com>
Cc: Alexander Shishkin <virtuoso@slind.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
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))
_
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] fs: bound mount propagation fix
2011-04-21 20:04 ` [PATCH v3] " Andrew Morton
@ 2011-04-22 9:02 ` Roman Borisov
2011-04-22 11:05 ` [PATCH v4] fs: namespacec " Roman Borisov
1 sibling, 0 replies; 11+ messages in thread
From: Roman Borisov @ 2011-04-22 9:02 UTC (permalink / raw)
To: ext Andrew Morton; +Cc: viro, vda.linux, linux-kernel
On 04/22/2011 12:04 AM, ext Andrew Morton wrote:
> On Thu, 21 Apr 2011 16:37:28 +0400
> Roman Borisov<ext-roman.borisov@nokia.com> 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<vda.linux@googlemail.com>
>>
>
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v4] fs: namespacec bound mount propagation fix
2011-04-21 20:04 ` [PATCH v3] " Andrew Morton
2011-04-22 9:02 ` Roman Borisov
@ 2011-04-22 11:05 ` Roman Borisov
1 sibling, 0 replies; 11+ messages in thread
From: Roman Borisov @ 2011-04-22 11:05 UTC (permalink / raw)
To: akpm; +Cc: viro, vda.linux, linux-kernel, Roman Borisov
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 <ext-roman.borisov@nokia.com>
---
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2011-04-22 11:04 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-03-14 4:20 2.6.35-rc4: mount results with and without MS_SILENT differ Denys Vlasenko
2011-03-25 3:47 ` Chuck Ebbert
2011-03-26 21:43 ` Denys Vlasenko
2011-04-01 14:48 ` [PATCH] fs: bound mount propagation fix Roman Borisov
2011-04-12 10:28 ` Roman Borisov
2011-04-19 21:04 ` Andrew Morton
2011-04-20 11:51 ` Roman Borisov
[not found] <cover.1303304011.git.ext-roman.borisov@nokia.com>
2011-04-20 14:11 ` [PATCH v2] " Roman Borisov
2011-04-21 20:04 ` [PATCH v3] " Andrew Morton
2011-04-22 9:02 ` Roman Borisov
2011-04-22 11:05 ` [PATCH v4] fs: namespacec " Roman Borisov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).