From 87bb5b60019c60e1f902e6885734cc4e5135c2d9 Mon Sep 17 00:00:00 2001 From: Christian Brauner Date: Thu, 3 Feb 2022 14:14:11 +0100 Subject: [PATCH] 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 | 88 ++++++++++++++++++++++++++------------------------ 1 file changed, 45 insertions(+), 43 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; + kattr->revert = m; + flags = recalc_flags(kattr, m); - if (!can_change_locked_flags(m, flags)) { - *err = -EPERM; - goto out; - } + if (!can_change_locked_flags(m, flags)) + return err; - *err = can_idmap_mount(kattr, m); - if (*err) - goto out; - - 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(); - /* - * Get the mount tree in a shape where we can change mount - * properties without failure. - */ - 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); + /* Ensure that this isn't anything purely vfs internal. */ + if (!is_mounted(&mnt->mnt)) + goto out; + /* + * 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. + */ + 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) {