From a26f788b6e7a10d193c4cc6889818e4d625e9461 Mon Sep 17 00:00:00 2001 From: Christian Brauner Date: Thu, 3 Feb 2022 14:14:08 +0100 Subject: fs: add mnt_allow_writers() and simplify mount_setattr_prepare() 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. Link: https://lore.kernel.org/r/20220203131411.3093040-5-brauner@kernel.org Cc: Seth Forshee Cc: Christoph Hellwig Cc: Al Viro Cc: linux-fsdevel@vger.kernel.org Reviewed-by: Christoph Hellwig Signed-off-by: Christian Brauner --- fs/namespace.c | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) (limited to 'fs/namespace.c') 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: -- cgit v1.2.3-70-g09d2 From 03b6abee9ba67c20c4e5253e1a347d8c26edc511 Mon Sep 17 00:00:00 2001 From: Christian Brauner Date: Thu, 3 Feb 2022 14:14:09 +0100 Subject: fs: simplify check in mount_setattr_commit() 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. Link: https://lore.kernel.org/r/20220203131411.3093040-6-brauner@kernel.org Cc: Seth Forshee Cc: Christoph Hellwig Cc: Al Viro Cc: linux-fsdevel@vger.kernel.org Reviewed-by: Christoph Hellwig Signed-off-by: Christian Brauner --- fs/namespace.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) (limited to 'fs/namespace.c') 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) -- cgit v1.2.3-70-g09d2 From ad1844a0127af8fbb87d3d7019907260daf6466b Mon Sep 17 00:00:00 2001 From: Christian Brauner Date: Thu, 3 Feb 2022 14:14:10 +0100 Subject: fs: don't open-code mnt_hold_writers() 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"). Link: https://lore.kernel.org/r/20220203131411.3093040-7-brauner@kernel.org Cc: Seth Forshee Cc: Christoph Hellwig Cc: Al Viro Cc: linux-fsdevel@vger.kernel.org Reviewed-by: Christoph Hellwig Signed-off-by: Christian Brauner --- fs/namespace.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) (limited to 'fs/namespace.c') 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)) -- cgit v1.2.3-70-g09d2 From 87bb5b60019c60e1f902e6885734cc4e5135c2d9 Mon Sep 17 00:00:00 2001 From: Christian Brauner Date: Thu, 3 Feb 2022 14:14:11 +0100 Subject: fs: clean up mount_setattr control flow Simplify the control flow of mount_setattr_{prepare,commit} so they become easier 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. Link: https://lore.kernel.org/r/20220203131411.3093040-8-brauner@kernel.org Cc: Seth Forshee Cc: Christoph Hellwig Cc: Al Viro Cc: linux-fsdevel@vger.kernel.org Reviewed-by: Christoph Hellwig Signed-off-by: Christian Brauner --- fs/namespace.c | 84 ++++++++++++++++++++++++++++++---------------------------- 1 file changed, 43 insertions(+), 41 deletions(-) (limited to 'fs/namespace.c') 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) { -- cgit v1.2.3-70-g09d2 From e257039f0fc7da36ac3a522ef9a5cb4ae7852e67 Mon Sep 17 00:00:00 2001 From: Al Viro Date: Mon, 28 Feb 2022 23:04:20 -0500 Subject: mount_setattr(): clean the control flow and calling conventions separate the "cleanup" and "apply" codepaths (they have almost no overlap), fold the "cleanup" into "prepare" (which eliminates the need of ->revert) and make loops more idiomatic. Signed-off-by: Al Viro --- fs/namespace.c | 82 ++++++++++++++++++++++++++++------------------------------ 1 file changed, 40 insertions(+), 42 deletions(-) (limited to 'fs/namespace.c') diff --git a/fs/namespace.c b/fs/namespace.c index 0e342e2ade83..7017c85ea3e6 100644 --- a/fs/namespace.c +++ b/fs/namespace.c @@ -82,7 +82,6 @@ struct mount_kattr { unsigned int lookup_flags; bool recurse; struct user_namespace *mnt_userns; - struct mount *revert; }; /* /sys/fs */ @@ -4014,32 +4013,39 @@ static inline bool mnt_allow_writers(const struct mount_kattr *kattr, static int mount_setattr_prepare(struct mount_kattr *kattr, struct mount *mnt) { - struct mount *m = mnt; - - do { - int err = -EPERM; - unsigned int flags; - - kattr->revert = m; + struct mount *m; + int err; - flags = recalc_flags(kattr, m); - if (!can_change_locked_flags(m, flags)) - return err; + for (m = mnt; m; m = next_mnt(m, mnt)) { + if (!can_change_locked_flags(m, recalc_flags(kattr, m))) { + err = -EPERM; + break; + } err = can_idmap_mount(kattr, m); if (err) - return err; + break; - if (mnt_allow_writers(kattr, m)) - continue; + if (!mnt_allow_writers(kattr, m)) { + err = mnt_hold_writers(m); + if (err) + break; + } - err = mnt_hold_writers(m); - if (err) - return err; - } while (kattr->recurse && (m = next_mnt(m, mnt))); + if (!kattr->recurse) + return 0; + } - kattr->revert = NULL; - return 0; + if (err) { + struct mount *p; + + for (p = mnt; p != m; p = next_mnt(p, mnt)) { + /* If we had to hold writers unblock them. */ + if (p->mnt.mnt_flags & MNT_WRITE_HOLD) + mnt_unhold_writers(p); + } + } + return err; } static void do_idmap_mount(const struct mount_kattr *kattr, struct mount *mnt) @@ -4067,36 +4073,27 @@ static void do_idmap_mount(const struct mount_kattr *kattr, struct mount *mnt) put_user_ns(old_mnt_userns); } -static void mount_setattr_finish(struct mount_kattr *kattr, struct mount *mnt) +static void mount_setattr_commit(struct mount_kattr *kattr, struct mount *mnt) { - struct mount *m = mnt; + struct mount *m; - do { - if (!kattr->revert) { - unsigned int flags; + for (m = mnt; m; m = next_mnt(m, mnt)) { + unsigned int flags; - do_idmap_mount(kattr, m); - flags = recalc_flags(kattr, m); - WRITE_ONCE(m->mnt.mnt_flags, flags); - } + do_idmap_mount(kattr, m); + flags = recalc_flags(kattr, m); + WRITE_ONCE(m->mnt.mnt_flags, flags); /* If we had to hold writers unblock them. */ if (m->mnt.mnt_flags & MNT_WRITE_HOLD) mnt_unhold_writers(m); - if (!kattr->revert && kattr->propagation) + if (kattr->propagation) change_mnt_propagation(m, kattr->propagation); - - /* - * On failure, only cleanup until we found the first mount - * we failed to handle. - */ - if (kattr->revert == m) - return; - } while (kattr->recurse && (m = next_mnt(m, mnt))); - - if (!kattr->revert) - touch_mnt_namespace(mnt->mnt_ns); + if (!kattr->recurse) + break; + } + touch_mnt_namespace(mnt->mnt_ns); } static int do_mount_setattr(struct path *path, struct mount_kattr *kattr) @@ -4144,7 +4141,8 @@ static int do_mount_setattr(struct path *path, struct mount_kattr *kattr) * changes and if we failed we clean up. */ err = mount_setattr_prepare(kattr, mnt); - mount_setattr_finish(kattr, mnt); + if (!err) + mount_setattr_commit(kattr, mnt); out: unlock_mount_hash(); -- cgit v1.2.3-70-g09d2