Re: [PATCH 24/31] fuse: implement inline data file IO via iomap

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Feb 05, 2026 at 11:01:07AM -0800, Chris Mason wrote:
> "Darrick J. Wong" <djwong@kernel.org> wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Implement inline data file IO by issuing FUSE_READ/FUSE_WRITE commands
> > in response to an inline data mapping.
> 
> Hi everyone,
> 
> I'm trying out my AI review prompts on a few more trees, and I ran it
> on the fuse-iomap-cache branch:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=fuse-iomap-cache
> 
> This was the latest email lore could find, apologies if I grabbed the wrong one.
> 
> Hopefully the review below is useful, please let me know if there are places it
> has gone wildly wrong.  I kept the header to make it obvious exactly which
> commit I'd run through:
> 
> commit 1263eae0852acb1359182377ad94d570701e2274
> Author: Darrick J. Wong <djwong@kernel.org>
> 
> fuse: implement inline data file IO via iomap
> 
> Implement inline data file IO by issuing FUSE_READ/FUSE_WRITE commands
> in response to an inline data mapping.
> 
> > diff --git a/fs/fuse/fuse_iomap.c b/fs/fuse/fuse_iomap.c
> > --- a/fs/fuse/fuse_iomap.c
> > +++ b/fs/fuse/fuse_iomap.c
> > @@ -398,6 +398,150 @@ fuse_iomap_find_dev(struct fuse_conn *fc, const struct fuse_iomap_io *map)
> >  	return ret;
> >  }
> >
> > +/*
> > + * Use the FUSE_READ command to read inline file data from the fuse server.
> > + * Note that there's no file handle attached, so the fuse server must be able
> > + * to reconnect to the inode via the nodeid.
> > + */
> > +static int fuse_iomap_inline_read(struct inode *inode, loff_t pos,
> > +				  loff_t count, struct iomap *iomap)
> > +{
> > +	struct fuse_read_in in = {
> > +		.offset = pos,
> > +		.size = count,
> > +	};
> > +	struct fuse_inode *fi = get_fuse_inode(inode);
> > +	struct fuse_mount *fm = get_fuse_mount(inode);
> > +	FUSE_ARGS(args);
> > +	ssize_t ret;
> > +
> > +	if (BAD_DATA(!iomap_inline_data_valid(iomap)))
> > +		return -EFSCORRUPTED;
> 
> Does this code leak memory when the BAD_DATA check fails?  Looking at
> the call chain:
> 
> fuse_iomap_set_inline()->fuse_iomap_inline_alloc() allocates
> iomap->inline_data via kvzalloc.  If the iomap_inline_data_valid check
> fails (i.e., iomap->length > PAGE_SIZE - offset_in_page(iomap->inline_data)),
> the function returns -EFSCORRUPTED without freeing iomap->inline_data.
> 
> Other error paths in this function (around the fuse_simple_request call)
> properly call fuse_iomap_inline_free(iomap) before returning errors.
> 
> The caller fuse_iomap_set_inline doesn't clean up iomap->inline_data on
> error in the read path (line 538: 'if (err) return err;'), so the
> allocation is leaked when a FUSE server returns an IOMAP_INLINE mapping
> with length > PAGE_SIZE.

Yep, it does leak the ioend and the bio.  I'll amend this to call
fuse_iomap_inline_free on its way out.

> > +
> > +	args.opcode = FUSE_READ;
> > +	args.nodeid = fi->nodeid;
> > +	args.in_numargs = 1;
> > +	args.in_args[0].size = sizeof(in);
> > +	args.in_args[0].value = &in;
> > +	args.out_argvar = true;
> > +	args.out_numargs = 1;
> > +	args.out_args[0].size = count;
> > +	args.out_args[0].value = iomap_inline_data(iomap, pos);
> > +
> > +	ret = fuse_simple_request(fm, &args);
> > +	if (ret < 0) {
> > +		fuse_iomap_inline_free(iomap);
> > +		return ret;
> > +	}
> > +	/* no readahead means something bad happened */
> > +	if (ret == 0) {
> > +		fuse_iomap_inline_free(iomap);
> > +		return -EIO;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> [ ... ]
> 
> > @@ -511,8 +663,28 @@ static int fuse_iomap_end(struct inode *inode, loff_t pos, loff_t count,
> >  {
> >  	struct fuse_inode *fi = get_fuse_inode(inode);
> >  	struct fuse_mount *fm = get_fuse_mount(inode);
> > +	struct iomap_iter *iter = container_of(iomap, struct iomap_iter, iomap);
> > +	struct iomap *srcmap = &iter->srcmap;
> 
> Can this container_of usage corrupt memory?  Looking at
> fuse_iomap_writeback_range, it calls fuse_iomap_end with &write_iomap,
> which is a stack-local struct iomap variable (not embedded in an
> iomap_iter):
> 
> fuse_iomap_writeback_range()
>     struct iomap write_iomap = { };
>     ...
>     fuse_iomap_end(inode, offset, len, 0,
>                    FUSE_IOMAP_OP_WRITEBACK, &write_iomap);
> 
> The container_of macro computes an invalid pointer by subtracting the
> offset of the iomap member from the address, resulting in iter pointing
> to garbage memory on the stack.  Subsequently, accessing iter->srcmap
> reads from invalid memory, potentially causing undefined behavior or
> crashes.
> 
> The iomap core calls fuse_iomap_end via fuse_iomap_ops where iomap IS
> properly embedded in iomap_iter, but the direct call from
> fuse_iomap_writeback_range violates this assumption.

Oops, that's a severe bug.  fuse_iomap_writeback_range should indeed
define a whole iomap_iter instead of just the iomap.  I'll fix that,
thanks for pointing out these bugs.

--D