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: <20260205185327.1776495-1-clm@meta.com>
Date: Thu, 5 Feb 2026 10:52:04 -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 03/10] fuse: use the iomap cache for iomap_begin

"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;

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;
    }

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