Re: [PATCH v3] fs: introduce getfsxattrat and setfsxattrat syscalls

Andreas Dilger
2025-02-21T19:10:48Z

[-- Attachment #1: Type: text/plain, Size: 6234 bytes --]

On Feb 21, 2025, at 11:11 AM, Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Tue, Feb 11, 2025 at 06:22:47PM +0100, Andrey Albershteyn wrote:
>> From: Andrey Albershteyn <aalbersh@redhat.com>
>>
>> Introduce getfsxattrat and setfsxattrat syscalls to manipulate inode
>> extended attributes/flags. The syscalls take parent directory fd and
>> path to the child together with struct fsxattr.
>>
>> 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.
>>
>> Also, as vfs_fileattr_set() is now will be called on special files
>> too, let's forbid any other attributes except projid and nextents
>> (symlink can have an extent).
>>
>> CC: linux-api@vger.kernel.org
>> CC: linux-fsdevel@vger.kernel.org
>> CC: linux-xfs@vger.kernel.org
>> Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
>> ---
>> v1:
>> https://lore.kernel.org/linuxppc-dev/20250109174540.893098-1-aalbersh@kernel.org/
>>
>> Previous discussion:
>> https://lore.kernel.org/linux-xfs/20240520164624.665269-2-aalbersh@redhat.com/
>>
>> XFS has project quotas which could be attached to a directory. All
>> new inodes in these directories inherit project ID set on parent
>> directory.
>>
>> The project is created from userspace by opening and calling
>> FS_IOC_FSSETXATTR on each inode. This is not possible for special
>> files such as FIFO, SOCK, BLK etc. Therefore, some inodes are left
>> with empty project ID. Those inodes then are not shown in the quota
>> accounting but still exist in the directory. Moreover, in the case
>> when special files are created in the directory with already
>> existing project quota, these inode inherit extended attributes.
>> This than leaves them with these attributes without the possibility
>> to clear them out. This, in turn, prevents userspace from
>> re-creating quota project on these existing files.
>> ---
>> Changes in v3:
>> - Remove unnecessary "dfd is dir" check as it checked in user_path_at()
>> - Remove unnecessary "same filesystem" check
>> - Use CLASS() instead of directly calling fdget/fdput
>> - Link to v2: https://lore.kernel.org/r/20250122-xattrat-syscall-v2-1-5b360d4fbcb2@kernel.org
>> ---
>> arch/alpha/kernel/syscalls/syscall.tbl | 2 +
>> arch/arm/tools/syscall.tbl | 2 +
>> arch/arm64/tools/syscall_32.tbl | 2 +
>> arch/m68k/kernel/syscalls/syscall.tbl | 2 +
>> arch/microblaze/kernel/syscalls/syscall.tbl | 2 +
>> arch/mips/kernel/syscalls/syscall_n32.tbl | 2 +
>> arch/mips/kernel/syscalls/syscall_n64.tbl | 2 +
>> arch/mips/kernel/syscalls/syscall_o32.tbl | 2 +
>> arch/parisc/kernel/syscalls/syscall.tbl | 2 +
>> arch/powerpc/kernel/syscalls/syscall.tbl | 2 +
>> arch/s390/kernel/syscalls/syscall.tbl | 2 +
>> arch/sh/kernel/syscalls/syscall.tbl | 2 +
>> arch/sparc/kernel/syscalls/syscall.tbl | 2 +
>> arch/x86/entry/syscalls/syscall_32.tbl | 2 +
>> arch/x86/entry/syscalls/syscall_64.tbl | 2 +
>> arch/xtensa/kernel/syscalls/syscall.tbl | 2 +
>> fs/inode.c | 75 +++++++++++++++++++++++++++++
>> fs/ioctl.c | 16 +++++-
>> include/linux/fileattr.h | 1 +
>> include/linux/syscalls.h | 4 ++
>> include/uapi/asm-generic/unistd.h | 8 ++-
>> 21 files changed, 133 insertions(+), 3 deletions(-)
>>
>
> <cut to the syscall definitions>
>
>> diff --git a/fs/inode.c b/fs/inode.c
>> index 6b4c77268fc0ecace4ac78a9ca777fbffc277f4a..b2dddd9db4fabaf67a6cbf541a86978b290411ec 100644
>> --- a/fs/inode.c
>> +++ b/fs/inode.c
>> @@ -23,6 +23,9 @@
>> #include <linux/rw_hint.h>
>> #include <linux/seq_file.h>
>> #include <linux/debugfs.h>
>> +#include <linux/syscalls.h>
>> +#include <linux/fileattr.h>
>> +#include <linux/namei.h>
>> #include <trace/events/writeback.h>
>> #define CREATE_TRACE_POINTS
>> #include <trace/events/timestamp.h>
>> @@ -2953,3 +2956,75 @@ umode_t mode_strip_sgid(struct mnt_idmap *idmap,
>> return mode & ~S_ISGID;
>> }
>> EXPORT_SYMBOL(mode_strip_sgid);
>> +
>> +SYSCALL_DEFINE4(getfsxattrat, int, dfd, const char __user *, filename,
>> + struct fsxattr __user *, fsx, unsigned int, at_flags)
>
> Should the kernel require userspace to pass the size of the fsx buffer?
> That way we avoid needing to rev the interface when we decide to grow
> the structure.

Definitely having some extensibility would be good, and there isn't much
room left today.

The struct size change would be handled automatically by the ioctl()
interface, but not the new syscall interface.

Another option would be to use an xflags to indicate "larger struct"
and then store the size after the end of the current struct. It would
also be possible to use one of the few remaining fields for this, but
one is earmarked for the DOS flags and/or a bitmask of supported flags,
and there isn't really any value to it until more fields are needed.

#define FS_XFLAG_LARGE_STRUCT 0x40000000

struct fsxattr {
__u32 fsx_xflags; /* xflags field value (get/set) */
__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)*/
unsigned char fsx_pad[8];
__u32 fsx_fsxattr_size; /* struct size in bytes (get/set) */
:
/* future fields */
:
};

Not su

Cheers, Andreas

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]