linux-kernel - Re: [PATCH] fuse: remove WARN_ON_ONCE() from fuse_iomap_writeback

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: <877bygsjda.fsf@wotan.olymp>
Date: Tue, 02 Sep 2025 21:13:37 +0100
From: Luis Henriques <luis@...lia.com>
To: Joanne Koong <joannelkoong@...il.com>
Cc: Miklos Szeredi <miklos@...redi.hu>,  linux-fsdevel@...r.kernel.org,
  linux-kernel@...r.kernel.org
Subject: Re: [PATCH] fuse: remove WARN_ON_ONCE() from
 fuse_iomap_writeback_{range,submit}()

On Tue, Sep 02 2025, Joanne Koong wrote:

> On Tue, Sep 2, 2025 at 11:07 AM Luis Henriques <luis@...lia.com> wrote:
>>
>> Hi Joanne,
>>
>> On Tue, Sep 02 2025, Joanne Koong wrote:
>>
>> > On Tue, Sep 2, 2025 at 8:22 AM Luis Henriques <luis@...lia.com> wrote:
>> >>
>> >> The usage of WARN_ON_ONCE doesn't seem to be necessary in these functions.
>> >> All fuse_iomap_writeback_submit() call sites already ensure that wpc->wb_ctx
>> >> contains a valid fuse_fill_wb_data.
>> >
>> > Hi Luis,
>> >
>> > Maybe I'm misunderstanding the purpose of WARN()s and when they should
>> > be added, but I thought its main purpose is to guarantee that the
>> > assumptions you're relying on are correct, even if that can be
>> > logically deduced in the code. That's how I see it being used in other
>> > parts of the fuse and non-fuse codebase. For instance, to take one
>> > example, in the main fuse dev.c code, there's a WARN_ON in
>> > fuse_request_queue_background() that the request has the FR_BACKGROUND
>> > bit set. All call sites already ensure that the FR_BACKGROUND bit is
>> > set when they send it as a background request. I don't feel strongly
>> > about whether we decide to remove the WARN or not, but it would be
>> > useful to know as a guiding principle when WARNs should be added vs
>> > when they should not.
>>
>> I'm obviously not an authority on the subject, but those two WARN_ON
>> caught my attention because if they were ever triggered, the kernel would
>> crash anyway and the WARNs would be useless.
>>
>> For example, in fuse_iomap_writeback_range() you have:
>>
>>         struct fuse_fill_wb_data *data = wpc->wb_ctx;
>>         struct fuse_writepage_args *wpa = data->wpa;
>>
>>         [...]
>>
>>         WARN_ON_ONCE(!data);
>>
>> In this case, if 'data' was NULL, you would see a BUG while initialising
>> 'wpa' and the WARN wouldn't help.
>>
>> I'm not 100% sure these WARN_ON_ONCE() should be dropped.  But if there is
>> a small chance of that assertion to ever be true, then there's a need to
>> fix the code and make it safer.  I.e. the 'wpa' initialisation should be
>> done after the WARN_ON_ONCE() and that WARN_ON_ONCE() should be changed to
>> something like:
>>
>>         if (WARN_ON_ONCE(!data))
>>                 return -EIO; /* or other errno */
>>
>> Does it make sense?
>
> Yes, perhaps you missed my previous reply where I stated
>
> "I agree, for the fuse_iomap_writeback_range() case, it would be more
> useful if "wpa = data->wpa" was moved below that warn."

Oops! Since it was past your signature I totally missed it indeed.  Sorry
about that.

>>
>> As I said, I can send another patch to keep those WARNs and fix these
>> error paths.  But again, after going through the call sites I believe it's
>> safe to assume that WARN_ON_ONCE() will never trigger.
>
> I am fine with either keeping (w/ the writeback_range() one reordered)
> or removing it, I don't feel strongly about it, but it seems
> inconsistent to me that if we are removing it because going through
> the call sites proves that it's logically safe, then doesn't that same
> logic apply to the other cases of existing WARN_ONs in the codebase
> where they are also logically safe if we go through the call sites?

OK, you definitely have a point there.  And also the WARN_ONs may be
useful for future changes as well, thus proving they can not be triggered
today may not be reason enough to remove them.

Note however that my reason for picking these two in particular was
mostly[*] because their assertions being true would result in an obvious
kernel crash, while in other WARN_ONs that effect isn't immediately
obvious.

Anyway, tomorrow I'll send v2, keeping the WARNs but preventing these NULL
pointers dereferences.

[*] I say "mostly" because the other reason was pure accident -- I was
actually reading through that code :-)

Cheers,
-- 
Luís