Amir Goldstein <amir73il@xxxxxxxxx> writes:
> On Fri, Jan 26, 2024 at 1:57 AM Vinicius Costa Gomes
> <vinicius.gomes@xxxxxxxxx> wrote:
>>
>> File operations in overlayfs also check against the credentials of the
>> mounter task, stored in the superblock, this credentials will outlive
>> most of the operations. For these cases, use the recently introduced
>> guard statements to guarantee that override/revert_creds() are paired.
>>
>> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@xxxxxxxxx>
>> ---
>> fs/overlayfs/copy_up.c | 4 +--
>> fs/overlayfs/dir.c | 22 +++++++------
>> fs/overlayfs/file.c | 70 ++++++++++++++++++++++--------------------
>> fs/overlayfs/inode.c | 60 +++++++++++++++++++-----------------
>> fs/overlayfs/namei.c | 21 ++++++-------
>> fs/overlayfs/readdir.c | 18 +++++------
>> fs/overlayfs/util.c | 23 +++++++-------
>> fs/overlayfs/xattrs.c | 34 ++++++++++----------
>> 8 files changed, 130 insertions(+), 122 deletions(-)
>>
>> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
>> index b8e25ca51016..55d1f2b60775 100644
>> --- a/fs/overlayfs/copy_up.c
>> +++ b/fs/overlayfs/copy_up.c
>> @@ -1202,7 +1202,8 @@ static int ovl_copy_up_flags(struct dentry *dentry, int flags)
>> if (err)
>> return err;
>>
>> - old_cred = ovl_override_creds(dentry->d_sb);
>> + old_cred = ovl_creds(dentry->d_sb);
>> + guard(cred)(old_cred);
>> while (!err) {
>> struct dentry *next;
>> struct dentry *parent = NULL;
>> @@ -1227,7 +1228,6 @@ static int ovl_copy_up_flags(struct dentry *dentry, int flags)
>> dput(parent);
>> dput(next);
>> }
>> - revert_creds(old_cred);
>>
>> return err;
>> }
>> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
>> index 0f8b4a719237..5aa43a3a7b3e 100644
>> --- a/fs/overlayfs/dir.c
>> +++ b/fs/overlayfs/dir.c
>> @@ -687,9 +687,9 @@ static int ovl_set_link_redirect(struct dentry *dentry)
>> const struct cred *old_cred;
>> int err;
>>
>> - old_cred = ovl_override_creds(dentry->d_sb);
>> + old_cred = ovl_creds(dentry->d_sb);
>> + guard(cred)(old_cred);
>> err = ovl_set_redirect(dentry, false);
>> - revert_creds(old_cred);
>>
>> return err;
>> }
>> @@ -894,12 +894,13 @@ static int ovl_do_remove(struct dentry *dentry, bool is_dir)
>> if (err)
>> goto out;
>>
>> - old_cred = ovl_override_creds(dentry->d_sb);
>> - if (!lower_positive)
>> - err = ovl_remove_upper(dentry, is_dir, &list);
>> - else
>> - err = ovl_remove_and_whiteout(dentry, &list);
>> - revert_creds(old_cred);
>> + old_cred = ovl_creds(dentry->d_sb);
>> + scoped_guard(cred, old_cred) {
>> + if (!lower_positive)
>> + err = ovl_remove_upper(dentry, is_dir, &list);
>> + else
>> + err = ovl_remove_and_whiteout(dentry, &list);
>> + }
>> if (!err) {
>> if (is_dir)
>> clear_nlink(dentry->d_inode);
>> @@ -1146,7 +1147,8 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
>> goto out;
>> }
>>
>> - old_cred = ovl_override_creds(old->d_sb);
>> + old_cred = ovl_creds(old->d_sb);
>> + old_cred = override_creds_light(old_cred);
>>
>> if (!list_empty(&list)) {
>> opaquedir = ovl_clear_empty(new, &list);
>> @@ -1279,7 +1281,7 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
>> out_unlock:
>> unlock_rename(new_upperdir, old_upperdir);
>> out_revert_creds:
>> - revert_creds(old_cred);
>> + revert_creds_light(old_cred);
>> if (update_nlink)
>> ovl_nlink_end(new);
>> else
>
> Most of my comments on this patch are identical to the ones I have made on
> backing file, so rather complete that review before moving on to this bigger
> patch.
>
> I even wonder if we need a specialized macro for overlayfs
> guard(ovl_creds, ofs); or if
> guard(cred, ovl_override_creds(dentry->d_sb));
> is good enough.
>
I think that if the DEFINE_LOCK_GUARD_1() idea works, that might be
unecessary. Let's see.
> One thing that stands out in functions like ovl_rename() is that,
> understandably, you tried to preserve logic, but in fact, the scope of
> override_creds/revert_creds() in some of the overlayfs functions ir rather
> arbitrary.
>
That's very good to learn.
> The simplest solution for functions like the above is to use guard(cred, ..
> and extend the scope till the end of the function.
> This needs more careful review, but the end result will be much cleaner.
>
Yeah, increasing the indentation level of whole blocks cause the whole
patch to be much harder to review.
Using more guard() statements will certainly help.
>> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
>> index 05536964d37f..482bf78555e2 100644
>> --- a/fs/overlayfs/file.c
>> +++ b/fs/overlayfs/file.c
>> @@ -42,7 +42,8 @@ static struct file *ovl_open_realfile(const struct file *file,
>> if (flags & O_APPEND)
>> acc_mode |= MAY_APPEND;
>>
>> - old_cred = ovl_override_creds(inode->i_sb);
>> + old_cred = ovl_creds(inode->i_sb);
>> + guard(cred)(old_cred);
>> real_idmap = mnt_idmap(realpath->mnt);
>> err = inode_permission(real_idmap, realinode, MAY_OPEN | acc_mode);
>> if (err) {
>> @@ -54,7 +55,6 @@ static struct file *ovl_open_realfile(const struct file *file,
>> realfile = backing_file_open(&file->f_path, flags, realpath,
>> current_cred());
>> }
>> - revert_creds(old_cred);
>>
>> pr_debug("open(%p[%pD2/%c], 0%o) -> (%p, 0%o)\n",
>> file, file, ovl_whatisit(inode, realinode), file->f_flags,
>> @@ -214,9 +214,9 @@ static loff_t ovl_llseek(struct file *file, loff_t offset, int whence)
>> ovl_inode_lock(inode);
>> real.file->f_pos = file->f_pos;
>>
>> - old_cred = ovl_override_creds(inode->i_sb);
>> - ret = vfs_llseek(real.file, offset, whence);
>> - revert_creds(old_cred);
>> + old_cred = ovl_creds(inode->i_sb);
>> + scoped_guard(cred, old_cred)
>> + ret = vfs_llseek(real.file, offset, whence);
>>
>> file->f_pos = real.file->f_pos;
>> ovl_inode_unlock(inode);
>> @@ -388,7 +388,6 @@ static ssize_t ovl_splice_write(struct pipe_inode_info *pipe, struct file *out,
>> static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
>> {
>> struct fd real;
>> - const struct cred *old_cred;
>> int ret;
>>
>> ret = ovl_sync_status(OVL_FS(file_inode(file)->i_sb));
>> @@ -401,9 +400,11 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
>>
>> /* Don't sync lower file for fear of receiving EROFS error */
>> if (file_inode(real.file) == ovl_inode_upper(file_inode(file))) {
>> - old_cred = ovl_override_creds(file_inode(file)->i_sb);
>> + const struct cred *old_cred;
>> +
>> + old_cred = ovl_creds(file_inode(file)->i_sb);
>> + guard(cred)(old_cred);
>> ret = vfs_fsync_range(real.file, start, end, datasync);
>> - revert_creds(old_cred);
>> }
>>
>> fdput(real);
>> @@ -441,9 +442,9 @@ static long ovl_fallocate(struct file *file, int mode, loff_t offset, loff_t len
>> if (ret)
>> goto out_unlock;
>>
>> - old_cred = ovl_override_creds(file_inode(file)->i_sb);
>> - ret = vfs_fallocate(real.file, mode, offset, len);
>> - revert_creds(old_cred);
>> + old_cred = ovl_creds(file_inode(file)->i_sb);
>> + scoped_guard(cred, old_cred)
>> + ret = vfs_fallocate(real.file, mode, offset, len);
>>
>> /* Update size */
>> ovl_file_modified(file);
>> @@ -466,9 +467,9 @@ static int ovl_fadvise(struct file *file, loff_t offset, loff_t len, int advice)
>> if (ret)
>> return ret;
>>
>> - old_cred = ovl_override_creds(file_inode(file)->i_sb);
>> - ret = vfs_fadvise(real.file, offset, len, advice);
>> - revert_creds(old_cred);
>> + old_cred = ovl_creds(file_inode(file)->i_sb);
>> + scoped_guard(cred, old_cred)
>> + ret = vfs_fadvise(real.file, offset, len, advice);
>>
>> fdput(real);
>>
>> @@ -509,25 +510,25 @@ static loff_t ovl_copyfile(struct file *file_in, loff_t pos_in,
>> goto out_unlock;
>> }
>>
>> - old_cred = ovl_override_creds(file_inode(file_out)->i_sb);
>> - switch (op) {
>> - case OVL_COPY:
>> - ret = vfs_copy_file_range(real_in.file, pos_in,
>> - real_out.file, pos_out, len, flags);
>> - break;
>> + old_cred = ovl_creds(file_inode(file_out)->i_sb);
>> + scoped_guard(cred, old_cred)
>> + switch (op) {
>> + case OVL_COPY:
>> + ret = vfs_copy_file_range(real_in.file, pos_in,
>> + real_out.file, pos_out, len, flags);
>> + break;
>>
>> - case OVL_CLONE:
>> - ret = vfs_clone_file_range(real_in.file, pos_in,
>> - real_out.file, pos_out, len, flags);
>> - break;
>> + case OVL_CLONE:
>> + ret = vfs_clone_file_range(real_in.file, pos_in,
>> + real_out.file, pos_out, len, flags);
>> + break;
>>
>> - case OVL_DEDUPE:
>> - ret = vfs_dedupe_file_range_one(real_in.file, pos_in,
>> - real_out.file, pos_out, len,
>> - flags);
>> - break;
>> - }
>> - revert_creds(old_cred);
>> + case OVL_DEDUPE:
>> + ret = vfs_dedupe_file_range_one(real_in.file, pos_in,
>> + real_out.file, pos_out, len,
>> + flags);
>> + break;
>> + }
>>
>> /* Update size */
>> ovl_file_modified(file_out);
>
> This is another case where extending the scope to the end of the function
> is the simpler/cleaner solution.
>
> Thanks,
> Amir.
--
Vinicius
