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