Re: [f2fs-dev] [PATCH 11/11] fsverity: use a hashtable to find the fsverity_info

[Eric Biggers via Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net>]

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


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel