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. Reviewed-by: Amir Goldstein 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 4fd6ddd6f4ef..3eb0bb0b8f3b 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_override_creator_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_creator_creds(const struct cred *old_cred) +{ + const struct cred *override_cred; + + override_cred = revert_creds(old_cred); + put_cred(override_cred); +} + +DEFINE_CLASS(ovl_override_creator_creds, + const struct cred *, + if (!IS_ERR_OR_NULL(_T)) ovl_revert_creator_creds(_T), + ovl_override_creator_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 3eb0bb0b8f3b..dad818de4386 100644 --- a/fs/overlayfs/dir.c +++ b/fs/overlayfs/dir.c @@ -1387,7 +1387,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; @@ -1396,10 +1395,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(ovl_override_creator_creds, 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, &realparentpath, @@ -1425,6 +1424,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 | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c index dad818de4386..150d2ae8e571 100644 --- a/fs/overlayfs/dir.c +++ b/fs/overlayfs/dir.c @@ -644,6 +644,15 @@ 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) { @@ -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. Reviewed-by: Amir Goldstein 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 150d2ae8e571..1bb311a25303 100644 --- a/fs/overlayfs/dir.c +++ b/fs/overlayfs/dir.c @@ -611,7 +611,7 @@ DEFINE_CLASS(ovl_override_creator_creds, ovl_override_creator_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 1bb311a25303..cb474b649ed2 100644 --- a/fs/overlayfs/dir.c +++ b/fs/overlayfs/dir.c @@ -657,10 +657,9 @@ 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) { + 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(ovl_override_creator_creds, 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. Reviewed-by: Amir Goldstein 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 cb474b649ed2..e428a2de59fc 100644 --- a/fs/overlayfs/dir.c +++ b/fs/overlayfs/dir.c @@ -611,39 +611,6 @@ DEFINE_CLASS(ovl_override_creator_creds, ovl_override_creator_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