On Tue, Oct 22, 2024 at 10:49:59AM +0200, Christian Brauner wrote:
> > 1) why is STATX_MNT_ID set without checking if it's in the mask passed to
> > the damn thing?
>
> If you look at the history you'll find that STATX_MNT_ID has always been
> set unconditionally, i.e., userspace didn't have to request it
> explicitly. And that's fine. It's not like it's expensive.
Not the issue, really - the difference between the by-fd (when we call vfs_fstat())
and by-path (calling vfs_statx()) cases of vfs_fstatat() is what worries me.
In by-fd case you hit vfs_getattr() and that's it. In by-path case you
hit exact same helper you do for statx(2), with STATX_BASIC_FLAGS for the
mask argument. Which ends with vfs_getattr() + a bunch of followups in
vfs_statx_path(). _IF_ all those followups had been no-ops when mask
is just STATX_BASIC_FLAGS, everything would've been fine. They are not,
though.
And while all current users of vfs_fstatat() ignore stat->attributes,
stat->attributes_mask and stat->mnt_id (as well as stat->result_mask),
that's a property of code we feed that struct kstat to after vfs_fstatat()
call has filled it. Currently that's 9 functions, spread over a lot of
places. Sure, each of those is fine, but any additional instance that does
care and we are getting an unpleasant inconsistency between by-fd and by-path
users.
Variants:
1) We can clone vfs_statx(), have the clone call vfs_getattr() instead
of vfs_statx_path() and use it in vfs_fstatat(). That would take care
of any future inconsistencies (as well as a minor side benefit of losing
request_mask argument in that thing). OTOH, it's extra code duplication.
2) go for your "it's cheap anyway" and have vfs_fstatat() use
vfs_statx_fd(fd, flags, stat, STATX_BASIC_FLAGS) on the by-fd path.
Again, that restores consistency. Cost: that extra work (tail of
vfs_statx_path()) done for fstat(2) just as it's currently done for
stat(2).
3) add an explicit "none of that fancy crap" flag argument to vfs_statx_path()
and pass it through vfs_statx(). Cost: extra argument passed through
the call chain.
And yes, it's all cheap, but {f,}stat(2) is often _very_ hot, so it's worth
getting right.
> > 2) why, in the name of everything unholy, does statx() on /dev/weird_shite
> > trigger modprobe on that thing? Without even a permission check on it...
>
> Block people should know. But afaict, that's a block device thing which
> happens with CONFIG_BLOCK_LEGACY_AUTOLOAD enabled which is supposedly
> deprecated.
Umm... Deprecated or not, one generally expects that if /dev/foo is
owned by root:root with 0600 for permissions, then non-root won't be
able to trigger anything in the vicinity of the hardware in question,
including "load the driver"... Note that open(2) won't get anywhere
near blkdev_open() - it will get EPERM before that. stat(2) also
won't go there. statx(2) will. And frankly, getting the STATX_DIOALIGN
and STATX_WRITE_ATOMIC information out of such device is also somewhat
dubious, but that's less obviously wrong.
