[PATCH] fuse: remove WARN_ON_ONCE() from fuse_iomap_writeback_{range,submit}()

[PATCH] fuse: remove WARN_ON_ONCE() from fuse_iomap_writeback_{range,submit}()

[Luis Henriques]

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.

Function fuse_iomap_writeback_range() also seems to always be called with a
valid value.  But even if this wasn't the case, there would be a crash
before this WARN_ON_ONCE() because ->wpa is being accessed before it.

Signed-off-by: Luis Henriques <luis@igalia.com>
---
As I'm saying above, I _think_ there's no need for these WARN_ON_ONCE().
However, if I'm wrong and they are required, I believe there's a need for
a different patch (I can send one) to actually prevent a kernel crash.

 fs/fuse/file.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 5525a4520b0f..fac52f9fb333 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -2142,8 +2142,6 @@ static ssize_t fuse_iomap_writeback_range(struct iomap_writepage_ctx *wpc,
 	struct fuse_conn *fc = get_fuse_conn(inode);
 	loff_t offset = offset_in_folio(folio, pos);
 
-	WARN_ON_ONCE(!data);
-
 	if (!data->ff) {
 		data->ff = fuse_write_file_get(fi);
 		if (!data->ff)
@@ -2182,8 +2180,6 @@ static int fuse_iomap_writeback_submit(struct iomap_writepage_ctx *wpc,
 {
 	struct fuse_fill_wb_data *data = wpc->wb_ctx;
 
-	WARN_ON_ONCE(!data);
-
 	if (data->wpa) {
 		WARN_ON(!data->wpa->ia.ap.num_folios);
 		fuse_writepages_send(wpc->inode, data);

Re: [PATCH] fuse: remove WARN_ON_ONCE() from fuse_iomap_writeback_{range,submit}()

[Joanne Koong]

On Tue, Sep 2, 2025 at 8:22 AM Luis Henriques <luis@igalia.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.

Thanks,
Joanne

>
> Function fuse_iomap_writeback_range() also seems to always be called with a
> valid value.  But even if this wasn't the case, there would be a crash
> before this WARN_ON_ONCE() because ->wpa is being accessed before it.
>

I agree, for the fuse_iomap_writeback_range() case, it would be more
useful if "wpa = data->wpa" was moved below that warn.

> Signed-off-by: Luis Henriques <luis@igalia.com>
> ---
> As I'm saying above, I _think_ there's no need for these WARN_ON_ONCE().
> However, if I'm wrong and they are required, I believe there's a need for
> a different patch (I can send one) to actually prevent a kernel crash.
>
>  fs/fuse/file.c | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 5525a4520b0f..fac52f9fb333 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -2142,8 +2142,6 @@ static ssize_t fuse_iomap_writeback_range(struct iomap_writepage_ctx *wpc,
>         struct fuse_conn *fc = get_fuse_conn(inode);
>         loff_t offset = offset_in_folio(folio, pos);
>
> -       WARN_ON_ONCE(!data);
> -
>         if (!data->ff) {
>                 data->ff = fuse_write_file_get(fi);
>                 if (!data->ff)
> @@ -2182,8 +2180,6 @@ static int fuse_iomap_writeback_submit(struct iomap_writepage_ctx *wpc,
>  {
>         struct fuse_fill_wb_data *data = wpc->wb_ctx;
>
> -       WARN_ON_ONCE(!data);
> -
>         if (data->wpa) {
>                 WARN_ON(!data->wpa->ia.ap.num_folios);
>                 fuse_writepages_send(wpc->inode, data);

Re: [PATCH] fuse: remove WARN_ON_ONCE() from fuse_iomap_writeback_{range,submit}()

[Luis Henriques]

Hi Joanne,

On Tue, Sep 02 2025, Joanne Koong wrote:

> On Tue, Sep 2, 2025 at 8:22 AM Luis Henriques <luis@igalia.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?

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.

Cheers,
-- 
Luís


> Thanks,
> Joanne
>
>>
>> Function fuse_iomap_writeback_range() also seems to always be called with a
>> valid value.  But even if this wasn't the case, there would be a crash
>> before this WARN_ON_ONCE() because ->wpa is being accessed before it.
>>
>
> I agree, for the fuse_iomap_writeback_range() case, it would be more
> useful if "wpa = data->wpa" was moved below that warn.
>
>> Signed-off-by: Luis Henriques <luis@igalia.com>
>> ---
>> As I'm saying above, I _think_ there's no need for these WARN_ON_ONCE().
>> However, if I'm wrong and they are required, I believe there's a need for
>> a different patch (I can send one) to actually prevent a kernel crash.
>>
>>  fs/fuse/file.c | 4 ----
>>  1 file changed, 4 deletions(-)
>>
>> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
>> index 5525a4520b0f..fac52f9fb333 100644
>> --- a/fs/fuse/file.c
>> +++ b/fs/fuse/file.c
>> @@ -2142,8 +2142,6 @@ static ssize_t fuse_iomap_writeback_range(struct iomap_writepage_ctx *wpc,
>>         struct fuse_conn *fc = get_fuse_conn(inode);
>>         loff_t offset = offset_in_folio(folio, pos);
>>
>> -       WARN_ON_ONCE(!data);
>> -
>>         if (!data->ff) {
>>                 data->ff = fuse_write_file_get(fi);
>>                 if (!data->ff)
>> @@ -2182,8 +2180,6 @@ static int fuse_iomap_writeback_submit(struct iomap_writepage_ctx *wpc,
>>  {
>>         struct fuse_fill_wb_data *data = wpc->wb_ctx;
>>
>> -       WARN_ON_ONCE(!data);
>> -
>>         if (data->wpa) {
>>                 WARN_ON(!data->wpa->ia.ap.num_folios);
>>                 fuse_writepages_send(wpc->inode, data);

Re: [PATCH] fuse: remove WARN_ON_ONCE() from fuse_iomap_writeback_{range,submit}()

[Joanne Koong]

On Tue, Sep 2, 2025 at 11:07 AM Luis Henriques <luis@igalia.com> wrote:
>
> Hi Joanne,
>
> On Tue, Sep 02 2025, Joanne Koong wrote:
>
> > On Tue, Sep 2, 2025 at 8:22 AM Luis Henriques <luis@igalia.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

Re: [PATCH] fuse: remove WARN_ON_ONCE() from fuse_iomap_writeback_{range,submit}()

[Luis Henriques]

On Tue, Sep 02 2025, Joanne Koong wrote:

> On Tue, Sep 2, 2025 at 11:07 AM Luis Henriques <luis@igalia.com> wrote:
>>
>> Hi Joanne,
>>
>> On Tue, Sep 02 2025, Joanne Koong wrote:
>>
>> > On Tue, Sep 2, 2025 at 8:22 AM Luis Henriques <luis@igalia.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