Re: [RFC PATCH 1/2] fs/xattr: add *at family syscalls

[Amir Goldstein]

On Thu, Sep 1, 2022 at 2:10 AM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> [linux-arch Cc'd for ABI-related stuff]
>
> On Tue, Aug 30, 2022 at 05:28:39PM +0200, Christian Göttsche wrote:
> > Add the four syscalls setxattrat(), getxattrat(), listxattrat() and
> > removexattrat() to enable extended attribute operations via file
> > descriptors.  This can be used from userspace to avoid race conditions,
> > especially on security related extended attributes, like SELinux labels
> > ("security.selinux") via setfiles(8).
> >
> > Use the do_{name}at() pattern from fs/open.c.
> > Use a single flag parameter for extended attribute flags (currently
> > XATTR_CREATE and XATTR_REPLACE) and *at() flags to not exceed six
> > syscall arguments in setxattrat().
>
>         I've no problems with the patchset aside of the flags part;
> however, note that XATTR_CREATE and XATTR_REPLACE are actually exposed
> to the network - the values are passed to nfsd by clients.
> See nfsd4_decode_setxattr() and
>         BUILD_BUG_ON(XATTR_CREATE != SETXATTR4_CREATE);
>         BUILD_BUG_ON(XATTR_REPLACE != SETXATTR4_REPLACE);
> in encode_setxattr() on the client side.
>
>         Makes me really nervous about constraints like that.  Sure,
> AT_... flags you are using are in the second octet and these are in
> the lowest one, but...

In this context, I would like to point at

AT_EACCESS
AT_REMOVEDIR

Which are using the same namespace as the AT_ flags but define
a flag in a "private section" of that namespace for faccessat() and
for unlinkat().
unlinkat() does not technically support any of the generic AT_ flags,
but the sycall name does suggest that it is the same namespace.

At the risk of getting shouted at, I propose that we retroactively
formalize this practice and also define
AT_XATTR_* and AT_RENAME_* constants
with the accompanied BUILD_BUG_ON()
and document above the AT_ definitions that the lowest 10 bits
are reserved as private namespace for the specific syscall.

There are also the AT_STATX_*SYNC* flags that could fall
into the category of syscall private namespace, but those flags could
actually be made more generic as there are other syscalls that may
benefit from supporting them.

linkat() is one example that comes to mind.
Similar suggestions have been posted in the past:
https://lore.kernel.org/linux-fsdevel/20190527172655.9287-1-amir73il@xxxxxxxxx/
https://lore.kernel.org/linux-fsdevel/CAOQ4uxit0KYiShpEXt8b8SvN8bWWp3Ky929b+UWNDozTCUeTxg@xxxxxxxxxxxxxx/

Thanks,
Amir.