Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
MIME-Version: 1.0
In-Reply-To: <CAHk-=wjUCLfuKks-VGTG9hrFAORb5cuzqyC0gRXptYGGgL=YYg@mail.gmail.com>
References: <20230926162228.68666-1-mjguzik@gmail.com> <CAHk-=wjUCLfuKks-VGTG9hrFAORb5cuzqyC0gRXptYGGgL=YYg@mail.gmail.com>
From:   Mateusz Guzik <mjguzik@gmail.com>
Date:   Tue, 26 Sep 2023 21:28:28 +0200
Message-ID: <CAGudoHGej+gmmv0OOoep2ENkf7hMBib-KL44Fu=Ym46j=r6VEA@mail.gmail.com>
Subject: Re: [PATCH v2] vfs: shave work on failed file open
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     brauner@kernel.org, viro@zeniv.linux.org.uk,
        linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On 9/26/23, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> On Tue, 26 Sept 2023 at 09:22, Mateusz Guzik <mjguzik@gmail.com> wrote:
>>
>> +void fput_badopen(struct file *file)
>> +{
>> +       if (unlikely(file->f_mode & (FMODE_BACKING | FMODE_OPENED))) {
>> +               fput(file);
>> +               return;
>> +       }
>
> I don't understand.
>
> Why the FMODE_BACKING test?
>
> The only thing that sets FMODE_BACKING is alloc_empty_backing_file(),
> and we know that isn't involved, because the file that is free'd is
>
>         file = alloc_empty_file(op->open_flag, current_cred());
>
> so that test makes no sense.
>

I tried to future proof by dodging the thing, but I can drop it if you
insist. Also see below.

> It might make sense as another WARN_ON_ONCE(), but honestly, why even
> that?  Why worry about FMODE_BACKING?
>
> Now, the FMODE_OPENED check makes sense to me, in that it most
> definitely can be set, and means we need to call the ->release()
> callback and a lot more. Although I get the feeling that this test
> would make more sense in the caller, since path_openat() _already_
> checks for FMODE_OPENED in the non-error path too.
>
>> +       if (WARN_ON_ONCE(atomic_long_cmpxchg(&file->f_count, 1, 0) != 1))
>> {
>> +               fput(file);
>> +               return;
>> +       }
>
> Ok, I kind of see why you'd want this safety check.  I don't see how
> f_count could be validly anything else, but that's what the
> WARN_ON_ONCE is all about.
>

This would be VFSDEBUG or whatever if it was available. But between
nobody checking this and production kernels suffering the check when
they should not, I take the latter.

I wanted to propose debug macros for vfs but could not be bothered to
type it up and argue about it, maybe I'll get around to it.

> Anyway, I think I'd be happier about this if it was more of a "just
> the reverse of alloc_empty_file()", and path_openat() literally did
> just
>
>         if (likely(file->f_mode & FMODE_OPENED))
>                 release_empty_file(file);
>         else
>                 fput(file);
>
> instead of having this fput_badopen() helper that feels like it needs
> to care about other cases than alloc_empty_file().
>

I don't have a strong opinion, I think my variant is cleaner and more
generic, but this boils down to taste and this is definitely not the
hill I'm willing to die on.

I am enable to whatever tidy ups without a fight as long as the core
remains (task work and rcu dodged).

All that said, I think it is Christian's call on how it should look like.

-- 
Mateusz Guzik <mjguzik gmail.com>