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

[Luis Henriques <luis@igalia.com>]

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);