linux-ext4 - Re: [PATCH 03/10] fuse: use the iomap cache for iomap

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: <20260206042831.GK7686@frogsfrogsfrogs>
Date: Thu, 5 Feb 2026 20:28:31 -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 03/10] fuse: use the iomap cache for iomap_begin

On Thu, Feb 05, 2026 at 10:52:04AM -0800, Chris Mason wrote:
> "Darrick J. Wong" <djwong@...nel.org> wrote:
> > From: Darrick J. Wong <djwong@...nel.org>
> > 
> > Look inside the iomap cache to try to satisfy iomap_begin.
> > 
> 
> 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 705acf20a7890a2ba0ab24fe6bf523b99c1a1f32
> Author: Darrick J. Wong <djwong@...nel.org>
> 
> fuse: use the iomap cache for iomap_begin
> 
> Look inside the iomap cache to try to satisfy iomap_begin.
> 
> > diff --git a/fs/fuse/fuse_iomap.c b/fs/fuse/fuse_iomap.c
> > index eade8dad8a570..cf46824dc50bf 100644
> > --- a/fs/fuse/fuse_iomap.c
> > +++ b/fs/fuse/fuse_iomap.c
> 
> [ ... ]
> 
> > @@ -238,9 +239,14 @@ static inline bool fuse_iomap_check_mapping(const struct inode *inode,
> >  	const unsigned int blocksize = i_blocksize(inode);
> >  	uint64_t end;
> >
> > -	/* Type and flags must be known */
> > +	/*
> > +	 * Type and flags must be known.  Mapping type "retry cache" doesn't
> > +	 * use any of the other fields.
> > +	 */
> >  	if (BAD_DATA(!fuse_iomap_check_type(map->type)))
> >  		return false;
> > +	if (map->type == FUSE_IOMAP_TYPE_RETRY_CACHE)
> > +		return true;
> >  	if (BAD_DATA(!fuse_iomap_check_flags(map->flags)))
> >  		return false;
> 
> [ ... ]
> 
> > @@ -271,6 +277,14 @@ static inline bool fuse_iomap_check_mapping(const struct inode *inode,
> >  		if (BAD_DATA(map->addr == FUSE_IOMAP_NULL_ADDR))
> >  			return false;
> >  		break;
> > +	case FUSE_IOMAP_TYPE_RETRY_CACHE:
> > +		/*
> > +		 * We only accept cache retries if we have a cache to query.
> > +		 * There must not be a device addr.
> > +		 */
> > +		if (BAD_DATA(!fuse_inode_caches_iomaps(inode)))
> > +			return false;
> > +		fallthrough;
> >  	case FUSE_IOMAP_TYPE_DELALLOC:
> >  	case FUSE_IOMAP_TYPE_HOLE:
> >  	case FUSE_IOMAP_TYPE_INLINE:
> 
> Does the validation at lines 280-287 ever execute? Looking at
> fuse_iomap_check_mapping(), the early return at line 248-249 handles
> FUSE_IOMAP_TYPE_RETRY_CACHE:
> 
>     if (map->type == FUSE_IOMAP_TYPE_RETRY_CACHE)
>         return true;

Indeed, this means we can't ever hit the RETRY_CACHE case in the switch
statement.

> This means the function returns before reaching the switch statement that
> contains the new RETRY_CACHE case. The validation check 'if (BAD_DATA(!
> fuse_inode_caches_iomaps(inode))) return false;' at lines 284-285 appears
> to be dead code because RETRY_CACHE mappings are accepted without verifying
> the inode has a cache, contradicting the comment "We only accept cache
> retries if we have a cache to query."
> 
> Either the fuse_inode_caches_iomaps check should move to the early return
> path like this:
> 
>     if (map->type == FUSE_IOMAP_TYPE_RETRY_CACHE) {
>         if (BAD_DATA(!fuse_inode_caches_iomaps(inode)))
>             return false;
>         return true;
>     }

That's the simplest resolution to the problem; I'll go with that.

> or the early return should be removed to let RETRY_CACHE fall through to
> the switch statement validation.

Nah, we definitely don't need to proceed with more validation.  Good
catch!

--D