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
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
