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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87mssr4o7v.fsf@intel.com>
Date: Fri, 26 Jan 2024 16:25:08 -0800
From: Vinicius Costa Gomes <vinicius.gomes@...el.com>
To: Amir Goldstein <amir73il@...il.com>
Cc: brauner@...nel.org, hu1.chen@...el.com, miklos@...redi.hu,
 malini.bhandaru@...el.com, tim.c.chen@...el.com, mikko.ylinen@...el.com,
 lizhen.you@...el.com, linux-unionfs@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-fsdevel
 <linux-fsdevel@...r.kernel.org>
Subject: Re: [RFC v2 4/4] fs: Optimize credentials reference count for
 backing file ops

Amir Goldstein <amir73il@...il.com> writes:

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