linux-kernel - Re: [PATCH] devtmpfs: don't use vfs_getattr_nosec to query i

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: <8ab4169a-27ea-4667-aadb-3a105d279d1f@amd.com>
Date: Thu, 24 Apr 2025 10:37:18 +0530
From: "Jain, Ayush" <ayushjai@....com>
To: Christoph Hellwig <hch@....de>, gregkh@...uxfoundation.org,
 rafael@...nel.org, dakr@...nel.org, brauner@...nel.org
Cc: linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
 hca@...ux.ibm.com, Shin'ichiro Kawasaki <shinichiro.kawasaki@....com>,
 Xiao Ni <xni@...hat.com>
Subject: Re: [PATCH] devtmpfs: don't use vfs_getattr_nosec to query i_mode

Thank you for the fix
This fixes blk devices hang issues on AMD EPYC x86 systems.

Tested-by: Ayush Jain <Ayush.jain3@....com>

On 4/23/2025 10:29 AM, Christoph Hellwig wrote:
> The recent move of the bdev_statx call to the low-level vfs_getattr_nosec
> helper caused it being used by devtmpfs, which leads to deadlocks in
> md teardown due to the block device lookup and put interfering with the
> unusual lifetime rules in md.
> 
> But as handle_remove only works on inodes created and owned by devtmpfs
> itself there is no need to use vfs_getattr_nosec vs simply reading the
> mode from the inode directly.  Switch to that to avoid the bdev lookup
> or any other unintentional side effect.
> 
> Reported-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@....com>
> Reported-by: Xiao Ni <xni@...hat.com>
> Fixes: 777d0961ff95 ("fs: move the bdex_statx call to vfs_getattr_nosec")
> Signed-off-by: Christoph Hellwig <hch@....de>
> Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@....com>
> Tested-by: Xiao Ni <xni@...hat.com>
> ---
>  drivers/base/devtmpfs.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/base/devtmpfs.c b/drivers/base/devtmpfs.c
> index 6dd1a8860f1c..53fb0829eb7b 100644
> --- a/drivers/base/devtmpfs.c
> +++ b/drivers/base/devtmpfs.c
> @@ -296,7 +296,7 @@ static int delete_path(const char *nodepath)
>  	return err;
>  }
>  
> -static int dev_mynode(struct device *dev, struct inode *inode, struct kstat *stat)
> +static int dev_mynode(struct device *dev, struct inode *inode)
>  {
>  	/* did we create it */
>  	if (inode->i_private != &thread)
> @@ -304,13 +304,13 @@ static int dev_mynode(struct device *dev, struct inode *inode, struct kstat *sta
>  
>  	/* does the dev_t match */
>  	if (is_blockdev(dev)) {
> -		if (!S_ISBLK(stat->mode))
> +		if (!S_ISBLK(inode->i_mode))
>  			return 0;
>  	} else {
> -		if (!S_ISCHR(stat->mode))
> +		if (!S_ISCHR(inode->i_mode))
>  			return 0;
>  	}
> -	if (stat->rdev != dev->devt)
> +	if (inode->i_rdev != dev->devt)
>  		return 0;
>  
>  	/* ours */
> @@ -321,8 +321,7 @@ static int handle_remove(const char *nodename, struct device *dev)
>  {
>  	struct path parent;
>  	struct dentry *dentry;
> -	struct kstat stat;
> -	struct path p;
> +	struct inode *inode;
>  	int deleted = 0;
>  	int err;
>  
> @@ -330,11 +329,8 @@ static int handle_remove(const char *nodename, struct device *dev)
>  	if (IS_ERR(dentry))
>  		return PTR_ERR(dentry);
>  
> -	p.mnt = parent.mnt;
> -	p.dentry = dentry;
> -	err = vfs_getattr(&p, &stat, STATX_TYPE | STATX_MODE,
> -			  AT_STATX_SYNC_AS_STAT);
> -	if (!err && dev_mynode(dev, d_inode(dentry), &stat)) {
> +	inode = d_inode(dentry);
> +	if (dev_mynode(dev, inode)) {
>  		struct iattr newattrs;
>  		/*
>  		 * before unlinking this node, reset permissions
> @@ -342,7 +338,7 @@ static int handle_remove(const char *nodename, struct device *dev)
>  		 */
>  		newattrs.ia_uid = GLOBAL_ROOT_UID;
>  		newattrs.ia_gid = GLOBAL_ROOT_GID;
> -		newattrs.ia_mode = stat.mode & ~0777;
> +		newattrs.ia_mode = inode->i_mode & ~0777;
>  		newattrs.ia_valid =
>  			ATTR_UID|ATTR_GID|ATTR_MODE;
>  		inode_lock(d_inode(dentry));