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:
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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
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 [20190717012719.5524-1-palmer@sifive.com]:
* Rebased to master.
* The lookup_flags passed to sys_fchmodat4 as suggested by Al Viro.
* Selftest added.
Changes since v1 [20190531191204.4044-1-palmer@sifive.com]:
* 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
--
2.33.8
From: Palmer Dabbelt <redacted>
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 <redacted>
---
include/linux/syscalls.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
@@ -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
On Tue, Jul 11, 2023, at 13:25, Alexey Gladkov wrote:
From: Palmer Dabbelt <redacted>
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 <redacted>
Signed-off-by: Alexey Gladkov <legion@kernel.org>
In linux-6.5-rc1, number 451 is used for __NR_cachestat, the
next free one at the moment is 452.
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
On Tue, Jul 11, 2023, at 13:25, Alexey Gladkov wrote:
From: Palmer Dabbelt <redacted>
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 <redacted>
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 <arnd@arndb.de>
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 <arnd@arndb.de>
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()
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 <arnd@arndb.de>
The system call itself is useful afaict. But please,
s/fchmodat4/fchmodat2/
Sure. I will.
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()
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 <arnd@arndb.de>
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.
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 <arnd@arndb.de>
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
--
Rgrds, legion
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 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 [cover.1689074739.git.legion@kernel.org]:
* 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 [20190717012719.5524-1-palmer@sifive.com]:
* Rebased to master.
* The lookup_flags passed to sys_fchmodat4 as suggested by Al Viro.
* Selftest added.
Changes since v1 [20190531191204.4044-1-palmer@sifive.com]:
* 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
--
2.33.8
From: Palmer Dabbelt <redacted>
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 <redacted>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
include/linux/syscalls.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
On Tue, Jul 11, 2023, at 18:16, Alexey Gladkov wrote:
From: Palmer Dabbelt <redacted>
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 <redacted>
Signed-off-by: Alexey Gladkov <legion@kernel.org>
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.
@@ -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--
@@ -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--
From: Christian Brauner <brauner@kernel.org> Date: 2023-07-11 17:37:17
On Tue, 11 Jul 2023 18:16:02 +0200, Alexey Gladkov wrote:
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 the AT_SYMLINK_NOFOLLOW flag to disable LOOKUP_FOLLOW. In all other
respects, this syscall is no different from fchmodat().
[...]
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
On Tue, Jul 11, 2023 at 6:25 PM Alexey Gladkov [off-list ref] wrote:
From: Palmer Dabbelt <redacted>
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 <redacted>
Signed-off-by: Alexey Gladkov <legion@kernel.org>
arch/m68k/kernel/syscalls/syscall.tbl | 1 +
Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
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
From: David Howells <dhowells@redhat.com> Date: 2023-07-25 15:59:44
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 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
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 hunk
{
struct path path;
int error;
- unsigned int lookup_flags = LOOKUP_FOLLOW;
+
retry:
error = user_path_at(dfd, filename, lookup_flags, &path);
if (!error) {
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.
On 2023-07-11, Alexey Gladkov [off-list ref] wrote:
From: Palmer Dabbelt <redacted>
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...
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
On 2023-07-25, David Howells [off-list ref] wrote:
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 to notify_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?
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
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).
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.
I'm not sure I understand. Can you explain what you mean?
--
Rgrds, legion
On Tue, Jul 11, 2023 at 06:16:02PM +0200, Alexey Gladkov wrote:
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 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 [cover.1689074739.git.legion@kernel.org]:
* 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 [20190717012719.5524-1-palmer@sifive.com]:
* Rebased to master.
* The lookup_flags passed to sys_fchmodat4 as suggested by Al Viro.
* Selftest added.
Changes since v1 [20190531191204.4044-1-palmer@sifive.com]:
* 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.
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/20200910170256.GK3265@brightrain.aerifal.cx/T/
Rich
From: Eric Biggers <ebiggers@kernel.org> Date: 2023-07-27 03:57:20
On Tue, Jul 25, 2023 at 04:58:34PM +0100, David Howells wrote:
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 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
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
From: David Laight <hidden> Date: 2023-07-27 09:13:34
From: Aleksa Sarai
Sent: 25 July 2023 17:36
...
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.
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
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
From: Christian Brauner <brauner@kernel.org> Date: 2023-07-27 10:28:28
quoted
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).
From: Christian Brauner <brauner@kernel.org> Date: 2023-07-27 10:29:07
On Wed, Jul 26, 2023 at 08:57:10PM -0700, Eric Biggers wrote:
On Tue, Jul 25, 2023 at 04:58:34PM +0100, David Howells wrote:
quoted
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 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
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.
From: Christian Brauner <brauner@kernel.org> Date: 2023-07-27 10:38:24
On Wed, Jul 26, 2023 at 02:43:41AM +1000, Aleksa Sarai wrote:
On 2023-07-11, Alexey Gladkov [off-list ref] wrote:
quoted
From: Palmer Dabbelt <redacted>
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.)
On Thu, Jul 27, 2023 at 09:01:06AM +0000, David Laight wrote:
From: Aleksa Sarai
quoted
Sent: 25 July 2023 17:36
....
quoted
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.
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
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).
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
quoted
{
struct path path;
int error;
- unsigned int lookup_flags = LOOKUP_FOLLOW;
+
retry:
error = user_path_at(dfd, filename, lookup_flags, &path);
if (!error) {
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.
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).)
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
On 2023-07-27, Christian Brauner [off-list ref] wrote:
On Wed, Jul 26, 2023 at 02:43:41AM +1000, Aleksa Sarai wrote:
quoted
On 2023-07-11, Alexey Gladkov [off-list ref] wrote:
quoted
From: Palmer Dabbelt <redacted>
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.)
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).
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
quoted
quoted
{
struct path path;
int error;
- unsigned int lookup_flags = LOOKUP_FOLLOW;
+
retry:
error = user_path_at(dfd, filename, lookup_flags, &path);
if (!error) {
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.
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).)
... 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.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
From: David Laight <hidden> Date: 2023-07-28 08:44:24
...
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.
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)
On Fri, Jul 28, 2023 at 08:43:58AM +0000, David Laight wrote:
....
quoted
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.
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