* Re: [PATCHv3 bpf-next 1/7] uprobe: Wire up uretprobe system call
From: Andrii Nakryiko @ 2024-04-26 17:56 UTC (permalink / raw)
To: Jiri Olsa
Cc: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
linux-kernel, linux-trace-kernel, linux-api, x86, bpf, Song Liu,
Yonghong Song, John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov (AMD), Ingo Molnar, Andy Lutomirski
In-Reply-To: <20240421194206.1010934-2-jolsa@kernel.org>
On Sun, Apr 21, 2024 at 12:42 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Wiring up uretprobe system call, which comes in following changes.
> We need to do the wiring before, because the uretprobe implementation
> needs the syscall number.
>
> Note at the moment uretprobe syscall is supported only for native
> 64-bit process.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> arch/x86/entry/syscalls/syscall_64.tbl | 1 +
> include/linux/syscalls.h | 2 ++
> include/uapi/asm-generic/unistd.h | 5 ++++-
> kernel/sys_ni.c | 2 ++
> 4 files changed, 9 insertions(+), 1 deletion(-)
>
LGTM
Acked-by: Andrii Nakryiko <andrii@kernel.org>
> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
> index 7e8d46f4147f..af0a33ab06ee 100644
> --- a/arch/x86/entry/syscalls/syscall_64.tbl
> +++ b/arch/x86/entry/syscalls/syscall_64.tbl
> @@ -383,6 +383,7 @@
> 459 common lsm_get_self_attr sys_lsm_get_self_attr
> 460 common lsm_set_self_attr sys_lsm_set_self_attr
> 461 common lsm_list_modules sys_lsm_list_modules
> +462 64 uretprobe sys_uretprobe
>
> #
> # Due to a historical design error, certain syscalls are numbered differently
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index e619ac10cd23..5318e0e76799 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -972,6 +972,8 @@ asmlinkage long sys_lsm_list_modules(u64 *ids, u32 *size, u32 flags);
> /* x86 */
> asmlinkage long sys_ioperm(unsigned long from, unsigned long num, int on);
>
> +asmlinkage long sys_uretprobe(void);
> +
> /* pciconfig: alpha, arm, arm64, ia64, sparc */
> asmlinkage long sys_pciconfig_read(unsigned long bus, unsigned long dfn,
> unsigned long off, unsigned long len,
> diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
> index 75f00965ab15..8a747cd1d735 100644
> --- a/include/uapi/asm-generic/unistd.h
> +++ b/include/uapi/asm-generic/unistd.h
> @@ -842,8 +842,11 @@ __SYSCALL(__NR_lsm_set_self_attr, sys_lsm_set_self_attr)
> #define __NR_lsm_list_modules 461
> __SYSCALL(__NR_lsm_list_modules, sys_lsm_list_modules)
>
> +#define __NR_uretprobe 462
> +__SYSCALL(__NR_uretprobe, sys_uretprobe)
> +
> #undef __NR_syscalls
> -#define __NR_syscalls 462
> +#define __NR_syscalls 463
>
> /*
> * 32 bit systems traditionally used different
> diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
> index faad00cce269..be6195e0d078 100644
> --- a/kernel/sys_ni.c
> +++ b/kernel/sys_ni.c
> @@ -391,3 +391,5 @@ COND_SYSCALL(setuid16);
>
> /* restartable sequence */
> COND_SYSCALL(rseq);
> +
> +COND_SYSCALL(uretprobe);
> --
> 2.44.0
>
^ permalink raw reply
* Re: [PATCH v3 2/2] fs/xattr: add *at family syscalls
From: Arnd Bergmann @ 2024-04-26 17:38 UTC (permalink / raw)
To: cgzones
Cc: x86, linux-alpha, linux-kernel, linux-arm-kernel, linux-ia64,
linux-m68k, linux-mips, linux-parisc, linuxppc-dev, linux-s390,
linux-sh, sparclinux, linux-fsdevel, audit, Linux-Arch, linux-api,
linux-security-module, selinux, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Russell King, Catalin Marinas,
Will Deacon, Geert Uytterhoeven, Michal Simek,
Thomas Bogendoerfer, James E . J . Bottomley, Helge Deller,
Michael Ellerman, Nicholas Piggin, Christophe Leroy,
Aneesh Kumar K.V, Naveen N. Rao, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
Yoshinori Sato, Rich Felker, John Paul Adrian Glaubitz,
David S . Miller, Andreas Larsson, Andy Lutomirski,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen,
H. Peter Anvin, Chris Zankel, Max Filippov, Alexander Viro,
Christian Brauner, Jan Kara, Paul Moore, Eric Paris, Jens Axboe,
Pavel Begunkov, Peter Zijlstra, Sohil Mehta, Palmer Dabbelt,
Miklos Szeredi, Nhat Pham, Casey Schaufler, Florian Fainelli,
Kees Cook, Rick Edgecombe, Mark Rutland, io-uring
In-Reply-To: <20240426162042.191916-1-cgoettsche@seltendoof.de>
On Fri, Apr 26, 2024, at 18:20, Christian Göttsche wrote:
> From: Christian Göttsche <cgzones@googlemail.com>
>
> Add the four syscalls setxattrat(), getxattrat(), listxattrat() and
> removexattrat(). Those can be used to operate on extended attributes,
> especially security related ones, either relative to a pinned directory
> or on a file descriptor without read access, avoiding a
> /proc/<pid>/fd/<fd> detour, requiring a mounted procfs.
>
> One use case will be setfiles(8) setting SELinux file contexts
> ("security.selinux") without race conditions and without a file
> descriptor opened with read access requiring SELinux read permission.
>
> Use the do_{name}at() pattern from fs/open.c.
>
> Pass the value of the extended attribute, its length, and for
> setxattrat(2) the command (XATTR_CREATE or XATTR_REPLACE) via an added
> struct xattr_args to not exceed six syscall arguments and not
> merging the AT_* and XATTR_* flags.
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> CC: x86@kernel.org
> CC: linux-alpha@vger.kernel.org
> CC: linux-kernel@vger.kernel.org
> CC: linux-arm-kernel@lists.infradead.org
> CC: linux-ia64@vger.kernel.org
> CC: linux-m68k@lists.linux-m68k.org
> CC: linux-mips@vger.kernel.org
> CC: linux-parisc@vger.kernel.org
> CC: linuxppc-dev@lists.ozlabs.org
> CC: linux-s390@vger.kernel.org
> CC: linux-sh@vger.kernel.org
> CC: sparclinux@vger.kernel.org
> CC: linux-fsdevel@vger.kernel.org
> CC: audit@vger.kernel.org
> CC: linux-arch@vger.kernel.org
> CC: linux-api@vger.kernel.org
> CC: linux-security-module@vger.kernel.org
> CC: selinux@vger.kernel.org
I checked that the syscalls are all well-formed regarding
argument types, number of arguments and (absence of)
compat handling, and that they are wired up correctly
across architectures
I did not look at the actual implementation in detail.
Reviewed-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply
* Re: RFC: Restricting userspace interfaces for CXL fabric management
From: Jonathan Cameron @ 2024-04-26 16:53 UTC (permalink / raw)
To: Dan Williams
Cc: linux-cxl, Sreenivas Bagalkote, Brett Henning, Harold Johnson,
Sumanesh Samanta, linux-kernel, Davidlohr Bueso, Dave Jiang,
Alison Schofield, Vishal Verma, Ira Weiny, linuxarm, linux-api,
Lorenzo Pieralisi, Natu, Mahesh, gregkh
In-Reply-To: <662bd36caae55_a96f2943f@dwillia2-mobl3.amr.corp.intel.com.notmuch>
On Fri, 26 Apr 2024 09:16:44 -0700
Dan Williams <dan.j.williams@intel.com> wrote:
> Jonathan Cameron wrote:
> [..]
> > To give people an incentive to play the standards game we have to
> > provide an alternative. Userspace libraries will provide some incentive
> > to standardize if we have enough vendors (we don't today - so they will
> > do their own libraries), but it is a lot easier to encourage if we
> > exercise control over the interface.
>
> Yes, and I expect you and I are not far off on what can be done
> here.
>
> However, lets cut to a sentiment hanging over this discussion. Referring
> to vendor specific commands:
>
> "CXL spec has them for a reason and they need to be supported."
>
> ...that is an aggressive "vendor specific first" sentiment that
> generates an aggressive "userspace drivers" reaction, because the best
> way to get around community discussions about what ABI makes sense is
> userspace drivers.
>
> Now, if we can step back to where this discussion started, where typical
> Linux collaboration shines, and where I think you and I are more aligned
> than this thread would indicate, is "vendor specific last". Lets
> carefully consider the vendor specific commands that are candidates to
> be de facto cross vendor semantics if not de jure standards.
>
Agreed. I'd go a little further and say I generally have much more warm and
fuzzy feelings when what is a vendor defined command (today) maps to more
or less the same bit of code for a proposed standards ECN.
IP rules prevent us commenting on specific proposals, but there will be
things we review quicker and with a lighter touch vs others where we
ask lots of annoying questions about generality of the feature etc.
Given the effort we are putting in on the kernel side we all want CXL
to succeed and will do our best to encourage activities that make that
more likely. There are other standards bodies available... which may
make more sense for some features.
Command interfaces are not a good place to compete and maintain secrecy.
If vendors want to do that, then they don't get the pony of upstream
support. They get to convince distros to do a custom kernel build for them:
Good luck with that, some of those folk are 'blunt' in their responses to
such requests.
My proposal is we go forward with a bunch of the CXL spec defined commands
to show the 'how' and consider specific proposals for upstream support
of vendor defined commands on a case by case basis (so pretty much
what you say above). Maybe after a few are done we can formalize some
rules of thumb help vendors makes such proposals, though maybe some
will figure out it is a better and longer term solution to do 'standards
first development'.
I think we do need to look at the safety filtering of tunneled
commands but don't see that as a particularly tricky addition -
for the simple non destructive commands at least.
Jonathan
^ permalink raw reply
* [PATCH v3 2/2] fs/xattr: add *at family syscalls
From: Christian Göttsche @ 2024-04-26 16:20 UTC (permalink / raw)
To: cgzones
Cc: x86, linux-alpha, linux-kernel, linux-arm-kernel, linux-ia64,
linux-m68k, linux-mips, linux-parisc, linuxppc-dev, linux-s390,
linux-sh, sparclinux, linux-fsdevel, audit, linux-arch, linux-api,
linux-security-module, selinux, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Russell King, Catalin Marinas,
Will Deacon, Geert Uytterhoeven, Michal Simek,
Thomas Bogendoerfer, James E.J. Bottomley, Helge Deller,
Michael Ellerman, Nicholas Piggin, Christophe Leroy,
Aneesh Kumar K.V, Naveen N. Rao, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
Yoshinori Sato, Rich Felker, John Paul Adrian Glaubitz,
David S. Miller, Andreas Larsson, Andy Lutomirski,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen,
H. Peter Anvin, Chris Zankel, Max Filippov, Alexander Viro,
Christian Brauner, Jan Kara, Paul Moore, Eric Paris,
Arnd Bergmann, Jens Axboe, Pavel Begunkov, Peter Zijlstra,
Sohil Mehta, Palmer Dabbelt, Miklos Szeredi, Nhat Pham,
Casey Schaufler, Florian Fainelli, Kees Cook, Rick Edgecombe,
Mark Rutland, io-uring
From: Christian Göttsche <cgzones@googlemail.com>
Add the four syscalls setxattrat(), getxattrat(), listxattrat() and
removexattrat(). Those can be used to operate on extended attributes,
especially security related ones, either relative to a pinned directory
or on a file descriptor without read access, avoiding a
/proc/<pid>/fd/<fd> detour, requiring a mounted procfs.
One use case will be setfiles(8) setting SELinux file contexts
("security.selinux") without race conditions and without a file
descriptor opened with read access requiring SELinux read permission.
Use the do_{name}at() pattern from fs/open.c.
Pass the value of the extended attribute, its length, and for
setxattrat(2) the command (XATTR_CREATE or XATTR_REPLACE) via an added
struct xattr_args to not exceed six syscall arguments and not
merging the AT_* and XATTR_* flags.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
CC: x86@kernel.org
CC: linux-alpha@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: linux-arm-kernel@lists.infradead.org
CC: linux-ia64@vger.kernel.org
CC: linux-m68k@lists.linux-m68k.org
CC: linux-mips@vger.kernel.org
CC: linux-parisc@vger.kernel.org
CC: linuxppc-dev@lists.ozlabs.org
CC: linux-s390@vger.kernel.org
CC: linux-sh@vger.kernel.org
CC: sparclinux@vger.kernel.org
CC: linux-fsdevel@vger.kernel.org
CC: audit@vger.kernel.org
CC: linux-arch@vger.kernel.org
CC: linux-api@vger.kernel.org
CC: linux-security-module@vger.kernel.org
CC: selinux@vger.kernel.org
---
v3:
- pass value, size and xattr_flags via new struct xattr_args to
split AT_* and XATTR_* flags
v2: https://lore.kernel.org/lkml/20230511150802.737477-1-cgzones@googlemail.com/
- squash syscall introduction and wire up commits
- add AT_XATTR_CREATE and AT_XATTR_REPLACE constants
v1 discussion: https://lore.kernel.org/all/20220830152858.14866-2-cgzones@googlemail.com/
Previous approach ("f*xattr: allow O_PATH descriptors"): https://lore.kernel.org/all/20220607153139.35588-1-cgzones@googlemail.com/
---
arch/alpha/kernel/syscalls/syscall.tbl | 4 +
arch/arm/tools/syscall.tbl | 4 +
arch/arm64/include/asm/unistd.h | 2 +-
arch/arm64/include/asm/unistd32.h | 8 ++
arch/m68k/kernel/syscalls/syscall.tbl | 4 +
arch/microblaze/kernel/syscalls/syscall.tbl | 4 +
arch/mips/kernel/syscalls/syscall_n32.tbl | 4 +
arch/mips/kernel/syscalls/syscall_n64.tbl | 4 +
arch/mips/kernel/syscalls/syscall_o32.tbl | 4 +
arch/parisc/kernel/syscalls/syscall.tbl | 4 +
arch/powerpc/kernel/syscalls/syscall.tbl | 4 +
arch/s390/kernel/syscalls/syscall.tbl | 4 +
arch/sh/kernel/syscalls/syscall.tbl | 4 +
arch/sparc/kernel/syscalls/syscall.tbl | 4 +
arch/x86/entry/syscalls/syscall_32.tbl | 4 +
arch/x86/entry/syscalls/syscall_64.tbl | 4 +
arch/xtensa/kernel/syscalls/syscall.tbl | 4 +
fs/xattr.c | 128 ++++++++++++++++----
include/asm-generic/audit_change_attr.h | 6 +
include/linux/syscalls.h | 10 ++
include/uapi/asm-generic/unistd.h | 12 +-
include/uapi/linux/xattr.h | 6 +
22 files changed, 208 insertions(+), 24 deletions(-)
diff --git a/arch/alpha/kernel/syscalls/syscall.tbl b/arch/alpha/kernel/syscalls/syscall.tbl
index 8ff110826ce2..fdc11249f1b8 100644
--- a/arch/alpha/kernel/syscalls/syscall.tbl
+++ b/arch/alpha/kernel/syscalls/syscall.tbl
@@ -501,3 +501,7 @@
569 common lsm_get_self_attr sys_lsm_get_self_attr
570 common lsm_set_self_attr sys_lsm_set_self_attr
571 common lsm_list_modules sys_lsm_list_modules
+572 common setxattrat sys_setxattrat
+573 common getxattrat sys_getxattrat
+574 common listxattrat sys_listxattrat
+575 common removexattrat sys_removexattrat
diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl
index b6c9e01e14f5..22fbbcd8e2b5 100644
--- a/arch/arm/tools/syscall.tbl
+++ b/arch/arm/tools/syscall.tbl
@@ -475,3 +475,7 @@
459 common lsm_get_self_attr sys_lsm_get_self_attr
460 common lsm_set_self_attr sys_lsm_set_self_attr
461 common lsm_list_modules sys_lsm_list_modules
+462 common setxattrat sys_setxattrat
+463 common getxattrat sys_getxattrat
+464 common listxattrat sys_listxattrat
+465 common removexattrat sys_removexattrat
diff --git a/arch/arm64/include/asm/unistd.h b/arch/arm64/include/asm/unistd.h
index 491b2b9bd553..f3a77719eb05 100644
--- a/arch/arm64/include/asm/unistd.h
+++ b/arch/arm64/include/asm/unistd.h
@@ -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 462
+#define __NR_compat_syscalls 466
#endif
#define __ARCH_WANT_SYS_CLONE
diff --git a/arch/arm64/include/asm/unistd32.h b/arch/arm64/include/asm/unistd32.h
index 7118282d1c79..963c999b8d2e 100644
--- a/arch/arm64/include/asm/unistd32.h
+++ b/arch/arm64/include/asm/unistd32.h
@@ -929,6 +929,14 @@ __SYSCALL(__NR_lsm_get_self_attr, sys_lsm_get_self_attr)
__SYSCALL(__NR_lsm_set_self_attr, sys_lsm_set_self_attr)
#define __NR_lsm_list_modules 461
__SYSCALL(__NR_lsm_list_modules, sys_lsm_list_modules)
+#define __NR_setxattrat 462
+__SYSCALL(__NR_setxattrat, sys_setxattrat)
+#define __NR_getxattrat 463
+__SYSCALL(__NR_getxattrat, sys_getxattrat)
+#define __NR_listxattrat 464
+__SYSCALL(__NR_listxattrat, sys_listxattrat)
+#define __NR_removexattrat 465
+__SYSCALL(__NR_removexattrat, sys_removexattrat)
/*
* Please add new compat syscalls above this comment and update
diff --git a/arch/m68k/kernel/syscalls/syscall.tbl b/arch/m68k/kernel/syscalls/syscall.tbl
index 7fd43fd4c9f2..7e8e2d9c3b81 100644
--- a/arch/m68k/kernel/syscalls/syscall.tbl
+++ b/arch/m68k/kernel/syscalls/syscall.tbl
@@ -461,3 +461,7 @@
459 common lsm_get_self_attr sys_lsm_get_self_attr
460 common lsm_set_self_attr sys_lsm_set_self_attr
461 common lsm_list_modules sys_lsm_list_modules
+462 common setxattrat sys_setxattrat
+463 common getxattrat sys_getxattrat
+464 common listxattrat sys_listxattrat
+465 common removexattrat sys_removexattrat
diff --git a/arch/microblaze/kernel/syscalls/syscall.tbl b/arch/microblaze/kernel/syscalls/syscall.tbl
index b00ab2cabab9..7df7fc7c0528 100644
--- a/arch/microblaze/kernel/syscalls/syscall.tbl
+++ b/arch/microblaze/kernel/syscalls/syscall.tbl
@@ -467,3 +467,7 @@
459 common lsm_get_self_attr sys_lsm_get_self_attr
460 common lsm_set_self_attr sys_lsm_set_self_attr
461 common lsm_list_modules sys_lsm_list_modules
+462 common setxattrat sys_setxattrat
+463 common getxattrat sys_getxattrat
+464 common listxattrat sys_listxattrat
+465 common removexattrat sys_removexattrat
diff --git a/arch/mips/kernel/syscalls/syscall_n32.tbl b/arch/mips/kernel/syscalls/syscall_n32.tbl
index 83cfc9eb6b88..07141274f9ff 100644
--- a/arch/mips/kernel/syscalls/syscall_n32.tbl
+++ b/arch/mips/kernel/syscalls/syscall_n32.tbl
@@ -400,3 +400,7 @@
459 n32 lsm_get_self_attr sys_lsm_get_self_attr
460 n32 lsm_set_self_attr sys_lsm_set_self_attr
461 n32 lsm_list_modules sys_lsm_list_modules
+462 n32 setxattrat sys_setxattrat
+463 n32 getxattrat sys_getxattrat
+464 n32 listxattrat sys_listxattrat
+465 n32 removexattrat sys_removexattrat
diff --git a/arch/mips/kernel/syscalls/syscall_n64.tbl b/arch/mips/kernel/syscalls/syscall_n64.tbl
index 532b855df589..9773412f2812 100644
--- a/arch/mips/kernel/syscalls/syscall_n64.tbl
+++ b/arch/mips/kernel/syscalls/syscall_n64.tbl
@@ -376,3 +376,7 @@
459 n64 lsm_get_self_attr sys_lsm_get_self_attr
460 n64 lsm_set_self_attr sys_lsm_set_self_attr
461 n64 lsm_list_modules sys_lsm_list_modules
+462 n64 setxattrat sys_setxattrat
+463 n64 getxattrat sys_getxattrat
+464 n64 listxattrat sys_listxattrat
+465 n64 removexattrat sys_removexattrat
diff --git a/arch/mips/kernel/syscalls/syscall_o32.tbl b/arch/mips/kernel/syscalls/syscall_o32.tbl
index f45c9530ea93..8b5fec66ec18 100644
--- a/arch/mips/kernel/syscalls/syscall_o32.tbl
+++ b/arch/mips/kernel/syscalls/syscall_o32.tbl
@@ -449,3 +449,7 @@
459 o32 lsm_get_self_attr sys_lsm_get_self_attr
460 o32 lsm_set_self_attr sys_lsm_set_self_attr
461 o32 lsm_list_modules sys_lsm_list_modules
+462 o32 setxattrat sys_setxattrat
+463 o32 getxattrat sys_getxattrat
+464 o32 listxattrat sys_listxattrat
+465 o32 removexattrat sys_removexattrat
diff --git a/arch/parisc/kernel/syscalls/syscall.tbl b/arch/parisc/kernel/syscalls/syscall.tbl
index b236a84c4e12..b6ebcaadd460 100644
--- a/arch/parisc/kernel/syscalls/syscall.tbl
+++ b/arch/parisc/kernel/syscalls/syscall.tbl
@@ -460,3 +460,7 @@
459 common lsm_get_self_attr sys_lsm_get_self_attr
460 common lsm_set_self_attr sys_lsm_set_self_attr
461 common lsm_list_modules sys_lsm_list_modules
+462 common setxattrat sys_setxattrat
+463 common getxattrat sys_getxattrat
+464 common listxattrat sys_listxattrat
+465 common removexattrat sys_removexattrat
diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
index 17173b82ca21..7e522a720e1a 100644
--- a/arch/powerpc/kernel/syscalls/syscall.tbl
+++ b/arch/powerpc/kernel/syscalls/syscall.tbl
@@ -548,3 +548,7 @@
459 common lsm_get_self_attr sys_lsm_get_self_attr
460 common lsm_set_self_attr sys_lsm_set_self_attr
461 common lsm_list_modules sys_lsm_list_modules
+462 common setxattrat sys_setxattrat
+463 common getxattrat sys_getxattrat
+464 common listxattrat sys_listxattrat
+465 common removexattrat sys_removexattrat
diff --git a/arch/s390/kernel/syscalls/syscall.tbl b/arch/s390/kernel/syscalls/syscall.tbl
index 095bb86339a7..71afa1eb35fb 100644
--- a/arch/s390/kernel/syscalls/syscall.tbl
+++ b/arch/s390/kernel/syscalls/syscall.tbl
@@ -464,3 +464,7 @@
459 common lsm_get_self_attr sys_lsm_get_self_attr sys_lsm_get_self_attr
460 common lsm_set_self_attr sys_lsm_set_self_attr sys_lsm_set_self_attr
461 common lsm_list_modules sys_lsm_list_modules sys_lsm_list_modules
+462 common setxattrat sys_setxattrat sys_setxattrat
+463 common getxattrat sys_getxattrat sys_getxattrat
+464 common listxattrat sys_listxattrat sys_listxattrat
+465 common removexattrat sys_removexattrat sys_removexattrat
diff --git a/arch/sh/kernel/syscalls/syscall.tbl b/arch/sh/kernel/syscalls/syscall.tbl
index 86fe269f0220..1fb0ac9f6c58 100644
--- a/arch/sh/kernel/syscalls/syscall.tbl
+++ b/arch/sh/kernel/syscalls/syscall.tbl
@@ -464,3 +464,7 @@
459 common lsm_get_self_attr sys_lsm_get_self_attr
460 common lsm_set_self_attr sys_lsm_set_self_attr
461 common lsm_list_modules sys_lsm_list_modules
+462 common setxattrat sys_setxattrat
+463 common getxattrat sys_getxattrat
+464 common listxattrat sys_listxattrat
+465 common removexattrat sys_removexattrat
diff --git a/arch/sparc/kernel/syscalls/syscall.tbl b/arch/sparc/kernel/syscalls/syscall.tbl
index b23d59313589..bdd90010c8d1 100644
--- a/arch/sparc/kernel/syscalls/syscall.tbl
+++ b/arch/sparc/kernel/syscalls/syscall.tbl
@@ -507,3 +507,7 @@
459 common lsm_get_self_attr sys_lsm_get_self_attr
460 common lsm_set_self_attr sys_lsm_set_self_attr
461 common lsm_list_modules sys_lsm_list_modules
+462 common setxattrat sys_setxattrat
+463 common getxattrat sys_getxattrat
+464 common listxattrat sys_listxattrat
+465 common removexattrat sys_removexattrat
diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
index 5f8591ce7f25..779dd1a9835d 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -466,3 +466,7 @@
459 i386 lsm_get_self_attr sys_lsm_get_self_attr
460 i386 lsm_set_self_attr sys_lsm_set_self_attr
461 i386 lsm_list_modules sys_lsm_list_modules
+462 i386 setxattrat sys_setxattrat
+463 i386 getxattrat sys_getxattrat
+464 i386 listxattrat sys_listxattrat
+465 i386 removexattrat sys_removexattrat
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index 7e8d46f4147f..819c90564f82 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -383,6 +383,10 @@
459 common lsm_get_self_attr sys_lsm_get_self_attr
460 common lsm_set_self_attr sys_lsm_set_self_attr
461 common lsm_list_modules sys_lsm_list_modules
+462 common setxattrat sys_setxattrat
+463 common getxattrat sys_getxattrat
+464 common listxattrat sys_listxattrat
+465 common removexattrat sys_removexattrat
#
# Due to a historical design error, certain syscalls are numbered differently
diff --git a/arch/xtensa/kernel/syscalls/syscall.tbl b/arch/xtensa/kernel/syscalls/syscall.tbl
index dd116598fb25..36bdfe759878 100644
--- a/arch/xtensa/kernel/syscalls/syscall.tbl
+++ b/arch/xtensa/kernel/syscalls/syscall.tbl
@@ -432,3 +432,7 @@
459 common lsm_get_self_attr sys_lsm_get_self_attr
460 common lsm_set_self_attr sys_lsm_set_self_attr
461 common lsm_list_modules sys_lsm_list_modules
+462 common setxattrat sys_setxattrat
+463 common getxattrat sys_getxattrat
+464 common listxattrat sys_listxattrat
+465 common removexattrat sys_removexattrat
diff --git a/fs/xattr.c b/fs/xattr.c
index 941aab719da0..d45e83224a7c 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -655,21 +655,28 @@ setxattr(struct mnt_idmap *idmap, struct dentry *d,
return error;
}
-static int path_setxattr(const char __user *pathname,
+static int do_setxattrat(int dfd, const char __user *pathname, unsigned int at_flags,
const char __user *name, const void __user *value,
- size_t size, int flags, unsigned int lookup_flags)
+ size_t size, int xattr_flags)
{
struct path path;
int error;
+ int lookup_flags;
+ if ((at_flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
+ return -EINVAL;
+
+ lookup_flags = (at_flags & AT_SYMLINK_NOFOLLOW) ? 0 : LOOKUP_FOLLOW;
+ if (at_flags & AT_EMPTY_PATH)
+ lookup_flags |= LOOKUP_EMPTY;
retry:
- error = user_path_at(AT_FDCWD, pathname, lookup_flags, &path);
+ error = user_path_at(dfd, pathname, lookup_flags, &path);
if (error)
return error;
error = mnt_want_write(path.mnt);
if (!error) {
error = setxattr(mnt_idmap(path.mnt), path.dentry, name,
- value, size, flags);
+ value, size, xattr_flags);
mnt_drop_write(path.mnt);
}
path_put(&path);
@@ -680,18 +687,38 @@ static int path_setxattr(const char __user *pathname,
return error;
}
+SYSCALL_DEFINE6(setxattrat, int, dfd, const char __user *, pathname, unsigned int, at_flags,
+ const char __user *, name, const struct xattr_args __user *, uargs,
+ size_t, usize)
+{
+ struct xattr_args args = {};
+ int error;
+
+ if (usize > PAGE_SIZE)
+ return -E2BIG;
+ if (usize < sizeof(args))
+ return -EINVAL;
+
+ error = copy_struct_from_user(&args, sizeof(args), uargs, usize);
+ if (error)
+ return error;
+
+ return do_setxattrat(dfd, pathname, at_flags, name, (const void __user *)args.value,
+ args.size, args.flags);
+}
+
SYSCALL_DEFINE5(setxattr, const char __user *, pathname,
const char __user *, name, const void __user *, value,
size_t, size, int, flags)
{
- return path_setxattr(pathname, name, value, size, flags, LOOKUP_FOLLOW);
+ return do_setxattrat(AT_FDCWD, pathname, 0, name, value, size, flags);
}
SYSCALL_DEFINE5(lsetxattr, const char __user *, pathname,
const char __user *, name, const void __user *, value,
size_t, size, int, flags)
{
- return path_setxattr(pathname, name, value, size, flags, 0);
+ return do_setxattrat(AT_FDCWD, pathname, AT_SYMLINK_NOFOLLOW, name, value, size, flags);
}
SYSCALL_DEFINE5(fsetxattr, int, fd, const char __user *, name,
@@ -774,14 +801,22 @@ getxattr(struct mnt_idmap *idmap, struct dentry *d,
return error;
}
-static ssize_t path_getxattr(const char __user *pathname,
+static ssize_t do_getxattrat(int dfd, const char __user *pathname, unsigned int at_flags,
const char __user *name, void __user *value,
- size_t size, unsigned int lookup_flags)
+ size_t size)
{
struct path path;
ssize_t error;
+ int lookup_flags;
+
+ if ((at_flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
+ return -EINVAL;
+
+ lookup_flags = (at_flags & AT_SYMLINK_NOFOLLOW) ? 0 : LOOKUP_FOLLOW;
+ if (at_flags & AT_EMPTY_PATH)
+ lookup_flags |= LOOKUP_EMPTY;
retry:
- error = user_path_at(AT_FDCWD, pathname, lookup_flags, &path);
+ error = user_path_at(dfd, pathname, lookup_flags, &path);
if (error)
return error;
error = getxattr(mnt_idmap(path.mnt), path.dentry, name, value, size);
@@ -793,16 +828,37 @@ static ssize_t path_getxattr(const char __user *pathname,
return error;
}
+SYSCALL_DEFINE6(getxattrat, int, dfd, const char __user *, pathname, unsigned int, at_flags,
+ const char __user *, name, struct xattr_args __user *, uargs, size_t, usize)
+{
+ struct xattr_args args = {};
+ int error;
+
+ if (usize > PAGE_SIZE)
+ return -E2BIG;
+ if (usize < sizeof(args))
+ return -EINVAL;
+
+ error = copy_struct_from_user(&args, sizeof(args), uargs, usize);
+ if (error)
+ return error;
+
+ if (args.flags != 0)
+ return -EINVAL;
+
+ return do_getxattrat(dfd, pathname, at_flags, name, (void __user *)args.value, args.size);
+}
+
SYSCALL_DEFINE4(getxattr, const char __user *, pathname,
const char __user *, name, void __user *, value, size_t, size)
{
- return path_getxattr(pathname, name, value, size, LOOKUP_FOLLOW);
+ return do_getxattrat(AT_FDCWD, pathname, 0, name, value, size);
}
SYSCALL_DEFINE4(lgetxattr, const char __user *, pathname,
const char __user *, name, void __user *, value, size_t, size)
{
- return path_getxattr(pathname, name, value, size, 0);
+ return do_getxattrat(AT_FDCWD, pathname, AT_SYMLINK_NOFOLLOW, name, value, size);
}
SYSCALL_DEFINE4(fgetxattr, int, fd, const char __user *, name,
@@ -852,13 +908,21 @@ listxattr(struct dentry *d, char __user *list, size_t size)
return error;
}
-static ssize_t path_listxattr(const char __user *pathname, char __user *list,
- size_t size, unsigned int lookup_flags)
+static ssize_t do_listxattrat(int dfd, const char __user *pathname, char __user *list,
+ size_t size, int flags)
{
struct path path;
ssize_t error;
+ int lookup_flags;
+
+ if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
+ return -EINVAL;
+
+ lookup_flags = (flags & AT_SYMLINK_NOFOLLOW) ? 0 : LOOKUP_FOLLOW;
+ if (flags & AT_EMPTY_PATH)
+ lookup_flags |= LOOKUP_EMPTY;
retry:
- error = user_path_at(AT_FDCWD, pathname, lookup_flags, &path);
+ error = user_path_at(dfd, pathname, lookup_flags, &path);
if (error)
return error;
error = listxattr(path.dentry, list, size);
@@ -870,16 +934,22 @@ static ssize_t path_listxattr(const char __user *pathname, char __user *list,
return error;
}
+SYSCALL_DEFINE5(listxattrat, int, dfd, const char __user *, pathname, char __user *, list,
+ size_t, size, int, flags)
+{
+ return do_listxattrat(dfd, pathname, list, size, flags);
+}
+
SYSCALL_DEFINE3(listxattr, const char __user *, pathname, char __user *, list,
size_t, size)
{
- return path_listxattr(pathname, list, size, LOOKUP_FOLLOW);
+ return do_listxattrat(AT_FDCWD, pathname, list, size, 0);
}
SYSCALL_DEFINE3(llistxattr, const char __user *, pathname, char __user *, list,
size_t, size)
{
- return path_listxattr(pathname, list, size, 0);
+ return do_listxattrat(AT_FDCWD, pathname, list, size, AT_SYMLINK_NOFOLLOW);
}
SYSCALL_DEFINE3(flistxattr, int, fd, char __user *, list, size_t, size)
@@ -898,7 +968,7 @@ SYSCALL_DEFINE3(flistxattr, int, fd, char __user *, list, size_t, size)
/*
* Extended attribute REMOVE operations
*/
-static long
+static int
removexattr(struct mnt_idmap *idmap, struct dentry *d,
const char __user *name)
{
@@ -917,13 +987,21 @@ removexattr(struct mnt_idmap *idmap, struct dentry *d,
return vfs_removexattr(idmap, d, kname);
}
-static int path_removexattr(const char __user *pathname,
- const char __user *name, unsigned int lookup_flags)
+static int do_removexattrat(int dfd, const char __user *pathname,
+ const char __user *name, int flags)
{
struct path path;
int error;
+ int lookup_flags;
+
+ if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
+ return -EINVAL;
+
+ lookup_flags = (flags & AT_SYMLINK_NOFOLLOW) ? 0 : LOOKUP_FOLLOW;
+ if (flags & AT_EMPTY_PATH)
+ lookup_flags |= LOOKUP_EMPTY;
retry:
- error = user_path_at(AT_FDCWD, pathname, lookup_flags, &path);
+ error = user_path_at(dfd, pathname, lookup_flags, &path);
if (error)
return error;
error = mnt_want_write(path.mnt);
@@ -939,16 +1017,22 @@ static int path_removexattr(const char __user *pathname,
return error;
}
+SYSCALL_DEFINE4(removexattrat, int, dfd, const char __user *, pathname,
+ const char __user *, name, int, flags)
+{
+ return do_removexattrat(dfd, pathname, name, flags);
+}
+
SYSCALL_DEFINE2(removexattr, const char __user *, pathname,
const char __user *, name)
{
- return path_removexattr(pathname, name, LOOKUP_FOLLOW);
+ return do_removexattrat(AT_FDCWD, pathname, name, 0);
}
SYSCALL_DEFINE2(lremovexattr, const char __user *, pathname,
const char __user *, name)
{
- return path_removexattr(pathname, name, 0);
+ return do_removexattrat(AT_FDCWD, pathname, name, AT_SYMLINK_NOFOLLOW);
}
SYSCALL_DEFINE2(fremovexattr, int, fd, const char __user *, name)
diff --git a/include/asm-generic/audit_change_attr.h b/include/asm-generic/audit_change_attr.h
index 331670807cf0..cc840537885f 100644
--- a/include/asm-generic/audit_change_attr.h
+++ b/include/asm-generic/audit_change_attr.h
@@ -11,9 +11,15 @@ __NR_lchown,
__NR_fchown,
#endif
__NR_setxattr,
+#ifdef __NR_setxattrat
+__NR_setxattrat,
+#endif
__NR_lsetxattr,
__NR_fsetxattr,
__NR_removexattr,
+#ifdef __NR_removexattrat
+__NR_removexattrat,
+#endif
__NR_lremovexattr,
__NR_fremovexattr,
#ifdef __NR_fchownat
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index e619ac10cd23..e06fffc48535 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -338,23 +338,33 @@ asmlinkage long sys_io_uring_register(unsigned int fd, unsigned int op,
void __user *arg, unsigned int nr_args);
asmlinkage long sys_setxattr(const char __user *path, const char __user *name,
const void __user *value, size_t size, int flags);
+asmlinkage long sys_setxattrat(int dfd, const char __user *path, unsigned int at_flags,
+ const char __user *name,
+ const struct xattr_args __user *args, size_t size);
asmlinkage long sys_lsetxattr(const char __user *path, const char __user *name,
const void __user *value, size_t size, int flags);
asmlinkage long sys_fsetxattr(int fd, const char __user *name,
const void __user *value, size_t size, int flags);
asmlinkage long sys_getxattr(const char __user *path, const char __user *name,
void __user *value, size_t size);
+asmlinkage long sys_getxattrat(int dfd, const char __user *path, unsigned int at_flags,
+ const char __user *name,
+ struct xattr_args __user *args, size_t size);
asmlinkage long sys_lgetxattr(const char __user *path, const char __user *name,
void __user *value, size_t size);
asmlinkage long sys_fgetxattr(int fd, const char __user *name,
void __user *value, size_t size);
asmlinkage long sys_listxattr(const char __user *path, char __user *list,
size_t size);
+asmlinkage long sys_listxattrat(int dfd, const char __user *path, char __user *list,
+ size_t size, int flags);
asmlinkage long sys_llistxattr(const char __user *path, char __user *list,
size_t size);
asmlinkage long sys_flistxattr(int fd, char __user *list, size_t size);
asmlinkage long sys_removexattr(const char __user *path,
const char __user *name);
+asmlinkage long sys_removexattrat(int dfd, const char __user *path,
+ const char __user *name, int flags);
asmlinkage long sys_lremovexattr(const char __user *path,
const char __user *name);
asmlinkage long sys_fremovexattr(int fd, const char __user *name);
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index 75f00965ab15..21b275a8dcd6 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -842,8 +842,18 @@ __SYSCALL(__NR_lsm_set_self_attr, sys_lsm_set_self_attr)
#define __NR_lsm_list_modules 461
__SYSCALL(__NR_lsm_list_modules, sys_lsm_list_modules)
+/* fs/xattr.c */
+#define __NR_setxattrat 462
+__SYSCALL(__NR_setxattrat, sys_setxattrat)
+#define __NR_getxattrat 463
+__SYSCALL(__NR_getxattrat, sys_getxattrat)
+#define __NR_listxattrat 464
+__SYSCALL(__NR_listxattrat, sys_listxattrat)
+#define __NR_removexattrat 465
+__SYSCALL(__NR_removexattrat, sys_removexattrat)
+
#undef __NR_syscalls
-#define __NR_syscalls 462
+#define __NR_syscalls 466
/*
* 32 bit systems traditionally used different
diff --git a/include/uapi/linux/xattr.h b/include/uapi/linux/xattr.h
index 9463db2dfa9d..e9ac2acc40c3 100644
--- a/include/uapi/linux/xattr.h
+++ b/include/uapi/linux/xattr.h
@@ -20,6 +20,12 @@
#define XATTR_CREATE 0x1 /* set value, fail if attr already exists */
#define XATTR_REPLACE 0x2 /* set value, fail if attr does not exist */
+
+struct xattr_args {
+ __aligned_u64 __user value;
+ __u32 size;
+ __u32 flags;
+};
#endif
/* Namespaces */
--
2.43.0
^ permalink raw reply related
* Re: RFC: Restricting userspace interfaces for CXL fabric management
From: Dan Williams @ 2024-04-26 16:16 UTC (permalink / raw)
To: Jonathan Cameron, Dan Williams
Cc: linux-cxl, Sreenivas Bagalkote, Brett Henning, Harold Johnson,
Sumanesh Samanta, linux-kernel, Davidlohr Bueso, Dave Jiang,
Alison Schofield, Vishal Verma, Ira Weiny, linuxarm, linux-api,
Lorenzo Pieralisi, Natu, Mahesh, gregkh
In-Reply-To: <20240426094550.00002e37@Huawei.com>
Jonathan Cameron wrote:
[..]
> To give people an incentive to play the standards game we have to
> provide an alternative. Userspace libraries will provide some incentive
> to standardize if we have enough vendors (we don't today - so they will
> do their own libraries), but it is a lot easier to encourage if we
> exercise control over the interface.
Yes, and I expect you and I are not far off on what can be done
here.
However, lets cut to a sentiment hanging over this discussion. Referring
to vendor specific commands:
"CXL spec has them for a reason and they need to be supported."
...that is an aggressive "vendor specific first" sentiment that
generates an aggressive "userspace drivers" reaction, because the best
way to get around community discussions about what ABI makes sense is
userspace drivers.
Now, if we can step back to where this discussion started, where typical
Linux collaboration shines, and where I think you and I are more aligned
than this thread would indicate, is "vendor specific last". Lets
carefully consider the vendor specific commands that are candidates to
be de facto cross vendor semantics if not de jure standards.
^ permalink raw reply
* [PATCH v5 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: Stas Sergeev @ 2024-04-26 13:33 UTC (permalink / raw)
To: linux-kernel
Cc: Stas Sergeev, Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Christian Brauner, Jan Kara, Jeff Layton,
Chuck Lever, Alexander Aring, David Laight, linux-fsdevel,
linux-api, Paolo Bonzini, Christian Göttsche
This patch-set implements the OA2_CRED_INHERIT flag for openat2() syscall.
It is needed to perform an open operation with the creds that were in
effect when the dir_fd was opened, if the dir was opened with O_CRED_ALLOW
flag. This allows the process to pre-open some dirs and switch eUID
(and other UIDs/GIDs) to the less-privileged user, while still retaining
the possibility to open/create files within the pre-opened directory set.
The sand-boxing is security-oriented: symlinks leading outside of a
sand-box are rejected. /proc magic links are rejected. fds opened with
O_CRED_ALLOW are always closed on exec() and cannot be passed via unix
socket.
The more detailed description (including security considerations)
is available in the log messages of individual patches.
Changes in v5:
- rename OA2_INHERIT_CRED to OA2_CRED_INHERIT
- add an "opt-in" flag O_CRED_ALLOW as was suggested by many reviewers
- stop using 64bit types, as suggested by
Christian Brauner <brauner@kernel.org>
- add BUILD_BUG_ON() for VALID_OPENAT2_FLAGS, based on Christian Brauner's
comments
- fixed problems reported by patch-testing bot
- made O_CRED_ALLOW fds not passable via unix sockets and exec(),
based on Christian Brauner's comments
Changes in v4:
- add optimizations suggested by David Laight <David.Laight@ACULAB.COM>
- move security checks to build_open_flags()
- force RESOLVE_NO_MAGICLINKS as suggested by Andy Lutomirski <luto@kernel.org>
Changes in v3:
- partially revert v2 changes to avoid overriding capabilities.
Only the bare minimum is overridden: fsuid, fsgid and group_info.
Document the fact the full cred override is unwanted, as it may
represent an unneeded security risk.
Changes in v2:
- capture full struct cred instead of just fsuid/fsgid.
Suggested by Stefan Metzmacher <metze@samba.org>
CC: Stefan Metzmacher <metze@samba.org>
CC: Eric Biederman <ebiederm@xmission.com>
CC: Alexander Viro <viro@zeniv.linux.org.uk>
CC: Andy Lutomirski <luto@kernel.org>
CC: Christian Brauner <brauner@kernel.org>
CC: Jan Kara <jack@suse.cz>
CC: Jeff Layton <jlayton@kernel.org>
CC: Chuck Lever <chuck.lever@oracle.com>
CC: Alexander Aring <alex.aring@gmail.com>
CC: David Laight <David.Laight@ACULAB.COM>
CC: linux-fsdevel@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: linux-api@vger.kernel.org
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Christian Göttsche <cgzones@googlemail.com>
--
2.44.0
^ permalink raw reply
* Re: [PATCH 2/2] openat2: add OA2_INHERIT_CRED flag
From: stsp @ 2024-04-26 13:36 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-kernel, Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Jan Kara, Jeff Layton, Chuck Lever,
Alexander Aring, David Laight, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche
In-Reply-To: <20240425-akrobatisch-palmen-4c1e7a0f69d2@brauner>
25.04.2024 17:02, Christian Brauner пишет:
>> struct open_flags {
>> - int open_flag;
>> + u64 open_flag;
> Btw, this change taken together with
All fixed in v5.
I dropped u64 use.
Other comments are addressed as well.
Please let me know if I missed some.
Thank you.
^ permalink raw reply
* [PATCH v5 2/3] open: add O_CRED_ALLOW flag
From: Stas Sergeev @ 2024-04-26 13:33 UTC (permalink / raw)
To: linux-kernel
Cc: Stas Sergeev, Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Christian Brauner, Jan Kara, Jeff Layton,
Chuck Lever, Alexander Aring, David Laight, linux-fsdevel,
linux-api, Paolo Bonzini, Christian Göttsche, Arnd Bergmann,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jens Axboe, Kuniyuki Iwashima, Pavel Begunkov, linux-arch, netdev
In-Reply-To: <20240426133310.1159976-1-stsp2@yandex.ru>
This flag prevents an fd from being passed via unix socket, and
makes it to be always closed on exec().
It is needed for the subsequent OA2_CRED_INHERIT addition, to work
as an "opt-in" for the new cred-inherit functionality. Without using
O_CRED_ALLOW when opening dir fd, it won't be possible to use
OA2_CRED_INHERIT on that dir fd.
Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
CC: Eric Biederman <ebiederm@xmission.com>
CC: Alexander Viro <viro@zeniv.linux.org.uk>
CC: Christian Brauner <brauner@kernel.org>
CC: Jan Kara <jack@suse.cz>
CC: Andy Lutomirski <luto@kernel.org>
CC: David Laight <David.Laight@ACULAB.COM>
CC: Arnd Bergmann <arnd@arndb.de>
CC: "David S. Miller" <davem@davemloft.net>
CC: Eric Dumazet <edumazet@google.com>
CC: Jakub Kicinski <kuba@kernel.org>
CC: Paolo Abeni <pabeni@redhat.com>
CC: Jens Axboe <axboe@kernel.dk>
CC: Kuniyuki Iwashima <kuniyu@amazon.com>
CC: Pavel Begunkov <asml.silence@gmail.com>
CC: linux-arch@vger.kernel.org
CC: netdev@vger.kernel.org
CC: linux-fsdevel@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: linux-api@vger.kernel.org
---
fs/fcntl.c | 2 +-
fs/file.c | 15 ++++++++-------
include/linux/fcntl.h | 2 +-
include/uapi/asm-generic/fcntl.h | 4 ++++
net/core/scm.c | 5 +++++
5 files changed, 19 insertions(+), 9 deletions(-)
diff --git a/fs/fcntl.c b/fs/fcntl.c
index 54cc85d3338e..78c96b1293c2 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -1039,7 +1039,7 @@ static int __init fcntl_init(void)
* Exceptions: O_NONBLOCK is a two bit define on parisc; O_NDELAY
* is defined as O_NONBLOCK on some platforms and not on others.
*/
- BUILD_BUG_ON(21 - 1 /* for O_RDONLY being 0 */ !=
+ BUILD_BUG_ON(22 - 1 /* for O_RDONLY being 0 */ !=
HWEIGHT32(
(VALID_OPEN_FLAGS & ~(O_NONBLOCK | O_NDELAY)) |
__FMODE_EXEC | __FMODE_NONOTIFY));
diff --git a/fs/file.c b/fs/file.c
index 3b683b9101d8..2a09d5276676 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -827,22 +827,23 @@ void do_close_on_exec(struct files_struct *files)
/* exec unshares first */
spin_lock(&files->file_lock);
for (i = 0; ; i++) {
+ int j;
unsigned long set;
unsigned fd = i * BITS_PER_LONG;
fdt = files_fdtable(files);
if (fd >= fdt->max_fds)
break;
set = fdt->close_on_exec[i];
- if (!set)
- continue;
fdt->close_on_exec[i] = 0;
- for ( ; set ; fd++, set >>= 1) {
- struct file *file;
- if (!(set & 1))
- continue;
- file = fdt->fd[fd];
+ for (j = 0; j < BITS_PER_LONG; j++, fd++, set >>= 1) {
+ struct file *file = fdt->fd[fd];
if (!file)
continue;
+ /* Close all cred-allow files. */
+ if (file->f_flags & O_CRED_ALLOW)
+ set |= 1;
+ if (!(set & 1))
+ continue;
rcu_assign_pointer(fdt->fd[fd], NULL);
__put_unused_fd(files, fd);
spin_unlock(&files->file_lock);
diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
index a332e79b3207..e074ee9c1e36 100644
--- a/include/linux/fcntl.h
+++ b/include/linux/fcntl.h
@@ -10,7 +10,7 @@
(O_RDONLY | O_WRONLY | O_RDWR | O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC | \
O_APPEND | O_NDELAY | O_NONBLOCK | __O_SYNC | O_DSYNC | \
FASYNC | O_DIRECT | O_LARGEFILE | O_DIRECTORY | O_NOFOLLOW | \
- O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE)
+ O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE | O_CRED_ALLOW)
/* List of all valid flags for the how->resolve argument: */
#define VALID_RESOLVE_FLAGS \
diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index 80f37a0d40d7..ee8c2267c516 100644
--- a/include/uapi/asm-generic/fcntl.h
+++ b/include/uapi/asm-generic/fcntl.h
@@ -89,6 +89,10 @@
#define __O_TMPFILE 020000000
#endif
+#ifndef O_CRED_ALLOW
+#define O_CRED_ALLOW 040000000
+#endif
+
/* a horrid kludge trying to make sure that this will fail on old kernels */
#define O_TMPFILE (__O_TMPFILE | O_DIRECTORY)
diff --git a/net/core/scm.c b/net/core/scm.c
index 9cd4b0a01cd6..f54fb0ee9727 100644
--- a/net/core/scm.c
+++ b/net/core/scm.c
@@ -111,6 +111,11 @@ static int scm_fp_copy(struct cmsghdr *cmsg, struct scm_fp_list **fplp)
fput(file);
return -EINVAL;
}
+ /* don't allow files with creds */
+ if (file->f_flags & O_CRED_ALLOW) {
+ fput(file);
+ return -EPERM;
+ }
if (unix_get_socket(file))
fpl->count_unix++;
--
2.44.0
^ permalink raw reply related
* [PATCH v5 3/3] openat2: add OA2_CRED_INHERIT flag
From: Stas Sergeev @ 2024-04-26 13:33 UTC (permalink / raw)
To: linux-kernel
Cc: Stas Sergeev, Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Christian Brauner, Jan Kara, Jeff Layton,
Chuck Lever, Alexander Aring, David Laight, linux-fsdevel,
linux-api, Paolo Bonzini, Christian Göttsche
In-Reply-To: <20240426133310.1159976-1-stsp2@yandex.ru>
This flag performs the open operation with the fs credentials
(fsuid, fsgid, group_info) that were in effect when dir_fd was opened.
dir_fd must be opened with O_CRED_ALLOW flag for this to work.
This allows the process to pre-open some directories and then
change eUID (and all other UIDs/GIDs) to a less-privileged user,
retaining the ability to open/create files within these directories.
Design goal:
The idea is to provide a very light-weight sandboxing, where the
process, without the use of any heavy-weight techniques like chroot
within namespaces, can restrict the access to the set of pre-opened
directories.
This patch is just a first step to such sandboxing. If things go
well, in the future the same extension can be added to more syscalls.
These should include at least unlinkat(), renameat2() and the
not-yet-upstreamed setxattrat().
Security considerations:
- Only the bare minimal set of credentials is overridden:
fsuid, fsgid and group_info. The rest, for example capabilities,
are not overridden to avoid unneeded security risks.
- To avoid sandboxing escape, this patch makes sure the restricted
lookup modes are used. Namely, RESOLVE_BENEATH or RESOLVE_IN_ROOT.
- Magic /proc symlinks are discarded, as suggested by
Andy Lutomirski <luto@kernel.org>
- O_CRED_ALLOW fds cannot be passed via unix socket and are always
closed on exec() to prevent "unsuspecting userspace" from not being
able to fully drop privs.
Use cases:
Virtual machines that deal with untrusted code, can use that
instead of a more heavy-weighted approaches.
Currently the approach is being tested on a dosemu2 VM.
Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
CC: Stefan Metzmacher <metze@samba.org>
CC: Eric Biederman <ebiederm@xmission.com>
CC: Alexander Viro <viro@zeniv.linux.org.uk>
CC: Andy Lutomirski <luto@kernel.org>
CC: Christian Brauner <brauner@kernel.org>
CC: Jan Kara <jack@suse.cz>
CC: Jeff Layton <jlayton@kernel.org>
CC: Chuck Lever <chuck.lever@oracle.com>
CC: Alexander Aring <alex.aring@gmail.com>
CC: linux-fsdevel@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Christian Göttsche <cgzones@googlemail.com>
---
fs/fcntl.c | 2 ++
fs/namei.c | 56 ++++++++++++++++++++++++++++++++++--
fs/open.c | 10 ++++++-
include/linux/fcntl.h | 2 ++
include/uapi/linux/openat2.h | 2 ++
5 files changed, 69 insertions(+), 3 deletions(-)
diff --git a/fs/fcntl.c b/fs/fcntl.c
index 78c96b1293c2..283c2e65fc2c 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -1043,6 +1043,8 @@ static int __init fcntl_init(void)
HWEIGHT32(
(VALID_OPEN_FLAGS & ~(O_NONBLOCK | O_NDELAY)) |
__FMODE_EXEC | __FMODE_NONOTIFY));
+ BUILD_BUG_ON(HWEIGHT32(VALID_OPENAT2_FLAGS) !=
+ HWEIGHT32(VALID_OPEN_FLAGS) + 1);
fasync_cache = kmem_cache_create("fasync_cache",
sizeof(struct fasync_struct), 0,
diff --git a/fs/namei.c b/fs/namei.c
index dd50345f7260..aa5dcf57851b 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3776,6 +3776,43 @@ static int do_o_path(struct nameidata *nd, unsigned flags, struct file *file)
return error;
}
+static const struct cred *openat2_init_creds(int dfd)
+{
+ struct cred *cred;
+ struct fd f;
+
+ if (dfd == AT_FDCWD)
+ return ERR_PTR(-EINVAL);
+
+ f = fdget_raw(dfd);
+ if (!f.file)
+ return ERR_PTR(-EBADF);
+
+ cred = ERR_PTR(-EPERM);
+ if (!(f.file->f_flags & O_CRED_ALLOW))
+ goto done;
+
+ cred = prepare_creds();
+ if (!cred) {
+ cred = ERR_PTR(-ENOMEM);
+ goto done;
+ }
+
+ cred->fsuid = f.file->f_cred->fsuid;
+ cred->fsgid = f.file->f_cred->fsgid;
+ cred->group_info = get_group_info(f.file->f_cred->group_info);
+
+done:
+ fdput(f);
+ return cred;
+}
+
+static void openat2_done_creds(const struct cred *cred)
+{
+ put_group_info(cred->group_info);
+ put_cred(cred);
+}
+
static struct file *path_openat(struct nameidata *nd,
const struct open_flags *op, unsigned flags)
{
@@ -3793,18 +3830,33 @@ static struct file *path_openat(struct nameidata *nd,
error = do_o_path(nd, flags, file);
} else {
const char *s;
+ const struct cred *old_cred = NULL, *cred = NULL;
- file = alloc_empty_file(open_flags, current_cred());
- if (IS_ERR(file))
+ if (open_flags & OA2_CRED_INHERIT) {
+ cred = openat2_init_creds(nd->dfd);
+ if (IS_ERR(cred))
+ return ERR_CAST(cred);
+ }
+ file = alloc_empty_file(open_flags, cred ?: current_cred());
+ if (IS_ERR(file)) {
+ if (cred)
+ openat2_done_creds(cred);
return file;
+ }
s = path_init(nd, flags);
+ if (cred)
+ old_cred = override_creds(cred);
while (!(error = link_path_walk(s, nd)) &&
(s = open_last_lookups(nd, file, op)) != NULL)
;
if (!error)
error = do_open(nd, file, op);
+ if (old_cred)
+ revert_creds(old_cred);
terminate_walk(nd);
+ if (cred)
+ openat2_done_creds(cred);
}
if (likely(!error)) {
if (likely(file->f_mode & FMODE_OPENED))
diff --git a/fs/open.c b/fs/open.c
index ee8460c83c77..dd4fab536135 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1225,7 +1225,7 @@ inline int build_open_flags(const struct open_how *how, struct open_flags *op)
* values before calling build_open_flags(), but openat2(2) checks all
* of its arguments.
*/
- if (flags & ~VALID_OPEN_FLAGS)
+ if (flags & ~VALID_OPENAT2_FLAGS)
return -EINVAL;
if (how->resolve & ~VALID_RESOLVE_FLAGS)
return -EINVAL;
@@ -1326,6 +1326,14 @@ inline int build_open_flags(const struct open_how *how, struct open_flags *op)
lookup_flags |= LOOKUP_CACHED;
}
+ if (flags & OA2_CRED_INHERIT) {
+ /* Inherit creds only with scoped look-up modes. */
+ if (!(lookup_flags & LOOKUP_IS_SCOPED))
+ return -EPERM;
+ /* Reject /proc "magic" links if inheriting creds. */
+ lookup_flags |= LOOKUP_NO_MAGICLINKS;
+ }
+
op->lookup_flags = lookup_flags;
return 0;
}
diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
index e074ee9c1e36..33b9c7ad056b 100644
--- a/include/linux/fcntl.h
+++ b/include/linux/fcntl.h
@@ -12,6 +12,8 @@
FASYNC | O_DIRECT | O_LARGEFILE | O_DIRECTORY | O_NOFOLLOW | \
O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE | O_CRED_ALLOW)
+#define VALID_OPENAT2_FLAGS (VALID_OPEN_FLAGS | OA2_CRED_INHERIT)
+
/* List of all valid flags for the how->resolve argument: */
#define VALID_RESOLVE_FLAGS \
(RESOLVE_NO_XDEV | RESOLVE_NO_MAGICLINKS | RESOLVE_NO_SYMLINKS | \
diff --git a/include/uapi/linux/openat2.h b/include/uapi/linux/openat2.h
index a5feb7604948..f803558ad62f 100644
--- a/include/uapi/linux/openat2.h
+++ b/include/uapi/linux/openat2.h
@@ -40,4 +40,6 @@ struct open_how {
return -EAGAIN if that's not
possible. */
+#define OA2_CRED_INHERIT (1UL << 28)
+
#endif /* _UAPI_LINUX_OPENAT2_H */
--
2.44.0
^ permalink raw reply related
* [PATCH v5 1/3] fs: reorganize path_openat()
From: Stas Sergeev @ 2024-04-26 13:33 UTC (permalink / raw)
To: linux-kernel
Cc: Stas Sergeev, Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Christian Brauner, Jan Kara, Jeff Layton,
Chuck Lever, Alexander Aring, David Laight, linux-fsdevel,
linux-api, Paolo Bonzini, Christian Göttsche
In-Reply-To: <20240426133310.1159976-1-stsp2@yandex.ru>
This patch moves the call to alloc_empty_file() down the if branches.
That changes is needed for the next patch, which adds a cred override
for alloc_empty_file(). The cred override is only needed in one branch,
i.e. it is not needed for O_PATH and O_TMPFILE..
No functional changes are intended by that patch.
Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
CC: Eric Biederman <ebiederm@xmission.com>
CC: Alexander Viro <viro@zeniv.linux.org.uk>
CC: Christian Brauner <brauner@kernel.org>
CC: Jan Kara <jack@suse.cz>
CC: Andy Lutomirski <luto@kernel.org>
CC: David Laight <David.Laight@ACULAB.COM>
CC: linux-fsdevel@vger.kernel.org
CC: linux-kernel@vger.kernel.org
---
fs/namei.c | 25 ++++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index c5b2a25be7d0..dd50345f7260 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3781,17 +3781,24 @@ static struct file *path_openat(struct nameidata *nd,
{
struct file *file;
int error;
+ int open_flags = op->open_flag;
- file = alloc_empty_file(op->open_flag, current_cred());
- if (IS_ERR(file))
- return file;
-
- if (unlikely(file->f_flags & __O_TMPFILE)) {
- error = do_tmpfile(nd, flags, op, file);
- } else if (unlikely(file->f_flags & O_PATH)) {
- error = do_o_path(nd, flags, file);
+ if (unlikely(open_flags & (__O_TMPFILE | O_PATH))) {
+ file = alloc_empty_file(open_flags, current_cred());
+ if (IS_ERR(file))
+ return file;
+ if (open_flags & __O_TMPFILE)
+ error = do_tmpfile(nd, flags, op, file);
+ else
+ error = do_o_path(nd, flags, file);
} else {
- const char *s = path_init(nd, flags);
+ const char *s;
+
+ file = alloc_empty_file(open_flags, current_cred());
+ if (IS_ERR(file))
+ return file;
+
+ s = path_init(nd, flags);
while (!(error = link_path_walk(s, nd)) &&
(s = open_last_lookups(nd, file, op)) != NULL)
;
--
2.44.0
^ permalink raw reply related
* Re: [RFC PATCH 0/1] Add FUTEX_SPIN operation
From: Christian Brauner @ 2024-04-26 10:26 UTC (permalink / raw)
To: André Almeida
Cc: Mathieu Desnoyers, Peter Zijlstra, Thomas Gleixner, linux-kernel,
Paul E . McKenney, Boqun Feng, H . Peter Anvin, Paul Turner,
linux-api, Florian Weimer, David.Laight, carlos, Peter Oskolkov,
Alexander Mikhalitsyn, Chris Kennelly, Ingo Molnar, Darren Hart,
Davidlohr Bueso, libc-alpha, Steven Rostedt, Jonathan Corbet,
Noah Goldstein, Daniel Colascione, longman, kernel-dev
In-Reply-To: <20240425204332.221162-1-andrealmeid@igalia.com>
On Thu, Apr 25, 2024 at 05:43:31PM -0300, André Almeida wrote:
> Hi,
>
> In the last LPC, Mathieu Desnoyers and I presented[0] a proposal to extend the
> rseq interface to be able to implement spin locks in userspace correctly. Thomas
> Gleixner agreed that this is something that Linux could improve, but asked for
> an alternative proposal first: a futex operation that allows to spin a user
> lock inside the kernel. This patchset implements a prototype of this idea for
> further discussion.
>
> With FUTEX2_SPIN flag set during a futex_wait(), the futex value is expected to
> be the PID of the lock owner. Then, the kernel gets the task_struct of the
> corresponding PID, and checks if it's running. It spins until the futex
> is awaken, the task is scheduled out or if a timeout happens. If the lock owner
> is scheduled out at any time, then the syscall follows the normal path of
> sleeping as usual.
>
> If the futex is awaken and we are spinning, we can return to userspace quickly,
> avoid the scheduling out and in again to wake from a futex_wait(), thus
> speeding up the wait operation.
>
> I didn't manage to find a good mechanism to prevent race conditions between
> setting *futex = PID in userspace and doing find_get_task_by_vpid(PID) in kernel
> space, giving that there's enough room for the original PID owner exit and such
> PID to be relocated to another unrelated task in the system. I didn't performed
One option would be to also allow pidfds. Starting with v6.9 they can be
used to reference individual threads.
So for the really fast case where you have multiple threads and you
somehow may really do care about the impact of the atomic_long_inc() on
pidfd_file->f_count during fdget() (for the single-threaded case the
increment is elided), callers can pass the TID. But in cases where the
inc and put aren't a performance sensitive, you can use pidfds.
So something like the _completely untested_ below:
diff --git a/kernel/futex/waitwake.c b/kernel/futex/waitwake.c
index 94feac92cf4f..b842680aa7e0 100644
--- a/kernel/futex/waitwake.c
+++ b/kernel/futex/waitwake.c
@@ -4,6 +4,9 @@
#include <linux/sched/task.h>
#include <linux/sched/signal.h>
#include <linux/freezer.h>
+#include <linux/cleanup.h>
+#include <linux/file.h>
+#include <uapi/linux/pidfd.h>
#include "futex.h"
@@ -385,19 +388,29 @@ static int futex_spin(struct futex_hash_bucket *hb, struct futex_q *q,
struct hrtimer_sleeper *timeout, void __user *uaddr, u32 val)
{
struct task_struct *p;
- u32 pid, uval;
+ struct pid *pid;
+ u32 pidfd, uval;
unsigned int i = 0;
if (futex_get_value_locked(&uval, uaddr))
return -EFAULT;
- pid = uval;
+ pidfd = uval;
+ CLASS(fd, f)(pidfd);
- p = find_get_task_by_vpid(pid);
- if (!p) {
- printk("%s: no task found with PID %d\n", __func__, pid);
- return -EAGAIN;
- }
+ if (!f.file)
+ return -EBADF;
+
+ pid = pidfd_pid(f.file);
+ if (IS_ERR(pid))
+ return PTR_ERR(pid);
+
+ if (f.file->f_flags & PIDFD_THREAD)
+ p = get_pid_task(pid, PIDTYPE_PID); /* individual thread */
+ else
+ p = get_pid_task(pid, PIDTYPE_TGID); /* thread-group leader */
+ if (!p)
+ return -ESRCH;
if (unlikely(p->flags & PF_KTHREAD)) {
put_task_struct(p);
> benchmarks so far, as I hope to clarify if this interface makes sense prior to
> doing measurements on it.
>
> This implementation has some debug prints to make it easy to inspect what the
> kernel is doing, so you can check if the futex woke during spinning or if
> just slept as the normal path:
>
> [ 6331] futex_spin: spinned 64738 times, sleeping
> [ 6331] futex_spin: woke after 1864606 spins
> [ 6332] futex_spin: woke after 1820906 spins
> [ 6351] futex_spin: spinned 1603293 times, sleeping
> [ 6352] futex_spin: woke after 1848199 spins
>
> [0] https://lpc.events/event/17/contributions/1481/
>
> You can find a small snippet to play with this interface here:
>
> ---
>
> /*
> * futex2_spin example, by André Almeida <andrealmeid@igalia.com>
> *
> * gcc spin.c -o spin
> */
>
> #define _GNU_SOURCE
> #include <err.h>
> #include <errno.h>
> #include <linux/futex.h>
> #include <linux/sched.h>
> #include <pthread.h>
> #include <stdint.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <sys/mman.h>
> #include <unistd.h>
>
> #define __NR_futex_wake 454
> #define __NR_futex_wait 455
>
> #define WAKE_WAIT_US 10000
> #define FUTEX2_SPIN 0x08
> #define STACK_SIZE (1024 * 1024)
>
> #define FUTEX2_SIZE_U32 0x02
> #define FUTEX2_PRIVATE FUTEX_PRIVATE_FLAG
>
> #define timeout_ns 30000000
>
> void *futex;
>
> static inline int futex2_wake(volatile void *uaddr, unsigned long mask, int nr, unsigned int flags)
> {
> return syscall(__NR_futex_wake, uaddr, mask, nr, flags);
> }
>
> static inline int futex2_wait(volatile void *uaddr, unsigned long val, unsigned long mask,
> unsigned int flags, struct timespec *timo, clockid_t clockid)
> {
> return syscall(__NR_futex_wait, uaddr, val, mask, flags, timo, clockid);
> }
>
> void waiter_fn()
> {
> struct timespec to;
> unsigned int flags = FUTEX2_PRIVATE | FUTEX2_SIZE_U32 | FUTEX2_SPIN;
>
> uint32_t child_pid = *(uint32_t *) futex;
>
> clock_gettime(CLOCK_MONOTONIC, &to);
> to.tv_nsec += timeout_ns;
> if (to.tv_nsec >= 1000000000) {
> to.tv_sec++;
> to.tv_nsec -= 1000000000;
> }
>
> printf("waiting on PID %d...\n", child_pid);
> if (futex2_wait(futex, child_pid, ~0U, flags, &to, CLOCK_MONOTONIC))
> printf("waiter failed errno %d\n", errno);
>
> puts("waiting done");
> }
>
> int function(int n)
> {
> return n + n;
> }
>
> #define CHILD_LOOPS 500000
>
> static int child_fn(void *arg)
> {
> int i, n = 2;
>
> for (i = 0; i < CHILD_LOOPS; i++)
> n = function(n);
>
> futex2_wake(futex, ~0U, 1, FUTEX2_SIZE_U32 | FUTEX_PRIVATE_FLAG);
>
> puts("child thread is done");
>
> return 0;
> }
>
> int main() {
> uint32_t child_pid = 0;
> char *stack;
>
> futex = &child_pid;
>
> stack = mmap(NULL, STACK_SIZE, PROT_READ | PROT_WRITE,
> MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0);
>
> if (stack == MAP_FAILED)
> err(EXIT_FAILURE, "mmap");
>
> child_pid = clone(child_fn, stack + STACK_SIZE, CLONE_VM, NULL);
>
> waiter_fn();
>
> usleep(WAKE_WAIT_US * 10);
>
> return 0;
> }
>
> ---
>
> André Almeida (1):
> futex: Add FUTEX_SPIN operation
>
> include/uapi/linux/futex.h | 2 +-
> kernel/futex/futex.h | 6 ++-
> kernel/futex/waitwake.c | 79 +++++++++++++++++++++++++++++++++++++-
> 3 files changed, 83 insertions(+), 4 deletions(-)
>
> --
> 2.44.0
>
^ permalink raw reply related
* Re: [RFC PATCH 0/1] Add FUTEX_SPIN operation
From: Peter Zijlstra @ 2024-04-26 10:14 UTC (permalink / raw)
To: Florian Weimer
Cc: André Almeida, Mathieu Desnoyers, Thomas Gleixner,
linux-kernel, Paul E . McKenney, Boqun Feng, H . Peter Anvin,
Paul Turner, linux-api, Christian Brauner, David.Laight, carlos,
Peter Oskolkov, Alexander Mikhalitsyn, Chris Kennelly,
Ingo Molnar, Darren Hart, Davidlohr Bueso, libc-alpha,
Steven Rostedt, Jonathan Corbet, Noah Goldstein,
Daniel Colascione, longman, kernel-dev
In-Reply-To: <875xw44fxk.fsf@oldenburg.str.redhat.com>
On Fri, Apr 26, 2024 at 11:43:51AM +0200, Florian Weimer wrote:
> * André Almeida:
>
> > With FUTEX2_SPIN flag set during a futex_wait(), the futex value is
> > expected to be the PID of the lock owner. Then, the kernel gets the
> > task_struct of the corresponding PID, and checks if it's running. It
> > spins until the futex is awaken, the task is scheduled out or if a
> > timeout happens. If the lock owner is scheduled out at any time, then
> > the syscall follows the normal path of sleeping as usual.
>
> PID or TID?
TID, just like PI_LOCK I would presume.
> I think we'd like to have at least one, possibly more, bits for free
> use, so the kernel ID comparison should at least mask off the MSB,
> possibly more.
Yeah, it should be using FUTEX_TID_MASK -- just like PI_LOCK :-)
I suppose the question is if this thing should then also imply
FUTEX_WAITERS or not.
^ permalink raw reply
* Re: [RFC PATCH 0/1] Add FUTEX_SPIN operation
From: Florian Weimer @ 2024-04-26 9:43 UTC (permalink / raw)
To: André Almeida
Cc: Mathieu Desnoyers, Peter Zijlstra, Thomas Gleixner, linux-kernel,
Paul E . McKenney, Boqun Feng, H . Peter Anvin, Paul Turner,
linux-api, Christian Brauner, David.Laight, carlos,
Peter Oskolkov, Alexander Mikhalitsyn, Chris Kennelly,
Ingo Molnar, Darren Hart, Davidlohr Bueso, libc-alpha,
Steven Rostedt, Jonathan Corbet, Noah Goldstein,
Daniel Colascione, longman, kernel-dev
In-Reply-To: <20240425204332.221162-1-andrealmeid@igalia.com>
* André Almeida:
> With FUTEX2_SPIN flag set during a futex_wait(), the futex value is
> expected to be the PID of the lock owner. Then, the kernel gets the
> task_struct of the corresponding PID, and checks if it's running. It
> spins until the futex is awaken, the task is scheduled out or if a
> timeout happens. If the lock owner is scheduled out at any time, then
> the syscall follows the normal path of sleeping as usual.
PID or TID?
I think we'd like to have at least one, possibly more, bits for free
use, so the kernel ID comparison should at least mask off the MSB,
possibly more.
I haven't really thought about the proposed locking protocol, sorry.
Thanks,
Florian
^ permalink raw reply
* Re: RFC: Restricting userspace interfaces for CXL fabric management
From: Jonathan Cameron @ 2024-04-26 8:45 UTC (permalink / raw)
To: Dan Williams
Cc: linux-cxl, Sreenivas Bagalkote, Brett Henning, Harold Johnson,
Sumanesh Samanta, linux-kernel, Davidlohr Bueso, Dave Jiang,
Alison Schofield, Vishal Verma, Ira Weiny, linuxarm, linux-api,
Lorenzo Pieralisi, Natu, Mahesh, gregkh
In-Reply-To: <662aae2fe4887_a96f294bf@dwillia2-mobl3.amr.corp.intel.com.notmuch>
On Thu, 25 Apr 2024 12:25:35 -0700
Dan Williams <dan.j.williams@intel.com> wrote:
> Jonathan Cameron wrote:
> > Dan Williams <dan.j.williams@intel.com> wrote:
> > > The minimum requirement for justifying an in kernel driver is that
> > > something else in the kernel consumes that facility. So, again, I want
> > > to get back to specifics what else in the kernel is going to leverage
> > > the Switch CCI mailbox?
> >
> > Why? I've never heard of such as a requirement and numerous drivers
> > provide fairly direct access to hardware. Sometimes there is a subsystem
> > aiding the data formatting etc, but fundamentally that's a convenience.
> >
> > Taking this to a silly level, on this basis all networking drivers would
> > not be in the kernel. They are there mainly to provide userspace access to
> > a network.
>
> Networking is an odd choice to bring into this discussion because that
> subsystem has a long history of wrestling with the "kernel bypass"
> concern. It has largely been able to weather the storm of calls to get
> out of the way and let vendor drivers have free reign.
>
> The AF_XDP socket family was the result of finding a path to let
> userspace networking stacks build functionality without forfeiting the
> relevance and ongoing collaboration on the in-kernel stack.
This first chunk of my reply was all about poking holes in the 'must
have an in kernel user' not trying to make any broader points. That argument
is one bullet of perhaps 20 good reasons for in-kernel support, you've listed
quite a few more so point hopefully made.
I fully agree with the other reasons you've given for why things
are in the kernel, but key here is we don't need to meet the
in-kernel user requirement. Here the focus is on encouraging standards
adoption.
>
> > Any of the hardware access subsystems such hwmon, input, IIO
> > etc are primarily about providing a convenient way to get data to/from
> > a device. They are kernel drivers because that is the cleaner path
> > for data marshaling, interrupt handling etc.
>
> Those are drivers supporting a subsystem to bring a sane kernel
> interface to front potenitally multiple vendor implementations of
> similar functionality.
I have painful memories of early feedback on IIO that was very similar
to arguments here - it wasn't far off killing of the whole effort.
>
> They are not asking for kernel bypass facilities that defeat the purpose
> of ever talking to the kernel community again for potentially
> system-integrity violating functionality behind disparate vendor
> interfaces.
I'm not asking for that at all. This discussion got a bit derailed
though so I can see why you might have taken that impression away.
>
> > In kernel users are a perfectly valid reason to have a kernel driver,
> > but it's far from the only one. None of the AI accelerators have in kernel
> > users today (maybe they will in future). Sure there are other arguments
> > that mean only a few such devices have been upstreamed, but it's not
> > that they need in kernel users. If it's really an issue I'll just submit
> > it to driver/misc and Greg can take a view on whether it's an acceptable
> > device to have driver for... (after he's asked the obvious question of
> > why aren't the CXL folk taking it!) +cc Greg to save providing info later.
>
> AI accelerators are heavy consumers of the core-mm you can not
> reasonably coordinate with the core-mm from userspace.
>
> If the proposal is to build a new CXL Fabric Management subsystem with
> proper ABIs and openly defined command sets that will sit behind thought
> out kernel interfaces then I can get on board with that.
>
> Where I am stuck currently is the assertion that step 1 is "build ioctl
> passthrough tunnels with 'do anything you want and get away with it'
> semantics".
If you think step 1 is the end goal, then indeed I see your problem.
Reality is that this stuff is needed today and similar to type 3 there
will be people who use the raw interface to enabled their custom stuff
- we can enable that but put similar measures in place to to encourage
upstreaming / standarization. I should perhaps not have diverted
into the fact openBMC 'might' patch out the taint. I suspect some
server distros will do that for type 3 devices to, but the advantages
to vendors of playing nicely with upstream still apply. I do however
want to encourage openBMC folk to collaborate on support and if that
means letting them play fast and loose with what they do downstream for
now that's fine - they will not want to carry such patches long term anyway.
The end goal is to enable multiple things:
1) Standardization: That's exactly why I proposed that if broadcomm want
to have upstream support for their interfaces they should do two things:
a) Take a proposal to the consortium to extend the standard definition
to incorporate those features (we want to see them playing the game
so everyone benefits!)
b) Provide clear description of the vendor defined commands so that
the safe ones can be audited and enabled as 'defacto' standards.
We do this with lots of other things - if you are playing the game
nicely we relax the rules a little to encourage you to keep doing so.
This is the path being proposed for the upstream driver.
2) A path sharing that same standard infrastructure for the more destructive
commands. That can have all the protections we want to add.
For the BMC stacks some of that stuff will be vital and it's complex
so there is an argument that not all that belongs in the kernel or at
least not today. One of the other comments I recall for the raw command
support in the type 3 driver was to allow people to test stuff that was
safe, was in the spec, but was not yet audited by the driver. I'd put
a lot of this in that category. We can absolutely do tunneled commands
nicely and filter them for destructive activity etc - it just doesn't
belong in the first patch set.
Ultimately I would expect approaches to enable the destructive commands
as well via standard interfaces, but those will of course need guard
rails / opt in etc.
I never intended to propose that the raw command path is the main one
used - that's there for the same reasons we have one in CXL type3, though
here I suspect the path to supporting everything needed via non
raw interfaces will be longer.
>
> Recall that the current restriction for raw commands was to encourage
> vendor collaboration and building sane kernel interfaces, and that
> distros would enable it in their "debug" kernels to enable hardware
> validation test benches. If the assertion is "that's too restrictive,
> enable a vendor ecosystem based on kernel bypass" that goes too far.
This is a clear description of the concern and it's one I fully share.
My interpretation of your earlier responses was clearly wrong - this
is about avoiding kernel bypass and encouraging standardization.
Your comments on doing user space drivers read to me like you were
telling the switch vendors to go do exactly that and that is why
I was pushing back strongly! All the other responses were based on
me trying to answer other concerns (maintainability for example)
rather than this one.
WRT to relationship to the raw commands: this is the same proposal.
Much like the initial CXL mailbox support provide a raw bypass for
the cases not supported and put the same guards around it (or tighter
ones). That is better in the upstream kernel than pushing the problem
downstream and basically removing any reason at all for switch vendors
to do things right. Give them a path and they will come. So far
what I suspect they are seeing is no path at all - driving fragmentation.
(handy switch vendors in the CC list feel free to say how you are interpretting
this discussion!)
If we'd had upstream support already in place with those restrictions
the rules for switch vendors would have been clear and (assuming they
have some active ecosystem folk) their design would have taken this
into account.
The early switch-cci code proposal was exactly the same as the CXL mailbox.
It had a bunch of commands (IIRC) included get temperature, get timestamp etc
that were safe and handled in exactly the same fashion as the in
kernel interfaces. The reason that I ripped that out was to reduce
the amount and complexity of the first proposal so the focus would lie
on the necessary refactoring of the CXL core to allow for code reuse.
That stuff will comeback in patch set 2 - we can easily put it back in
the first patch set if that helps make the strategy clear - it's just
a few lines of code.
All the infrastructure is still there to support safe commands vs unsafe ones.
Same logic, same approach and actually the same code as for the CXL mailbox.
I fully agree with all that approach.
>
> > For background this is a PCI function with a mailbox used for switch
> > configuration. The mailbox is identical to the one found on CXL type3
> > devices. Whole thing defined in the CXL spec. It gets a little complex
> > because you can tunnel commands to devices connected to the switch,
> > potentially affecting other hosts. Typical Linux device doing this
> > would be a BMC, but there have been repeated questions about providing
> > a subset of access to any Linux system (avoiding the foot guns)
> > Whole thing fully discoverable - proposal is a standard PCI driver.
> >
> > > The generic-Type-3-device mailbox has an in kernel driver because the
> > > kernel has need to send mailbox commands internally and it is
> > > fundamental to RAS and provisioning flows that the kernel have this
> > > coordination. What are the motivations for an in-band Switch CCI command
> > > submission path?
> > >
> > > It could be the case that you have a self-evident example in mind that I
> > > have thus far failed to realize.
> > >
> >
> > There are possibilities, but for now it's a transport driver just like
> > MCTP etc with a well defined chardev interface, with documented ioctl
> > interface etc (which I'd keep inline with one the CXL mailbox uses
> > just to avoid reinventing the wheel - I'd prefer to use that directly
> > to avoid divergence but I don't care that much).
> >
> > As far as I can see, with the security / blast radius concern alleviated
> > by disabling this if lockdown is in use + taint for unaudited commands
> > (and a nasty sounding config similar to the cxl mailbox one)
> > there is little reason not to take such a driver into the kernel.
> > It has next to no maintenance impact outside of itself and a bit of
> > library code which I've proposed pushing down to the level of MMPT
> > (so PCI not CXL) if you think that is necessary.
> >
> > We want interrupt handling and basic access controls / command
> > interface to userspace.
> >
> > Apologies if I'm grumpy - several long days of battling cpu hotplug code.
>
> Again, can we please get back to the specifics of the commands to be
> enabled here? I am open to CXL Fabric Management as a first class
> citizen, I am not currently open to CXL Fabric Management gets to live
> in the corner of the kernel that is unreviewable because all it does is
> take opaque ioctl blobs and marshal them to hardware.
>
That I'm fully on board with. However there are plenty of spec defined
commands that in this safe category and to show how this works we can
implement those in parallel to a discussion about those defined outside
the CXL specification.
Encouraging people to do things via a hacky userspace driver as I read
you as doing earlier in this thread will have given exactly the opposite
impression.
To give people an incentive to play the standards game we have to
provide an alternative. Userspace libraries will provide some incentive
to standardize if we have enough vendors (we don't today - so they will
do their own libraries), but it is a lot easier to encourage if we
exercise control over the interface.
Jonathan
^ permalink raw reply
* [RFC PATCH 1/1] futex: Add FUTEX_SPIN operation
From: André Almeida @ 2024-04-25 20:43 UTC (permalink / raw)
To: Mathieu Desnoyers, Peter Zijlstra, Thomas Gleixner
Cc: linux-kernel, Paul E . McKenney, Boqun Feng, H . Peter Anvin,
Paul Turner, linux-api, Christian Brauner, Florian Weimer,
David.Laight, carlos, Peter Oskolkov, Alexander Mikhalitsyn,
Chris Kennelly, Ingo Molnar, Darren Hart, Davidlohr Bueso,
André Almeida, libc-alpha, Steven Rostedt, Jonathan Corbet,
Noah Goldstein, Daniel Colascione, longman, kernel-dev
In-Reply-To: <20240425204332.221162-1-andrealmeid@igalia.com>
Add a new futex mode for futex wait, the futex spin.
Given the FUTEX2_SPIN flag, parse the futex value as the PID of the lock
owner. Then, before going to the normal wait path, spins while the lock
owner is running in a different CPU, to avoid the whole context switch
operation and to quickly return to userspace. If the lock owner is not
running, just sleep as the normal futex wait path.
The check for the owner to be running or not is important to avoid
spinning for something that won't be released quickly. Userspace is
responsible on providing the proper PID, the kernel does a basic check.
Signed-off-by: André Almeida <andrealmeid@igalia.com>
---
include/uapi/linux/futex.h | 2 +-
kernel/futex/futex.h | 6 ++-
kernel/futex/waitwake.c | 79 +++++++++++++++++++++++++++++++++++++-
3 files changed, 83 insertions(+), 4 deletions(-)
diff --git a/include/uapi/linux/futex.h b/include/uapi/linux/futex.h
index d2ee625ea189..d77d692ffac2 100644
--- a/include/uapi/linux/futex.h
+++ b/include/uapi/linux/futex.h
@@ -63,7 +63,7 @@
#define FUTEX2_SIZE_U32 0x02
#define FUTEX2_SIZE_U64 0x03
#define FUTEX2_NUMA 0x04
- /* 0x08 */
+#define FUTEX2_SPIN 0x08
/* 0x10 */
/* 0x20 */
/* 0x40 */
diff --git a/kernel/futex/futex.h b/kernel/futex/futex.h
index 8b195d06f4e8..180c1c10dc81 100644
--- a/kernel/futex/futex.h
+++ b/kernel/futex/futex.h
@@ -37,6 +37,7 @@
#define FLAGS_HAS_TIMEOUT 0x0040
#define FLAGS_NUMA 0x0080
#define FLAGS_STRICT 0x0100
+#define FLAGS_SPIN 0x0200
/* FUTEX_ to FLAGS_ */
static inline unsigned int futex_to_flags(unsigned int op)
@@ -52,7 +53,7 @@ static inline unsigned int futex_to_flags(unsigned int op)
return flags;
}
-#define FUTEX2_VALID_MASK (FUTEX2_SIZE_MASK | FUTEX2_PRIVATE)
+#define FUTEX2_VALID_MASK (FUTEX2_SIZE_MASK | FUTEX2_PRIVATE | FUTEX2_SPIN)
/* FUTEX2_ to FLAGS_ */
static inline unsigned int futex2_to_flags(unsigned int flags2)
@@ -65,6 +66,9 @@ static inline unsigned int futex2_to_flags(unsigned int flags2)
if (flags2 & FUTEX2_NUMA)
flags |= FLAGS_NUMA;
+ if (flags2 & FUTEX2_SPIN)
+ flags |= FLAGS_SPIN;
+
return flags;
}
diff --git a/kernel/futex/waitwake.c b/kernel/futex/waitwake.c
index 3a10375d9521..94feac92cf4f 100644
--- a/kernel/futex/waitwake.c
+++ b/kernel/futex/waitwake.c
@@ -372,6 +372,78 @@ void futex_wait_queue(struct futex_hash_bucket *hb, struct futex_q *q,
__set_current_state(TASK_RUNNING);
}
+static inline bool task_on_cpu(struct task_struct *p)
+{
+#ifdef CONFIG_SMP
+ return !!(p->on_cpu);
+#else
+ return false;
+#endif
+}
+
+static int futex_spin(struct futex_hash_bucket *hb, struct futex_q *q,
+ struct hrtimer_sleeper *timeout, void __user *uaddr, u32 val)
+{
+ struct task_struct *p;
+ u32 pid, uval;
+ unsigned int i = 0;
+
+ if (futex_get_value_locked(&uval, uaddr))
+ return -EFAULT;
+
+ pid = uval;
+
+ p = find_get_task_by_vpid(pid);
+ if (!p) {
+ printk("%s: no task found with PID %d\n", __func__, pid);
+ return -EAGAIN;
+ }
+
+ if (unlikely(p->flags & PF_KTHREAD)) {
+ put_task_struct(p);
+ printk("%s: can't spin in a kernel task\n", __func__);
+ return -EPERM;
+ }
+
+ futex_queue(q, hb);
+
+ if (timeout)
+ hrtimer_sleeper_start_expires(timeout, HRTIMER_MODE_ABS);
+
+ while (1) {
+ if (likely(!plist_node_empty(&q->list))) {
+ if (timeout && !timeout->task)
+ return 0;
+
+ /* spin */
+ if (task_on_cpu(p)) {
+ i++;
+ continue;
+ /* task is not running, sleep */
+ } else {
+ break;
+ }
+ } else {
+ printk("%s: woke after %d spins\n", __func__, i);
+ return 0;
+ }
+ }
+
+ printk("%s: spinned %d times, sleeping\n", __func__, i);
+
+ /* spinning didn't work, go to the normal path */
+ set_current_state(TASK_INTERRUPTIBLE|TASK_FREEZABLE);
+
+ if (likely(!plist_node_empty(&q->list))) {
+ if (!timeout || timeout->task)
+ schedule();
+ }
+
+ __set_current_state(TASK_RUNNING);
+
+ return 0;
+}
+
/**
* futex_unqueue_multiple - Remove various futexes from their hash bucket
* @v: The list of futexes to unqueue
@@ -665,8 +737,11 @@ int __futex_wait(u32 __user *uaddr, unsigned int flags, u32 val,
if (ret)
return ret;
- /* futex_queue and wait for wakeup, timeout, or a signal. */
- futex_wait_queue(hb, &q, to);
+ if (flags & FLAGS_SPIN)
+ futex_spin(hb, &q, to, uaddr, val);
+ else
+ /* futex_queue and wait for wakeup, timeout, or a signal. */
+ futex_wait_queue(hb, &q, to);
/* If we were woken (and unqueued), we succeeded, whatever. */
if (!futex_unqueue(&q))
--
2.44.0
^ permalink raw reply related
* [RFC PATCH 0/1] Add FUTEX_SPIN operation
From: André Almeida @ 2024-04-25 20:43 UTC (permalink / raw)
To: Mathieu Desnoyers, Peter Zijlstra, Thomas Gleixner
Cc: linux-kernel, Paul E . McKenney, Boqun Feng, H . Peter Anvin,
Paul Turner, linux-api, Christian Brauner, Florian Weimer,
David.Laight, carlos, Peter Oskolkov, Alexander Mikhalitsyn,
Chris Kennelly, Ingo Molnar, Darren Hart, Davidlohr Bueso,
André Almeida, libc-alpha, Steven Rostedt, Jonathan Corbet,
Noah Goldstein, Daniel Colascione, longman, kernel-dev
Hi,
In the last LPC, Mathieu Desnoyers and I presented[0] a proposal to extend the
rseq interface to be able to implement spin locks in userspace correctly. Thomas
Gleixner agreed that this is something that Linux could improve, but asked for
an alternative proposal first: a futex operation that allows to spin a user
lock inside the kernel. This patchset implements a prototype of this idea for
further discussion.
With FUTEX2_SPIN flag set during a futex_wait(), the futex value is expected to
be the PID of the lock owner. Then, the kernel gets the task_struct of the
corresponding PID, and checks if it's running. It spins until the futex
is awaken, the task is scheduled out or if a timeout happens. If the lock owner
is scheduled out at any time, then the syscall follows the normal path of
sleeping as usual.
If the futex is awaken and we are spinning, we can return to userspace quickly,
avoid the scheduling out and in again to wake from a futex_wait(), thus
speeding up the wait operation.
I didn't manage to find a good mechanism to prevent race conditions between
setting *futex = PID in userspace and doing find_get_task_by_vpid(PID) in kernel
space, giving that there's enough room for the original PID owner exit and such
PID to be relocated to another unrelated task in the system. I didn't performed
benchmarks so far, as I hope to clarify if this interface makes sense prior to
doing measurements on it.
This implementation has some debug prints to make it easy to inspect what the
kernel is doing, so you can check if the futex woke during spinning or if
just slept as the normal path:
[ 6331] futex_spin: spinned 64738 times, sleeping
[ 6331] futex_spin: woke after 1864606 spins
[ 6332] futex_spin: woke after 1820906 spins
[ 6351] futex_spin: spinned 1603293 times, sleeping
[ 6352] futex_spin: woke after 1848199 spins
[0] https://lpc.events/event/17/contributions/1481/
You can find a small snippet to play with this interface here:
---
/*
* futex2_spin example, by André Almeida <andrealmeid@igalia.com>
*
* gcc spin.c -o spin
*/
#define _GNU_SOURCE
#include <err.h>
#include <errno.h>
#include <linux/futex.h>
#include <linux/sched.h>
#include <pthread.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/mman.h>
#include <unistd.h>
#define __NR_futex_wake 454
#define __NR_futex_wait 455
#define WAKE_WAIT_US 10000
#define FUTEX2_SPIN 0x08
#define STACK_SIZE (1024 * 1024)
#define FUTEX2_SIZE_U32 0x02
#define FUTEX2_PRIVATE FUTEX_PRIVATE_FLAG
#define timeout_ns 30000000
void *futex;
static inline int futex2_wake(volatile void *uaddr, unsigned long mask, int nr, unsigned int flags)
{
return syscall(__NR_futex_wake, uaddr, mask, nr, flags);
}
static inline int futex2_wait(volatile void *uaddr, unsigned long val, unsigned long mask,
unsigned int flags, struct timespec *timo, clockid_t clockid)
{
return syscall(__NR_futex_wait, uaddr, val, mask, flags, timo, clockid);
}
void waiter_fn()
{
struct timespec to;
unsigned int flags = FUTEX2_PRIVATE | FUTEX2_SIZE_U32 | FUTEX2_SPIN;
uint32_t child_pid = *(uint32_t *) futex;
clock_gettime(CLOCK_MONOTONIC, &to);
to.tv_nsec += timeout_ns;
if (to.tv_nsec >= 1000000000) {
to.tv_sec++;
to.tv_nsec -= 1000000000;
}
printf("waiting on PID %d...\n", child_pid);
if (futex2_wait(futex, child_pid, ~0U, flags, &to, CLOCK_MONOTONIC))
printf("waiter failed errno %d\n", errno);
puts("waiting done");
}
int function(int n)
{
return n + n;
}
#define CHILD_LOOPS 500000
static int child_fn(void *arg)
{
int i, n = 2;
for (i = 0; i < CHILD_LOOPS; i++)
n = function(n);
futex2_wake(futex, ~0U, 1, FUTEX2_SIZE_U32 | FUTEX_PRIVATE_FLAG);
puts("child thread is done");
return 0;
}
int main() {
uint32_t child_pid = 0;
char *stack;
futex = &child_pid;
stack = mmap(NULL, STACK_SIZE, PROT_READ | PROT_WRITE,
MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0);
if (stack == MAP_FAILED)
err(EXIT_FAILURE, "mmap");
child_pid = clone(child_fn, stack + STACK_SIZE, CLONE_VM, NULL);
waiter_fn();
usleep(WAKE_WAIT_US * 10);
return 0;
}
---
André Almeida (1):
futex: Add FUTEX_SPIN operation
include/uapi/linux/futex.h | 2 +-
kernel/futex/futex.h | 6 ++-
kernel/futex/waitwake.c | 79 +++++++++++++++++++++++++++++++++++++-
3 files changed, 83 insertions(+), 4 deletions(-)
--
2.44.0
^ permalink raw reply
* Re: RFC: Restricting userspace interfaces for CXL fabric management
From: Dan Williams @ 2024-04-25 19:25 UTC (permalink / raw)
To: Jonathan Cameron, Dan Williams
Cc: linux-cxl, Sreenivas Bagalkote, Brett Henning, Harold Johnson,
Sumanesh Samanta, linux-kernel, Davidlohr Bueso, Dave Jiang,
Alison Schofield, Vishal Verma, Ira Weiny, linuxarm, linux-api,
Lorenzo Pieralisi, Natu, Mahesh, gregkh
In-Reply-To: <20240425182605.00005f22@Huawei.com>
Jonathan Cameron wrote:
> Dan Williams <dan.j.williams@intel.com> wrote:
> > The minimum requirement for justifying an in kernel driver is that
> > something else in the kernel consumes that facility. So, again, I want
> > to get back to specifics what else in the kernel is going to leverage
> > the Switch CCI mailbox?
>
> Why? I've never heard of such as a requirement and numerous drivers
> provide fairly direct access to hardware. Sometimes there is a subsystem
> aiding the data formatting etc, but fundamentally that's a convenience.
>
> Taking this to a silly level, on this basis all networking drivers would
> not be in the kernel. They are there mainly to provide userspace access to
> a network.
Networking is an odd choice to bring into this discussion because that
subsystem has a long history of wrestling with the "kernel bypass"
concern. It has largely been able to weather the storm of calls to get
out of the way and let vendor drivers have free reign.
The AF_XDP socket family was the result of finding a path to let
userspace networking stacks build functionality without forfeiting the
relevance and ongoing collaboration on the in-kernel stack.
> Any of the hardware access subsystems such hwmon, input, IIO
> etc are primarily about providing a convenient way to get data to/from
> a device. They are kernel drivers because that is the cleaner path
> for data marshaling, interrupt handling etc.
Those are drivers supporting a subsystem to bring a sane kernel
interface to front potenitally multiple vendor implementations of
similar functionality.
They are not asking for kernel bypass facilities that defeat the purpose
of ever talking to the kernel community again for potentially
system-integrity violating functionality behind disparate vendor
interfaces.
> In kernel users are a perfectly valid reason to have a kernel driver,
> but it's far from the only one. None of the AI accelerators have in kernel
> users today (maybe they will in future). Sure there are other arguments
> that mean only a few such devices have been upstreamed, but it's not
> that they need in kernel users. If it's really an issue I'll just submit
> it to driver/misc and Greg can take a view on whether it's an acceptable
> device to have driver for... (after he's asked the obvious question of
> why aren't the CXL folk taking it!) +cc Greg to save providing info later.
AI accelerators are heavy consumers of the core-mm you can not
reasonably coordinate with the core-mm from userspace.
If the proposal is to build a new CXL Fabric Management subsystem with
proper ABIs and openly defined command sets that will sit behind thought
out kernel interfaces then I can get on board with that.
Where I am stuck currently is the assertion that step 1 is "build ioctl
passthrough tunnels with 'do anything you want and get away with it'
semantics".
Recall that the current restriction for raw commands was to encourage
vendor collaboration and building sane kernel interfaces, and that
distros would enable it in their "debug" kernels to enable hardware
validation test benches. If the assertion is "that's too restrictive,
enable a vendor ecosystem based on kernel bypass" that goes too far.
> For background this is a PCI function with a mailbox used for switch
> configuration. The mailbox is identical to the one found on CXL type3
> devices. Whole thing defined in the CXL spec. It gets a little complex
> because you can tunnel commands to devices connected to the switch,
> potentially affecting other hosts. Typical Linux device doing this
> would be a BMC, but there have been repeated questions about providing
> a subset of access to any Linux system (avoiding the foot guns)
> Whole thing fully discoverable - proposal is a standard PCI driver.
>
> > The generic-Type-3-device mailbox has an in kernel driver because the
> > kernel has need to send mailbox commands internally and it is
> > fundamental to RAS and provisioning flows that the kernel have this
> > coordination. What are the motivations for an in-band Switch CCI command
> > submission path?
> >
> > It could be the case that you have a self-evident example in mind that I
> > have thus far failed to realize.
> >
>
> There are possibilities, but for now it's a transport driver just like
> MCTP etc with a well defined chardev interface, with documented ioctl
> interface etc (which I'd keep inline with one the CXL mailbox uses
> just to avoid reinventing the wheel - I'd prefer to use that directly
> to avoid divergence but I don't care that much).
>
> As far as I can see, with the security / blast radius concern alleviated
> by disabling this if lockdown is in use + taint for unaudited commands
> (and a nasty sounding config similar to the cxl mailbox one)
> there is little reason not to take such a driver into the kernel.
> It has next to no maintenance impact outside of itself and a bit of
> library code which I've proposed pushing down to the level of MMPT
> (so PCI not CXL) if you think that is necessary.
>
> We want interrupt handling and basic access controls / command
> interface to userspace.
>
> Apologies if I'm grumpy - several long days of battling cpu hotplug code.
Again, can we please get back to the specifics of the commands to be
enabled here? I am open to CXL Fabric Management as a first class
citizen, I am not currently open to CXL Fabric Management gets to live
in the corner of the kernel that is unreviewable because all it does is
take opaque ioctl blobs and marshal them to hardware.
^ permalink raw reply
* Re: RFC: Restricting userspace interfaces for CXL fabric management
From: Jonathan Cameron @ 2024-04-25 17:26 UTC (permalink / raw)
To: Dan Williams
Cc: linux-cxl, Sreenivas Bagalkote, Brett Henning, Harold Johnson,
Sumanesh Samanta, linux-kernel, Davidlohr Bueso, Dave Jiang,
Alison Schofield, Vishal Verma, Ira Weiny, linuxarm, linux-api,
Lorenzo Pieralisi, Natu, Mahesh, gregkh
In-Reply-To: <662a826dbeeff_b6e02943c@dwillia2-mobl3.amr.corp.intel.com.notmuch>
On Thu, 25 Apr 2024 09:18:53 -0700
Dan Williams <dan.j.williams@intel.com> wrote:
> Jonathan Cameron wrote:
> [..]
> > > Also, the assertion that these kernels will be built with
> > > CONFIG_SECURITY_LOCKDOWN_LSM=n and likely CONFIG_STRICT_DEVMEM=n, then
> > > the entire user-mode driver ABI is available for use. CXL commands are
> > > simple polled mmio, does Linux really benefit from carrying drivers in
> > > the kernel that the kernel itself does not care about?
> >
> > Sure we could it in userspace... It's bad engineering, limits the design
> > to polling only and uses a bunch of interfaces we put a lot of effort into
> > telling people not to use except for debug.
> >
> > I really don't see the advantage in pushing a project/group of projects
> > all of which are picking the upstream kernel up directly, to do a dirty
> > hack. We loose all the advantages of a proper well maintained kernel
> > driver purely on the argument that one use model is not the same as
> > this one. Sensible security lockdown requirements is fine (along
> > with all the other kernel features that must be disable for that
> > to work), making open kernel development on for a large Linux
> > market harder is not.
>
> The minimum requirement for justifying an in kernel driver is that
> something else in the kernel consumes that facility. So, again, I want
> to get back to specifics what else in the kernel is going to leverage
> the Switch CCI mailbox?
Why? I've never heard of such as a requirement and numerous drivers
provide fairly direct access to hardware. Sometimes there is a subsystem
aiding the data formatting etc, but fundamentally that's a convenience.
Taking this to a silly level, on this basis all networking drivers would
not be in the kernel. They are there mainly to provide userspace access to
a network. Any of the hardware access subsystems such hwmon, input, IIO
etc are primarily about providing a convenient way to get data to/from
a device. They are kernel drivers because that is the cleaner path
for data marshaling, interrupt handling etc.
In kernel users are a perfectly valid reason to have a kernel driver,
but it's far from the only one. None of the AI accelerators have in kernel
users today (maybe they will in future). Sure there are other arguments
that mean only a few such devices have been upstreamed, but it's not
that they need in kernel users. If it's really an issue I'll just submit
it to driver/misc and Greg can take a view on whether it's an acceptable
device to have driver for... (after he's asked the obvious question of
why aren't the CXL folk taking it!) +cc Greg to save providing info later.
For background this is a PCI function with a mailbox used for switch
configuration. The mailbox is identical to the one found on CXL type3
devices. Whole thing defined in the CXL spec. It gets a little complex
because you can tunnel commands to devices connected to the switch,
potentially affecting other hosts. Typical Linux device doing this
would be a BMC, but there have been repeated questions about providing
a subset of access to any Linux system (avoiding the foot guns)
Whole thing fully discoverable - proposal is a standard PCI driver.
>
> The generic-Type-3-device mailbox has an in kernel driver because the
> kernel has need to send mailbox commands internally and it is
> fundamental to RAS and provisioning flows that the kernel have this
> coordination. What are the motivations for an in-band Switch CCI command
> submission path?
>
> It could be the case that you have a self-evident example in mind that I
> have thus far failed to realize.
>
There are possibilities, but for now it's a transport driver just like
MCTP etc with a well defined chardev interface, with documented ioctl
interface etc (which I'd keep inline with one the CXL mailbox uses
just to avoid reinventing the wheel - I'd prefer to use that directly
to avoid divergence but I don't care that much).
As far as I can see, with the security / blast radius concern alleviated
by disabling this if lockdown is in use + taint for unaudited commands
(and a nasty sounding config similar to the cxl mailbox one)
there is little reason not to take such a driver into the kernel.
It has next to no maintenance impact outside of itself and a bit of
library code which I've proposed pushing down to the level of MMPT
(so PCI not CXL) if you think that is necessary.
We want interrupt handling and basic access controls / command
interface to userspace.
Apologies if I'm grumpy - several long days of battling cpu hotplug code.
Jonathan
^ permalink raw reply
* Re: RFC: Restricting userspace interfaces for CXL fabric management
From: Dan Williams @ 2024-04-25 16:18 UTC (permalink / raw)
To: Jonathan Cameron, Dan Williams
Cc: linux-cxl, Sreenivas Bagalkote, Brett Henning, Harold Johnson,
Sumanesh Samanta, linux-kernel, Davidlohr Bueso, Dave Jiang,
Alison Schofield, Vishal Verma, Ira Weiny, linuxarm, linux-api,
Lorenzo Pieralisi, Natu, Mahesh
In-Reply-To: <20240425123344.000001a9@Huawei.com>
Jonathan Cameron wrote:
[..]
> > Also, the assertion that these kernels will be built with
> > CONFIG_SECURITY_LOCKDOWN_LSM=n and likely CONFIG_STRICT_DEVMEM=n, then
> > the entire user-mode driver ABI is available for use. CXL commands are
> > simple polled mmio, does Linux really benefit from carrying drivers in
> > the kernel that the kernel itself does not care about?
>
> Sure we could it in userspace... It's bad engineering, limits the design
> to polling only and uses a bunch of interfaces we put a lot of effort into
> telling people not to use except for debug.
>
> I really don't see the advantage in pushing a project/group of projects
> all of which are picking the upstream kernel up directly, to do a dirty
> hack. We loose all the advantages of a proper well maintained kernel
> driver purely on the argument that one use model is not the same as
> this one. Sensible security lockdown requirements is fine (along
> with all the other kernel features that must be disable for that
> to work), making open kernel development on for a large Linux
> market harder is not.
The minimum requirement for justifying an in kernel driver is that
something else in the kernel consumes that facility. So, again, I want
to get back to specifics what else in the kernel is going to leverage
the Switch CCI mailbox?
The generic-Type-3-device mailbox has an in kernel driver because the
kernel has need to send mailbox commands internally and it is
fundamental to RAS and provisioning flows that the kernel have this
coordination. What are the motivations for an in-band Switch CCI command
submission path?
It could be the case that you have a self-evident example in mind that I
have thus far failed to realize.
^ permalink raw reply
* Re: [PATCH 2/2] openat2: add OA2_INHERIT_CRED flag
From: Christian Brauner @ 2024-04-25 14:02 UTC (permalink / raw)
To: Stas Sergeev
Cc: linux-kernel, Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Jan Kara, Jeff Layton, Chuck Lever,
Alexander Aring, David Laight, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche
In-Reply-To: <20240424105248.189032-3-stsp2@yandex.ru>
> struct open_flags {
> - int open_flag;
> + u64 open_flag;
Btw, this change taken together with
> +#define VALID_OPENAT2_FLAGS (VALID_OPEN_FLAGS | OA2_INHERIT_CRED)
is also ripe to causes subtle bugs and security issues. This new
VALID_OPENAT2_FLAGS define bypasses
BUILD_BUG_ON_MSG(upper_32_bits(VALID_OPEN_FLAGS),
"struct open_flags doesn't yet handle flags > 32 bits");
in build_open_flags(). And right now lookup_open(), open_last_lookups(),
and do_open() just do:
int open_flag = op->open_flag;
Because op->open_flag was 32bit that was fine. But now ->open_flag is
64bit which means we truncate the upper 32bit including OA2_INHERIT_CRED
or any other new flag in the upper 32bits in those functions.
So as soon as there's an additional check in e.g., do_open() for
OA2_INHERIT_CRED or in any of the other helpers that's security relevant
we're fscked because that flag is never seen and no bot will help us
here. And it's super easy to miss during review...
^ permalink raw reply
* Re: [PATCH 2/2] openat2: add OA2_INHERIT_CRED flag
From: kernel test robot @ 2024-04-25 13:50 UTC (permalink / raw)
To: Stas Sergeev
Cc: oe-lkp, lkp, Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Christian Brauner, Jan Kara, Jeff Layton,
Chuck Lever, Alexander Aring, Paolo Bonzini,
Christian Göttsche, linux-fsdevel, linux-kernel,
Stas Sergeev, David Laight, linux-api, oliver.sang
In-Reply-To: <20240424105248.189032-3-stsp2@yandex.ru>
Hello,
kernel test robot noticed "BUG:KASAN:wild-memory-access_in_terminate_walk" on:
commit: 97bb54b42b1d6150e9ae11a7bf7833ed9f8c471d ("[PATCH 2/2] openat2: add OA2_INHERIT_CRED flag")
url: https://github.com/intel-lab-lkp/linux/commits/Stas-Sergeev/fs-reorganize-path_openat/20240424-185527
base: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git 9d1ddab261f3e2af7c384dc02238784ce0cf9f98
patch link: https://lore.kernel.org/all/20240424105248.189032-3-stsp2@yandex.ru/
patch subject: [PATCH 2/2] openat2: add OA2_INHERIT_CRED flag
in testcase: boot
compiler: clang-17
test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G
(please refer to attached dmesg/kmsg for entire log/backtrace)
+---------------------------------------------------------------------------------------+------------+------------+
| | 831d3c6cc6 | 97bb54b42b |
+---------------------------------------------------------------------------------------+------------+------------+
| BUG:KASAN:wild-memory-access_in_terminate_walk | 0 | 12 |
| canonical_address#:#[##] | 0 | 12 |
| RIP:terminate_walk | 0 | 12 |
| Kernel_panic-not_syncing:Fatal_exception | 0 | 12 |
+---------------------------------------------------------------------------------------+------------+------------+
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202404252107.3c18eed2-lkp@intel.com
[ 2.555857][ T16] BUG: KASAN: wild-memory-access in terminate_walk (include/linux/instrumented.h:? include/linux/atomic/atomic-instrumented.h:400 include/linux/refcount.h:264 include/linux/refcount.h:307 include/linux/refcount.h:325 fs/namei.c:702)
[ 2.556181][ T16] Write of size 4 at addr aaaaaaaaaaaaaaaa by task kdevtmpfs/16
[ 2.556181][ T16]
[ 2.556181][ T16] CPU: 0 PID: 16 Comm: kdevtmpfs Tainted: G T 6.9.0-rc5-00038-g97bb54b42b1d #1 c90cc2d91176f38ca16e85ead0a72934082854cd
[ 2.556181][ T16] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
[ 2.556181][ T16] Call Trace:
[ 2.556181][ T16] <TASK>
[ 2.556181][ T16] dump_stack_lvl (lib/dump_stack.c:116)
[ 2.556181][ T16] print_report (mm/kasan/report.c:?)
[ 2.556181][ T16] ? kasan_report (mm/kasan/report.c:214 mm/kasan/report.c:590)
[ 2.556181][ T16] ? terminate_walk (include/linux/instrumented.h:? include/linux/atomic/atomic-instrumented.h:400 include/linux/refcount.h:264 include/linux/refcount.h:307 include/linux/refcount.h:325 fs/namei.c:702)
[ 2.556181][ T16] kasan_report (mm/kasan/report.c:603)
[ 2.556181][ T16] ? terminate_walk (include/linux/instrumented.h:? include/linux/atomic/atomic-instrumented.h:400 include/linux/refcount.h:264 include/linux/refcount.h:307 include/linux/refcount.h:325 fs/namei.c:702)
[ 2.556181][ T16] kasan_check_range (mm/kasan/generic.c:?)
[ 2.556181][ T16] terminate_walk (include/linux/instrumented.h:? include/linux/atomic/atomic-instrumented.h:400 include/linux/refcount.h:264 include/linux/refcount.h:307 include/linux/refcount.h:325 fs/namei.c:702)
[ 2.556181][ T16] path_lookupat (fs/namei.c:2515)
[ 2.556181][ T16] filename_lookup (fs/namei.c:2526)
[ 2.556181][ T16] kern_path (fs/namei.c:2634)
[ 2.556181][ T16] init_mount (fs/init.c:22)
[ 2.556181][ T16] devtmpfs_setup (drivers/base/devtmpfs.c:419)
[ 2.556181][ T16] devtmpfsd (drivers/base/devtmpfs.c:436)
[ 2.556181][ T16] kthread (kernel/kthread.c:390)
[ 2.556181][ T16] ? vclkdev_alloc (drivers/base/devtmpfs.c:435)
[ 2.556181][ T16] ? kthread_unuse_mm (kernel/kthread.c:341)
[ 2.556181][ T16] ret_from_fork (arch/x86/kernel/process.c:153)
[ 2.556181][ T16] ? kthread_unuse_mm (kernel/kthread.c:341)
[ 2.556181][ T16] ret_from_fork_asm (arch/x86/entry/entry_64.S:257)
[ 2.556181][ T16] </TASK>
[ 2.556181][ T16] ==================================================================
[ 2.556184][ T16] Disabling lock debugging due to kernel taint
[ 2.556901][ T16] general protection fault, probably for non-canonical address 0xaaaaaaaaaaaaaaaa: 0000 [#1] KASAN PTI
[ 2.558131][ T16] CPU: 0 PID: 16 Comm: kdevtmpfs Tainted: G B T 6.9.0-rc5-00038-g97bb54b42b1d #1 c90cc2d91176f38ca16e85ead0a72934082854cd
[ 2.559653][ T16] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
[ 2.560181][ T16] RIP: 0010:terminate_walk (arch/x86/include/asm/atomic.h:103 include/linux/atomic/atomic-arch-fallback.h:949 include/linux/atomic/atomic-instrumented.h:401 include/linux/refcount.h:264 include/linux/refcount.h:307 include/linux/refcount.h:325 fs/namei.c:702)
[ 2.560181][ T16] Code: 03 43 80 3c 2e 00 74 08 4c 89 ff e8 01 61 f4 ff 49 8b 1f 48 85 db 74 41 48 89 df be 04 00 00 00 e8 dc 61 f4 ff b8 ff ff ff ff <0f> c1 03 83 f8 01 75 25 43 80 3c 2e 00 74 08 4c 89 ff e8 d0 60 f4
All code
========
0: 03 43 80 add -0x80(%rbx),%eax
3: 3c 2e cmp $0x2e,%al
5: 00 74 08 4c add %dh,0x4c(%rax,%rcx,1)
9: 89 ff mov %edi,%edi
b: e8 01 61 f4 ff call 0xfffffffffff46111
10: 49 8b 1f mov (%r15),%rbx
13: 48 85 db test %rbx,%rbx
16: 74 41 je 0x59
18: 48 89 df mov %rbx,%rdi
1b: be 04 00 00 00 mov $0x4,%esi
20: e8 dc 61 f4 ff call 0xfffffffffff46201
25: b8 ff ff ff ff mov $0xffffffff,%eax
2a:* 0f c1 03 xadd %eax,(%rbx) <-- trapping instruction
2d: 83 f8 01 cmp $0x1,%eax
30: 75 25 jne 0x57
32: 43 80 3c 2e 00 cmpb $0x0,(%r14,%r13,1)
37: 74 08 je 0x41
39: 4c 89 ff mov %r15,%rdi
3c: e8 .byte 0xe8
3d: d0 60 f4 shlb -0xc(%rax)
Code starting with the faulting instruction
===========================================
0: 0f c1 03 xadd %eax,(%rbx)
3: 83 f8 01 cmp $0x1,%eax
6: 75 25 jne 0x2d
8: 43 80 3c 2e 00 cmpb $0x0,(%r14,%r13,1)
d: 74 08 je 0x17
f: 4c 89 ff mov %r15,%rdi
12: e8 .byte 0xe8
13: d0 60 f4 shlb -0xc(%rax)
[ 2.560181][ T16] RSP: 0000:ffffc9000010fc40 EFLAGS: 00010246
[ 2.560181][ T16] RAX: 00000000ffffffff RBX: aaaaaaaaaaaaaaaa RCX: ffffffff811e4a0f
[ 2.560181][ T16] RDX: 0000000000000001 RSI: 0000000000000008 RDI: ffffffff8792adc0
[ 2.560181][ T16] RBP: 0000000000000011 R08: ffffffff8792adc7 R09: 1ffffffff0f255b8
[ 2.560181][ T16] R10: dffffc0000000000 R11: fffffbfff0f255b9 R12: 1ffff92000021fc4
[ 2.560181][ T16] R13: dffffc0000000000 R14: 1ffff92000021fc1 R15: ffffc9000010fe08
[ 2.560181][ T16] FS: 0000000000000000(0000) GS:ffffffff878dc000(0000) knlGS:0000000000000000
[ 2.560181][ T16] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 2.560181][ T16] CR2: ffff88843ffff000 CR3: 000000000789c000 CR4: 00000000000406f0
[ 2.560181][ T16] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 2.560181][ T16] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 2.560181][ T16] Call Trace:
[ 2.560181][ T16] <TASK>
[ 2.560181][ T16] ? __die_body (arch/x86/kernel/dumpstack.c:421)
[ 2.560181][ T16] ? die_addr (arch/x86/kernel/dumpstack.c:?)
[ 2.560181][ T16] ? exc_general_protection (arch/x86/kernel/traps.c:?)
[ 2.560181][ T16] ? end_report (arch/x86/include/asm/current.h:49 mm/kasan/report.c:240)
[ 2.560181][ T16] ? asm_exc_general_protection (arch/x86/include/asm/idtentry.h:617)
[ 2.560181][ T16] ? add_taint (arch/x86/include/asm/bitops.h:60 include/asm-generic/bitops/instrumented-atomic.h:29 kernel/panic.c:555)
[ 2.560181][ T16] ? terminate_walk (arch/x86/include/asm/atomic.h:103 include/linux/atomic/atomic-arch-fallback.h:949 include/linux/atomic/atomic-instrumented.h:401 include/linux/refcount.h:264 include/linux/refcount.h:307 include/linux/refcount.h:325 fs/namei.c:702)
[ 2.560181][ T16] path_lookupat (fs/namei.c:2515)
[ 2.560181][ T16] filename_lookup (fs/namei.c:2526)
[ 2.560181][ T16] kern_path (fs/namei.c:2634)
[ 2.560181][ T16] init_mount (fs/init.c:22)
[ 2.560181][ T16] devtmpfs_setup (drivers/base/devtmpfs.c:419)
[ 2.560181][ T16] devtmpfsd (drivers/base/devtmpfs.c:436)
[ 2.560181][ T16] kthread (kernel/kthread.c:390)
[ 2.560181][ T16] ? vclkdev_alloc (drivers/base/devtmpfs.c:435)
[ 2.560181][ T16] ? kthread_unuse_mm (kernel/kthread.c:341)
[ 2.560181][ T16] ret_from_fork (arch/x86/kernel/process.c:153)
[ 2.560181][ T16] ? kthread_unuse_mm (kernel/kthread.c:341)
[ 2.560181][ T16] ret_from_fork_asm (arch/x86/entry/entry_64.S:257)
[ 2.560181][ T16] </TASK>
[ 2.560181][ T16] Modules linked in:
[ 2.560183][ T16] ---[ end trace 0000000000000000 ]---
[ 2.560820][ T16] RIP: 0010:terminate_walk (arch/x86/include/asm/atomic.h:103 include/linux/atomic/atomic-arch-fallback.h:949 include/linux/atomic/atomic-instrumented.h:401 include/linux/refcount.h:264 include/linux/refcount.h:307 include/linux/refcount.h:325 fs/namei.c:702)
[ 2.561462][ T16] Code: 03 43 80 3c 2e 00 74 08 4c 89 ff e8 01 61 f4 ff 49 8b 1f 48 85 db 74 41 48 89 df be 04 00 00 00 e8 dc 61 f4 ff b8 ff ff ff ff <0f> c1 03 83 f8 01 75 25 43 80 3c 2e 00 74 08 4c 89 ff e8 d0 60 f4
All code
========
0: 03 43 80 add -0x80(%rbx),%eax
3: 3c 2e cmp $0x2e,%al
5: 00 74 08 4c add %dh,0x4c(%rax,%rcx,1)
9: 89 ff mov %edi,%edi
b: e8 01 61 f4 ff call 0xfffffffffff46111
10: 49 8b 1f mov (%r15),%rbx
13: 48 85 db test %rbx,%rbx
16: 74 41 je 0x59
18: 48 89 df mov %rbx,%rdi
1b: be 04 00 00 00 mov $0x4,%esi
20: e8 dc 61 f4 ff call 0xfffffffffff46201
25: b8 ff ff ff ff mov $0xffffffff,%eax
2a:* 0f c1 03 xadd %eax,(%rbx) <-- trapping instruction
2d: 83 f8 01 cmp $0x1,%eax
30: 75 25 jne 0x57
32: 43 80 3c 2e 00 cmpb $0x0,(%r14,%r13,1)
37: 74 08 je 0x41
39: 4c 89 ff mov %r15,%rdi
3c: e8 .byte 0xe8
3d: d0 60 f4 shlb -0xc(%rax)
Code starting with the faulting instruction
===========================================
0: 0f c1 03 xadd %eax,(%rbx)
3: 83 f8 01 cmp $0x1,%eax
6: 75 25 jne 0x2d
8: 43 80 3c 2e 00 cmpb $0x0,(%r14,%r13,1)
d: 74 08 je 0x17
f: 4c 89 ff mov %r15,%rdi
12: e8 .byte 0xe8
13: d0 60 f4 shlb -0xc(%rax)
The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20240425/202404252107.3c18eed2-lkp@intel.com
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH v4 0/2] implement OA2_INHERIT_CRED flag for openat2()
From: stsp @ 2024-04-25 12:39 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-kernel, Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Jan Kara, Jeff Layton, Chuck Lever,
Alexander Aring, David Laight, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche
In-Reply-To: <20240425-loslassen-hexen-a1664a579ea1@brauner>
25.04.2024 15:08, Christian Brauner пишет:
>> But I am sure you still don't understand
>> what exactly the patch does, so why not
>> to ask instead of asserting?
>> You say uid/gid can be stolen, but no,
>> it can't: the creds are reverted. Only
>> fsuid/fsgid (and caps in v2 of the patch)
>> actually affect openat2(), but nothing is
>> "leaked" after openat2() finished.
> I say "stolen" because the original opener has no say in this.
The initial idea was to keep this all
within a single-process boundary. It
wasn't coded that way though. :(
> You're
> taking their fsuid/fsgid and groups and overriding creds for the
> duration of the lookup and open. Something the original opener never
> consented to. But let's call it "borrowed" if you're hung up on
> terminology here.
Not a terminology: you were explicitly
talking about uid/gid, blaming a v2 of
my patch. But v2 was not any more
harmful than others and uid/gid cannot
be leaked even there.
But I don't mind: if now you realize v2
is not a leak for uid/gid, then we are on
the same track.
> But ultimately it's the same complaint: the original opener has no way
> of controlling this behavior.
It wasn't clear if the opener should
control that behaviour, or maybe instead
that all should be limited within a single
process?
Andy Lutomirski's explanation made it
clear that even if the openers are the
same, the control is still needed. So now
this argument is undeniable.
> Once ignored in my first reply, and now
> again conveniently cut off again. Let alone all the other objections.
Sorry but your complains were about
stealing uid/gid in v2, which were clearly
wrong. But this is a matter of the past.
> And fwiw, the same way you somehow feel like I haven't read your patch
> it seems you to consistently underestimate the implications of this
> change. Which is really strange because it's pretty obvious. It's
> effectively temporary setuid/setgid with some light limitations.
Temporary cred override is quite common
within the current code AFAICS when I grep
it for override_creds() call.
> Leaking directory descriptors across security boundaries becomes a lot
> more interesting with this patch applied. Something which has happened
> multiple times already and heavily figures in container escapes. And the
> RESOLVE_BENEATH/IN_ROOT restrictions only stave off symlink (and magic
> link) attacks. If a directory fd is leaked a container can take the
> fsuid/fsgid/groups from the original opener of that directory and write
> to disk with whatever it resolves to in that namespace's idmapping. It's
> basically a nice way to puzzle together arbitrary fsuid/fsgid and
> idmapping pairs in whatever namespace the opener happens to be in.
Yes, so the opt-in flag is undeniably needed.
> And to the "unsuspecting userspace" point you dismissed earlier.
> Providing a dirfd to a lesser privileged process isn't horrendously
> dangerous right now. But with this change it sure is. For stuff like
> libpathrs or systemd's fdstore this change has immediate security
> implications.
So am I getting your point correctly:
- process A uses the opt-in flag (eg O_CAPTURE_CREDS)
and passes the fd to "unsuspecting userspace" B.
- process B is not going to use O_INHERIT_CREDS,
but it now still can't drop his privs fully.
Is this what you mean?
^ permalink raw reply
* Re: [PATCH v4 0/2] implement OA2_INHERIT_CRED flag for openat2()
From: Christian Brauner @ 2024-04-25 12:08 UTC (permalink / raw)
To: stsp
Cc: linux-kernel, Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Jan Kara, Jeff Layton, Chuck Lever,
Alexander Aring, David Laight, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche
In-Reply-To: <df51f2fd-385a-47bf-a072-a8988a801d52@yandex.ru>
> But I am sure you still don't understand
> what exactly the patch does, so why not
> to ask instead of asserting?
> You say uid/gid can be stolen, but no,
> it can't: the creds are reverted. Only
> fsuid/fsgid (and caps in v2 of the patch)
> actually affect openat2(), but nothing is
> "leaked" after openat2() finished.
I say "stolen" because the original opener has no say in this. You're
taking their fsuid/fsgid and groups and overriding creds for the
duration of the lookup and open. Something the original opener never
consented to. But let's call it "borrowed" if you're hung up on
terminology here.
But ultimately it's the same complaint: the original opener has no way
of controlling this behavior. Once ignored in my first reply, and now
again conveniently cut off again. Let alone all the other objections.
And fwiw, the same way you somehow feel like I haven't read your patch
it seems you to consistently underestimate the implications of this
change. Which is really strange because it's pretty obvious. It's
effectively temporary setuid/setgid with some light limitations.
Leaking directory descriptors across security boundaries becomes a lot
more interesting with this patch applied. Something which has happened
multiple times already and heavily figures in container escapes. And the
RESOLVE_BENEATH/IN_ROOT restrictions only stave off symlink (and magic
link) attacks. If a directory fd is leaked a container can take the
fsuid/fsgid/groups from the original opener of that directory and write
to disk with whatever it resolves to in that namespace's idmapping. It's
basically a nice way to puzzle together arbitrary fsuid/fsgid and
idmapping pairs in whatever namespace the opener happens to be in.
And again it also messes with LSMs because you're changing credentials
behind their back.
And to the "unsuspecting userspace" point you dismissed earlier.
Providing a dirfd to a lesser privileged process isn't horrendously
dangerous right now. But with this change it sure is. For stuff like
libpathrs or systemd's fdstore this change has immediate security
implications.
^ permalink raw reply
* Re: [PATCH v5 2/3] VT: Add KDFONTINFO ioctl
From: Helge Deller @ 2024-04-25 11:35 UTC (permalink / raw)
To: Alexey Gladkov
Cc: Greg Kroah-Hartman, Jiri Slaby, LKML, kbd, linux-api, linux-fbdev,
linux-serial
In-Reply-To: <Zio5JfRBvzxuVUbX@example.org>
On 4/25/24 13:06, Alexey Gladkov wrote:
> On Thu, Apr 25, 2024 at 12:33:28PM +0200, Helge Deller wrote:
>>>>> diff --git a/include/uapi/linux/kd.h b/include/uapi/linux/kd.h
>>>>> index 8ddb2219a84b..68b715ad4d5c 100644
>>>>> --- a/include/uapi/linux/kd.h
>>>>> +++ b/include/uapi/linux/kd.h
>>>>> @@ -185,6 +185,20 @@ struct console_font {
>>>>>
>>>>> #define KD_FONT_FLAG_DONT_RECALC 1 /* Don't recalculate hw charcell size [compat] */
>>>>>
>>>>> +/* font information */
>>>>> +
>>>>> +#define KD_FONT_INFO_FLAG_LOW_SIZE _BITUL(0) /* 256 */
>>>>> +#define KD_FONT_INFO_FLAG_HIGH_SIZE _BITUL(1) /* 512 */
>>>>
>>>> Do we really need those bits?
>>>> You set a default min/max font size in con_font_info() above,
>>>> and all drivers can override those values.
>>>> So, there are always min/max sizes available.
>>>
>>> These bits are not about the minimum and maximum glyph size, but about the
>>> number of glyphs in the font.
>>>
>>> Maybe this is an overkill, but sticon has this check:
>>>
>>> if ((w < 6) || (h < 6) || (w > 32) || (h > 32) || (vpitch != 32)
>>> || (op->charcount != 256 && op->charcount != 512))
>>>
>>> [ to be honest, I don’t know why this driver doesn’t accept a glyph of
>>> width 4 ]
>>
>> I think there was no technical limitation when I added that.
>> It's just that the font would be so small...
>
> If so, then I can remove min_height/min_width from the ioctl structure.
> And most likely the flags can also be left empty since at the moment all
> drivers support 512.
Yes, I think that's ok.
Helge
^ permalink raw reply
* Re: RFC: Restricting userspace interfaces for CXL fabric management
From: Jonathan Cameron @ 2024-04-25 11:33 UTC (permalink / raw)
To: Dan Williams
Cc: linux-cxl, Sreenivas Bagalkote, Brett Henning, Harold Johnson,
Sumanesh Samanta, linux-kernel, Davidlohr Bueso, Dave Jiang,
Alison Schofield, Vishal Verma, Ira Weiny, linuxarm, linux-api,
Lorenzo Pieralisi, Natu, Mahesh
In-Reply-To: <66284d5e8ac01_690a29431@dwillia2-xfh.jf.intel.com.notmuch>
On Tue, 23 Apr 2024 17:07:58 -0700
Dan Williams <dan.j.williams@intel.com> wrote:
> Jonathan Cameron wrote:
> [..]
> > > It is not clear to me that this material makes sense to house in
> > > drivers/ vs tools/ or even out-of-tree just for maintenance burden
> > > relief of keeping the universes separated. What does the Linux kernel
> > > project get out of carrying this in mainline alongside the inband code?
> >
> > I'm not sure what you mean by in band. Aim here was to discuss
> > in-band drivers for switch CCI etc. Same reason from a kernel point of
> > view for why we include embedded drivers. I'll interpret in band
> > as host driven and not inband as FM-API stuff.
> >
> > > I do think the mailbox refactoring to support non-CXL use cases is
> > > interesting, but only so far as refactoring is consumed for inband use
> > > cases like RAS API.
> >
> > If I read this right, I disagree with the 'only so far' bit.
> >
> > In all substantial ways we should support BMC use case of the Linux Kernel
> > at a similar level to how we support forms of Linux Distros.
>
> I think we need to talk in terms of specifics, because in the general
> case I do not see the blockage. OpenBMC currently is based on v6.6.28
> and carries 136 patches. An additional patch to turn off raw commands
> restrictions over there would not even be noticed.
Hi Dan,
That I'm fine with - it's a reasonable middle ground where we ensure
they have a sensible upstream solution, but just patch around the
taint etc in the downstream projects.
Note 136 patches is tiny for a distro and reflects their hard work
upstreaming stuff.
>
> > It may not be our target market as developers for particular parts of
> > our companies, but we should not block those who want to support it.
>
> It is also the case that there is a responsibility to build maintainable
> kernel interfaces that can be reasoned about, especially with devices as
> powerful as CXL that are trusted to host system memory and be caching
> agents. For example, I do not want to be in the position of auditing
> whether proposed tunnels and passthroughs violate lockdown expectations.
I agree with that - this can be made dependent on not locking down
in the same way lots of other somewhat dangerous interfaces are.
We can relax that restriction as things do get audited - sure
tunnels aren't going to be on that allowed list in the short term.
>
> Also, the assertion that these kernels will be built with
> CONFIG_SECURITY_LOCKDOWN_LSM=n and likely CONFIG_STRICT_DEVMEM=n, then
> the entire user-mode driver ABI is available for use. CXL commands are
> simple polled mmio, does Linux really benefit from carrying drivers in
> the kernel that the kernel itself does not care about?
Sure we could it in userspace... It's bad engineering, limits the design
to polling only and uses a bunch of interfaces we put a lot of effort into
telling people not to use except for debug.
I really don't see the advantage in pushing a project/group of projects
all of which are picking the upstream kernel up directly, to do a dirty
hack. We loose all the advantages of a proper well maintained kernel
driver purely on the argument that one use model is not the same as
this one. Sensible security lockdown requirements is fine (along
with all the other kernel features that must be disable for that
to work), making open kernel development on for a large Linux
market harder is not.
>
> [..]
> > Switch CCI Driver: PCI driver doing everything beyond the CXL mbox specific bit.
> > Type 3 Stack: All the normal stack just with the CXL Mailbox specific stuff factored
> > out. Note we can move different amounts of shared logic in here, but
> > in essence it deals with the extra layer on top of the raw MMPT mbox.
> > MMPT Mbox: Mailbox as per the PCI spec.
> > RAS API: Shared RAS API specific infrastructure used by other drivers.
>
> Once the CXL mailbox core is turned into a library for kernel internal
> consumers, like RAS API, or CXL accelerators, then it becomes easier to
> add a Switch CCI consumer (perhaps as an out-of-tree module in tools/),
> but it is still not clear why the kernel benefits from that arrangement.
We can argue later on this. But from my point of view, in tree module not
in tools is a must. Doesn't have to be in drivers/cxl if its simply the
association aspect that is a blocker.
>
> This is less about blocking developers that have different goals it is
> about finding the right projects / places to solve the problem
> especially when disjoint design goals are in play and user space drivers
> might be in reach.
Key here is this is not a case of openBMC is the one true distro on
which all Linux BMCs and fabric management platforms are based.
So we are really talking a random out of tree driver with all the maintenance
overhead that brings. Yuck.
So I don't see there being any good solution out side of upstream support
other than pushing this to be a userspace hack.
>
> [..]
> > > > The various CXL upstream developers and maintainers may have
> > > > differing views of course, but my current understanding is we want
> > > > to support 1 and 2, but are very resistant to 3!
> > >
> > > 1, yes, 2, need to see the patches, and agree on 3.
> >
> > If we end up with top architecture of the diagrams above, 2 will look pretty
> > similar to last version of the switch-cci patches. So raw commands only + taint.
> > Factoring out MMPT is another layer that doesn't make that much difference in
> > practice to this discussion. Good to have, but the reuse here would be one layer
> > above that.
> >
> > Or we just say go for second proposed architecture and 0 impact on the
> > CXL specific code, just reuse of the MMPT layer. I'd imagine people will get
> > grumpy on code duplication (and we'll spend years rejecting patch sets that
> > try to share the cdoe) but there should be no maintenance burden as
> > a result.
>
> I am assuming that the shared code between MMPT and CXL will happen and
> that all of the command infrastructure is where centralized policy can
> not keep up.
There is actually very little to MMPT, but sure there will be some sharing
of code and the policy won't sit in that shared part as it is protocol
specific.
> If OpenBMC wants to land a driver that consumes the MMPT
> core in tools/ that would seem to satisfy both the concerns of mainline
> not shipping ABI that host kernels need to strictly reason about while
> letting OpenBMC not need to carry out-of-tree patches indefinitely.
We can argue that detail later, but tools is not in my opinion
a valid solution to supporting properly maintained upstream drivers.
Its a hack for test and example modules only. The path of just
blocking this driver in any locked down situation seems much more inline
with kernel norms. It is also extremely bad precedence.
Jonathan
p.s. I don't care in the slightest about openBMC (other than general
warm fuzzy feelings about a good open source project), I do care
rather more about BMCs and other fabric managers.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox