linux-ext4 - Re: [PATCH 11/11] fsverity: use a hashtable to find the fsverity

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: <20260125013104.GA2255@sol>
Date: Sat, 24 Jan 2026 17:31:04 -0800
From: Eric Biggers <ebiggers@...nel.org>
To: Christoph Hellwig <hch@....de>
Cc: Al Viro <viro@...iv.linux.org.uk>,
	Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
	David Sterba <dsterba@...e.com>, Theodore Ts'o <tytso@....edu>,
	Jaegeuk Kim <jaegeuk@...nel.org>, Chao Yu <chao@...nel.org>,
	Andrey Albershteyn <aalbersh@...hat.com>,
	"Matthew Wilcox (Oracle)" <willy@...radead.org>,
	linux-fsdevel@...r.kernel.org, linux-btrfs@...r.kernel.org,
	linux-ext4@...r.kernel.org, linux-f2fs-devel@...ts.sourceforge.net,
	fsverity@...ts.linux.dev
Subject: Re: [PATCH 11/11] fsverity: use a hashtable to find the fsverity_info

On Thu, Jan 22, 2026 at 09:22:07AM +0100, Christoph Hellwig wrote:
> +int fsverity_set_info(struct fsverity_info *vi)
>  {
> -	/*
> -	 * Multiple tasks may race to set the inode's verity info pointer, so
> -	 * use cmpxchg_release().  This pairs with the smp_load_acquire() in
> -	 * fsverity_get_info().  I.e., publish the pointer with a RELEASE
> -	 * barrier so that other tasks can ACQUIRE it.
> -	 */
> -	if (cmpxchg_release(fsverity_info_addr(inode), NULL, vi) != NULL) {
> -		/* Lost the race, so free the verity info we allocated. */
> -		fsverity_free_info(vi);
> -		/*
> -		 * Afterwards, the caller may access the inode's verity info
> -		 * directly, so make sure to ACQUIRE the winning verity info.
> -		 */
> -		(void)fsverity_get_info(inode);
> -	}
> +	return rhashtable_lookup_insert_fast(&fsverity_info_hash,
> +			&vi->rhash_head, fsverity_info_hash_params);
>  }
>  
> -void fsverity_free_info(struct fsverity_info *vi)
> +struct fsverity_info *__fsverity_get_info(const struct inode *inode)
>  {
> -	if (!vi)
> -		return;
> -	kfree(vi->tree_params.hashstate);
> -	kvfree(vi->hash_block_verified);
> -	kmem_cache_free(fsverity_info_cachep, vi);
> +	return rhashtable_lookup_fast(&fsverity_info_hash, &inode,
> +			fsverity_info_hash_params);
[...]
> +	/*
> +	 * Multiple tasks may race to set the inode's verity info, in which case
> +	 * we might find an existing fsverity_info in the hash table.
> +	 */
> +	found = rhashtable_lookup_get_insert_fast(&fsverity_info_hash,
> +			&vi->rhash_head, fsverity_info_hash_params);
> +	if (found) {
> +		fsverity_free_info(vi);
> +		if (IS_ERR(found))
> +			err = PTR_ERR(found);
> +	}

Is there any explanation for why it's safe to use the *_fast variants of
these functions?

>   * fsverity_active() - do reads from the inode need to go through fs-verity?
>   * @inode: inode to check
>   *
> - * This checks whether the inode's verity info has been set.
> - *
> - * Filesystems call this from ->readahead() to check whether the pages need to
> - * be verified or not.  Don't use IS_VERITY() for this purpose; it's subject to
> - * a race condition where the file is being read concurrently with
> - * FS_IOC_ENABLE_VERITY completing.  (S_VERITY is set before the verity info.)
> + * This checks whether the inode's verity info has been set, and reads need
> + * to verify the verity information.
>   *
>   * Return: true if reads need to go through fs-verity, otherwise false
>   */
>  static inline bool fsverity_active(const struct inode *inode)
>  {
> -	return fsverity_get_info(inode) != NULL;
> +	/*
> +	 * The memory barrier pairs with the try_cmpxchg in set_mask_bits used
> +	 * to set the S_VERITY bit in i_flags.
> +	 */
> +	smp_mb();
> +	return IS_ENABLED(CONFIG_FS_VERITY) && IS_VERITY(inode);
> +}

This looks incorrect.  The memory barrier is needed after reading the
flag, not before.  (See how smp_load_acquire() works.)

Also, it's needed only for verity inodes.

Maybe do:

	if (IS_ENABLED(CONFIG_FS_VERITY) && IS_VERITY(inode)) {
		/*
                 * This pairs with the try_cmpxchg in set_mask_bits()
                 * used to set the S_VERITY bit in i_flags.
		 */
		smp_mb();
		return true;
	}
	return false;

- Eric