On Fri, Feb 21, 2025 at 7:13 PM Darrick J. Wong <djwong@xxxxxxxxxx> wrote:
On Tue, Feb 11, 2025 at 06:22:47PM +0100, Andrey Albershteyn wrote:
From: Andrey Albershteyn <aalbersh@xxxxxxxxxx>
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@xxxxxxxxxxxxxxx
CC: linux-fsdevel@xxxxxxxxxxxxxxx
CC: linux-xfs@xxxxxxxxxxxxxxx
Signed-off-by: Andrey Albershteyn <aalbersh@xxxxxxxxxx>
---
v1:
https://lore.kernel.org/linuxppc-dev/20250109174540.893098-1-aalbersh@xxxxxxxxxx/
Previous discussion:
https://lore.kernel.org/linux-xfs/20240520164624.665269-2-aalbersh@xxxxxxxxxx/
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@xxxxxxxxxx
---
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.
This makes sense to me, but I see that Andreas proposed other ways,
as long as we have a plan on how to extend the struct if we need more space.
Andrey, I am sorry to bring this up in v3, but I would like to request
two small changes before merging this API.
This patch by Pali [1] adds fsx_xflags_mask for the filesystem to
report the supported set of xflags.
It was argued that we can make this change with the existing ioctl,
because it is not going to break xfs_io -c lsattr/chattr, which is fine,
but I think that we should merge the fsx_xflags_mask change along
with getfsxattrat() which is a new UAPI.
The second request is related to setfsxattrat().
With current FS_IOC_FSSETXATTR, IIUC, xfs ignores unsupported
fsx_xflags. I think this needs to be fixed before merging setfsxattrat().
It's ok that a program calling FS_IOC_FSSETXATTR will not know
if unsupported flags will be ignored, because that's the way it is,
but I think that setfsxattrat() must return -EINVAL for trying to
set unsupported xflags.
As I explained in [2] I think it is fine if FS_IOC_FSSETXATTR
will also start returning -EINVAL for unsupported flags, but I would
like setfsxattrat() to make that a guarantee.
There was an open question, what does fsx_xflags_mask mean
for setfsxattrat() - it is a mask like in inode_set_flags() as Andreas
suggested? I think that would be a good idea.
Thanks,
Amir.
[1] https://lore.kernel.org/linux-fsdevel/20250216164029.20673-4-pali@xxxxxxxxxx/
[2] https://lore.kernel.org/linux-fsdevel/CAOQ4uxjwQJiKAqyjEmKUnq-VihyeSsxyEy2F+J38NXwrAXurFQ@xxxxxxxxxxxxxx/
