Re: [RFC v2 3/4] overlayfs: Optimize credentials usage

[Vinicius Costa Gomes <vinicius.gomes@intel.com>]

Amir Goldstein <amir73il@gmail.com> writes:

> On Fri, Jan 26, 2024 at 1:57 AM Vinicius Costa Gomes
> <vinicius.gomes@intel.com> 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@intel.com>
>> ---
>>  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