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

On Fri 26-04-24 18:20:14, Christian Göttsche wrote:
> From: Christian Göttsche <[email protected]>
> 
> 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 <[email protected]>

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.

> @@ -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.

> @@ -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.

> @@ -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.

> @@ -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 <[email protected]>
SUSE Labs, CR