Message-ID: <20260206020832.GE7686@frogsfrogsfrogs>
Date: Thu, 5 Feb 2026 18:08:32 -0800
From: "Darrick J. Wong" <djwong@...nel.org>
To: Chris Mason <clm@...a.com>
Cc: miklos@...redi.hu, joannelkoong@...il.com, bernd@...ernd.com,
	neal@...pa.dev, linux-ext4@...r.kernel.org,
	linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH 12/31] fuse: implement direct IO with iomap

On Thu, Feb 05, 2026 at 11:19:11AM -0800, Chris Mason wrote:
> "Darrick J. Wong" <djwong@...nel.org> wrote:
> > From: Darrick J. Wong <djwong@...nel.org>
> > 
> > Start implementing the fuse-iomap file I/O paths by adding direct I/O
> > support and all the signalling flags that come with it.  Buffered I/O
> > is much more complicated, so we leave that to a subsequent patch.
> > 
> 
> 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

I should mention: I appreciate you rolling out the AI reviews by slowly
expanding the number of trees you scan for bugs.

> 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 fab88aeb077db04c5f4fdbaff31a58f4dd9450c8
> Author: Darrick J. Wong <djwong@...nel.org>
> 
> fuse: implement direct IO with iomap
> 
> Start implementing the fuse-iomap file I/O paths by adding direct I/O
> support and all the signalling flags that come with it. Buffered I/O is
> much more complicated, so we leave that to a subsequent patch.
> 
> > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > index bec299865d8fe..07be70d314eff 100644
> > --- a/fs/fuse/file.c
> > +++ b/fs/fuse/file.c
> 
> [ ... ]
> 
> > @@ -408,7 +416,7 @@ void fuse_sync_release(struct fuse_inode *fi, struct fuse_file *ff,
> >  {
> >  	WARN_ON(refcount_read(&ff->count) > 1);
> >  	fuse_prepare_release(fi, ff, flags, FUSE_RELEASE, true);
> > -	fuse_file_put(ff, true);
> > +	fuse_file_put(ff, &fi->inode, true);
> >  }
> >  EXPORT_SYMBOL_GPL(fuse_sync_release);
> 
> Can this code dereference a NULL pointer when fi is NULL?

Yikes, yes it can...

> Looking at fuse_create_open in dir.c, when fuse_iget() fails, it calls:
> 
>     fuse_sync_release(NULL, ff, flags);
> 
> The fuse_prepare_release function explicitly handles fi == NULL with
> the comment "Inode is NULL on error path of fuse_create_open()".
> 
> However, the new call to fuse_file_put passes &fi->inode, which
> evaluates to &NULL->inode. While the inode parameter is unused in the
> current commit's implementation of fuse_file_put, the API contract is
> broken.

...so this is a very good catch!  Though as it turns out we don't need
this change to fuse_file_put until we actually add fuse_iomap_release()
several patchsets from now, so I think I'll revert this part of the
patch.

> In commit 5f4e9030b26b (fuse: enable iomap), fuse_file_put uses the
> inode parameter to call fuse_inode_has_iomap(inode). Since struct inode
> is the first member of struct fuse_inode (offsetof == 0), &fi->inode
> evaluates to NULL, and fuse_inode_has_iomap(NULL) will dereference NULL.
> 
> > diff --git a/fs/fuse/fuse_iomap.c b/fs/fuse/fuse_iomap.c
> > index bc55e87751215..9796e3b994565 100644
> > --- a/fs/fuse/fuse_iomap.c
> > +++ b/fs/fuse/fuse_iomap.c
> 
> [ ... ]
> 
> > @@ -545,6 +551,120 @@ static const struct iomap_ops fuse_iomap_ops = {
> >  	.iomap_end		= fuse_iomap_end,
> >  };
> >
> > +static inline bool
> > +fuse_should_send_iomap_ioend(const struct fuse_mount *fm,
> > +			     const struct fuse_iomap_ioend_in *inarg)
> > +{
> > +	/* Not implemented on fuse server */
> > +	if (fm->fc->iomap_conn.no_ioend)
> > +		return false;
> > +
> > +	/* Always send an ioend for errors. */
> > +	if (inarg->error)
> > +		return true;
> > +
> > +	/* Send an ioend if we performed an IO involving metadata changes. */
> > +	return inarg->written > 0 &&
> > +	       (inarg->flags & (FUSE_IOMAP_IOEND_SHARED |
> > +				FUSE_IOMAP_IOEND_UNWRITTEN |
> > +				FUSE_IOMAP_IOEND_APPEND));
> > +}
> 
> [ ... ]
> 
> > +static int fuse_iomap_ioend(struct inode *inode, loff_t pos, size_t written,
> > +			    int error, unsigned ioendflags,
> > +			    struct block_device *bdev, sector_t new_addr)
> > +{
> > +	struct fuse_inode *fi = get_fuse_inode(inode);
> > +	struct fuse_mount *fm = get_fuse_mount(inode);
> > +	struct fuse_iomap_ioend_in inarg = {
> > +		.flags = ioendflags,
> > +		.error = error,
> > +		.attr_ino = fi->orig_ino,
> > +		.pos = pos,
> > +		.written = written,
> > +		.dev = fuse_iomap_find_backing_id(fm->fc, bdev),
> > +		.new_addr = new_addr,
> > +	};
> > +	struct fuse_iomap_ioend_out outarg = { };
> > +
> > +	if (fuse_ioend_is_append(fi, pos, written))
> > +		inarg.flags |= FUSE_IOMAP_IOEND_APPEND;
> > +
> > +	if (fuse_should_send_iomap_ioend(fm, &inarg)) {
> > +		FUSE_ARGS(args);
> > +		int iomap_error;
> > +
> > +		args.opcode = FUSE_IOMAP_IOEND;
> > +		args.nodeid = get_node_id(inode);
> > +		args.in_numargs = 1;
> > +		args.in_args[0].size = sizeof(inarg);
> > +		args.in_args[0].value = &inarg;
> > +		args.out_numargs = 1;
> > +		args.out_args[0].size = sizeof(outarg);
> > +		args.out_args[0].value = &outarg;
> > +		iomap_error = fuse_simple_request(fm, &args);
> > +		switch (iomap_error) {
> > +		case -ENOSYS:
> > +			/*
> > +			 * fuse servers can return ENOSYS if ioend processing
> > +			 * is never needed for this filesystem.  Don't pass
> > +			 * that up to iomap.
> > +			 */
> > +			fm->fc->iomap_conn.no_ioend = 1;
> > +			break;
> > +		case 0:
> > +			break;
> > +		default:
> > +			/*
> > +			 * If the write IO failed, return the failure code to
> > +			 * the caller no matter what happens with the ioend.
> > +			 * If the write IO succeeded but the ioend did not,
> > +			 * pass the new error up to the caller.
> > +			 */
> > +			if (!error)
> > +				error = iomap_error;
> > +			break;
> > +		}
> > +	}
> > +
> > +	/*
> > +	 * Pass whatever error iomap gave us (or any new errors since then)
> > +	 * back to iomap.
> > +	 */
> > +	if (error)
> > +		return error;
> > +
> > +	/*
> > +	 * If there weren't any ioend errors, update the incore isize, which
> > +	 * confusingly takes the new i_size as "pos".
> > +	 */
> > +	fi->i_disk_size = outarg.newsize;
> > +	fuse_write_update_attr(inode, pos + written, written);
> > +	return 0;
> > +}
> 
> When the fuse server returns -ENOSYS (indicating it doesn't implement
> FUSE_IOMAP_IOEND), the code sets fm->fc->iomap_conn.no_ioend = 1 but
> does NOT return early. It falls through to the update code where
> 'outarg' was zero-initialized and never filled by the server.
> 
> This corrupts i_disk_size by setting it to 0:
> 
>     fi->i_disk_size = outarg.newsize;  // outarg.newsize == 0
> 
> On subsequent writes, fuse_should_send_iomap_ioend() returns false due
> to no_ioend being set, so the code skips the server call and again uses
> the zero-initialized outarg.newsize.

Yeah.  outarg should be initialized with fi->i_disk_size, though I think
this bug is benign because no_ioend being set to 1 in that ENOSYS case
means that this function doesn't do much of anything after that.

> The i_disk_size field is used by fuse_ioend_is_append() to detect
> append writes, so corrupting it to 0 will cause incorrect append
> detection for all subsequent writes.
> 
> Should the code return early when the request is not sent or returns
> -ENOSYS, or skip the i_disk_size update in this case?
> 
> > +static int fuse_iomap_ilock_iocb(const struct kiocb *iocb,
> > +				 enum fuse_ilock_type type)
> > +{
> > +	struct inode *inode = file_inode(iocb->ki_filp);
> > +
> > +	if (iocb->ki_flags & IOCB_NOWAIT) {
> > +		switch (type) {
> > +		case SHARED:
> > +			return inode_trylock_shared(inode) ? 0 : -EAGAIN;
> > +		case EXCL:
> > +			return inode_trylock(inode) ? 0 : -EAGAIN;
> > +		default:
> > +			ASSERT(0);
> > +			return -EIO;
> > +		}
> > +
> > +		return 0;
> > +	}
> 
> This isn't a bug, but the 'return 0;' after the switch statement inside
> the IOCB_NOWAIT block is unreachable since every switch case returns.

gcc is too stupid to detect that it's impossible to reach this case and
whines about the lack of a return.

--D

Powered by blists - more mailing lists