linux-kernel - Re: [PATCH 2/2] fs: make several inode lock operations killable

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: <20250429-anpassen-exkremente-98686d53a021@brauner>
Date: Tue, 29 Apr 2025 13:12:28 +0200
From: Christian Brauner <brauner@...nel.org>
To: Max Kellermann <max.kellermann@...os.com>
Cc: viro@...iv.linux.org.uk, jack@...e.cz, linux-fsdevel@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] fs: make several inode lock operations killable

On Tue, Apr 29, 2025 at 11:46:44AM +0200, Max Kellermann wrote:
> Allows killing processes that are waiting for the inode lock.
> 
> Signed-off-by: Max Kellermann <max.kellermann@...os.com>
> ---

Oha, that's interesting.

>  fs/open.c       | 14 +++++++++++---
>  fs/read_write.c |  4 +++-
>  2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/open.c b/fs/open.c
> index a9063cca9911..7828234a7caa 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -60,7 +60,10 @@ int do_truncate(struct mnt_idmap *idmap, struct dentry *dentry,
>  	if (ret)
>  		newattrs.ia_valid |= ret | ATTR_FORCE;
>  
> -	inode_lock(dentry->d_inode);
> +	ret = inode_lock_killable(dentry->d_inode);
> +	if (ret)
> +		return ret;
> +
>  	/* Note any delegations or leases have already been broken: */
>  	ret = notify_change(idmap, dentry, &newattrs, NULL);
>  	inode_unlock(dentry->d_inode);
> @@ -635,7 +638,9 @@ int chmod_common(const struct path *path, umode_t mode)
>  	if (error)
>  		return error;
>  retry_deleg:
> -	inode_lock(inode);
> +	error = inode_lock_killable(inode);

That's probably fine.

> +	if (error)
> +		goto out_mnt_unlock;
>  	error = security_path_chmod(path, mode);
>  	if (error)
>  		goto out_unlock;
> @@ -650,6 +655,7 @@ int chmod_common(const struct path *path, umode_t mode)
>  		if (!error)
>  			goto retry_deleg;
>  	}
> +out_mnt_unlock:
>  	mnt_drop_write(path->mnt);
>  	return error;
>  }
> @@ -769,7 +775,9 @@ int chown_common(const struct path *path, uid_t user, gid_t group)
>  		return -EINVAL;
>  	if ((group != (gid_t)-1) && !setattr_vfsgid(&newattrs, gid))
>  		return -EINVAL;
> -	inode_lock(inode);
> +	error = inode_lock_killable(inode);
> +	if (error)
> +		return error;

That too.

>  	if (!S_ISDIR(inode->i_mode))
>  		newattrs.ia_valid |= ATTR_KILL_SUID | ATTR_KILL_PRIV |
>  				     setattr_should_drop_sgid(idmap, inode);
> diff --git a/fs/read_write.c b/fs/read_write.c
> index bb0ed26a0b3a..0ef70e128c4a 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -332,7 +332,9 @@ loff_t default_llseek(struct file *file, loff_t offset, int whence)
>  	struct inode *inode = file_inode(file);
>  	loff_t retval;
>  
> -	inode_lock(inode);
> +	retval = inode_lock_killable(inode);

That change doesn't seem so obviously fine to me.
Either way I'd like to see this split in three patches and some
reasoning why it's safe and some justification why it's wanted...