linux-kernel - Re: [PATCH] vfs: shave work on failed file open

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] [day] [month] [year] [list]

Message-ID: <25876.30666.549398.562913@quad.stoffel.home>
Date:   Wed, 27 Sep 2023 14:43:22 -0400
From:   "John Stoffel" <john@...ffel.org>
To:     Mateusz Guzik <mjguzik@...il.com>
Cc:     John Stoffel <john@...ffel.org>, brauner@...nel.org,
        viro@...iv.linux.org.uk, linux-kernel@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, torvalds@...ux-foundation.org
Subject: Re: [PATCH] vfs: shave work on failed file open

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

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

>> 
>>> Signed-off-by: Mateusz Guzik <mjguzik@...il.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