[PATCH v2 0/1] vfs: Respect MS_RDONLY at bind mount creation

[PATCH v2 0/1] vfs: Respect MS_RDONLY at bind mount creation

[Richard Yao]

The first version of this patch had a few issues:

1. It marked mount points readonly after graft_tree().
2. The commit message did not read very well.
3. It failed to make it to the LKML archives.
4. One person absent from the CC list asked to be included after I sent it.

Issue #1 merits a v2, so I am sending it again with some minor corrections. I
had considered splitting the mnt_make_readonly() loop into its own helper
function for readability, but that turned out to make the code less readable.

Richard Yao (1):
  vfs: Respect MS_RDONLY at bind mount creation

 fs/namespace.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

-- 
1.8.5.5

[PATCH v2 1/1] vfs: Respect MS_RDONLY at bind mount creation

[Richard Yao]

`mount -o bind,ro ...` suffers from a silent failure where the readonly
flag is ignored. The bind mount will be created rw whenever the target
is rw. Users typically workaround this by remounting readonly, but that
does not work when you want to define readonly bind mounts in fstab.
This is a major annoyance when dealing with recursive bind mounts
because the userland mount command does not expose the option to
recursively remount a subtree as readonly.

Signed-off-by: Richard Yao <ryao@gentoo.org>
---
 fs/namespace.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 182bc41..0d23525 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1827,11 +1827,12 @@ static bool has_locked_children(struct mount *mnt, struct dentry *dentry)
  * do loopback mount.
  */
 static int do_loopback(struct path *path, const char *old_name,
-				int recurse)
+				unsigned long flags)
 {
 	struct path old_path;
-	struct mount *mnt = NULL, *old, *parent;
+	struct mount *mnt = NULL, *old, *parent, *m;
 	struct mountpoint *mp;
+	int recurse = flags & MS_REC;
 	int err;
 	if (!old_name || !*old_name)
 		return -EINVAL;
@@ -1871,6 +1872,10 @@ static int do_loopback(struct path *path, const char *old_name,
 		goto out2;
 	}
 
+	if (flags & MS_RDONLY)
+		for (m = mnt; m; m = (recurse ? next_mnt(m, mnt) : NULL))
+			mnt_make_readonly(m);
+
 	mnt->mnt.mnt_flags &= ~MNT_LOCKED;
 
 	err = graft_tree(mnt, parent, mp);
@@ -2444,7 +2449,8 @@ long do_mount(const char *dev_name, const char *dir_name,
 		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);
+		retval = do_loopback(&path, dev_name, flags & (MS_REC |
+							       MS_RDONLY));
 	else if (flags & (MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE))
 		retval = do_change_type(&path, flags);
 	else if (flags & MS_MOVE)
-- 
1.8.5.5

Re: [PATCH v2 1/1] vfs: Respect MS_RDONLY at bind mount creation

[Mateusz Guzik]

On Fri, Aug 01, 2014 at 02:12:24PM -0400, Richard Yao wrote:
> `mount -o bind,ro ...` suffers from a silent failure where the readonly
> flag is ignored. The bind mount will be created rw whenever the target
> is rw. Users typically workaround this by remounting readonly, but that
> does not work when you want to define readonly bind mounts in fstab.
> This is a major annoyance when dealing with recursive bind mounts
> because the userland mount command does not expose the option to
> recursively remount a subtree as readonly.
> 
> Signed-off-by: Richard Yao <ryao@gentoo.org>
> ---
>  fs/namespace.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 182bc41..0d23525 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -1827,11 +1827,12 @@ static bool has_locked_children(struct mount *mnt, struct dentry *dentry)
>   * do loopback mount.
>   */
>  static int do_loopback(struct path *path, const char *old_name,
> -				int recurse)
> +				unsigned long flags)
>  {
>  	struct path old_path;
> -	struct mount *mnt = NULL, *old, *parent;
> +	struct mount *mnt = NULL, *old, *parent, *m;
>  	struct mountpoint *mp;
> +	int recurse = flags & MS_REC;
>  	int err;
>  	if (!old_name || !*old_name)
>  		return -EINVAL;
> @@ -1871,6 +1872,10 @@ static int do_loopback(struct path *path, const char *old_name,
>  		goto out2;
>  	}
>  
> +	if (flags & MS_RDONLY)
> +		for (m = mnt; m; m = (recurse ? next_mnt(m, mnt) : NULL))
> +			mnt_make_readonly(m);
> +
>  	mnt->mnt.mnt_flags &= ~MNT_LOCKED;
>  
>  	err = graft_tree(mnt, parent, mp);
> @@ -2444,7 +2449,8 @@ long do_mount(const char *dev_name, const char *dir_name,
>  		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);
> +		retval = do_loopback(&path, dev_name, flags & (MS_REC |
> +							       MS_RDONLY));
>  	else if (flags & (MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE))
>  		retval = do_change_type(&path, flags);
>  	else if (flags & MS_MOVE)

I don't really know this code, but have to ask.

Would not it be much better to pass down info about rdonly request to
copy_tree/clone_mnt (perhaps CL_MOUNT_RDONLY flag or a separate flags
argument) and handle it there?

This would avoid fishy-looking traversal before graft_tree, which even
if correct should not be necessary.

-- 
Mateusz Guzik

Re: [PATCH v2 1/1] vfs: Respect MS_RDONLY at bind mount creation

[Richard Yao]

[-- Attachment #1: Type: text/plain, Size: 2840 bytes --]

On 08/01/2014 03:20 PM, Mateusz Guzik wrote:
> On Fri, Aug 01, 2014 at 02:12:24PM -0400, Richard Yao wrote:
>> `mount -o bind,ro ...` suffers from a silent failure where the readonly
>> flag is ignored. The bind mount will be created rw whenever the target
>> is rw. Users typically workaround this by remounting readonly, but that
>> does not work when you want to define readonly bind mounts in fstab.
>> This is a major annoyance when dealing with recursive bind mounts
>> because the userland mount command does not expose the option to
>> recursively remount a subtree as readonly.
>>
>> Signed-off-by: Richard Yao <ryao@gentoo.org>
>> ---
>>  fs/namespace.c | 12 +++++++++---
>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/namespace.c b/fs/namespace.c
>> index 182bc41..0d23525 100644
>> --- a/fs/namespace.c
>> +++ b/fs/namespace.c
>> @@ -1827,11 +1827,12 @@ static bool has_locked_children(struct mount *mnt, struct dentry *dentry)
>>   * do loopback mount.
>>   */
>>  static int do_loopback(struct path *path, const char *old_name,
>> -				int recurse)
>> +				unsigned long flags)
>>  {
>>  	struct path old_path;
>> -	struct mount *mnt = NULL, *old, *parent;
>> +	struct mount *mnt = NULL, *old, *parent, *m;
>>  	struct mountpoint *mp;
>> +	int recurse = flags & MS_REC;
>>  	int err;
>>  	if (!old_name || !*old_name)
>>  		return -EINVAL;
>> @@ -1871,6 +1872,10 @@ static int do_loopback(struct path *path, const char *old_name,
>>  		goto out2;
>>  	}
>>  
>> +	if (flags & MS_RDONLY)
>> +		for (m = mnt; m; m = (recurse ? next_mnt(m, mnt) : NULL))
>> +			mnt_make_readonly(m);
>> +
>>  	mnt->mnt.mnt_flags &= ~MNT_LOCKED;
>>  
>>  	err = graft_tree(mnt, parent, mp);
>> @@ -2444,7 +2449,8 @@ long do_mount(const char *dev_name, const char *dir_name,
>>  		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);
>> +		retval = do_loopback(&path, dev_name, flags & (MS_REC |
>> +							       MS_RDONLY));
>>  	else if (flags & (MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE))
>>  		retval = do_change_type(&path, flags);
>>  	else if (flags & MS_MOVE)
> 
> I don't really know this code, but have to ask.
> 
> Would not it be much better to pass down info about rdonly request to
> copy_tree/clone_mnt (perhaps CL_MOUNT_RDONLY flag or a separate flags
> argument) and handle it there?
> 
> This would avoid fishy-looking traversal before graft_tree, which even
> if correct should not be necessary.
> 

I have a nagging feeling that people doing backports will hate me for
adding a power of 2 plus 1 bit, but your suggestion makes this more
readable. I will send v3 after I finish my regression tests.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

[PATCH v3 0/1] vfs: Respect MS_RDONLY at bind mount creation

[Richard Yao]

This revision introduces CL_MAKE_RDONLY to propagate the request to make things
readonly all the way to clone_mnt() as suggested by Mateusz Guzik.

Richard Yao (1):
  vfs: Respect MS_RDONLY at bind mount creation

 fs/namespace.c | 15 +++++++++++----
 fs/pnode.h     | 17 +++++++++--------
 2 files changed, 20 insertions(+), 12 deletions(-)

-- 
1.8.5.5

[PATCH v3 1/1] vfs: Respect MS_RDONLY at bind mount creation

[Richard Yao]

`mount -o bind,ro ...` suffers from a silent failure where the readonly
flag is ignored. The bind mount will be created rw whenever the target
is rw. Users typically workaround this by remounting readonly, but that
does not work when you want to define readonly bind mounts in fstab.
This is a major annoyance when dealing with recursive bind mounts
because the userland mount command does not expose the option to
recursively remount a subtree as readonly.

Signed-off-by: Richard Yao <ryao@gentoo.org>
---
 fs/namespace.c | 15 +++++++++++----
 fs/pnode.h     | 17 +++++++++--------
 2 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 182bc41..6b3a566 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -921,6 +921,9 @@ static struct mount *clone_mnt(struct mount *old, struct dentry *root,
 	if (flag & CL_MAKE_SHARED)
 		set_mnt_shared(mnt);
 
+	if (flag & CL_MAKE_RDONLY)
+		mnt_make_readonly(mnt);
+
 	/* stick the duplicate mount on the same expiry list
 	 * as the original if that was on one */
 	if (flag & CL_EXPIRE) {
@@ -1827,11 +1830,13 @@ static bool has_locked_children(struct mount *mnt, struct dentry *dentry)
  * do loopback mount.
  */
 static int do_loopback(struct path *path, const char *old_name,
-				int recurse)
+				unsigned long flags)
 {
 	struct path old_path;
 	struct mount *mnt = NULL, *old, *parent;
 	struct mountpoint *mp;
+	int recurse = flags & MS_REC;
+	int clflags = (flags & MS_RDONLY) ? CL_MAKE_RDONLY : 0;
 	int err;
 	if (!old_name || !*old_name)
 		return -EINVAL;
@@ -1862,9 +1867,10 @@ static int do_loopback(struct path *path, const char *old_name,
 		goto out2;
 
 	if (recurse)
-		mnt = copy_tree(old, old_path.dentry, CL_COPY_MNT_NS_FILE);
+		mnt = copy_tree(old, old_path.dentry, CL_COPY_MNT_NS_FILE |
+			clflags);
 	else
-		mnt = clone_mnt(old, old_path.dentry, 0);
+		mnt = clone_mnt(old, old_path.dentry, clflags);
 
 	if (IS_ERR(mnt)) {
 		err = PTR_ERR(mnt);
@@ -2444,7 +2450,8 @@ long do_mount(const char *dev_name, const char *dir_name,
 		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);
+		retval = do_loopback(&path, dev_name, flags & (MS_REC |
+							       MS_RDONLY));
 	else if (flags & (MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE))
 		retval = do_change_type(&path, flags);
 	else if (flags & MS_MOVE)
diff --git a/fs/pnode.h b/fs/pnode.h
index 4a24635..7d32196 100644
--- a/fs/pnode.h
+++ b/fs/pnode.h
@@ -20,14 +20,15 @@
 #define SET_MNT_MARK(m) ((m)->mnt.mnt_flags |= MNT_MARKED)
 #define CLEAR_MNT_MARK(m) ((m)->mnt.mnt_flags &= ~MNT_MARKED)
 
-#define CL_EXPIRE    		0x01
-#define CL_SLAVE     		0x02
-#define CL_COPY_UNBINDABLE	0x04
-#define CL_MAKE_SHARED 		0x08
-#define CL_PRIVATE 		0x10
-#define CL_SHARED_TO_SLAVE	0x20
-#define CL_UNPRIVILEGED		0x40
-#define CL_COPY_MNT_NS_FILE	0x80
+#define CL_EXPIRE    		0x001
+#define CL_SLAVE     		0x002
+#define CL_COPY_UNBINDABLE	0x004
+#define CL_MAKE_SHARED 		0x008
+#define CL_PRIVATE 		0x010
+#define CL_SHARED_TO_SLAVE	0x020
+#define CL_UNPRIVILEGED		0x040
+#define CL_COPY_MNT_NS_FILE	0x080
+#define CL_MAKE_RDONLY 		0x100
 
 #define CL_COPY_ALL		(CL_COPY_UNBINDABLE | CL_COPY_MNT_NS_FILE)
 
-- 
1.8.5.5

Re: [PATCH v3 1/1] vfs: Respect MS_RDONLY at bind mount creation

[Mateusz Guzik]

On Fri, Aug 01, 2014 at 11:55:12PM -0400, Richard Yao wrote:
> `mount -o bind,ro ...` suffers from a silent failure where the readonly
> flag is ignored. The bind mount will be created rw whenever the target
> is rw. Users typically workaround this by remounting readonly, but that
> does not work when you want to define readonly bind mounts in fstab.
> This is a major annoyance when dealing with recursive bind mounts
> because the userland mount command does not expose the option to
> recursively remount a subtree as readonly.
> 
> Signed-off-by: Richard Yao <ryao@gentoo.org>
> ---
>  fs/namespace.c | 15 +++++++++++----
>  fs/pnode.h     | 17 +++++++++--------
>  2 files changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 182bc41..6b3a566 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -921,6 +921,9 @@ static struct mount *clone_mnt(struct mount *old, struct dentry *root,
>  	if (flag & CL_MAKE_SHARED)
>  		set_mnt_shared(mnt);
>  
> +	if (flag & CL_MAKE_RDONLY)
> +		mnt_make_readonly(mnt);
> +

Is not this more heavyweight than necessary?

i.e. would not mnt->mnt.mnt_flags |= MNT_READONLY suffice?

Since this mount point is not attached anywhere there cannot be any
writers to synchronize against (and if there could be some, you would
have to check error code from mnt_make_readonly).

That said, apart from this thingy and cosmetic issues looks good to me,
but note I'm just some random guy commenting on a patch.

>  	/* stick the duplicate mount on the same expiry list
>  	 * as the original if that was on one */
>  	if (flag & CL_EXPIRE) {
> @@ -1827,11 +1830,13 @@ static bool has_locked_children(struct mount *mnt, struct dentry *dentry)
>   * do loopback mount.
>   */
>  static int do_loopback(struct path *path, const char *old_name,
> -				int recurse)
> +				unsigned long flags)
>  {
>  	struct path old_path;
>  	struct mount *mnt = NULL, *old, *parent;
>  	struct mountpoint *mp;
> +	int recurse = flags & MS_REC;
> +	int clflags = (flags & MS_RDONLY) ? CL_MAKE_RDONLY : 0;
>  	int err;
>  	if (!old_name || !*old_name)
>  		return -EINVAL;
> @@ -1862,9 +1867,10 @@ static int do_loopback(struct path *path, const char *old_name,
>  		goto out2;
>  
>  	if (recurse)
> -		mnt = copy_tree(old, old_path.dentry, CL_COPY_MNT_NS_FILE);
> +		mnt = copy_tree(old, old_path.dentry, CL_COPY_MNT_NS_FILE |
> +			clflags);
>  	else
> -		mnt = clone_mnt(old, old_path.dentry, 0);
> +		mnt = clone_mnt(old, old_path.dentry, clflags);
>  
>  	if (IS_ERR(mnt)) {
>  		err = PTR_ERR(mnt);
> @@ -2444,7 +2450,8 @@ long do_mount(const char *dev_name, const char *dir_name,
>  		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);
> +		retval = do_loopback(&path, dev_name, flags & (MS_REC |
> +							       MS_RDONLY));
>  	else if (flags & (MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE))
>  		retval = do_change_type(&path, flags);
>  	else if (flags & MS_MOVE)
> diff --git a/fs/pnode.h b/fs/pnode.h
> index 4a24635..7d32196 100644
> --- a/fs/pnode.h
> +++ b/fs/pnode.h
> @@ -20,14 +20,15 @@
>  #define SET_MNT_MARK(m) ((m)->mnt.mnt_flags |= MNT_MARKED)
>  #define CLEAR_MNT_MARK(m) ((m)->mnt.mnt_flags &= ~MNT_MARKED)
>  
> -#define CL_EXPIRE    		0x01
> -#define CL_SLAVE     		0x02
> -#define CL_COPY_UNBINDABLE	0x04
> -#define CL_MAKE_SHARED 		0x08
> -#define CL_PRIVATE 		0x10
> -#define CL_SHARED_TO_SLAVE	0x20
> -#define CL_UNPRIVILEGED		0x40
> -#define CL_COPY_MNT_NS_FILE	0x80
> +#define CL_EXPIRE    		0x001
> +#define CL_SLAVE     		0x002
> +#define CL_COPY_UNBINDABLE	0x004
> +#define CL_MAKE_SHARED 		0x008
> +#define CL_PRIVATE 		0x010
> +#define CL_SHARED_TO_SLAVE	0x020
> +#define CL_UNPRIVILEGED		0x040
> +#define CL_COPY_MNT_NS_FILE	0x080
> +#define CL_MAKE_RDONLY 		0x100
>  
>  #define CL_COPY_ALL		(CL_COPY_UNBINDABLE | CL_COPY_MNT_NS_FILE)
>  
> -- 
> 1.8.5.5
> 

-- 
Mateusz Guzik