On Wed, Sep 03 2025, Joanne Koong wrote:
> On Wed, Sep 3, 2025 at 1:35 AM Luis Henriques <luis@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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);
