Re: [PATCH v3 2/2] fs/xattr: add *at family syscalls
From: Jan Kara <jack@suse.cz>
Date: 2024-04-30 10:09:04
Also in:
io-uring, linux-alpha, linux-arch, linux-fsdevel, linux-m68k, linux-mips, linux-s390, linux-security-module, linux-sh, linuxppc-dev, lkml, selinux, sparclinux
On Fri 26-04-24 18:20:14, Christian Göttsche wrote:
From: Christian Göttsche <redacted>
Add the four syscalls setxattrat(), getxattrat(), listxattrat() and
removexattrat(). Those can be used to operate on extended attributes,
especially security related ones, either relative to a pinned directory
or on a file descriptor without read access, avoiding a
/proc/<pid>/fd/<fd> detour, requiring a mounted procfs.
One use case will be setfiles(8) setting SELinux file contexts
("security.selinux") without race conditions and without a file
descriptor opened with read access requiring SELinux read permission.
Use the do_{name}at() pattern from fs/open.c.
Pass the value of the extended attribute, its length, and for
setxattrat(2) the command (XATTR_CREATE or XATTR_REPLACE) via an added
struct xattr_args to not exceed six syscall arguments and not
merging the AT_* and XATTR_* flags.
Signed-off-by: Christian Göttsche <redacted>The patch looks good to me. Just a few nits below:
-static int path_setxattr(const char __user *pathname, +static int do_setxattrat(int dfd, const char __user *pathname, unsigned int at_flags,
Can we please stay within 80 columns (happens in multiple places in the patch)? I don't insist but it makes things easier to read in some setups so I prefer it.
quoted hunk ↗ jump to hunk
@@ -852,13 +908,21 @@ listxattr(struct dentry *d, char __user *list, size_t size) return error; } -static ssize_t path_listxattr(const char __user *pathname, char __user *list, - size_t size, unsigned int lookup_flags) +static ssize_t do_listxattrat(int dfd, const char __user *pathname, char __user *list, + size_t size, int flags)
So I like how in previous syscalls you have 'at_flags', 'lookup_flags', and 'xattr_flags'. That makes things much easier to digest. Can you please stay with that convention here as well and call this argument 'at_flags'? Also I think the argument ordering like "dfd, pathname, at_flags, list, size" is more consistent with other syscalls you define.
quoted hunk ↗ jump to hunk
@@ -870,16 +934,22 @@ static ssize_t path_listxattr(const char __user *pathname, char __user *list, return error; } +SYSCALL_DEFINE5(listxattrat, int, dfd, const char __user *, pathname, char __user *, list, + size_t, size, int, flags) +{ + return do_listxattrat(dfd, pathname, list, size, flags); +} +
Same comment as above - "flags" -> "at_flags" and reorder args please.
quoted hunk ↗ jump to hunk
@@ -917,13 +987,21 @@ removexattr(struct mnt_idmap *idmap, struct dentry *d, return vfs_removexattr(idmap, d, kname); } -static int path_removexattr(const char __user *pathname, - const char __user *name, unsigned int lookup_flags) +static int do_removexattrat(int dfd, const char __user *pathname, + const char __user *name, int flags) {
Same comment as above - "flags" -> "at_flags" and reorder args please.
quoted hunk ↗ jump to hunk
@@ -939,16 +1017,22 @@ static int path_removexattr(const char __user *pathname, return error; } +SYSCALL_DEFINE4(removexattrat, int, dfd, const char __user *, pathname, + const char __user *, name, int, flags) +{
Same comment as above - "flags" -> "at_flags" and reorder args please. Honza -- Jan Kara [off-list ref] SUSE Labs, CR