- linux-morello - op-lists.linaro.org

[PATCH] kselftests/arm64: morello: Support R_MORELLO_FUNC_RELATIVE

by Jessica Clarke

Morello LLVM allows opting into distinguishing between function pointers (which may be interposed on by a compartmentalisation runtime) and code pointers (which always directly point to specific instructions, for cases like C++ landing pads and GNU C indirect goto). Supporting both relocations (which are equivalent for kselftest's non-compartmentalised purposes) will allow for Morello LLVM to make this opt-in behaviour always-on, reducing the number of ABI combinations. Signed-off-by: Jessica Clarke <jrtc27(a)jrtc27.com> --- .../selftests/arm64/morello/freestanding_init_globals.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tools/testing/selftests/arm64/morello/freestanding_init_globals.c b/tools/testing/selftests/arm64/morello/freestanding_init_globals.c index 12eb0af97..2fa2aa643 100644 --- a/tools/testing/selftests/arm64/morello/freestanding_init_globals.c +++ b/tools/testing/selftests/arm64/morello/freestanding_init_globals.c @@ -38,6 +38,9 @@ #ifndef R_MORELLO_RELATIVE #define R_MORELLO_RELATIVE 59395 #endif +#ifndef R_MORELLO_FUNC_RELATIVE +#define R_MORELLO_FUNC_RELATIVE 59400 +#endif struct cap_reloc { size_t capability_location; @@ -181,7 +184,8 @@ void __morello_process_dynamic_relocs(void *auxv) for (reloc = rela_dyn_start; reloc < rela_dyn_end; ++reloc) { uintptr_t *reloc_addr, value; - if (reloc->r_info != R_MORELLO_RELATIVE) + if (reloc->r_info != R_MORELLO_RELATIVE && + reloc->r_info != R_MORELLO_FUNC_RELATIVE) continue; reloc_addr = (uintptr_t *)cheri_address_set(cap_rw, reloc->r_offset); value = morello_relative(0, cap_rx, cap_rw, reloc, reloc_addr); -- 2.34.1

3 days, 8 hours

2
1
0 0

[PATCH v1] compiler: add __has_feature() fallback for old GCC

by Hesham Almatary

The CHERI port of Linux, similar to CheriBSD, now uses __has_feature() to check whether the compiler supports CHERI and provides the proper flags to enable it. However, __has_feature() only exists in Clang and recent versions of GCC. Building with older GCC versions that do not define this macro will cause the Linux build to fail. This commit adds backward compatibility for such cases by defining __has_feature() to 0. Signed-off-by: Hesham Almatary <Hesham.Almatary(a)cl.cam.ac.uk> --- include/linux/compiler_types.h | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h index f67fc7b9d..d92163333 100644 --- a/include/linux/compiler_types.h +++ b/include/linux/compiler_types.h @@ -11,6 +11,14 @@ #define __has_builtin(x) (0) #endif +/* + * __has_feature is supported in Clang and GCC >= 14. + * For older GCC versions, define __has_feature manually. + */ +#ifndef __has_feature +#define __has_feature(x) 0 +#endif + #ifndef __ASSEMBLY__ /* -- 2.34.1

4 months, 2 weeks

2
1
0 0

[PATCH] compiler: add __has_feature() fallback for old GCC

by Hesham Almatary

The CHERI port of Linux, similar to CheriBSD, now uses __has_feature() to check whether the compiler supports CHERI and provides the proper flags to enable it. However, __has_feature() only exists in Clang and recent versions of GCC. Building with older GCC versions that do not define this macro will cause the Linux build to fail. This commit adds backward compatibility for such cases by defining __has_feature() to 0. In the corner case of building with an older CHERI-GCC that uses CHERI ABIs, it defines __has_feature(capabilities) to 1. Signed-off-by: Hesham Almatary <Hesham.Almatary(a)cl.cam.ac.uk> --- include/linux/compiler_types.h | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h index f67fc7b9d..4677d9b17 100644 --- a/include/linux/compiler_types.h +++ b/include/linux/compiler_types.h @@ -11,6 +11,25 @@ #define __has_builtin(x) (0) #endif +/* + * __has_feature is supported in Clang and GCC >= 14. + * For older GCC versions, define __has_feature manually. + */ +#ifndef __has_feature +#define __has_feature(x) 0 +#endif + +/* + * Provide backward compatibility for older CHERI GCC toolchains that + * may not define __has_feature, but still support CHERI-enabled code. + */ +#if !__has_feature(capabilities) && \ + (defined(__CHERI__) || defined(__CHERI_HYBRID__) || \ + defined(__CHERI_PURE_CAPABILITY__)) +#undef __has_feature +#define __has_feature(x) (__builtin_strcmp(#x, "capabilities") == 0) +#endif + #ifndef __ASSEMBLY__ /* -- 2.34.1

4 months, 2 weeks

1
0
0 0

[PATCH] morello: Abandon the use of __CHERI__ define

by Hesham Almatary

This commit replaces the use of __CHERI__ macro with __has_feature(capabilities) similar to CheriBSD. Recent CHERI-LLVM (morello/dev), changes the assumption about __CHERI__, and only defines it for purecap code, while adding a new __CHERI_HYBRID__ define. __has_feature(capabilities) is 1:1 mapped to the older __CHERI__ behaviour as it is defined for either hybrid or purecap code. Future efforts to support purecap kernels will have to revisit this assumption especially if both hybrid and purecap kernels are going to be supported in the same codebase. __has_feature is always defined in Clang and recent GCC toolchain from GCC 14, but not older. This commit will break this Linux build if attempted to build with older GCC. Signed-off-by: Hesham Almatary <Hesham.Almatary(a)cl.cam.ac.uk> --- Documentation/cheri/pcuabi.rst | 88 +++++++++++------------ arch/arm64/include/uapi/asm/posix_types.h | 2 +- include/linux/cheri.h | 4 +- include/linux/compiler_types.h | 2 +- include/linux/types.h | 2 +- lib/cheri.c | 4 +- lib/test_printf.c | 2 +- lib/vsprintf.c | 14 ++-- 8 files changed, 59 insertions(+), 59 deletions(-) diff --git a/Documentation/cheri/pcuabi.rst b/Documentation/cheri/pcuabi.rst index a8a9a105c9e9..fea2c35afb79 100644 --- a/Documentation/cheri/pcuabi.rst +++ b/Documentation/cheri/pcuabi.rst @@ -142,54 +142,54 @@ kernel is built to support PCuABI. The table below provides the *representation* of various types **in the kernel** on a 64-bit architecture, depending on the supported user ABI: -+----------------------------------+------------------+----------------+--------------------------------------------------------------------------+ -| Type | Traditional uABI | PCuABI | Notes | -+==================================+==================+================+==========================================================================+ -| ``void *`` | 64-bit integer | 64-bit integer | | -+----------------------------------+------------------+----------------+--------------------------------------------------------------------------+ -| ``uintptr_t`` | 64-bit integer | 64-bit integer | | -+----------------------------------+------------------+----------------+--------------------------------------------------------------------------+ -| | ``(unsigned) long`` | 64-bit integer | 64-bit integer | ``ptraddr_t`` is a new generic type that represents an address. | -| | ``(unsigned) long long`` | | | | -| | ``ptraddr_t`` | | | | -+----------------------------------+------------------+----------------+--------------------------------------------------------------------------+ -| ``void __user *`` | 64-bit integer | Capability | | -+----------------------------------+------------------+----------------+--------------------------------------------------------------------------+ -| ``user_uintptr_t`` | 64-bit integer | Capability | Represented as ``uintcap_t`` in PCuABI, see below. | -+----------------------------------+------------------+----------------+--------------------------------------------------------------------------+ -| | ``__kernel_uintptr_t`` | 64-bit integer | Capability | * Represented as ``uintcap_t`` in PCuABI, see below. | -| | ``__kernel_aligned_uintptr_t`` | | | * At least 64-bit regardless of the ABI. | -+----------------------------------+------------------+----------------+--------------------------------------------------------------------------+ -| | ``void __capability *`` | Capability | Capability | Only available on CHERI-enabled architectures (``__CHERI__`` defined). | -| | ``void * __capability`` | | | | -+----------------------------------+------------------+----------------+--------------------------------------------------------------------------+ -| ``uintcap_t`` | Capability | Capability | * Only available on CHERI-enabled architectures (``__CHERI__`` defined). | -| | | | * Represented as a capability, but otherwise behaves as a 64-bit integer | -| | | | (when performing arithmetic, converting to other integer types, etc.). | -+----------------------------------+------------------+----------------+--------------------------------------------------------------------------+ ++----------------------------------+------------------+----------------+------------------------------------------------------------------------------------+ +| Type | Traditional uABI | PCuABI | Notes | ++==================================+==================+================+====================================================================================+ +| ``void *`` | 64-bit integer | 64-bit integer | | ++----------------------------------+------------------+----------------+------------------------------------------------------------------------------------+ +| ``uintptr_t`` | 64-bit integer | 64-bit integer | | ++----------------------------------+------------------+----------------+------------------------------------------------------------------------------------+ +| | ``(unsigned) long`` | 64-bit integer | 64-bit integer | ``ptraddr_t`` is a new generic type that represents an address. | +| | ``(unsigned) long long`` | | | | +| | ``ptraddr_t`` | | | | ++----------------------------------+------------------+----------------+------------------------------------------------------------------------------------+ +| ``void __user *`` | 64-bit integer | Capability | | ++----------------------------------+------------------+----------------+------------------------------------------------------------------------------------+ +| ``user_uintptr_t`` | 64-bit integer | Capability | Represented as ``uintcap_t`` in PCuABI, see below. | ++----------------------------------+------------------+----------------+------------------------------------------------------------------------------------+ +| | ``__kernel_uintptr_t`` | 64-bit integer | Capability | * Represented as ``uintcap_t`` in PCuABI, see below. | +| | ``__kernel_aligned_uintptr_t`` | | | * At least 64-bit regardless of the ABI. | ++----------------------------------+------------------+----------------+------------------------------------------------------------------------------------+ +| | ``void __capability *`` | Capability | Capability | Only available on CHERI-enabled architectures (``__has_feature(capabilities)``). | +| | ``void * __capability`` | | | | ++----------------------------------+------------------+----------------+------------------------------------------------------------------------------------+ +| ``uintcap_t`` | Capability | Capability | * Only available on CHERI-enabled architectures (``__has_feature(capabilities)``). | +| | | | * Represented as a capability, but otherwise behaves as a 64-bit integer | +| | | | (when performing arithmetic, converting to other integer types, etc.). | ++----------------------------------+------------------+----------------+------------------------------------------------------------------------------------+ For reference, the table below provides the representation of relevant types **in userspace**, depending on the chosen ABI: -+----------------------------------+-----------------+---------------------+------------------------------------------------------------------------+ -| Type | Traditional ABI | Pure-capability ABI | Notes | -+==================================+=================+=====================+========================================================================+ -| ``void *`` | 64-bit integer | Capability | | -+----------------------------------+-----------------+---------------------+------------------------------------------------------------------------+ -| ``uintptr_t`` | 64-bit integer | Capability | Represented as ``uintcap_t`` in purecap. | -+----------------------------------+-----------------+---------------------+------------------------------------------------------------------------+ -| | ``(unsigned) long`` | 64-bit integer | 64-bit integer | | -| | ``(unsigned) long long`` | | | | -| | ``ptraddr_t`` | | | | -+----------------------------------+-----------------+---------------------+------------------------------------------------------------------------+ -| | ``__kernel_uintptr_t`` | 64-bit integer | Capability | * Represented as ``uintcap_t`` in purecap. | -| | ``__kernel_aligned_uintptr_t`` | | | * At least 64-bit regardless of the ABI. | -+----------------------------------+-----------------+---------------------+------------------------------------------------------------------------+ -| | ``void __capability *`` | Capability | Capability | Only available on CHERI-enabled architectures (``__CHERI__`` defined). | -| | ``void * __capability`` | | | | -+----------------------------------+-----------------+---------------------+------------------------------------------------------------------------+ -| ``uintcap_t`` | Capability | Capability | Only available on CHERI-enabled architectures (``__CHERI__`` defined). | -+----------------------------------+-----------------+---------------------+------------------------------------------------------------------------+ ++----------------------------------+-----------------+---------------------+----------------------------------------------------------------------------------+ +| Type | Traditional ABI | Pure-capability ABI | Notes | ++==================================+=================+=====================+==================================================================================+ +| ``void *`` | 64-bit integer | Capability | | ++----------------------------------+-----------------+---------------------+----------------------------------------------------------------------------------+ +| ``uintptr_t`` | 64-bit integer | Capability | Represented as ``uintcap_t`` in purecap. | ++----------------------------------+-----------------+---------------------+----------------------------------------------------------------------------------+ +| | ``(unsigned) long`` | 64-bit integer | 64-bit integer | | +| | ``(unsigned) long long`` | | | | +| | ``ptraddr_t`` | | | | ++----------------------------------+-----------------+---------------------+----------------------------------------------------------------------------------+ +| | ``__kernel_uintptr_t`` | 64-bit integer | Capability | * Represented as ``uintcap_t`` in purecap. | +| | ``__kernel_aligned_uintptr_t`` | | | * At least 64-bit regardless of the ABI. | ++----------------------------------+-----------------+---------------------+----------------------------------------------------------------------------------+ +| | ``void __capability *`` | Capability | Capability | Only available on CHERI-enabled architectures (``__has_feature(capabilities)``). | +| | ``void * __capability`` | | | | ++----------------------------------+-----------------+---------------------+----------------------------------------------------------------------------------+ +| ``uintcap_t`` | Capability | Capability | Only available on CHERI-enabled architectures (``__has_feature(capabilities)``). | ++----------------------------------+-----------------+---------------------+----------------------------------------------------------------------------------+ For more information about user pointers and related conversions, please refer to the `user pointer documentation`_. diff --git a/arch/arm64/include/uapi/asm/posix_types.h b/arch/arm64/include/uapi/asm/posix_types.h index aafe38f9510a..4d75957977e7 100644 --- a/arch/arm64/include/uapi/asm/posix_types.h +++ b/arch/arm64/include/uapi/asm/posix_types.h @@ -6,7 +6,7 @@ typedef unsigned short __kernel_old_uid_t; typedef unsigned short __kernel_old_gid_t; #define __kernel_old_uid_t __kernel_old_uid_t -#ifdef __CHERI__ +#if __has_feature(capabilities) typedef __uintcap_t __kernel_uintcap_t; #else typedef __uint128_t __kernel_uintcap_t; diff --git a/include/linux/cheri.h b/include/linux/cheri.h index e5f588b056ad..641c9fc501fa 100644 --- a/include/linux/cheri.h +++ b/include/linux/cheri.h @@ -2,7 +2,7 @@ #ifndef _LINUX_CHERI_H #define _LINUX_CHERI_H -#ifdef __CHERI__ +#if __has_feature(capabilities) #include <cheriintrin.h> @@ -129,6 +129,6 @@ extern uintcap_t cheri_user_root_seal_cap; /* Userspace sealing root */ extern uintcap_t cheri_user_root_cid_cap; /* Userspace compartment ID root */ extern uintcap_t cheri_user_root_allperms_cap; /* Userspace root (all permissions) */ -#endif /* __CHERI__ */ +#endif /* __has_feature(capabilities) */ #endif /* _LINUX_CHERI_H */ diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h index 9718113906c1..f67fc7b9de1a 100644 --- a/include/linux/compiler_types.h +++ b/include/linux/compiler_types.h @@ -406,7 +406,7 @@ struct ftrace_likely_data { default: (x))) /* Is this type a native word size -- useful for atomic operations */ -#ifdef __CHERI__ +#if __has_feature(capabilities) #define __native_word(t) \ (sizeof(t) == sizeof(char) || sizeof(t) == sizeof(short) || \ sizeof(t) == sizeof(int) || sizeof(t) == sizeof(long) || \ diff --git a/include/linux/types.h b/include/linux/types.h index c1e3c406aa3e..751c471cca50 100644 --- a/include/linux/types.h +++ b/include/linux/types.h @@ -73,7 +73,7 @@ typedef __PTRADDR_TYPE__ ptraddr_t; typedef unsigned long ptraddr_t; #endif -#ifdef __CHERI__ +#if __has_feature(capabilities) typedef __uintcap_t uintcap_t; #endif diff --git a/lib/cheri.c b/lib/cheri.c index c308a2c75559..3f55899842c7 100644 --- a/lib/cheri.c +++ b/lib/cheri.c @@ -1,5 +1,5 @@ /* SPDX-License-Identifier: GPL-2.0-only */ -#ifdef __CHERI__ +#if __has_feature(capabilities) #include <linux/bug.h> #include <linux/cheri.h> @@ -70,4 +70,4 @@ bool cheri_check_cap(const void * __capability cap, size_t len, return true; } -#endif /* __CHERI__ */ +#endif /* __has_feature(capabilities) */ diff --git a/lib/test_printf.c b/lib/test_printf.c index 49cefbfbb742..1fb10c440580 100644 --- a/lib/test_printf.c +++ b/lib/test_printf.c @@ -784,7 +784,7 @@ errptr(void) static void __init capability_pointer(void) { -#ifdef __CHERI__ +#if __has_feature(capabilities) enum action { /* No action - use as is */ CAP_AXN_NONE, diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 1a2a01843775..e14e459f2e41 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -440,7 +440,7 @@ enum format_type { FORMAT_TYPE_INT, FORMAT_TYPE_SIZE_T, FORMAT_TYPE_PTRDIFF, -#ifdef __CHERI__ +#if __has_feature(capabilities) FORMAT_TYPE_CAPABILITY, #endif }; @@ -2510,7 +2510,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, } } -#ifdef __CHERI__ +#if __has_feature(capabilities) /* * Support for printing capabilities with %[#]lp[x] format. * It stands slightly in contradiction to kernel extensions @@ -2679,7 +2679,7 @@ char *capability(const char *fmt, char *buf, char *end, void * __capability cap, #undef update_buf_single } -#endif /* __CHERI__ */ +#endif /* __has_feature(capabilities) */ /* * Helper function to decode printf style format. @@ -2817,7 +2817,7 @@ int format_decode(const char *fmt, struct printf_spec *spec) case 'p': spec->type = FORMAT_TYPE_PTR; -#ifdef __CHERI__ +#if __has_feature(capabilities) if (qualifier == 'l') spec->type = FORMAT_TYPE_CAPABILITY; #endif @@ -3016,7 +3016,7 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args) *str = '%'; ++str; break; -#ifdef __CHERI__ +#if __has_feature(capabilities) case FORMAT_TYPE_CAPABILITY: str = capability(fmt, str, end, va_arg(args, void * __capability), @@ -3344,7 +3344,7 @@ int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args) while (isalnum(*fmt)) fmt++; break; -#ifdef __CHERI__ +#if __has_feature(capabilities) case FORMAT_TYPE_CAPABILITY: /* * Capabilities shall be handled now: subject to @@ -3543,7 +3543,7 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf) fmt++; break; } -#ifdef __CHERI__ +#if __has_feature(capabilities) case FORMAT_TYPE_CAPABILITY: if (str < end) { long length = strlen(args); -- 2.34.1

5 months, 1 week

2
1
0 0

[PATCH v5 0/5] netfilter: Create new type for kernel pointers.

by Joshua Lant

Changes from v4: - [03/05] Fix error with single struct union for two distinct members. - [04/05] Removed changes to ebt_replace_kernel - [05/05] Extend headers to include all x_tables.h rather than in c files. Joshua Lant (5): netfilter: Create new type for kernel pointers. x_tables.h: pointers to unions in uapi struct xt plugins: pointers to unions in uapi struct ebtables: pointers to unions in uapi struct xtables: move include to headers include/linux/netfilter.h | 6 +++++ include/uapi/linux/netfilter.h | 8 ++++++ include/uapi/linux/netfilter/x_tables.h | 18 +++++++++++-- include/uapi/linux/netfilter/xt_CT.h | 11 ++++++-- include/uapi/linux/netfilter/xt_IDLETIMER.h | 13 ++++++++-- include/uapi/linux/netfilter/xt_RATEEST.h | 7 +++++- include/uapi/linux/netfilter/xt_TEE.h | 7 +++++- include/uapi/linux/netfilter/xt_bpf.h | 13 ++++++++-- include/uapi/linux/netfilter/xt_connlimit.h | 6 ++++- include/uapi/linux/netfilter/xt_hashlimit.h | 25 ++++++++++++++++--- include/uapi/linux/netfilter/xt_limit.h | 7 +++++- include/uapi/linux/netfilter/xt_nfacct.h | 13 ++++++++-- include/uapi/linux/netfilter/xt_quota.h | 7 +++++- include/uapi/linux/netfilter/xt_rateest.h | 12 +++++++-- include/uapi/linux/netfilter/xt_statistic.h | 7 +++++- .../uapi/linux/netfilter_bridge/ebtables.h | 18 ++++++++++--- net/netfilter/xt_CT.c | 1 - net/netfilter/xt_IDLETIMER.c | 1 - net/netfilter/xt_RATEEST.c | 1 - net/netfilter/xt_TEE.c | 1 - net/netfilter/xt_bpf.c | 1 - net/netfilter/xt_connlimit.c | 1 - net/netfilter/xt_hashlimit.c | 1 - net/netfilter/xt_limit.c | 1 - net/netfilter/xt_nfacct.c | 1 - net/netfilter/xt_quota.c | 1 - net/netfilter/xt_rateest.c | 1 - net/netfilter/xt_statistic.c | 1 - 28 files changed, 153 insertions(+), 37 deletions(-) -- 2.34.1

1 year

2
6
0 0

[PATCH v4 0/5] netfilter: Create new type for kernel pointers.

by Joshua Lant

Hi Kevin, Sorry I know I said that I would get a spare moment to do this work over a month ago, but the University of Manchester's elves are very busy this time of year! Here are the changes you requested... Please let me know if there are issues building. I still don't much understand why that last series had a problem. Changes from v3: - [XX/05] Modified the use of __nf_kptr_t in the xtables plugin structs to use a union, with the original struct as a member. This trick allows for removal of the heavy casting in the kernel which was required in the earlier version. - [XX/05] Squashed many of the commits (those from the xtables plugin header files) into a single commit, since each individual commit now makes far fewer changes. Testing: - Tested with purecap iptables tests (nftables only), passing 65/68 tests. Those which fail are expected to fail at this point, due primarily to improperly written test cases, or missing versions of userspace tooling. Joshua Lant (5): netfilter: Create new type for kernel pointers. x_tables.h: pointers to unions in uapi struct xt plugins: pointers to unions in uapi struct ebtables: pointers to unions in uapi struct xtables: move include to headers include/linux/netfilter.h | 6 +++++ include/uapi/linux/netfilter.h | 8 ++++++ include/uapi/linux/netfilter/x_tables.h | 18 +++++++++++-- include/uapi/linux/netfilter/xt_CT.h | 10 +++++-- include/uapi/linux/netfilter/xt_IDLETIMER.h | 12 +++++++-- include/uapi/linux/netfilter/xt_RATEEST.h | 6 ++++- include/uapi/linux/netfilter/xt_TEE.h | 6 ++++- include/uapi/linux/netfilter/xt_bpf.h | 13 +++++++-- include/uapi/linux/netfilter/xt_connlimit.h | 6 ++++- include/uapi/linux/netfilter/xt_hashlimit.h | 24 ++++++++++++++--- include/uapi/linux/netfilter/xt_limit.h | 6 ++++- include/uapi/linux/netfilter/xt_nfacct.h | 12 +++++++-- include/uapi/linux/netfilter/xt_quota.h | 6 ++++- include/uapi/linux/netfilter/xt_rateest.h | 9 +++++-- include/uapi/linux/netfilter/xt_statistic.h | 7 ++++- .../uapi/linux/netfilter_bridge/ebtables.h | 27 +++++++++++++++---- net/netfilter/xt_bpf.c | 1 - net/netfilter/xt_statistic.c | 1 - 18 files changed, 149 insertions(+), 29 deletions(-) -- 2.34.1

1 year

2
10
0 0

Re: [RESEND PATCH] fs/pipe: Introduce a check to skip sleeping processes during pipe read/write

by Manfred Spraul

Hi, (I had to remove many cc, my mail server rejected to send) On 12/26/24 9:11 PM, Oleg Nesterov wrote: > I mostly agree, see my reply to this patch, but... > > On 12/26, Linus Torvalds wrote: >> If the optimization is correct, there is no point to having a config option. >> >> If the optimization is incorrect, there is no point to having the code. >> >> Either way, there's no way we'd ever have a config option for this. > Agreed, > >>> + if (was_full && pipe_check_wq_has_sleeper(&pipe->wr_wait)) >>> wake_up_interruptible_sync_poll(&pipe->wr_wait, EPOLLOUT | EPOLLWRNORM); >> End result: you need to explain why the race cannot exist. > Agreed, > >> And I think your patch is simply buggy > Agreed again, but probably my reasoning was wrong, > >> But now waiters use "wait_event_interruptible_exclusive()" explicitly >> outside the pipe mutex, so the waiters and wakers aren't actually >> serialized. > This is what I don't understand... Could you spell ? > I _think_ that > > wait_event_whatever(WQ, CONDITION); > vs > > CONDITION = 1; > if (wq_has_sleeper(WQ)) > wake_up_xxx(WQ, ...); > > is fine. This pattern is documented in wait.h: https://elixir.bootlin.com/linux/v6.12.6/source/include/linux/wait.h#L96 Thus if there an issue, then the documentation should be updated. > Both wq_has_sleeper() and wait_event_whatever()->prepare_to_wait_event() > have the necessary barriers to serialize the waiters and wakers. > > Damn I am sure I missed something ;) Actually: Does this work universally? I.e. can we add the optimization into __wake_up()? But I do not understand this comment (from 2.6.0) https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/ker… > /* * Note: we use "set_current_state()" _after_ the wait-queue add, * > because we need a memory barrier there on SMP, so that any * > wake-function that tests for the wait-queue being active * will be > guaranteed to see waitqueue addition _or_ subsequent * tests in this > thread will see the wakeup having taken place. * * The spin_unlock() > itself is semi-permeable and only protects * one way (it only protects > stuff inside the critical region and * stops them from bleeding out - > it would still allow subsequent * loads to move into the the critical > region). */ voidprepare_to_wait > <https://elixir.bootlin.com/linux/v2.6.0/C/ident/prepare_to_wait>(wait_queue_head_t > <https://elixir.bootlin.com/linux/v2.6.0/C/ident/wait_queue_head_t>*q,wait_queue_t > <https://elixir.bootlin.com/linux/v2.6.0/C/ident/wait_queue_t>*wait > <https://elixir.bootlin.com/linux/v2.6.0/C/ident/wait>,intstate) { > unsignedlongflags; wait > <https://elixir.bootlin.com/linux/v2.6.0/C/ident/wait>->flags&=~WQ_FLAG_EXCLUSIVE > <https://elixir.bootlin.com/linux/v2.6.0/C/ident/WQ_FLAG_EXCLUSIVE>; > spin_lock_irqsave > <https://elixir.bootlin.com/linux/v2.6.0/C/ident/spin_lock_irqsave>(&q->lock,flags); > if(list_empty > <https://elixir.bootlin.com/linux/v2.6.0/C/ident/list_empty>(&wait > <https://elixir.bootlin.com/linux/v2.6.0/C/ident/wait>->task_list > <https://elixir.bootlin.com/linux/v2.6.0/C/ident/task_list>)) > __add_wait_queue > <https://elixir.bootlin.com/linux/v2.6.0/C/ident/__add_wait_queue>(q,wait > <https://elixir.bootlin.com/linux/v2.6.0/C/ident/wait>); > set_current_state > <https://elixir.bootlin.com/linux/v2.6.0/C/ident/set_current_state>(state); > spin_unlock_irqrestore > <https://elixir.bootlin.com/linux/v2.6.0/C/ident/spin_unlock_irqrestore>(&q->lock,flags); > } set_current_state() now uses smp_store_mb(), which is a memory barrier _after_ the store. Thus I do not see what enforces that the store happens before the store for the __add_wait_queue(). -- Manfred

1 year, 2 months

2
3
0 0

[RESEND PATCH] fs/pipe: Introduce a check to skip sleeping processes during pipe read/write

by WangYuli

When a user calls the read/write system call and passes a pipe descriptor, the pipe_read/pipe_write functions are invoked: 1. pipe_read(): 1). Checks if the pipe is valid and if there is any data in the pipe buffer. 2). Waits for data: *If there is no data in the pipe and the write end is still open, the current process enters a sleep state (wait_event()) until data is written. *If the write end is closed, return 0. 3). Reads data: *Wakes up the process and copies data from the pipe's memory buffer to user space. *When the buffer is full, the writing process will go to sleep, waiting for the pipe state to change to be awakened (using the wake_up_interruptible_sync_poll() mechanism). Once data is read from the buffer, the writing process can continue writing, and the reading process can continue reading new data. 4). Returns the number of bytes read upon successful read. 2. pipe_write(): 1). Checks if the pipe is valid and if there is any available space in the pipe buffer. 2). Waits for buffer space: *If the pipe buffer is full and the reading process has not read any data, pipe_write() may put the current process to sleep until there is space in the buffer. *If the read end of the pipe is closed (no process is waiting to read), an error code -EPIPE is returned, and a SIGPIPE signal may be sent to the process. 3). Writes data: *If there is enough space in the pipe buffer, pipe_write() copies data from the user space buffer to the kernel buffer of the pipe (using copy_from_user()). *If the amount of data the user requests to write is larger than the available space in the buffer, multiple writes may be required, or the process may wait for new space to be freed. 4). Wakes up waiting reading processes: *After the data is successfully written, pipe_write() wakes up any processes that may be waiting to read data (using the wake_up_interruptible_sync_poll() mechanism). 5). Returns the number of bytes successfully written. Check if there are any waiting processes in the process wait queue by introducing wq_has_sleeper() when waking up processes for pipe read/write operations. If no processes are waiting, there's no need to execute wake_up_interruptible_sync_poll(), thus avoiding unnecessary wake-ups. Unnecessary wake-ups can lead to context switches, where a process is woken up to handle I/O events even when there is no immediate need. Only wake up processes when there are actually waiting processes to reduce context switches and system overhead by checking with wq_has_sleeper(). Additionally, by reducing unnecessary synchronization and wake-up operations, wq_has_sleeper() can decrease system resource waste and lock contention, improving overall system performance. For pipe read/write operations, this eliminates ineffective scheduling and enhances concurrency. It's important to note that enabling this option means invoking wq_has_sleeper() to check for sleeping processes in the wait queue for every read or write operation. While this is a lightweight operation, it still incurs some overhead. In low-load or single-task scenarios, this overhead may not yield significant benefits and could even introduce minor performance degradation. UnixBench Pipe benchmark results on Zhaoxin KX-U6780A processor: With the option disabled: Single-core: 841.8, Multi-core (8): 4621.6 With the option enabled: Single-core: 877.8, Multi-core (8): 4854.7 Single-core performance improved by 4.1%, multi-core performance improved by 4.8%. Co-developed-by: Shengjin Yu <yushengjin(a)uniontech.com> Signed-off-by: Shengjin Yu <yushengjin(a)uniontech.com> Co-developed-by: Dandan Zhang <zhangdandan(a)uniontech.com> Signed-off-by: Dandan Zhang <zhangdandan(a)uniontech.com> Tested-by: Dandan Zhang <zhangdandan(a)uniontech.com> Signed-off-by: WangYuli <wangyuli(a)uniontech.com> --- fs/Kconfig | 13 +++++++++++++ fs/pipe.c | 21 +++++++++++++++------ 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/fs/Kconfig b/fs/Kconfig index 64d420e3c475..0dacc46a73fe 100644 --- a/fs/Kconfig +++ b/fs/Kconfig @@ -429,4 +429,17 @@ source "fs/unicode/Kconfig" config IO_WQ bool +config PIPE_SKIP_SLEEPER + bool "Skip sleeping processes during pipe read/write" + default n + help + This option introduces a check whether the sleep queue will + be awakened during pipe read/write. + + It often leads to a performance improvement. However, in + low-load or single-task scenarios, it may introduce minor + performance overhead. + + If unsure, say N. + endmenu diff --git a/fs/pipe.c b/fs/pipe.c index 12b22c2723b7..c085333ae72c 100644 --- a/fs/pipe.c +++ b/fs/pipe.c @@ -247,6 +247,15 @@ static inline unsigned int pipe_update_tail(struct pipe_inode_info *pipe, return tail; } +static inline bool +pipe_check_wq_has_sleeper(struct wait_queue_head *wq_head) +{ + if (IS_ENABLED(CONFIG_PIPE_SKIP_SLEEPER)) + return wq_has_sleeper(wq_head); + else + return true; +} + static ssize_t pipe_read(struct kiocb *iocb, struct iov_iter *to) { @@ -377,7 +386,7 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to) * _very_ unlikely case that the pipe was full, but we got * no data. */ - if (unlikely(was_full)) + if (unlikely(was_full) && pipe_check_wq_has_sleeper(&pipe->wr_wait)) wake_up_interruptible_sync_poll(&pipe->wr_wait, EPOLLOUT | EPOLLWRNORM); kill_fasync(&pipe->fasync_writers, SIGIO, POLL_OUT); @@ -398,9 +407,9 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to) wake_next_reader = false; mutex_unlock(&pipe->mutex); - if (was_full) + if (was_full && pipe_check_wq_has_sleeper(&pipe->wr_wait)) wake_up_interruptible_sync_poll(&pipe->wr_wait, EPOLLOUT | EPOLLWRNORM); - if (wake_next_reader) + if (wake_next_reader && pipe_check_wq_has_sleeper(&pipe->rd_wait)) wake_up_interruptible_sync_poll(&pipe->rd_wait, EPOLLIN | EPOLLRDNORM); kill_fasync(&pipe->fasync_writers, SIGIO, POLL_OUT); if (ret > 0) @@ -573,7 +582,7 @@ pipe_write(struct kiocb *iocb, struct iov_iter *from) * become empty while we dropped the lock. */ mutex_unlock(&pipe->mutex); - if (was_empty) + if (was_empty && pipe_check_wq_has_sleeper(&pipe->rd_wait)) wake_up_interruptible_sync_poll(&pipe->rd_wait, EPOLLIN | EPOLLRDNORM); kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN); wait_event_interruptible_exclusive(pipe->wr_wait, pipe_writable(pipe)); @@ -598,10 +607,10 @@ pipe_write(struct kiocb *iocb, struct iov_iter *from) * Epoll nonsensically wants a wakeup whether the pipe * was already empty or not. */ - if (was_empty || pipe->poll_usage) + if ((was_empty || pipe->poll_usage) && pipe_check_wq_has_sleeper(&pipe->rd_wait)) wake_up_interruptible_sync_poll(&pipe->rd_wait, EPOLLIN | EPOLLRDNORM); kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN); - if (wake_next_writer) + if (wake_next_writer && pipe_check_wq_has_sleeper(&pipe->wr_wait)) wake_up_interruptible_sync_poll(&pipe->wr_wait, EPOLLOUT | EPOLLWRNORM); if (ret > 0 && sb_start_write_trylock(file_inode(filp)->i_sb)) { int err = file_update_time(filp); -- 2.45.2

1 year, 2 months

3
3
0 0

[PATCH v3 00/17] netfilter: Create new type for kernel pointers.

by Joshua Lant

Changes from v2: - [01/17] Updated first commit comment to make more sense to the reader. - [XX/17] Numerous formatting nits fixed. - [05/17] Fixed up local assignment and functional regression found. - [08/17] Fixed up functional regression found. Requested changes not addressed: In patch [01/17]: > Do we really have to duplicate the typedefs here? There are already > quite a few headers including <linux/netfilter.h> under > include/uapi/linux/netfilter/ so including it in x_tables.h too > shouldn't be too problematic. Unfortunately this duplication is required. I encountered many issues building when I tried this initially before the last patch submission. Presumably this is an artefact of xtables being deprecated by nftables? Many definitions are duplicated between these two headers. Many thanks, Josh Joshua Lant (17): netfilter: Create new type for kernel pointers. x_tables.h: remove kernel pointer from uapi xt_entry_match struct x_tables.h: remove kernel pointer from uapi xt_entry_target struct xt_CT: remove pointer from uapi struct xt_IDLETIMER: remove pointer from uapi struct xt_RATEEST: remove pointer from uapi struct xt_TEE: remove pointer from uapi struct xt_bpf: remove pointer from uapi struct xt_connlimit: remove pointer from uapi struct xt_hashlimit: remove pointer from uapi struct xt_limit: remove pointer from uapi struct xt_nfacct: remove pointer from uapi struct xt_quota: remove pointer from uapi struct xt_rateest: remove pointer from uapi struct xt_statistic: remove pointer from uapi struct ebtables: remove pointer from uapi struct xtables: move include to headers include/linux/netfilter.h | 6 + include/uapi/linux/netfilter.h | 8 + include/uapi/linux/netfilter/x_tables.h | 12 +- include/uapi/linux/netfilter/xt_CT.h | 4 +- include/uapi/linux/netfilter/xt_IDLETIMER.h | 6 +- include/uapi/linux/netfilter/xt_RATEEST.h | 3 +- include/uapi/linux/netfilter/xt_TEE.h | 3 +- include/uapi/linux/netfilter/xt_bpf.h | 7 +- include/uapi/linux/netfilter/xt_connlimit.h | 3 +- include/uapi/linux/netfilter/xt_hashlimit.h | 17 ++- include/uapi/linux/netfilter/xt_limit.h | 3 +- include/uapi/linux/netfilter/xt_nfacct.h | 6 +- include/uapi/linux/netfilter/xt_quota.h | 3 +- include/uapi/linux/netfilter/xt_rateest.h | 5 +- include/uapi/linux/netfilter/xt_statistic.h | 4 +- .../uapi/linux/netfilter_bridge/ebtables.h | 15 +- net/bridge/netfilter/ebtable_broute.c | 2 +- net/bridge/netfilter/ebtable_filter.c | 6 +- net/bridge/netfilter/ebtable_nat.c | 6 +- net/bridge/netfilter/ebtables.c | 64 ++++---- net/ipv4/netfilter/arp_tables.c | 22 +-- net/ipv4/netfilter/ip_tables.c | 44 +++--- net/ipv6/netfilter/ip6_tables.c | 44 +++--- net/netfilter/x_tables.c | 20 +-- net/netfilter/xt_CT.c | 10 +- net/netfilter/xt_IDLETIMER.c | 143 ++++++++++-------- net/netfilter/xt_RATEEST.c | 12 +- net/netfilter/xt_TCPMSS.c | 2 +- net/netfilter/xt_TEE.c | 12 +- net/netfilter/xt_bpf.c | 19 +-- net/netfilter/xt_connlimit.c | 8 +- net/netfilter/xt_hashlimit.c | 24 +-- net/netfilter/xt_limit.c | 6 +- net/netfilter/xt_nfacct.c | 8 +- net/netfilter/xt_quota.c | 12 +- net/netfilter/xt_rateest.c | 12 +- net/netfilter/xt_statistic.c | 13 +- 37 files changed, 327 insertions(+), 267 deletions(-) -- 2.34.1

1 year, 3 months

2
20
0 0

[PATCH v2 00/17] netfilter: remove kernel pointers from uapi structs

by Joshua Lant

This is v2 of the previous patchset found here: https://op-lists.linaro.org/archives/list/linux-morello@op-lists.linaro.org… I have added the new __nf_kptr_t type and modified all the patches accordingly, as well as fixing up all (hopefully) of the nits found in the previous patches. Many thanks Kevin for your input on everything. I hope that this is now in a decent state. Cheers. Joshua Lant (17): netfilter: Create new type for kernel pointers. x_tables.h: remove kernel pointer from uapi xt_entry_match struct x_tables.h: remove kernel pointer from uapi xt_entry_target struct xt_CT: remove pointer from uapi struct xt_IDLETIMER: remove pointer from uapi struct xt_RATEEST: remove pointer from uapi struct xt_TEE: remove pointer from uapi struct xt_bpf: remove pointer from uapi struct xt_connlimit: remove pointer from uapi struct xt_hashlimit: remove pointer from uapi struct xt_limit: remove pointer from uapi struct xt_nfacct: remove pointer from uapi struct xt_quota: remove pointer from uapi struct xt_rateest: remove pointer from uapi struct xt_statistic: remove pointer from uapi struct ebtables: remove pointer from uapi struct xtables: move include to headers include/linux/netfilter.h | 6 + include/uapi/linux/netfilter.h | 8 + include/uapi/linux/netfilter/x_tables.h | 12 +- include/uapi/linux/netfilter/xt_CT.h | 4 +- include/uapi/linux/netfilter/xt_IDLETIMER.h | 6 +- include/uapi/linux/netfilter/xt_RATEEST.h | 3 +- include/uapi/linux/netfilter/xt_TEE.h | 3 +- include/uapi/linux/netfilter/xt_bpf.h | 7 +- include/uapi/linux/netfilter/xt_connlimit.h | 3 +- include/uapi/linux/netfilter/xt_hashlimit.h | 17 ++- include/uapi/linux/netfilter/xt_limit.h | 3 +- include/uapi/linux/netfilter/xt_nfacct.h | 6 +- include/uapi/linux/netfilter/xt_quota.h | 3 +- include/uapi/linux/netfilter/xt_rateest.h | 5 +- include/uapi/linux/netfilter/xt_statistic.h | 4 +- .../uapi/linux/netfilter_bridge/ebtables.h | 15 +- net/bridge/netfilter/ebtable_broute.c | 2 +- net/bridge/netfilter/ebtable_filter.c | 6 +- net/bridge/netfilter/ebtable_nat.c | 6 +- net/bridge/netfilter/ebtables.c | 64 ++++---- net/ipv4/netfilter/arp_tables.c | 22 +-- net/ipv4/netfilter/ip_tables.c | 44 +++--- net/ipv6/netfilter/ip6_tables.c | 44 +++--- net/netfilter/x_tables.c | 20 +-- net/netfilter/xt_CT.c | 10 +- net/netfilter/xt_IDLETIMER.c | 139 +++++++++--------- net/netfilter/xt_RATEEST.c | 12 +- net/netfilter/xt_TCPMSS.c | 2 +- net/netfilter/xt_TEE.c | 12 +- net/netfilter/xt_bpf.c | 19 +-- net/netfilter/xt_connlimit.c | 8 +- net/netfilter/xt_hashlimit.c | 24 +-- net/netfilter/xt_limit.c | 6 +- net/netfilter/xt_nfacct.c | 8 +- net/netfilter/xt_quota.c | 12 +- net/netfilter/xt_rateest.c | 12 +- net/netfilter/xt_statistic.c | 13 +- 37 files changed, 323 insertions(+), 267 deletions(-) -- 2.34.1

1 year, 4 months

2
25
0 0

2026

2025

2024

2023

2022

linux-morello