Re: [RFC][PATCH] getname_maybe_null() - the third variant of pathname copy-in

[Al Viro <viro@zeniv.linux.org.uk>]

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.