[PATCH v2] fuse: prevent possible NULL pointer dereference in fuse_iomap_writeback_{range,submit}()

[PATCH v2] fuse: prevent possible NULL pointer dereference in fuse_iomap_writeback_{range,submit}()

[Luis Henriques]

These two functions make use of the WARN_ON_ONCE() macro to help debugging
a NULL wpc->wb_ctx.  However, this doesn't prevent the possibility of NULL
pointer dereferences in the code.  This patch adds some extra defensive
checks to avoid these NULL pointer accesses.

Fixes: ef7e7cbb323f ("fuse: use iomap for writeback")
Signed-off-by: Luis Henriques <luis@igalia.com>
---
Hi!

This v2 results from Joanne's inputs -- I now believe that it is better to
keep the WARN_ON_ONCE() macros, but it's still good to try to minimise
the undesirable effects of a NULL wpc->wb_ctx.

I've also added the 'Fixes:' tag to the commit message.

 fs/fuse/file.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 5525a4520b0f..990c287bc3e3 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -2135,14 +2135,18 @@ static ssize_t fuse_iomap_writeback_range(struct iomap_writepage_ctx *wpc,
 					  unsigned len, u64 end_pos)
 {
 	struct fuse_fill_wb_data *data = wpc->wb_ctx;
-	struct fuse_writepage_args *wpa = data->wpa;
-	struct fuse_args_pages *ap = &wpa->ia.ap;
+	struct fuse_writepage_args *wpa;
+	struct fuse_args_pages *ap;
 	struct inode *inode = wpc->inode;
 	struct fuse_inode *fi = get_fuse_inode(inode);
 	struct fuse_conn *fc = get_fuse_conn(inode);
 	loff_t offset = offset_in_folio(folio, pos);
 
-	WARN_ON_ONCE(!data);
+	if (WARN_ON_ONCE(!data))
+		return -EIO;
+
+	wpa = data->wpa;
+	ap = &wpa->ia.ap;
 
 	if (!data->ff) {
 		data->ff = fuse_write_file_get(fi);
@@ -2182,7 +2186,8 @@ 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 (WARN_ON_ONCE(!data))
+		return error ? error : -EIO;
 
 	if (data->wpa) {
 		WARN_ON(!data->wpa->ia.ap.num_folios);

Re: [PATCH v2] fuse: prevent possible NULL pointer dereference in fuse_iomap_writeback_{range,submit}()

[Joanne Koong]

On Wed, Sep 3, 2025 at 1:35 AM Luis Henriques <luis@igalia.com> wrote:
>
> These two functions make use of the WARN_ON_ONCE() macro to help debugging
> a NULL wpc->wb_ctx.  However, this doesn't prevent the possibility of NULL
> pointer dereferences in the code.  This patch adds some extra defensive
> checks to avoid these NULL pointer accesses.
>
> Fixes: ef7e7cbb323f ("fuse: use iomap for writeback")
> Signed-off-by: Luis Henriques <luis@igalia.com>
> ---
> Hi!
>
> This v2 results from Joanne's inputs -- I now believe that it is better to
> keep the WARN_ON_ONCE() macros, but it's still good to try to minimise
> the undesirable effects of a NULL wpc->wb_ctx.
>
> I've also added the 'Fixes:' tag to the commit message.
>
>  fs/fuse/file.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 5525a4520b0f..990c287bc3e3 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -2135,14 +2135,18 @@ static ssize_t fuse_iomap_writeback_range(struct iomap_writepage_ctx *wpc,
>                                           unsigned len, u64 end_pos)
>  {
>         struct fuse_fill_wb_data *data = wpc->wb_ctx;
> -       struct fuse_writepage_args *wpa = data->wpa;
> -       struct fuse_args_pages *ap = &wpa->ia.ap;
> +       struct fuse_writepage_args *wpa;
> +       struct fuse_args_pages *ap;
>         struct inode *inode = wpc->inode;
>         struct fuse_inode *fi = get_fuse_inode(inode);
>         struct fuse_conn *fc = get_fuse_conn(inode);
>         loff_t offset = offset_in_folio(folio, pos);
>
> -       WARN_ON_ONCE(!data);
> +       if (WARN_ON_ONCE(!data))
> +               return -EIO;

imo this WARN_ON_ONCE (and the one below) should be left as is instead
of embedded in the "if" construct. The data pointer passed in is set
by fuse and as such, we're able to reasonably guarantee that data is a
valid pointer. Looking at other examples of WARN_ON in the fuse
codebase, the places where an "if" construct is used are for cases
where the assumptions that are made are more delicate (eg folio
mapping state, in fuse_try_move_folio()) and less clearly obvious. I
think this WARN_ON_ONCE here and below should be left as is.


Thanks,
Joanne

> +
> +       wpa = data->wpa;
> +       ap = &wpa->ia.ap;
>
>         if (!data->ff) {
>                 data->ff = fuse_write_file_get(fi);
> @@ -2182,7 +2186,8 @@ 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 (WARN_ON_ONCE(!data))
> +               return error ? error : -EIO;
>
>         if (data->wpa) {
>                 WARN_ON(!data->wpa->ia.ap.num_folios);

Re: [PATCH v2] fuse: prevent possible NULL pointer dereference in fuse_iomap_writeback_{range,submit}()

[Luis Henriques]

On Wed, Sep 03 2025, Joanne Koong wrote:

> On Wed, Sep 3, 2025 at 1:35 AM Luis Henriques <luis@igalia.com> wrote:
>>
>> These two functions make use of the WARN_ON_ONCE() macro to help debugging
>> a NULL wpc->wb_ctx.  However, this doesn't prevent the possibility of NULL
>> pointer dereferences in the code.  This patch adds some extra defensive
>> checks to avoid these NULL pointer accesses.
>>
>> Fixes: ef7e7cbb323f ("fuse: use iomap for writeback")
>> Signed-off-by: Luis Henriques <luis@igalia.com>
>> ---
>> Hi!
>>
>> This v2 results from Joanne's inputs -- I now believe that it is better to
>> keep the WARN_ON_ONCE() macros, but it's still good to try to minimise
>> the undesirable effects of a NULL wpc->wb_ctx.
>>
>> I've also added the 'Fixes:' tag to the commit message.
>>
>>  fs/fuse/file.c | 13 +++++++++----
>>  1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
>> index 5525a4520b0f..990c287bc3e3 100644
>> --- a/fs/fuse/file.c
>> +++ b/fs/fuse/file.c
>> @@ -2135,14 +2135,18 @@ static ssize_t fuse_iomap_writeback_range(struct iomap_writepage_ctx *wpc,
>>                                           unsigned len, u64 end_pos)
>>  {
>>         struct fuse_fill_wb_data *data = wpc->wb_ctx;
>> -       struct fuse_writepage_args *wpa = data->wpa;
>> -       struct fuse_args_pages *ap = &wpa->ia.ap;
>> +       struct fuse_writepage_args *wpa;
>> +       struct fuse_args_pages *ap;
>>         struct inode *inode = wpc->inode;
>>         struct fuse_inode *fi = get_fuse_inode(inode);
>>         struct fuse_conn *fc = get_fuse_conn(inode);
>>         loff_t offset = offset_in_folio(folio, pos);
>>
>> -       WARN_ON_ONCE(!data);
>> +       if (WARN_ON_ONCE(!data))
>> +               return -EIO;
>
> imo this WARN_ON_ONCE (and the one below) should be left as is instead
> of embedded in the "if" construct. The data pointer passed in is set
> by fuse and as such, we're able to reasonably guarantee that data is a
> valid pointer. Looking at other examples of WARN_ON in the fuse
> codebase, the places where an "if" construct is used are for cases
> where the assumptions that are made are more delicate (eg folio
> mapping state, in fuse_try_move_folio()) and less clearly obvious. I
> think this WARN_ON_ONCE here and below should be left as is.

OK, thank you for your feedback, Joanne.  So, if Miklos agrees with that,
I guess we can drop this patch.

Cheers,
-- 
Luís

>
>
> Thanks,
> Joanne
>
>> +
>> +       wpa = data->wpa;
>> +       ap = &wpa->ia.ap;
>>
>>         if (!data->ff) {
>>                 data->ff = fuse_write_file_get(fi);
>> @@ -2182,7 +2186,8 @@ 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 (WARN_ON_ONCE(!data))
>> +               return error ? error : -EIO;
>>
>>         if (data->wpa) {
>>                 WARN_ON(!data->wpa->ia.ap.num_folios);

Re: [PATCH v2] fuse: prevent possible NULL pointer dereference in fuse_iomap_writeback_{range,submit}()

[Darrick J. Wong]

On Wed, Sep 03, 2025 at 09:08:12PM +0100, Luis Henriques wrote:
> On Wed, Sep 03 2025, Joanne Koong wrote:
> 
> > On Wed, Sep 3, 2025 at 1:35 AM Luis Henriques <luis@igalia.com> wrote:
> >>
> >> These two functions make use of the WARN_ON_ONCE() macro to help debugging
> >> a NULL wpc->wb_ctx.  However, this doesn't prevent the possibility of NULL
> >> pointer dereferences in the code.  This patch adds some extra defensive
> >> checks to avoid these NULL pointer accesses.
> >>
> >> Fixes: ef7e7cbb323f ("fuse: use iomap for writeback")
> >> Signed-off-by: Luis Henriques <luis@igalia.com>
> >> ---
> >> Hi!
> >>
> >> This v2 results from Joanne's inputs -- I now believe that it is better to
> >> keep the WARN_ON_ONCE() macros, but it's still good to try to minimise
> >> the undesirable effects of a NULL wpc->wb_ctx.
> >>
> >> I've also added the 'Fixes:' tag to the commit message.
> >>
> >>  fs/fuse/file.c | 13 +++++++++----
> >>  1 file changed, 9 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> >> index 5525a4520b0f..990c287bc3e3 100644
> >> --- a/fs/fuse/file.c
> >> +++ b/fs/fuse/file.c
> >> @@ -2135,14 +2135,18 @@ static ssize_t fuse_iomap_writeback_range(struct iomap_writepage_ctx *wpc,
> >>                                           unsigned len, u64 end_pos)
> >>  {
> >>         struct fuse_fill_wb_data *data = wpc->wb_ctx;
> >> -       struct fuse_writepage_args *wpa = data->wpa;
> >> -       struct fuse_args_pages *ap = &wpa->ia.ap;
> >> +       struct fuse_writepage_args *wpa;
> >> +       struct fuse_args_pages *ap;
> >>         struct inode *inode = wpc->inode;
> >>         struct fuse_inode *fi = get_fuse_inode(inode);
> >>         struct fuse_conn *fc = get_fuse_conn(inode);
> >>         loff_t offset = offset_in_folio(folio, pos);
> >>
> >> -       WARN_ON_ONCE(!data);
> >> +       if (WARN_ON_ONCE(!data))
> >> +               return -EIO;
> >
> > imo this WARN_ON_ONCE (and the one below) should be left as is instead
> > of embedded in the "if" construct. The data pointer passed in is set
> > by fuse and as such, we're able to reasonably guarantee that data is a
> > valid pointer. Looking at other examples of WARN_ON in the fuse
> > codebase, the places where an "if" construct is used are for cases
> > where the assumptions that are made are more delicate (eg folio
> > mapping state, in fuse_try_move_folio()) and less clearly obvious. I
> > think this WARN_ON_ONCE here and below should be left as is.
> 
> OK, thank you for your feedback, Joanne.  So, if Miklos agrees with that,
> I guess we can drop this patch.

AFAICT, this function can only be called by other iomap-using functions
in file.c, and those other functions always set
iomap_writepage_ctx::wb_ctx so I /think/ the assertions aren't necessary
at all...

> Cheers,
> -- 
> Luís
> 
> >
> >
> > Thanks,
> > Joanne
> >
> >> +
> >> +       wpa = data->wpa;
> >> +       ap = &wpa->ia.ap;
> >>
> >>         if (!data->ff) {

...because if someone fails to set wpc->wb_ctx, this line will crash the
kernel at least as much as the WARN_ON would.  IOWs, the WARN_ONs aren't
necessary but I don't think they hurt much.

You could introduce a CONFIG_FUSE_DEBUG option and hide some assertions
and whatnot behind it, ala CONFIG_FUSE_IOMAP_DEBUG*:

https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/tree/fs/fuse/iomap_priv.h?h=djwong-wtf&id=170269a48ae83ea7ce1e23ea5ff39995600efff0

--D

> >>                 data->ff = fuse_write_file_get(fi);
> >> @@ -2182,7 +2186,8 @@ 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 (WARN_ON_ONCE(!data))
> >> +               return error ? error : -EIO;
> >>
> >>         if (data->wpa) {
> >>                 WARN_ON(!data->wpa->ia.ap.num_folios);
> 
> 

Re: [PATCH v2] fuse: prevent possible NULL pointer dereference in fuse_iomap_writeback_{range,submit}()

[Joanne Koong]

On Wed, Sep 3, 2025 at 1:48 PM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Wed, Sep 03, 2025 at 09:08:12PM +0100, Luis Henriques wrote:
> > On Wed, Sep 03 2025, Joanne Koong wrote:
> >
> > > On Wed, Sep 3, 2025 at 1:35 AM Luis Henriques <luis@igalia.com> wrote:
> > >>
> > >> These two functions make use of the WARN_ON_ONCE() macro to help debugging
> > >> a NULL wpc->wb_ctx.  However, this doesn't prevent the possibility of NULL
> > >> pointer dereferences in the code.  This patch adds some extra defensive
> > >> checks to avoid these NULL pointer accesses.
> > >>
> > >> Fixes: ef7e7cbb323f ("fuse: use iomap for writeback")
> > >> Signed-off-by: Luis Henriques <luis@igalia.com>
> > >> ---
> > >> Hi!
> > >>
> > >> This v2 results from Joanne's inputs -- I now believe that it is better to
> > >> keep the WARN_ON_ONCE() macros, but it's still good to try to minimise
> > >> the undesirable effects of a NULL wpc->wb_ctx.
> > >>
> > >> I've also added the 'Fixes:' tag to the commit message.
> > >>
> > >>  fs/fuse/file.c | 13 +++++++++----
> > >>  1 file changed, 9 insertions(+), 4 deletions(-)
> > >>
> > >> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > >> index 5525a4520b0f..990c287bc3e3 100644
> > >> --- a/fs/fuse/file.c
> > >> +++ b/fs/fuse/file.c
> > >> @@ -2135,14 +2135,18 @@ static ssize_t fuse_iomap_writeback_range(struct iomap_writepage_ctx *wpc,
> > >>                                           unsigned len, u64 end_pos)
> > >>  {
> > >>         struct fuse_fill_wb_data *data = wpc->wb_ctx;
> > >> -       struct fuse_writepage_args *wpa = data->wpa;
> > >> -       struct fuse_args_pages *ap = &wpa->ia.ap;
> > >> +       struct fuse_writepage_args *wpa;
> > >> +       struct fuse_args_pages *ap;
> > >>         struct inode *inode = wpc->inode;
> > >>         struct fuse_inode *fi = get_fuse_inode(inode);
> > >>         struct fuse_conn *fc = get_fuse_conn(inode);
> > >>         loff_t offset = offset_in_folio(folio, pos);
> > >>
> > >> -       WARN_ON_ONCE(!data);
> > >> +       if (WARN_ON_ONCE(!data))
> > >> +               return -EIO;
> > >
> > > imo this WARN_ON_ONCE (and the one below) should be left as is instead
> > > of embedded in the "if" construct. The data pointer passed in is set
> > > by fuse and as such, we're able to reasonably guarantee that data is a
> > > valid pointer. Looking at other examples of WARN_ON in the fuse
> > > codebase, the places where an "if" construct is used are for cases
> > > where the assumptions that are made are more delicate (eg folio
> > > mapping state, in fuse_try_move_folio()) and less clearly obvious. I
> > > think this WARN_ON_ONCE here and below should be left as is.
> >
> > OK, thank you for your feedback, Joanne.  So, if Miklos agrees with that,
> > I guess we can drop this patch.

I think having the two lines "wpa = data->wpa;" and "ap = &wpa->ia.ap"
moved to below the "WARN_ON_ONCE(!data);" would still be useful

>
> AFAICT, this function can only be called by other iomap-using functions
> in file.c, and those other functions always set
> iomap_writepage_ctx::wb_ctx so I /think/ the assertions aren't necessary
> at all...
>
> > Cheers,
> > --
> > Luís
> >
> > >
> > >
> > > Thanks,
> > > Joanne
> > >
> > >> +
> > >> +       wpa = data->wpa;
> > >> +       ap = &wpa->ia.ap;
> > >>
> > >>         if (!data->ff) {
>
> ...because if someone fails to set wpc->wb_ctx, this line will crash the
> kernel at least as much as the WARN_ON would.  IOWs, the WARN_ONs aren't
> necessary but I don't think they hurt much.
>

Oh, I see. Actually, this explanation makes a lot of sense. When I was
looking at the other WARN_ON usages in fuse, I noticed they were also
used even if it's logically proven that the code path can never be
triggered. But I guess what you're saying is that WARN_ONs in general
should be used if it's otherwise somehow undetectable / non-obvious
that the condition is violated? That makes sense to me, and checks out
with the other fuse WARN_ON uses.

I'm fine with just removing the WARN_ON(!data) here and below. I think
I added some more WARN_ONs in my other fuse iomap patchset, so I'll
remove those as well when I send out a new version.

> You could introduce a CONFIG_FUSE_DEBUG option and hide some assertions
> and whatnot behind it, ala CONFIG_FUSE_IOMAP_DEBUG*:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/tree/fs/fuse/iomap_priv.h?h=djwong-wtf&id=170269a48ae83ea7ce1e23ea5ff39995600efff0
>

In that case, personally I'd much prefer removing the WARN_ONs here
than having a new config for it.

Thanks,
Joanne

> --D
>
> > >>                 data->ff = fuse_write_file_get(fi);
> > >> @@ -2182,7 +2186,8 @@ 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 (WARN_ON_ONCE(!data))
> > >> +               return error ? error : -EIO;
> > >>
> > >>         if (data->wpa) {
> > >>                 WARN_ON(!data->wpa->ia.ap.num_folios);
> >
> >

Re: [PATCH v2] fuse: prevent possible NULL pointer dereference in fuse_iomap_writeback_{range,submit}()

[Darrick J. Wong]

On Wed, Sep 03, 2025 at 03:32:40PM -0700, Joanne Koong wrote:
> On Wed, Sep 3, 2025 at 1:48 PM Darrick J. Wong <djwong@kernel.org> wrote:
> >
> > On Wed, Sep 03, 2025 at 09:08:12PM +0100, Luis Henriques wrote:
> > > On Wed, Sep 03 2025, Joanne Koong wrote:
> > >
> > > > On Wed, Sep 3, 2025 at 1:35 AM Luis Henriques <luis@igalia.com> wrote:
> > > >>
> > > >> These two functions make use of the WARN_ON_ONCE() macro to help debugging
> > > >> a NULL wpc->wb_ctx.  However, this doesn't prevent the possibility of NULL
> > > >> pointer dereferences in the code.  This patch adds some extra defensive
> > > >> checks to avoid these NULL pointer accesses.
> > > >>
> > > >> Fixes: ef7e7cbb323f ("fuse: use iomap for writeback")
> > > >> Signed-off-by: Luis Henriques <luis@igalia.com>
> > > >> ---
> > > >> Hi!
> > > >>
> > > >> This v2 results from Joanne's inputs -- I now believe that it is better to
> > > >> keep the WARN_ON_ONCE() macros, but it's still good to try to minimise
> > > >> the undesirable effects of a NULL wpc->wb_ctx.
> > > >>
> > > >> I've also added the 'Fixes:' tag to the commit message.
> > > >>
> > > >>  fs/fuse/file.c | 13 +++++++++----
> > > >>  1 file changed, 9 insertions(+), 4 deletions(-)
> > > >>
> > > >> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > > >> index 5525a4520b0f..990c287bc3e3 100644
> > > >> --- a/fs/fuse/file.c
> > > >> +++ b/fs/fuse/file.c
> > > >> @@ -2135,14 +2135,18 @@ static ssize_t fuse_iomap_writeback_range(struct iomap_writepage_ctx *wpc,
> > > >>                                           unsigned len, u64 end_pos)
> > > >>  {
> > > >>         struct fuse_fill_wb_data *data = wpc->wb_ctx;
> > > >> -       struct fuse_writepage_args *wpa = data->wpa;
> > > >> -       struct fuse_args_pages *ap = &wpa->ia.ap;
> > > >> +       struct fuse_writepage_args *wpa;
> > > >> +       struct fuse_args_pages *ap;
> > > >>         struct inode *inode = wpc->inode;
> > > >>         struct fuse_inode *fi = get_fuse_inode(inode);
> > > >>         struct fuse_conn *fc = get_fuse_conn(inode);
> > > >>         loff_t offset = offset_in_folio(folio, pos);
> > > >>
> > > >> -       WARN_ON_ONCE(!data);
> > > >> +       if (WARN_ON_ONCE(!data))
> > > >> +               return -EIO;
> > > >
> > > > imo this WARN_ON_ONCE (and the one below) should be left as is instead
> > > > of embedded in the "if" construct. The data pointer passed in is set
> > > > by fuse and as such, we're able to reasonably guarantee that data is a
> > > > valid pointer. Looking at other examples of WARN_ON in the fuse
> > > > codebase, the places where an "if" construct is used are for cases
> > > > where the assumptions that are made are more delicate (eg folio
> > > > mapping state, in fuse_try_move_folio()) and less clearly obvious. I
> > > > think this WARN_ON_ONCE here and below should be left as is.
> > >
> > > OK, thank you for your feedback, Joanne.  So, if Miklos agrees with that,
> > > I guess we can drop this patch.
> 
> I think having the two lines "wpa = data->wpa;" and "ap = &wpa->ia.ap"
> moved to below the "WARN_ON_ONCE(!data);" would still be useful

<shrug> Compilers are magic, they can rearrange the function unless you
explicitly put in barriers or data dependencies to prevent that. 8-)

> >
> > AFAICT, this function can only be called by other iomap-using functions
> > in file.c, and those other functions always set
> > iomap_writepage_ctx::wb_ctx so I /think/ the assertions aren't necessary
> > at all...
> >
> > > Cheers,
> > > --
> > > Luís
> > >
> > > >
> > > >
> > > > Thanks,
> > > > Joanne
> > > >
> > > >> +
> > > >> +       wpa = data->wpa;
> > > >> +       ap = &wpa->ia.ap;
> > > >>
> > > >>         if (!data->ff) {
> >
> > ...because if someone fails to set wpc->wb_ctx, this line will crash the
> > kernel at least as much as the WARN_ON would.  IOWs, the WARN_ONs aren't
> > necessary but I don't think they hurt much.
> >
> 
> Oh, I see. Actually, this explanation makes a lot of sense. When I was
> looking at the other WARN_ON usages in fuse, I noticed they were also
> used even if it's logically proven that the code path can never be
> triggered. But I guess what you're saying is that WARN_ONs in general
> should be used if it's otherwise somehow undetectable / non-obvious
> that the condition is violated? That makes sense to me, and checks out
> with the other fuse WARN_ON uses.
> 
> I'm fine with just removing the WARN_ON(!data) here and below. I think
> I added some more WARN_ONs in my other fuse iomap patchset, so I'll
> remove those as well when I send out a new version.

<nod>

> > You could introduce a CONFIG_FUSE_DEBUG option and hide some assertions
> > and whatnot behind it, ala CONFIG_FUSE_IOMAP_DEBUG*:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/tree/fs/fuse/iomap_priv.h?h=djwong-wtf&id=170269a48ae83ea7ce1e23ea5ff39995600efff0
> >
> 
> In that case, personally I'd much prefer removing the WARN_ONs here
> than having a new config for it.

<nod> I added it to mine because there are a lot of things that iomap
/can/ get cranky about, so it's useful to have a "BAD_DATA" macro that
screams when the fuse server feeds garbage to the kernel.

--D

> Thanks,
> Joanne
> 
> > --D
> >
> > > >>                 data->ff = fuse_write_file_get(fi);
> > > >> @@ -2182,7 +2186,8 @@ 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 (WARN_ON_ONCE(!data))
> > > >> +               return error ? error : -EIO;
> > > >>
> > > >>         if (data->wpa) {
> > > >>                 WARN_ON(!data->wpa->ia.ap.num_folios);
> > >
> > >

Re: [PATCH v2] fuse: prevent possible NULL pointer dereference in fuse_iomap_writeback_{range,submit}()

[Luis Henriques]

On Wed, Sep 03 2025, Joanne Koong wrote:

> On Wed, Sep 3, 2025 at 1:48 PM Darrick J. Wong <djwong@kernel.org> wrote:

>> ...because if someone fails to set wpc->wb_ctx, this line will crash the
>> kernel at least as much as the WARN_ON would.  IOWs, the WARN_ONs aren't
>> necessary but I don't think they hurt much.
>>
>
> Oh, I see. Actually, this explanation makes a lot of sense. When I was
> looking at the other WARN_ON usages in fuse, I noticed they were also
> used even if it's logically proven that the code path can never be
> triggered. But I guess what you're saying is that WARN_ONs in general
> should be used if it's otherwise somehow undetectable / non-obvious
> that the condition is violated? That makes sense to me, and checks out
> with the other fuse WARN_ON uses.
>
> I'm fine with just removing the WARN_ON(!data) here and below. I think
> I added some more WARN_ONs in my other fuse iomap patchset, so I'll
> remove those as well when I send out a new version.

I don't have a preference between v1 and v2 of this patch.  v1 removed the
WARNs because I don't think they are useful:

1. the assertions are never true, but
2. if they are, they are useless because we'll hit a NULL pointer
   dereference anyway.

v2 tries to fix the code assuming the assertions can be triggered.

So, yeah I'll just leave the 3 options (v1, v2, or do nothing) on the
table :-)

Cheers,
-- 
Luís

Re: [PATCH v2] fuse: prevent possible NULL pointer dereference in fuse_iomap_writeback_{range,submit}()

[Miklos Szeredi]

On Thu, 4 Sept 2025 at 10:24, Luis Henriques <luis@igalia.com> wrote:

> I don't have a preference between v1 and v2 of this patch.  v1 removed the
> WARNs because I don't think they are useful:
>
> 1. the assertions are never true, but
> 2. if they are, they are useless because we'll hit a NULL pointer
>    dereference anyway.
>
> v2 tries to fix the code assuming the assertions can be triggered.
>
> So, yeah I'll just leave the 3 options (v1, v2, or do nothing) on the
> table :-)

WARN_ON is a useful tool to document interface constrains.  But so is
dereferencing of a pointer.

V2 style WARN_ON should only be used if it's difficult to prove that
the condition will evaluate to false and we don't want the kernel to
crash in some unknown corner case.  AFAICS it is not the case here, so
I'd opt for v1.

Thanks,
Miklos