do_change_type() is buggy when passed multiple MS_* flags.  Discovered
because mount(8) incorrectly adds MS_RDONLY flag to
shared/slave/private/unbindable mounts.  Karel Zak will fix the
mount(8) bug shortly.

A test program is attached.  Against Viro's #untested branch.

-VAL

commit 208ca52f69ea53cf0723b8492fe54ebf9a3bf36a
Author: Valerie Aurora <vaurora@redhat.com>
Date:   Thu Aug 26 11:07:22 2010 -0700

    VFS: Sanity check mount flags passed to change_mnt_propagation()
    
    Sanity check the flags passed to change_mnt_propagation().  Exactly
    one flag should be set.  Return EINVAL otherwise.
    
    Userspace can pass in arbitrary combinations of MS_* flags to mount().
    do_change_type() is called if any of MS_SHARED, MS_PRIVATE, MS_SLAVE,
    or MS_UNBINDABLE is set.  do_change_type() clears MS_REC and then
    calls change_mnt_propagation() with the rest of the user-supplied
    flags.  change_mnt_propagation() clearly assumes only one flag is set
    but do_change_type() does not check that this is true.  For example,
    mount() with flags MS_SHARED | MS_RDONLY does not actually make the
    mount shared or read-only but does clear MNT_UNBINDABLE.
    
    Signed-off-by: Valerie Aurora <vaurora@redhat.com>

diff --git a/fs/namespace.c b/fs/namespace.c
index de402eb..4987c4c 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1484,13 +1484,32 @@ out_unlock:
 }
 
 /*
+ * Sanity check the flags to change_mnt_propagation.
+ */
+
+static int flags_to_propagation_type(int flags) {
+	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 (((type & (MS_SHARED)) && (type & ~MS_SHARED)) ||
+	    ((type & (MS_PRIVATE)) && (type & ~MS_PRIVATE)) ||
+	    ((type & (MS_SLAVE)) && (type & ~MS_SLAVE)) ||
+	    ((type & (MS_UNBINDABLE)) && (type & ~MS_UNBINDABLE)))
+		return 0;
+	return ...
[ message continues ]

Hrm.  I think we can do this a bit more pithily.

	/* Only one propagation flag should be set, and no others */
	if (hweight32(type) != 1 &&
	    (type & ~(MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE))
		return 0;

Too clever?

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."
--

[ message continues ]

I was hoping someone would go find the best bitop for me, thanks. :)
hweight32() is an awkward name but the comment makes it clear.  I'm
happy with either.

Thanks for the help,

-VAL
--

[ message continues ]

Didn't read surrounding code, but is that supposed to be '||'?

Otherwise the case where only a single non-propagation flag is
set no longer returns 0...

-- 
Bob Copeland %% www.bobcopeland.com
--

[ message continues ]

Yes, thanks!

I'll run the test program again before resubmitting. :)

-VAL
--

[ message continues ]

Val's original code returned 0 as failure.  So a single non-propagation
flag set shouldn't return 0.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."
--

[ message continues ]

Not to belabor the point -- but her original code did return zero in
that case (2nd assertion fails):

#include <linux/fs.h>
#include <stdio.h>
#include <assert.h>

#define hweight32 dumb_hweight32

int dumb_hweight32(int a)
{
	int i;
	int weight = 0;

	for (i=0; i < 32; i++)
		weight += !!(a & (1 << i));

	return weight;
}

static int flags_to_propagation_type(int flags) {
	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 (((type & (MS_SHARED)) && (type & ~MS_SHARED)) ||
	   ((type & (MS_PRIVATE)) && (type & ~MS_PRIVATE)) ||
	   ((type & (MS_SLAVE)) && (type & ~MS_SLAVE)) ||
	   ((type & (MS_UNBINDABLE)) && (type & ~MS_UNBINDABLE)))
	       return 0;
	return type;
}

static int flags_to_propagation_type_2(int flags) {
 	int type = flags & ~MS_REC;

	/* Only one propagation flag should be set, and no others */
	if (hweight32(type) != 1 &&
	    (type & ~(MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE)))
		return 0;

	return type;
}

int main(int argc, char *argv[])
{
	int mytype = MS_RDONLY;

	assert(flags_to_propagation_type(mytype) == 0);
	assert(flags_to_propagation_type_2(mytype) == 0);
}

-- 
Bob Copeland %% www.bobcopeland.com

--

[ message continues ]

Guys, stop with "teh crazy".

What the f*ck kind of expression is that? We don't do this kind of
crap. Even if it's possible that the compiler might be able to
optimize it, it's just crazy to call "hweight32()" for something like
this.

Please think about what you're really after for a second, and realize
that "one bit set" is just another way of saying "power of two". And
then sit back, relax, and realize that there are way better ways to
say "is this is a power of two" than counting bits, for chrissake!

Just a simple "is_power_of_2()" would work. But for anybody who cares
about how it works, here's how to see if there is just a single bit
set:

   n & (n-1)

and then zero is a special case. _Why_ it works is left as an exercise
for the reader, because it's an interesting trick that becomes obvious
once you start thinking about how borrowing works in binary
arithmetic, and what that "subtract one" does for the borrow case for
a power-of-two value vs a non-power-of-two. You can also think about
why zero is a special case, and why you might sometimes consider it an

Not clever at all, I'm afraid.

                     Linus
--

[ message continues ]

I considered is_power_of_2() initially but rejected it as not
particularly readable.  But hey, this is VFS code!

-VAL

commit ce3708ab514d850b0d0939f3fa6c64daad306a15
Author: Valerie Aurora <vaurora@redhat.com>
Date:   Thu Aug 26 11:07:22 2010 -0700

    VFS: Sanity check mount flags passed to change_mnt_propagation()
    
    Sanity check the flags passed to change_mnt_propagation().  Exactly
    one flag should be set.  Return EINVAL otherwise.
    
    Userspace can pass in arbitrary combinations of MS_* flags to mount().
    do_change_type() is called if any of MS_SHARED, MS_PRIVATE, MS_SLAVE,
    or MS_UNBINDABLE is set.  do_change_type() clears MS_REC and then
    calls change_mnt_propagation() with the rest of the user-supplied
    flags.  change_mnt_propagation() clearly assumes only one flag is set
    but do_change_type() does not check that this is true.  For example,
    mount() with flags MS_SHARED | MS_RDONLY does not actually make the
    mount shared or read-only but does clear MNT_UNBINDABLE.
    
    Signed-off-by: Valerie Aurora <vaurora@redhat.com>

diff --git a/fs/namespace.c b/fs/namespace.c
index de402eb..ddc5565 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1484,13 +1484,29 @@ out_unlock:
 }
 
 /*
+ * Sanity check the flags to change_mnt_propagation.
+ */
+
+static int flags_to_propagation_type(int flags) {
+	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;
+	return type;
+}
+
+/*
  * recursively change the type of the mountpoint.
  */
 static int do_change_type(struct path *path, int flag)
 {
 	struct vfsmount *m, *mnt = path->mnt;
 	int recurse = flag & MS_REC;
-	int type = flag & ~MS_REC;
+	int type;
 	int err = 0;
 
 	if (!capable(CAP_SYS_ADMIN))
@@ -1499,6 +1515,10 @@ static int do_change_type(struct path *path, int ...
[ message continues ]

Fixed, thanks for your bug report.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com
--

[ message continues ]