The current code to override credentials for creation operations is pretty difficult to understand. We effectively override the credentials twice: (1) override with the mounter's credentials (2) copy the mounts credentials and override the fs{g,u}id with the inode {u,g}id And then we elide the revert because it would be an idempotent revert. That elision doesn't buy us anything anymore though because I've made it all work without any reference counting anyway. All it does is mix the two credential overrides together. We can use a cleanup guard to clarify the creation codepaths and make them easier to understand. This just introduces the cleanup guard keeping the patch reviewable. We'll convert the caller in follow-up patches and then drop the duplicated code. Signed-off-by: Christian Brauner --- fs/overlayfs/dir.c | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c index 0030f5a69d22..87f6c5ea6ce0 100644 --- a/fs/overlayfs/dir.c +++ b/fs/overlayfs/dir.c @@ -575,6 +575,42 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode, goto out_dput; } +static const struct cred *ovl_prepare_creds(struct dentry *dentry, struct inode *inode, umode_t mode) +{ + int err; + + if (WARN_ON_ONCE(current->cred != ovl_creds(dentry->d_sb))) + return ERR_PTR(-EINVAL); + + CLASS(prepare_creds, override_cred)(); + if (!override_cred) + return ERR_PTR(-ENOMEM); + + override_cred->fsuid = inode->i_uid; + override_cred->fsgid = inode->i_gid; + + err = security_dentry_create_files_as(dentry, mode, &dentry->d_name, + current->cred, override_cred); + if (err) + return ERR_PTR(err); + + return override_creds(no_free_ptr(override_cred)); +} + +static void ovl_revert_creds(const struct cred *old_cred) +{ + const struct cred *override_cred; + + override_cred = revert_creds(old_cred); + put_cred(override_cred); +} + +DEFINE_CLASS(prepare_creds_ovl, + const struct cred *, + if (!IS_ERR(_T)) ovl_revert_creds(_T), + ovl_prepare_creds(dentry, inode, mode), + struct dentry *dentry, struct inode *inode, umode_t mode) + static const struct cred *ovl_setup_cred_for_create(struct dentry *dentry, struct inode *inode, umode_t mode, -- 2.47.3 This clearly indicates the double-credential override and makes the code a lot easier to grasp with one glance. Signed-off-by: Christian Brauner --- fs/overlayfs/dir.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c index 87f6c5ea6ce0..a276eafb5e78 100644 --- a/fs/overlayfs/dir.c +++ b/fs/overlayfs/dir.c @@ -1372,7 +1372,6 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir, static int ovl_create_tmpfile(struct file *file, struct dentry *dentry, struct inode *inode, umode_t mode) { - const struct cred *new_cred __free(put_cred) = NULL; struct path realparentpath; struct file *realfile; struct ovl_file *of; @@ -1381,10 +1380,10 @@ static int ovl_create_tmpfile(struct file *file, struct dentry *dentry, int flags = file->f_flags | OVL_OPEN_FLAGS; int err; - scoped_class(override_creds_ovl, old_cred, dentry->d_sb) { - new_cred = ovl_setup_cred_for_create(dentry, inode, mode, old_cred); - if (IS_ERR(new_cred)) - return PTR_ERR(new_cred); + with_ovl_creds(dentry->d_sb) { + scoped_class(prepare_creds_ovl, cred, dentry, inode, mode) { + if (IS_ERR(cred)) + return PTR_ERR(cred); ovl_path_upper(dentry->d_parent, &realparentpath); realfile = backing_tmpfile_open(&file->f_path, flags, @@ -1412,6 +1411,7 @@ static int ovl_create_tmpfile(struct file *file, struct dentry *dentry, ovl_file_free(of); } } + } return err; } -- 2.47.3 Reflow the creation routine in preparation of porting it to a guard. Signed-off-by: Christian Brauner --- fs/overlayfs/dir.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c index a276eafb5e78..ff30a91e07f8 100644 --- a/fs/overlayfs/dir.c +++ b/fs/overlayfs/dir.c @@ -644,14 +644,23 @@ static const struct cred *ovl_setup_cred_for_create(struct dentry *dentry, return override_cred; } +static int do_ovl_create_or_link(struct dentry *dentry, struct inode *inode, + struct ovl_cattr *attr) +{ + if (!ovl_dentry_is_whiteout(dentry)) + return ovl_create_upper(dentry, inode, attr); + + return ovl_create_over_whiteout(dentry, inode, attr); +} + static int ovl_create_or_link(struct dentry *dentry, struct inode *inode, struct ovl_cattr *attr, bool origin) { int err; - const struct cred *new_cred __free(put_cred) = NULL; struct dentry *parent = dentry->d_parent; scoped_class(override_creds_ovl, old_cred, dentry->d_sb) { + const struct cred *new_cred __free(put_cred) = NULL; /* * When linking a file with copy up origin into a new parent, mark the * new parent dir "impure". @@ -662,7 +671,6 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode, return err; } - if (!attr->hardlink) { /* * In the creation cases(create, mkdir, mknod, symlink), * ovl should transfer current's fs{u,g}id to underlying @@ -676,16 +684,15 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode, * create a new inode, so just use the ovl mounter's * fs{u,g}id. */ + + if (attr->hardlink) + return do_ovl_create_or_link(dentry, inode, attr); + new_cred = ovl_setup_cred_for_create(dentry, inode, attr->mode, old_cred); if (IS_ERR(new_cred)) return PTR_ERR(new_cred); - } - - if (!ovl_dentry_is_whiteout(dentry)) - return ovl_create_upper(dentry, inode, attr); - - return ovl_create_over_whiteout(dentry, inode, attr); + return do_ovl_create_or_link(dentry, inode, attr); } return err; } -- 2.47.3 The function will become unused in the next patch. We'll remove it in later patches to keep the diff legible. Signed-off-by: Christian Brauner --- fs/overlayfs/dir.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c index ff30a91e07f8..f42e1a22bcb8 100644 --- a/fs/overlayfs/dir.c +++ b/fs/overlayfs/dir.c @@ -611,7 +611,7 @@ DEFINE_CLASS(prepare_creds_ovl, ovl_prepare_creds(dentry, inode, mode), struct dentry *dentry, struct inode *inode, umode_t mode) -static const struct cred *ovl_setup_cred_for_create(struct dentry *dentry, +static const __maybe_unused struct cred *ovl_setup_cred_for_create(struct dentry *dentry, struct inode *inode, umode_t mode, const struct cred *old_cred) -- 2.47.3 This clearly indicates the double-credential override and makes the code a lot easier to grasp with one glance. Signed-off-by: Christian Brauner --- fs/overlayfs/dir.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c index f42e1a22bcb8..d6a3589c0da7 100644 --- a/fs/overlayfs/dir.c +++ b/fs/overlayfs/dir.c @@ -659,8 +659,7 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode, int err; struct dentry *parent = dentry->d_parent; - scoped_class(override_creds_ovl, old_cred, dentry->d_sb) { - const struct cred *new_cred __free(put_cred) = NULL; + with_ovl_creds(dentry->d_sb) { /* * When linking a file with copy up origin into a new parent, mark the * new parent dir "impure". @@ -688,12 +687,12 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode, if (attr->hardlink) return do_ovl_create_or_link(dentry, inode, attr); - new_cred = ovl_setup_cred_for_create(dentry, inode, attr->mode, old_cred); - if (IS_ERR(new_cred)) - return PTR_ERR(new_cred); - + scoped_class(prepare_creds_ovl, cred, dentry, inode, attr->mode) { + if (IS_ERR(cred)) + return PTR_ERR(cred); return do_ovl_create_or_link(dentry, inode, attr); } + } return err; } -- 2.47.3 It is now unused and can be removed. Signed-off-by: Christian Brauner --- fs/overlayfs/dir.c | 33 --------------------------------- 1 file changed, 33 deletions(-) diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c index d6a3589c0da7..7d365203dd0e 100644 --- a/fs/overlayfs/dir.c +++ b/fs/overlayfs/dir.c @@ -611,39 +611,6 @@ DEFINE_CLASS(prepare_creds_ovl, ovl_prepare_creds(dentry, inode, mode), struct dentry *dentry, struct inode *inode, umode_t mode) -static const __maybe_unused struct cred *ovl_setup_cred_for_create(struct dentry *dentry, - struct inode *inode, - umode_t mode, - const struct cred *old_cred) -{ - int err; - struct cred *override_cred; - - override_cred = prepare_creds(); - if (!override_cred) - return ERR_PTR(-ENOMEM); - - override_cred->fsuid = inode->i_uid; - override_cred->fsgid = inode->i_gid; - err = security_dentry_create_files_as(dentry, mode, &dentry->d_name, - old_cred, override_cred); - if (err) { - put_cred(override_cred); - return ERR_PTR(err); - } - - /* - * Caller is going to match this with revert_creds() and drop - * referenec on the returned creds. - * We must be called with creator creds already, otherwise we risk - * leaking creds. - */ - old_cred = override_creds(override_cred); - WARN_ON_ONCE(old_cred != ovl_creds(dentry->d_sb)); - - return override_cred; -} - static int do_ovl_create_or_link(struct dentry *dentry, struct inode *inode, struct ovl_cattr *attr) { -- 2.47.3