On Thu, Apr 17, 2025 at 08:40:42AM +0200, Christoph Hellwig wrote:
> Currently bdex_statx is only called from the very high-level
> vfs_statx_path function, and thus bypassing it for in-kernel calls
> to vfs_getattr or vfs_getattr_nosec.
>
> This breaks querying the block ѕize of the underlying device in the
> loop driver and also is a pitfall for any other new kernel caller.
>
> Move the call into the lowest level helper to ensure all callers get
> the right results.
>
> Fixes: 2d985f8c6b91 ("vfs: support STATX_DIOALIGN on block devices")
> Fixes: f4774e92aab8 ("loop: take the file system minimum dio alignment into account")
> Reported-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
This looks fine but one thing below.
> block/bdev.c | 3 +--
> fs/stat.c | 32 ++++++++++++++++++--------------
> include/linux/blkdev.h | 6 +++---
> 3 files changed, 22 insertions(+), 19 deletions(-)
>
> diff --git a/block/bdev.c b/block/bdev.c
> index 4844d1e27b6f..6a34179192c9 100644
> --- a/block/bdev.c
> +++ b/block/bdev.c
> @@ -1272,8 +1272,7 @@ void sync_bdevs(bool wait)
> /*
> * Handle STATX_{DIOALIGN, WRITE_ATOMIC} for block devices.
> */
> -void bdev_statx(struct path *path, struct kstat *stat,
> - u32 request_mask)
> +void bdev_statx(const 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 f13308bfdc98..3d9222807214 100644
> --- a/fs/stat.c
> +++ b/fs/stat.c
> @@ -204,12 +204,25 @@ int vfs_getattr_nosec(const struct path *path, struct kstat *stat,
> STATX_ATTR_DAX);
>
> idmap = mnt_idmap(path->mnt);
> - if (inode->i_op->getattr)
> - return inode->i_op->getattr(idmap, path, stat,
> - request_mask,
> - query_flags);
> + 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);
>
> - generic_fillattr(idmap, request_mask, inode, stat);
> return 0;
> }
> EXPORT_SYMBOL(vfs_getattr_nosec);
> @@ -295,15 +308,6 @@ 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);
bdev_statx()
-> blkdev_get_no_open()
{
if (!inode && IS_ENABLED(CONFIG_BLOCK_LEGACY_AUTOLOAD)) {
blk_request_module(dev);
inode = ilookup(blockdev_superblock, dev);
if (inode)
pr_warn_ratelimited("block device autoloading is deprecated and will be removed.\n");
}
}
So that means any unprivileged Schmock can trigger a module autoload
from statx() if that's enabled? It feels like this should really be
disabled for statx().
