[PATCH] Revert "fs: move the bdex_statx call to vfs_getattr_nosec"

[PATCH] Revert "fs: move the bdex_statx call to vfs_getattr_nosec"

[Christian Brauner]

This reverts commit 777d0961ff95b26d5887fdae69900374364976f3.

Now that we have fixed the original issue in devtmpfs we can revert this
commit because the bdev_statx() call in vfs_getattr_nosec() causes
issues with the lifetime logic of dm devices.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 block/bdev.c           |  3 ++-
 fs/stat.c              | 32 ++++++++++++++------------------
 include/linux/blkdev.h |  6 +++---
 3 files changed, 19 insertions(+), 22 deletions(-)

diff --git a/block/bdev.c b/block/bdev.c
index 6a34179192c9..4844d1e27b6f 100644
--- a/block/bdev.c
+++ b/block/bdev.c
@@ -1272,7 +1272,8 @@ void sync_bdevs(bool wait)
 /*
  * Handle STATX_{DIOALIGN, WRITE_ATOMIC} for block devices.
  */
-void bdev_statx(const struct path *path, struct kstat *stat, u32 request_mask)
+void bdev_statx(struct path *path, struct kstat *stat,
+		u32 request_mask)
 {
 	struct inode *backing_inode;
 	struct block_device *bdev;
diff --git a/fs/stat.c b/fs/stat.c
index 3d9222807214..f13308bfdc98 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -204,25 +204,12 @@ int vfs_getattr_nosec(const struct path *path, struct kstat *stat,
 				  STATX_ATTR_DAX);
 
 	idmap = mnt_idmap(path->mnt);
-	if (inode->i_op->getattr) {
-		int ret;
-
-		ret = inode->i_op->getattr(idmap, path, stat, request_mask,
-				query_flags);
-		if (ret)
-			return ret;
-	} else {
-		generic_fillattr(idmap, request_mask, inode, stat);
-	}
-
-	/*
-	 * If this is a block device inode, override the filesystem attributes
-	 * with the block device specific parameters that need to be obtained
-	 * from the bdev backing inode.
-	 */
-	if (S_ISBLK(stat->mode))
-		bdev_statx(path, stat, request_mask);
+	if (inode->i_op->getattr)
+		return inode->i_op->getattr(idmap, path, stat,
+					    request_mask,
+					    query_flags);
 
+	generic_fillattr(idmap, request_mask, inode, stat);
 	return 0;
 }
 EXPORT_SYMBOL(vfs_getattr_nosec);
@@ -308,6 +295,15 @@ static int vfs_statx_path(struct path *path, int flags, struct kstat *stat,
 	if (path_mounted(path))
 		stat->attributes |= STATX_ATTR_MOUNT_ROOT;
 	stat->attributes_mask |= STATX_ATTR_MOUNT_ROOT;
+
+	/*
+	 * If this is a block device inode, override the filesystem
+	 * attributes with the block device specific parameters that need to be
+	 * obtained from the bdev backing inode.
+	 */
+	if (S_ISBLK(stat->mode))
+		bdev_statx(path, stat, request_mask);
+
 	return 0;
 }
 
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 678dc38442bf..e39c45bc0a97 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1685,7 +1685,7 @@ int sync_blockdev(struct block_device *bdev);
 int sync_blockdev_range(struct block_device *bdev, loff_t lstart, loff_t lend);
 int sync_blockdev_nowait(struct block_device *bdev);
 void sync_bdevs(bool wait);
-void bdev_statx(const struct path *path, struct kstat *stat, u32 request_mask);
+void bdev_statx(struct path *, struct kstat *, u32);
 void printk_all_partitions(void);
 int __init early_lookup_bdev(const char *pathname, dev_t *dev);
 #else
@@ -1703,8 +1703,8 @@ static inline int sync_blockdev_nowait(struct block_device *bdev)
 static inline void sync_bdevs(bool wait)
 {
 }
-static inline void bdev_statx(const struct path *path, struct kstat *stat,
-		u32 request_mask)
+static inline void bdev_statx(struct path *path, struct kstat *stat,
+				u32 request_mask)
 {
 }
 static inline void printk_all_partitions(void)
-- 
2.47.2

Re: [PATCH] Revert "fs: move the bdex_statx call to vfs_getattr_nosec"

[Christoph Hellwig]

On Thu, Apr 24, 2025 at 10:59:44AM +0200, Christian Brauner wrote:
> This reverts commit 777d0961ff95b26d5887fdae69900374364976f3.
> 
> Now that we have fixed the original issue in devtmpfs we can revert this
> commit because the bdev_statx() call in vfs_getattr_nosec() causes
> issues with the lifetime logic of dm devices.

Umm, no.  We need this patch.  And the devtmpfs fixes the issue that
it caused for devtmpfs.

Re: [PATCH] Revert "fs: move the bdex_statx call to vfs_getattr_nosec"

[Christian Brauner]

On Thu, Apr 24, 2025 at 11:04:44AM +0200, Christoph Hellwig wrote:
> On Thu, Apr 24, 2025 at 10:59:44AM +0200, Christian Brauner wrote:
> > This reverts commit 777d0961ff95b26d5887fdae69900374364976f3.
> > 
> > Now that we have fixed the original issue in devtmpfs we can revert this
> > commit because the bdev_statx() call in vfs_getattr_nosec() causes
> > issues with the lifetime logic of dm devices.
> 
> Umm, no.  We need this patch.  And the devtmpfs fixes the issue that
> it caused for devtmpfs.

For loop devices only afaict. The bdev_statx() implementation is
absolutely terrifying because it triggers all that block module
autoloading stuff and quite a few kernels still have that turned on. By
adding that to vfs_getattr_nosec() suddenly all kernel consumers are
able to do the same thing by accident. This already crapped devtmpfs. We
have no idea what else it will start breaking unless you audit every
single caller.

If this stays in then please figure out how to skip a call into
blkdev_get_no_open() unless it's explicitly requested. I don't care if
we have to add an in-kernel only request flag. We have one already
anyway.

Re: [PATCH] Revert "fs: move the bdex_statx call to vfs_getattr_nosec"

[Christoph Hellwig]

On Thu, Apr 24, 2025 at 11:22:40AM +0200, Christian Brauner wrote:
> On Thu, Apr 24, 2025 at 11:04:44AM +0200, Christoph Hellwig wrote:
> > On Thu, Apr 24, 2025 at 10:59:44AM +0200, Christian Brauner wrote:
> > > This reverts commit 777d0961ff95b26d5887fdae69900374364976f3.
> > > 
> > > Now that we have fixed the original issue in devtmpfs we can revert this
> > > commit because the bdev_statx() call in vfs_getattr_nosec() causes
> > > issues with the lifetime logic of dm devices.
> > 
> > Umm, no.  We need this patch.  And the devtmpfs fixes the issue that
> > it caused for devtmpfs.
> 
> For loop devices only afaict.

For anything that wants to query bdev attributes.  We are about to
add the same for nvmet, and in the future also for the scsi target
and zloop if it gets merged.

Either way the above sentence is wrong - reverting it will cause a
regression.

> The bdev_statx() implementation is
> absolutely terrifying because it triggers all that block module
> autoloading stuff and quite a few kernels still have that turned on. By
> adding that to vfs_getattr_nosec() suddenly all kernel consumers are
> able to do the same thing by accident.

I send a series to fi that yesterday.

> This already crapped devtmpfs. We
> have no idea what else it will start breaking unless you audit every
> single caller.

I don't think so.  The issue really is that devtmpfs calls this from
the block device shutdown path, and md has a really weird shutdown
path.  The first is fixed by the patch you applied, and the second is
being looked into, a series was posted about half an hour ago.

Callers that aren't directly tied into block device shutdown can't
have that issue.

> If this stays in then please figure out how to skip a call into
> blkdev_get_no_open() unless it's explicitly requested.

The problem with that is the blksize that is reported unconditionally,
for the LBS stuff.  I don't have a good answer for that as it is
reported unconditionally.  But as said, the extra bdev reference is
fairly harmless outside of magic code like devtmpfs.