Add a new fchmodat4() syscall, v2
Subsystems (19)
This patch set adds fchmodat4(), a new syscall. The actual implementation is super simple: essentially it's just the same as fchmodat(), but LOOKUP_FOLLOW is conditionally set based on the flags. I've attempted to make this match "man 2 fchmodat" as closely as possible, which says EINVAL is returned for invalid flags (as opposed to ENOTSUPP, which is currently returned by glibc for AT_SYMLINK_NOFOLLOW). I have a sketch of a glibc patch that I haven't even compiled yet, but seems fairly straight-forward:
diff --git a/sysdeps/unix/sysv/linux/fchmodat.c b/sysdeps/unix/sysv/linux/fchmodat.c index 6d9cbc1ce9e0..b1beab76d56c 100644
--- a/sysdeps/unix/sysv/linux/fchmodat.c
+++ b/sysdeps/unix/sysv/linux/fchmodat.c
@@ -29,12 +29,36 @@
int
fchmodat (int fd, const char *file, mode_t mode, int flag)
{
- if (flag & ~AT_SYMLINK_NOFOLLOW)
- return INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL);
-#ifndef __NR_lchmod /* Linux so far has no lchmod syscall. */
+ /* There are four paths through this code:
+ - The flags are zero. In this case it's fine to call fchmodat.
+ - The flags are non-zero and glibc doesn't have access to
+ __NR_fchmodat4. In this case all we can do is emulate the error codes
+ defined by the glibc interface from userspace.
+ - The flags are non-zero, glibc has __NR_fchmodat4, and the kernel has
+ fchmodat4. This is the simplest case, as the fchmodat4 syscall exactly
+ matches glibc's library interface so it can be called directly.
+ - The flags are non-zero, glibc has __NR_fchmodat4, but the kernel does
+ not. In this case we must respect the error codes defined by the glibc
+ interface instead of returning ENOSYS.
+ The intent here is to ensure that the kernel is called at most once per
+ library call, and that the error types defined by glibc are always
+ respected. */
+
+#ifdef __NR_fchmodat4
+ long result;
+#endif
+
+ if (flag == 0)
+ return INLINE_SYSCALL (fchmodat, 3, fd, file, mode);
+
+#ifdef __NR_fchmodat4
+ result = INLINE_SYSCALL (fchmodat4, 4, fd, file, mode, flag);
+ if (result == 0 || errno != ENOSYS)
+ return result;
+#endif
+
if (flag & AT_SYMLINK_NOFOLLOW)
return INLINE_SYSCALL_ERROR_RETURN_VALUE (ENOTSUP);
-#endif
- return INLINE_SYSCALL (fchmodat, 3, fd, file, mode);
+ return INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL); }
I've never added a new syscall before so I'm not really sure what the proper procedure to follow is. Based on the feedback from my v1 patch set it seems this is somewhat uncontroversial. At this point I don't think there's anything I'm missing, though note that I haven't gotten around to testing it this time because the diff from v1 is trivial for any platform I could reasonably test on. The v1 patches suggest a simple test case, but I didn't re-run it because I don't want to reboot my laptop.
$ touch test-file
$ ln -s test-file test-link
$ cat > test.c
#include <fcntl.h>
#include <stdio.h>
#include <unistd.h> int main(int argc, char **argv)
{
long out;
out = syscall(434, AT_FDCWD, "test-file", 0x888, AT_SYMLINK_NOFOLLOW);
printf("fchmodat4(AT_FDCWD, \"test-file\", 0x888, AT_SYMLINK_NOFOLLOW): %ld\n", out);
out = syscall(434, AT_FDCWD, "test-file", 0x888, 0);
printf("fchmodat4(AT_FDCWD, \"test-file\", 0x888, 0): %ld\n", out);
out = syscall(268, AT_FDCWD, "test-file", 0x888);
printf("fchmodat(AT_FDCWD, \"test-file\", 0x888): %ld\n", out);
out = syscall(434, AT_FDCWD, "test-link", 0x888, AT_SYMLINK_NOFOLLOW);
printf("fchmodat4(AT_FDCWD, \"test-link\", 0x888, AT_SYMLINK_NOFOLLOW): %ld\n", out);
out = syscall(434, AT_FDCWD, "test-link", 0x888, 0);
printf("fchmodat4(AT_FDCWD, \"test-link\", 0x888, 0): %ld\n", out);
out = syscall(268, AT_FDCWD, "test-link", 0x888);
printf("fchmodat(AT_FDCWD, \"test-link\", 0x888): %ld\n", out);
return 0;
}
$ gcc test.c -o test
$ ./test
fchmodat4(AT_FDCWD, "test-file", 0x888, AT_SYMLINK_NOFOLLOW): 0
fchmodat4(AT_FDCWD, "test-file", 0x888, 0): 0
fchmodat(AT_FDCWD, "test-file", 0x888): 0
fchmodat4(AT_FDCWD, "test-link", 0x888, AT_SYMLINK_NOFOLLOW): -1
fchmodat4(AT_FDCWD, "test-link", 0x888, 0): 0
fchmodat(AT_FDCWD, "test-link", 0x888): 0I've only built this on 64-bit x86.
Changes since v1 [[email protected]]:
* All architectures are now supported, which support squashed into a single patch.
* The do_fchmodat() helper function has been removed, in favor of directly calling do_fchmodat4().
* The patches are based on 5.2 instead of 5.1.
PATCH v2 1/4 Non-functional cleanup of a "__user * filename"
The next patch defines a very similar interface, which I copied from this definition. Since I'm touching it anyway I don't see any reason not to just go fix this one up.
--- include/linux/syscalls.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/syscalls.h +1 -1
@@ -431,7 +431,7 @@ asmlinkage long sys_chdir(const char __user *filename);asmlinkage long sys_fchdir (unsigned int fd);asmlinkage long sys_chroot (const char __user *filename);asmlinkage long sys_fchmod (unsigned int fd,umode_t mode); -asmlinkage long sys_fchmodat (int dfd,const char __user * filename, +asmlinkage long sys_fchmodat (int dfd,const char __user *filename,umode_t mode);asmlinkage long sys_fchownat (int dfd,const char __user *filename,uid_t user,gid_t group,int flag);
Signature · 1 line
2.21.0
PATCH v2 2/4 Add fchmodat4(), a new syscall
In reply to Palmer Dabbeltman 3p says that fchmodat() takes a flags argument, but the Linux syscall does not. There doesn't appear to be a good userspace workaround for this issue but the implementation in the kernel is pretty straight-forward. The specific use case where the missing flags came up was WRT a fuse filesystem implemenation, but the functionality is pretty generic so I'm assuming there would be other use cases.
--- fs/open.c | 20 ++++++++++++++++---- include/linux/syscalls.h | 7 +++++-- 2 files changed, 21 insertions(+), 6 deletions(-)
fs/open.c +16 -4
@@ -569,11 +569,17 @@ SYSCALL_DEFINE2(fchmod, unsigned int, fd, umode_t, mode)return ksys_fchmod (fd, mode); } -int do_fchmodat (int dfd,const char __user *filename,umode_t mode) +int do_fchmodat4 (int dfd,const char __user *filename,umode_t mode,int flags) {struct path path;int error; -unsigned int lookup_flags =LOOKUP_FOLLOW ; +unsigned int lookup_flags; + +if (unlikely (flags & ~AT_SYMLINK_NOFOLLOW )) +return -EINVAL ; + + lookup_flags = flags &AT_SYMLINK_NOFOLLOW ?0 :LOOKUP_FOLLOW ; + retry: error =user_path_at (dfd, filename, lookup_flags, &path);if (!error) {
@@ -587,15 +593,21 @@ int do_fchmodat(int dfd, const char __user *filename, umode_t mode)return error; } +SYSCALL_DEFINE4(fchmodat4, int, dfd, const char __user *, filename, + umode_t, mode, int, flags) +{ +return do_fchmodat4 (dfd, filename, mode, flags); +} + SYSCALL_DEFINE3(fchmodat, int, dfd, const char __user *, filename, umode_t, mode) { -return do_fchmodat (dfd, filename, mode); +return do_fchmodat4 (dfd, filename, mode,0 ); } SYSCALL_DEFINE2(chmod, const char __user *, filename, umode_t, mode) { -return do_fchmodat (AT_FDCWD , filename, mode); +return do_fchmodat4 (AT_FDCWD , filename, mode,0 ); }static int chown_common (const struct path *path,uid_t user,gid_t group)
include/linux/syscalls.h +5 -2
@@ -433,6 +433,8 @@ asmlinkage long sys_chroot(const char __user *filename);asmlinkage long sys_fchmod (unsigned int fd,umode_t mode);asmlinkage long sys_fchmodat (int dfd,const char __user *filename,umode_t mode); +asmlinkage long sys_fchmodat4 (int dfd,const char __user *filename, +umode_t mode,int flags);asmlinkage long sys_fchownat (int dfd,const char __user *filename,uid_t user,gid_t group,int flag);asmlinkage long sys_fchown (unsigned int fd,uid_t user,gid_t group);
@@ -1320,11 +1322,12 @@ static inline long ksys_link(const char __user *oldname,return do_linkat (AT_FDCWD , oldname,AT_FDCWD , newname,0 ); } -extern int do_fchmodat (int dfd,const char __user *filename,umode_t mode); +extern int do_fchmodat4 (int dfd,const char __user *filename,umode_t mode, +int flags);static inline int ksys_chmod (const char __user *filename,umode_t mode) { -return do_fchmodat (AT_FDCWD , filename, mode); +return do_fchmodat4 (AT_FDCWD , filename, mode,0 ); }extern long do_faccessat (int dfd,const char __user *filename,int mode);
Signature · 1 line
2.21.0
Al Viro
maintainerRe: [PATCH v2 2/4] Add fchmodat4(), a new syscall
fs/open.c SYSCALL_DEFINE2(fchmod, unsigned int, fd, umode_t, mode) · line 582
Quoted 13 lines · Palmer Dabbelt
fs/open.c +8
-2-int do_fchmodat (int dfd,const char __user *filename,umode_t mode) +int do_fchmodat4 (int dfd,const char __user *filename,umode_t mode,int flags) {struct path path;int error; -unsigned int lookup_flags =LOOKUP_FOLLOW ; +unsigned int lookup_flags; + +if (unlikely (flags & ~AT_SYMLINK_NOFOLLOW )) +return -EINVAL ; + + lookup_flags = flags &AT_SYMLINK_NOFOLLOW ?0 :LOOKUP_FOLLOW ; +
Why not do that in sys_fchmodat4() itself, passing lookup_flags todo_fchmodat() and updating old callers to pass it 0 as extra argument?
Re: [PATCH v2 2/4] Add fchmodat4(), a new syscall
Quoted 18 lines · On Tue, 16 Jul 2019 18:48:02 PDT (-0700), [email protected] wrote:
fs/open.c SYSCALL_DEFINE2(fchmod, unsigned int, fd, umode_t, mode) · line 582
Quoted 13 lines · Palmer Dabbelt
fs/open.c +8
-2-int do_fchmodat (int dfd,const char __user *filename,umode_t mode) +int do_fchmodat4 (int dfd,const char __user *filename,umode_t mode,int flags) {struct path path;int error; -unsigned int lookup_flags =LOOKUP_FOLLOW ; +unsigned int lookup_flags; + +if (unlikely (flags & ~AT_SYMLINK_NOFOLLOW )) +return -EINVAL ; + + lookup_flags = flags &AT_SYMLINK_NOFOLLOW ?0 :LOOKUP_FOLLOW ; +Why not do that in
sys_fchmodat4()itself, passinglookup_flagstodo_fchmodat()and updating old callers to pass it 0 as extra argument?
Ya, that seems better -- passing LOOKUP_FOLLOW instead of 0, to keep the behavior the same. That way I could avoid the overhead of these checks for the old syscalls, as we know they're not necessary.
I'll replace this patch with the following for a v3
diff --git a/fs/open.c b/fs/open.c
index b5b80469b93d..a5f99408af11 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -569,11 +569,12 @@ SYSCALL_DEFINE2(fchmod, unsigned int, fd, umode_t, mode)
return ksys_fchmod(fd, mode);
}
-int do_fchmodat(int dfd, const char __user *filename, umode_t mode)
+int do_fchmodat4(int dfd, const char __user *filename, umode_t mode,
+ int lookup_flags)
{
struct path path;
int error;
- unsigned int lookup_flags = LOOKUP_FOLLOW;
+
retry:
error = user_path_at(dfd, filename, lookup_flags, &path);
if (!error) {
@@ -587,15 +588,28 @@ int do_fchmodat(int dfd, const char __user *filename, umode_t mode)
return error;
} +SYSCALL_DEFINE4(fchmodat4, int, dfd, const char __user *, filename,
+ umode_t, mode, int, flags)
+{
+ unsigned int lookup_flags;
+
+ if (unlikely(flags & ~AT_SYMLINK_NOFOLLOW))
+ return -EINVAL;
+
+ lookup_flags = flags & AT_SYMLINK_NOFOLLOW ? 0 : LOOKUP_FOLLOW;
+
+ return do_fchmodat4(dfd, filename, mode, lookup_flags);
+}
+
SYSCALL_DEFINE3(fchmodat, int, dfd, const char __user *, filename,
umode_t, mode)
{
- return do_fchmodat(dfd, filename, mode);
+ return do_fchmodat4(dfd, filename, mode, LOOKUP_FOLLOW);
}
SYSCALL_DEFINE2(chmod, const char __user *, filename, umode_t, mode)
{ - return do_fchmodat(AT_FDCWD, filename, mode);
+ return do_fchmodat4(AT_FDCWD, filename, mode, LOOKUP_FOLLOW);
}
static int chown_common(const struct path *path, uid_t user, gid_t group)
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h index e1c20f1d0525..6676b1cc5485 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -81,6 +81,7 @@ struct io_uring_params;
#include <linux/quota.h>
#include <linux/key.h>
#include <linux/personality.h>
+#include <linux/namei.h>
#include <trace/syscall.h>
#ifdef CONFIG_ARCH_HAS_SYSCALL_WRAPPER
@@ -433,6 +434,8 @@ asmlinkage long sys_chroot(const char __user *filename);
asmlinkage long sys_fchmod(unsigned int fd, umode_t mode);
asmlinkage long sys_fchmodat(int dfd, const char __user *filename,
umode_t mode);
+asmlinkage long sys_fchmodat4(int dfd, const char __user *filename,
+ umode_t mode, int flags);
asmlinkage long sys_fchownat(int dfd, const char __user *filename, uid_t user,
gid_t group, int flag);
asmlinkage long sys_fchown(unsigned int fd, uid_t user, gid_t group);
@@ -1320,11 +1323,12 @@ static inline long ksys_link(const char __user *oldname,
return do_linkat(AT_FDCWD, oldname, AT_FDCWD, newname, 0);
} -extern int do_fchmodat(int dfd, const char __user *filename, umode_t mode); +extern int do_fchmodat4(int dfd, const char __user *filename, umode_t mode,
+ int flags);
static inline int ksys_chmod(const char __user *filename, umode_t mode) {
- return do_fchmodat(AT_FDCWD, filename, mode);
+ return do_fchmodat4(AT_FDCWD, filename, mode, LOOKUP_FOLLOW);
}
extern long do_faccessat(int dfd, const char __user *filename, int mode);Rich Felker
maintainerRe: [PATCH v2 2/4] Add fchmodat4(), a new syscall
In reply to Palmer DabbeltQuoted 6 lines · Palmer Dabbelt
man 3p says that
fchmodat()takes a flags argument, but the Linux syscall does not. There doesn't appear to be a good userspace workaround for this issue but the implementation in the kernel is pretty straight-forward. The specific use case where the missing flags came up was WRT a fuse filesystem implemenation, but the functionality is pretty generic so I'm assuming there would be other use cases.
Note that we do have a workaround in musl libc with O_PATH and /proc/self/fd, but a syscall that allows a proper fix with the ugly workaround only in the fallback path for old kernels will be much appreciated!
What about also doing a new SYS_faccessat4 with working AT_EACCESS flag? The workaround we have to do for it is far worse.
Rich
Al Viro
maintainerRe: [PATCH v2 2/4] Add fchmodat4(), a new syscall
Quoted 8 lines · Rich Felker
Quoted 6 lines · Palmer Dabbelt
man 3p says that
fchmodat()takes a flags argument, but the Linux syscall does not. There doesn't appear to be a good userspace workaround for this issue but the implementation in the kernel is pretty straight-forward. The specific use case where the missing flags came up was WRT a fuse filesystem implemenation, but the functionality is pretty generic so I'm assuming there would be other use cases.
Quoted 7 lines
Note that we do have a workaround in musl libc with
O_PATHand/proc/self/fd, but a syscall that allows a proper fix with the ugly workaround only in the fallback path for old kernels will be much appreciated!What about also doing a new
SYS_faccessat4with workingAT_EACCESSflag? The workaround we have to do for it is far worse.
Umm... That's doable, but getting into the "don't switch creds unless needed" territory. I'll need to play with that a bit and see what gives a tolerable variant...
What of this part wrt AT_EACCESS?
if (!issecure(SECURE_NO_SETUID_FIXUP)) {
/* Clear the capabilities if we switch to a non-root user */
kuid_t root_uid = make_kuid(override_cred->user_ns, 0);
if (!uid_eq(override_cred->uid, root_uid))
cap_clear(override_cred->cap_effective);
else
override_cred->cap_effective =
override_cred->cap_permitted;
}PATCH v2 3/4 arch: Register fchmodat4, usually as syscall 434
In reply to Palmer DabbeltThis registers the new fchmodat4 syscall in most places as nuber 434, with alpha being the exception where it's 544. I found all these sites by grepping for fspick, which I assume has found me everything.
--- arch/alpha/kernel/syscalls/syscall.tbl | 1 + arch/arm/tools/syscall.tbl | 1 + arch/arm64/include/asm/unistd32.h | 2 ++ arch/ia64/kernel/syscalls/syscall.tbl | 1 + arch/m68k/kernel/syscalls/syscall.tbl | 1 + arch/microblaze/kernel/syscalls/syscall.tbl | 1 + arch/mips/kernel/syscalls/syscall_n32.tbl | 1 + arch/mips/kernel/syscalls/syscall_n64.tbl | 1 + arch/mips/kernel/syscalls/syscall_o32.tbl | 1 + arch/parisc/kernel/syscalls/syscall.tbl | 1 + arch/powerpc/kernel/syscalls/syscall.tbl | 1 + arch/s390/kernel/syscalls/syscall.tbl | 1 + arch/sh/kernel/syscalls/syscall.tbl | 1 + arch/sparc/kernel/syscalls/syscall.tbl | 1 + arch/x86/entry/syscalls/syscall_32.tbl | 1 + arch/x86/entry/syscalls/syscall_64.tbl | 1 + include/uapi/asm-generic/unistd.h | 5 ++++- 17 files changed, 21 insertions(+), 1 deletion(-)
arch/alpha/kernel/syscalls/syscall.tbl +1 -0
@@ -473,3 +473,4 @@ 541 common fsconfig sys_fsconfig 542 common fsmount sys_fsmount 543 common fspick sys_fspick +544 common fcmodat4 sys_fchmodat4
arch/arm/tools/syscall.tbl +1 -0
@@ -447,3 +447,4 @@ 431 common fsconfig sys_fsconfig 432 common fsmount sys_fsmount 433 common fspick sys_fspick +434 common fchmodat4 sys_fchmodat4
arch/arm64/include/asm/unistd32.h +2 -0
@@ -875,6 +875,8 @@ __SYSCALL(__NR_fsconfig, sys_fsconfig) __SYSCALL(__NR_fsmount, sys_fsmount)#define __NR_fspick 433 __SYSCALL(__NR_fspick, sys_fspick) +#define __NR_fchmodat4 434 +__SYSCALL (__NR_fchmodat4, sys_fchmodat4)/* * Please add new compat syscalls above this comment and update
arch/ia64/kernel/syscalls/syscall.tbl +1 -0
@@ -354,3 +354,4 @@ 431 common fsconfig sys_fsconfig 432 common fsmount sys_fsmount 433 common fspick sys_fspick +434 common fchmodat4 sys_fchmodat4
arch/m68k/kernel/syscalls/syscall.tbl +1 -0
@@ -433,3 +433,4 @@ 431 common fsconfig sys_fsconfig 432 common fsmount sys_fsmount 433 common fspick sys_fspick +434 common fchmodat4 sys_fchmodat4
arch/microblaze/kernel/syscalls/syscall.tbl +1 -0
@@ -439,3 +439,4 @@ 431 common fsconfig sys_fsconfig 432 common fsmount sys_fsmount 433 common fspick sys_fspick +434 common fchmodat4 sys_fchmodat4
arch/mips/kernel/syscalls/syscall_n32.tbl +1 -0
@@ -372,3 +372,4 @@ 431 n32 fsconfig sys_fsconfig 432 n32 fsmount sys_fsmount 433 n32 fspick sys_fspick +434 n32 fchmodat4 sys_fchmodat4
arch/mips/kernel/syscalls/syscall_n64.tbl +1 -0
@@ -348,3 +348,4 @@ 431 n64 fsconfig sys_fsconfig 432 n64 fsmount sys_fsmount 433 n64 fspick sys_fspick +434 n64 fchmodat4 sys_fchmodat4
arch/mips/kernel/syscalls/syscall_o32.tbl +1 -0
@@ -421,3 +421,4 @@ 431 o32 fsconfig sys_fsconfig 432 o32 fsmount sys_fsmount 433 o32 fspick sys_fspick +434 o32 fchmodat4 sys_fchmodat4
arch/parisc/kernel/syscalls/syscall.tbl +1 -0
@@ -430,3 +430,4 @@ 431 common fsconfig sys_fsconfig 432 common fsmount sys_fsmount 433 common fspick sys_fspick +434 common fchmodat4 sys_fchmodat4
arch/powerpc/kernel/syscalls/syscall.tbl +1 -0
@@ -515,3 +515,4 @@ 431 common fsconfig sys_fsconfig 432 common fsmount sys_fsmount 433 common fspick sys_fspick +434 common fchmodat4 sys_fchmodat4
arch/s390/kernel/syscalls/syscall.tbl +1 -0
@@ -436,3 +436,4 @@ 431 common fsconfig sys_fsconfig sys_fsconfig 432 common fsmount sys_fsmount sys_fsmount 433 common fspick sys_fspick sys_fspick +434 common fchmodat4 sys_fchmodat4 sys_fchmodat4
arch/sh/kernel/syscalls/syscall.tbl +1 -0
@@ -436,3 +436,4 @@ 431 common fsconfig sys_fsconfig 432 common fsmount sys_fsmount 433 common fspick sys_fspick +434 common fchmodat4 sys_fchmodat4
arch/sparc/kernel/syscalls/syscall.tbl +1 -0
@@ -479,3 +479,4 @@ 431 common fsconfig sys_fsconfig 432 common fsmount sys_fsmount 433 common fspick sys_fspick +434 common fchmodat4 sys_fchmodat4
arch/x86/entry/syscalls/syscall_32.tbl +1 -0
@@ -438,3 +438,4 @@ 431 i386 fsconfig sys_fsconfig __ia32_sys_fsconfig 432 i386 fsmount sys_fsmount __ia32_sys_fsmount 433 i386 fspick sys_fspick __ia32_sys_fspick +434 i386 fchmodat4 sys_fchmodat4 __ia32_sys_fchmodat4
arch/x86/entry/syscalls/syscall_64.tbl +1 -0
@@ -355,6 +355,7 @@ 431 common fsconfig __x64_sys_fsconfig 432 common fsmount __x64_sys_fsmount 433 common fspick __x64_sys_fspick +434 common fchmodat4 __x64_sys_fchmodat4 # # x32-specific system call numbers start at 512 to avoid cache impact
include/uapi/asm-generic/unistd.h +4 -1
@@ -845,8 +845,11 @@ __SYSCALL(__NR_fsmount, sys_fsmount)#define __NR_fspick 433 __SYSCALL(__NR_fspick, sys_fspick) +#define __NR_fchmodat4 434 +__SYSCALL(__NR_fchmodat4, sys_fchmodat4) +#undef __NR_syscalls -#define __NR_syscalls 434 +#define __NR_syscalls 435/* * 32 bit systems traditionally used different
Signature · 1 line
2.21.0
Arnd Bergmann
maintainerRe: [PATCH v2 3/4] arch: Register fchmodat4, usually as syscall 434
Quoted 4 lines · Palmer Dabbelt
This registers the new fchmodat4 syscall in most places as nuber 434, with alpha being the exception where it's 544. I found all these sites by grepping for fspick, which I assume has found me everything.
434 is now pidfd_open, the next free one is 436.
Quoted 3 lines
arch/alpha/kernel/syscalls/syscall.tbl | 1 + arch/arm/tools/syscall.tbl | 1 + arch/arm64/include/asm/unistd32.h | 2 ++
You missed arch/arm64/include/asm/unistd.h, which
contains __NR_compat_syscalls
Arnd
PATCH v2 4/4 tools: Add fchmodat4
In reply to Palmer DabbeltI'm not sure why it's necessary to add this explicitly to tools/ as well as arch/, but there were a few instances of fspick in here so I blindly added fchmodat4 in the same fashion.
--- tools/include/uapi/asm-generic/unistd.h | 4 +++- tools/perf/arch/x86/entry/syscalls/syscall_64.tbl | 1 + 2 files changed, 4 insertions(+), 1 deletion(-)
tools/include/uapi/asm-generic/unistd.h +3 -1
@@ -844,9 +844,11 @@ __SYSCALL(__NR_fsconfig, sys_fsconfig) __SYSCALL(__NR_fsmount, sys_fsmount)#define __NR_fspick 433 __SYSCALL(__NR_fspick, sys_fspick) +#define __NR_fchmodat4 434 +__SYSCALL(__NR_fchmodat4, sys_fchmodat4)#undef __NR_syscalls -#define __NR_syscalls 434 +#define __NR_syscalls 435/* * 32 bit systems traditionally used different
tools/perf/arch/x86/entry/syscalls/syscall_64.tbl +1 -0
@@ -355,6 +355,7 @@ 431 common fsconfig __x64_sys_fsconfig 432 common fsmount __x64_sys_fsmount 433 common fspick __x64_sys_fspick +434 common fchmodat4 __x64_sys_fchmodat4 # # x32-specific system call numbers start at 512 to avoid cache impact
Signature · 1 line
2.21.0
Re: [PATCH v2 4/4] tools: Add fchmodat4
Em Tue, Jul 16, 2019 at 06:27:19PM -0700, Palmer Dabbelt escreveu:
Quoted 3 lines
I'm not sure why it's necessary to add this explicitly to
tools/as well asarch/, but there were a few instances of fspick in here so I blindly added fchmodat4 in the same fashion.
The copies in tools/ for these specific files are used to generate a syscall table used by 'perf trace', and we don't/can't access files outside of tools/ to build tools/perf/, so we grab a copy and have checks in place to warn perf developers when those copies get out of sync.
Its not required that kernel developers update anything in tools, you're welcomed to do so if you wish tho.
Thanks,
- Arnaldo
Quoted 37 lines
Signed-off-by Palmer Dabbelt <[email protected]>--- tools/include/uapi/asm-generic/unistd.h | 4 +++- tools/perf/arch/x86/entry/syscalls/syscall_64.tbl | 1 + 2 files changed, 4 insertions(+), 1 deletion(-)tools/include/uapi/asm-generic/unistd.h +3
-1@@ -844,9 +844,11 @@ __SYSCALL(__NR_fsconfig, sys_fsconfig) __SYSCALL(__NR_fsmount, sys_fsmount)#define __NR_fspick 433 __SYSCALL(__NR_fspick, sys_fspick) +#define __NR_fchmodat4 434 +__SYSCALL(__NR_fchmodat4, sys_fchmodat4)#undef __NR_syscalls -#define __NR_syscalls 434 +#define __NR_syscalls 435/* * 32 bit systems traditionally used different tools/perf/arch/x86/entry/syscalls/syscall_64.tbl +1
-0@@ -355,6 +355,7 @@ 431 common fsconfig __x64_sys_fsconfig 432 common fsmount __x64_sys_fsmount 433 common fspick __x64_sys_fspick +434 common fchmodat4 __x64_sys_fchmodat4 # # x32-specific system call numbers start at 512 to avoid cache impactSignature · 1 line
2.21.0
Signature · 1 line
- Arnaldo
* Palmer Dabbelt:
Quoted 8 lines
This patch set adds
fchmodat4(), a new syscall. The actual implementation is super simple: essentially it's just the same asfchmodat(), butLOOKUP_FOLLOWis conditionally set based on the flags. I've attempted to make this match "man 2 fchmodat" as closely as possible, which says EINVAL is returned for invalid flags (as opposed to ENOTSUPP, which is currently returned by glibc forAT_SYMLINK_NOFOLLOW). I have a sketch of a glibc patch that I haven't even compiled yet, but seems fairly straight-forward:
What's the status here? We'd really like to see this system call in the kernel because our emulation in glibc has its problems (especially if /proc is not mounted).
Thanks,
Florian
v3 cover Add a new fchmodat4() syscall
This patch set adds fchmodat4(), a new syscall. The actual implementation is super simple: essentially it's just the same as fchmodat(), but LOOKUP_FOLLOW is conditionally set based on the flags. I've attempted to make this match "man 2 fchmodat" as closely as possible, which says EINVAL is returned for invalid flags (as opposed to ENOTSUPP, which is currently returned by glibc for AT_SYMLINK_NOFOLLOW). I have a sketch of a glibc patch that I haven't even compiled yet, but seems fairly straight-forward:
diff --git a/sysdeps/unix/sysv/linux/fchmodat.c b/sysdeps/unix/sysv/linux/fchmodat.c index 6d9cbc1ce9e0..b1beab76d56c 100644
--- a/sysdeps/unix/sysv/linux/fchmodat.c
+++ b/sysdeps/unix/sysv/linux/fchmodat.c
@@ -29,12 +29,36 @@
int
fchmodat (int fd, const char *file, mode_t mode, int flag)
{
- if (flag & ~AT_SYMLINK_NOFOLLOW)
- return INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL);
-#ifndef __NR_lchmod /* Linux so far has no lchmod syscall. */
+ /* There are four paths through this code:
+ - The flags are zero. In this case it's fine to call fchmodat.
+ - The flags are non-zero and glibc doesn't have access to
+ __NR_fchmodat4. In this case all we can do is emulate the error codes
+ defined by the glibc interface from userspace.
+ - The flags are non-zero, glibc has __NR_fchmodat4, and the kernel has
+ fchmodat4. This is the simplest case, as the fchmodat4 syscall exactly
+ matches glibc's library interface so it can be called directly.
+ - The flags are non-zero, glibc has __NR_fchmodat4, but the kernel does
+ not. In this case we must respect the error codes defined by the glibc
+ interface instead of returning ENOSYS.
+ The intent here is to ensure that the kernel is called at most once per
+ library call, and that the error types defined by glibc are always
+ respected. */
+
+#ifdef __NR_fchmodat4
+ long result;
+#endif
+
+ if (flag == 0)
+ return INLINE_SYSCALL (fchmodat, 3, fd, file, mode);
+
+#ifdef __NR_fchmodat4
+ result = INLINE_SYSCALL (fchmodat4, 4, fd, file, mode, flag);
+ if (result == 0 || errno != ENOSYS)
+ return result;
+#endif
+
if (flag & AT_SYMLINK_NOFOLLOW)
return INLINE_SYSCALL_ERROR_RETURN_VALUE (ENOTSUP);
-#endif
- return INLINE_SYSCALL (fchmodat, 3, fd, file, mode);
+ return INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL); }
I've never added a new syscall before so I'm not really sure what the proper procedure to follow is. Based on the feedback from my v1 patch set it seems this is somewhat uncontroversial. At this point I don't think there's anything I'm missing, though note that I haven't gotten around to testing it this time because the diff from v1 is trivial for any platform I could reasonably test on. The v1 patches suggest a simple test case, but I didn't re-run it because I don't want to reboot my laptop.
Changes since v2 [[email protected]]:
* Rebased to master.
* The lookup_flags passed to sys_fchmodat4 as suggested by Al Viro.
* Selftest added.
Changes since v1 [[email protected]]:
* All architectures are now supported, which support squashed into a single patch.
* The do_fchmodat() helper function has been removed, in favor of directly calling do_fchmodat4().
* The patches are based on 5.2 instead of 5.1.
---
Alexey Gladkov (1):
selftests: add fchmodat4(2) selftest
Palmer Dabbelt (4):
Non-functional cleanup of a "__user * filename"
fs: Add fchmodat4()
arch: Register fchmodat4, usually as syscall 451 tools headers UAPI: Sync files changed by new fchmodat4 syscall
arch/alpha/kernel/syscalls/syscall.tbl | 1 + arch/arm/tools/syscall.tbl | 1 + arch/arm64/include/asm/unistd32.h | 2 + arch/ia64/kernel/syscalls/syscall.tbl | 1 + arch/m68k/kernel/syscalls/syscall.tbl | 1 + arch/microblaze/kernel/syscalls/syscall.tbl | 1 + arch/mips/kernel/syscalls/syscall_n32.tbl | 1 + arch/mips/kernel/syscalls/syscall_n64.tbl | 1 + arch/mips/kernel/syscalls/syscall_o32.tbl | 1 + arch/parisc/kernel/syscalls/syscall.tbl | 1 + arch/powerpc/kernel/syscalls/syscall.tbl | 1 + arch/s390/kernel/syscalls/syscall.tbl | 1 + arch/sh/kernel/syscalls/syscall.tbl | 1 + arch/sparc/kernel/syscalls/syscall.tbl | 1 + arch/x86/entry/syscalls/syscall_32.tbl | 1 + arch/x86/entry/syscalls/syscall_64.tbl | 1 + arch/xtensa/kernel/syscalls/syscall.tbl | 1 + fs/open.c | 18 ++- include/linux/syscalls.h | 4 +- include/uapi/asm-generic/unistd.h | 5 +- tools/include/uapi/asm-generic/unistd.h | 5 +- .../arch/mips/entry/syscalls/syscall_n64.tbl | 1 + .../arch/powerpc/entry/syscalls/syscall.tbl | 1 + .../perf/arch/s390/entry/syscalls/syscall.tbl | 1 + .../arch/x86/entry/syscalls/syscall_64.tbl | 1 + tools/testing/selftests/Makefile | 1 + tools/testing/selftests/fchmodat4/.gitignore | 2 + tools/testing/selftests/fchmodat4/Makefile | 6 + .../selftests/fchmodat4/fchmodat4_test.c | 151 ++++++++++++++++++ 29 files changed, 207 insertions(+), 7 deletions(-) create mode 100644 tools/testing/selftests/fchmodat4/.gitignore create mode 100644 tools/testing/selftests/fchmodat4/Makefile create mode 100644 tools/testing/selftests/fchmodat4/fchmodat4_test.c
Signature · 1 line
2.33.8
PATCH v3 1/5 Non-functional cleanup of a "__user * filename"
From: Palmer Dabbelt <[email protected]>
The next patch defines a very similar interface, which I copied from this definition. Since I'm touching it anyway I don't see any reason not to just go fix this one up.
--- include/linux/syscalls.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/syscalls.h +1 -1
@@ -464,7 +464,7 @@ asmlinkage long sys_chdir(const char __user *filename);asmlinkage long sys_fchdir (unsigned int fd);asmlinkage long sys_chroot (const char __user *filename);asmlinkage long sys_fchmod (unsigned int fd,umode_t mode); -asmlinkage long sys_fchmodat (int dfd,const char __user * filename, +asmlinkage long sys_fchmodat (int dfd,const char __user *filename,umode_t mode);asmlinkage long sys_fchownat (int dfd,const char __user *filename,uid_t user,gid_t group,int flag);
Signature · 1 line
2.33.8
Re: [PATCH v3 1/5] Non-functional cleanup of a "__user * filename"
Quoted 7 lines · Alexey Gladkov
From: Palmer Dabbelt <[email protected]>
The next patch defines a very similar interface, which I copied from this definition. Since I'm touching it anyway I don't see any reason not to just go fix this one up.
Signed-off-by Palmer Dabbelt <[email protected]>
PATCH v3 2/5 fs: Add fchmodat4()
In reply to Alexey GladkovFrom: Palmer Dabbelt <[email protected]>
On the userspace side fchmodat(3) is implemented as a wrapper function which implements the POSIX-specified interface. This interface differs from the underlying kernel system call, which does not have a flags argument. Most implementations require procfs [1][2].
There doesn't appear to be a good userspace workaround for this issue but the implementation in the kernel is pretty straight-forward.
The new fchmodat4() syscall allows to pass the AT_SYMLINK_NOFOLLOW flag, unlike existing fchmodat.
[1] https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/fchmodat.c;h=17eca54051ee28ba1ec3f9aed170a62630959143;hb=a492b1e5ef7ab50c6fdd4e4e9879ea5569ab0a6c#l35
[2] https://git.musl-libc.org/cgit/musl/tree/src/stat/fchmodat.c?id=718f363bc2067b6487900eddc9180c84e7739f80#n28
--- fs/open.c | 18 ++++++++++++++---- include/linux/syscalls.h | 2 ++ 2 files changed, 16 insertions(+), 4 deletions(-)
fs/open.c +14 -4
@@ -671,11 +671,11 @@ SYSCALL_DEFINE2(fchmod, unsigned int, fd, umode_t, mode)return err; } -static int do_fchmodat (int dfd,const char __user *filename,umode_t mode) +static int do_fchmodat4 (int dfd,const char __user *filename,umode_t mode,int lookup_flags) {struct path path;int error; -unsigned int lookup_flags =LOOKUP_FOLLOW ; + retry: error =user_path_at (dfd, filename, lookup_flags, &path);if (!error) {
@@ -689,15 +689,25 @@ static int do_fchmodat(int dfd, const char __user *filename, umode_t mode)return error; } +SYSCALL_DEFINE4(fchmodat4, int, dfd, const char __user *, filename, + umode_t, mode, int, flags) +{ +if (unlikely (flags & ~AT_SYMLINK_NOFOLLOW )) +return -EINVAL ; + +return do_fchmodat4 (dfd, filename, mode, + flags &AT_SYMLINK_NOFOLLOW ?0 :LOOKUP_FOLLOW ); +} + SYSCALL_DEFINE3(fchmodat, int, dfd, const char __user *, filename, umode_t, mode) { -return do_fchmodat (dfd, filename, mode); +return do_fchmodat4 (dfd, filename, mode,LOOKUP_FOLLOW ); } SYSCALL_DEFINE2(chmod, const char __user *, filename, umode_t, mode) { -return do_fchmodat (AT_FDCWD , filename, mode); +return do_fchmodat4 (AT_FDCWD , filename, mode,LOOKUP_FOLLOW ); }/**
include/linux/syscalls.h +2 -0
@@ -466,6 +466,8 @@ asmlinkage long sys_chroot(const char __user *filename);asmlinkage long sys_fchmod (unsigned int fd,umode_t mode);asmlinkage long sys_fchmodat (int dfd,const char __user *filename,umode_t mode); +asmlinkage long sys_fchmodat4 (int dfd,const char __user *filename, +umode_t mode,int flags);asmlinkage long sys_fchownat (int dfd,const char __user *filename,uid_t user,gid_t group,int flag);asmlinkage long sys_fchown (unsigned int fd,uid_t user,gid_t group);
Signature · 1 line
2.33.8
Re: [PATCH v3 2/5] fs: Add fchmodat4()
Quoted 13 lines · Alexey Gladkov
From: Palmer Dabbelt <[email protected]>
On the userspace side fchmodat(3) is implemented as a wrapper function which implements the POSIX-specified interface. This interface differs from the underlying kernel system call, which does not have a flags argument. Most implementations require procfs [1][2].
There doesn't appear to be a good userspace workaround for this issue but the implementation in the kernel is pretty straight-forward.
The new
fchmodat4()syscall allows to pass theAT_SYMLINK_NOFOLLOWflag, unlike existing fchmodat.
Quoted 7 lines
[1]
https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/fchmodat.c;h=17eca54051ee28ba1ec3f9aed170a62630959143;hb=a492b1e5ef7ab50c6fdd4e4e9879ea5569ab0a6c#l35
[2]
https://git.musl-libc.org/cgit/musl/tree/src/stat/fchmodat.c?id=718f363bc2067b6487900eddc9180c84e7739f80#n28Signed-off-by Palmer Dabbelt <[email protected]>Signed-off-by Alexey Gladkov <[email protected]>
I don't know the history of why we ended up with the different interface, or whether this was done intentionally in the kernel or if we want this syscall.
Assuming this is in fact needed, I double-checked that the implementation looks correct to me and is portable to all the architectures, without the need for a compat wrapper.
Christian Brauner
maintainerRe: [PATCH v3 2/5] fs: Add fchmodat4()
Quoted 26 lines · Arnd Bergmann
Quoted 13 lines · Alexey Gladkov
From: Palmer Dabbelt <[email protected]>
On the userspace side fchmodat(3) is implemented as a wrapper function which implements the POSIX-specified interface. This interface differs from the underlying kernel system call, which does not have a flags argument. Most implementations require procfs [1][2].
There doesn't appear to be a good userspace workaround for this issue but the implementation in the kernel is pretty straight-forward.
The new
fchmodat4()syscall allows to pass theAT_SYMLINK_NOFOLLOWflag, unlike existing fchmodat.Quoted 7 lines
[1]
https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/fchmodat.c;h=17eca54051ee28ba1ec3f9aed170a62630959143;hb=a492b1e5ef7ab50c6fdd4e4e9879ea5569ab0a6c#l35
[2]
https://git.musl-libc.org/cgit/musl/tree/src/stat/fchmodat.c?id=718f363bc2067b6487900eddc9180c84e7739f80#n28Signed-off-by Palmer Dabbelt <[email protected]>Signed-off-by Alexey Gladkov <[email protected]>I don't know the history of why we ended up with the different interface, or whether this was done intentionally in the kernel or if we want this syscall.
Quoted 5 lines
Assuming this is in fact needed, I double-checked that the implementation looks correct to me and is portable to all the architectures, without the need for a compat wrapper.
Acked-by Arnd Bergmann <[email protected]>
The system call itself is useful afaict. But please,
s/fchmodat4/fchmodat2/
With very few exceptions we don't version by argument number but by revision and we should stick to one scheme:
openat()->openat2()
eventfd()->eventfd2() clone()/clone2()->clone3() dup()->dup2()->dup3() // coincides with nr of arguments pipe()->pipe2() // coincides with nr of arguments renameat()->renameat2()
Re: [PATCH v3 2/5] fs: Add fchmodat4()
Quoted 31 lines · Christian Brauner
Quoted 30 lines · Arnd Bergmann
Quoted 13 lines · Alexey Gladkov
From: Palmer Dabbelt <[email protected]>
On the userspace side fchmodat(3) is implemented as a wrapper function which implements the POSIX-specified interface. This interface differs from the underlying kernel system call, which does not have a flags argument. Most implementations require procfs [1][2].
There doesn't appear to be a good userspace workaround for this issue but the implementation in the kernel is pretty straight-forward.
The new
fchmodat4()syscall allows to pass theAT_SYMLINK_NOFOLLOWflag, unlike existing fchmodat.Quoted 7 lines
[1]
https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/fchmodat.c;h=17eca54051ee28ba1ec3f9aed170a62630959143;hb=a492b1e5ef7ab50c6fdd4e4e9879ea5569ab0a6c#l35
[2]
https://git.musl-libc.org/cgit/musl/tree/src/stat/fchmodat.c?id=718f363bc2067b6487900eddc9180c84e7739f80#n28Signed-off-by Palmer Dabbelt <[email protected]>Signed-off-by Alexey Gladkov <[email protected]>I don't know the history of why we ended up with the different interface, or whether this was done intentionally in the kernel or if we want this syscall.
Assuming this is in fact needed, I double-checked that the implementation looks correct to me and is portable to all the architectures, without the need for a compat wrapper.
Quoted 5 lines
Quoted 1 line
Acked-by Arnd Bergmann <[email protected]>The system call itself is useful afaict. But please,
s/fchmodat4/fchmodat2/
Sure. I will.
Quoted 10 lines
With very few exceptions we don't version by argument number but by revision and we should stick to one scheme:
openat()->openat2()
eventfd()->eventfd2() clone()/clone2()->clone3() dup()->dup2()->dup3() // coincides with nr of arguments pipe()->pipe2() // coincides with nr of arguments renameat()->renameat2()
Signature · 1 line
Rgrds, legion
Christian Brauner
maintainerRe: [PATCH v3 2/5] fs: Add fchmodat4()
Quoted 32 lines · Alexey Gladkov
Quoted 31 lines · Christian Brauner
Quoted 30 lines · Arnd Bergmann
Quoted 13 lines · Alexey Gladkov
From: Palmer Dabbelt <[email protected]>
On the userspace side fchmodat(3) is implemented as a wrapper function which implements the POSIX-specified interface. This interface differs from the underlying kernel system call, which does not have a flags argument. Most implementations require procfs [1][2].
There doesn't appear to be a good userspace workaround for this issue but the implementation in the kernel is pretty straight-forward.
The new
fchmodat4()syscall allows to pass theAT_SYMLINK_NOFOLLOWflag, unlike existing fchmodat.Quoted 7 lines
[1]
https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/fchmodat.c;h=17eca54051ee28ba1ec3f9aed170a62630959143;hb=a492b1e5ef7ab50c6fdd4e4e9879ea5569ab0a6c#l35
[2]
https://git.musl-libc.org/cgit/musl/tree/src/stat/fchmodat.c?id=718f363bc2067b6487900eddc9180c84e7739f80#n28Signed-off-by Palmer Dabbelt <[email protected]>Signed-off-by Alexey Gladkov <[email protected]>I don't know the history of why we ended up with the different interface, or whether this was done intentionally in the kernel or if we want this syscall.
Assuming this is in fact needed, I double-checked that the implementation looks correct to me and is portable to all the architectures, without the need for a compat wrapper.
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
Acked-by Arnd Bergmann <[email protected]>The system call itself is useful afaict. But please,
s/fchmodat4/fchmodat2/
Sure. I will.
Thanks. Can you also wire this up for every architecture, please? I don't see that this has been done in this series.
Re: [PATCH v3 2/5] fs: Add fchmodat4()
Quoted 35 lines · Christian Brauner
Quoted 34 lines · Alexey Gladkov
Quoted 33 lines · Christian Brauner
Quoted 31 lines · Arnd Bergmann
Quoted 13 lines · Alexey Gladkov
From: Palmer Dabbelt <[email protected]>
On the userspace side fchmodat(3) is implemented as a wrapper function which implements the POSIX-specified interface. This interface differs from the underlying kernel system call, which does not have a flags argument. Most implementations require procfs [1][2].
There doesn't appear to be a good userspace workaround for this issue but the implementation in the kernel is pretty straight-forward.
The new
fchmodat4()syscall allows to pass theAT_SYMLINK_NOFOLLOWflag, unlike existing fchmodat.Quoted 7 lines
[1]
https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/fchmodat.c;h=17eca54051ee28ba1ec3f9aed170a62630959143;hb=a492b1e5ef7ab50c6fdd4e4e9879ea5569ab0a6c#l35
[2]
https://git.musl-libc.org/cgit/musl/tree/src/stat/fchmodat.c?id=718f363bc2067b6487900eddc9180c84e7739f80#n28Signed-off-by Palmer Dabbelt <[email protected]>Signed-off-by Alexey Gladkov <[email protected]>I don't know the history of why we ended up with the different interface, or whether this was done intentionally in the kernel or if we want this syscall.
Assuming this is in fact needed, I double-checked that the implementation looks correct to me and is portable to all the architectures, without the need for a compat wrapper.
Acked-by Arnd Bergmann <[email protected]>
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
The system call itself is useful afaict. But please,
s/fchmodat4/fchmodat2/
Sure. I will.
Thanks. Can you also wire this up for every architecture, please? I don't see that this has been done in this series.
Sure. I have already added in all architectures as far as I can tell:
$ diff -s <(find arch/ -name '*.tbl' |sort -u) <(git grep -lw fchmodat2 arch/ |sort -u) Files /dev/fd/63 and /dev/fd/62 are identical
Signature · 1 line
Rgrds, legion
Re: [PATCH v3 2/5] fs: Add fchmodat4()
In reply to Alexey Gladkovfs/open.c SYSCALL_DEFINE2(fchmod, unsigned int, fd, umode_t, mode) · line 674
Quoted 2 lines · Alexey Gladkov
fs/open.c +1
-1-static int do_fchmodat (int dfd,const char __user *filename,umode_t mode) +static int do_fchmodat4 (int dfd,const char __user *filename,umode_t mode,int lookup_flags)
This function can still be called do_fchmodat(); we don't need to version internal functions.
Re: [PATCH v3 2/5] fs: Add fchmodat4()
Quoted 6 lines · Matthew Wilcox
fs/open.c SYSCALL_DEFINE2(fchmod, unsigned int, fd, umode_t, mode) · line 674
Quoted 2 lines · Alexey Gladkov
fs/open.c +1
-1-static int do_fchmodat (int dfd,const char __user *filename,umode_t mode) +static int do_fchmodat4 (int dfd,const char __user *filename,umode_t mode,int lookup_flags)This function can still be called
do_fchmodat(); we don't need to version internal functions.
Yes. I tried not to change too much when adopting a patch. In the new version, I will return the old name. Thanks.
Signature · 1 line
Rgrds, legion
PATCH v3 3/5 arch: Register fchmodat4, usually as syscall 451
In reply to Alexey GladkovFrom: Palmer Dabbelt <[email protected]>
This registers the new fchmodat4 syscall in most places as nuber 451, with alpha being the exception where it's 561. I found all these sites by grepping for fspick, which I assume has found me everything.
--- arch/alpha/kernel/syscalls/syscall.tbl | 1 + arch/arm/tools/syscall.tbl | 1 + arch/arm64/include/asm/unistd32.h | 2 ++ arch/ia64/kernel/syscalls/syscall.tbl | 1 + arch/m68k/kernel/syscalls/syscall.tbl | 1 + arch/microblaze/kernel/syscalls/syscall.tbl | 1 + arch/mips/kernel/syscalls/syscall_n32.tbl | 1 + arch/mips/kernel/syscalls/syscall_n64.tbl | 1 + arch/mips/kernel/syscalls/syscall_o32.tbl | 1 + arch/parisc/kernel/syscalls/syscall.tbl | 1 + arch/powerpc/kernel/syscalls/syscall.tbl | 1 + arch/s390/kernel/syscalls/syscall.tbl | 1 + arch/sh/kernel/syscalls/syscall.tbl | 1 + arch/sparc/kernel/syscalls/syscall.tbl | 1 + arch/x86/entry/syscalls/syscall_32.tbl | 1 + arch/x86/entry/syscalls/syscall_64.tbl | 1 + arch/xtensa/kernel/syscalls/syscall.tbl | 1 + include/uapi/asm-generic/unistd.h | 5 ++++- 18 files changed, 22 insertions(+), 1 deletion(-)
arch/alpha/kernel/syscalls/syscall.tbl +1 -0
@@ -490,3 +490,4 @@ 558 common process_mrelease sys_process_mrelease 559 common futex_waitv sys_futex_waitv 560 common set_mempolicy_home_node sys_ni_syscall +561 common fchmodat4 sys_fchmodat4
arch/arm/tools/syscall.tbl +1 -0
@@ -464,3 +464,4 @@ 448 common process_mrelease sys_process_mrelease 449 common futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node +451 common fchmodat4 sys_fchmodat4
arch/arm64/include/asm/unistd32.h +2 -0
@@ -907,6 +907,8 @@ __SYSCALL(__NR_process_mrelease, sys_process_mrelease) __SYSCALL(__NR_futex_waitv, sys_futex_waitv)#define __NR_set_mempolicy_home_node 450 __SYSCALL(__NR_set_mempolicy_home_node, sys_set_mempolicy_home_node) +#define __NR_fchmodat4 451 +__SYSCALL (__NR_fchmodat4, sys_fchmodat4)/* * Please add new compat syscalls above this comment and update
arch/ia64/kernel/syscalls/syscall.tbl +1 -0
@@ -371,3 +371,4 @@ 448 common process_mrelease sys_process_mrelease 449 common futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node +451 common fchmodat4 sys_fchmodat4
arch/m68k/kernel/syscalls/syscall.tbl +1 -0
@@ -450,3 +450,4 @@ 448 common process_mrelease sys_process_mrelease 449 common futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node +451 common fchmodat4 sys_fchmodat4
arch/microblaze/kernel/syscalls/syscall.tbl +1 -0
@@ -456,3 +456,4 @@ 448 common process_mrelease sys_process_mrelease 449 common futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node +451 common fchmodat4 sys_fchmodat4
arch/mips/kernel/syscalls/syscall_n32.tbl +1 -0
@@ -389,3 +389,4 @@ 448 n32 process_mrelease sys_process_mrelease 449 n32 futex_waitv sys_futex_waitv 450 n32 set_mempolicy_home_node sys_set_mempolicy_home_node +451 n32 fchmodat4 sys_fchmodat4
arch/mips/kernel/syscalls/syscall_n64.tbl +1 -0
@@ -365,3 +365,4 @@ 448 n64 process_mrelease sys_process_mrelease 449 n64 futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node +451 n64 fchmodat4 sys_fchmodat4
arch/mips/kernel/syscalls/syscall_o32.tbl +1 -0
@@ -438,3 +438,4 @@ 448 o32 process_mrelease sys_process_mrelease 449 o32 futex_waitv sys_futex_waitv 450 o32 set_mempolicy_home_node sys_set_mempolicy_home_node +451 o32 fchmodat4 sys_fchmodat4
arch/parisc/kernel/syscalls/syscall.tbl +1 -0
@@ -448,3 +448,4 @@ 448 common process_mrelease sys_process_mrelease 449 common futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node +451 common fchmodat4 sys_fchmodat4
arch/powerpc/kernel/syscalls/syscall.tbl +1 -0
@@ -537,3 +537,4 @@ 448 common process_mrelease sys_process_mrelease 449 common futex_waitv sys_futex_waitv 450 nospu set_mempolicy_home_node sys_set_mempolicy_home_node +451 common fchmodat4 sys_fchmodat4
arch/s390/kernel/syscalls/syscall.tbl +1 -0
@@ -453,3 +453,4 @@ 448 common process_mrelease sys_process_mrelease sys_process_mrelease 449 common futex_waitv sys_futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node sys_set_mempolicy_home_node +451 common fchmodat4 sys_fchmodat4 sys_fchmodat4
arch/sh/kernel/syscalls/syscall.tbl +1 -0
@@ -453,3 +453,4 @@ 448 common process_mrelease sys_process_mrelease 449 common futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node +451 common fchmodat4 sys_fchmodat4
arch/sparc/kernel/syscalls/syscall.tbl +1 -0
@@ -496,3 +496,4 @@ 448 common process_mrelease sys_process_mrelease 449 common futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node +451 common fchmodat4 sys_fchmodat4
arch/x86/entry/syscalls/syscall_32.tbl +1 -0
@@ -455,3 +455,4 @@ 448 i386 process_mrelease sys_process_mrelease 449 i386 futex_waitv sys_futex_waitv 450 i386 set_mempolicy_home_node sys_set_mempolicy_home_node +451 i386 fchmodat4 sys_fchmodat4
arch/x86/entry/syscalls/syscall_64.tbl +1 -0
@@ -372,6 +372,7 @@ 448 common process_mrelease sys_process_mrelease 449 common futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node +451 common fchmodat4 sys_fchmodat4 # # Due to a historical design error, certain syscalls are numbered differently
arch/xtensa/kernel/syscalls/syscall.tbl +1 -0
@@ -421,3 +421,4 @@ 448 common process_mrelease sys_process_mrelease 449 common futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node +451 common fchmodat4 sys_fchmodat4
include/uapi/asm-generic/unistd.h +4 -1
@@ -886,8 +886,11 @@ __SYSCALL(__NR_futex_waitv, sys_futex_waitv)#define __NR_set_mempolicy_home_node 450 __SYSCALL(__NR_set_mempolicy_home_node, sys_set_mempolicy_home_node) +#define __NR_fchmodat4 451 +__SYSCALL(__NR_fchmodat4, sys_fchmodat4) +#undef __NR_syscalls -#define __NR_syscalls 451 +#define __NR_syscalls 452/* * 32 bit systems traditionally used different
Signature · 1 line
2.33.8
Arnd Bergmann
maintainerRe: [PATCH v3 3/5] arch: Register fchmodat4, usually as syscall 451
Quoted 8 lines · Alexey Gladkov
From: Palmer Dabbelt <[email protected]>
This registers the new fchmodat4 syscall in most places as nuber 451, with alpha being the exception where it's 561. I found all these sites by grepping for fspick, which I assume has found me everything.
Signed-off-by Palmer Dabbelt <[email protected]>Signed-off-by Alexey Gladkov <[email protected]>
In linux-6.5-rc1, number 451 is used for __NR_cachestat, the next free one at the moment is 452.
Quoted 2 lines
arch/arm/tools/syscall.tbl | 1 + arch/arm64/include/asm/unistd32.h | 2 ++
Unfortunately, you still also need to change __NR_compat_syscalls in arch/arm64/include/asm/unistd.h. Aside from these two issues, your patch is the correct way to hook up a new syscall.
Arnd
PATCH v3 4/5 tools headers UAPI: Sync files changed by new fchmodat4 syscall
In reply to Alexey GladkovFrom: Palmer Dabbelt <[email protected]>
That add support for this new syscall in tools such as 'perf trace'.
--- tools/include/uapi/asm-generic/unistd.h | 5 ++++- tools/perf/arch/mips/entry/syscalls/syscall_n64.tbl | 1 + tools/perf/arch/powerpc/entry/syscalls/syscall.tbl | 1 + tools/perf/arch/s390/entry/syscalls/syscall.tbl | 1 + tools/perf/arch/x86/entry/syscalls/syscall_64.tbl | 1 + 5 files changed, 8 insertions(+), 1 deletion(-)
tools/include/uapi/asm-generic/unistd.h +4 -1
@@ -886,8 +886,11 @@ __SYSCALL(__NR_futex_waitv, sys_futex_waitv)#define __NR_set_mempolicy_home_node 450 __SYSCALL(__NR_set_mempolicy_home_node, sys_set_mempolicy_home_node) +#define __NR_fchmodat4 451 +__SYSCALL(__NR_fchmodat4, sys_fchmodat4) +#undef __NR_syscalls -#define __NR_syscalls 451 +#define __NR_syscalls 452/* * 32 bit systems traditionally used different
tools/perf/arch/mips/entry/syscalls/syscall_n64.tbl +1 -0
@@ -365,3 +365,4 @@ 448 n64 process_mrelease sys_process_mrelease 449 n64 futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node +451 n64 fchmodat4 sys_fchmodat4
tools/perf/arch/powerpc/entry/syscalls/syscall.tbl +1 -0
@@ -537,3 +537,4 @@ 448 common process_mrelease sys_process_mrelease 449 common futex_waitv sys_futex_waitv 450 nospu set_mempolicy_home_node sys_set_mempolicy_home_node +451 common fchmodat4 sys_fchmodat4
tools/perf/arch/s390/entry/syscalls/syscall.tbl +1 -0
@@ -453,3 +453,4 @@ 448 common process_mrelease sys_process_mrelease sys_process_mrelease 449 common futex_waitv sys_futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node sys_set_mempolicy_home_node +451 common fchmodat4 sys_fchmodat4 sys_fchmodat4
tools/perf/arch/x86/entry/syscalls/syscall_64.tbl +1 -0
@@ -372,6 +372,7 @@ 448 common process_mrelease sys_process_mrelease 449 common futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node +451 common fchmodat4 sys_fchmodat4 # # Due to a historical design error, certain syscalls are numbered differently
Signature · 1 line
2.33.8
PATCH v3 5/5 selftests: add fchmodat4(2) selftest
In reply to Alexey GladkovThe test marks as skipped if a syscall with the AT_SYMLINK_NOFOLLOW flag fails. This is because not all filesystems support changing the mode bits of symlinks properly. These filesystems return an error but change the mode bits:
newfstatat(4, "regfile", {st_mode=S_IFREG|0640, st_size=0, ...}, AT_SYMLINK_NOFOLLOW) = 0 newfstatat(4, "symlink", {st_mode=S_IFLNK|0777, st_size=7, ...}, AT_SYMLINK_NOFOLLOW) = 0 syscall_0x1c3(0x4, 0x55fa1f244396, 0x180, 0x100, 0x55fa1f24438e, 0x34) = -1 EOPNOTSUPP (Operation not supported) newfstatat(4, "regfile", {st_mode=S_IFREG|0640, st_size=0, ...}, AT_SYMLINK_NOFOLLOW) = 0
This happens with btrfs and xfs:
$ /kernel/tools/testing/selftests/fchmodat4/fchmodat4_test TAP version 13 1..1 ok 1 # SKIP fchmodat4(symlink) # Totals: pass:0 fail:0 xfail:0 xpass:0 skip:1 error:0
$ stat /tmp/ksft-fchmodat4.*/symlink File: /tmp/ksft-fchmodat4.3NCqlE/symlink -> regfile Size: 7 Blocks: 0 IO Block: 4096 symbolic link Device: 7,0 Inode: 133 Links: 1 Access: (0600/lrw-------) Uid: ( 0/ root) Gid: ( 0/ root)
--- tools/testing/selftests/Makefile | 1 + tools/testing/selftests/fchmodat4/.gitignore | 2 + tools/testing/selftests/fchmodat4/Makefile | 6 + .../selftests/fchmodat4/fchmodat4_test.c | 151 ++++++++++++++++++ 4 files changed, 160 insertions(+) create mode 100644 tools/testing/selftests/fchmodat4/.gitignore create mode 100644 tools/testing/selftests/fchmodat4/Makefile create mode 100644 tools/testing/selftests/fchmodat4/fchmodat4_test.c
tools/testing/selftests/Makefile +1 -0
@@ -17,6 +17,7 @@ TARGETS += drivers/net/bonding TARGETS += drivers/net/team TARGETS += efivarfs TARGETS += exec +TARGETS += fchmodat4 TARGETS += filesystems TARGETS += filesystems/binderfs TARGETS += filesystems/epoll
tools/testing/selftests/fchmodat4/.gitignore new file +2 -0
@@ -0,0 +1,2 @@ +# SPDX-License-Identifier: GPL-2.0-only +/*_test
tools/testing/selftests/fchmodat4/Makefile new file +6 -0
@@ -0,0 +1,6 @@ +# SPDX-License-Identifier: GPL-2.0-or-later + +CFLAGS += -Wall -O2 -g -fsanitize=address -fsanitize=undefined +TEST_GEN_PROGS := fchmodat4_test + +include ../lib.mk
tools/testing/selftests/fchmodat4/fchmodat4_test.c new file +151 -0
151 lines not on this page · raw message
Signature · 1 line
2.33.8
Re: [PATCH v3 5/5] selftests: add fchmodat4(2) selftest
* Alexey Gladkov:
Quoted 18 lines
The test marks as skipped if a syscall with the
AT_SYMLINK_NOFOLLOWflag fails. This is because not all filesystems support changing the mode bits of symlinks properly. These filesystems return an error but change the mode bits:
newfstatat(4, "regfile", {st_mode=S_IFREG|0640, st_size=0, ...}, AT_SYMLINK_NOFOLLOW)= 0newfstatat(4, "symlink", {st_mode=S_IFLNK|0777, st_size=7, ...}, AT_SYMLINK_NOFOLLOW)= 0syscall_0x1c3(0x4, 0x55fa1f244396, 0x180, 0x100, 0x55fa1f24438e, 0x34)= -1 EOPNOTSUPP (Operation not supported)newfstatat(4, "regfile", {st_mode=S_IFREG|0640, st_size=0, ...}, AT_SYMLINK_NOFOLLOW)= 0This happens with btrfs and xfs:
$ /kernel/tools/testing/selftests/fchmodat4/fchmodat4_test TAP version 13 1..1 ok 1 # SKIP fchmodat4(symlink) # Totals: pass:0 fail:0 xfail:0 xpass:0 skip:1 error:0
Quoted 7 lines
$ stat /tmp/ksft-fchmodat4.*/symlink File: /tmp/ksft-fchmodat4.3NCqlE/symlink -> regfile Size: 7 Blocks: 0 IO Block: 4096 symbolic link Device: 7,0 Inode: 133 Links: 1 Access: (0600/lrw-------) Uid: ( 0/ root) Gid: ( 0/ root)Signed-off-by Alexey Gladkov <[email protected]>
This looks like a bug in those file systems?
As an extra test, “echo 3 > /proc/sys/vm/drop_caches” sometimes has strange effects in such cases because the bits are not actually stored on disk, only in the dentry cache.
Thanks,
Florian
Re: [PATCH v3 5/5] selftests: add fchmodat4(2) selftest
Quoted 26 lines · Florian Weimer
* Alexey Gladkov:
Quoted 24 lines
The test marks as skipped if a syscall with the
AT_SYMLINK_NOFOLLOWflag fails. This is because not all filesystems support changing the mode bits of symlinks properly. These filesystems return an error but change the mode bits:
newfstatat(4, "regfile", {st_mode=S_IFREG|0640, st_size=0, ...}, AT_SYMLINK_NOFOLLOW)= 0newfstatat(4, "symlink", {st_mode=S_IFLNK|0777, st_size=7, ...}, AT_SYMLINK_NOFOLLOW)= 0syscall_0x1c3(0x4, 0x55fa1f244396, 0x180, 0x100, 0x55fa1f24438e, 0x34)= -1 EOPNOTSUPP (Operation not supported)newfstatat(4, "regfile", {st_mode=S_IFREG|0640, st_size=0, ...}, AT_SYMLINK_NOFOLLOW)= 0This happens with btrfs and xfs:
$ /kernel/tools/testing/selftests/fchmodat4/fchmodat4_test TAP version 13 1..1 ok 1 # SKIP fchmodat4(symlink) # Totals: pass:0 fail:0 xfail:0 xpass:0 skip:1 error:0$ stat /tmp/ksft-fchmodat4.*/symlink File: /tmp/ksft-fchmodat4.3NCqlE/symlink -> regfile Size: 7 Blocks: 0 IO Block: 4096 symbolic link Device: 7,0 Inode: 133 Links: 1 Access: (0600/lrw-------) Uid: ( 0/ root) Gid: ( 0/ root)
Quoted 3 lines
Quoted 1 line
Signed-off-by Alexey Gladkov <[email protected]>This looks like a bug in those file systems?
To me this looks like a bug. I'm fine if the operation ends with EOPNOTSUPP, but in that case the mode bits shouldn't change.
Quoted 3 lines
As an extra test, “echo 3 > /proc/sys/vm/drop_caches” sometimes has strange effects in such cases because the bits are not actually stored on disk, only in the dentry cache.
tmpfssyscall_0x1c3(0xffffff9c, 0x7ffd58758574, 0, 0x100, 0x7f6cf18adc70, 0x7ffd58756ad8) = 0 +++ exited with 0 +++
l--------- 1 root root 1 Jul 11 16:36 /tmp/dir/link -> f
=== dropping caches ===
l--------- 1 root root 1 Jul 11 16:36 /tmp/dir/link -> f
ext4syscall_0x1c3(0xffffff9c, 0x7ffedfdb4574, 0, 0x100, 0x7f7f40b45c70, 0x7ffedfdb3ae8) = -1 EOPNOTSUPP (Operation not supported) +++ exited with 1 +++
l--------- 1 root root 1 Jul 11 16:36 /tmp/dir/link -> f
=== dropping caches ===
l--------- 1 root root 1 Jul 11 16:36 /tmp/dir/link -> f
xfssyscall_0x1c3(0xffffff9c, 0x7ffcd03ce574, 0, 0x100, 0x7ff2f2980c70, 0x7ffcd03cdd38) = -1 EOPNOTSUPP (Operation not supported) +++ exited with 1 +++
l--------- 1 root root 1 Jul 11 16:36 /tmp/dir/link -> f
=== dropping caches ===
l--------- 1 root root 1 Jul 11 16:36 /tmp/dir/link -> f
btrfssyscall_0x1c3(0xffffff9c, 0x7fff13d2e574, 0, 0x100, 0x7f9b67f59c70, 0x7fff13d2ca88) = -1 EOPNOTSUPP (Operation not supported) +++ exited with 1 +++
l--------- 1 root root 1 Jul 11 16:36 /tmp/dir/link -> f
=== dropping caches ===
l--------- 1 root root 1 Jul 11 16:36 /tmp/dir/link -> f
reiserfssyscall_0x1c3(0xffffff9c, 0x7ffdf75af574, 0, 0x100, 0x7f7ad0634c70, 0x7ffdf75ae478) = 0 +++ exited with 0 +++
l--------- 1 root root 1 Jul 11 16:43 /tmp/dir/link -> f
=== dropping caches ===
l--------- 1 root root 1 Jul 11 16:43 /tmp/dir/link -> f
Signature · 1 line
Rgrds, legion
Re: [PATCH v3 0/5] Add a new fchmodat4() syscall
In reply to Alexey Gladkov* Alexey Gladkov:
Quoted 21 lines
This patch set adds
fchmodat4(), a new syscall. The actual implementation is super simple: essentially it's just the same asfchmodat(), butLOOKUP_FOLLOWis conditionally set based on the flags. I've attempted to make this match "man 2 fchmodat" as closely as possible, which says EINVAL is returned for invalid flags (as opposed to ENOTSUPP, which is currently returned by glibc forAT_SYMLINK_NOFOLLOW). I have a sketch of a glibc patch that I haven't even compiled yet, but seems fairly straight-forward:diff --git
a/sysdeps/unix/sysv/linux/fchmodat.cb/sysdeps/unix/sysv/linux/fchmodat.cindex6d9cbc1ce9e0..b1beab76d56c100644
---a/sysdeps/unix/sysv/linux/fchmodat.c
+++b/sysdeps/unix/sysv/linux/fchmodat.c
@@ -29,12 +29,36 @@
int
fchmodat (int fd, const char *file,mode_tmode, int flag)
{
- if (flag & ~AT_SYMLINK_NOFOLLOW)
- returnINLINE_SYSCALL_ERROR_RETURN_VALUE(EINVAL);
-#ifndef__NR_lchmod/* Linux so far has no lchmod syscall. */
+ /* There are four paths through this code:
Quoted 8 lines
+ - The flags are zero. In this case it's fine to call fchmodat.
+ - The flags are non-zero and glibc doesn't have access to
+__NR_fchmodat4. In this case all we can do is emulate the error codes
+ defined by the glibc interface from userspace.
+ - The flags are non-zero, glibc has__NR_fchmodat4, and the kernel has
+ fchmodat4. This is the simplest case, as the fchmodat4 syscall exactly
+ matches glibc's library interface so it can be called directly.
+ - The flags are non-zero, glibc has__NR_fchmodat4, but the kernel does
If you define __NR_fchmodat4 on all architectures, we can use these constants directly in glibc. We no longer depend on the UAPI definitions of those constants, to cut down the number of code variants, and to make glibc's system call profile independent of the kernel header version at build time.
Your version is based on 2.31, more recent versions have some reasonable emulation for fchmodat based on /proc/self/fd. I even wrote a comment describing the same buggy behavior that you witnessed:
+ /* Some Linux versions with some file systems can actually
+ change symbolic link permissions via /proc, but this is not
+ intentional, and it gives inconsistent results (e.g., error
+ return despite mode change). The expected behavior is that
+ symbolic link modes cannot be changed at all, and this check
+ enforces that. */
+ if (S_ISLNK (st.st_mode))
+ {
+ __close_nocancel (pathfd);
+ __set_errno (EOPNOTSUPP);
+ return -1;+ }
I think there was some kernel discussion about that behavior before, but apparently, it hasn't led to fixes.
I wonder if it makes sense to add a similar error return to the system call implementation?
Quoted 10 lines
+ not. In this case we must respect the error codes defined by the glibc
+ interface instead of returning ENOSYS.
+ The intent here is to ensure that the kernel is called at most once per
+ library call, and that the error types defined by glibc are always
+ respected. */
+
+#ifdef__NR_fchmodat4
+ long result;
+#endif
+
Quoted 8 lines
+ if (flag == 0)
+ returnINLINE_SYSCALL(fchmodat, 3, fd, file, mode);
+
+#ifdef__NR_fchmodat4
+ result =INLINE_SYSCALL(fchmodat4, 4, fd, file, mode, flag);
+ if (result == 0 || errno != ENOSYS)
+ return result;
+#endif
The last if condition is the recommended approach, but in the past, it broke container host compatibility pretty badly due to seccomp filters that return EPERM instead of ENOSYS. I guess we'll learn soon enough if that's been fixed by now. 8-P
Thanks,
Florian
Christian Brauner
maintainerRe: [PATCH v3 0/5] Add a new fchmodat4() syscall
Quoted 55 lines · Florian Weimer
* Alexey Gladkov:
Quoted 21 lines
This patch set adds
fchmodat4(), a new syscall. The actual implementation is super simple: essentially it's just the same asfchmodat(), butLOOKUP_FOLLOWis conditionally set based on the flags. I've attempted to make this match "man 2 fchmodat" as closely as possible, which says EINVAL is returned for invalid flags (as opposed to ENOTSUPP, which is currently returned by glibc forAT_SYMLINK_NOFOLLOW). I have a sketch of a glibc patch that I haven't even compiled yet, but seems fairly straight-forward:diff --git
a/sysdeps/unix/sysv/linux/fchmodat.cb/sysdeps/unix/sysv/linux/fchmodat.cindex6d9cbc1ce9e0..b1beab76d56c100644
---a/sysdeps/unix/sysv/linux/fchmodat.c
+++b/sysdeps/unix/sysv/linux/fchmodat.c
@@ -29,12 +29,36 @@
int
fchmodat (int fd, const char *file,mode_tmode, int flag)
{
- if (flag & ~AT_SYMLINK_NOFOLLOW)
- returnINLINE_SYSCALL_ERROR_RETURN_VALUE(EINVAL);
-#ifndef__NR_lchmod/* Linux so far has no lchmod syscall. */
+ /* There are four paths through this code:Quoted 8 lines
+ - The flags are zero. In this case it's fine to call fchmodat.
+ - The flags are non-zero and glibc doesn't have access to
+__NR_fchmodat4. In this case all we can do is emulate the error codes
+ defined by the glibc interface from userspace.
+ - The flags are non-zero, glibc has__NR_fchmodat4, and the kernel has
+ fchmodat4. This is the simplest case, as the fchmodat4 syscall exactly
+ matches glibc's library interface so it can be called directly.
+ - The flags are non-zero, glibc has__NR_fchmodat4, but the kernel doesIf you define
__NR_fchmodat4on all architectures, we can use these constants directly in glibc. We no longer depend on the UAPI definitions of those constants, to cut down the number of code variants, and to make glibc's system call profile independent of the kernel header version at build time.Your version is based on 2.31, more recent versions have some reasonable emulation for fchmodat based on
/proc/self/fd. I even wrote a comment describing the same buggy behavior that you witnessed:(quoted diff) +12
-0+ /* Some Linux versions with some file systems can actually + change symbolic link permissions via /proc, but this is not + intentional, and it gives inconsistent results (e.g., error + return despite mode change). The expected behavior is that + symbolic link modes cannot be changed at all, and this check + enforces that. */ + if (S_ISLNK (st.st_mode)) + { + __close_nocancel (pathfd); + __set_errno (EOPNOTSUPP); + return -1; + }
Quoted 2 lines
I think there was some kernel discussion about that behavior before, but apparently, it hasn't led to fixes.
I think I've explained this somewhere else a couple of months ago but just in case you weren't on that thread or don't remember and apologies if you should already know.
A lot of filesystem will happily update the mode of a symlink. The VFS doesn't do anything to prevent this from happening. This is filesystem specific.
The EOPNOTSUPP you're seeing very likely comes from POSIX ACLs. Specifically it comes from filesystems that call posix_acl_chmod(), e.g., btrfs via
if (!err && attr->ia_valid & ATTR_MODE)
err = posix_acl_chmod(idmap, dentry, inode->i_mode);Most filesystems don't implement i_op->set_acl() for POSIX ACLs. So posix_acl_chmod() will report EOPNOTSUPP. By the time posix_acl_chmod() is called, most filesystems will have finished updating the inode. POSIX ACLs also often aren't integrated into transactions so a rollback wouldn't even be possible on some filesystems.
Any filesystem that doesn't implement POSIX ACLs at all will obviously never fail unless it blocks mode changes on symlinks. Or filesystems that do have a way to rollback failures from posix_acl_chmod(), or filesystems that do return an error on chmod() on symlinks such as 9p, ntfs, ocfs2.
Quoted 3 lines
I wonder if it makes sense to add a similar error return to the system call implementation?
Hm, blocking symlink mode changes is pretty regression prone. And just blocking it through one interface seems weird and makes things even more inconsistent.
So two options I see:
(1) minimally invasive:
Filesystems that do call posix_acl_chmod() on symlinks need to be changed to stop doing that.
(2) might hit us on the head invasive:
Try and block symlink mode changes in chmod_common().
Thoughts?
Re: [PATCH v3 0/5] Add a new fchmodat4() syscall
Quoted 95 lines · Christian Brauner
Quoted 55 lines · Florian Weimer
* Alexey Gladkov:
Quoted 21 lines
This patch set adds
fchmodat4(), a new syscall. The actual implementation is super simple: essentially it's just the same asfchmodat(), butLOOKUP_FOLLOWis conditionally set based on the flags. I've attempted to make this match "man 2 fchmodat" as closely as possible, which says EINVAL is returned for invalid flags (as opposed to ENOTSUPP, which is currently returned by glibc forAT_SYMLINK_NOFOLLOW). I have a sketch of a glibc patch that I haven't even compiled yet, but seems fairly straight-forward:diff --git
a/sysdeps/unix/sysv/linux/fchmodat.cb/sysdeps/unix/sysv/linux/fchmodat.cindex6d9cbc1ce9e0..b1beab76d56c100644
---a/sysdeps/unix/sysv/linux/fchmodat.c
+++b/sysdeps/unix/sysv/linux/fchmodat.c
@@ -29,12 +29,36 @@
int
fchmodat (int fd, const char *file,mode_tmode, int flag)
{
- if (flag & ~AT_SYMLINK_NOFOLLOW)
- returnINLINE_SYSCALL_ERROR_RETURN_VALUE(EINVAL);
-#ifndef__NR_lchmod/* Linux so far has no lchmod syscall. */
+ /* There are four paths through this code:Quoted 8 lines
+ - The flags are zero. In this case it's fine to call fchmodat.
+ - The flags are non-zero and glibc doesn't have access to
+__NR_fchmodat4. In this case all we can do is emulate the error codes
+ defined by the glibc interface from userspace.
+ - The flags are non-zero, glibc has__NR_fchmodat4, and the kernel has
+ fchmodat4. This is the simplest case, as the fchmodat4 syscall exactly
+ matches glibc's library interface so it can be called directly.
+ - The flags are non-zero, glibc has__NR_fchmodat4, but the kernel doesIf you define
__NR_fchmodat4on all architectures, we can use these constants directly in glibc. We no longer depend on the UAPI definitions of those constants, to cut down the number of code variants, and to make glibc's system call profile independent of the kernel header version at build time.Your version is based on 2.31, more recent versions have some reasonable emulation for fchmodat based on
/proc/self/fd. I even wrote a comment describing the same buggy behavior that you witnessed:(quoted diff) +12
-0+ /* Some Linux versions with some file systems can actually + change symbolic link permissions via /proc, but this is not + intentional, and it gives inconsistent results (e.g., error + return despite mode change). The expected behavior is that + symbolic link modes cannot be changed at all, and this check + enforces that. */ + if (S_ISLNK (st.st_mode)) + { + __close_nocancel (pathfd); + __set_errno (EOPNOTSUPP); + return -1; + }Quoted 2 lines
I think there was some kernel discussion about that behavior before, but apparently, it hasn't led to fixes.
I think I've explained this somewhere else a couple of months ago but just in case you weren't on that thread or don't remember and apologies if you should already know.
A lot of filesystem will happily update the mode of a symlink. The VFS doesn't do anything to prevent this from happening. This is filesystem specific.
The EOPNOTSUPP you're seeing very likely comes from POSIX ACLs. Specifically it comes from filesystems that call
posix_acl_chmod(), e.g., btrfs viaif (!err && attr->ia_valid & ATTR_MODE) err = posix_acl_chmod(idmap, dentry, inode->i_mode);Most filesystems don't implement
i_op->set_acl()for POSIX ACLs. Soposix_acl_chmod()will report EOPNOTSUPP. By the timeposix_acl_chmod()is called, most filesystems will have finished updating the inode. POSIX ACLs also often aren't integrated into transactions so a rollback wouldn't even be possible on some filesystems.Any filesystem that doesn't implement POSIX ACLs at all will obviously never fail unless it blocks mode changes on symlinks. Or filesystems that do have a way to rollback failures from
posix_acl_chmod(), or filesystems that do return an error onchmod()on symlinks such as 9p, ntfs, ocfs2.Quoted 3 lines
I wonder if it makes sense to add a similar error return to the system call implementation?
Hm, blocking symlink mode changes is pretty regression prone. And just blocking it through one interface seems weird and makes things even more inconsistent.
Quoted 9 lines
So two options I see:
(1) minimally invasive:
Filesystems that do callposix_acl_chmod()on symlinks need to be changed to stop doing that.
(2) might hit us on the head invasive:
Try and block symlink mode changes inchmod_common().Thoughts?
We have third option. We can choose not to call chmod_common and return an error right away:
fs/open.c +3 -1
@@ -679,7 +679,9 @@ static int do_fchmodat(int dfd, const char __user *filename, umode_t mode, int l retry: error =user_path_at (dfd, filename, lookup_flags, &path);if (!error) { - error =chmod_common (&path, mode); + error = -EOPNOTSUPP ; +if (!(flags &AT_SYMLINK_NOFOLLOW ) || !S_ISLNK (path.dentry->d_inode->i_mode)) + error =chmod_common (&path, mode);path_put (&path);if (retry_estale (error, lookup_flags)) { lookup_flags |=LOOKUP_REVAL ;
It doesn't seem to be invasive.
Signature · 1 line
Rgrds, legion
Christian Brauner
maintainerRe: [PATCH v3 0/5] Add a new fchmodat4() syscall
Quoted 125 lines · Alexey Gladkov
Quoted 95 lines · Christian Brauner
Quoted 55 lines · Florian Weimer
* Alexey Gladkov:
Quoted 21 lines
This patch set adds
fchmodat4(), a new syscall. The actual implementation is super simple: essentially it's just the same asfchmodat(), butLOOKUP_FOLLOWis conditionally set based on the flags. I've attempted to make this match "man 2 fchmodat" as closely as possible, which says EINVAL is returned for invalid flags (as opposed to ENOTSUPP, which is currently returned by glibc forAT_SYMLINK_NOFOLLOW). I have a sketch of a glibc patch that I haven't even compiled yet, but seems fairly straight-forward:diff --git
a/sysdeps/unix/sysv/linux/fchmodat.cb/sysdeps/unix/sysv/linux/fchmodat.cindex6d9cbc1ce9e0..b1beab76d56c100644
---a/sysdeps/unix/sysv/linux/fchmodat.c
+++b/sysdeps/unix/sysv/linux/fchmodat.c
@@ -29,12 +29,36 @@
int
fchmodat (int fd, const char *file,mode_tmode, int flag)
{
- if (flag & ~AT_SYMLINK_NOFOLLOW)
- returnINLINE_SYSCALL_ERROR_RETURN_VALUE(EINVAL);
-#ifndef__NR_lchmod/* Linux so far has no lchmod syscall. */
+ /* There are four paths through this code:Quoted 8 lines
+ - The flags are zero. In this case it's fine to call fchmodat.
+ - The flags are non-zero and glibc doesn't have access to
+__NR_fchmodat4. In this case all we can do is emulate the error codes
+ defined by the glibc interface from userspace.
+ - The flags are non-zero, glibc has__NR_fchmodat4, and the kernel has
+ fchmodat4. This is the simplest case, as the fchmodat4 syscall exactly
+ matches glibc's library interface so it can be called directly.
+ - The flags are non-zero, glibc has__NR_fchmodat4, but the kernel doesIf you define
__NR_fchmodat4on all architectures, we can use these constants directly in glibc. We no longer depend on the UAPI definitions of those constants, to cut down the number of code variants, and to make glibc's system call profile independent of the kernel header version at build time.Your version is based on 2.31, more recent versions have some reasonable emulation for fchmodat based on
/proc/self/fd. I even wrote a comment describing the same buggy behavior that you witnessed:(quoted diff) +12
-0+ /* Some Linux versions with some file systems can actually + change symbolic link permissions via /proc, but this is not + intentional, and it gives inconsistent results (e.g., error + return despite mode change). The expected behavior is that + symbolic link modes cannot be changed at all, and this check + enforces that. */ + if (S_ISLNK (st.st_mode)) + { + __close_nocancel (pathfd); + __set_errno (EOPNOTSUPP); + return -1; + }Quoted 2 lines
I think there was some kernel discussion about that behavior before, but apparently, it hasn't led to fixes.
I think I've explained this somewhere else a couple of months ago but just in case you weren't on that thread or don't remember and apologies if you should already know.
A lot of filesystem will happily update the mode of a symlink. The VFS doesn't do anything to prevent this from happening. This is filesystem specific.
The EOPNOTSUPP you're seeing very likely comes from POSIX ACLs. Specifically it comes from filesystems that call
posix_acl_chmod(), e.g., btrfs viaif (!err && attr->ia_valid & ATTR_MODE) err = posix_acl_chmod(idmap, dentry, inode->i_mode);Most filesystems don't implement
i_op->set_acl()for POSIX ACLs. Soposix_acl_chmod()will report EOPNOTSUPP. By the timeposix_acl_chmod()is called, most filesystems will have finished updating the inode. POSIX ACLs also often aren't integrated into transactions so a rollback wouldn't even be possible on some filesystems.Any filesystem that doesn't implement POSIX ACLs at all will obviously never fail unless it blocks mode changes on symlinks. Or filesystems that do have a way to rollback failures from
posix_acl_chmod(), or filesystems that do return an error onchmod()on symlinks such as 9p, ntfs, ocfs2.Quoted 3 lines
I wonder if it makes sense to add a similar error return to the system call implementation?
Hm, blocking symlink mode changes is pretty regression prone. And just blocking it through one interface seems weird and makes things even more inconsistent.
Quoted 9 lines
So two options I see:
(1) minimally invasive:
Filesystems that do callposix_acl_chmod()on symlinks need to be changed to stop doing that.
(2) might hit us on the head invasive:
Try and block symlink mode changes inchmod_common().Thoughts?
We have third option. We can choose not to call
chmod_commonand return an error right away:fs/open.c +3
-1@@ -679,7 +679,9 @@ static int do_fchmodat(int dfd, const char __user *filename, umode_t mode, int l retry: error =user_path_at (dfd, filename, lookup_flags, &path);if (!error) { - error =chmod_common (&path, mode); + error = -EOPNOTSUPP ; +if (!(flags &AT_SYMLINK_NOFOLLOW ) || !S_ISLNK (path.dentry->d_inode->i_mode)) + error =chmod_common (&path, mode);path_put (&path);if (retry_estale (error, lookup_flags)) { lookup_flags |=LOOKUP_REVAL ;
Quoted 1 line
It doesn't seem to be invasive.
v4 cover Add a new fchmodat2() syscall
In reply to Alexey GladkovIn glibc, the fchmodat(3) function has a flags argument according to the POSIX specification [1], but kernel syscalls has no such argument. Therefore, libc implementations do workarounds using /proc. However, this requires procfs to be mounted and accessible.
This patch set adds fchmodat2(), a new syscall. The syscall allows to pass the AT_SYMLINK_NOFOLLOW flag to disable LOOKUP_FOLLOW. In all other respects, this syscall is no different from fchmodat().
[1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/chmod.html
Changes since v3 [[email protected]]:
* Rebased to master because a new syscall has appeared in master.
* Increased __NR_compat_syscalls as pointed out by Arnd Bergmann.
* Syscall renamed fchmodat4 -> fchmodat2 as suggested by Christian Brauner.
* Returned do_fchmodat4() the original name. We don't need to version internal functions.
* Fixed warnings found by checkpatch.pl.
Changes since v2 [[email protected]]:
* Rebased to master.
* The lookup_flags passed to sys_fchmodat4 as suggested by Al Viro.
* Selftest added.
Changes since v1 [[email protected]]:
* All architectures are now supported, which support squashed into a single patch.
* The do_fchmodat() helper function has been removed, in favor of directly calling do_fchmodat4().
* The patches are based on 5.2 instead of 5.1.
---
Alexey Gladkov (2):
fs: Add fchmodat2()
selftests: Add fchmodat2 selftest
Palmer Dabbelt (3):
Non-functional cleanup of a "__user * filename"
arch: Register fchmodat2, usually as syscall 452 tools headers UAPI: Sync files changed by new fchmodat2 syscall
arch/alpha/kernel/syscalls/syscall.tbl | 1 + arch/arm/tools/syscall.tbl | 1 + arch/arm64/include/asm/unistd.h | 2 +- arch/arm64/include/asm/unistd32.h | 2 + arch/ia64/kernel/syscalls/syscall.tbl | 1 + arch/m68k/kernel/syscalls/syscall.tbl | 1 + arch/microblaze/kernel/syscalls/syscall.tbl | 1 + arch/mips/kernel/syscalls/syscall_n32.tbl | 1 + arch/mips/kernel/syscalls/syscall_n64.tbl | 1 + arch/mips/kernel/syscalls/syscall_o32.tbl | 1 + arch/parisc/kernel/syscalls/syscall.tbl | 1 + arch/powerpc/kernel/syscalls/syscall.tbl | 1 + arch/s390/kernel/syscalls/syscall.tbl | 1 + arch/sh/kernel/syscalls/syscall.tbl | 1 + arch/sparc/kernel/syscalls/syscall.tbl | 1 + arch/x86/entry/syscalls/syscall_32.tbl | 1 + arch/x86/entry/syscalls/syscall_64.tbl | 1 + arch/xtensa/kernel/syscalls/syscall.tbl | 1 + fs/open.c | 18 +- include/linux/syscalls.h | 4 +- include/uapi/asm-generic/unistd.h | 5 +- tools/include/uapi/asm-generic/unistd.h | 5 +- .../arch/mips/entry/syscalls/syscall_n64.tbl | 2 + .../arch/powerpc/entry/syscalls/syscall.tbl | 2 + .../perf/arch/s390/entry/syscalls/syscall.tbl | 2 + .../arch/x86/entry/syscalls/syscall_64.tbl | 2 + tools/testing/selftests/Makefile | 1 + tools/testing/selftests/fchmodat2/.gitignore | 2 + tools/testing/selftests/fchmodat2/Makefile | 6 + .../selftests/fchmodat2/fchmodat2_test.c | 162 ++++++++++++++++++ 30 files changed, 223 insertions(+), 8 deletions(-) create mode 100644 tools/testing/selftests/fchmodat2/.gitignore create mode 100644 tools/testing/selftests/fchmodat2/Makefile create mode 100644 tools/testing/selftests/fchmodat2/fchmodat2_test.c
Signature · 1 line
2.33.8
PATCH v4 1/5 Non-functional cleanup of a "__user * filename"
From: Palmer Dabbelt <[email protected]>
The next patch defines a very similar interface, which I copied from this definition. Since I'm touching it anyway I don't see any reason not to just go fix this one up.
--- include/linux/syscalls.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/syscalls.h +1 -1
@@ -438,7 +438,7 @@ asmlinkage long sys_chdir(const char __user *filename);asmlinkage long sys_fchdir (unsigned int fd);asmlinkage long sys_chroot (const char __user *filename);asmlinkage long sys_fchmod (unsigned int fd,umode_t mode); -asmlinkage long sys_fchmodat (int dfd,const char __user * filename, +asmlinkage long sys_fchmodat (int dfd,const char __user *filename,umode_t mode);asmlinkage long sys_fchownat (int dfd,const char __user *filename,uid_t user,gid_t group,int flag);
Signature · 1 line
2.33.8
PATCH v4 2/5 fs: Add fchmodat2()
In reply to Alexey GladkovOn the userspace side fchmodat(3) is implemented as a wrapper function which implements the POSIX-specified interface. This interface differs from the underlying kernel system call, which does not have a flags argument. Most implementations require procfs [1][2].
There doesn't appear to be a good userspace workaround for this issue but the implementation in the kernel is pretty straight-forward.
The new fchmodat2() syscall allows to pass the AT_SYMLINK_NOFOLLOW flag, unlike existing fchmodat.
[1] https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/fchmodat.c;h=17eca54051ee28ba1ec3f9aed170a62630959143;hb=a492b1e5ef7ab50c6fdd4e4e9879ea5569ab0a6c#l35
[2] https://git.musl-libc.org/cgit/musl/tree/src/stat/fchmodat.c?id=718f363bc2067b6487900eddc9180c84e7739f80#n28
--- fs/open.c | 18 ++++++++++++++---- include/linux/syscalls.h | 2 ++ 2 files changed, 16 insertions(+), 4 deletions(-)
fs/open.c +14 -4
@@ -671,11 +671,11 @@ SYSCALL_DEFINE2(fchmod, unsigned int, fd, umode_t, mode)return err; } -static int do_fchmodat (int dfd,const char __user *filename,umode_t mode) +static int do_fchmodat (int dfd,const char __user *filename,umode_t mode,int lookup_flags) {struct path path;int error; -unsigned int lookup_flags =LOOKUP_FOLLOW ; + retry: error =user_path_at (dfd, filename, lookup_flags, &path);if (!error) {
@@ -689,15 +689,25 @@ static int do_fchmodat(int dfd, const char __user *filename, umode_t mode)return error; } +SYSCALL_DEFINE4(fchmodat2, int, dfd, const char __user *, filename, + umode_t, mode, int, flags) +{ +if (unlikely (flags & ~AT_SYMLINK_NOFOLLOW )) +return -EINVAL ; + +return do_fchmodat (dfd, filename, mode, + flags &AT_SYMLINK_NOFOLLOW ?0 :LOOKUP_FOLLOW ); +} + SYSCALL_DEFINE3(fchmodat, int, dfd, const char __user *, filename, umode_t, mode) { -return do_fchmodat (dfd, filename, mode); +return do_fchmodat (dfd, filename, mode,LOOKUP_FOLLOW ); } SYSCALL_DEFINE2(chmod, const char __user *, filename, umode_t, mode) { -return do_fchmodat (AT_FDCWD , filename, mode); +return do_fchmodat (AT_FDCWD , filename, mode,LOOKUP_FOLLOW ); }/*
include/linux/syscalls.h +2 -0
@@ -440,6 +440,8 @@ asmlinkage long sys_chroot(const char __user *filename);asmlinkage long sys_fchmod (unsigned int fd,umode_t mode);asmlinkage long sys_fchmodat (int dfd,const char __user *filename,umode_t mode); +asmlinkage long sys_fchmodat2 (int dfd,const char __user *filename, +umode_t mode,int flags);asmlinkage long sys_fchownat (int dfd,const char __user *filename,uid_t user,gid_t group,int flag);asmlinkage long sys_fchown (unsigned int fd,uid_t user,gid_t group);
Signature · 1 line
2.33.8
Christian Brauner
maintainerRe: [PATCH v4 2/5] fs: Add fchmodat2()
Quoted 28 lines · Alexey Gladkov
On the userspace side fchmodat(3) is implemented as a wrapper function which implements the POSIX-specified interface. This interface differs from the underlying kernel system call, which does not have a flags argument. Most implementations require procfs [1][2].
There doesn't appear to be a good userspace workaround for this issue but the implementation in the kernel is pretty straight-forward.
The new
fchmodat2()syscall allows to pass theAT_SYMLINK_NOFOLLOWflag, unlike existing fchmodat.[1] https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/fchmodat.c;h=17eca54051ee28ba1ec3f9aed170a62630959143;hb=a492b1e5ef7ab50c6fdd4e4e9879ea5569ab0a6c#l35
[2] https://git.musl-libc.org/cgit/musl/tree/src/stat/fchmodat.c?id=718f363bc2067b6487900eddc9180c84e7739f80#n28Co-developed-by Palmer Dabbelt <[email protected]>Signed-off-by Palmer Dabbelt <[email protected]>Signed-off-by Alexey Gladkov <[email protected]>Acked-by Arnd Bergmann <[email protected]>--- fs/open.c | 18 ++++++++++++++---- include/linux/syscalls.h | 2 ++ 2 files changed, 16 insertions(+), 4 deletions(-)fs/open.c +0
-0
fs/open.c SYSCALL_DEFINE2(fchmod, unsigned int, fd, umode_t, mode) · line 674
Quoted 5 lines
fs/open.c +1
-1@@ -671,11 +671,11 @@ SYSCALL_DEFINE2(fchmod, unsigned int, fd, umode_t, mode)return err; } -static int do_fchmodat (int dfd,const char __user *filename,umode_t mode) +static int do_fchmodat (int dfd,const char __user *filename,umode_t mode,int lookup_flags)
Should all be unsigned instead of int here for flags. We also had a documentation update to that effect but smh never sent it. user_path_at() itself takes an unsigned as well.
I'll fix that up though.
Re: [PATCH v4 2/5] fs: Add fchmodat2()
In reply to Alexey GladkovQuoted 28 lines · Alexey Gladkov
On the userspace side fchmodat(3) is implemented as a wrapper function which implements the POSIX-specified interface. This interface differs from the underlying kernel system call, which does not have a flags argument. Most implementations require procfs [1][2].
There doesn't appear to be a good userspace workaround for this issue but the implementation in the kernel is pretty straight-forward.
The new
fchmodat2()syscall allows to pass theAT_SYMLINK_NOFOLLOWflag, unlike existing fchmodat.[1] https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/fchmodat.c;h=17eca54051ee28ba1ec3f9aed170a62630959143;hb=a492b1e5ef7ab50c6fdd4e4e9879ea5569ab0a6c#l35
[2] https://git.musl-libc.org/cgit/musl/tree/src/stat/fchmodat.c?id=718f363bc2067b6487900eddc9180c84e7739f80#n28Co-developed-by Palmer Dabbelt <[email protected]>Signed-off-by Palmer Dabbelt <[email protected]>Signed-off-by Alexey Gladkov <[email protected]>Acked-by Arnd Bergmann <[email protected]>--- fs/open.c | 18 ++++++++++++++---- include/linux/syscalls.h | 2 ++ 2 files changed, 16 insertions(+), 4 deletions(-)fs/open.c +0
-0
fs/open.c SYSCALL_DEFINE2(fchmod, unsigned int, fd, umode_t, mode) · line 674
Quoted 5 lines
fs/open.c +1
-1@@ -671,11 +671,11 @@ SYSCALL_DEFINE2(fchmod, unsigned int, fd, umode_t, mode)return err; } -static int do_fchmodat (int dfd,const char __user *filename,umode_t mode) +static int do_fchmodat (int dfd,const char __user *filename,umode_t mode,int lookup_flags)
I think it'd be much neater to do the conversion of AT_ flags here and pass 0 as a flags argument for all of the wrappers (this is how most of the other xyz(), fxyz(), fxyzat() syscall wrappers are done IIRC).
Quoted 9 lines
fs/open.c +1
-1{struct path path;int error; -unsigned int lookup_flags =LOOKUP_FOLLOW ; + retry: error =user_path_at (dfd, filename, lookup_flags, &path);if (!error) {
fs/open.c static int do_fchmodat(int dfd, const char __user *filename, umode_t mode) · line 696
Quoted 8 lines
fs/open.c +5
-0@@ -689,15 +689,25 @@ static int do_fchmodat(int dfd, const char __user *filename, umode_t mode)return error; } +SYSCALL_DEFINE4(fchmodat2, int, dfd, const char __user *, filename, + umode_t, mode, int, flags) +{ +if (unlikely (flags & ~AT_SYMLINK_NOFOLLOW )) +return -EINVAL ;
We almost certainly want to support AT_EMPTY_PATH at the same time. Otherwise userspace will still need to go through /proc when trying to chmod a file handle they have.
Quoted 35 lines
fs/open.c +7
-2+ +return do_fchmodat (dfd, filename, mode, + flags &AT_SYMLINK_NOFOLLOW ?0 :LOOKUP_FOLLOW ); +} + SYSCALL_DEFINE3(fchmodat, int, dfd, const char __user *, filename, umode_t, mode) { -return do_fchmodat (dfd, filename, mode); +return do_fchmodat (dfd, filename, mode,LOOKUP_FOLLOW ); } SYSCALL_DEFINE2(chmod, const char __user *, filename, umode_t, mode) { -return do_fchmodat (AT_FDCWD , filename, mode); +return do_fchmodat (AT_FDCWD , filename, mode,LOOKUP_FOLLOW ); }/* include/linux/syscalls.h +2
-0@@ -440,6 +440,8 @@ asmlinkage long sys_chroot(const char __user *filename);asmlinkage long sys_fchmod (unsigned int fd,umode_t mode);asmlinkage long sys_fchmodat (int dfd,const char __user *filename,umode_t mode); +asmlinkage long sys_fchmodat2 (int dfd,const char __user *filename, +umode_t mode,int flags);asmlinkage long sys_fchownat (int dfd,const char __user *filename,uid_t user,gid_t group,int flag);asmlinkage long sys_fchown (unsigned int fd,uid_t user,gid_t group);Signature · 1 line
2.33.8
Signature · 4 lines
Aleksa Sarai Senior Software Engineer (Containers) SUSE Linux GmbH <https://www.cyphar.com/>
Re: [PATCH v4 2/5] fs: Add fchmodat2()
Quoted 32 lines · Aleksa Sarai
Quoted 31 lines · Alexey Gladkov
On the userspace side fchmodat(3) is implemented as a wrapper function which implements the POSIX-specified interface. This interface differs from the underlying kernel system call, which does not have a flags argument. Most implementations require procfs [1][2].
There doesn't appear to be a good userspace workaround for this issue but the implementation in the kernel is pretty straight-forward.
The new
fchmodat2()syscall allows to pass theAT_SYMLINK_NOFOLLOWflag, unlike existing fchmodat.[1] https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/fchmodat.c;h=17eca54051ee28ba1ec3f9aed170a62630959143;hb=a492b1e5ef7ab50c6fdd4e4e9879ea5569ab0a6c#l35
[2] https://git.musl-libc.org/cgit/musl/tree/src/stat/fchmodat.c?id=718f363bc2067b6487900eddc9180c84e7739f80#n28Co-developed-by Palmer Dabbelt <[email protected]>Signed-off-by Palmer Dabbelt <[email protected]>Signed-off-by Alexey Gladkov <[email protected]>Acked-by Arnd Bergmann <[email protected]>--- fs/open.c | 18 ++++++++++++++---- include/linux/syscalls.h | 2 ++ 2 files changed, 16 insertions(+), 4 deletions(-)fs/open.c +0
-0@@ -671,11 +671,11 @@ SYSCALL_DEFINE2(fchmod, unsigned int, fd, umode_t, mode)return err; }
Quoted 6 lines
fs/open.c SYSCALL_DEFINE2(fchmod, unsigned int, fd, umode_t, mode) · line 674
Quoted 2 lines
fs/open.c +1
-1-static int do_fchmodat (int dfd,const char __user *filename,umode_t mode) +static int do_fchmodat (int dfd,const char __user *filename,umode_t mode,int lookup_flags)I think it'd be much neater to do the conversion of
AT_flags here and pass 0 as a flags argument for all of the wrappers (this is how most of the otherxyz(),fxyz(),fxyzat()syscall wrappers are done IIRC).
I just addressed the Al Viro's suggestion.
https://lore.kernel.org/lkml/[email protected]/
Quoted 12 lines
Quoted 12 lines
fs/open.c +1
-1{struct path path;int error; -unsigned int lookup_flags =LOOKUP_FOLLOW ; + retry: error =user_path_at (dfd, filename, lookup_flags, &path);if (!error) {@@ -689,15 +689,25 @@ static int do_fchmodat(int dfd, const char __user *filename, umode_t mode)return error; }
Quoted 9 lines
fs/open.c static int do_fchmodat(int dfd, const char __user *filename, umode_t mode) · line 696
Quoted 5 lines
fs/open.c +5
-0+SYSCALL_DEFINE4(fchmodat2, int, dfd, const char __user *, filename, + umode_t, mode, int, flags) +{ +if (unlikely (flags & ~AT_SYMLINK_NOFOLLOW )) +return -EINVAL ;We almost certainly want to support
AT_EMPTY_PATHat the same time. Otherwise userspace will still need to go through /proc when trying to chmod a file handle they have.
I'm not sure I understand. Can you explain what you mean?
Signature · 1 line
Rgrds, legion
Christian Brauner
maintainerRe: [PATCH v4 2/5] fs: Add fchmodat2()
Quoted 3 lines
Quoted 3 lines
I think it'd be much neater to do the conversion of
AT_flags here and pass 0 as a flags argument for all of the wrappers (this is how most of the otherxyz(),fxyz(),fxyzat()syscall wrappers are done IIRC).
I've fixed that up in-tree.
Re: [PATCH v4 2/5] fs: Add fchmodat2()
In reply to Alexey GladkovQuoted 40 lines · Alexey Gladkov
Quoted 32 lines · Aleksa Sarai
Quoted 31 lines · Alexey Gladkov
On the userspace side fchmodat(3) is implemented as a wrapper function which implements the POSIX-specified interface. This interface differs from the underlying kernel system call, which does not have a flags argument. Most implementations require procfs [1][2].
There doesn't appear to be a good userspace workaround for this issue but the implementation in the kernel is pretty straight-forward.
The new
fchmodat2()syscall allows to pass theAT_SYMLINK_NOFOLLOWflag, unlike existing fchmodat.[1] https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/fchmodat.c;h=17eca54051ee28ba1ec3f9aed170a62630959143;hb=a492b1e5ef7ab50c6fdd4e4e9879ea5569ab0a6c#l35
[2] https://git.musl-libc.org/cgit/musl/tree/src/stat/fchmodat.c?id=718f363bc2067b6487900eddc9180c84e7739f80#n28Co-developed-by Palmer Dabbelt <[email protected]>Signed-off-by Palmer Dabbelt <[email protected]>Signed-off-by Alexey Gladkov <[email protected]>Acked-by Arnd Bergmann <[email protected]>--- fs/open.c | 18 ++++++++++++++---- include/linux/syscalls.h | 2 ++ 2 files changed, 16 insertions(+), 4 deletions(-)fs/open.c +0
-0@@ -671,11 +671,11 @@ SYSCALL_DEFINE2(fchmod, unsigned int, fd, umode_t, mode)return err; }Quoted 6 lines
fs/open.c SYSCALL_DEFINE2(fchmod, unsigned int, fd, umode_t, mode) · line 674
Quoted 2 lines
fs/open.c +1
-1-static int do_fchmodat (int dfd,const char __user *filename,umode_t mode) +static int do_fchmodat (int dfd,const char __user *filename,umode_t mode,int lookup_flags)I think it'd be much neater to do the conversion of
AT_flags here and pass 0 as a flags argument for all of the wrappers (this is how most of the otherxyz(),fxyz(),fxyzat()syscall wrappers are done IIRC).
Quoted 3 lines
I just addressed the Al Viro's suggestion.
I think Al misspoke, because he also said "pass it 0 as an extra argument", but you actually have to pass LOOKUP_FOLLOW from the wrappers. If you look at how faccessat2 and faccessat are implemented, it follows the behaviour I described.
Quoted 22 lines
Quoted 12 lines
Quoted 12 lines
fs/open.c +1
-1{struct path path;int error; -unsigned int lookup_flags =LOOKUP_FOLLOW ; + retry: error =user_path_at (dfd, filename, lookup_flags, &path);if (!error) {@@ -689,15 +689,25 @@ static int do_fchmodat(int dfd, const char __user *filename, umode_t mode)return error; }Quoted 9 lines
fs/open.c static int do_fchmodat(int dfd, const char __user *filename, umode_t mode) · line 696
Quoted 5 lines
fs/open.c +5
-0+SYSCALL_DEFINE4(fchmodat2, int, dfd, const char __user *, filename, + umode_t, mode, int, flags) +{ +if (unlikely (flags & ~AT_SYMLINK_NOFOLLOW )) +return -EINVAL ;We almost certainly want to support
AT_EMPTY_PATHat the same time. Otherwise userspace will still need to go through /proc when trying to chmod a file handle they have.
Quoted 1 line
I'm not sure I understand. Can you explain what you mean?
You should add support for AT_EMPTY_PATH (LOOKUP_EMPTY) as well as AT_SYMLINK_NOFOLLOW. It would only require something like:
unsigned int lookup_flags = LOOKUP_FOLLOW; if (flags & ~(AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW)) return -EINVAL; if (flags & AT_EMPTY_PATH) lookup_flags |= LOOKUP_EMPTY; if (flags & AT_SYMLINK_NOFOLLOW) lookup_flags &= ~LOOKUP_FOLLOW; /* ... */
This would be effectively equivalent to fchmod(fd, mode). (I was wrong when I said this wasn't already possible -- I forgot about fchmod(2).)
Signature · 4 lines
Aleksa Sarai Senior Software Engineer (Containers) SUSE Linux GmbH <https://www.cyphar.com/>
Re: [PATCH v4 2/5] fs: Add fchmodat2()
Quoted 89 lines · Aleksa Sarai
Quoted 40 lines · Alexey Gladkov
Quoted 32 lines · Aleksa Sarai
Quoted 31 lines · Alexey Gladkov
On the userspace side fchmodat(3) is implemented as a wrapper function which implements the POSIX-specified interface. This interface differs from the underlying kernel system call, which does not have a flags argument. Most implementations require procfs [1][2].
There doesn't appear to be a good userspace workaround for this issue but the implementation in the kernel is pretty straight-forward.
The new
fchmodat2()syscall allows to pass theAT_SYMLINK_NOFOLLOWflag, unlike existing fchmodat.[1] https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/fchmodat.c;h=17eca54051ee28ba1ec3f9aed170a62630959143;hb=a492b1e5ef7ab50c6fdd4e4e9879ea5569ab0a6c#l35
[2] https://git.musl-libc.org/cgit/musl/tree/src/stat/fchmodat.c?id=718f363bc2067b6487900eddc9180c84e7739f80#n28Co-developed-by Palmer Dabbelt <[email protected]>Signed-off-by Palmer Dabbelt <[email protected]>Signed-off-by Alexey Gladkov <[email protected]>Acked-by Arnd Bergmann <[email protected]>--- fs/open.c | 18 ++++++++++++++---- include/linux/syscalls.h | 2 ++ 2 files changed, 16 insertions(+), 4 deletions(-)fs/open.c +0
-0@@ -671,11 +671,11 @@ SYSCALL_DEFINE2(fchmod, unsigned int, fd, umode_t, mode)return err; }Quoted 6 lines
fs/open.c SYSCALL_DEFINE2(fchmod, unsigned int, fd, umode_t, mode) · line 674
Quoted 2 lines
fs/open.c +1
-1-static int do_fchmodat (int dfd,const char __user *filename,umode_t mode) +static int do_fchmodat (int dfd,const char __user *filename,umode_t mode,int lookup_flags)I think it'd be much neater to do the conversion of
AT_flags here and pass 0 as a flags argument for all of the wrappers (this is how most of the otherxyz(),fxyz(),fxyzat()syscall wrappers are done IIRC).Quoted 3 lines
I just addressed the Al Viro's suggestion.
I think Al misspoke, because he also said "pass it 0 as an extra argument", but you actually have to pass
LOOKUP_FOLLOWfrom the wrappers. If you look at how faccessat2 and faccessat are implemented, it follows the behaviour I described.Quoted 22 lines
Quoted 12 lines
Quoted 12 lines
fs/open.c +1
-1{struct path path;int error; -unsigned int lookup_flags =LOOKUP_FOLLOW ; + retry: error =user_path_at (dfd, filename, lookup_flags, &path);if (!error) {@@ -689,15 +689,25 @@ static int do_fchmodat(int dfd, const char __user *filename, umode_t mode)return error; }Quoted 9 lines
fs/open.c static int do_fchmodat(int dfd, const char __user *filename, umode_t mode) · line 696
Quoted 5 lines
fs/open.c +5
-0+SYSCALL_DEFINE4(fchmodat2, int, dfd, const char __user *, filename, + umode_t, mode, int, flags) +{ +if (unlikely (flags & ~AT_SYMLINK_NOFOLLOW )) +return -EINVAL ;We almost certainly want to support
AT_EMPTY_PATHat the same time. Otherwise userspace will still need to go through /proc when trying to chmod a file handle they have.Quoted 1 line
I'm not sure I understand. Can you explain what you mean?
You should add support for
AT_EMPTY_PATH(LOOKUP_EMPTY) as well asAT_SYMLINK_NOFOLLOW. It would only require something like:unsigned int lookup_flags = LOOKUP_FOLLOW; if (flags & ~(AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW)) return -EINVAL; if (flags & AT_EMPTY_PATH) lookup_flags |= LOOKUP_EMPTY; if (flags & AT_SYMLINK_NOFOLLOW) lookup_flags &= ~LOOKUP_FOLLOW; /* ... */
Quoted 2 lines
This would be effectively equivalent to
fchmod(fd, mode). (I was wrong when I said this wasn't already possible -- I forgot about fchmod(2).)
... with the exception (as Christian mentioned) of O_PATH descriptors. However, there are two counter-points to this:
* fchownat(AT_EMPTY_PATH) exists but fchown() doesn't work on O_PATH descriptors *by design* (according to open(2)).
* chmod(/proc/self/fd/$n) works on O_PATH descriptors, meaning this behaviour is already allowed and all that AT_EMPTY_PATH would do is allow programs to avoid depending on procfs for this.
FWIW, I agree with Christian that these behaviours are not ideal (and I'm working on a series that might allow for these things to be properly blocked in the future) but there's also the consistency argument -- I don't think fchownat() is much safer to allow in this way than fchmodat() and (again) this behaviour is already possible through procfs.
Ultimately, we can always add AT_EMPTY_PATH later. It just seemed like an obvious omission to me that would be easy to resolve.
Signature · 4 lines
Aleksa Sarai Senior Software Engineer (Containers) SUSE Linux GmbH <https://www.cyphar.com/>
RE: [PATCH v4 2/5] fs: Add fchmodat2()
...
Quoted 6 lines
FWIW, I agree with Christian that these behaviours are not ideal (and I'm working on a series that might allow for these things to be properly blocked in the future) but there's also the consistency argument -- I don't think
fchownat()is much safer to allow in this way thanfchmodat()and (again) this behaviour is already possible through procfs.
If the 'through procfs' involves readlink("/proc/self/fd/n") and accessing through the returned path then the permission checks are different.
Using the returned path requires search permissions on all the directories.
This is all fine for xxxat() functions where a real open directory fd is supplied.
But other cases definitely need a lot of thought to ensure they don't let programs acquire permissions they aren't supposed to have.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
[email protected]
maintainerRe: [PATCH v4 2/5] fs: Add fchmodat2()
Quoted 8 lines · David Laight
....
Quoted 6 lines
FWIW, I agree with Christian that these behaviours are not ideal (and I'm working on a series that might allow for these things to be properly blocked in the future) but there's also the consistency argument -- I don't think
fchownat()is much safer to allow in this way thanfchmodat()and (again) this behaviour is already possible through procfs.
Quoted 5 lines
If the 'through procfs' involves
readlink("/proc/self/fd/n")and accessing through the returned path then the permission checks are different.
Using the returned path requires search permissions on all the directories.
That's *not* how "through procfs" works. The "magic symlinks" in /proc/*/fd are not actual symlinks that get dereferenced to the contents they readlink() to, but special-type objects that dereference directly to the underlying file associated with the open file description.
Rich
RE: [PATCH v4 2/5] fs: Add fchmodat2()
In reply to Aleksa SaraiFrom: Aleksa Sarai
Quoted 1 line
Sent: 25 July 2023 17:36
...
Quoted 3 lines
We almost certainly want to support
AT_EMPTY_PATHat the same time. Otherwise userspace will still need to go through /proc when trying to chmod a file handle they have.
That can't be allowed.
Just because a process has a file open and write access to the directory that contains it doesn't mean they are allowed to change the file permissions.
They also need directory search access from a directory they have open through to the containing directory.
David
Re: [PATCH v4 2/5] fs: Add fchmodat2()
Quoted 8 lines · David Laight
From: Aleksa Sarai
Quoted 1 line
Sent: 25 July 2023 17:36
...
Quoted 3 lines
We almost certainly want to support
AT_EMPTY_PATHat the same time. Otherwise userspace will still need to go through /proc when trying to chmod a file handle they have.That can't be allowed.
IIUC, fchmodat2(fd, "", m, AT_EMPTY_PATH) is equivalent to fchmod(fd, m). With that, new architectures only need to implement the fchmodat2 syscall to cover all chmod variants.
Signature · 3 lines
Andreas Schwab, [email protected] GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1 "And now for something completely different."
Christian Brauner
maintainerRe: [PATCH v4 2/5] fs: Add fchmodat2()
Quoted 9 lines · Andreas Schwab
Quoted 7 lines · David Laight
From: Aleksa Sarai
Quoted 1 line
Sent: 25 July 2023 17:36
...
Quoted 3 lines
We almost certainly want to support
AT_EMPTY_PATHat the same time. Otherwise userspace will still need to go through /proc when trying to chmod a file handle they have.
Quoted 5 lines
Quoted 1 line
That can't be allowed.
IIUC,
fchmodat2(fd, "", m, AT_EMPTY_PATH)is equivalent tofchmod(fd, m). With that, new architectures only need to implement the fchmodat2 syscall to cover all chmod variants.
There's a difference though as fchmod() doesn't work with O_PATH file descriptors while AT_EMPTY_PATH does. Similar to how fchown() doesn't work with O_PATH file descriptors.
However, we do allow AT_EMPTY_PATH with fchownat() so there's no reason to not allow it for fchmodat2().
But it's a bit of a shame that O_PATH looks less and less like O_PATH. It came from can-do-barely-anything to can-do-quite-a-lot-of-things over the years.
In any case, AT_EMPTY_PATH for fchmodat2() can be an additional patch on top.
[email protected]
maintainerRe: [PATCH v4 2/5] fs: Add fchmodat2()
Quoted 23 lines · Christian Brauner
Quoted 9 lines · Andreas Schwab
Quoted 7 lines · David Laight
From: Aleksa Sarai
Quoted 1 line
Sent: 25 July 2023 17:36
...
Quoted 3 lines
We almost certainly want to support
AT_EMPTY_PATHat the same time. Otherwise userspace will still need to go through /proc when trying to chmod a file handle they have.Quoted 5 lines
Quoted 1 line
That can't be allowed.
IIUC,
fchmodat2(fd, "", m, AT_EMPTY_PATH)is equivalent tofchmod(fd, m). With that, new architectures only need to implement the fchmodat2 syscall to cover all chmod variants.There's a difference though as
fchmod()doesn't work withO_PATHfile descriptors whileAT_EMPTY_PATHdoes. Similar to howfchown()doesn't work withO_PATHfile descriptors.However, we do allow
AT_EMPTY_PATHwithfchownat()so there's no reason to not allow it forfchmodat2().
Quoted 6 lines
But it's a bit of a shame that
O_PATHlooks less and less likeO_PATH. It came from can-do-barely-anything to can-do-quite-a-lot-of-things over the years.In any case,
AT_EMPTY_PATHforfchmodat2()can be an additional patch on top.
From a standpoint of implementing O_SEARCH/O_EXEC using it, I don't see any reason fchown/fchmod should not work on O_PATH file descriptors. And indeed when you have procfs available to emulate them via procfs, it already does. So I don't see this as unwanted functionality or an access control regression. I see it as things behaving as expected.
Semantically, O_PATH is a reference to the inode, not to the dirent. So there is no reason you should not be able to do things that need permission to the inode (changing permissions on it) rather than to the dirent (renaming/moving).
Rich
Christian Brauner
maintainerRe: [PATCH v4 2/5] fs: Add fchmodat2()
Quoted 28 lines · On Thu, Jul 27, 2023 at 01:13:37PM -0400, [email protected] wrote:
Quoted 27 lines · Christian Brauner
Quoted 9 lines · Andreas Schwab
Quoted 7 lines · David Laight
From: Aleksa Sarai
Quoted 1 line
Sent: 25 July 2023 17:36
...
Quoted 3 lines
We almost certainly want to support
AT_EMPTY_PATHat the same time. Otherwise userspace will still need to go through /proc when trying to chmod a file handle they have.Quoted 5 lines
Quoted 1 line
That can't be allowed.
IIUC,
fchmodat2(fd, "", m, AT_EMPTY_PATH)is equivalent tofchmod(fd, m). With that, new architectures only need to implement the fchmodat2 syscall to cover all chmod variants.There's a difference though as
fchmod()doesn't work withO_PATHfile descriptors whileAT_EMPTY_PATHdoes. Similar to howfchown()doesn't work withO_PATHfile descriptors.However, we do allow
AT_EMPTY_PATHwithfchownat()so there's no reason to not allow it forfchmodat2().But it's a bit of a shame that
O_PATHlooks less and less likeO_PATH. It came from can-do-barely-anything to can-do-quite-a-lot-of-things over the years.
Quoted 7 lines
Quoted 2 lines
In any case,
AT_EMPTY_PATHforfchmodat2()can be an additional patch on top.From a standpoint of implementing O_SEARCH/O_EXEC using it, I don't see any reason fchown/fchmod should not work on
O_PATHfile descriptors. And indeed when you have procfs available to emulate them via procfs, it already does. So I don't see this as unwanted
I'm really not talking about the fact that proc is a giant loophole for basically everyhing related to O_PATH and reopening fds.
I'm saying that both fchmod() and fchown() don't work on O_PATH fds. They explicitly reject them.
AT_EMPTY_PATH and therefore O_PATH for fchmodat2() is fine given that we do it for fchownat() already.
[email protected]
maintainerRe: [PATCH v4 2/5] fs: Add fchmodat2()
In reply to David LaightQuoted 7 lines · David Laight
From: Aleksa Sarai
Quoted 1 line
Sent: 25 July 2023 17:36
....
Quoted 3 lines
We almost certainly want to support
AT_EMPTY_PATHat the same time. Otherwise userspace will still need to go through /proc when trying to chmod a file handle they have.
Quoted 8 lines
That can't be allowed.
Just because a process has a file open and write access to the directory that contains it doesn't mean they are allowed to change the file permissions.
They also need directory search access from a directory they have open through to the containing directory.
Am I missing something? How is this different from fchmod?
Rich
PATCH v4 3/5 arch: Register fchmodat2, usually as syscall 452
In reply to Alexey GladkovFrom: Palmer Dabbelt <[email protected]>
This registers the new fchmodat2 syscall in most places as nuber 452, with alpha being the exception where it's 562. I found all these sites by grepping for fspick, which I assume has found me everything.
--- arch/alpha/kernel/syscalls/syscall.tbl | 1 + arch/arm/tools/syscall.tbl | 1 + arch/arm64/include/asm/unistd.h | 2 +- arch/arm64/include/asm/unistd32.h | 2 ++ arch/ia64/kernel/syscalls/syscall.tbl | 1 + arch/m68k/kernel/syscalls/syscall.tbl | 1 + arch/microblaze/kernel/syscalls/syscall.tbl | 1 + arch/mips/kernel/syscalls/syscall_n32.tbl | 1 + arch/mips/kernel/syscalls/syscall_n64.tbl | 1 + arch/mips/kernel/syscalls/syscall_o32.tbl | 1 + arch/parisc/kernel/syscalls/syscall.tbl | 1 + arch/powerpc/kernel/syscalls/syscall.tbl | 1 + arch/s390/kernel/syscalls/syscall.tbl | 1 + arch/sh/kernel/syscalls/syscall.tbl | 1 + arch/sparc/kernel/syscalls/syscall.tbl | 1 + arch/x86/entry/syscalls/syscall_32.tbl | 1 + arch/x86/entry/syscalls/syscall_64.tbl | 1 + arch/xtensa/kernel/syscalls/syscall.tbl | 1 + include/uapi/asm-generic/unistd.h | 5 ++++- 19 files changed, 23 insertions(+), 2 deletions(-)
arch/alpha/kernel/syscalls/syscall.tbl +1 -0
@@ -491,3 +491,4 @@ 559 common futex_waitv sys_futex_waitv 560 common set_mempolicy_home_node sys_ni_syscall 561 common cachestat sys_cachestat +562 common fchmodat2 sys_fchmodat2
arch/arm/tools/syscall.tbl +1 -0
@@ -465,3 +465,4 @@ 449 common futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node 451 common cachestat sys_cachestat +452 common fchmodat2 sys_fchmodat2
arch/arm64/include/asm/unistd.h +1 -1
@@ -39,7 +39,7 @@#define __ARM_NR_compat_set_tls (__ARM_NR_COMPAT_BASE + 5)#define __ARM_NR_COMPAT_END (__ARM_NR_COMPAT_BASE + 0x800) -#define __NR_compat_syscalls 452 +#define __NR_compat_syscalls 453#endif #define __ARCH_WANT_SYS_CLONE
arch/arm64/include/asm/unistd32.h +2 -0
@@ -909,6 +909,8 @@ __SYSCALL(__NR_futex_waitv, sys_futex_waitv) __SYSCALL(__NR_set_mempolicy_home_node, sys_set_mempolicy_home_node)#define __NR_cachestat 451 __SYSCALL(__NR_cachestat, sys_cachestat) +#define __NR_fchmodat2 452 +__SYSCALL (__NR_fchmodat2, sys_fchmodat2)/* * Please add new compat syscalls above this comment and update
arch/ia64/kernel/syscalls/syscall.tbl +1 -0
@@ -372,3 +372,4 @@ 449 common futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node 451 common cachestat sys_cachestat +452 common fchmodat2 sys_fchmodat2
arch/m68k/kernel/syscalls/syscall.tbl +1 -0
@@ -451,3 +451,4 @@ 449 common futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node 451 common cachestat sys_cachestat +452 common fchmodat2 sys_fchmodat2
arch/microblaze/kernel/syscalls/syscall.tbl +1 -0
@@ -457,3 +457,4 @@ 449 common futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node 451 common cachestat sys_cachestat +452 common fchmodat2 sys_fchmodat2
arch/mips/kernel/syscalls/syscall_n32.tbl +1 -0
@@ -390,3 +390,4 @@ 449 n32 futex_waitv sys_futex_waitv 450 n32 set_mempolicy_home_node sys_set_mempolicy_home_node 451 n32 cachestat sys_cachestat +452 n32 fchmodat2 sys_fchmodat2
arch/mips/kernel/syscalls/syscall_n64.tbl +1 -0
@@ -366,3 +366,4 @@ 449 n64 futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node 451 n64 cachestat sys_cachestat +452 n64 fchmodat2 sys_fchmodat2
arch/mips/kernel/syscalls/syscall_o32.tbl +1 -0
@@ -439,3 +439,4 @@ 449 o32 futex_waitv sys_futex_waitv 450 o32 set_mempolicy_home_node sys_set_mempolicy_home_node 451 o32 cachestat sys_cachestat +452 o32 fchmodat2 sys_fchmodat2
arch/parisc/kernel/syscalls/syscall.tbl +1 -0
@@ -450,3 +450,4 @@ 449 common futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node 451 common cachestat sys_cachestat +452 common fchmodat2 sys_fchmodat2
arch/powerpc/kernel/syscalls/syscall.tbl +1 -0
@@ -538,3 +538,4 @@ 449 common futex_waitv sys_futex_waitv 450 nospu set_mempolicy_home_node sys_set_mempolicy_home_node 451 common cachestat sys_cachestat +452 common fchmodat2 sys_fchmodat2
arch/s390/kernel/syscalls/syscall.tbl +1 -0
@@ -454,3 +454,4 @@ 449 common futex_waitv sys_futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node sys_set_mempolicy_home_node 451 common cachestat sys_cachestat sys_cachestat +452 common fchmodat2 sys_fchmodat2 sys_fchmodat2
arch/sh/kernel/syscalls/syscall.tbl +1 -0
@@ -454,3 +454,4 @@ 449 common futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node 451 common cachestat sys_cachestat +452 common fchmodat2 sys_fchmodat2
arch/sparc/kernel/syscalls/syscall.tbl +1 -0
@@ -497,3 +497,4 @@ 449 common futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node 451 common cachestat sys_cachestat +452 common fchmodat2 sys_fchmodat2
arch/x86/entry/syscalls/syscall_32.tbl +1 -0
@@ -456,3 +456,4 @@ 449 i386 futex_waitv sys_futex_waitv 450 i386 set_mempolicy_home_node sys_set_mempolicy_home_node 451 i386 cachestat sys_cachestat +452 i386 fchmodat2 sys_fchmodat2
arch/x86/entry/syscalls/syscall_64.tbl +1 -0
@@ -373,6 +373,7 @@ 449 common futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node 451 common cachestat sys_cachestat +452 common fchmodat2 sys_fchmodat2 # # Due to a historical design error, certain syscalls are numbered differently
arch/xtensa/kernel/syscalls/syscall.tbl +1 -0
@@ -422,3 +422,4 @@ 449 common futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node 451 common cachestat sys_cachestat +452 common fchmodat2 sys_fchmodat2
include/uapi/asm-generic/unistd.h +4 -1
@@ -820,8 +820,11 @@ __SYSCALL(__NR_set_mempolicy_home_node, sys_set_mempolicy_home_node)#define __NR_cachestat 451 __SYSCALL(__NR_cachestat, sys_cachestat) +#define __NR_fchmodat2 452 +__SYSCALL(__NR_fchmodat2, sys_fchmodat2) +#undef __NR_syscalls -#define __NR_syscalls 452 +#define __NR_syscalls 453/* * 32 bit systems traditionally used different
Signature · 1 line
2.33.8
Re: [PATCH v4 3/5] arch: Register fchmodat2, usually as syscall 452
Quoted 8 lines · Alexey Gladkov
From: Palmer Dabbelt <[email protected]>
This registers the new fchmodat2 syscall in most places as nuber 452, with alpha being the exception where it's 562. I found all these sites by grepping for fspick, which I assume has found me everything.
Signed-off-by Palmer Dabbelt <[email protected]>Signed-off-by Alexey Gladkov <[email protected]>
Re: [PATCH v4 3/5] arch: Register fchmodat2, usually as syscall 452
In reply to Alexey GladkovQuoted 10 lines · On Tue, Jul 11, 2023 at 6:25 PM Alexey Gladkov…
From: Palmer Dabbelt <[email protected]>
This registers the new fchmodat2 syscall in most places as nuber 452, with alpha being the exception where it's 562. I found all these sites by grepping for fspick, which I assume has found me everything.
Signed-off-by Palmer Dabbelt <[email protected]>Signed-off-by Alexey Gladkov <[email protected]>arch/m68k/kernel/syscalls/syscall.tbl | 1 +
Gr{oetje,eeting}s,
Geert
Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected] In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Re: [PATCH v4 3/5] arch: Register fchmodat2, usually as syscall 452
In reply to Alexey GladkovQuoted 5 lines · Alexey Gladkov
From: Palmer Dabbelt <[email protected]>
This registers the new fchmodat2 syscall in most places as nuber 452, with alpha being the exception where it's 562. I found all these sites by grepping for fspick, which I assume has found me everything.
Shouldn't this patch be squashed with the patch that adds the syscall? At least, that's how I've usually seen it done...
Quoted 217 lines
Signed-off-by Palmer Dabbelt <[email protected]>Signed-off-by Alexey Gladkov <[email protected]>--- arch/alpha/kernel/syscalls/syscall.tbl | 1 + arch/arm/tools/syscall.tbl | 1 + arch/arm64/include/asm/unistd.h | 2 +- arch/arm64/include/asm/unistd32.h | 2 ++ arch/ia64/kernel/syscalls/syscall.tbl | 1 + arch/m68k/kernel/syscalls/syscall.tbl | 1 + arch/microblaze/kernel/syscalls/syscall.tbl | 1 + arch/mips/kernel/syscalls/syscall_n32.tbl | 1 + arch/mips/kernel/syscalls/syscall_n64.tbl | 1 + arch/mips/kernel/syscalls/syscall_o32.tbl | 1 + arch/parisc/kernel/syscalls/syscall.tbl | 1 + arch/powerpc/kernel/syscalls/syscall.tbl | 1 + arch/s390/kernel/syscalls/syscall.tbl | 1 + arch/sh/kernel/syscalls/syscall.tbl | 1 + arch/sparc/kernel/syscalls/syscall.tbl | 1 + arch/x86/entry/syscalls/syscall_32.tbl | 1 + arch/x86/entry/syscalls/syscall_64.tbl | 1 + arch/xtensa/kernel/syscalls/syscall.tbl | 1 + include/uapi/asm-generic/unistd.h | 5 ++++- 19 files changed, 23 insertions(+), 2 deletions(-)arch/alpha/kernel/syscalls/syscall.tbl +1
-0@@ -491,3 +491,4 @@ 559 common futex_waitv sys_futex_waitv 560 common set_mempolicy_home_node sys_ni_syscall 561 common cachestat sys_cachestat +562 common fchmodat2 sys_fchmodat2arch/arm/tools/syscall.tbl +1
-0@@ -465,3 +465,4 @@ 449 common futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node 451 common cachestat sys_cachestat +452 common fchmodat2 sys_fchmodat2arch/arm64/include/asm/unistd.h +1
-1@@ -39,7 +39,7 @@#define __ARM_NR_compat_set_tls (__ARM_NR_COMPAT_BASE + 5)#define __ARM_NR_COMPAT_END (__ARM_NR_COMPAT_BASE + 0x800) -#define __NR_compat_syscalls 452 +#define __NR_compat_syscalls 453#endif #define __ARCH_WANT_SYS_CLONEarch/arm64/include/asm/unistd32.h +2
-0@@ -909,6 +909,8 @@ __SYSCALL(__NR_futex_waitv, sys_futex_waitv) __SYSCALL(__NR_set_mempolicy_home_node, sys_set_mempolicy_home_node)#define __NR_cachestat 451 __SYSCALL(__NR_cachestat, sys_cachestat) +#define __NR_fchmodat2 452 +__SYSCALL (__NR_fchmodat2, sys_fchmodat2)/* * Please add new compat syscalls above this comment and update arch/ia64/kernel/syscalls/syscall.tbl +1
-0@@ -372,3 +372,4 @@ 449 common futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node 451 common cachestat sys_cachestat +452 common fchmodat2 sys_fchmodat2arch/m68k/kernel/syscalls/syscall.tbl +1
-0@@ -451,3 +451,4 @@ 449 common futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node 451 common cachestat sys_cachestat +452 common fchmodat2 sys_fchmodat2arch/microblaze/kernel/syscalls/syscall.tbl +1
-0@@ -457,3 +457,4 @@ 449 common futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node 451 common cachestat sys_cachestat +452 common fchmodat2 sys_fchmodat2arch/mips/kernel/syscalls/syscall_n32.tbl +1
-0@@ -390,3 +390,4 @@ 449 n32 futex_waitv sys_futex_waitv 450 n32 set_mempolicy_home_node sys_set_mempolicy_home_node 451 n32 cachestat sys_cachestat +452 n32 fchmodat2 sys_fchmodat2arch/mips/kernel/syscalls/syscall_n64.tbl +1
-0@@ -366,3 +366,4 @@ 449 n64 futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node 451 n64 cachestat sys_cachestat +452 n64 fchmodat2 sys_fchmodat2arch/mips/kernel/syscalls/syscall_o32.tbl +1
-0@@ -439,3 +439,4 @@ 449 o32 futex_waitv sys_futex_waitv 450 o32 set_mempolicy_home_node sys_set_mempolicy_home_node 451 o32 cachestat sys_cachestat +452 o32 fchmodat2 sys_fchmodat2arch/parisc/kernel/syscalls/syscall.tbl +1
-0@@ -450,3 +450,4 @@ 449 common futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node 451 common cachestat sys_cachestat +452 common fchmodat2 sys_fchmodat2arch/powerpc/kernel/syscalls/syscall.tbl +1
-0@@ -538,3 +538,4 @@ 449 common futex_waitv sys_futex_waitv 450 nospu set_mempolicy_home_node sys_set_mempolicy_home_node 451 common cachestat sys_cachestat +452 common fchmodat2 sys_fchmodat2arch/s390/kernel/syscalls/syscall.tbl +1
-0@@ -454,3 +454,4 @@ 449 common futex_waitv sys_futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node sys_set_mempolicy_home_node 451 common cachestat sys_cachestat sys_cachestat +452 common fchmodat2 sys_fchmodat2 sys_fchmodat2arch/sh/kernel/syscalls/syscall.tbl +1
-0@@ -454,3 +454,4 @@ 449 common futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node 451 common cachestat sys_cachestat +452 common fchmodat2 sys_fchmodat2arch/sparc/kernel/syscalls/syscall.tbl +1
-0@@ -497,3 +497,4 @@ 449 common futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node 451 common cachestat sys_cachestat +452 common fchmodat2 sys_fchmodat2arch/x86/entry/syscalls/syscall_32.tbl +1
-0@@ -456,3 +456,4 @@ 449 i386 futex_waitv sys_futex_waitv 450 i386 set_mempolicy_home_node sys_set_mempolicy_home_node 451 i386 cachestat sys_cachestat +452 i386 fchmodat2 sys_fchmodat2arch/x86/entry/syscalls/syscall_64.tbl +1
-0@@ -373,6 +373,7 @@ 449 common futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node 451 common cachestat sys_cachestat +452 common fchmodat2 sys_fchmodat2 # # Due to a historical design error, certain syscalls are numbered differentlyarch/xtensa/kernel/syscalls/syscall.tbl +1
-0@@ -422,3 +422,4 @@ 449 common futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node 451 common cachestat sys_cachestat +452 common fchmodat2 sys_fchmodat2include/uapi/asm-generic/unistd.h +4
-1@@ -820,8 +820,11 @@ __SYSCALL(__NR_set_mempolicy_home_node, sys_set_mempolicy_home_node)#define __NR_cachestat 451 __SYSCALL(__NR_cachestat, sys_cachestat) +#define __NR_fchmodat2 452 +__SYSCALL(__NR_fchmodat2, sys_fchmodat2) +#undef __NR_syscalls -#define __NR_syscalls 452 +#define __NR_syscalls 453/* * 32 bit systems traditionally used different Signature · 1 line
2.33.8
Signature · 4 lines
Aleksa Sarai Senior Software Engineer (Containers) SUSE Linux GmbH <https://www.cyphar.com/>
Christian Brauner
maintainerRe: [PATCH v4 3/5] arch: Register fchmodat2, usually as syscall 452
Quoted 9 lines · Aleksa Sarai
Quoted 5 lines · Alexey Gladkov
From: Palmer Dabbelt <[email protected]>
This registers the new fchmodat2 syscall in most places as nuber 452, with alpha being the exception where it's 562. I found all these sites by grepping for fspick, which I assume has found me everything.
Shouldn't this patch be squashed with the patch that adds the syscall? At least, that's how I've usually seen it done...
Depends. Iirc, someone said they'd prefer for doing it in one patch in some circumstances on some system call we added years ago. But otoh, having the syscall wiring done separately makes it easy for arch maintainers to ack only the wiring up part. Both ways are valid imho. (cachestat() did it for x86 and then all the others separately. So really it seems a bit all over the place depending on the scenario.)
Re: [PATCH v4 3/5] arch: Register fchmodat2, usually as syscall 452
Quoted 11 lines · Christian Brauner
Quoted 9 lines · Aleksa Sarai
Quoted 5 lines · Alexey Gladkov
From: Palmer Dabbelt <[email protected]>
This registers the new fchmodat2 syscall in most places as nuber 452, with alpha being the exception where it's 562. I found all these sites by grepping for fspick, which I assume has found me everything.
Shouldn't this patch be squashed with the patch that adds the syscall? At least, that's how I've usually seen it done...
Quoted 6 lines
Depends. Iirc, someone said they'd prefer for doing it in one patch in some circumstances on some system call we added years ago. But otoh, having the syscall wiring done separately makes it easy for arch maintainers to ack only the wiring up part. Both ways are valid imho. (
cachestat()did it for x86 and then all the others separately. So really it seems a bit all over the place depending on the scenario.)
Fair enough!
Signature · 4 lines
Aleksa Sarai Senior Software Engineer (Containers) SUSE Linux GmbH <https://www.cyphar.com/>
PATCH v4 4/5 tools headers UAPI: Sync files changed by new fchmodat2 syscall
In reply to Alexey GladkovFrom: Palmer Dabbelt <[email protected]>
That add support for this new syscall in tools such as 'perf trace'.
--- tools/include/uapi/asm-generic/unistd.h | 5 ++++- tools/perf/arch/mips/entry/syscalls/syscall_n64.tbl | 2 ++ tools/perf/arch/powerpc/entry/syscalls/syscall.tbl | 2 ++ tools/perf/arch/s390/entry/syscalls/syscall.tbl | 2 ++ tools/perf/arch/x86/entry/syscalls/syscall_64.tbl | 2 ++ 5 files changed, 12 insertions(+), 1 deletion(-)
tools/include/uapi/asm-generic/unistd.h +4 -1
@@ -817,8 +817,11 @@ __SYSCALL(__NR_futex_waitv, sys_futex_waitv)#define __NR_set_mempolicy_home_node 450 __SYSCALL(__NR_set_mempolicy_home_node, sys_set_mempolicy_home_node) +#define __NR_fchmodat2 452 +__SYSCALL(__NR_fchmodat2, sys_fchmodat2) +#undef __NR_syscalls -#define __NR_syscalls 451 +#define __NR_syscalls 453/* * 32 bit systems traditionally used different
tools/perf/arch/mips/entry/syscalls/syscall_n64.tbl +2 -0
@@ -365,3 +365,5 @@ 448 n64 process_mrelease sys_process_mrelease 449 n64 futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node +# 451 reserved for cachestat +452 n64 fchmodat2 sys_fchmodat2
tools/perf/arch/powerpc/entry/syscalls/syscall.tbl +2 -0
@@ -537,3 +537,5 @@ 448 common process_mrelease sys_process_mrelease 449 common futex_waitv sys_futex_waitv 450 nospu set_mempolicy_home_node sys_set_mempolicy_home_node +# 451 reserved for cachestat +452 common fchmodat2 sys_fchmodat2
tools/perf/arch/s390/entry/syscalls/syscall.tbl +2 -0
@@ -453,3 +453,5 @@ 448 common process_mrelease sys_process_mrelease sys_process_mrelease 449 common futex_waitv sys_futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node sys_set_mempolicy_home_node +# 451 reserved for cachestat +452 common fchmodat2 sys_fchmodat2 sys_fchmodat2
tools/perf/arch/x86/entry/syscalls/syscall_64.tbl +2 -0
@@ -372,6 +372,8 @@ 448 common process_mrelease sys_process_mrelease 449 common futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node +# 451 reserved for cachestat +452 common fchmodat2 sys_fchmodat2 # # Due to a historical design error, certain syscalls are numbered differently
Signature · 1 line
2.33.8
Namhyung Kim
maintainerRe: [PATCH v4 4/5] tools headers UAPI: Sync files changed by new fchmodat2 syscall
Hello,
Quoted 7 lines · On Tue, Jul 11, 2023 at 9:18 AM Alexey Gladkov <[email protected]> wrote:
From: Palmer Dabbelt <[email protected]>
That add support for this new syscall in tools such as 'perf trace'.
Signed-off-by Palmer Dabbelt <[email protected]>Signed-off-by Alexey Gladkov <[email protected]>
Quoted 7 lines
--- tools/include/uapi/asm-generic/unistd.h | 5 ++++- tools/perf/arch/mips/entry/syscalls/syscall_n64.tbl | 2 ++ tools/perf/arch/powerpc/entry/syscalls/syscall.tbl | 2 ++ tools/perf/arch/s390/entry/syscalls/syscall.tbl | 2 ++ tools/perf/arch/x86/entry/syscalls/syscall_64.tbl | 2 ++ 5 files changed, 12 insertions(+), 1 deletion(-)
It'd be nice if you route this patch separately through the perf tools tree. We can add this after the kernel change is accepted.
Thanks,
Namhyung
Quoted 64 lines
tools/include/uapi/asm-generic/unistd.h +4
-1@@ -817,8 +817,11 @@ __SYSCALL(__NR_futex_waitv, sys_futex_waitv)#define __NR_set_mempolicy_home_node 450 __SYSCALL(__NR_set_mempolicy_home_node, sys_set_mempolicy_home_node) +#define __NR_fchmodat2 452 +__SYSCALL(__NR_fchmodat2, sys_fchmodat2) +#undef __NR_syscalls -#define __NR_syscalls 451 +#define __NR_syscalls 453/* * 32 bit systems traditionally used different tools/perf/arch/mips/entry/syscalls/syscall_n64.tbl +2
-0@@ -365,3 +365,5 @@ 448 n64 process_mrelease sys_process_mrelease 449 n64 futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node +# 451 reserved for cachestat +452 n64 fchmodat2 sys_fchmodat2tools/perf/arch/powerpc/entry/syscalls/syscall.tbl +2
-0@@ -537,3 +537,5 @@ 448 common process_mrelease sys_process_mrelease 449 common futex_waitv sys_futex_waitv 450 nospu set_mempolicy_home_node sys_set_mempolicy_home_node +# 451 reserved for cachestat +452 common fchmodat2 sys_fchmodat2tools/perf/arch/s390/entry/syscalls/syscall.tbl +2
-0@@ -453,3 +453,5 @@ 448 common process_mrelease sys_process_mrelease sys_process_mrelease 449 common futex_waitv sys_futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node sys_set_mempolicy_home_node +# 451 reserved for cachestat +452 common fchmodat2 sys_fchmodat2 sys_fchmodat2tools/perf/arch/x86/entry/syscalls/syscall_64.tbl +2
-1@@ -372,6 +372,8 @@ 448 common process_mrelease sys_process_mrelease 449 common futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node +# 451 reserved for cachestat +452 common fchmodat2 sys_fchmodat2 # # Due to a historical design error, certain syscalls are numbered differently --2.33.8
Re: [PATCH v4 4/5] tools headers UAPI: Sync files changed by new fchmodat2 syscall
Quoted 18 lines · Namhyung Kim
Hello,
Quoted 7 lines · On Tue, Jul 11, 2023 at 9:18 AM Alexey Gladkov <[email protected]> wrote:
From: Palmer Dabbelt <[email protected]>
That add support for this new syscall in tools such as 'perf trace'.
Signed-off-by Palmer Dabbelt <[email protected]>Signed-off-by Alexey Gladkov <[email protected]>Quoted 7 lines
--- tools/include/uapi/asm-generic/unistd.h | 5 ++++- tools/perf/arch/mips/entry/syscalls/syscall_n64.tbl | 2 ++ tools/perf/arch/powerpc/entry/syscalls/syscall.tbl | 2 ++ tools/perf/arch/s390/entry/syscalls/syscall.tbl | 2 ++ tools/perf/arch/x86/entry/syscalls/syscall_64.tbl | 2 ++ 5 files changed, 12 insertions(+), 1 deletion(-)
Quoted 3 lines
It'd be nice if you route this patch separately through the perf tools tree. We can add this after the kernel change is accepted.
Sure. No problem.
Quoted 65 lines
Quoted 64 lines
tools/include/uapi/asm-generic/unistd.h +4
-1@@ -817,8 +817,11 @@ __SYSCALL(__NR_futex_waitv, sys_futex_waitv)#define __NR_set_mempolicy_home_node 450 __SYSCALL(__NR_set_mempolicy_home_node, sys_set_mempolicy_home_node) +#define __NR_fchmodat2 452 +__SYSCALL(__NR_fchmodat2, sys_fchmodat2) +#undef __NR_syscalls -#define __NR_syscalls 451 +#define __NR_syscalls 453/* * 32 bit systems traditionally used different tools/perf/arch/mips/entry/syscalls/syscall_n64.tbl +2
-0@@ -365,3 +365,5 @@ 448 n64 process_mrelease sys_process_mrelease 449 n64 futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node +# 451 reserved for cachestat +452 n64 fchmodat2 sys_fchmodat2tools/perf/arch/powerpc/entry/syscalls/syscall.tbl +2
-0@@ -537,3 +537,5 @@ 448 common process_mrelease sys_process_mrelease 449 common futex_waitv sys_futex_waitv 450 nospu set_mempolicy_home_node sys_set_mempolicy_home_node +# 451 reserved for cachestat +452 common fchmodat2 sys_fchmodat2tools/perf/arch/s390/entry/syscalls/syscall.tbl +2
-0@@ -453,3 +453,5 @@ 448 common process_mrelease sys_process_mrelease sys_process_mrelease 449 common futex_waitv sys_futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node sys_set_mempolicy_home_node +# 451 reserved for cachestat +452 common fchmodat2 sys_fchmodat2 sys_fchmodat2tools/perf/arch/x86/entry/syscalls/syscall_64.tbl +2
-1@@ -372,6 +372,8 @@ 448 common process_mrelease sys_process_mrelease 449 common futex_waitv sys_futex_waitv 450 common set_mempolicy_home_node sys_set_mempolicy_home_node +# 451 reserved for cachestat +452 common fchmodat2 sys_fchmodat2 # # Due to a historical design error, certain syscalls are numbered differently --2.33.8
Signature · 1 line
Rgrds, legion
PATCH v4 5/5 selftests: Add fchmodat2 selftest
In reply to Alexey GladkovThe test marks as skipped if a syscall with the AT_SYMLINK_NOFOLLOW flag fails. This is because not all filesystems support changing the mode bits of symlinks properly. These filesystems return an error but change the mode bits:
newfstatat(4, "regfile", {st_mode=S_IFREG|0640, st_size=0, ...}, AT_SYMLINK_NOFOLLOW) = 0 newfstatat(4, "symlink", {st_mode=S_IFLNK|0777, st_size=7, ...}, AT_SYMLINK_NOFOLLOW) = 0 syscall_0x1c3(0x4, 0x55fa1f244396, 0x180, 0x100, 0x55fa1f24438e, 0x34) = -1 EOPNOTSUPP (Operation not supported) newfstatat(4, "regfile", {st_mode=S_IFREG|0640, st_size=0, ...}, AT_SYMLINK_NOFOLLOW) = 0
This happens with btrfs and xfs:
$ tools/testing/selftests/fchmodat2/fchmodat2_test TAP version 13 1..1 ok 1 # SKIP fchmodat2(symlink) # Totals: pass:0 fail:0 xfail:0 xpass:0 skip:1 error:0
$ stat /tmp/ksft-fchmodat2.*/symlink File: /tmp/ksft-fchmodat2.3NCqlE/symlink -> regfile Size: 7 Blocks: 0 IO Block: 4096 symbolic link Device: 7,0 Inode: 133 Links: 1 Access: (0600/lrw-------) Uid: ( 0/ root) Gid: ( 0/ root)
--- tools/testing/selftests/Makefile | 1 + tools/testing/selftests/fchmodat2/.gitignore | 2 + tools/testing/selftests/fchmodat2/Makefile | 6 + .../selftests/fchmodat2/fchmodat2_test.c | 162 ++++++++++++++++++ 4 files changed, 171 insertions(+) create mode 100644 tools/testing/selftests/fchmodat2/.gitignore create mode 100644 tools/testing/selftests/fchmodat2/Makefile create mode 100644 tools/testing/selftests/fchmodat2/fchmodat2_test.c
tools/testing/selftests/Makefile +1 -0
@@ -18,6 +18,7 @@ TARGETS += drivers/net/bonding TARGETS += drivers/net/team TARGETS += efivarfs TARGETS += exec +TARGETS += fchmodat2 TARGETS += filesystems TARGETS += filesystems/binderfs TARGETS += filesystems/epoll
tools/testing/selftests/fchmodat2/.gitignore new file +2 -0
@@ -0,0 +1,2 @@ +# SPDX-License-Identifier: GPL-2.0-only +/*_test
tools/testing/selftests/fchmodat2/Makefile new file +6 -0
@@ -0,0 +1,6 @@ +# SPDX-License-Identifier: GPL-2.0-or-later + +CFLAGS += -Wall -O2 -g -fsanitize=address -fsanitize=undefined +TEST_GEN_PROGS := fchmodat2_test + +include ../lib.mk
tools/testing/selftests/fchmodat2/fchmodat2_test.c new file +162 -0
162 lines not on this page · raw message
Signature · 1 line
2.33.8
Re: (subset) [PATCH v4 0/5] Add a new fchmodat2() syscall
In reply to Alexey GladkovQuoted 10 lines · Alexey Gladkov
In glibc, the fchmodat(3) function has a flags argument according to the POSIX specification [1], but kernel syscalls has no such argument. Therefore, libc implementations do workarounds using /proc. However, this requires procfs to be mounted and accessible.
This patch set adds
fchmodat2(), a new syscall. The syscall allows to pass theAT_SYMLINK_NOFOLLOWflag to disableLOOKUP_FOLLOW. In all other respects, this syscall is no different fromfchmodat().[...]
Tools updates usually go separately.
Flags argument ported to unsigned int; otherwise unchanged.
---
Applied to the master branch of the vfs/vfs.git tree. Patches in the master branch should appear in linux-next soon.
Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it.
It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated.
Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: master
[1/5] Non-functional cleanup of a "__user * filename"
https://git.kernel.org/vfs/vfs/c/0f05a6af6b7e [2/5] fs: Add fchmodat2() https://git.kernel.org/vfs/vfs/c/8d593559ec09 [3/5] arch: Register fchmodat2, usually as syscall 452 https://git.kernel.org/vfs/vfs/c/2ee63b04f206 [5/5] selftests: Add fchmodat2 selftest https://git.kernel.org/vfs/vfs/c/f175b92081ec
Rich Felker
maintainerRe: [PATCH v4 0/5] Add a new fchmodat2() syscall
In reply to Alexey GladkovQuoted 26 lines · Alexey Gladkov
In glibc, the fchmodat(3) function has a flags argument according to the POSIX specification [1], but kernel syscalls has no such argument. Therefore, libc implementations do workarounds using /proc. However, this requires procfs to be mounted and accessible.
This patch set adds
fchmodat2(), a new syscall. The syscall allows to pass theAT_SYMLINK_NOFOLLOWflag to disableLOOKUP_FOLLOW. In all other respects, this syscall is no different fromfchmodat().[1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/chmod.html
Changes since v3 [[email protected]]:
* Rebased to master because a new syscall has appeared in master.
* Increased__NR_compat_syscallsas pointed out by Arnd Bergmann.
* Syscall renamed fchmodat4 -> fchmodat2 as suggested by Christian Brauner.
* Returneddo_fchmodat4()the original name. We don't need to version internal functions.
* Fixed warnings found by checkpatch.pl.Changes since v2 [[email protected]]:
* Rebased to master.
* Thelookup_flagspassed tosys_fchmodat4as suggested by Al Viro.
* Selftest added.
Quoted 7 lines
Changes since v1 [[email protected]]:
* All architectures are now supported, which support squashed into a single patch.
* Thedo_fchmodat()helper function has been removed, in favor of directly callingdo_fchmodat4().
* The patches are based on 5.2 instead of 5.1.
It's good to see this moving forward. I originally proposed this in a patch submitted in 2020. I suspect implementation details have changed since then, but it might make sense to look back at that discussion if nobody has done so yet (apologies if this was already done and I missed it) to make sure nothing is overlooked -- I remember there were some subtleties with what fs backends might try to do with chmod on symlinks. My proposed commit message also documented a lot of the history of the issue that might be useful to have as context.
https://lore.kernel.org/all/[email protected]/T/
Rich
Add fchmodat2() - or add a more general syscall?
In reply to Alexey GladkovRather than adding a fchmodat2() syscall, should we add a "set_file_attrs()" syscall that takes a mask and allows you to set a bunch of stuff all in one go? Basically, an interface to notify_change() in the kernel that would allow several stats to be set atomically. This might be of particular interest to network filesystems.
David
Re: Add fchmodat2() - or add a more general syscall?
* David Howells:
Quoted 5 lines
Rather than adding a
fchmodat2()syscall, should we add a "set_file_attrs()" syscall that takes a mask and allows you to set a bunch of stuff all in one go? Basically, an interface tonotify_change()in the kernel that would allow several stats to be set atomically. This might be of particular interest to network filesystems.
Do you mean atomically as in compare-and-swap (update only if old values match), or just a way to update multiple file attributes with a single system call?
Thanks,
Florian
Re: Add fchmodat2() - or add a more general syscall?
Quoted 9 lines · Florian Weimer
Quoted 5 lines
Rather than adding a
fchmodat2()syscall, should we add a "set_file_attrs()" syscall that takes a mask and allows you to set a bunch of stuff all in one go? Basically, an interface tonotify_change()in the kernel that would allow several stats to be set atomically. This might be of particular interest to network filesystems.Do you mean atomically as in compare-and-swap (update only if old values match), or just a way to update multiple file attributes with a single system call?
I was thinking more in terms of the latter. AFAIK, there aren't any network filesystems support a CAS interface on file attributes like that. To be able to do a CAS operation, we'd need to pass in the old values as well as the new.
Another thing we could look at is doing "create_and_set_attrs()", possibly allowing it to take a list of xattrs also.
David
Rich Felker
maintainerRe: Add fchmodat2() - or add a more general syscall?
Quoted 12 lines · David Howells
Quoted 9 lines · Florian Weimer
Quoted 5 lines
Rather than adding a
fchmodat2()syscall, should we add a "set_file_attrs()" syscall that takes a mask and allows you to set a bunch of stuff all in one go? Basically, an interface tonotify_change()in the kernel that would allow several stats to be set atomically. This might be of particular interest to network filesystems.Do you mean atomically as in compare-and-swap (update only if old values match), or just a way to update multiple file attributes with a single system call?
Quoted 6 lines
I was thinking more in terms of the latter. AFAIK, there aren't any network filesystems support a CAS interface on file attributes like that. To be able to do a CAS operation, we'd need to pass in the old values as well as the new.
Another thing we could look at is doing "
create_and_set_attrs()", possibly allowing it to take a list of xattrs also.
Can we please not let " hey let's invent a new interface to do something that will be hard for underlying filesystems to even provide and that nothing needs because there's no standard API to do it" be the enemy of "fixing a known problem implementing an existing standard API that just requires a simple, clearly-scoped syscall to do it"?
Rich
Christian Brauner
maintainerRe: Add fchmodat2() - or add a more general syscall?
In reply to David HowellsQuoted 5 lines · David Howells
Quoted 3 lines · Florian Weimer
Quoted 3 lines
Rather than adding a
fchmodat2()syscall, should we add a "set_file_attrs()" syscall that takes a mask and allows you to set a bunch of stuff all in one go? Basically, an interface tonotify_change()in the
That system call would likely be blocked in seccomp sandboxes completely as seccomp cannot filter structs. I don't consider this an argument to block new good functionality in general as that would mean arbitrarily limiting us but it is something to keep in mind. If there's additional benefit other than just being able to set mutliple values at once then yeah might be something to discuss.
Quoted 7 lines
Quoted 6 lines
Quoted 2 lines
kernel that would allow several stats to be set atomically. This might be of particular interest to network filesystems.
Do you mean atomically as in compare-and-swap (update only if old values match), or just a way to update multiple file attributes with a single system call?
Quoted 6 lines
I was thinking more in terms of the latter. AFAIK, there aren't any network filesystems support a CAS interface on file attributes like that. To be able to do a CAS operation, we'd need to pass in the old values as well as the new.
Another thing we could look at is doing "
create_and_set_attrs()", possibly allowing it to take a list of xattrs also.
That would likely require variable sized pointers in a struct which is something we really try to stay away from. I also think it's not a good idea to lump xattrs toegether with generic file attributes. They should remain a separate api imho.
Re: Add fchmodat2() - or add a more general syscall?
In reply to David HowellsQuoted 5 lines · David Howells
Rather than adding a
fchmodat2()syscall, should we add a "set_file_attrs()" syscall that takes a mask and allows you to set a bunch of stuff all in one go? Basically, an interface tonotify_change()in the kernel that would allow several stats to be set atomically. This might be of particular interest to network filesystems.
Presumably looking something like statx(2) (except hopefully with extensible structs this time :P)? I think that could also be useful, but given this is a fairly straight-forward syscall addition (and it also would resolve the AT_EMPTY_PATH issue for chmod as well as simplify the glibc wrapper), I think it makes sense to take this and we can do set_statx(2) separately?
Signature · 4 lines
Aleksa Sarai Senior Software Engineer (Containers) SUSE Linux GmbH <https://www.cyphar.com/>
Re: Add fchmodat2() - or add a more general syscall?
In reply to David HowellsQuoted 8 lines · David Howells
Rather than adding a
fchmodat2()syscall, should we add a "set_file_attrs()" syscall that takes a mask and allows you to set a bunch of stuff all in one go? Basically, an interface tonotify_change()in the kernel that would allow several stats to be set atomically. This might be of particular interest to network filesystems.David
fchmodat2() is a simple addition that fits well with the existing syscalls. It fixes an oversight in fchmodat().
IMO we should just add fchmodat2(), and not get sidetracked by trying to add some super-generalized syscall instead. That can always be done later.
- Eric
Christian Brauner
maintainerRe: Add fchmodat2() - or add a more general syscall?
Quoted 7 lines · Eric Biggers
Quoted 6 lines · David Howells
Rather than adding a
fchmodat2()syscall, should we add a "set_file_attrs()" syscall that takes a mask and allows you to set a bunch of stuff all in one go? Basically, an interface tonotify_change()in the kernel that would allow several stats to be set atomically. This might be of particular interest to network filesystems.
Quoted 8 lines
Quoted 2 lines
David
fchmodat2()is a simple addition that fits well with the existing syscalls. It fixes an oversight infchmodat().IMO we should just add
fchmodat2(), and not get sidetracked by trying to add some super-generalized syscall instead. That can always be done later.
Agreed.