linux-kernel - Re: [PATCH v6 6/6] fs: introduce file_getattr and file

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250701184317.GQ10009@frogsfrogsfrogs>
Date: Tue, 1 Jul 2025 11:43:17 -0700
From: "Darrick J. Wong" <djwong@...nel.org>
To: Andrey Albershteyn <aalbersh@...hat.com>
Cc: Amir Goldstein <amir73il@...il.com>, Arnd Bergmann <arnd@...db.de>,
	Casey Schaufler <casey@...aufler-ca.com>,
	Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
	Pali Rohár <pali@...nel.org>,
	Paul Moore <paul@...l-moore.com>, linux-api@...r.kernel.org,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-xfs@...r.kernel.org, selinux@...r.kernel.org,
	Andrey Albershteyn <aalbersh@...nel.org>
Subject: Re: [PATCH v6 6/6] fs: introduce file_getattr and file_setattr
 syscalls

On Mon, Jun 30, 2025 at 06:20:16PM +0200, Andrey Albershteyn wrote:
> From: Andrey Albershteyn <aalbersh@...hat.com>
> 
> Introduce file_getattr() and file_setattr() syscalls to manipulate inode
> extended attributes. The syscalls takes pair of file descriptor and
> pathname. Then it operates on inode opened accroding to openat()
> semantics. The struct fsx_fileattr is passed to obtain/change extended
> attributes.
> 
> This is an alternative to FS_IOC_FSSETXATTR ioctl with a difference
> that file don't need to be open as we can reference it with a path
> instead of fd. By having this we can manipulated inode extended
> attributes not only on regular files but also on special ones. This
> is not possible with FS_IOC_FSSETXATTR ioctl as with special files
> we can not call ioctl() directly on the filesystem inode using fd.
> 
> This patch adds two new syscalls which allows userspace to get/set
> extended inode attributes on special files by using parent directory
> and a path - *at() like syscall.
> 
> CC: linux-api@...r.kernel.org
> CC: linux-fsdevel@...r.kernel.org
> CC: linux-xfs@...r.kernel.org
> Signed-off-by: Andrey Albershteyn <aalbersh@...nel.org>
> Acked-by: Arnd Bergmann <arnd@...db.de>
> ---

<snip syscall table>

> diff --git a/fs/file_attr.c b/fs/file_attr.c
> index 62f08872d4ad..fda9d847eee5 100644
> --- a/fs/file_attr.c
> +++ b/fs/file_attr.c
> @@ -3,6 +3,10 @@
>  #include <linux/security.h>
>  #include <linux/fscrypt.h>
>  #include <linux/fileattr.h>
> +#include <linux/syscalls.h>
> +#include <linux/namei.h>
> +
> +#include "internal.h"
>  
>  /**
>   * fileattr_fill_xflags - initialize fileattr with xflags
> @@ -89,6 +93,19 @@ int vfs_fileattr_get(struct dentry *dentry, struct fileattr *fa)
>  }
>  EXPORT_SYMBOL(vfs_fileattr_get);
>  
> +static void fileattr_to_fsx_fileattr(const struct fileattr *fa,
> +				     struct fsx_fileattr *fsx)

Er... "fsx_fileattr" is the struct that the system call uses?

That's a little confusing considering that xfs already has a
xfs_fill_fsxattr function that actually fills a struct fileattr.
That could be renamed xfs_fill_fileattr.

I dunno.  There's a part of me that would really rather that the
file_getattr and file_setattr syscalls operate on a struct file_attr.

More whining/bikeshedding to come.

<snip stuff that looks ok to me>

<<well, I still dislike the CLASS(fd, fd)(fd) syntax...>>

> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> index 0098b0ce8ccb..0784f2033ba4 100644
> --- a/include/uapi/linux/fs.h
> +++ b/include/uapi/linux/fs.h
> @@ -148,6 +148,24 @@ struct fsxattr {
>  	unsigned char	fsx_pad[8];
>  };
>  
> +/*
> + * Variable size structure for file_[sg]et_attr().
> + *
> + * Note. This is alternative to the structure 'struct fileattr'/'struct fsxattr'.
> + * As this structure is passed to/from userspace with its size, this can
> + * be versioned based on the size.
> + */
> +struct fsx_fileattr {
> +	__u32	fsx_xflags;	/* xflags field value (get/set) */

Should this to be __u64 from the start?  Seeing as (a) this struct is
not already a multiple of 8 bytes and (b) it's likely that we'll have to
add a u64 field at some point.  That would also address brauner's
comment about padding.

--D

> +	__u32	fsx_extsize;	/* extsize field value (get/set)*/
> +	__u32	fsx_nextents;	/* nextents field value (get)   */
> +	__u32	fsx_projid;	/* project identifier (get/set) */
> +	__u32	fsx_cowextsize;	/* CoW extsize field value (get/set) */
> +};
> +
> +#define FSX_FILEATTR_SIZE_VER0 20
> +#define FSX_FILEATTR_SIZE_LATEST FSX_FILEATTR_SIZE_VER0
> +
>  /*
>   * Flags for the fsx_xflags field
>   */
> diff --git a/scripts/syscall.tbl b/scripts/syscall.tbl
> index 580b4e246aec..d1ae5e92c615 100644
> --- a/scripts/syscall.tbl
> +++ b/scripts/syscall.tbl
> @@ -408,3 +408,5 @@
>  465	common	listxattrat			sys_listxattrat
>  466	common	removexattrat			sys_removexattrat
>  467	common	open_tree_attr			sys_open_tree_attr
> +468	common	file_getattr			sys_file_getattr
> +469	common	file_setattr			sys_file_setattr
> 
> -- 
> 2.47.2
> 
>