linux-ext4 - Re: [PATCH v2 02/54] fs: add an icount

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: <xvjqgfecmhgb4ngzmjveo7w5iib2qnh2te4x7hmpr7cjtul6mq@utgtnbfuxhco>
Date: Wed, 27 Aug 2025 00:18:37 +0200
From: Mateusz Guzik <mjguzik@...il.com>
To: Josef Bacik <josef@...icpanda.com>
Cc: linux-fsdevel@...r.kernel.org, linux-btrfs@...r.kernel.org, 
	kernel-team@...com, linux-ext4@...r.kernel.org, linux-xfs@...r.kernel.org, 
	brauner@...nel.org, viro@...iv.linux.org.uk, amir73il@...il.com
Subject: Re: [PATCH v2 02/54] fs: add an icount_read helper

On Tue, Aug 26, 2025 at 11:39:02AM -0400, Josef Bacik wrote:
> Instead of doing direct access to ->i_count, add a helper to handle
> this. This will make it easier to convert i_count to a refcount later.
> 
> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> index 079b868552c2..46bfc543f946 100644
> --- a/fs/notify/fsnotify.c
> +++ b/fs/notify/fsnotify.c
> @@ -66,7 +66,7 @@ static void fsnotify_unmount_inodes(struct super_block *sb)
>  		 * removed all zero refcount inodes, in any case.  Test to
>  		 * be sure.
>  		 */
> -		if (!atomic_read(&inode->i_count)) {
> +		if (!icount_read(inode)) {
>  			spin_unlock(&inode->i_lock);
>  			continue;
>  		}
[snip]
> +static inline int icount_read(const struct inode *inode)
> +{
> +	return atomic_read(&inode->i_count);
> +}
> +
>  extern void iget_failed(struct inode *);
>  extern void clear_inode(struct inode *);
>  extern void __destroy_inode(struct inode *);

The placement issue I mentioned in another e-mail aside, I would
recommend further error-proofing this.

Above I quoted an example user which treats i_count == 0 as special.

While moving this into helpers is definitely a step in the right
direction, I think having consumer open-code this check is avoidably
error-prone.

Notably, as is there is nothing to indicate whether the consumer expects
the value to remain stable or is perhaps doing a quick check for other
reasons.

As such, specific naming aside, I would create 2 variants:
1. icount_read_unstable() -- the value can change from under you
arbitrarily. I don't there are any consumers for this sucker atm.
2. icount_read() -- the caller expects the transition 0<->1 is
guaranteed to not take place, notably if the value is found to be 0, it
stay at 0. to that end the caller is expected to hold the inode spinlock
*and* the fact that the lock is held is asserted on with lockdep.

All that aside, I think open-coding "is the inode unused" with an
explicit count check is bad form -- a dedicated helper for that would
also be nice.

My 3 CZK.