* [PATCH v2 1/3] lib/test_kasan: Add bitops tests
From: Marco Elver @ 2019-05-29 14:14 UTC (permalink / raw)
To: peterz, aryabinin, dvyukov, glider, andreyknvl, mark.rutland
Cc: corbet, tglx, mingo, bp, hpa, x86, arnd, jpoimboe, linux-doc,
linux-kernel, linux-arch, kasan-dev, Marco Elver
In-Reply-To: <20190529141500.193390-1-elver@google.com>
This adds bitops tests to the test_kasan module. In a follow-up patch,
support for bitops instrumentation will be added.
Signed-off-by: Marco Elver <elver@google.com>
---
Changes in v2:
* Use BITS_PER_LONG.
* Use heap allocated memory for test, as newer compilers (correctly)
warn on OOB stack access.
---
lib/test_kasan.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 72 insertions(+), 3 deletions(-)
diff --git a/lib/test_kasan.c b/lib/test_kasan.c
index 7de2702621dc..6562df0ca30d 100644
--- a/lib/test_kasan.c
+++ b/lib/test_kasan.c
@@ -11,16 +11,17 @@
#define pr_fmt(fmt) "kasan test: %s " fmt, __func__
+#include <linux/bitops.h>
#include <linux/delay.h>
+#include <linux/kasan.h>
#include <linux/kernel.h>
-#include <linux/mman.h>
#include <linux/mm.h>
+#include <linux/mman.h>
+#include <linux/module.h>
#include <linux/printk.h>
#include <linux/slab.h>
#include <linux/string.h>
#include <linux/uaccess.h>
-#include <linux/module.h>
-#include <linux/kasan.h>
/*
* Note: test functions are marked noinline so that their names appear in
@@ -623,6 +624,73 @@ static noinline void __init kasan_strings(void)
strnlen(ptr, 1);
}
+static noinline void __init kasan_bitops(void)
+{
+ long *bits = kmalloc(sizeof(long), GFP_KERNEL | __GFP_ZERO);
+ if (!bits)
+ return;
+
+ pr_info("within-bounds in set_bit");
+ set_bit(0, bits);
+
+ pr_info("within-bounds in set_bit");
+ set_bit(BITS_PER_LONG - 1, bits);
+
+ pr_info("out-of-bounds in set_bit\n");
+ set_bit(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in __set_bit\n");
+ __set_bit(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in clear_bit\n");
+ clear_bit(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in __clear_bit\n");
+ __clear_bit(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in clear_bit_unlock\n");
+ clear_bit_unlock(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in __clear_bit_unlock\n");
+ __clear_bit_unlock(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in change_bit\n");
+ change_bit(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in __change_bit\n");
+ __change_bit(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in test_and_set_bit\n");
+ test_and_set_bit(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in __test_and_set_bit\n");
+ __test_and_set_bit(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in test_and_set_bit_lock\n");
+ test_and_set_bit_lock(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in test_and_clear_bit\n");
+ test_and_clear_bit(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in __test_and_clear_bit\n");
+ __test_and_clear_bit(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in test_and_change_bit\n");
+ test_and_change_bit(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in __test_and_change_bit\n");
+ __test_and_change_bit(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in test_bit\n");
+ (void)test_bit(BITS_PER_LONG, bits);
+
+#if defined(clear_bit_unlock_is_negative_byte)
+ pr_info("out-of-bounds in clear_bit_unlock_is_negative_byte\n");
+ clear_bit_unlock_is_negative_byte(BITS_PER_LONG, bits);
+#endif
+ kfree(bits);
+}
+
static int __init kmalloc_tests_init(void)
{
/*
@@ -664,6 +732,7 @@ static int __init kmalloc_tests_init(void)
kasan_memchr();
kasan_memcmp();
kasan_strings();
+ kasan_bitops();
kasan_restore_multi_shot(multishot);
--
2.22.0.rc1.257.g3120a18244-goog
^ permalink raw reply related
* [PATCH 2/3] x86: Move CPU feature test out of uaccess region
From: Marco Elver @ 2019-05-29 14:15 UTC (permalink / raw)
To: peterz, aryabinin, dvyukov, glider, andreyknvl, mark.rutland
Cc: corbet, tglx, mingo, bp, hpa, x86, arnd, jpoimboe, linux-doc,
linux-kernel, linux-arch, kasan-dev, Marco Elver
In-Reply-To: <20190529141500.193390-1-elver@google.com>
This patch is a pre-requisite for enabling KASAN bitops instrumentation:
moves boot_cpu_has feature test out of the uaccess region, as
boot_cpu_has uses test_bit. With instrumentation, the KASAN check would
otherwise be flagged by objtool.
This approach is preferred over adding the explicit kasan_check_*
functions to the uaccess whitelist of objtool, as the case here appears
to be the only one.
Signed-off-by: Marco Elver <elver@google.com>
---
v1:
* This patch replaces patch: 'tools/objtool: add kasan_check_* to
uaccess whitelist'
---
arch/x86/ia32/ia32_signal.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
index 629d1ee05599..12264e3c9c43 100644
--- a/arch/x86/ia32/ia32_signal.c
+++ b/arch/x86/ia32/ia32_signal.c
@@ -333,6 +333,7 @@ int ia32_setup_rt_frame(int sig, struct ksignal *ksig,
void __user *restorer;
int err = 0;
void __user *fpstate = NULL;
+ bool has_xsave;
/* __copy_to_user optimizes that into a single 8 byte store */
static const struct {
@@ -352,13 +353,19 @@ int ia32_setup_rt_frame(int sig, struct ksignal *ksig,
if (!access_ok(frame, sizeof(*frame)))
return -EFAULT;
+ /*
+ * Move non-uaccess accesses out of uaccess region if not strictly
+ * required; this also helps avoid objtool flagging these accesses with
+ * instrumentation enabled.
+ */
+ has_xsave = boot_cpu_has(X86_FEATURE_XSAVE);
put_user_try {
put_user_ex(sig, &frame->sig);
put_user_ex(ptr_to_compat(&frame->info), &frame->pinfo);
put_user_ex(ptr_to_compat(&frame->uc), &frame->puc);
/* Create the ucontext. */
- if (boot_cpu_has(X86_FEATURE_XSAVE))
+ if (has_xsave)
put_user_ex(UC_FP_XSTATE, &frame->uc.uc_flags);
else
put_user_ex(0, &frame->uc.uc_flags);
--
2.22.0.rc1.257.g3120a18244-goog
^ permalink raw reply related
* [PATCH v2 3/3] asm-generic, x86: Add bitops instrumentation for KASAN
From: Marco Elver @ 2019-05-29 14:15 UTC (permalink / raw)
To: peterz, aryabinin, dvyukov, glider, andreyknvl, mark.rutland
Cc: corbet, tglx, mingo, bp, hpa, x86, arnd, jpoimboe, linux-doc,
linux-kernel, linux-arch, kasan-dev, Marco Elver
In-Reply-To: <20190529141500.193390-1-elver@google.com>
This adds a new header to asm-generic to allow optionally instrumenting
architecture-specific asm implementations of bitops.
This change includes the required change for x86 as reference and
changes the kernel API doc to point to bitops-instrumented.h instead.
Rationale: the functions in x86's bitops.h are no longer the kernel API
functions, but instead the arch_ prefixed functions, which are then
instrumented via bitops-instrumented.h.
Other architectures can similarly add support for asm implementations of
bitops.
The documentation text has been copied/moved, and *no* changes to it
have been made in this patch.
Tested: using lib/test_kasan with bitops tests (pre-requisite patch).
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=198439
Signed-off-by: Marco Elver <elver@google.com>
---
Changes in v2:
* Instrument word-sized accesses, as specified by the interface.
---
Documentation/core-api/kernel-api.rst | 2 +-
arch/x86/include/asm/bitops.h | 210 ++++----------
include/asm-generic/bitops-instrumented.h | 317 ++++++++++++++++++++++
3 files changed, 370 insertions(+), 159 deletions(-)
create mode 100644 include/asm-generic/bitops-instrumented.h
diff --git a/Documentation/core-api/kernel-api.rst b/Documentation/core-api/kernel-api.rst
index a29c99d13331..65266fa1b706 100644
--- a/Documentation/core-api/kernel-api.rst
+++ b/Documentation/core-api/kernel-api.rst
@@ -51,7 +51,7 @@ The Linux kernel provides more basic utility functions.
Bit Operations
--------------
-.. kernel-doc:: arch/x86/include/asm/bitops.h
+.. kernel-doc:: include/asm-generic/bitops-instrumented.h
:internal:
Bitmap Operations
diff --git a/arch/x86/include/asm/bitops.h b/arch/x86/include/asm/bitops.h
index 8e790ec219a5..8ebf7af9a0f4 100644
--- a/arch/x86/include/asm/bitops.h
+++ b/arch/x86/include/asm/bitops.h
@@ -49,23 +49,8 @@
#define CONST_MASK_ADDR(nr, addr) WBYTE_ADDR((void *)(addr) + ((nr)>>3))
#define CONST_MASK(nr) (1 << ((nr) & 7))
-/**
- * set_bit - Atomically set a bit in memory
- * @nr: the bit to set
- * @addr: the address to start counting from
- *
- * This function is atomic and may not be reordered. See __set_bit()
- * if you do not require the atomic guarantees.
- *
- * Note: there are no guarantees that this function will not be reordered
- * on non x86 architectures, so if you are writing portable code,
- * make sure not to rely on its reordering guarantees.
- *
- * Note that @nr may be almost arbitrarily large; this function is not
- * restricted to acting on a single-word quantity.
- */
static __always_inline void
-set_bit(long nr, volatile unsigned long *addr)
+arch_set_bit(long nr, volatile unsigned long *addr)
{
if (IS_IMMEDIATE(nr)) {
asm volatile(LOCK_PREFIX "orb %1,%0"
@@ -77,33 +62,17 @@ set_bit(long nr, volatile unsigned long *addr)
: : RLONG_ADDR(addr), "Ir" (nr) : "memory");
}
}
+#define arch_set_bit arch_set_bit
-/**
- * __set_bit - Set a bit in memory
- * @nr: the bit to set
- * @addr: the address to start counting from
- *
- * Unlike set_bit(), this function is non-atomic and may be reordered.
- * If it's called on the same region of memory simultaneously, the effect
- * may be that only one operation succeeds.
- */
-static __always_inline void __set_bit(long nr, volatile unsigned long *addr)
+static __always_inline void
+arch___set_bit(long nr, volatile unsigned long *addr)
{
asm volatile(__ASM_SIZE(bts) " %1,%0" : : ADDR, "Ir" (nr) : "memory");
}
+#define arch___set_bit arch___set_bit
-/**
- * clear_bit - Clears a bit in memory
- * @nr: Bit to clear
- * @addr: Address to start counting from
- *
- * clear_bit() is atomic and may not be reordered. However, it does
- * not contain a memory barrier, so if it is used for locking purposes,
- * you should call smp_mb__before_atomic() and/or smp_mb__after_atomic()
- * in order to ensure changes are visible on other processors.
- */
static __always_inline void
-clear_bit(long nr, volatile unsigned long *addr)
+arch_clear_bit(long nr, volatile unsigned long *addr)
{
if (IS_IMMEDIATE(nr)) {
asm volatile(LOCK_PREFIX "andb %1,%0"
@@ -114,27 +83,25 @@ clear_bit(long nr, volatile unsigned long *addr)
: : RLONG_ADDR(addr), "Ir" (nr) : "memory");
}
}
+#define arch_clear_bit arch_clear_bit
-/*
- * clear_bit_unlock - Clears a bit in memory
- * @nr: Bit to clear
- * @addr: Address to start counting from
- *
- * clear_bit() is atomic and implies release semantics before the memory
- * operation. It can be used for an unlock.
- */
-static __always_inline void clear_bit_unlock(long nr, volatile unsigned long *addr)
+static __always_inline void
+arch_clear_bit_unlock(long nr, volatile unsigned long *addr)
{
barrier();
- clear_bit(nr, addr);
+ arch_clear_bit(nr, addr);
}
+#define arch_clear_bit_unlock arch_clear_bit_unlock
-static __always_inline void __clear_bit(long nr, volatile unsigned long *addr)
+static __always_inline void
+arch___clear_bit(long nr, volatile unsigned long *addr)
{
asm volatile(__ASM_SIZE(btr) " %1,%0" : : ADDR, "Ir" (nr) : "memory");
}
+#define arch___clear_bit arch___clear_bit
-static __always_inline bool clear_bit_unlock_is_negative_byte(long nr, volatile unsigned long *addr)
+static __always_inline bool
+arch_clear_bit_unlock_is_negative_byte(long nr, volatile unsigned long *addr)
{
bool negative;
asm volatile(LOCK_PREFIX "andb %2,%1"
@@ -143,48 +110,25 @@ static __always_inline bool clear_bit_unlock_is_negative_byte(long nr, volatile
: "ir" ((char) ~(1 << nr)) : "memory");
return negative;
}
+#define arch_clear_bit_unlock_is_negative_byte \
+ arch_clear_bit_unlock_is_negative_byte
-// Let everybody know we have it
-#define clear_bit_unlock_is_negative_byte clear_bit_unlock_is_negative_byte
-
-/*
- * __clear_bit_unlock - Clears a bit in memory
- * @nr: Bit to clear
- * @addr: Address to start counting from
- *
- * __clear_bit() is non-atomic and implies release semantics before the memory
- * operation. It can be used for an unlock if no other CPUs can concurrently
- * modify other bits in the word.
- */
-static __always_inline void __clear_bit_unlock(long nr, volatile unsigned long *addr)
+static __always_inline void
+arch___clear_bit_unlock(long nr, volatile unsigned long *addr)
{
- __clear_bit(nr, addr);
+ arch___clear_bit(nr, addr);
}
+#define arch___clear_bit_unlock arch___clear_bit_unlock
-/**
- * __change_bit - Toggle a bit in memory
- * @nr: the bit to change
- * @addr: the address to start counting from
- *
- * Unlike change_bit(), this function is non-atomic and may be reordered.
- * If it's called on the same region of memory simultaneously, the effect
- * may be that only one operation succeeds.
- */
-static __always_inline void __change_bit(long nr, volatile unsigned long *addr)
+static __always_inline void
+arch___change_bit(long nr, volatile unsigned long *addr)
{
asm volatile(__ASM_SIZE(btc) " %1,%0" : : ADDR, "Ir" (nr) : "memory");
}
+#define arch___change_bit arch___change_bit
-/**
- * change_bit - Toggle a bit in memory
- * @nr: Bit to change
- * @addr: Address to start counting from
- *
- * change_bit() is atomic and may not be reordered.
- * Note that @nr may be almost arbitrarily large; this function is not
- * restricted to acting on a single-word quantity.
- */
-static __always_inline void change_bit(long nr, volatile unsigned long *addr)
+static __always_inline void
+arch_change_bit(long nr, volatile unsigned long *addr)
{
if (IS_IMMEDIATE(nr)) {
asm volatile(LOCK_PREFIX "xorb %1,%0"
@@ -195,43 +139,24 @@ static __always_inline void change_bit(long nr, volatile unsigned long *addr)
: : RLONG_ADDR(addr), "Ir" (nr) : "memory");
}
}
+#define arch_change_bit arch_change_bit
-/**
- * test_and_set_bit - Set a bit and return its old value
- * @nr: Bit to set
- * @addr: Address to count from
- *
- * This operation is atomic and cannot be reordered.
- * It also implies a memory barrier.
- */
-static __always_inline bool test_and_set_bit(long nr, volatile unsigned long *addr)
+static __always_inline bool
+arch_test_and_set_bit(long nr, volatile unsigned long *addr)
{
return GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(bts), *addr, c, "Ir", nr);
}
+#define arch_test_and_set_bit arch_test_and_set_bit
-/**
- * test_and_set_bit_lock - Set a bit and return its old value for lock
- * @nr: Bit to set
- * @addr: Address to count from
- *
- * This is the same as test_and_set_bit on x86.
- */
static __always_inline bool
-test_and_set_bit_lock(long nr, volatile unsigned long *addr)
+arch_test_and_set_bit_lock(long nr, volatile unsigned long *addr)
{
- return test_and_set_bit(nr, addr);
+ return arch_test_and_set_bit(nr, addr);
}
+#define arch_test_and_set_bit_lock arch_test_and_set_bit_lock
-/**
- * __test_and_set_bit - Set a bit and return its old value
- * @nr: Bit to set
- * @addr: Address to count from
- *
- * This operation is non-atomic and can be reordered.
- * If two examples of this operation race, one can appear to succeed
- * but actually fail. You must protect multiple accesses with a lock.
- */
-static __always_inline bool __test_and_set_bit(long nr, volatile unsigned long *addr)
+static __always_inline bool
+arch___test_and_set_bit(long nr, volatile unsigned long *addr)
{
bool oldbit;
@@ -241,37 +166,17 @@ static __always_inline bool __test_and_set_bit(long nr, volatile unsigned long *
: ADDR, "Ir" (nr) : "memory");
return oldbit;
}
+#define arch___test_and_set_bit arch___test_and_set_bit
-/**
- * test_and_clear_bit - Clear a bit and return its old value
- * @nr: Bit to clear
- * @addr: Address to count from
- *
- * This operation is atomic and cannot be reordered.
- * It also implies a memory barrier.
- */
-static __always_inline bool test_and_clear_bit(long nr, volatile unsigned long *addr)
+static __always_inline bool
+arch_test_and_clear_bit(long nr, volatile unsigned long *addr)
{
return GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(btr), *addr, c, "Ir", nr);
}
+#define arch_test_and_clear_bit arch_test_and_clear_bit
-/**
- * __test_and_clear_bit - Clear a bit and return its old value
- * @nr: Bit to clear
- * @addr: Address to count from
- *
- * This operation is non-atomic and can be reordered.
- * If two examples of this operation race, one can appear to succeed
- * but actually fail. You must protect multiple accesses with a lock.
- *
- * Note: the operation is performed atomically with respect to
- * the local CPU, but not other CPUs. Portable code should not
- * rely on this behaviour.
- * KVM relies on this behaviour on x86 for modifying memory that is also
- * accessed from a hypervisor on the same CPU if running in a VM: don't change
- * this without also updating arch/x86/kernel/kvm.c
- */
-static __always_inline bool __test_and_clear_bit(long nr, volatile unsigned long *addr)
+static __always_inline bool
+arch___test_and_clear_bit(long nr, volatile unsigned long *addr)
{
bool oldbit;
@@ -281,9 +186,10 @@ static __always_inline bool __test_and_clear_bit(long nr, volatile unsigned long
: ADDR, "Ir" (nr) : "memory");
return oldbit;
}
+#define arch___test_and_clear_bit arch___test_and_clear_bit
-/* WARNING: non atomic and it can be reordered! */
-static __always_inline bool __test_and_change_bit(long nr, volatile unsigned long *addr)
+static __always_inline bool
+arch___test_and_change_bit(long nr, volatile unsigned long *addr)
{
bool oldbit;
@@ -294,19 +200,14 @@ static __always_inline bool __test_and_change_bit(long nr, volatile unsigned lon
return oldbit;
}
+#define arch___test_and_change_bit arch___test_and_change_bit
-/**
- * test_and_change_bit - Change a bit and return its old value
- * @nr: Bit to change
- * @addr: Address to count from
- *
- * This operation is atomic and cannot be reordered.
- * It also implies a memory barrier.
- */
-static __always_inline bool test_and_change_bit(long nr, volatile unsigned long *addr)
+static __always_inline bool
+arch_test_and_change_bit(long nr, volatile unsigned long *addr)
{
return GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(btc), *addr, c, "Ir", nr);
}
+#define arch_test_and_change_bit arch_test_and_change_bit
static __always_inline bool constant_test_bit(long nr, const volatile unsigned long *addr)
{
@@ -326,16 +227,7 @@ static __always_inline bool variable_test_bit(long nr, volatile const unsigned l
return oldbit;
}
-#if 0 /* Fool kernel-doc since it doesn't do macros yet */
-/**
- * test_bit - Determine whether a bit is set
- * @nr: bit number to test
- * @addr: Address to start counting from
- */
-static bool test_bit(int nr, const volatile unsigned long *addr);
-#endif
-
-#define test_bit(nr, addr) \
+#define arch_test_bit(nr, addr) \
(__builtin_constant_p((nr)) \
? constant_test_bit((nr), (addr)) \
: variable_test_bit((nr), (addr)))
@@ -504,6 +396,8 @@ static __always_inline int fls64(__u64 x)
#include <asm-generic/bitops/const_hweight.h>
+#include <asm-generic/bitops-instrumented.h>
+
#include <asm-generic/bitops/le.h>
#include <asm-generic/bitops/ext2-atomic-setbit.h>
diff --git a/include/asm-generic/bitops-instrumented.h b/include/asm-generic/bitops-instrumented.h
new file mode 100644
index 000000000000..b01b0dd93964
--- /dev/null
+++ b/include/asm-generic/bitops-instrumented.h
@@ -0,0 +1,317 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * This file provides wrappers with sanitizer instrumentation for bit
+ * operations.
+ *
+ * To use this functionality, an arch's bitops.h file needs to define each of
+ * the below bit operations with an arch_ prefix (e.g. arch_set_bit(),
+ * arch___set_bit(), etc.), #define each provided arch_ function, and include
+ * this file after their definitions. For undefined arch_ functions, it is
+ * assumed that they are provided via asm-generic/bitops, which are implicitly
+ * instrumented.
+ */
+#ifndef _ASM_GENERIC_BITOPS_INSTRUMENTED_H
+#define _ASM_GENERIC_BITOPS_INSTRUMENTED_H
+
+#include <linux/kasan-checks.h>
+
+#if defined(arch_set_bit)
+/**
+ * set_bit - Atomically set a bit in memory
+ * @nr: the bit to set
+ * @addr: the address to start counting from
+ *
+ * This function is atomic and may not be reordered. See __set_bit()
+ * if you do not require the atomic guarantees.
+ *
+ * Note: there are no guarantees that this function will not be reordered
+ * on non x86 architectures, so if you are writing portable code,
+ * make sure not to rely on its reordering guarantees.
+ *
+ * Note that @nr may be almost arbitrarily large; this function is not
+ * restricted to acting on a single-word quantity.
+ */
+static inline void set_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ arch_set_bit(nr, addr);
+}
+#endif
+
+#if defined(arch___set_bit)
+/**
+ * __set_bit - Set a bit in memory
+ * @nr: the bit to set
+ * @addr: the address to start counting from
+ *
+ * Unlike set_bit(), this function is non-atomic and may be reordered.
+ * If it's called on the same region of memory simultaneously, the effect
+ * may be that only one operation succeeds.
+ */
+static inline void __set_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ arch___set_bit(nr, addr);
+}
+#endif
+
+#if defined(arch_clear_bit)
+/**
+ * clear_bit - Clears a bit in memory
+ * @nr: Bit to clear
+ * @addr: Address to start counting from
+ *
+ * clear_bit() is atomic and may not be reordered. However, it does
+ * not contain a memory barrier, so if it is used for locking purposes,
+ * you should call smp_mb__before_atomic() and/or smp_mb__after_atomic()
+ * in order to ensure changes are visible on other processors.
+ */
+static inline void clear_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ arch_clear_bit(nr, addr);
+}
+#endif
+
+#if defined(arch___clear_bit)
+/**
+ * __clear_bit - Clears a bit in memory
+ * @nr: the bit to clear
+ * @addr: the address to start counting from
+ *
+ * Unlike clear_bit(), this function is non-atomic and may be reordered.
+ * If it's called on the same region of memory simultaneously, the effect
+ * may be that only one operation succeeds.
+ */
+static inline void __clear_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ arch___clear_bit(nr, addr);
+}
+#endif
+
+#if defined(arch_clear_bit_unlock)
+/**
+ * clear_bit_unlock - Clears a bit in memory
+ * @nr: Bit to clear
+ * @addr: Address to start counting from
+ *
+ * clear_bit_unlock() is atomic and implies release semantics before the memory
+ * operation. It can be used for an unlock.
+ */
+static inline void clear_bit_unlock(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ arch_clear_bit_unlock(nr, addr);
+}
+#endif
+
+#if defined(arch___clear_bit_unlock)
+/**
+ * __clear_bit_unlock - Clears a bit in memory
+ * @nr: Bit to clear
+ * @addr: Address to start counting from
+ *
+ * __clear_bit_unlock() is non-atomic and implies release semantics before the
+ * memory operation. It can be used for an unlock if no other CPUs can
+ * concurrently modify other bits in the word.
+ */
+static inline void __clear_bit_unlock(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ arch___clear_bit_unlock(nr, addr);
+}
+#endif
+
+#if defined(arch_change_bit)
+/**
+ * change_bit - Toggle a bit in memory
+ * @nr: Bit to change
+ * @addr: Address to start counting from
+ *
+ * change_bit() is atomic and may not be reordered.
+ * Note that @nr may be almost arbitrarily large; this function is not
+ * restricted to acting on a single-word quantity.
+ */
+static inline void change_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ arch_change_bit(nr, addr);
+}
+#endif
+
+#if defined(arch___change_bit)
+/**
+ * __change_bit - Toggle a bit in memory
+ * @nr: the bit to change
+ * @addr: the address to start counting from
+ *
+ * Unlike change_bit(), this function is non-atomic and may be reordered.
+ * If it's called on the same region of memory simultaneously, the effect
+ * may be that only one operation succeeds.
+ */
+static inline void __change_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ arch___change_bit(nr, addr);
+}
+#endif
+
+#if defined(arch_test_and_set_bit)
+/**
+ * test_and_set_bit - Set a bit and return its old value
+ * @nr: Bit to set
+ * @addr: Address to count from
+ *
+ * This operation is atomic and cannot be reordered.
+ * It also implies a memory barrier.
+ */
+static inline bool test_and_set_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ return arch_test_and_set_bit(nr, addr);
+}
+#endif
+
+#if defined(arch___test_and_set_bit)
+/**
+ * __test_and_set_bit - Set a bit and return its old value
+ * @nr: Bit to set
+ * @addr: Address to count from
+ *
+ * This operation is non-atomic and can be reordered.
+ * If two examples of this operation race, one can appear to succeed
+ * but actually fail. You must protect multiple accesses with a lock.
+ */
+static inline bool __test_and_set_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ return arch___test_and_set_bit(nr, addr);
+}
+#endif
+
+#if defined(arch_test_and_set_bit_lock)
+/**
+ * test_and_set_bit_lock - Set a bit and return its old value, for lock
+ * @nr: Bit to set
+ * @addr: Address to count from
+ *
+ * This operation is atomic and provides acquire barrier semantics if
+ * the returned value is 0.
+ * It can be used to implement bit locks.
+ */
+static inline bool test_and_set_bit_lock(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ return arch_test_and_set_bit_lock(nr, addr);
+}
+#endif
+
+#if defined(arch_test_and_clear_bit)
+/**
+ * test_and_clear_bit - Clear a bit and return its old value
+ * @nr: Bit to clear
+ * @addr: Address to count from
+ *
+ * This operation is atomic and cannot be reordered.
+ * It also implies a memory barrier.
+ */
+static inline bool test_and_clear_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ return arch_test_and_clear_bit(nr, addr);
+}
+#endif
+
+#if defined(arch___test_and_clear_bit)
+/**
+ * __test_and_clear_bit - Clear a bit and return its old value
+ * @nr: Bit to clear
+ * @addr: Address to count from
+ *
+ * This operation is non-atomic and can be reordered.
+ * If two examples of this operation race, one can appear to succeed
+ * but actually fail. You must protect multiple accesses with a lock.
+ *
+ * Note: the operation is performed atomically with respect to
+ * the local CPU, but not other CPUs. Portable code should not
+ * rely on this behaviour.
+ * KVM relies on this behaviour on x86 for modifying memory that is also
+ * accessed from a hypervisor on the same CPU if running in a VM: don't change
+ * this without also updating arch/x86/kernel/kvm.c
+ */
+static inline bool __test_and_clear_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ return arch___test_and_clear_bit(nr, addr);
+}
+#endif
+
+#if defined(arch_test_and_change_bit)
+/**
+ * test_and_change_bit - Change a bit and return its old value
+ * @nr: Bit to change
+ * @addr: Address to count from
+ *
+ * This operation is atomic and cannot be reordered.
+ * It also implies a memory barrier.
+ */
+static inline bool test_and_change_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ return arch_test_and_change_bit(nr, addr);
+}
+#endif
+
+#if defined(arch___test_and_change_bit)
+/**
+ * __test_and_change_bit - Change a bit and return its old value
+ * @nr: Bit to change
+ * @addr: Address to count from
+ *
+ * This operation is non-atomic and can be reordered.
+ * If two examples of this operation race, one can appear to succeed
+ * but actually fail. You must protect multiple accesses with a lock.
+ */
+static inline bool __test_and_change_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ return arch___test_and_change_bit(nr, addr);
+}
+#endif
+
+#if defined(arch_test_bit)
+/**
+ * test_bit - Determine whether a bit is set
+ * @nr: bit number to test
+ * @addr: Address to start counting from
+ */
+static inline bool test_bit(long nr, const volatile unsigned long *addr)
+{
+ kasan_check_read(addr + BIT_WORD(nr), sizeof(long));
+ return arch_test_bit(nr, addr);
+}
+#endif
+
+#if defined(arch_clear_bit_unlock_is_negative_byte)
+/**
+ * clear_bit_unlock_is_negative_byte - Clear a bit in memory and test if bottom
+ * byte is negative, for unlock.
+ * @nr: the bit to clear
+ * @addr: the address to start counting from
+ *
+ * This is a bit of a one-trick-pony for the filemap code, which clears
+ * PG_locked and tests PG_waiters,
+ */
+static inline bool
+clear_bit_unlock_is_negative_byte(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ return arch_clear_bit_unlock_is_negative_byte(nr, addr);
+}
+/* Let everybody know we have it. */
+#define clear_bit_unlock_is_negative_byte clear_bit_unlock_is_negative_byte
+#endif
+
+#endif /* _ASM_GENERIC_BITOPS_INSTRUMENTED_H */
--
2.22.0.rc1.257.g3120a18244-goog
^ permalink raw reply related
* Re: [PATCH 2/3] x86: Move CPU feature test out of uaccess region
From: hpa @ 2019-05-29 14:29 UTC (permalink / raw)
To: Marco Elver, peterz, aryabinin, dvyukov, glider, andreyknvl,
mark.rutland
Cc: corbet, tglx, mingo, bp, x86, arnd, jpoimboe, linux-doc,
linux-kernel, linux-arch, kasan-dev
In-Reply-To: <20190529141500.193390-3-elver@google.com>
On May 29, 2019 7:15:00 AM PDT, Marco Elver <elver@google.com> wrote:
>This patch is a pre-requisite for enabling KASAN bitops
>instrumentation:
>moves boot_cpu_has feature test out of the uaccess region, as
>boot_cpu_has uses test_bit. With instrumentation, the KASAN check would
>otherwise be flagged by objtool.
>
>This approach is preferred over adding the explicit kasan_check_*
>functions to the uaccess whitelist of objtool, as the case here appears
>to be the only one.
>
>Signed-off-by: Marco Elver <elver@google.com>
>---
>v1:
>* This patch replaces patch: 'tools/objtool: add kasan_check_* to
> uaccess whitelist'
>---
> arch/x86/ia32/ia32_signal.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
>diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
>index 629d1ee05599..12264e3c9c43 100644
>--- a/arch/x86/ia32/ia32_signal.c
>+++ b/arch/x86/ia32/ia32_signal.c
>@@ -333,6 +333,7 @@ int ia32_setup_rt_frame(int sig, struct ksignal
>*ksig,
> void __user *restorer;
> int err = 0;
> void __user *fpstate = NULL;
>+ bool has_xsave;
>
> /* __copy_to_user optimizes that into a single 8 byte store */
> static const struct {
>@@ -352,13 +353,19 @@ int ia32_setup_rt_frame(int sig, struct ksignal
>*ksig,
> if (!access_ok(frame, sizeof(*frame)))
> return -EFAULT;
>
>+ /*
>+ * Move non-uaccess accesses out of uaccess region if not strictly
>+ * required; this also helps avoid objtool flagging these accesses
>with
>+ * instrumentation enabled.
>+ */
>+ has_xsave = boot_cpu_has(X86_FEATURE_XSAVE);
> put_user_try {
> put_user_ex(sig, &frame->sig);
> put_user_ex(ptr_to_compat(&frame->info), &frame->pinfo);
> put_user_ex(ptr_to_compat(&frame->uc), &frame->puc);
>
> /* Create the ucontext. */
>- if (boot_cpu_has(X86_FEATURE_XSAVE))
>+ if (has_xsave)
> put_user_ex(UC_FP_XSTATE, &frame->uc.uc_flags);
> else
> put_user_ex(0, &frame->uc.uc_flags);
This was meant to use static_cpu_has(). Why did that get dropped?
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* Re: [PATCH] ftrace: add simple oneshot function tracer
From: Steven Rostedt @ 2019-05-29 14:45 UTC (permalink / raw)
To: Thomas Preisner; +Cc: Ingo Molnar, Jonathan Corbet, linux-doc, linux-kernel
In-Reply-To: <20190529093124.2872-1-linux@tpreisner.de>
On Wed, 29 May 2019 11:31:23 +0200
Thomas Preisner <linux@tpreisner.de> wrote:
> The "oneshot" tracer records every address (ip, parent_ip) exactly once.
> As a result, "oneshot" can be used to efficiently create kernel function
> coverage/usage reports such as in undertaker-tailor[0].
>
> In order to provide this functionality, "oneshot" uses a
> configurable hashset for blacklisting already recorded addresses. This
> way, no user space application is required to parse the function
> tracer's output and to deactivate functions after they have been
> recorded once. Additionally, the tracer's output is reduced to a bare
> mininum so that it can be passed directly to undertaker-tailor.
>
> Further information regarding this oneshot function tracer can also be
> found at [1].
>
> [0]: https://undertaker.cs.fau.de
> [1]: https://tpreisner.de/pub/ba-thesis.pdf
>
> Signed-off-by: Thomas Preisner <linux@tpreisner.de>
>
Hi,
If you are only interested in seeing what functions are called (and
don't care about the order), why not just make another function
profiler (see register_ftrace_profiler and friends)? Then you could
just list the hash table entries instead of having to record into the
ftrace ring buffer.
-- Steve
^ permalink raw reply
* Re: [PATCH v2 1/3] lib/test_kasan: Add bitops tests
From: Mark Rutland @ 2019-05-29 15:15 UTC (permalink / raw)
To: Marco Elver
Cc: peterz, aryabinin, dvyukov, glider, andreyknvl, corbet, tglx,
mingo, bp, hpa, x86, arnd, jpoimboe, linux-doc, linux-kernel,
linux-arch, kasan-dev
In-Reply-To: <20190529141500.193390-2-elver@google.com>
On Wed, May 29, 2019 at 04:14:59PM +0200, Marco Elver wrote:
> This adds bitops tests to the test_kasan module. In a follow-up patch,
> support for bitops instrumentation will be added.
>
> Signed-off-by: Marco Elver <elver@google.com>
> ---
> Changes in v2:
> * Use BITS_PER_LONG.
> * Use heap allocated memory for test, as newer compilers (correctly)
> warn on OOB stack access.
> ---
> lib/test_kasan.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 72 insertions(+), 3 deletions(-)
>
> diff --git a/lib/test_kasan.c b/lib/test_kasan.c
> index 7de2702621dc..6562df0ca30d 100644
> --- a/lib/test_kasan.c
> +++ b/lib/test_kasan.c
> @@ -11,16 +11,17 @@
>
> #define pr_fmt(fmt) "kasan test: %s " fmt, __func__
>
> +#include <linux/bitops.h>
> #include <linux/delay.h>
> +#include <linux/kasan.h>
> #include <linux/kernel.h>
> -#include <linux/mman.h>
> #include <linux/mm.h>
> +#include <linux/mman.h>
> +#include <linux/module.h>
> #include <linux/printk.h>
> #include <linux/slab.h>
> #include <linux/string.h>
> #include <linux/uaccess.h>
> -#include <linux/module.h>
> -#include <linux/kasan.h>
>
> /*
> * Note: test functions are marked noinline so that their names appear in
> @@ -623,6 +624,73 @@ static noinline void __init kasan_strings(void)
> strnlen(ptr, 1);
> }
>
> +static noinline void __init kasan_bitops(void)
> +{
> + long *bits = kmalloc(sizeof(long), GFP_KERNEL | __GFP_ZERO);
Trivial nit, but this can/should be:
long *bits = kzalloc(sizeof(*bits), GFP_KERNEL);
... which is the usual style for sizeof() to keep the LHS and RHS types
the same, and using kzalloc avoids the need to explicitly pass
__GFP_ZERO.
Otherwise, this looks good to me.
> + if (!bits)
> + return;
> +
> + pr_info("within-bounds in set_bit");
> + set_bit(0, bits);
> +
> + pr_info("within-bounds in set_bit");
> + set_bit(BITS_PER_LONG - 1, bits);
> +
> + pr_info("out-of-bounds in set_bit\n");
> + set_bit(BITS_PER_LONG, bits);
> +
> + pr_info("out-of-bounds in __set_bit\n");
> + __set_bit(BITS_PER_LONG, bits);
> +
> + pr_info("out-of-bounds in clear_bit\n");
> + clear_bit(BITS_PER_LONG, bits);
> +
> + pr_info("out-of-bounds in __clear_bit\n");
> + __clear_bit(BITS_PER_LONG, bits);
> +
> + pr_info("out-of-bounds in clear_bit_unlock\n");
> + clear_bit_unlock(BITS_PER_LONG, bits);
> +
> + pr_info("out-of-bounds in __clear_bit_unlock\n");
> + __clear_bit_unlock(BITS_PER_LONG, bits);
> +
> + pr_info("out-of-bounds in change_bit\n");
> + change_bit(BITS_PER_LONG, bits);
> +
> + pr_info("out-of-bounds in __change_bit\n");
> + __change_bit(BITS_PER_LONG, bits);
> +
> + pr_info("out-of-bounds in test_and_set_bit\n");
> + test_and_set_bit(BITS_PER_LONG, bits);
> +
> + pr_info("out-of-bounds in __test_and_set_bit\n");
> + __test_and_set_bit(BITS_PER_LONG, bits);
> +
> + pr_info("out-of-bounds in test_and_set_bit_lock\n");
> + test_and_set_bit_lock(BITS_PER_LONG, bits);
> +
> + pr_info("out-of-bounds in test_and_clear_bit\n");
> + test_and_clear_bit(BITS_PER_LONG, bits);
> +
> + pr_info("out-of-bounds in __test_and_clear_bit\n");
> + __test_and_clear_bit(BITS_PER_LONG, bits);
> +
> + pr_info("out-of-bounds in test_and_change_bit\n");
> + test_and_change_bit(BITS_PER_LONG, bits);
> +
> + pr_info("out-of-bounds in __test_and_change_bit\n");
> + __test_and_change_bit(BITS_PER_LONG, bits);
> +
> + pr_info("out-of-bounds in test_bit\n");
> + (void)test_bit(BITS_PER_LONG, bits);
> +
> +#if defined(clear_bit_unlock_is_negative_byte)
> + pr_info("out-of-bounds in clear_bit_unlock_is_negative_byte\n");
> + clear_bit_unlock_is_negative_byte(BITS_PER_LONG, bits);
> +#endif
> + kfree(bits);
> +}
> +
> static int __init kmalloc_tests_init(void)
> {
> /*
> @@ -664,6 +732,7 @@ static int __init kmalloc_tests_init(void)
> kasan_memchr();
> kasan_memcmp();
> kasan_strings();
> + kasan_bitops();
>
> kasan_restore_multi_shot(multishot);
>
> --
> 2.22.0.rc1.257.g3120a18244-goog
>
^ permalink raw reply
* Re: [PATCH v2 3/3] asm-generic, x86: Add bitops instrumentation for KASAN
From: Mark Rutland @ 2019-05-29 15:32 UTC (permalink / raw)
To: Marco Elver, peterz
Cc: aryabinin, dvyukov, glider, andreyknvl, corbet, tglx, mingo, bp,
hpa, x86, arnd, jpoimboe, linux-doc, linux-kernel, linux-arch,
kasan-dev
In-Reply-To: <20190529141500.193390-4-elver@google.com>
On Wed, May 29, 2019 at 04:15:01PM +0200, Marco Elver wrote:
> This adds a new header to asm-generic to allow optionally instrumenting
> architecture-specific asm implementations of bitops.
>
> This change includes the required change for x86 as reference and
> changes the kernel API doc to point to bitops-instrumented.h instead.
> Rationale: the functions in x86's bitops.h are no longer the kernel API
> functions, but instead the arch_ prefixed functions, which are then
> instrumented via bitops-instrumented.h.
>
> Other architectures can similarly add support for asm implementations of
> bitops.
>
> The documentation text has been copied/moved, and *no* changes to it
> have been made in this patch.
>
> Tested: using lib/test_kasan with bitops tests (pre-requisite patch).
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=198439
> Signed-off-by: Marco Elver <elver@google.com>
> ---
> Changes in v2:
> * Instrument word-sized accesses, as specified by the interface.
> ---
> Documentation/core-api/kernel-api.rst | 2 +-
> arch/x86/include/asm/bitops.h | 210 ++++----------
> include/asm-generic/bitops-instrumented.h | 317 ++++++++++++++++++++++
> 3 files changed, 370 insertions(+), 159 deletions(-)
> create mode 100644 include/asm-generic/bitops-instrumented.h
[...]
> diff --git a/include/asm-generic/bitops-instrumented.h b/include/asm-generic/bitops-instrumented.h
> new file mode 100644
> index 000000000000..b01b0dd93964
> --- /dev/null
> +++ b/include/asm-generic/bitops-instrumented.h
> @@ -0,0 +1,317 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +/*
> + * This file provides wrappers with sanitizer instrumentation for bit
> + * operations.
> + *
> + * To use this functionality, an arch's bitops.h file needs to define each of
> + * the below bit operations with an arch_ prefix (e.g. arch_set_bit(),
> + * arch___set_bit(), etc.), #define each provided arch_ function, and include
> + * this file after their definitions. For undefined arch_ functions, it is
> + * assumed that they are provided via asm-generic/bitops, which are implicitly
> + * instrumented.
> + */
If using the asm-generic/bitops.h, all of the below will be defined
unconditionally, so I don't believe we need the ifdeffery for each
function.
> +#ifndef _ASM_GENERIC_BITOPS_INSTRUMENTED_H
> +#define _ASM_GENERIC_BITOPS_INSTRUMENTED_H
> +
> +#include <linux/kasan-checks.h>
> +
> +#if defined(arch_set_bit)
> +/**
> + * set_bit - Atomically set a bit in memory
> + * @nr: the bit to set
> + * @addr: the address to start counting from
> + *
> + * This function is atomic and may not be reordered. See __set_bit()
> + * if you do not require the atomic guarantees.
> + *
> + * Note: there are no guarantees that this function will not be reordered
> + * on non x86 architectures, so if you are writing portable code,
> + * make sure not to rely on its reordering guarantees.
These two paragraphs are contradictory.
Since this is not under arch/x86, please fix this to describe the
generic semantics; any x86-specific behaviour should be commented under
arch/x86.
AFAICT per include/asm-generic/bitops/atomic.h, generically this
provides no ordering guarantees. So I think this can be:
/**
* set_bit - Atomically set a bit in memory
* @nr: the bit to set
* @addr: the address to start counting from
*
* This function is atomic and may be reordered.
*
* Note that @nr may be almost arbitrarily large; this function is not
* restricted to acting on a single-word quantity.
*/
... with the x86 ordering beahviour commented in x86's arch_set_bit.
Peter, do you have a better wording for the above?
[...]
> +#if defined(arch___test_and_clear_bit)
> +/**
> + * __test_and_clear_bit - Clear a bit and return its old value
> + * @nr: Bit to clear
> + * @addr: Address to count from
> + *
> + * This operation is non-atomic and can be reordered.
> + * If two examples of this operation race, one can appear to succeed
> + * but actually fail. You must protect multiple accesses with a lock.
> + *
> + * Note: the operation is performed atomically with respect to
> + * the local CPU, but not other CPUs. Portable code should not
> + * rely on this behaviour.
> + * KVM relies on this behaviour on x86 for modifying memory that is also
> + * accessed from a hypervisor on the same CPU if running in a VM: don't change
> + * this without also updating arch/x86/kernel/kvm.c
> + */
Likewise, please only specify the generic semantics in this header, and
leave the x86-specific behaviour commented under arch/x86.
Otherwise this looks sound to me.
Thanks,
Mark.
^ permalink raw reply
* Re: [PATCH v2 3/3] asm-generic, x86: Add bitops instrumentation for KASAN
From: Marco Elver @ 2019-05-29 15:40 UTC (permalink / raw)
To: Mark Rutland
Cc: peterz, Andrey Ryabinin, Dmitry Vyukov, Alexander Potapenko,
Andrey Konovalov, corbet, tglx, mingo, bp, hpa, x86, arnd,
jpoimboe, linux-doc, LKML, linux-arch, kasan-dev
In-Reply-To: <20190529153258.GJ31777@lakrids.cambridge.arm.com>
On Wed, 29 May 2019 at 17:33, Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Wed, May 29, 2019 at 04:15:01PM +0200, Marco Elver wrote:
> > This adds a new header to asm-generic to allow optionally instrumenting
> > architecture-specific asm implementations of bitops.
> >
> > This change includes the required change for x86 as reference and
> > changes the kernel API doc to point to bitops-instrumented.h instead.
> > Rationale: the functions in x86's bitops.h are no longer the kernel API
> > functions, but instead the arch_ prefixed functions, which are then
> > instrumented via bitops-instrumented.h.
> >
> > Other architectures can similarly add support for asm implementations of
> > bitops.
> >
> > The documentation text has been copied/moved, and *no* changes to it
> > have been made in this patch.
> >
> > Tested: using lib/test_kasan with bitops tests (pre-requisite patch).
> >
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=198439
> > Signed-off-by: Marco Elver <elver@google.com>
> > ---
> > Changes in v2:
> > * Instrument word-sized accesses, as specified by the interface.
> > ---
> > Documentation/core-api/kernel-api.rst | 2 +-
> > arch/x86/include/asm/bitops.h | 210 ++++----------
> > include/asm-generic/bitops-instrumented.h | 317 ++++++++++++++++++++++
> > 3 files changed, 370 insertions(+), 159 deletions(-)
> > create mode 100644 include/asm-generic/bitops-instrumented.h
>
> [...]
>
> > diff --git a/include/asm-generic/bitops-instrumented.h b/include/asm-generic/bitops-instrumented.h
> > new file mode 100644
> > index 000000000000..b01b0dd93964
> > --- /dev/null
> > +++ b/include/asm-generic/bitops-instrumented.h
> > @@ -0,0 +1,317 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +/*
> > + * This file provides wrappers with sanitizer instrumentation for bit
> > + * operations.
> > + *
> > + * To use this functionality, an arch's bitops.h file needs to define each of
> > + * the below bit operations with an arch_ prefix (e.g. arch_set_bit(),
> > + * arch___set_bit(), etc.), #define each provided arch_ function, and include
> > + * this file after their definitions. For undefined arch_ functions, it is
> > + * assumed that they are provided via asm-generic/bitops, which are implicitly
> > + * instrumented.
> > + */
>
> If using the asm-generic/bitops.h, all of the below will be defined
> unconditionally, so I don't believe we need the ifdeffery for each
> function.
>
> > +#ifndef _ASM_GENERIC_BITOPS_INSTRUMENTED_H
> > +#define _ASM_GENERIC_BITOPS_INSTRUMENTED_H
> > +
> > +#include <linux/kasan-checks.h>
> > +
> > +#if defined(arch_set_bit)
> > +/**
> > + * set_bit - Atomically set a bit in memory
> > + * @nr: the bit to set
> > + * @addr: the address to start counting from
> > + *
> > + * This function is atomic and may not be reordered. See __set_bit()
> > + * if you do not require the atomic guarantees.
> > + *
> > + * Note: there are no guarantees that this function will not be reordered
> > + * on non x86 architectures, so if you are writing portable code,
> > + * make sure not to rely on its reordering guarantees.
>
> These two paragraphs are contradictory.
>
> Since this is not under arch/x86, please fix this to describe the
> generic semantics; any x86-specific behaviour should be commented under
> arch/x86.
>
> AFAICT per include/asm-generic/bitops/atomic.h, generically this
> provides no ordering guarantees. So I think this can be:
>
> /**
> * set_bit - Atomically set a bit in memory
> * @nr: the bit to set
> * @addr: the address to start counting from
> *
> * This function is atomic and may be reordered.
> *
> * Note that @nr may be almost arbitrarily large; this function is not
> * restricted to acting on a single-word quantity.
> */
>
> ... with the x86 ordering beahviour commented in x86's arch_set_bit.
>
> Peter, do you have a better wording for the above?
>
> [...]
>
> > +#if defined(arch___test_and_clear_bit)
> > +/**
> > + * __test_and_clear_bit - Clear a bit and return its old value
> > + * @nr: Bit to clear
> > + * @addr: Address to count from
> > + *
> > + * This operation is non-atomic and can be reordered.
> > + * If two examples of this operation race, one can appear to succeed
> > + * but actually fail. You must protect multiple accesses with a lock.
> > + *
> > + * Note: the operation is performed atomically with respect to
> > + * the local CPU, but not other CPUs. Portable code should not
> > + * rely on this behaviour.
> > + * KVM relies on this behaviour on x86 for modifying memory that is also
> > + * accessed from a hypervisor on the same CPU if running in a VM: don't change
> > + * this without also updating arch/x86/kernel/kvm.c
> > + */
>
> Likewise, please only specify the generic semantics in this header, and
> leave the x86-specific behaviour commented under arch/x86.
The current official API documentation refers to x86 bitops.h (also
see the Documentation/core-api/kernel-api.rst change):
https://www.kernel.org/doc/htmldocs/kernel-api/API-set-bit.html
I'm happy to change in this patch, but note that this would change the
official API documentation. Alternatively it could be done in a
separate patch.
Let me know what you prefer.
Thanks,
-- Marco
^ permalink raw reply
* [GIT PULL] Fixes for sphinx 2.0 docs build failures
From: Jonathan Corbet @ 2019-05-29 15:54 UTC (permalink / raw)
To: Linus Torvalds; +Cc: LKML, linux-doc
The following changes since commit
a65fd4f0def56f59822b2c49522d36319bc8da8b:
Documentation: kdump: fix minor typo (2019-05-21 09:31:28 -0600)
are available in the Git repository at:
git://git.lwn.net/linux.git tags/docs-5.2-fixes2
for you to fetch changes up to 551bd3368a7b3cfef01edaade8970948d178d40a:
drm/i915: Maintain consistent documentation subsection ordering (2019-05-24 09:15:45 -0600)
----------------------------------------------------------------
The Sphinx 2.0 release contained a few incompatible API changes that broke
our extensions and, thus, the documentation build in general. Who knew
that those deprecation warnings it was outputting actually meant we should
change something? This set of fixes makes the build work again with
Sphinx 2.0 and eliminates the warnings for 1.8. As part of that, we also
need a few fixes to the docs for places where the new Sphinx is more
strict.
It is a bit late in the cycle for this kind of change, but it does fix
problems that people are experiencing now.
There has been some talk of raising the minimum version of Sphinx we
support. I don't want to do that abruptly, though, so these changes add
some glue to continue to support versions back to 1.3. We will be adding
some infrastructure soon to nudge users of old versions forward, with the
idea of maybe increasing our minimum version (and removing this glue)
sometime in the future.
----------------------------------------------------------------
Jonathan Corbet (7):
doc: Cope with Sphinx logging deprecations
doc: Cope with the deprecation of AutoReporter
docs: fix numaperf.rst and add it to the doc tree
lib/list_sort: fix kerneldoc build error
docs: fix multiple doc build warnings in enumeration.rst
docs: Fix conf.py for Sphinx 2.0
drm/i915: Maintain consistent documentation subsection ordering
Mauro Carvalho Chehab (1):
scripts/sphinx-pre-install: make it handle Sphinx versions
Documentation/admin-guide/mm/index.rst | 1 +
Documentation/admin-guide/mm/numaperf.rst | 2 +-
Documentation/conf.py | 2 +-
Documentation/firmware-guide/acpi/enumeration.rst | 2 +-
Documentation/sphinx/kerneldoc.py | 44 +++++++++---
Documentation/sphinx/kernellog.py | 28 ++++++++
Documentation/sphinx/kfigure.py | 40 ++++++-----
drivers/gpu/drm/i915/i915_reg.h | 6 +-
drivers/gpu/drm/i915/intel_workarounds.c | 2 +-
lib/list_sort.c | 3 +-
scripts/sphinx-pre-install | 86 +++++++++++++++++++++--
11 files changed, 173 insertions(+), 43 deletions(-)
create mode 100644 Documentation/sphinx/kernellog.py
^ permalink raw reply
* [PATCH v3 1/1] sched/fair: Fix low cpu usage with high throttling by removing expiration of cpu-local slices
From: Dave Chiluk @ 2019-05-29 19:08 UTC (permalink / raw)
To: Ben Segall, Phil Auld, Peter Oskolkov, Peter Zijlstra,
Ingo Molnar, cgroups, linux-kernel, Brendan Gregg, Kyle Anderson,
Gabriel Munos, John Hammond, Cong Wang, Jonathan Corbet,
linux-doc
In-Reply-To: <1559156926-31336-1-git-send-email-chiluk+linux@indeed.com>
It has been observed, that highly-threaded, non-cpu-bound applications
running under cpu.cfs_quota_us constraints can hit a high percentage of
periods throttled while simultaneously not consuming the allocated
amount of quota. This use case is typical of user-interactive non-cpu
bound applications, such as those running in kubernetes or mesos when
run on multiple cpu cores.
This has been root caused to threads being allocated per cpu bandwidth
slices, and then not fully using that slice within the period. At which
point the slice and quota expires. This expiration of unused slice
results in applications not being able to utilize the quota for which
they are allocated.
The expiration of per-cpu slices was recently fixed by
'commit 512ac999d275 ("sched/fair: Fix bandwidth timer clock drift
condition")'. Prior to that it appears that this has been broken since
at least 'commit 51f2176d74ac ("sched/fair: Fix unlocked reads of some
cfs_b->quota/period")' which was introduced in v3.16-rc1 in 2014. That
added the following conditional which resulted in slices never being
expired.
if (cfs_rq->runtime_expires != cfs_b->runtime_expires) {
/* extend local deadline, drift is bounded above by 2 ticks */
cfs_rq->runtime_expires += TICK_NSEC;
Because this was broken for nearly 5 years, and has recently been fixed
and is now being noticed by many users running kubernetes
(https://github.com/kubernetes/kubernetes/issues/67577) it is my opinion
that the mechanisms around expiring runtime should be removed
altogether.
This allows only per-cpu slices to live longer than the period boundary.
This allows threads on runqueues that do not use much CPU to continue to
use their remaining slice over a longer period of time than
cpu.cfs_period_us. However, this helps prevents the above condition of
hitting throttling while also not fully utilizing your cpu quota.
This theoretically allows a machine to use slightly more than it's
allotted quota in some periods. This overflow would be bounded by the
remaining per-cpu slice that was left un-used in the previous period.
For CPU bound tasks this will change nothing, as they should
theoretically fully utilize all of their quota and slices in each
period. For user-interactive tasks as described above this provides a
much better user/application experience as their cpu utilization will
more closely match the amount they requested when they hit throttling.
This greatly improves performance of high-thread-count, non-cpu bound
applications with low cfs_quota_us allocation on high-core-count
machines. In the case of an artificial testcase, this performance
discrepancy has been observed to be almost 30x performance improvement,
while still maintaining correct cpu quota restrictions albeit over
longer time intervals than cpu.cfs_period_us. That testcase is
available at https://github.com/indeedeng/fibtest.
Fixes: 512ac999d275 ("sched/fair: Fix bandwidth timer clock drift condition")
Signed-off-by: Dave Chiluk <chiluk+linux@indeed.com>
---
Documentation/scheduler/sched-bwc.txt | 56 ++++++++++++++++++++++-----
kernel/sched/fair.c | 71 +++--------------------------------
kernel/sched/sched.h | 4 --
3 files changed, 53 insertions(+), 78 deletions(-)
diff --git a/Documentation/scheduler/sched-bwc.txt b/Documentation/scheduler/sched-bwc.txt
index f6b1873..260fd65 100644
--- a/Documentation/scheduler/sched-bwc.txt
+++ b/Documentation/scheduler/sched-bwc.txt
@@ -8,15 +8,16 @@ CFS bandwidth control is a CONFIG_FAIR_GROUP_SCHED extension which allows the
specification of the maximum CPU bandwidth available to a group or hierarchy.
The bandwidth allowed for a group is specified using a quota and period. Within
-each given "period" (microseconds), a group is allowed to consume only up to
-"quota" microseconds of CPU time. When the CPU bandwidth consumption of a
-group exceeds this limit (for that period), the tasks belonging to its
-hierarchy will be throttled and are not allowed to run again until the next
-period.
-
-A group's unused runtime is globally tracked, being refreshed with quota units
-above at each period boundary. As threads consume this bandwidth it is
-transferred to cpu-local "silos" on a demand basis. The amount transferred
+each given "period" (microseconds), a task group is allocated up to "quota"
+microseconds of CPU time. That quota is assigned to per cpu run queues in
+slices as threads in the cgroup become runnable. Once all quota has been
+assigned any additional requests for quota will result in those threads being
+throttled. Throttled threads will not be able to run again until the next
+period when the quota is replenished.
+
+A group's unassigned quota is globally tracked, being refreshed back to
+cfs_quota units at each period boundary. As threads consume this bandwidth it
+is transferred to cpu-local "silos" on a demand basis. The amount transferred
within each of these updates is tunable and described as the "slice".
Management
@@ -90,6 +91,43 @@ There are two ways in which a group may become throttled:
In case b) above, even though the child may have runtime remaining it will not
be allowed to until the parent's runtime is refreshed.
+Real-world behavior of slice non-expiration
+-------------------------------------------
+The fact that cpu-local slices do not expire results in some interesting corner
+cases that should be understood.
+
+For cgroup cpu constrained applications that are cpu limited this is a
+relatively moot point because they will naturally consume the entirety of their
+quota as well as the entirety of each cpu-local slice in each period. As a
+result it is expected that nr_periods roughly equal nr_throttled, and that
+cpuacct.usage will increase roughly equal to cfs_quota_us in each period.
+
+However in a worst-case scenario, highly-threaded, interactive/non-cpu bound
+applications this non-expiration nuance allows applications to briefly burst
+past their quota limits by the amount of unused slice on each cpu that the task
+group is running on. This slight burst requires that quota had been assigned
+and then not fully used in previous periods. This burst amount will not be
+transferred between cores. As a result, this mechanism still strictly limits
+the task group to quota average usage, albeit over a longer time window than
+period. This provides better more predictable user experience for highly
+threaded applications with small quota limits on high core count machines. It
+also eliminates the propensity to throttle these applications while
+simultanously using less than quota amounts of cpu. Another way to say this,
+is that by allowing the unused portion of a slice to remain valid across
+periods we have decreased the possibility of wasting quota on cpu-local silos
+that don't need a full slice's amount of cpu time.
+
+The interaction between cpu-bound and non-cpu-bound-interactive applications
+should also be considered, especially when single core usage hits 100%. If you
+gave each of these applications half of a cpu-core and they both got scheduled
+on the same CPU it is theoretically possible that the non-cpu bound application
+will use up to sched_cfs_bandwidth_slice_us additional quota in some periods,
+thereby preventing the cpu-bound application from fully using it's quota by
+that same amount. In these instances it will be up to the CFS algorithm (see
+sched-design-CFS.txt) to decide which application is chosen to run, as they
+will both be runnable and have remaining quota. This runtime discrepancy will
+should made up in the following periods when the interactive application idles.
+
Examples
--------
1. Limit a group to 1 CPU worth of runtime.
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index f35930f..a675c69 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -4295,8 +4295,6 @@ void __refill_cfs_bandwidth_runtime(struct cfs_bandwidth *cfs_b)
now = sched_clock_cpu(smp_processor_id());
cfs_b->runtime = cfs_b->quota;
- cfs_b->runtime_expires = now + ktime_to_ns(cfs_b->period);
- cfs_b->expires_seq++;
}
static inline struct cfs_bandwidth *tg_cfs_bandwidth(struct task_group *tg)
@@ -4318,8 +4316,7 @@ static int assign_cfs_rq_runtime(struct cfs_rq *cfs_rq)
{
struct task_group *tg = cfs_rq->tg;
struct cfs_bandwidth *cfs_b = tg_cfs_bandwidth(tg);
- u64 amount = 0, min_amount, expires;
- int expires_seq;
+ u64 amount = 0, min_amount;
/* note: this is a positive sum as runtime_remaining <= 0 */
min_amount = sched_cfs_bandwidth_slice() - cfs_rq->runtime_remaining;
@@ -4336,61 +4333,17 @@ static int assign_cfs_rq_runtime(struct cfs_rq *cfs_rq)
cfs_b->idle = 0;
}
}
- expires_seq = cfs_b->expires_seq;
- expires = cfs_b->runtime_expires;
raw_spin_unlock(&cfs_b->lock);
cfs_rq->runtime_remaining += amount;
- /*
- * we may have advanced our local expiration to account for allowed
- * spread between our sched_clock and the one on which runtime was
- * issued.
- */
- if (cfs_rq->expires_seq != expires_seq) {
- cfs_rq->expires_seq = expires_seq;
- cfs_rq->runtime_expires = expires;
- }
return cfs_rq->runtime_remaining > 0;
}
-/*
- * Note: This depends on the synchronization provided by sched_clock and the
- * fact that rq->clock snapshots this value.
- */
-static void expire_cfs_rq_runtime(struct cfs_rq *cfs_rq)
-{
- struct cfs_bandwidth *cfs_b = tg_cfs_bandwidth(cfs_rq->tg);
-
- /* if the deadline is ahead of our clock, nothing to do */
- if (likely((s64)(rq_clock(rq_of(cfs_rq)) - cfs_rq->runtime_expires) < 0))
- return;
-
- if (cfs_rq->runtime_remaining < 0)
- return;
-
- /*
- * If the local deadline has passed we have to consider the
- * possibility that our sched_clock is 'fast' and the global deadline
- * has not truly expired.
- *
- * Fortunately we can check determine whether this the case by checking
- * whether the global deadline(cfs_b->expires_seq) has advanced.
- */
- if (cfs_rq->expires_seq == cfs_b->expires_seq) {
- /* extend local deadline, drift is bounded above by 2 ticks */
- cfs_rq->runtime_expires += TICK_NSEC;
- } else {
- /* global deadline is ahead, expiration has passed */
- cfs_rq->runtime_remaining = 0;
- }
-}
-
static void __account_cfs_rq_runtime(struct cfs_rq *cfs_rq, u64 delta_exec)
{
/* dock delta_exec before expiring quota (as it could span periods) */
cfs_rq->runtime_remaining -= delta_exec;
- expire_cfs_rq_runtime(cfs_rq);
if (likely(cfs_rq->runtime_remaining > 0))
return;
@@ -4581,8 +4534,7 @@ void unthrottle_cfs_rq(struct cfs_rq *cfs_rq)
resched_curr(rq);
}
-static u64 distribute_cfs_runtime(struct cfs_bandwidth *cfs_b,
- u64 remaining, u64 expires)
+static u64 distribute_cfs_runtime(struct cfs_bandwidth *cfs_b, u64 remaining)
{
struct cfs_rq *cfs_rq;
u64 runtime;
@@ -4604,7 +4556,6 @@ static u64 distribute_cfs_runtime(struct cfs_bandwidth *cfs_b,
remaining -= runtime;
cfs_rq->runtime_remaining += runtime;
- cfs_rq->runtime_expires = expires;
/* we check whether we're throttled above */
if (cfs_rq->runtime_remaining > 0)
@@ -4629,7 +4580,7 @@ static u64 distribute_cfs_runtime(struct cfs_bandwidth *cfs_b,
*/
static int do_sched_cfs_period_timer(struct cfs_bandwidth *cfs_b, int overrun, unsigned long flags)
{
- u64 runtime, runtime_expires;
+ u64 runtime;
int throttled;
/* no need to continue the timer with no bandwidth constraint */
@@ -4657,8 +4608,6 @@ static int do_sched_cfs_period_timer(struct cfs_bandwidth *cfs_b, int overrun, u
/* account preceding periods in which throttling occurred */
cfs_b->nr_throttled += overrun;
- runtime_expires = cfs_b->runtime_expires;
-
/*
* This check is repeated as we are holding onto the new bandwidth while
* we unthrottle. This can potentially race with an unthrottled group
@@ -4671,8 +4620,7 @@ static int do_sched_cfs_period_timer(struct cfs_bandwidth *cfs_b, int overrun, u
cfs_b->distribute_running = 1;
raw_spin_unlock_irqrestore(&cfs_b->lock, flags);
/* we can't nest cfs_b->lock while distributing bandwidth */
- runtime = distribute_cfs_runtime(cfs_b, runtime,
- runtime_expires);
+ runtime = distribute_cfs_runtime(cfs_b, runtime);
raw_spin_lock_irqsave(&cfs_b->lock, flags);
cfs_b->distribute_running = 0;
@@ -4749,8 +4697,7 @@ static void __return_cfs_rq_runtime(struct cfs_rq *cfs_rq)
return;
raw_spin_lock(&cfs_b->lock);
- if (cfs_b->quota != RUNTIME_INF &&
- cfs_rq->runtime_expires == cfs_b->runtime_expires) {
+ if (cfs_b->quota != RUNTIME_INF) {
cfs_b->runtime += slack_runtime;
/* we are under rq->lock, defer unthrottling using a timer */
@@ -4783,7 +4730,6 @@ static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
{
u64 runtime = 0, slice = sched_cfs_bandwidth_slice();
unsigned long flags;
- u64 expires;
/* confirm we're still not at a refresh boundary */
raw_spin_lock_irqsave(&cfs_b->lock, flags);
@@ -4800,7 +4746,6 @@ static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
if (cfs_b->quota != RUNTIME_INF && cfs_b->runtime > slice)
runtime = cfs_b->runtime;
- expires = cfs_b->runtime_expires;
if (runtime)
cfs_b->distribute_running = 1;
@@ -4809,11 +4754,9 @@ static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
if (!runtime)
return;
- runtime = distribute_cfs_runtime(cfs_b, runtime, expires);
+ runtime = distribute_cfs_runtime(cfs_b, runtime);
raw_spin_lock_irqsave(&cfs_b->lock, flags);
- if (expires == cfs_b->runtime_expires)
- lsub_positive(&cfs_b->runtime, runtime);
cfs_b->distribute_running = 0;
raw_spin_unlock_irqrestore(&cfs_b->lock, flags);
}
@@ -4969,8 +4912,6 @@ void start_cfs_bandwidth(struct cfs_bandwidth *cfs_b)
cfs_b->period_active = 1;
overrun = hrtimer_forward_now(&cfs_b->period_timer, cfs_b->period);
- cfs_b->runtime_expires += (overrun + 1) * ktime_to_ns(cfs_b->period);
- cfs_b->expires_seq++;
hrtimer_start_expires(&cfs_b->period_timer, HRTIMER_MODE_ABS_PINNED);
}
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index b52ed1a..0c0ed23 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -341,8 +341,6 @@ struct cfs_bandwidth {
u64 quota;
u64 runtime;
s64 hierarchical_quota;
- u64 runtime_expires;
- int expires_seq;
short idle;
short period_active;
@@ -562,8 +560,6 @@ struct cfs_rq {
#ifdef CONFIG_CFS_BANDWIDTH
int runtime_enabled;
- int expires_seq;
- u64 runtime_expires;
s64 runtime_remaining;
u64 throttled_clock;
--
1.8.3.1
^ permalink raw reply related
* [PATCH v3 0/1] sched/fair: Fix low cpu usage with high throttling by removing expiration of cpu-local slices
From: Dave Chiluk @ 2019-05-29 19:08 UTC (permalink / raw)
To: Ben Segall, Phil Auld, Peter Oskolkov, Peter Zijlstra,
Ingo Molnar, cgroups, linux-kernel, Brendan Gregg, Kyle Anderson,
Gabriel Munos, John Hammond, Cong Wang, Jonathan Corbet,
linux-doc
In-Reply-To: <1558121424-2914-1-git-send-email-chiluk+linux@indeed.com>
Changelog v3
- Reworked documentation to better describe behavior of slice expiration
per feedback from Peter Oskolkov
Changelog v2
- Fixed some checkpatch errors in the commit message.
^ permalink raw reply
* Re: [PATCH v3 1/1] sched/fair: Fix low cpu usage with high throttling by removing expiration of cpu-local slices
From: Phil Auld @ 2019-05-29 19:28 UTC (permalink / raw)
To: Dave Chiluk
Cc: Ben Segall, Peter Oskolkov, Peter Zijlstra, Ingo Molnar, cgroups,
linux-kernel, Brendan Gregg, Kyle Anderson, Gabriel Munos,
John Hammond, Cong Wang, Jonathan Corbet, linux-doc
In-Reply-To: <1559156926-31336-2-git-send-email-chiluk+linux@indeed.com>
On Wed, May 29, 2019 at 02:08:46PM -0500 Dave Chiluk wrote:
> It has been observed, that highly-threaded, non-cpu-bound applications
> running under cpu.cfs_quota_us constraints can hit a high percentage of
> periods throttled while simultaneously not consuming the allocated
> amount of quota. This use case is typical of user-interactive non-cpu
> bound applications, such as those running in kubernetes or mesos when
> run on multiple cpu cores.
>
> This has been root caused to threads being allocated per cpu bandwidth
> slices, and then not fully using that slice within the period. At which
> point the slice and quota expires. This expiration of unused slice
> results in applications not being able to utilize the quota for which
> they are allocated.
>
> The expiration of per-cpu slices was recently fixed by
> 'commit 512ac999d275 ("sched/fair: Fix bandwidth timer clock drift
> condition")'. Prior to that it appears that this has been broken since
> at least 'commit 51f2176d74ac ("sched/fair: Fix unlocked reads of some
> cfs_b->quota/period")' which was introduced in v3.16-rc1 in 2014. That
> added the following conditional which resulted in slices never being
> expired.
>
> if (cfs_rq->runtime_expires != cfs_b->runtime_expires) {
> /* extend local deadline, drift is bounded above by 2 ticks */
> cfs_rq->runtime_expires += TICK_NSEC;
>
> Because this was broken for nearly 5 years, and has recently been fixed
> and is now being noticed by many users running kubernetes
> (https://github.com/kubernetes/kubernetes/issues/67577) it is my opinion
> that the mechanisms around expiring runtime should be removed
> altogether.
>
> This allows only per-cpu slices to live longer than the period boundary.
> This allows threads on runqueues that do not use much CPU to continue to
> use their remaining slice over a longer period of time than
> cpu.cfs_period_us. However, this helps prevents the above condition of
> hitting throttling while also not fully utilizing your cpu quota.
>
> This theoretically allows a machine to use slightly more than it's
> allotted quota in some periods. This overflow would be bounded by the
> remaining per-cpu slice that was left un-used in the previous period.
> For CPU bound tasks this will change nothing, as they should
> theoretically fully utilize all of their quota and slices in each
> period. For user-interactive tasks as described above this provides a
> much better user/application experience as their cpu utilization will
> more closely match the amount they requested when they hit throttling.
>
> This greatly improves performance of high-thread-count, non-cpu bound
> applications with low cfs_quota_us allocation on high-core-count
> machines. In the case of an artificial testcase, this performance
> discrepancy has been observed to be almost 30x performance improvement,
> while still maintaining correct cpu quota restrictions albeit over
> longer time intervals than cpu.cfs_period_us. That testcase is
> available at https://github.com/indeedeng/fibtest.
>
> Fixes: 512ac999d275 ("sched/fair: Fix bandwidth timer clock drift condition")
> Signed-off-by: Dave Chiluk <chiluk+linux@indeed.com>
> ---
> Documentation/scheduler/sched-bwc.txt | 56 ++++++++++++++++++++++-----
> kernel/sched/fair.c | 71 +++--------------------------------
> kernel/sched/sched.h | 4 --
> 3 files changed, 53 insertions(+), 78 deletions(-)
>
> diff --git a/Documentation/scheduler/sched-bwc.txt b/Documentation/scheduler/sched-bwc.txt
> index f6b1873..260fd65 100644
> --- a/Documentation/scheduler/sched-bwc.txt
> +++ b/Documentation/scheduler/sched-bwc.txt
> @@ -8,15 +8,16 @@ CFS bandwidth control is a CONFIG_FAIR_GROUP_SCHED extension which allows the
> specification of the maximum CPU bandwidth available to a group or hierarchy.
>
> The bandwidth allowed for a group is specified using a quota and period. Within
> -each given "period" (microseconds), a group is allowed to consume only up to
> -"quota" microseconds of CPU time. When the CPU bandwidth consumption of a
> -group exceeds this limit (for that period), the tasks belonging to its
> -hierarchy will be throttled and are not allowed to run again until the next
> -period.
> -
> -A group's unused runtime is globally tracked, being refreshed with quota units
> -above at each period boundary. As threads consume this bandwidth it is
> -transferred to cpu-local "silos" on a demand basis. The amount transferred
> +each given "period" (microseconds), a task group is allocated up to "quota"
> +microseconds of CPU time. That quota is assigned to per cpu run queues in
> +slices as threads in the cgroup become runnable. Once all quota has been
> +assigned any additional requests for quota will result in those threads being
> +throttled. Throttled threads will not be able to run again until the next
> +period when the quota is replenished.
> +
> +A group's unassigned quota is globally tracked, being refreshed back to
> +cfs_quota units at each period boundary. As threads consume this bandwidth it
> +is transferred to cpu-local "silos" on a demand basis. The amount transferred
> within each of these updates is tunable and described as the "slice".
>
> Management
> @@ -90,6 +91,43 @@ There are two ways in which a group may become throttled:
> In case b) above, even though the child may have runtime remaining it will not
> be allowed to until the parent's runtime is refreshed.
>
> +Real-world behavior of slice non-expiration
> +-------------------------------------------
> +The fact that cpu-local slices do not expire results in some interesting corner
> +cases that should be understood.
> +
> +For cgroup cpu constrained applications that are cpu limited this is a
> +relatively moot point because they will naturally consume the entirety of their
> +quota as well as the entirety of each cpu-local slice in each period. As a
> +result it is expected that nr_periods roughly equal nr_throttled, and that
> +cpuacct.usage will increase roughly equal to cfs_quota_us in each period.
> +
> +However in a worst-case scenario, highly-threaded, interactive/non-cpu bound
> +applications this non-expiration nuance allows applications to briefly burst
> +past their quota limits by the amount of unused slice on each cpu that the task
> +group is running on. This slight burst requires that quota had been assigned
> +and then not fully used in previous periods. This burst amount will not be
> +transferred between cores. As a result, this mechanism still strictly limits
> +the task group to quota average usage, albeit over a longer time window than
> +period. This provides better more predictable user experience for highly
> +threaded applications with small quota limits on high core count machines. It
> +also eliminates the propensity to throttle these applications while
> +simultanously using less than quota amounts of cpu. Another way to say this,
> +is that by allowing the unused portion of a slice to remain valid across
> +periods we have decreased the possibility of wasting quota on cpu-local silos
> +that don't need a full slice's amount of cpu time.
> +
> +The interaction between cpu-bound and non-cpu-bound-interactive applications
> +should also be considered, especially when single core usage hits 100%. If you
> +gave each of these applications half of a cpu-core and they both got scheduled
> +on the same CPU it is theoretically possible that the non-cpu bound application
> +will use up to sched_cfs_bandwidth_slice_us additional quota in some periods,
> +thereby preventing the cpu-bound application from fully using it's quota by
"its quota"
> +that same amount. In these instances it will be up to the CFS algorithm (see
> +sched-design-CFS.txt) to decide which application is chosen to run, as they
> +will both be runnable and have remaining quota. This runtime discrepancy will
> +should made up in the following periods when the interactive application idles.
> +
"discrepancy will be made" or "descrepancy should be made" but not both :)
Otherwise, fwiw,
Acked-by: Phil Auld <pauld@redhat.com>
Cheers,
Phil
--
^ permalink raw reply
* Re: [PATCH v3 1/1] sched/fair: Fix low cpu usage with high throttling by removing expiration of cpu-local slices
From: bsegall @ 2019-05-29 19:50 UTC (permalink / raw)
To: Dave Chiluk
Cc: Phil Auld, Peter Oskolkov, Peter Zijlstra, Ingo Molnar, cgroups,
linux-kernel, Brendan Gregg, Kyle Anderson, Gabriel Munos,
John Hammond, Cong Wang, Jonathan Corbet, linux-doc
In-Reply-To: <1559156926-31336-2-git-send-email-chiluk+linux@indeed.com>
Dave Chiluk <chiluk+linux@indeed.com> writes:
> It has been observed, that highly-threaded, non-cpu-bound applications
> running under cpu.cfs_quota_us constraints can hit a high percentage of
> periods throttled while simultaneously not consuming the allocated
> amount of quota. This use case is typical of user-interactive non-cpu
> bound applications, such as those running in kubernetes or mesos when
> run on multiple cpu cores.
>
> This has been root caused to threads being allocated per cpu bandwidth
> slices, and then not fully using that slice within the period. At which
> point the slice and quota expires. This expiration of unused slice
> results in applications not being able to utilize the quota for which
> they are allocated.
>
So most of the bandwidth is supposed to be returned, leaving only 1ms.
I'm setting up to try your test now, but there was a bug with the
unthrottle part of this where it could be continuously pushed into the
future. You might try this patch instead, possibly along with lowering
min_cfs_rq_runtime (currently not runtime tunable) if you see that cost
more than the cost of extra locking to acquire runtime.
From: Ben Segall <bsegall@google.com>
Date: Tue, 28 May 2019 12:30:25 -0700
Subject: [PATCH] sched/fair: don't push cfs_bandwith slack timers forward
When a cfs_rq sleeps and returns its quota, we delay for 5ms before waking any
throttled cfs_rqs to coalesce with other cfs_rqs going to sleep, as this has has
to be done outside of the rq lock we hold. The current code waits for 5ms
without any sleeps, instead of waiting for 5ms from the first sleep, which can
delay the unthrottle more than we want. Switch this around.
Change-Id: Ife536bb54af633863de486ba6ec0d20e55ada8c9
Signed-off-by: Ben Segall <bsegall@google.com>
---
kernel/sched/fair.c | 7 +++++++
kernel/sched/sched.h | 1 +
2 files changed, 8 insertions(+)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 8213ff6e365d..2ead252cfa32 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -4729,6 +4729,11 @@ static void start_cfs_slack_bandwidth(struct cfs_bandwidth *cfs_b)
if (runtime_refresh_within(cfs_b, min_left))
return;
+ /* don't push forwards an existing deferred unthrottle */
+ if (cfs_b->slack_started)
+ return;
+ cfs_b->slack_started = true;
+
hrtimer_start(&cfs_b->slack_timer,
ns_to_ktime(cfs_bandwidth_slack_period),
HRTIMER_MODE_REL);
@@ -4782,6 +4787,7 @@ static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
/* confirm we're still not at a refresh boundary */
raw_spin_lock_irqsave(&cfs_b->lock, flags);
+ cfs_b->slack_started = false;
if (cfs_b->distribute_running) {
raw_spin_unlock_irqrestore(&cfs_b->lock, flags);
return;
@@ -4920,6 +4926,7 @@ void init_cfs_bandwidth(struct cfs_bandwidth *cfs_b)
hrtimer_init(&cfs_b->slack_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
cfs_b->slack_timer.function = sched_cfs_slack_timer;
cfs_b->distribute_running = 0;
+ cfs_b->slack_started = false;
}
static void init_cfs_rq_runtime(struct cfs_rq *cfs_rq)
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index efa686eeff26..60219acda94b 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -356,6 +356,7 @@ struct cfs_bandwidth {
u64 throttled_time;
bool distribute_running;
+ bool slack_started;
#endif
};
--
2.22.0.rc1.257.g3120a18244-goog
^ permalink raw reply related
* Re: [PATCH v3 1/1] sched/fair: Fix low cpu usage with high throttling by removing expiration of cpu-local slices
From: bsegall @ 2019-05-29 21:05 UTC (permalink / raw)
To: Dave Chiluk
Cc: Phil Auld, Peter Oskolkov, Peter Zijlstra, Ingo Molnar, cgroups,
linux-kernel, Brendan Gregg, Kyle Anderson, Gabriel Munos,
John Hammond, Cong Wang, Jonathan Corbet, linux-doc, pjt
In-Reply-To: <1559156926-31336-2-git-send-email-chiluk+linux@indeed.com>
Dave Chiluk <chiluk+linux@indeed.com> writes:
> It has been observed, that highly-threaded, non-cpu-bound applications
> running under cpu.cfs_quota_us constraints can hit a high percentage of
> periods throttled while simultaneously not consuming the allocated
> amount of quota. This use case is typical of user-interactive non-cpu
> bound applications, such as those running in kubernetes or mesos when
> run on multiple cpu cores.
>
> This has been root caused to threads being allocated per cpu bandwidth
> slices, and then not fully using that slice within the period. At which
> point the slice and quota expires. This expiration of unused slice
> results in applications not being able to utilize the quota for which
> they are allocated.
>
> The expiration of per-cpu slices was recently fixed by
> 'commit 512ac999d275 ("sched/fair: Fix bandwidth timer clock drift
> condition")'. Prior to that it appears that this has been broken since
> at least 'commit 51f2176d74ac ("sched/fair: Fix unlocked reads of some
> cfs_b->quota/period")' which was introduced in v3.16-rc1 in 2014. That
> added the following conditional which resulted in slices never being
> expired.
Yeah, having run the test, stranding only 1 ms per cpu rather than 5
doesn't help if you only have 10 ms of quota and even 10 threads/cpus.
The slack timer isn't important in this test, though I think it probably
should be changed.
Decreasing min_cfs_rq_runtime helps, but would mean that we have to pull
quota more often / always. The worst case here I think is where you
run/sleep for ~1ns, so you wind up taking the lock twice every
min_cfs_rq_runtime: once for assign and once to return all but min,
which you then use up doing short run/sleep. I suppose that determines
how much we care about this overhead at all.
Removing expiration means that in the worst case period and quota can be
effectively twice what the user specified, but only on very particular
workloads.
I think we should at least think about instead lowering
min_cfs_rq_runtime to some smaller value.
^ permalink raw reply
* Re: [GIT PULL] Fixes for sphinx 2.0 docs build failures
From: pr-tracker-bot @ 2019-05-29 21:45 UTC (permalink / raw)
To: Jonathan Corbet; +Cc: Linus Torvalds, LKML, linux-doc
In-Reply-To: <20190529095408.1429a5ab@lwn.net>
The pull request you sent on Wed, 29 May 2019 09:54:08 -0600:
> git://git.lwn.net/linux.git tags/docs-5.2-fixes2
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/bec7550cca106c3ccc061e3e625516af63054fe4
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
^ permalink raw reply
* Re: [PATCH] docs: cdomain.py: get rid of a warning since version 1.8
From: Jonathan Corbet @ 2019-05-29 21:48 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Linux Doc Mailing List, Mauro Carvalho Chehab, linux-kernel
In-Reply-To: <b38a9fdfdcda49b2c6118072afac564e96406800.1558608217.git.mchehab+samsung@kernel.org>
On Thu, 23 May 2019 07:43:43 -0300
Mauro Carvalho Chehab <mchehab+samsung@kernel.org> wrote:
> There's a new warning about a deprecation function. Add a
> logic at cdomain.py to avoid that.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
Applied, thanks.
jon
^ permalink raw reply
* Re: [PATCH] [linux-next] docs: tracing: Fix typos in histogram.rst
From: Jonathan Corbet @ 2019-05-29 21:49 UTC (permalink / raw)
To: Steven Rostedt; +Cc: Masanari Iida, linux-doc, linux-kernel, mingo, rdunlap
In-Reply-To: <20190521085525.2963c10f@gandalf.local.home>
On Tue, 21 May 2019 08:55:25 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:
> On Tue, 21 May 2019 21:30:00 +0900
> Masanari Iida <standby24x7@gmail.com> wrote:
>
> > This patch fixes some spelling typos in histogram.rst
>
> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
>
> Jon,
>
> Care to take this in your tree?
Done, thanks.
jon
^ permalink raw reply
* [PATCH] docs cgroups: add another example size for hugetlb
From: Odin Ugedal @ 2019-05-29 22:24 UTC (permalink / raw)
To: odin
Cc: Tejun Heo, Li Zefan, Johannes Weiner, Jonathan Corbet,
open list:CONTROL GROUP (CGROUP), open list:DOCUMENTATION,
open list
Add another example to clarify that HugePages smaller than 1MB will
be displayed using "KB", with an uppercased K (eg. 20KB), and not the
normal SI prefix kilo (small k).
Because of a misunderstanding/copy-paste error inside runc
(see https://github.com/opencontainers/runc/pull/2065), it tried
accessing the cgroup control file of a 64kB HugePage using
"hugetlb.64kB._____" instead of the correct "hugetlb.64KB._____".
Adding a new example will make it clear how sizes smaller than 1MB are
handled.
Signed-off-by: Odin Ugedal <odin@ugedal.com>
---
Documentation/cgroup-v1/hugetlb.txt | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/Documentation/cgroup-v1/hugetlb.txt b/Documentation/cgroup-v1/hugetlb.txt
index 106245c3aecc..1260e5369b9b 100644
--- a/Documentation/cgroup-v1/hugetlb.txt
+++ b/Documentation/cgroup-v1/hugetlb.txt
@@ -32,14 +32,18 @@ Brief summary of control files
hugetlb.<hugepagesize>.usage_in_bytes # show current usage for "hugepagesize" hugetlb
hugetlb.<hugepagesize>.failcnt # show the number of allocation failure due to HugeTLB limit
-For a system supporting two hugepage size (16M and 16G) the control
+For a system supporting three hugepage sizes (64k, 32M and 1G), the control
files include:
-hugetlb.16GB.limit_in_bytes
-hugetlb.16GB.max_usage_in_bytes
-hugetlb.16GB.usage_in_bytes
-hugetlb.16GB.failcnt
-hugetlb.16MB.limit_in_bytes
-hugetlb.16MB.max_usage_in_bytes
-hugetlb.16MB.usage_in_bytes
-hugetlb.16MB.failcnt
+hugetlb.1GB.limit_in_bytes
+hugetlb.1GB.max_usage_in_bytes
+hugetlb.1GB.usage_in_bytes
+hugetlb.1GB.failcnt
+hugetlb.64KB.limit_in_bytes
+hugetlb.64KB.max_usage_in_bytes
+hugetlb.64KB.usage_in_bytes
+hugetlb.64KB.failcnt
+hugetlb.32MB.limit_in_bytes
+hugetlb.32MB.max_usage_in_bytes
+hugetlb.32MB.usage_in_bytes
+hugetlb.32MB.failcnt
--
2.21.0
^ permalink raw reply related
* Re: [PATCH v4 0/9] docs: Convert VFS doc to RST
From: Jonathan Corbet @ 2019-05-29 22:30 UTC (permalink / raw)
To: Tobin C. Harding
Cc: Al Viro, Mauro Carvalho Chehab, Neil Brown, Randy Dunlap,
linux-doc, linux-fsdevel, linux-kernel
In-Reply-To: <20190515002913.12586-1-tobin@kernel.org>
On Wed, 15 May 2019 10:29:04 +1000
"Tobin C. Harding" <tobin@kernel.org> wrote:
> Here is an updated version of the VFS doc conversion. This series in no
> way represents a final point for the VFS documentation rather it is a
> small step towards getting VFS docs updated. This series does not
> update the content of vfs.txt, only does formatting.
I've finally gotten to this, sorry for taking so long. Applying it to
docs-next turned out to be a bit of a chore; there have been intervening
changes to vfs.txt that we didn't want to lose. But I did it.
Unfortunately, there's still a remaining issue. You did a lot of list
conversions like this:
> - struct file_system_type *fs_type: describes the filesystem, partly initialized
> +``struct file_system_type *fs_type``: describes the filesystem, partly initialized
> by the specific filesystem code
but that does not render the way you would like, trust me. You really
want to use the list format, something like:
``struct file_system_type *fs_type``
describes the filesystem, partly initialized by the specific
filesystem code
There are, unfortunately, a lot of these to fix... I bet it could be done
with an elisp function, but I don't have time to beat my head against that
wall right now.
Any chance you would have time to send me a followup patch fixing these
up? I'll keep my branch with this set for now so there's no need to
rebase those.
Thanks,
jon
^ permalink raw reply
* Re: [PATCH v6 00/12] Include linux PCI docs into Sphinx TOC tree
From: Jonathan Corbet @ 2019-05-29 22:35 UTC (permalink / raw)
To: Changbin Du; +Cc: bhelgaas, linux-pci, linux-doc, linux-kernel, mchehab+samsung
In-Reply-To: <20190520061014.qtq6tc366pnnqcio@mail.google.com>
On Mon, 20 May 2019 06:10:15 +0000
Changbin Du <changbin.du@gmail.com> wrote:
> Bjorn and Jonathan,
> Could we consider to merge this serias now? Thanks.
Somewhat belatedly, but I think we could. Bjorn, do you have a preference
for which tree this goes through? I don't remember if we'd come to an
agreement on that or not, sorry...
Thanks,
jon
^ permalink raw reply
* [PATCH 2/2] Docs: hwmon: pmbus: Add PXE1610 driver
From: Vijay Khemka @ 2019-05-29 22:35 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, Jonathan Corbet, linux-hwmon,
linux-doc, linux-kernel
Cc: vijaykhemka, joel, linux-aspeed, sdasari, Greg Kroah-Hartman
In-Reply-To: <20190529223511.4059120-1-vijaykhemka@fb.com>
Added support for Infenion PXE1610 driver
Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
---
Documentation/hwmon/pxe1610 | 84 +++++++++++++++++++++++++++++++++++++
1 file changed, 84 insertions(+)
create mode 100644 Documentation/hwmon/pxe1610
diff --git a/Documentation/hwmon/pxe1610 b/Documentation/hwmon/pxe1610
new file mode 100644
index 000000000000..b5c83edf027a
--- /dev/null
+++ b/Documentation/hwmon/pxe1610
@@ -0,0 +1,84 @@
+Kernel driver pxe1610
+=====================
+
+Supported chips:
+ * Infinion PXE1610
+ Prefix: 'pxe1610'
+ Addresses scanned: -
+ Datasheet: Datasheet is not publicly available.
+
+ * Infinion PXE1110
+ Prefix: 'pxe1110'
+ Addresses scanned: -
+ Datasheet: Datasheet is not publicly available.
+
+ * Infinion PXM1310
+ Prefix: 'pxm1310'
+ Addresses scanned: -
+ Datasheet: Datasheet is not publicly available.
+
+Author: Vijay Khemka <vijaykhemka@fb.com>
+
+
+Description
+-----------
+
+PXE1610 is a Multi-rail/Multiphase Digital Controllers and
+it is compliant to Intel VR13 DC-DC converter specifications.
+
+
+Usage Notes
+-----------
+
+This driver can be enabled with kernel config CONFIG_SENSORS_PXE1610
+set to 'y' or 'm'(for module).
+
+This driver does not probe for PMBus devices. You will have
+to instantiate devices explicitly.
+
+Example: the following commands will load the driver for an PXE1610
+at address 0x70 on I2C bus #4:
+
+# modprobe pxe1610
+# echo pxe1610 0x70 > /sys/bus/i2c/devices/i2c-4/new_device
+
+It can also be instantiated by declaring in device tree if it is
+built as a kernel not as a module.
+
+
+Sysfs attributes
+----------------
+
+curr1_label "iin"
+curr1_input Measured input current
+curr1_alarm Current high alarm
+
+curr[2-4]_label "iout[1-3]"
+curr[2-4]_input Measured output current
+curr[2-4]_crit Critical maximum current
+curr[2-4]_crit_alarm Current critical high alarm
+
+in1_label "vin"
+in1_input Measured input voltage
+in1_crit Critical maximum input voltage
+in1_crit_alarm Input voltage critical high alarm
+
+in[2-4]_label "vout[1-3]"
+in[2-4]_input Measured output voltage
+in[2-4]_lcrit Critical minimum output voltage
+in[2-4]_lcrit_alarm Output voltage critical low alarm
+in[2-4]_crit Critical maximum output voltage
+in[2-4]_crit_alarm Output voltage critical high alarm
+
+power1_label "pin"
+power1_input Measured input power
+power1_alarm Input power high alarm
+
+power[2-4]_label "pout[1-3]"
+power[2-4]_input Measured output power
+
+temp[1-3]_input Measured temperature
+temp[1-3]_crit Critical high temperature
+temp[1-3]_crit_alarm Chip temperature critical high alarm
+temp[1-3]_max Maximum temperature
+temp[1-3]_max_alarm Chip temperature high alarm
--
2.17.1
^ permalink raw reply related
* [PATCH 1/2] hwmon: pmbus: Add Infineon PXE1610 VR driver
From: Vijay Khemka @ 2019-05-29 22:35 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, Jonathan Corbet, linux-hwmon,
linux-doc, linux-kernel
Cc: vijaykhemka, joel, linux-aspeed, sdasari, Greg Kroah-Hartman
Added pmbus driver for the new device Infineon pxe1610
voltage regulator. It also supports similar family device
PXE1110 and PXM1310.
Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
---
drivers/hwmon/pmbus/Kconfig | 9 +++
drivers/hwmon/pmbus/Makefile | 1 +
drivers/hwmon/pmbus/pxe1610.c | 119 ++++++++++++++++++++++++++++++++++
3 files changed, 129 insertions(+)
create mode 100644 drivers/hwmon/pmbus/pxe1610.c
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 30751eb9550a..338ef9b5a395 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -154,6 +154,15 @@ config SENSORS_MAX8688
This driver can also be built as a module. If so, the module will
be called max8688.
+config SENSORS_PXE1610
+ tristate "Infineon PXE1610"
+ help
+ If you say yes here you get hardware monitoring support for Infineon
+ PXE1610.
+
+ This driver can also be built as a module. If so, the module will
+ be called pxe1610.
+
config SENSORS_TPS40422
tristate "TI TPS40422"
help
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index 2219b9300316..b0fbd017a91a 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_SENSORS_MAX20751) += max20751.o
obj-$(CONFIG_SENSORS_MAX31785) += max31785.o
obj-$(CONFIG_SENSORS_MAX34440) += max34440.o
obj-$(CONFIG_SENSORS_MAX8688) += max8688.o
+obj-$(CONFIG_SENSORS_PXE1610) += pxe1610.o
obj-$(CONFIG_SENSORS_TPS40422) += tps40422.o
obj-$(CONFIG_SENSORS_TPS53679) += tps53679.o
obj-$(CONFIG_SENSORS_UCD9000) += ucd9000.o
diff --git a/drivers/hwmon/pmbus/pxe1610.c b/drivers/hwmon/pmbus/pxe1610.c
new file mode 100644
index 000000000000..01e267944df5
--- /dev/null
+++ b/drivers/hwmon/pmbus/pxe1610.c
@@ -0,0 +1,119 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Hardware monitoring driver for Infineon PXE1610
+ *
+ * Copyright (c) 2019 Facebook Inc
+ *
+ */
+
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include "pmbus.h"
+
+/*
+ * Identify chip parameters.
+ */
+static int pxe1610_identify(struct i2c_client *client,
+ struct pmbus_driver_info *info)
+{
+ if (pmbus_check_byte_register(client, 0, PMBUS_VOUT_MODE)) {
+ int vout_mode;
+
+ vout_mode = pmbus_read_byte_data(client, 0, PMBUS_VOUT_MODE);
+ switch (vout_mode & 0x1f) {
+ case 1:
+ info->vrm_version = vr12;
+ break;
+ case 2:
+ info->vrm_version = vr13;
+ break;
+ default:
+ return -ENODEV;
+ }
+ }
+ return 0;
+}
+
+static int pxe1610_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct pmbus_driver_info *info;
+ u8 buf[I2C_SMBUS_BLOCK_MAX];
+ int ret;
+
+ if (!i2c_check_functionality(client->adapter,
+ I2C_FUNC_SMBUS_READ_BYTE_DATA
+ | I2C_FUNC_SMBUS_READ_WORD_DATA
+ | I2C_FUNC_SMBUS_READ_BLOCK_DATA))
+ return -ENODEV;
+
+ /* By default this device doesn't boot to page 0, so set page 0
+ * to access all pmbus registers.
+ */
+ i2c_smbus_write_byte_data(client, 0, 0);
+
+ /* Read Manufacturer id */
+ ret = i2c_smbus_read_block_data(client, PMBUS_MFR_ID, buf);
+ if (ret < 0) {
+ dev_err(&client->dev, "Failed to read PMBUS_MFR_ID\n");
+ return ret;
+ }
+ if (ret != 2 || strncmp(buf, "XP", strlen("XP"))) {
+ dev_err(&client->dev, "MFR_ID unrecognised\n");
+ return -ENODEV;
+ }
+
+ info = devm_kzalloc(&client->dev, sizeof(struct pmbus_driver_info),
+ GFP_KERNEL);
+ if (!info)
+ return -ENOMEM;
+
+ info->format[PSC_VOLTAGE_IN] = linear;
+ info->format[PSC_VOLTAGE_OUT] = vid;
+ info->format[PSC_CURRENT_IN] = linear;
+ info->format[PSC_CURRENT_OUT] = linear;
+ info->format[PSC_POWER] = linear;
+ info->format[PSC_TEMPERATURE] = linear;
+
+ info->func[0] = PMBUS_HAVE_VIN
+ | PMBUS_HAVE_VOUT | PMBUS_HAVE_IIN
+ | PMBUS_HAVE_IOUT | PMBUS_HAVE_PIN
+ | PMBUS_HAVE_POUT | PMBUS_HAVE_TEMP
+ | PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT
+ | PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP;
+ info->func[1] = info->func[0];
+ info->func[2] = info->func[0];
+
+ info->pages = id->driver_data;
+ info->identify = pxe1610_identify;
+
+ return pmbus_do_probe(client, id, info);
+}
+
+static const struct i2c_device_id pxe1610_id[] = {
+ {"pxe1610", 3},
+ {"pxe1110", 3},
+ {"pxm1310", 3},
+ {}
+};
+
+MODULE_DEVICE_TABLE(i2c, pxe1610_id);
+
+/* This is the driver that will be inserted */
+static struct i2c_driver pxe1610_driver = {
+ .driver = {
+ .name = "pxe1610",
+ },
+ .probe = pxe1610_probe,
+ .remove = pmbus_do_remove,
+ .id_table = pxe1610_id,
+};
+
+module_i2c_driver(pxe1610_driver);
+
+MODULE_AUTHOR("Vijay Khemka <vijaykhemka@fb.com>");
+MODULE_DESCRIPTION("PMBus driver for Infineon PXE1610, PXE1110 and PXM1310");
+MODULE_LICENSE("GPL");
--
2.17.1
^ permalink raw reply related
* Re: [PATCH] scripts/sphinx-pre-install: make activate hint smarter
From: Jonathan Corbet @ 2019-05-29 22:35 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Linux Doc Mailing List, Mauro Carvalho Chehab, linux-kernel
In-Reply-To: <582e6b7351f619d92035db8cfe24ecec5d95a489.1558610037.git.mchehab+samsung@kernel.org>
On Thu, 23 May 2019 08:14:02 -0300
Mauro Carvalho Chehab <mchehab+samsung@kernel.org> wrote:
> It is possible that multiple Sphinx virtualenvs are installed
> on a given kernel tree. Change the logic to get the latest
> version of those, as this is probably what the user wants.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
Applied, thanks.
jon
^ permalink raw reply
* [PATCH] Allow to exclude specific file types in LoadPin
From: Ke Wu @ 2019-05-29 22:43 UTC (permalink / raw)
To: Kees Cook, Jonathan Corbet, James Morris, Serge E. Hallyn
Cc: linux-doc, linux-kernel, linux-security-module, Ke Wu
Linux kernel already provide MODULE_SIG and KEXEC_VERIFY_SIG to
make sure loaded kernel module and kernel image are trusted. This
patch adds a kernel command line option "loadpin.exclude" which
allows to exclude specific file types from LoadPin. This is useful
when people want to use different mechanisms to verify module and
kernel image while still use LoadPin to protect the integrity of
other files kernel loads.
Signed-off-by: Ke Wu <mikewu@google.com>
---
Documentation/admin-guide/LSM/LoadPin.rst | 10 ++++++
security/loadpin/loadpin.c | 37 +++++++++++++++++++++++
2 files changed, 47 insertions(+)
diff --git a/Documentation/admin-guide/LSM/LoadPin.rst b/Documentation/admin-guide/LSM/LoadPin.rst
index 32070762d24c..716ad9b23c9a 100644
--- a/Documentation/admin-guide/LSM/LoadPin.rst
+++ b/Documentation/admin-guide/LSM/LoadPin.rst
@@ -19,3 +19,13 @@ block device backing the filesystem is not read-only, a sysctl is
created to toggle pinning: ``/proc/sys/kernel/loadpin/enabled``. (Having
a mutable filesystem means pinning is mutable too, but having the
sysctl allows for easy testing on systems with a mutable filesystem.)
+
+It's also possible to exclude specific file types from LoadPin using kernel
+command line option "``loadpin.exclude``". By default, all files are
+included, but they can be excluded using kernel command line option such
+as "``loadpin.exclude=kernel-module,kexec-image``". This allows to use
+different mechanisms such as ``CONFIG_MODULE_SIG`` and
+``CONFIG_KEXEC_VERIFY_SIG`` to verify kernel module and kernel image while
+still use LoadPin to protect the integrity of other files kernel loads. The
+full list of valid file types can be found in ``kernel_read_file_str``
+defined in ``include/linux/fs.h``.
diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
index 055fb0a64169..8ee0c58fea40 100644
--- a/security/loadpin/loadpin.c
+++ b/security/loadpin/loadpin.c
@@ -45,6 +45,8 @@ static void report_load(const char *origin, struct file *file, char *operation)
}
static int enforce = IS_ENABLED(CONFIG_SECURITY_LOADPIN_ENFORCE);
+static char *exclude_read_files[READING_MAX_ID];
+static int ignore_read_file_id[READING_MAX_ID];
static struct super_block *pinned_root;
static DEFINE_SPINLOCK(pinned_root_spinlock);
@@ -129,6 +131,12 @@ static int loadpin_read_file(struct file *file, enum kernel_read_file_id id)
struct super_block *load_root;
const char *origin = kernel_read_file_id_str(id);
+ /* If the file id is excluded, ignore the pinning. */
+ if ((unsigned int)id < READING_MAX_ID && ignore_read_file_id[id]) {
+ report_load(origin, file, "pinning-excluded");
+ return 0;
+ }
+
/* This handles the older init_module API that has a NULL file. */
if (!file) {
if (!enforce) {
@@ -187,10 +195,37 @@ static struct security_hook_list loadpin_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(kernel_load_data, loadpin_load_data),
};
+static void parse_exclude(void)
+{
+ int i, j;
+ char *cur;
+
+ for (i = 0; i < ARRAY_SIZE(exclude_read_files); i++) {
+ cur = exclude_read_files[i];
+ if (!cur)
+ break;
+ if (*cur == '\0')
+ continue;
+
+ for (j = 0; j < ARRAY_SIZE(kernel_read_file_str); j++) {
+ if (strcmp(cur, kernel_read_file_str[j]) == 0) {
+ pr_info("excluding: %s\n",
+ kernel_read_file_str[j]);
+ ignore_read_file_id[j] = 1;
+ /*
+ * Can not break, because one read_file_str
+ * may map to more than on read_file_id.
+ */
+ }
+ }
+ }
+}
+
static int __init loadpin_init(void)
{
pr_info("ready to pin (currently %senforcing)\n",
enforce ? "" : "not ");
+ parse_exclude();
security_add_hooks(loadpin_hooks, ARRAY_SIZE(loadpin_hooks), "loadpin");
return 0;
}
@@ -203,3 +238,5 @@ DEFINE_LSM(loadpin) = {
/* Should not be mutable after boot, so not listed in sysfs (perm == 0). */
module_param(enforce, int, 0);
MODULE_PARM_DESC(enforce, "Enforce module/firmware pinning");
+module_param_array_named(exclude, exclude_read_files, charp, NULL, 0);
+MODULE_PARM_DESC(exclude, "Exclude pinning specific read file types");
--
2.22.0.rc1.257.g3120a18244-goog
^ permalink raw reply related
* Re: [PATCH 4/5] docs: by default, build docs a lot faster with Sphinx >= 1.7
From: Jonathan Corbet @ 2019-05-29 23:02 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Linux Doc Mailing List, Mauro Carvalho Chehab, linux-kernel
In-Reply-To: <baf19095789f2b2ed0c7a940703037a00cd77850.1558955082.git.mchehab+samsung@kernel.org>
On Mon, 27 May 2019 08:07:40 -0300
Mauro Carvalho Chehab <mchehab+samsung@kernel.org> wrote:
> Since Sphinx version 1.7, it is possible to use "-jauto" in
> order to speedup documentation builds. On older versions,
> while -j was already supported, one would need to set the
> number of threads manually.
>
> So, if SPHINXOPTS is not provided, add -jauto, in order to
> speed up the build. That makes it *a lot* times faster than
> without -j.
>
> If one really wants to slow things down, it can just use:
>
> make SPHINXOPTS=-j1 htmldocs
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> ---
> Documentation/Makefile | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/Makefile b/Documentation/Makefile
> index 380e24053d6f..794233d05789 100644
> --- a/Documentation/Makefile
> +++ b/Documentation/Makefile
> @@ -28,6 +28,8 @@ ifeq ($(HAVE_SPHINX),0)
>
> else # HAVE_SPHINX
>
> +SPHINXOPTS = $(shell perl -e 'open IN,"sphinx-build --version |"; while (<IN>) { if (m/([\d\.]+)/) { print "-jauto" if ($$1 >= "1.7") } ;} close IN')
> +
So this totally fails to work for me with any version of sphinx, and I'm
not enough of a Perl person to figure it out. Sometimes I'll see the
sphinx-build output, i.e.:
sphinx-build 1.8.4
and sometimes (like with 2.0) I don't, but I never get -jauto regardless.
Not sure what's going on here?
Thanks,
jon
^ 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