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:
> 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.
On Friday 25 March 2011 04:47, Chuck Ebbert wrote:
> 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:
> > 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
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 <[email protected]>
---
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
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<[email protected]>
> ---
> 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
On Fri, 1 Apr 2011 18:48:20 +0400
Roman Borisov <[email protected]> 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??
On 04/20/2011 01:04 AM, ext Andrew Morton wrote:
> On Fri, 1 Apr 2011 18:48:20 +0400
> Roman Borisov<[email protected]> 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