Re: [PATCH 12/31] fuse: implement direct IO with iomap

[Chris Mason <clm@meta.com>]

"Darrick J. Wong" <djwong@kernel.org> wrote:
> From: Darrick J. Wong <djwong@kernel.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

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@kernel.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?

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.

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.

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.