linux-kernel - Re: [PATCH] fs: optimize chown_common by skipping unnecessary ownership changes

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] [day] [month] [year] [list]

Message-ID: <uh2mcjliqvhew2myuvi4hyvpwv5a7bnnzqlptfpwq5gguiskad@ciafu7tfwr4h>
Date: Thu, 13 Nov 2025 03:08:47 +0100
From: Mateusz Guzik <mjguzik@...il.com>
To: jayxu1990@...il.com
Cc: viro@...iv.linux.org.uk, brauner@...nel.org, jack@...e.cz, 
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org, rdlee.upstream@...il.com, 
	avnerkhan@...xas.edu
Subject: Re: [PATCH] fs: optimize chown_common by skipping unnecessary
 ownership changes

On Thu, Nov 13, 2025 at 09:34:49AM +0800, jayxu1990@...il.com wrote:
> From: Jay Xu <jayxu1990@...il.com>
> 
> Add early return optimization to chown_common() when the requested
> uid/gid already matches the current inode ownership. This avoids
> calling notify_change() and associated filesystem operations when
> no actual change is needed.
> 

If this is useful in practice, then so is probably chmod.

> The check is performed after acquiring the inode lock to ensure
> atomicity and uses the kernel's uid_eq()/gid_eq() functions for
> proper comparison.
> 
> This optimization provides several benefits:
> - Reduces unnecessary filesystem metadata updates and journal writes
> - Prevents redundant storage I/O when files are on persistent storage
> - Improves performance for recursive chown operations that encounter
>   files with already-correct ownership
> - Avoids invoking security hooks and filesystem-specific setattr
>   operations when no change is required

The last bit is a breaking change in behavior though. For example right
now invoking chown 0:0 /bin as an unprivileged user fails. It will succeed
with your patch.

iow you can't avoid any of the security checks.

However, if there are real workloads which chown/chmod to the current
value already, perhaps it would make sense for the routine to track if
anything changed and if not to avoid dirtying the inode, which still
might save on I/O.

> 
> Signed-off-by: Jay Xu <jayxu1990@...il.com>
> ---
>  fs/open.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/fs/open.c b/fs/open.c
> index 3d64372ecc67..82bde70c6c08 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -761,6 +761,7 @@ int chown_common(const struct path *path, uid_t user, gid_t group)
>  	struct iattr newattrs;
>  	kuid_t uid;
>  	kgid_t gid;
> +	bool needs_update = false;
>  
>  	uid = make_kuid(current_user_ns(), user);
>  	gid = make_kgid(current_user_ns(), group);
> @@ -779,6 +780,17 @@ int chown_common(const struct path *path, uid_t user, gid_t group)
>  	error = inode_lock_killable(inode);
>  	if (error)
>  		return error;
> +
> +	/* Check if ownership actually needs to change */
> +	if ((newattrs.ia_valid & ATTR_UID) && !uid_eq(inode->i_uid, uid))
> +		needs_update = true;
> +	if ((newattrs.ia_valid & ATTR_GID) && !gid_eq(inode->i_gid, gid))
> +		needs_update = true;
> +
> +	if (!needs_update) {
> +		inode_unlock(inode);
> +		return 0;
> +	}
>  	if (!S_ISDIR(inode->i_mode))
>  		newattrs.ia_valid |= ATTR_KILL_SUID | ATTR_KILL_PRIV |
>  				     setattr_should_drop_sgid(idmap, inode);
> -- 
> 2.34.1
>