by Vinicius Costa Gomes

[permalink] [raw]

Subject: Re: [RFC v2 4/4] fs: Optimize credentials reference count for backing file ops

Amir Goldstein <[email protected]> writes:

> On Fri, Jan 26, 2024 at 1:57 AM Vinicius Costa Gomes
> <[email protected]> wrote:
>>
>> For backing file operations, users are expected to pass credentials
>> that will outlive the backing file common operations.
>>
>> Use the specialized guard statements to override/revert the
>> credentials.
>>
>
> As I wrote before, I prefer to see this patch gets reviewed and merged
> before the overlayfs large patch, so please reorder the series.
>

Sure. Will do.

>> Signed-off-by: Vinicius Costa Gomes <[email protected]>
>> ---
>> fs/backing-file.c | 124 ++++++++++++++++++++++------------------------
>> 1 file changed, 60 insertions(+), 64 deletions(-)
>>
>> diff --git a/fs/backing-file.c b/fs/backing-file.c
>> index a681f38d84d8..9874f09f860f 100644
>> --- a/fs/backing-file.c
>> +++ b/fs/backing-file.c
>> @@ -140,7 +140,7 @@ ssize_t backing_file_read_iter(struct file *file, struct iov_iter *iter,
>> struct backing_file_ctx *ctx)
>> {
>> struct backing_aio *aio = NULL;
>> - const struct cred *old_cred;
>> + const struct cred *old_cred = ctx->cred;
>> ssize_t ret;
>>
>> if (WARN_ON_ONCE(!(file->f_mode & FMODE_BACKING)))
>> @@ -153,29 +153,28 @@ ssize_t backing_file_read_iter(struct file *file, struct iov_iter *iter,
>> !(file->f_mode & FMODE_CAN_ODIRECT))
>> return -EINVAL;
>>
>> - old_cred = override_creds(ctx->cred);
>> - if (is_sync_kiocb(iocb)) {
>> - rwf_t rwf = iocb_to_rw_flags(flags);
>> + scoped_guard(cred, old_cred) {
>
> This reads very strage.
>
> Also, I see that e.g. scoped_guard(spinlock_irqsave, ... hides the local var
> used for save/restore of flags inside the macro.
>
> Perhaps you use the same technique for scoped_guard(cred, ..
> loose the local old_cred variable in all those functions and then the
> code will read:
>
> scoped_guard(cred, ctx->cred) {
>
> which is nicer IMO.

Most likely using DEFINE_LOCK_GUARD_1() would allow us to use the nicer version.

>
>> + if (is_sync_kiocb(iocb)) {
>> + rwf_t rwf = iocb_to_rw_flags(flags);
>>
>> - ret = vfs_iter_read(file, iter, &iocb->ki_pos, rwf);
>> - } else {
>> - ret = -ENOMEM;
>> - aio = kmem_cache_zalloc(backing_aio_cachep, GFP_KERNEL);
>> - if (!aio)
>> - goto out;
>> + ret = vfs_iter_read(file, iter, &iocb->ki_pos, rwf);
>> + } else {
>> + ret = -ENOMEM;
>> + aio = kmem_cache_zalloc(backing_aio_cachep, GFP_KERNEL);
>> + if (!aio)
>> + goto out;
>>
>> - aio->orig_iocb = iocb;
>> - kiocb_clone(&aio->iocb, iocb, get_file(file));
>> - aio->iocb.ki_complete = backing_aio_rw_complete;
>> - refcount_set(&aio->ref, 2);
>> - ret = vfs_iocb_iter_read(file, &aio->iocb, iter);
>> - backing_aio_put(aio);
>> - if (ret != -EIOCBQUEUED)
>> - backing_aio_cleanup(aio, ret);
>> + aio->orig_iocb = iocb;
>> + kiocb_clone(&aio->iocb, iocb, get_file(file));
>> + aio->iocb.ki_complete = backing_aio_rw_complete;
>> + refcount_set(&aio->ref, 2);
>> + ret = vfs_iocb_iter_read(file, &aio->iocb, iter);
>> + backing_aio_put(aio);
>> + if (ret != -EIOCBQUEUED)
>> + backing_aio_cleanup(aio, ret);
>> + }
>
> if possible, I would rather avoid all this churn in functions that mostly
> do work with the new cred, so either use guard(cred, ) directly or split a
> helper that uses guard(cred, ) form the rest.
>

Yeah, I think what happened is that I tried to keep the scope of the
guard to be as close as possible to override/revert (as you said), and
that caused the churn.

Probably using guard() more will reduce these confusing code changes. I
am going to try that.

> Thanks,
> Amir.

Cheers,
--
Vinicius

2024-01-27 00:57:09

by Vinicius Costa Gomes

[permalink] [raw]

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

Amir Goldstein <[email protected]> writes:

> On Fri, Jan 26, 2024 at 1:57 AM Vinicius Costa Gomes
> <[email protected]> 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 <[email protected]>
>> ---
>> 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