[PATCH] devtmpfs: don't use vfs_getattr_nosec to query i_mode

[PATCH] devtmpfs: don't use vfs_getattr_nosec to query i_mode

[Christoph Hellwig]

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@wdc.com>
Reported-by: Xiao Ni <xni@redhat.com>
Fixes: 777d0961ff95 ("fs: move the bdex_statx call to vfs_getattr_nosec")
Signed-off-by: Christoph Hellwig <hch@lst.de>
Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Tested-by: Xiao Ni <xni@redhat.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));
-- 
2.47.2

Re: [PATCH] devtmpfs: don't use vfs_getattr_nosec to query i_mode

[Christian Brauner]

On Wed, Apr 23, 2025 at 06:59:41AM +0200, 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@wdc.com>
> Reported-by: Xiao Ni <xni@redhat.com>
> Fixes: 777d0961ff95 ("fs: move the bdex_statx call to vfs_getattr_nosec")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Tested-by: Xiao Ni <xni@redhat.com>
> ---

Reviewed-by: Christian Brauner <brauner@kernel.org>

Re: [PATCH] devtmpfs: don't use vfs_getattr_nosec to query i_mode

[Greg KH]

On Wed, Apr 23, 2025 at 07:54:51AM +0200, Christian Brauner wrote:
> On Wed, Apr 23, 2025 at 06:59:41AM +0200, 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@wdc.com>
> > Reported-by: Xiao Ni <xni@redhat.com>
> > Fixes: 777d0961ff95 ("fs: move the bdex_statx call to vfs_getattr_nosec")
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > Tested-by: Xiao Ni <xni@redhat.com>
> > ---
> 
> Reviewed-by: Christian Brauner <brauner@kernel.org>

Do you want me to take this through the driver-core tree, or is this a
fs-specific thing as that's where the regression showed up from?  Either
is fine with me.

thanks,

greg k-h

Re: [PATCH] devtmpfs: don't use vfs_getattr_nosec to query i_mode

[Heiko Carstens]

On Wed, Apr 23, 2025 at 06:59:41AM +0200, 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@wdc.com>
> Reported-by: Xiao Ni <xni@redhat.com>
> Fixes: 777d0961ff95 ("fs: move the bdex_statx call to vfs_getattr_nosec")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Tested-by: Xiao Ni <xni@redhat.com>
> ---
>  drivers/base/devtmpfs.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)

Tested-by: Heiko Carstens <hca@linux.ibm.com>

Re: [PATCH] devtmpfs: don't use vfs_getattr_nosec to query i_mode

[Jain, Ayush]

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

Tested-by: Ayush Jain <Ayush.jain3@amd.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@wdc.com>
> Reported-by: Xiao Ni <xni@redhat.com>
> Fixes: 777d0961ff95 ("fs: move the bdex_statx call to vfs_getattr_nosec")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Tested-by: Xiao Ni <xni@redhat.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));

Re: [PATCH] devtmpfs: don't use vfs_getattr_nosec to query i_mode

[Christian Brauner]

On Wed, Apr 23, 2025 at 08:42:34AM +0200, Greg KH wrote:
> On Wed, Apr 23, 2025 at 07:54:51AM +0200, Christian Brauner wrote:
> > On Wed, Apr 23, 2025 at 06:59:41AM +0200, 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@wdc.com>
> > > Reported-by: Xiao Ni <xni@redhat.com>
> > > Fixes: 777d0961ff95 ("fs: move the bdex_statx call to vfs_getattr_nosec")
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > > Tested-by: Xiao Ni <xni@redhat.com>
> > > ---
> > 
> > Reviewed-by: Christian Brauner <brauner@kernel.org>
> 
> Do you want me to take this through the driver-core tree, or is this a
> fs-specific thing as that's where the regression showed up from?  Either
> is fine with me.

I'd grab it because it's a fixes for an earlier commit from Christoph.
I also need to check whether we need to revert the bdev_statx() addition
to vfs_getattr_nosec().

Thanks, Greg!

Re: [PATCH] devtmpfs: don't use vfs_getattr_nosec to query i_mode

[Christian Brauner]

On Wed, 23 Apr 2025 06:59:41 +0200, 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.
> 
> [...]

Applied to the vfs.fixes branch of the vfs/vfs.git tree.
Patches in the vfs.fixes branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.fixes

[1/1] devtmpfs: don't use vfs_getattr_nosec to query i_mode
      https://git.kernel.org/vfs/vfs/c/5a876a5097fe

Re: [PATCH] devtmpfs: don't use vfs_getattr_nosec to query i_mode

[Heiko Carstens]

On Wed, Apr 23, 2025 at 06:59:41AM +0200, 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@wdc.com>
> Reported-by: Xiao Ni <xni@redhat.com>
> Fixes: 777d0961ff95 ("fs: move the bdex_statx call to vfs_getattr_nosec")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Tested-by: Xiao Ni <xni@redhat.com>
> ---
>  drivers/base/devtmpfs.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)

...

> @@ -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)) {

clang rightfully complains:

    CC      drivers/base/devtmpfs.o
      drivers/base/devtmpfs.c:333:6: warning: variable 'err' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
        333 |         if (dev_mynode(dev, inode)) {
            |             ^~~~~~~~~~~~~~~~~~~~~~
      drivers/base/devtmpfs.c:358:9: note: uninitialized use occurs here
        358 |         return err;
            |                ^~~
      drivers/base/devtmpfs.c:333:2: note: remove the 'if' if its condition is always true
        333 |         if (dev_mynode(dev, inode)) {
            |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~
      drivers/base/devtmpfs.c:326:9: note: initialize the variable 'err' to silence this warning
        326 |         int err;
            |                ^
            |                 = 0

That is: if dev_mynode(dev, inode) is not true some random value will be returned.

Re: [PATCH] devtmpfs: don't use vfs_getattr_nosec to query i_mode

[Christian Brauner]

On Fri, Apr 25, 2025 at 12:03:04PM +0200, Heiko Carstens wrote:
> On Wed, Apr 23, 2025 at 06:59:41AM +0200, 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@wdc.com>
> > Reported-by: Xiao Ni <xni@redhat.com>
> > Fixes: 777d0961ff95 ("fs: move the bdex_statx call to vfs_getattr_nosec")
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > Tested-by: Xiao Ni <xni@redhat.com>
> > ---
> >  drivers/base/devtmpfs.c | 20 ++++++++------------
> >  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> ...
> 
> > @@ -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)) {
> 
> clang rightfully complains:
> 
>     CC      drivers/base/devtmpfs.o
>       drivers/base/devtmpfs.c:333:6: warning: variable 'err' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
>         333 |         if (dev_mynode(dev, inode)) {
>             |             ^~~~~~~~~~~~~~~~~~~~~~
>       drivers/base/devtmpfs.c:358:9: note: uninitialized use occurs here
>         358 |         return err;
>             |                ^~~
>       drivers/base/devtmpfs.c:333:2: note: remove the 'if' if its condition is always true
>         333 |         if (dev_mynode(dev, inode)) {
>             |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>       drivers/base/devtmpfs.c:326:9: note: initialize the variable 'err' to silence this warning
>         326 |         int err;
>             |                ^
>             |                 = 0
> 
> That is: if dev_mynode(dev, inode) is not true some random value will be returned.

Don't bother resending, Christoph.
I've already fixed this with int err = 0 in the tree.

Re: [PATCH] devtmpfs: don't use vfs_getattr_nosec to query i_mode

[Christoph Hellwig]

On Fri, Apr 25, 2025 at 12:12:36PM +0200, Christian Brauner wrote:
> > That is: if dev_mynode(dev, inode) is not true some random value will be returned.
> 
> Don't bother resending, Christoph.
> I've already fixed this with int err = 0 in the tree.

Thanks!  Let me use this as a platform to rant about our option
defaults and/or gcc error handling.  It seems like ever since we started
zeroing on-stack variables by default gcc stopped warnings about using
uninitialized on-stack variables, leading to tons of these case where
we don't catch uninitialized variables.  Now in this and in many cases
the code works fine because it assumed zero initialization, but there are
also cases where it didn't, leading to new bugs.

Can we fix this somehow?

Re: [PATCH] devtmpfs: don't use vfs_getattr_nosec to query i_mode

[Kees Cook]

On April 25, 2025 6:32:59 AM PDT, Christoph Hellwig <hch@lst.de> wrote:
>On Fri, Apr 25, 2025 at 12:12:36PM +0200, Christian Brauner wrote:
>> > That is: if dev_mynode(dev, inode) is not true some random value will be returned.
>> 
>> Don't bother resending, Christoph.
>> I've already fixed this with int err = 0 in the tree.
>
>Thanks!  Let me use this as a platform to rant about our option
>defaults and/or gcc error handling.  It seems like ever since we started
>zeroing on-stack variables by default gcc stopped warnings about using
>uninitialized on-stack variables, leading to tons of these case where
>we don't catch uninitialized variables.  Now in this and in many cases
>the code works fine because it assumed zero initialization, but there are
>also cases where it didn't, leading to new bugs.

This isn't the case: the feature was explicitly designed in both GCC and Clang to not disrupt -Wuninitialized. But -Wuninitialized has been so flakey for so long that it is almost useless (there was even -Wmaybe-uninitialized added to try to cover some of the missed diagnostics). And it's one of the many reasons stack variable zeroing is so important, since so much goes undiagnosed. :(

>Can we fix this somehow?

Fixing -Wuninitialized would be lovely, but it seems no one has been able to for years now. 😭

-- 
Kees Cook

Re: [PATCH] devtmpfs: don't use vfs_getattr_nosec to query i_mode

[Nathan Chancellor]

On Fri, Apr 25, 2025 at 08:40:23AM -0700, Kees Cook wrote:
> 
> 
> On April 25, 2025 6:32:59 AM PDT, Christoph Hellwig <hch@lst.de> wrote:
> >On Fri, Apr 25, 2025 at 12:12:36PM +0200, Christian Brauner wrote:
> >> > That is: if dev_mynode(dev, inode) is not true some random value will be returned.
> >> 
> >> Don't bother resending, Christoph.
> >> I've already fixed this with int err = 0 in the tree.
> >
> >Thanks!  Let me use this as a platform to rant about our option
> >defaults and/or gcc error handling.  It seems like ever since we started
> >zeroing on-stack variables by default gcc stopped warnings about using
> >uninitialized on-stack variables, leading to tons of these case where
> >we don't catch uninitialized variables.  Now in this and in many cases
> >the code works fine because it assumed zero initialization, but there are
> >also cases where it didn't, leading to new bugs.

I don't think developers can assume that zero initialization is
universally available because 1. there are supported compiler versions
that might not support it and 2. someone may have turned it off or
switched to pattern initialization. Isn't default initialization of
variables supposed to be viewed more as a mitigation against missed
initializations than something to be relied on implicitly? We still want
to know unambiguously and explicitly what the default value of variables
should be.

> This isn't the case: the feature was explicitly designed in both GCC
> and Clang to not disrupt -Wuninitialized. But -Wuninitialized has been
> so flakey for so long that it is almost useless (there was even
> -Wmaybe-uninitialized added to try to cover some of the missed

Right, the fact that GCC does not warn on uninitialized variables is
somewhat self inflicted for the kernel because of 6e8d666e9253 ("Disable
"maybe-uninitialized" warning globally"); I say somewhat because I
understand that the warning was disabled for false positives but it does
mean that there are no true positives either.

> diagnostics). And it's one of the many reasons stack variable zeroing
> is so important, since so much goes undiagnosed. :(
> 
> Fixing -Wuninitialized would be lovely, but it seems no one has been
> able to for years now.

I think clang at one point had a similar problem to GCC's
-Wmaybe-uninitialized (it is -Wconditional-uninitialized there) and that
is how -Wsometimes-uninitialized came into existence. Perhaps GCC could
explore something similar to help gain back some coverage?

There is another big difference between clang and GCC's -Wuninitialized
is that clang's -Wuninitialized will trigger whenever a variable is
guaranteed to be used initialized at its first use, regardless of what
control flow may happen between the declaration and that point, whereas
GCC may turn it into a -Wmaybe-uninitialized.

https://godbolt.org/z/MYxeozc36

Cheers,
Nathan

Re: [PATCH] devtmpfs: don't use vfs_getattr_nosec to query i_mode

[Christoph Hellwig]

On Fri, Apr 25, 2025 at 08:40:23AM -0700, Kees Cook wrote:
> This isn't the case: the feature was explicitly designed in both GCC
> and Clang to not disrupt -Wuninitialized. But -Wuninitialized has been
> so flakey for so long that it is almost useless (there was even
> -Wmaybe-uninitialized added to try to cover some of the missed
> diagnostics).

I do remember a fair amount of bogus uninitialized variable warnings,
but they could be easily shut up without negative impact on the
code.  Not getting any warnings at all on the other hand is catastrophic.


> And it's one of the many reasons stack variable zeroing
> is so important, since so much goes undiagnosed. :(

That only helps if the expected but forgotten initialization value
actually is zero.