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

Message-ID: <CAJnrk1bd62RcE9UU8COdpzSF0kk3DPYwgmwk+xCQew0-C43WXg@mail.gmail.com>
Date: Tue, 2 Sep 2025 11:31:35 -0700
From: Joanne Koong <joannelkoong@...il.com>
To: Luis Henriques <luis@...lia.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 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."

>
> 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?

Thanks,
Joanne
>
> Cheers,
> --
> Luís
>
>
> > Thanks,
> > Joanne