[PATCH 0/7] mount_setattr fixes

[PATCH 0/7] mount_setattr fixes

[Christian Brauner]

Hey,

This contains a couple of minor fixes for the mount_setattr() code I
didn't get around to sending so far. Apart from making the control flow
a little easier to follow there's a fix for a long-standing braino in
one of the idmapped mount kernel selftests. The kernel selftests are
separate from the large fstests suite and are only concerned with
testing the syscall itself whereas the fstests test all related vfs and
filesystem specific functionality for idmapped mounts. I would also like
to add a maintenance entry for idmapped mounts for the few files and
documentation that I've written.
I'll be gone for ~10 days starting next week but I'll try to check-in
regularly.

Thanks!
Christian

Christian Brauner (7):
  tests: fix idmapped mount_setattr test
  MAINTAINERS: add entry for idmapped mounts
  fs: add kernel doc for mnt_{hold,unhold}_writers()
  fs: add mnt_allow_writers() and simplify mount_setattr_prepare()
  fs: simplify check in mount_setattr_commit()
  fs: don't open-code mnt_hold_writers()
  fs: clean up mount_setattr control flow

 MAINTAINERS                                   |   9 ++
 fs/namespace.c                                | 148 +++++++++++-------
 .../mount_setattr/mount_setattr_test.c        |   4 +-
 3 files changed, 105 insertions(+), 56 deletions(-)


base-commit: 26291c54e111ff6ba87a164d85d4a4e134b7315c
-- 
2.32.0

[PATCH 1/7] tests: fix idmapped mount_setattr test

[Christian Brauner]

The test treated zero as a successful run when it really should treat
non-zero as a successful run. A mount's idmapping can't change once it
has been attached to the filesystem.

Fixes: 01eadc8dd96d ("tests: add mount_setattr() selftests")
Cc: Seth Forshee <seth.forshee@digitalocean.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 tools/testing/selftests/mount_setattr/mount_setattr_test.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/mount_setattr/mount_setattr_test.c b/tools/testing/selftests/mount_setattr/mount_setattr_test.c
index f31205f04ee0..8c5fea68ae67 100644
--- a/tools/testing/selftests/mount_setattr/mount_setattr_test.c
+++ b/tools/testing/selftests/mount_setattr/mount_setattr_test.c
@@ -1236,7 +1236,7 @@ static int get_userns_fd(unsigned long nsid, unsigned long hostid, unsigned long
 }
 
 /**
- * Validate that an attached mount in our mount namespace can be idmapped.
+ * Validate that an attached mount in our mount namespace cannot be idmapped.
  * (The kernel enforces that the mount's mount namespace and the caller's mount
  *  namespace match.)
  */
@@ -1259,7 +1259,7 @@ TEST_F(mount_setattr_idmapped, attached_mount_inside_current_mount_namespace)
 
 	attr.userns_fd	= get_userns_fd(0, 10000, 10000);
 	ASSERT_GE(attr.userns_fd, 0);
-	ASSERT_EQ(sys_mount_setattr(open_tree_fd, "", AT_EMPTY_PATH, &attr, sizeof(attr)), 0);
+	ASSERT_NE(sys_mount_setattr(open_tree_fd, "", AT_EMPTY_PATH, &attr, sizeof(attr)), 0);
 	ASSERT_EQ(close(attr.userns_fd), 0);
 	ASSERT_EQ(close(open_tree_fd), 0);
 }
-- 
2.32.0

Re: [PATCH 1/7] tests: fix idmapped mount_setattr test

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 2/7] MAINTAINERS: add entry for idmapped mounts

[Christian Brauner]

I'd like to continue maintaining the work that was done around idmapped,
make sure that I'm Cced on new patches and work that impacts the
infrastructure.

Cc: Seth Forshee <seth.forshee@digitalocean.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 MAINTAINERS | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index f41088418aae..0496b973bb87 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9253,6 +9253,15 @@ S:	Maintained
 W:	https://github.com/o2genum/ideapad-slidebar
 F:	drivers/input/misc/ideapad_slidebar.c
 
+IDMAPPED MOUNTS
+M:	Christian Brauner <brauner@kernel.org>
+L:	linux-fsdevel@vger.kernel.org
+S:	Maintained
+T:	git git://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git
+F:	Documentation/filesystems/idmappings.rst
+F:	tools/testing/selftests/mount_setattr/
+F:	include/linux/mnt_idmapping.h
+
 IDT VersaClock 5 CLOCK DRIVER
 M:	Luca Ceresoli <luca@lucaceresoli.net>
 S:	Maintained
-- 
2.32.0

Re: [PATCH 2/7] MAINTAINERS: add entry for idmapped mounts

[Christoph Hellwig]

On Thu, Feb 03, 2022 at 02:14:06PM +0100, Christian Brauner wrote:
> I'd like to continue maintaining the work that was done around idmapped,
> make sure that I'm Cced on new patches and work that impacts the
> infrastructure.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 3/7] fs: add kernel doc for mnt_{hold,unhold}_writers()

[Christian Brauner]

When I introduced mnt_{hold,unhold}_writers() in commit fbdc2f6c40f6
("fs: split out functions to hold writers") I did not add kernel doc for
them. Fix this and introduce proper documentation.

Fixes: commit fbdc2f6c40f6 ("fs: split out functions to hold writers")
Cc: Seth Forshee <seth.forshee@digitalocean.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/namespace.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/fs/namespace.c b/fs/namespace.c
index 40b994a29e90..de6fae84f1a1 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -469,6 +469,24 @@ void mnt_drop_write_file(struct file *file)
 }
 EXPORT_SYMBOL(mnt_drop_write_file);
 
+/**
+ * mnt_hold_writers - prevent write access to the given mount
+ * @mnt: mnt to prevent write access to
+ *
+ * Prevents write access to @mnt if there are no active writers for @mnt.
+ * This function needs to be called and return successfully before changing
+ * properties of @mnt that need to remain stable for callers with write access
+ * to @mnt.
+ *
+ * After this functions has been called successfully callers must pair it with
+ * a call to mnt_unhold_writers() in order to stop preventing write access to
+ * @mnt.
+ *
+ * Context: This function expects lock_mount_hash() to be held serializing
+ *          setting MNT_WRITE_HOLD.
+ * Return: On success 0 is returned.
+ *	   On error, -EBUSY is returned.
+ */
 static inline int mnt_hold_writers(struct mount *mnt)
 {
 	mnt->mnt.mnt_flags |= MNT_WRITE_HOLD;
@@ -500,6 +518,18 @@ static inline int mnt_hold_writers(struct mount *mnt)
 	return 0;
 }
 
+/**
+ * mnt_unhold_writers - stop preventing write access to the given mount
+ * @mnt: mnt to stop preventing write access to
+ *
+ * Stop preventing write access to @mnt allowing callers to gain write access
+ * to @mnt again.
+ *
+ * This function can only be called after a successful call to
+ * mnt_hold_writers().
+ *
+ * Context: This function expects lock_mount_hash() to be held.
+ */
 static inline void mnt_unhold_writers(struct mount *mnt)
 {
 	/*
-- 
2.32.0

Re: [PATCH 3/7] fs: add kernel doc for mnt_{hold,unhold}_writers()

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 4/7] fs: add mnt_allow_writers() and simplify mount_setattr_prepare()

[Christian Brauner]

Add a tiny helper that lets us simplify the control-flow and can be used
in the next patch to avoid adding another condition open-coded into
mount_setattr_prepare(). Instead we can add it into the new helper.

Cc: Seth Forshee <seth.forshee@digitalocean.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/namespace.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index de6fae84f1a1..7e5535ed155d 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -3998,6 +3998,22 @@ static int can_idmap_mount(const struct mount_kattr *kattr, struct mount *mnt)
 	return 0;
 }
 
+/**
+ * mnt_allow_writers() - check whether the attribute change allows writers
+ * @kattr: the new mount attributes
+ * @mnt: the mount to which @kattr will be applied
+ *
+ * Check whether thew new mount attributes in @kattr allow concurrent writers.
+ *
+ * Return: true if writers need to be held, false if not
+ */
+static inline bool mnt_allow_writers(const struct mount_kattr *kattr,
+				     const struct mount *mnt)
+{
+	return !(kattr->attr_set & MNT_READONLY) ||
+	       (mnt->mnt.mnt_flags & MNT_READONLY);
+}
+
 static struct mount *mount_setattr_prepare(struct mount_kattr *kattr,
 					   struct mount *mnt, int *err)
 {
@@ -4028,12 +4044,12 @@ static struct mount *mount_setattr_prepare(struct mount_kattr *kattr,
 
 		last = m;
 
-		if ((kattr->attr_set & MNT_READONLY) &&
-		    !(m->mnt.mnt_flags & MNT_READONLY)) {
-			*err = mnt_hold_writers(m);
-			if (*err)
-				goto out;
-		}
+		if (mnt_allow_writers(kattr, m))
+			continue;
+
+		*err = mnt_hold_writers(m);
+		if (*err)
+			goto out;
 	} while (kattr->recurse && (m = next_mnt(m, mnt)));
 
 out:
-- 
2.32.0

Re: [PATCH 4/7] fs: add mnt_allow_writers() and simplify mount_setattr_prepare()

[Christoph Hellwig]

On Thu, Feb 03, 2022 at 02:14:08PM +0100, Christian Brauner wrote:
> Add a tiny helper that lets us simplify the control-flow and can be used
> in the next patch to avoid adding another condition open-coded into
> mount_setattr_prepare(). Instead we can add it into the new helper.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 5/7] fs: simplify check in mount_setattr_commit()

[Christian Brauner]

In order to determine whether we need to call mnt_unhold_writers() in
mount_setattr_commit() we currently do not just check whether
MNT_WRITE_HOLD is set but also if a read-only mount was requested.

However, checking whether MNT_WRITE_HOLD is set is enough. Setting
MNT_WRITE_HOLD requires lock_mount_hash() to be held and it must be
unset before calling unlock_mount_hash(). This guarantees that if we see
MNT_WRITE_HOLD we know that we were the ones who set it earlier. We
don't need to care about why we set it. Plus, leaving this additional
read-only check in makes the code more confusing because it implies that
MNT_WRITE_HOLD could've been set by another thread when it really can't.
Remove it and update the associated comment.

Cc: Seth Forshee <seth.forshee@digitalocean.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/namespace.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 7e5535ed155d..ddae5c08ea8c 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -4096,13 +4096,8 @@ static void mount_setattr_commit(struct mount_kattr *kattr,
 			WRITE_ONCE(m->mnt.mnt_flags, flags);
 		}
 
-		/*
-		 * We either set MNT_READONLY above so make it visible
-		 * before ~MNT_WRITE_HOLD or we failed to recursively
-		 * apply mount options.
-		 */
-		if ((kattr->attr_set & MNT_READONLY) &&
-		    (m->mnt.mnt_flags & MNT_WRITE_HOLD))
+		/* If we had to hold writers unblock them. */
+		if (m->mnt.mnt_flags & MNT_WRITE_HOLD)
 			mnt_unhold_writers(m);
 
 		if (!err && kattr->propagation)
-- 
2.32.0

Re: [PATCH 5/7] fs: simplify check in mount_setattr_commit()

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 6/7] fs: don't open-code mnt_hold_writers()

[Christian Brauner]

Remove sb_prepare_remount_readonly()'s open-coded mnt_hold_writers()
implementation with the real helper we introduced in commit fbdc2f6c40f6
("fs: split out functions to hold writers").

Cc: Seth Forshee <seth.forshee@digitalocean.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/namespace.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index ddae5c08ea8c..00762f9a736a 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -563,12 +563,9 @@ int sb_prepare_remount_readonly(struct super_block *sb)
 	lock_mount_hash();
 	list_for_each_entry(mnt, &sb->s_mounts, mnt_instance) {
 		if (!(mnt->mnt.mnt_flags & MNT_READONLY)) {
-			mnt->mnt.mnt_flags |= MNT_WRITE_HOLD;
-			smp_mb();
-			if (mnt_get_writers(mnt) > 0) {
-				err = -EBUSY;
+			err = mnt_hold_writers(mnt);
+			if (err)
 				break;
-			}
 		}
 	}
 	if (!err && atomic_long_read(&sb->s_remove_count))
-- 
2.32.0

Re: [PATCH 6/7] fs: don't open-code mnt_hold_writers()

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 7/7] fs: clean up mount_setattr control flow

[Christian Brauner]

Simplify the control flow of mount_setattr_{prepare,commit} so they
became easiert to follow. We kept using both an integer error variable
that was passed by pointer as well as a pointer as an indicator for
whether or not we need to revert or commit the prepared changes.
Simplify this and just use the pointer. If we successfully changed
properties the revert pointer will be NULL and if we failed to change
properties it will indicate where we failed and thus need to stop
reverting.

Cc: Seth Forshee <seth.forshee@digitalocean.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/namespace.c | 84 ++++++++++++++++++++++++++------------------------
 1 file changed, 43 insertions(+), 41 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 00762f9a736a..0e342e2ade83 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -82,6 +82,7 @@ struct mount_kattr {
 	unsigned int lookup_flags;
 	bool recurse;
 	struct user_namespace *mnt_userns;
+	struct mount *revert;
 };
 
 /* /sys/fs */
@@ -4011,46 +4012,34 @@ static inline bool mnt_allow_writers(const struct mount_kattr *kattr,
 	       (mnt->mnt.mnt_flags & MNT_READONLY);
 }
 
-static struct mount *mount_setattr_prepare(struct mount_kattr *kattr,
-					   struct mount *mnt, int *err)
+static int mount_setattr_prepare(struct mount_kattr *kattr, struct mount *mnt)
 {
-	struct mount *m = mnt, *last = NULL;
-
-	if (!is_mounted(&m->mnt)) {
-		*err = -EINVAL;
-		goto out;
-	}
-
-	if (!(mnt_has_parent(m) ? check_mnt(m) : is_anon_ns(m->mnt_ns))) {
-		*err = -EINVAL;
-		goto out;
-	}
+	struct mount *m = mnt;
 
 	do {
+		int err = -EPERM;
 		unsigned int flags;
 
-		flags = recalc_flags(kattr, m);
-		if (!can_change_locked_flags(m, flags)) {
-			*err = -EPERM;
-			goto out;
-		}
+		kattr->revert = m;
 
-		*err = can_idmap_mount(kattr, m);
-		if (*err)
-			goto out;
+		flags = recalc_flags(kattr, m);
+		if (!can_change_locked_flags(m, flags))
+			return err;
 
-		last = m;
+		err = can_idmap_mount(kattr, m);
+		if (err)
+			return err;
 
 		if (mnt_allow_writers(kattr, m))
 			continue;
 
-		*err = mnt_hold_writers(m);
-		if (*err)
-			goto out;
+		err = mnt_hold_writers(m);
+		if (err)
+			return err;
 	} while (kattr->recurse && (m = next_mnt(m, mnt)));
 
-out:
-	return last;
+	kattr->revert = NULL;
+	return 0;
 }
 
 static void do_idmap_mount(const struct mount_kattr *kattr, struct mount *mnt)
@@ -4078,14 +4067,12 @@ static void do_idmap_mount(const struct mount_kattr *kattr, struct mount *mnt)
 		put_user_ns(old_mnt_userns);
 }
 
-static void mount_setattr_commit(struct mount_kattr *kattr,
-				 struct mount *mnt, struct mount *last,
-				 int err)
+static void mount_setattr_finish(struct mount_kattr *kattr, struct mount *mnt)
 {
 	struct mount *m = mnt;
 
 	do {
-		if (!err) {
+		if (!kattr->revert) {
 			unsigned int flags;
 
 			do_idmap_mount(kattr, m);
@@ -4097,24 +4084,24 @@ static void mount_setattr_commit(struct mount_kattr *kattr,
 		if (m->mnt.mnt_flags & MNT_WRITE_HOLD)
 			mnt_unhold_writers(m);
 
-		if (!err && kattr->propagation)
+		if (!kattr->revert && kattr->propagation)
 			change_mnt_propagation(m, kattr->propagation);
 
 		/*
 		 * On failure, only cleanup until we found the first mount
 		 * we failed to handle.
 		 */
-		if (err && m == last)
-			break;
+		if (kattr->revert == m)
+			return;
 	} while (kattr->recurse && (m = next_mnt(m, mnt)));
 
-	if (!err)
+	if (!kattr->revert)
 		touch_mnt_namespace(mnt->mnt_ns);
 }
 
 static int do_mount_setattr(struct path *path, struct mount_kattr *kattr)
 {
-	struct mount *mnt = real_mount(path->mnt), *last = NULL;
+	struct mount *mnt = real_mount(path->mnt);
 	int err = 0;
 
 	if (path->dentry != mnt->mnt.mnt_root)
@@ -4135,16 +4122,31 @@ static int do_mount_setattr(struct path *path, struct mount_kattr *kattr)
 		}
 	}
 
+	err = -EINVAL;
 	lock_mount_hash();
 
+	/* Ensure that this isn't anything purely vfs internal. */
+	if (!is_mounted(&mnt->mnt))
+		goto out;
+
 	/*
-	 * Get the mount tree in a shape where we can change mount
-	 * properties without failure.
+	 * If this is an attached mount make sure it's located in the callers
+	 * mount namespace. If it's not don't let the caller interact with it.
+	 * If this is a detached mount make sure it has an anonymous mount
+	 * namespace attached to it, i.e. we've created it via OPEN_TREE_CLONE.
 	 */
-	last = mount_setattr_prepare(kattr, mnt, &err);
-	if (last) /* Commit all changes or revert to the old state. */
-		mount_setattr_commit(kattr, mnt, last, err);
+	if (!(mnt_has_parent(mnt) ? check_mnt(mnt) : is_anon_ns(mnt->mnt_ns)))
+		goto out;
 
+	/*
+	 * First, we get the mount tree in a shape where we can change mount
+	 * properties without failure. If we succeeded to do so we commit all
+	 * changes and if we failed we clean up.
+	 */
+	err = mount_setattr_prepare(kattr, mnt);
+	mount_setattr_finish(kattr, mnt);
+
+out:
 	unlock_mount_hash();
 
 	if (kattr->propagation) {
-- 
2.32.0

Re: [PATCH 7/7] fs: clean up mount_setattr control flow

[Christoph Hellwig]

On Thu, Feb 03, 2022 at 02:14:11PM +0100, Christian Brauner wrote:
> Simplify the control flow of mount_setattr_{prepare,commit} so they
> became easiert to follow. We kept using both an integer error variable

s/became easiert/become easier/g

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>