linux-ext4 - Re: [PATCH 07/10] fuse: enable iomap cache management

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20260205184044.1551228-1-clm@meta.com>
Date: Thu, 5 Feb 2026 10:33:26 -0800
From: Chris Mason <clm@...a.com>
To: "Darrick J. Wong" <djwong@...nel.org>
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 07/10] fuse: enable iomap cache management

"Darrick J. Wong" <djwong@...nel.org> wrote:
> From: Darrick J. Wong <djwong@...nel.org>
> 
> Provide a means for the fuse server to upload iomappings to the kernel
> and invalidate them.  This is how we enable iomap caching for better
> performance.  This is also required for correct synchronization between
> pagecache writes and writeback.
> 

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 84c726dd312b652c0fd2d4f43d65eefd16bfa901
Author: Darrick J. Wong <djwong@...nel.org>

fuse: enable iomap cache management

This commit provides a means for the fuse server to upload iomappings to
the kernel and invalidate them, enabling iomap caching for better
performance and ensuring correct synchronization between pagecache writes
and writeback.

> diff --git a/fs/fuse/fuse_iomap.c b/fs/fuse/fuse_iomap.c
> index b56718dd256a6..dcadd66f6ec95 100644
> --- a/fs/fuse/fuse_iomap.c
> +++ b/fs/fuse/fuse_iomap.c

[ ... ]

> +int fuse_iomap_inval(struct fuse_conn *fc,
> +		     const struct fuse_iomap_inval_out *outarg)
> +{
> +	struct inode *inode;
> +	struct fuse_inode *fi;
> +	int ret = 0, ret2 = 0;
> +
> +	if (!fc->iomap)
> +		return -EINVAL;
> +
> +	down_read(&fc->killsb);
> +	inode = fuse_ilookup(fc, outarg->nodeid, NULL);
> +	if (!inode) {
> +		ret = -ESTALE;
> +		goto out_sb;
> +	}
> +
> +	fi = get_fuse_inode(inode);
> +	if (BAD_DATA(fi->orig_ino != outarg->attr_ino)) {
> +		ret = -EINVAL;
> +		goto out_inode;
> +	}
> +
> +	if (fuse_is_bad(inode)) {
> +		ret = -EIO;
> +		goto out_inode;
> +	}
> +
> +	if (!fuse_iomap_inval_validate(inode, outarg->write_offset,
> +				       outarg->write_length)) {
> +		ret = -EFSCORRUPTED;
> +		goto out_inode;
> +	}
> +
> +	if (!fuse_iomap_inval_validate(inode, outarg->read_offset,
> +				       outarg->read_length)) {
> +		ret = -EFSCORRUPTED;
> +		goto out_inode;
> +	}
> +
> +	fuse_iomap_cache_lock(inode);
> +	if (outarg->read_length)
> +		ret2 = fuse_iomap_cache_remove(inode, READ_MAPPING,
> +					       outarg->read_offset,
> +					       outarg->read_length);
> +	if (outarg->write_length)
> +		ret = fuse_iomap_cache_remove(inode, WRITE_MAPPING,
> +					      outarg->write_offset,
> +					      outarg->write_length);
> +	fuse_iomap_cache_unlock(inode);
>                    ^^^^

Can fuse_iomap_cache_lock() dereference a NULL pointer here? Looking at
fuse_iomap_upsert() above, there's a check for fuse_inode_caches_iomaps()
before accessing the cache and allocating if necessary. In fuse_iomap_inval(),
there's no such check before calling fuse_iomap_cache_lock().

fuse_iomap_cache_lock() accesses fi->cache and calls down_write(&ic->ic_lock),
which will crash if fi->cache is NULL. This could happen when a fuse server
sends FUSE_NOTIFY_IOMAP_INVAL for an inode that never had mappings upserted
to it.

Should fuse_iomap_inval() check fuse_inode_caches_iomaps(inode) before
calling fuse_iomap_cache_lock()? If the cache doesn't exist, there's
nothing to invalidate, so returning early (perhaps with 0 or -EINVAL)
would be appropriate.