Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Message-ID: <25876.30666.549398.562913@quad.stoffel.home>
Date:   Wed, 27 Sep 2023 14:43:22 -0400
From:   "John Stoffel" <john@stoffel.org>
To:     Mateusz Guzik <mjguzik@gmail.com>
Cc:     John Stoffel <john@stoffel.org>, brauner@kernel.org,
        viro@zeniv.linux.org.uk, linux-kernel@vger.kernel.org,
        linux-fsdevel@vger.kernel.org, torvalds@linux-foundation.org
Subject: Re: [PATCH] vfs: shave work on failed file open
In-Reply-To: <CAGudoHF9nrmR6eH91YpcG4795YAKxemKeMvWNSLaiWtQAYX0uA@mail.gmail.com>
References: <20230925205545.4135472-1-mjguzik@gmail.com>
        <25875.17995.247620.601505@quad.stoffel.home>
        <CAGudoHF9nrmR6eH91YpcG4795YAKxemKeMvWNSLaiWtQAYX0uA@mail.gmail.com>
Precedence: bulk

>>>>> "Mateusz" == Mateusz Guzik <mjguzik@gmail.com> writes:

> On 9/26/23, John Stoffel <john@stoffel.org> wrote:

>> 
>>> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
>>> ---
>>> fs/file_table.c      | 39 +++++++++++++++++++++++++++++++++++++++
>>> fs/namei.c           |  2 +-
>>> include/linux/file.h |  1 +
>>> 3 files changed, 41 insertions(+), 1 deletion(-)
>> 
>>> diff --git a/fs/file_table.c b/fs/file_table.c
>>> index ee21b3da9d08..320dc1f9aa0e 100644
>>> --- a/fs/file_table.c
>>> +++ b/fs/file_table.c
>>> @@ -82,6 +82,16 @@ static inline void file_free(struct file *f)
>>> call_rcu(&f->f_rcuhead, file_free_rcu);
>>> }
>> 
>>> +static inline void file_free_badopen(struct file *f)
>>> +{
>>> +	BUG_ON(f->f_mode & (FMODE_BACKING | FMODE_OPENED));
>> 
>> eww... what a BUG_ON() here?  This seems *way* overkill to crash the
>> system here, and you don't even check if f exists first as well, since
>> I assume the caller checks it or already knows it?
>> 
>> Why not just return an error here and keep going?  What happens if you do?
>> 

> The only caller already checked these flags, so I think BUGing out is prudent.

So how would the flags change if they had been checked before?  And if
they are wrong, why not just exit without doing anything?  Crashing
the system just because you can't free some memory seems like a
horrible thing to do.  

Linus has said multiple times that BUG_ON() isn't the answer.  You
should just do a WARN_ON() instead.  Or WARN_ONCE(), don't just kill
the entire system like this.

John