* Re: [PATCH v2 0/3] Bitops instrumentation for KASAN
From: Marco Elver @ 2019-05-31 15:12 UTC (permalink / raw)
To: Peter Zijlstra, Andrey Ryabinin, Dmitry Vyukov,
Alexander Potapenko, Andrey Konovalov, Mark Rutland
Cc: Jonathan Corbet, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
H. Peter Anvin, the arch/x86 maintainers, Arnd Bergmann,
Josh Poimboeuf, open list:DOCUMENTATION, LKML, linux-arch,
kasan-dev
In-Reply-To: <20190529141500.193390-1-elver@google.com>
Addressed comments, and sent v3:
http://lkml.kernel.org/r/20190531150828.157832-1-elver@google.com
Many thanks!
-- Marco
On Wed, 29 May 2019 at 16:23, Marco Elver <elver@google.com> wrote:
>
> The previous version of this patch series and discussion can be found
> here: https://lkml.org/lkml/2019/5/28/769
>
> The most significant change is the change of the instrumented access
> size to cover the entire word of a bit.
>
> Marco Elver (3):
> lib/test_kasan: Add bitops tests
> x86: Move CPU feature test out of uaccess region
> asm-generic, x86: Add bitops instrumentation for KASAN
>
> Documentation/core-api/kernel-api.rst | 2 +-
> arch/x86/ia32/ia32_signal.c | 9 +-
> arch/x86/include/asm/bitops.h | 210 ++++----------
> include/asm-generic/bitops-instrumented.h | 317 ++++++++++++++++++++++
> lib/test_kasan.c | 75 ++++-
> 5 files changed, 450 insertions(+), 163 deletions(-)
> create mode 100644 include/asm-generic/bitops-instrumented.h
>
> --
> 2.22.0.rc1.257.g3120a18244-goog
>
^ permalink raw reply
* Re: [PATCH] Documentation/stackprotector: powerpc supports stack protector
From: Michael Ellerman @ 2019-05-31 15:13 UTC (permalink / raw)
To: Jonathan Corbet, Bhupesh Sharma
Cc: linuxppc-dev, Arnd Bergmann, Bhupesh SHARMA,
Benjamin Herrenschmidt, Paul Mackerras, Linux Kernel Mailing List,
linux-doc
In-Reply-To: <20190530081358.650930ad@lwn.net>
Jonathan Corbet <corbet@lwn.net> writes:
> On Thu, 30 May 2019 18:37:46 +0530
> Bhupesh Sharma <bhsharma@redhat.com> wrote:
>
>> > This should probably go via the documentation tree?
>> >
>> > Acked-by: Michael Ellerman <mpe@ellerman.id.au>
>>
>> Thanks for the review Michael.
>> I am ok with this going through the documentation tree as well.
>
> Works for me too, but I don't seem to find the actual patch anywhere I
> look. Can you send me a copy?
You can get it from lore:
https://lore.kernel.org/linuxppc-dev/1559212177-7072-1-git-send-email-bhsharma@redhat.com/raw
Or patchwork (automatically adds my ack):
https://patchwork.ozlabs.org/patch/1107706/mbox/
Or Bhupesh can send it to you :)
cheers
^ permalink raw reply
* Re: [PATCH v3 1/3] lib/test_kasan: Add bitops tests
From: Mark Rutland @ 2019-05-31 15:57 UTC (permalink / raw)
To: Marco Elver
Cc: peterz, aryabinin, dvyukov, glider, andreyknvl, hpa, corbet, tglx,
mingo, bp, x86, arnd, jpoimboe, linux-doc, linux-kernel,
linux-arch, kasan-dev
In-Reply-To: <20190531150828.157832-2-elver@google.com>
On Fri, May 31, 2019 at 05:08:29PM +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 v3:
> * Use kzalloc instead of kmalloc.
> * Use sizeof(*bits).
Thatnks for cleaning these up! FWIW:
Acked-by: Mark Rutland <mark.rutland@arm.com>
Mark.
>
> 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..1ef9702327d2 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 = kzalloc(sizeof(*bits), GFP_KERNEL);
> + 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 v3 3/3] asm-generic, x86: Add bitops instrumentation for KASAN
From: Mark Rutland @ 2019-05-31 16:01 UTC (permalink / raw)
To: Marco Elver
Cc: peterz, aryabinin, dvyukov, glider, andreyknvl, hpa, corbet, tglx,
mingo, bp, x86, arnd, jpoimboe, linux-doc, linux-kernel,
linux-arch, kasan-dev
In-Reply-To: <20190531150828.157832-4-elver@google.com>
On Fri, May 31, 2019 at 05:08:31PM +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 was derived from x86 and existing bitops
> asm-generic versions: 1) references to x86 have been removed; 2) as a
> result, some of the text had to be reworded for clarity and consistency.
>
> 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 v3:
> * Remove references to 'x86' in API documentation; as a result, had to
> reword doc text for clarify and consistency.
> * Remove #ifdef, since it is assumed that if asm-generic bitops
> implementations are used, bitops-instrumented.h is not needed.
Thanks for sorting this out. FWIW:
Acked-by: Mark Rutland <mark.rutland@arm.com>
Mark.
>
> 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 | 189 ++++------------
> include/asm-generic/bitops-instrumented.h | 263 ++++++++++++++++++++++
> 3 files changed, 302 insertions(+), 152 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..ba15d53c1ca7 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"
> @@ -78,32 +63,14 @@ set_bit(long nr, volatile unsigned long *addr)
> }
> }
>
> -/**
> - * __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");
> }
>
> -/**
> - * 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"
> @@ -115,26 +82,21 @@ clear_bit(long nr, volatile unsigned long *addr)
> }
> }
>
> -/*
> - * 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);
> }
>
> -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");
> }
>
> -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 +105,23 @@ 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);
> }
>
> -/**
> - * __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");
> }
>
> -/**
> - * 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"
> @@ -196,42 +133,20 @@ static __always_inline void change_bit(long nr, volatile unsigned long *addr)
> }
> }
>
> -/**
> - * 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);
> }
>
> -/**
> - * 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);
> }
>
> -/**
> - * __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;
>
> @@ -242,28 +157,13 @@ static __always_inline bool __test_and_set_bit(long nr, volatile unsigned long *
> return oldbit;
> }
>
> -/**
> - * 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);
> }
>
> -/**
> - * __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.
> @@ -271,7 +171,8 @@ static __always_inline bool test_and_clear_bit(long nr, volatile unsigned long *
> * 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;
>
> @@ -282,8 +183,8 @@ static __always_inline bool __test_and_clear_bit(long nr, volatile unsigned long
> return oldbit;
> }
>
> -/* 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;
>
> @@ -295,15 +196,8 @@ static __always_inline bool __test_and_change_bit(long nr, volatile unsigned lon
> return oldbit;
> }
>
> -/**
> - * 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);
> }
> @@ -326,16 +220,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 +389,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..ddd1c6d9d8db
> --- /dev/null
> +++ b/include/asm-generic/bitops-instrumented.h
> @@ -0,0 +1,263 @@
> +/* 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.).
> + */
> +#ifndef _ASM_GENERIC_BITOPS_INSTRUMENTED_H
> +#define _ASM_GENERIC_BITOPS_INSTRUMENTED_H
> +
> +#include <linux/kasan-checks.h>
> +
> +/**
> + * set_bit - Atomically set a bit in memory
> + * @nr: the bit to set
> + * @addr: the address to start counting from
> + *
> + * This is a relaxed atomic operation (no implied memory barriers).
> + *
> + * 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);
> +}
> +
> +/**
> + * __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. If it is called on the same
> + * region of memory concurrently, 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);
> +}
> +
> +/**
> + * clear_bit - Clears a bit in memory
> + * @nr: Bit to clear
> + * @addr: Address to start counting from
> + *
> + * This is a relaxed atomic operation (no implied memory barriers).
> + */
> +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);
> +}
> +
> +/**
> + * __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. If it is called on the same
> + * region of memory concurrently, 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);
> +}
> +
> +/**
> + * clear_bit_unlock - Clear a bit in memory, for unlock
> + * @nr: the bit to set
> + * @addr: the address to start counting from
> + *
> + * This operation is atomic and provides release barrier semantics.
> + */
> +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);
> +}
> +
> +/**
> + * __clear_bit_unlock - Clears a bit in memory
> + * @nr: Bit to clear
> + * @addr: Address to start counting from
> + *
> + * This is a non-atomic operation but implies a release barrier 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);
> +}
> +
> +/**
> + * change_bit - Toggle a bit in memory
> + * @nr: Bit to change
> + * @addr: Address to start counting from
> + *
> + * This is a relaxed atomic operation (no implied memory barriers).
> + *
> + * 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);
> +}
> +
> +/**
> + * __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. If it is called on the same
> + * region of memory concurrently, 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);
> +}
> +
> +/**
> + * test_and_set_bit - Set a bit and return its old value
> + * @nr: Bit to set
> + * @addr: Address to count from
> + *
> + * This is an atomic fully-ordered operation (implied full 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);
> +}
> +
> +/**
> + * __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. If two instances of this operation race, one
> + * can appear to succeed but actually fail.
> + */
> +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);
> +}
> +
> +/**
> + * 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);
> +}
> +
> +/**
> + * test_and_clear_bit - Clear a bit and return its old value
> + * @nr: Bit to clear
> + * @addr: Address to count from
> + *
> + * This is an atomic fully-ordered operation (implied full 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);
> +}
> +
> +/**
> + * __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. If two instances of this operation race, one
> + * can appear to succeed but actually fail.
> + */
> +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);
> +}
> +
> +/**
> + * test_and_change_bit - Change a bit and return its old value
> + * @nr: Bit to change
> + * @addr: Address to count from
> + *
> + * This is an atomic fully-ordered operation (implied full 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);
> +}
> +
> +/**
> + * __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. If two instances of this operation race, one
> + * can appear to succeed but actually fail.
> + */
> +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);
> +}
> +
> +/**
> + * 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);
> +}
> +
> +#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 operation is atomic and provides release barrier semantics.
> + *
> + * 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
* [PATCH v3] Allow to exclude specific file types in LoadPin
From: Ke Wu @ 2019-05-31 18:25 UTC (permalink / raw)
To: Kees Cook, Jonathan Corbet, James Morris, Serge E. Hallyn
Cc: linux-doc, linux-kernel, linux-security-module, Ke Wu
In-Reply-To: <20190529224350.6460-1-mikewu@google.com>
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>
---
Changelog since v2:
- Make size of exclude_read_files and ignore_read_file_id to be
equal to the size of kernel_read_file_str.
Changelog since v1:
- Mark ignore_read_file_id with __ro_after_init.
- Mark parse_exclude() with __init.
- Use ARRAY_SIZE(ignore_read_file_id) instead of READING_MAX_ID.
Documentation/admin-guide/LSM/LoadPin.rst | 10 ++++++
security/loadpin/loadpin.c | 42 +++++++++++++++++++++++
2 files changed, 52 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..baa8a5b08c53 100644
--- a/security/loadpin/loadpin.c
+++ b/security/loadpin/loadpin.c
@@ -45,6 +45,12 @@ static void report_load(const char *origin, struct file *file, char *operation)
}
static int enforce = IS_ENABLED(CONFIG_SECURITY_LOADPIN_ENFORCE);
+/*
+ * The size should be READING_MAX_ID + 1 to be equal to the size of
+ * kernel_read_file_str.
+ */
+static char *exclude_read_files[READING_MAX_ID + 1];
+static int ignore_read_file_id[READING_MAX_ID + 1] __ro_after_init;
static struct super_block *pinned_root;
static DEFINE_SPINLOCK(pinned_root_spinlock);
@@ -129,6 +135,13 @@ 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 < ARRAY_SIZE(ignore_read_file_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 +200,37 @@ static struct security_hook_list loadpin_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(kernel_load_data, loadpin_load_data),
};
+static void __init 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 +243,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 RFC] Rough draft document on merging and rebasing
From: Linus Torvalds @ 2019-05-31 19:49 UTC (permalink / raw)
To: Jonathan Corbet; +Cc: LKML, open list:DOCUMENTATION
In-Reply-To: <20190530135317.3c8d0d7b@lwn.net>
On Thu, May 30, 2019 at 12:53 PM Jonathan Corbet <corbet@lwn.net> wrote:
>
> This is a first attempt at following through on last month's discussion
> about common merging and rebasing errors.
Looks good to me,
Linus
^ permalink raw reply
* [PATCH v3 1/3] Move *_ucounts functions above
From: Albert Vaca Cintora @ 2019-05-31 19:50 UTC (permalink / raw)
To: albertvaka, akpm, rdunlap, mingo, jack, ebiederm, nsaenzjulienne,
linux-kernel, corbet, linux-doc, mbrugger
So we can use them from proc_handler functions in user_table
Signed-off-by: Albert Vaca Cintora <albertvaka@gmail.com>
---
kernel/ucount.c | 122 ++++++++++++++++++++++++------------------------
1 file changed, 61 insertions(+), 61 deletions(-)
diff --git a/kernel/ucount.c b/kernel/ucount.c
index f48d1b6376a4..909c856e809f 100644
--- a/kernel/ucount.c
+++ b/kernel/ucount.c
@@ -57,6 +57,67 @@ static struct ctl_table_root set_root = {
.permissions = set_permissions,
};
+static struct ucounts *find_ucounts(struct user_namespace *ns, kuid_t uid, struct hlist_head *hashent)
+{
+ struct ucounts *ucounts;
+
+ hlist_for_each_entry(ucounts, hashent, node) {
+ if (uid_eq(ucounts->uid, uid) && (ucounts->ns == ns))
+ return ucounts;
+ }
+ return NULL;
+}
+
+static struct ucounts *get_ucounts(struct user_namespace *ns, kuid_t uid)
+{
+ struct hlist_head *hashent = ucounts_hashentry(ns, uid);
+ struct ucounts *ucounts, *new;
+
+ spin_lock_irq(&ucounts_lock);
+ ucounts = find_ucounts(ns, uid, hashent);
+ if (!ucounts) {
+ spin_unlock_irq(&ucounts_lock);
+
+ new = kzalloc(sizeof(*new), GFP_KERNEL);
+ if (!new)
+ return NULL;
+
+ new->ns = ns;
+ new->uid = uid;
+ new->count = 0;
+
+ spin_lock_irq(&ucounts_lock);
+ ucounts = find_ucounts(ns, uid, hashent);
+ if (ucounts) {
+ kfree(new);
+ } else {
+ hlist_add_head(&new->node, hashent);
+ ucounts = new;
+ }
+ }
+ if (ucounts->count == INT_MAX)
+ ucounts = NULL;
+ else
+ ucounts->count += 1;
+ spin_unlock_irq(&ucounts_lock);
+ return ucounts;
+}
+
+static void put_ucounts(struct ucounts *ucounts)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&ucounts_lock, flags);
+ ucounts->count -= 1;
+ if (!ucounts->count)
+ hlist_del_init(&ucounts->node);
+ else
+ ucounts = NULL;
+ spin_unlock_irqrestore(&ucounts_lock, flags);
+
+ kfree(ucounts);
+}
+
static int zero = 0;
static int int_max = INT_MAX;
#define UCOUNT_ENTRY(name) \
@@ -118,67 +179,6 @@ void retire_userns_sysctls(struct user_namespace *ns)
#endif
}
-static struct ucounts *find_ucounts(struct user_namespace *ns, kuid_t uid, struct hlist_head *hashent)
-{
- struct ucounts *ucounts;
-
- hlist_for_each_entry(ucounts, hashent, node) {
- if (uid_eq(ucounts->uid, uid) && (ucounts->ns == ns))
- return ucounts;
- }
- return NULL;
-}
-
-static struct ucounts *get_ucounts(struct user_namespace *ns, kuid_t uid)
-{
- struct hlist_head *hashent = ucounts_hashentry(ns, uid);
- struct ucounts *ucounts, *new;
-
- spin_lock_irq(&ucounts_lock);
- ucounts = find_ucounts(ns, uid, hashent);
- if (!ucounts) {
- spin_unlock_irq(&ucounts_lock);
-
- new = kzalloc(sizeof(*new), GFP_KERNEL);
- if (!new)
- return NULL;
-
- new->ns = ns;
- new->uid = uid;
- new->count = 0;
-
- spin_lock_irq(&ucounts_lock);
- ucounts = find_ucounts(ns, uid, hashent);
- if (ucounts) {
- kfree(new);
- } else {
- hlist_add_head(&new->node, hashent);
- ucounts = new;
- }
- }
- if (ucounts->count == INT_MAX)
- ucounts = NULL;
- else
- ucounts->count += 1;
- spin_unlock_irq(&ucounts_lock);
- return ucounts;
-}
-
-static void put_ucounts(struct ucounts *ucounts)
-{
- unsigned long flags;
-
- spin_lock_irqsave(&ucounts_lock, flags);
- ucounts->count -= 1;
- if (!ucounts->count)
- hlist_del_init(&ucounts->node);
- else
- ucounts = NULL;
- spin_unlock_irqrestore(&ucounts_lock, flags);
-
- kfree(ucounts);
-}
-
static inline bool atomic_inc_below(atomic_t *v, int u)
{
int c, old;
--
2.21.0
^ permalink raw reply related
* [PATCH v3 2/3] kernel/ucounts: expose count of inotify watches in use
From: Albert Vaca Cintora @ 2019-05-31 19:50 UTC (permalink / raw)
To: albertvaka, akpm, rdunlap, mingo, jack, ebiederm, nsaenzjulienne,
linux-kernel, corbet, linux-doc, mbrugger
In-Reply-To: <20190531195016.4430-1-albertvaka@gmail.com>
Adds a readonly 'current_inotify_watches' entry to the user sysctl table.
The handler for this entry is a custom function that ends up calling
proc_dointvec. Said sysctl table already contains 'max_inotify_watches'
and it gets mounted under /proc/sys/user/.
Inotify watches are a finite resource, in a similar way to available file
descriptors. The motivation for this patch is to be able to set up
monitoring and alerting before an application starts failing because
it runs out of inotify watches.
Signed-off-by: Albert Vaca Cintora <albertvaka@gmail.com>
Acked-by: Jan Kara <jack@suse.cz>
Reviewed-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
---
kernel/ucount.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/kernel/ucount.c b/kernel/ucount.c
index 909c856e809f..05b0e76208d3 100644
--- a/kernel/ucount.c
+++ b/kernel/ucount.c
@@ -118,6 +118,26 @@ static void put_ucounts(struct ucounts *ucounts)
kfree(ucounts);
}
+#ifdef CONFIG_INOTIFY_USER
+int proc_read_inotify_watches(struct ctl_table *table, int write,
+ void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+ struct ucounts *ucounts;
+ struct ctl_table fake_table;
+ int count = -1;
+
+ ucounts = get_ucounts(current_user_ns(), current_euid());
+ if (ucounts != NULL) {
+ count = atomic_read(&ucounts->ucount[UCOUNT_INOTIFY_WATCHES]);
+ put_ucounts(ucounts);
+ }
+
+ fake_table.data = &count;
+ fake_table.maxlen = sizeof(count);
+ return proc_dointvec(&fake_table, write, buffer, lenp, ppos);
+}
+#endif
+
static int zero = 0;
static int int_max = INT_MAX;
#define UCOUNT_ENTRY(name) \
@@ -140,6 +160,12 @@ static struct ctl_table user_table[] = {
#ifdef CONFIG_INOTIFY_USER
UCOUNT_ENTRY("max_inotify_instances"),
UCOUNT_ENTRY("max_inotify_watches"),
+ {
+ .procname = "current_inotify_watches",
+ .maxlen = sizeof(int),
+ .mode = 0444,
+ .proc_handler = proc_read_inotify_watches,
+ },
#endif
{ }
};
--
2.21.0
^ permalink raw reply related
* [PATCH v3 3/3] Documentation for /proc/sys/user/*_inotify_*
From: Albert Vaca Cintora @ 2019-05-31 19:50 UTC (permalink / raw)
To: albertvaka, akpm, rdunlap, mingo, jack, ebiederm, nsaenzjulienne,
linux-kernel, corbet, linux-doc, mbrugger
In-Reply-To: <20190531195016.4430-1-albertvaka@gmail.com>
Added docs for the existing and new inotify-related files
Signed-off-by: Albert Vaca Cintora <albertvaka@gmail.com>
---
Documentation/sysctl/user.txt | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/Documentation/sysctl/user.txt b/Documentation/sysctl/user.txt
index a5882865836e..99c288d39cf6 100644
--- a/Documentation/sysctl/user.txt
+++ b/Documentation/sysctl/user.txt
@@ -30,11 +30,26 @@ user namespace does not allow a user to escape their current limits.
Currently, these files are in /proc/sys/user:
+- current_inotify_watches
+
+ The number of inotify watches in use in the current user namespace.
+ Calling inotify_add_watch() increases this.
+
- max_cgroup_namespaces
The maximum number of cgroup namespaces that any user in the current
user namespace may create.
+- max_inotify_instances
+
+ The maximum number of inotify instances that any user in the current
+ user namespace may create. Calling inotify_init() uses an instance.
+
+- max_inotify_watches
+
+ The maximum number of inotify watches that any user in the current
+ user namespace may create. Calling inotify_add_watch() uses a watch.
+
- max_ipc_namespaces
The maximum number of ipc namespaces that any user in the current
--
2.21.0
^ permalink raw reply related
* Re: [PATCH v3] Allow to exclude specific file types in LoadPin
From: Kees Cook @ 2019-05-31 21:01 UTC (permalink / raw)
To: Ke Wu
Cc: Jonathan Corbet, James Morris, Serge E. Hallyn, linux-doc,
linux-kernel, linux-security-module
In-Reply-To: <20190531182553.51721-1-mikewu@google.com>
On Fri, May 31, 2019 at 11:25:53AM -0700, Ke Wu wrote:
> 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>
> ---
> Changelog since v2:
> - Make size of exclude_read_files and ignore_read_file_id to be
> equal to the size of kernel_read_file_str.
Thanks! I've fixed this differently and it should be visible shortly.
-Kees
>
> Changelog since v1:
> - Mark ignore_read_file_id with __ro_after_init.
> - Mark parse_exclude() with __init.
> - Use ARRAY_SIZE(ignore_read_file_id) instead of READING_MAX_ID.
>
>
> Documentation/admin-guide/LSM/LoadPin.rst | 10 ++++++
> security/loadpin/loadpin.c | 42 +++++++++++++++++++++++
> 2 files changed, 52 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..baa8a5b08c53 100644
> --- a/security/loadpin/loadpin.c
> +++ b/security/loadpin/loadpin.c
> @@ -45,6 +45,12 @@ static void report_load(const char *origin, struct file *file, char *operation)
> }
>
> static int enforce = IS_ENABLED(CONFIG_SECURITY_LOADPIN_ENFORCE);
> +/*
> + * The size should be READING_MAX_ID + 1 to be equal to the size of
> + * kernel_read_file_str.
> + */
> +static char *exclude_read_files[READING_MAX_ID + 1];
> +static int ignore_read_file_id[READING_MAX_ID + 1] __ro_after_init;
> static struct super_block *pinned_root;
> static DEFINE_SPINLOCK(pinned_root_spinlock);
>
> @@ -129,6 +135,13 @@ 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 < ARRAY_SIZE(ignore_read_file_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 +200,37 @@ static struct security_hook_list loadpin_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(kernel_load_data, loadpin_load_data),
> };
>
> +static void __init 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 +243,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
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH RFC] Rough draft document on merging and rebasing
From: David Rientjes @ 2019-05-31 21:14 UTC (permalink / raw)
To: Jonathan Corbet; +Cc: Linus Torvalds, LKML, linux-doc
In-Reply-To: <20190530135317.3c8d0d7b@lwn.net>
On Thu, 30 May 2019, Jonathan Corbet wrote:
> docs: Add a document on repository management
>
> Every merge window seems to involve at least one episode where subsystem
> maintainers don't manage their trees as Linus would like. Document the
> expectations so that at least he has something to point people to.
>
> Signed-off-by: Jonathan Corbet <corbet@lwn.net>
We're in the process of defining the pros and cons of merging vs rebasing
as part of our workflows internally and this is a great doc that does that
with less words and more clarity than I did it, so very happy to add
Acked-by: David Rientjes <rientjes@google.com>
^ permalink raw reply
* Re: [PATCH RFC] Rough draft document on merging and rebasing
From: Jonathan Corbet @ 2019-05-31 23:36 UTC (permalink / raw)
To: Randy Dunlap; +Cc: Linus Torvalds, LKML, linux-doc
In-Reply-To: <7979b995-6b03-783b-e3d7-0023fabc43bc@infradead.org>
On Thu, 30 May 2019 17:45:23 -0700
Randy Dunlap <rdunlap@infradead.org> wrote:
> On 5/30/19 12:53 PM, Jonathan Corbet wrote:
> > + git merge v5.2-rc1^0
>
> That line is presented in my email client (Thunderbird) as
>
> git merge v5.2-rc1{superscript 0}
>
> Could you escape/quote it to prevent that?
So I'm a wee bit confused. That's a literal string that one needs to
type to obtain the needed effect; if thunderbird is doing weird things
with it, I think that the problem does not lie with the document...? What
change would you have me make here?
Thanks,
jon
^ permalink raw reply
* Re: [PATCH RFC] Rough draft document on merging and rebasing
From: Randy Dunlap @ 2019-05-31 23:52 UTC (permalink / raw)
To: Jonathan Corbet; +Cc: Linus Torvalds, LKML, linux-doc
In-Reply-To: <20190531173618.465ae659@lwn.net>
On 5/31/19 4:36 PM, Jonathan Corbet wrote:
> On Thu, 30 May 2019 17:45:23 -0700
> Randy Dunlap <rdunlap@infradead.org> wrote:
>
>> On 5/30/19 12:53 PM, Jonathan Corbet wrote:
>>> + git merge v5.2-rc1^0
>>
>> That line is presented in my email client (Thunderbird) as
>>
>> git merge v5.2-rc1{superscript 0}
>>
>> Could you escape/quote it to prevent that?
>
> So I'm a wee bit confused. That's a literal string that one needs to
> type to obtain the needed effect; if thunderbird is doing weird things
> with it, I think that the problem does not lie with the document...? What
> change would you have me make here?
I dunno. I just won't depend on my email client.
I'll read it some other way (like a text editor).
cheers.
--
~Randy
^ permalink raw reply
* Re: [PATCH v3 2/3] kernel/ucounts: expose count of inotify watches in use
From: Andrew Morton @ 2019-06-01 0:00 UTC (permalink / raw)
To: Albert Vaca Cintora
Cc: rdunlap, mingo, jack, ebiederm, nsaenzjulienne, linux-kernel,
corbet, linux-doc, mbrugger
In-Reply-To: <20190531195016.4430-2-albertvaka@gmail.com>
On Fri, 31 May 2019 21:50:15 +0200 Albert Vaca Cintora <albertvaka@gmail.com> wrote:
> Adds a readonly 'current_inotify_watches' entry to the user sysctl table.
> The handler for this entry is a custom function that ends up calling
> proc_dointvec. Said sysctl table already contains 'max_inotify_watches'
> and it gets mounted under /proc/sys/user/.
>
> Inotify watches are a finite resource, in a similar way to available file
> descriptors. The motivation for this patch is to be able to set up
> monitoring and alerting before an application starts failing because
> it runs out of inotify watches.
>
> ...
>
> --- a/kernel/ucount.c
> +++ b/kernel/ucount.c
> @@ -118,6 +118,26 @@ static void put_ucounts(struct ucounts *ucounts)
> kfree(ucounts);
> }
>
> +#ifdef CONFIG_INOTIFY_USER
> +int proc_read_inotify_watches(struct ctl_table *table, int write,
> + void __user *buffer, size_t *lenp, loff_t *ppos)
> +{
> + struct ucounts *ucounts;
> + struct ctl_table fake_table;
hmm.
> + int count = -1;
> +
> + ucounts = get_ucounts(current_user_ns(), current_euid());
> + if (ucounts != NULL) {
> + count = atomic_read(&ucounts->ucount[UCOUNT_INOTIFY_WATCHES]);
> + put_ucounts(ucounts);
> + }
> +
> + fake_table.data = &count;
> + fake_table.maxlen = sizeof(count);
> + return proc_dointvec(&fake_table, write, buffer, lenp, ppos);
proc_dointvec
->do_proc_dointvec
->__do_proc_dointvec
->proc_first_pos_non_zero_ignore
->warn_sysctl_write
->pr_warn_once(..., table->procname)
and I think ->procname is uninitialized.
That's from a cursory check. Perhaps other uninitialized members of
fake_table are accessed, dunno.
we could do
{
struct ctl_table fake_table = {
.data = &count,
.maxlen = sizeof(count),
};
return proc_dointvec(&fake_table, write, buffer, lenp, ppos);
}
or whatever. That will cause the pr_warn_once to print "(null)" but
that's OK I guess.
Are there other places in the kernel which do this temp ctl_table
trick? If so, what do they do? If not, what is special about this
code?
^ permalink raw reply
* Re: [PATCH 2/3] x86: Move CPU feature test out of uaccess region
From: hpa @ 2019-05-31 23:41 UTC (permalink / raw)
To: Marco Elver
Cc: Peter Zijlstra, Andrey Ryabinin, Dmitry Vyukov,
Alexander Potapenko, Andrey Konovalov, Mark Rutland,
Jonathan Corbet, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
the arch/x86 maintainers, Arnd Bergmann, Josh Poimboeuf,
open list:DOCUMENTATION, LKML, linux-arch, kasan-dev
In-Reply-To: <CANpmjNOsPnVd50cTzUW8UYXPGqpSnRLcjj=JbZraTYVq1n18Fw@mail.gmail.com>
On May 31, 2019 2:57:36 AM PDT, Marco Elver <elver@google.com> wrote:
>On Wed, 29 May 2019 at 16:29, <hpa@zytor.com> wrote:
>>
>> 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?
>
>I couldn't find any mailing list thread referring to why this doesn't
>use static_cpu_has, do you have any background?
>
>static_cpu_has also solves the UACCESS warning.
>
>If you confirm it is safe to change to static_cpu_has(), I will change
>this patch. Note that I should then also change
>arch/x86/kernel/signal.c to mirror the change for 32bit (although
>KASAN is not supported for 32bit x86).
>
>Thanks,
>-- Marco
I believe at some point the intent was that boot_cpu_has() was safer and could be used everywhere.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* [PATCH RFC 1/1] doc/rcu: Add some more listRCU patterns in the kernel
From: Joel Fernandes (Google) @ 2019-06-01 9:39 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), rcu, Jonathan Corbet, Josh Triplett,
kernel-team, Lai Jiangshan, linux-doc, Mathieu Desnoyers,
Paul E. McKenney, Steven Rostedt, peterz
We keep the initially written audit examples and add to it, since the
code that audit has is still relevant even though slightly different in
the kernel.
Cc: rcu@vger.kernel.org
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
Documentation/RCU/listRCU.txt | 154 +++++++++++++++++++++++++++++++---
1 file changed, 144 insertions(+), 10 deletions(-)
diff --git a/Documentation/RCU/listRCU.txt b/Documentation/RCU/listRCU.txt
index adb5a3782846..af5bf1bd689c 100644
--- a/Documentation/RCU/listRCU.txt
+++ b/Documentation/RCU/listRCU.txt
@@ -7,8 +7,54 @@ is that all of the required memory barriers are included for you in
the list macros. This document describes several applications of RCU,
with the best fits first.
-
-Example 1: Read-Side Action Taken Outside of Lock, No In-Place Updates
+Example 1: Read-mostly list: Deferred Destruction
+
+A widely used usecase for RCU lists in the kernel is lockless iteration over
+all processes in the system. task_struct::tasks represents the list node that
+links all the processes. The list can be traversed in parallel to any list
+additions or removals.
+
+The traversal of the list is done using for_each_process() which is defined by
+the 2 macros:
+
+#define next_task(p) \
+ list_entry_rcu((p)->tasks.next, struct task_struct, tasks)
+
+#define for_each_process(p) \
+ for (p = &init_task ; (p = next_task(p)) != &init_task ; )
+
+The code traversing the list of all processes typically looks like:
+rcu_read_lock();
+for_each_process(p) {
+ /* Do something with p */
+}
+rcu_read_unlock();
+
+Thes code (simplified) removing a process from the task lists is in
+release_task():
+
+void release_task(struct task_struct *p)
+{
+ write_lock(&tasklist_lock);
+ list_del_rcu(&p->tasks);
+ write_unlock(&tasklist_lock);
+ call_rcu(&p->rcu, delayed_put_task_struct);
+}
+
+When a process exits, release_task() calls list_del_rcu(&p->tasks) to remove
+the task from the list of all tasks, under tasklist_lock writer lock
+protection. The tasklist_lock prevents concurrent list adds/removes from
+corrupting the list. Readers using for_each_process() are not protected with
+the tasklist_lock. To prevent readers from appearing to notice changes in the
+list pointers, the task_struct object is freed only after one more more grace
+periods elapse (with the help of call_rcu). This deferring of destruction
+ensures that any readers traversing the list will see valid p->tasks.next
+pointers and deletion/freeing can happen in parallel to traversal of the list.
+This pattern is also called an "existence lock" sometimes, since RCU makes sure
+the object exists in memory as long as readers exist, that are traversing.
+
+
+Example 2: Read-Side Action Taken Outside of Lock, No In-Place Updates
The best applications are cases where, if reader-writer locking were
used, the read-side lock would be dropped before taking any action
@@ -32,7 +78,7 @@ implementation of audit_filter_task() might be as follows:
enum audit_state state;
read_lock(&auditsc_lock);
- /* Note: audit_netlink_sem held by caller. */
+ /* Note: audit_filter_mutex held by caller. */
list_for_each_entry(e, &audit_tsklist, list) {
if (audit_filter_rules(tsk, &e->rule, NULL, &state)) {
read_unlock(&auditsc_lock);
@@ -56,7 +102,7 @@ This means that RCU can be easily applied to the read side, as follows:
enum audit_state state;
rcu_read_lock();
- /* Note: audit_netlink_sem held by caller. */
+ /* Note: audit_filter_mutex held by caller. */
list_for_each_entry_rcu(e, &audit_tsklist, list) {
if (audit_filter_rules(tsk, &e->rule, NULL, &state)) {
rcu_read_unlock();
@@ -139,7 +185,7 @@ Following are the RCU equivalents for these two functions:
Normally, the write_lock() and write_unlock() would be replaced by
a spin_lock() and a spin_unlock(), but in this case, all callers hold
-audit_netlink_sem, so no additional locking is required. The auditsc_lock
+audit_filter_mutex, so no additional locking is required. The auditsc_lock
can therefore be eliminated, since use of RCU eliminates the need for
writers to exclude readers. Normally, the write_lock() calls would
be converted into spin_lock() calls.
@@ -155,7 +201,7 @@ So, when readers can tolerate stale data and when entries are either added
or deleted, without in-place modification, it is very easy to use RCU!
-Example 2: Handling In-Place Updates
+Example 3: Handling In-Place Updates
The system-call auditing code does not update auditing rules in place.
However, if it did, reader-writer-locked code to do so might look as
@@ -171,7 +217,7 @@ otherwise, the added fields would need to be filled in):
struct audit_newentry *ne;
write_lock(&auditsc_lock);
- /* Note: audit_netlink_sem held by caller. */
+ /* Note: audit_filter_mutex held by caller. */
list_for_each_entry(e, list, list) {
if (!audit_compare_rule(rule, &e->rule)) {
e->rule.action = newaction;
@@ -213,13 +259,23 @@ RCU ("read-copy update") its name. The RCU code is as follows:
return -EFAULT; /* No matching rule */
}
-Again, this assumes that the caller holds audit_netlink_sem. Normally,
+Again, this assumes that the caller holds audit_filter_mutex. Normally,
the reader-writer lock would become a spinlock in this sort of code.
+Another use of this pattern can be found in the openswitch driver's "connection
+tracking table" code (ct_limit_set()). The table holds connection tracking
+entries and has a limit on the maximum entries. There is one such table
+per-zone and hence one "limit" per zone. The zones are mapped to their limits
+through a hashtable using an RCU-managed hlist for the hash chains. When a new
+limit is to be set, a new limit object is allocated and ct_limit_set() is
+called to replace the old limit object with the new one using
+list_replace_rcu(). The old limit object is then freed after a grace period
+using kfree_rcu().
+
-Example 3: Eliminating Stale Data
+Example 4: Eliminating Stale Data
-The auditing examples above tolerate stale data, as do most algorithms
+The auditing exampes above tolerates stale data, as do most algorithms
that are tracking external state. Because there is a delay from the
time the external state changes before Linux becomes aware of the change,
additional RCU-induced staleness is normally not a problem.
@@ -291,6 +347,84 @@ flag under the spinlock as follows:
}
+EXAMPLE 5: Skipping Stale Objects
+
+Stale data can also be eliminated for performance reasons since it is pointless
+to process items in a list, if the object is being destroyed. One such example
+can be found in the timerfd subsystem. When a CLOCK_REALTIME clock is
+reprogrammed - for example due to setting of the system time, then all programmed
+timerfds that depend on this clock get triggered and processes waiting on them
+to expire are woken up in advance of their scheduled expiry. To facilitate
+this, all such timers are added to a 'cancel_list' when they are setup in
+timerfd_setup_cancel:
+
+static void timerfd_setup_cancel(struct timerfd_ctx *ctx, int flags)
+{
+ spin_lock(&ctx->cancel_lock);
+ if ((ctx->clockid == CLOCK_REALTIME &&
+ (flags & TFD_TIMER_ABSTIME) && (flags & TFD_TIMER_CANCEL_ON_SET)) {
+ if (!ctx->might_cancel) {
+ ctx->might_cancel = true;
+ spin_lock(&cancel_lock);
+ list_add_rcu(&ctx->clist, &cancel_list);
+ spin_unlock(&cancel_lock);
+ }
+ }
+ spin_unlock(&ctx->cancel_lock);
+}
+
+When a timerfd is freed (fd is closed), then the might_cancel flag of the
+timerfd object is cleared, the object removed from the cancel_list and destroyed:
+
+int timerfd_release(struct inode *inode, struct file *file)
+{
+ struct timerfd_ctx *ctx = file->private_data;
+
+ spin_lock(&ctx->cancel_lock);
+ if (ctx->might_cancel) {
+ ctx->might_cancel = false;
+ spin_lock(&cancel_lock);
+ list_del_rcu(&ctx->clist);
+ spin_unlock(&cancel_lock);
+ }
+ spin_unlock(&ctx->cancel_lock);
+
+ hrtimer_cancel(&ctx->t.tmr);
+ kfree_rcu(ctx, rcu);
+ return 0;
+}
+
+If the CLOCK_REALTIME clock is set, for example by a time server, the hrtimer
+framework calls timerfd_clock_was_set() which walks the cancel_list and wakes
+up processes waiting on the timerfd. While iterating the cancel list, the
+might_cancel flag is consulted to skip stale objects:
+
+void timerfd_clock_was_set(void)
+{
+ struct timerfd_ctx *ctx;
+ unsigned long flags;
+
+ rcu_read_lock();
+ list_for_each_entry_rcu(ctx, &cancel_list, clist) {
+ if (!ctx->might_cancel)
+ continue;
+ spin_lock_irqsave(&ctx->wqh.lock, flags);
+ if (ctx->moffs != ktime_mono_to_real(0)) {
+ ctx->moffs = KTIME_MAX;
+ ctx->ticks++;
+ wake_up_locked_poll(&ctx->wqh, EPOLLIN);
+ }
+ spin_unlock_irqrestore(&ctx->wqh.lock, flags);
+ }
+ rcu_read_unlock();
+}
+
+The key point here is, because RCU-traversal of the cancel_list happens while
+objects are being added and removed to the list, sometimes the traversal can
+step on an object that has been removed from the list. In this example, it is
+seen that it is better to skip such objects using a flag.
+
+
Summary
Read-mostly list-based data structures that can tolerate stale data are
--
2.22.0.rc1.311.g5d7573a151-goog
^ permalink raw reply related
* Re: [PATCH RFC 1/1] doc/rcu: Add some more listRCU patterns in the kernel
From: Joel Fernandes @ 2019-06-01 9:41 UTC (permalink / raw)
To: LKML, Neil Brown
Cc: rcu, Jonathan Corbet, Josh Triplett, kernel-team, Lai Jiangshan,
open list:DOCUMENTATION, Mathieu Desnoyers, Paul E. McKenney,
Steven Rostedt, Peter Zilstra
In-Reply-To: <20190601093926.28158-1-joel@joelfernandes.org>
Forgot to CC +Neil Brown , will do in the next posting, thanks!
On Sat, Jun 1, 2019 at 5:39 AM Joel Fernandes (Google)
<joel@joelfernandes.org> wrote:
>
> We keep the initially written audit examples and add to it, since the
> code that audit has is still relevant even though slightly different in
> the kernel.
>
> Cc: rcu@vger.kernel.org
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
> Documentation/RCU/listRCU.txt | 154 +++++++++++++++++++++++++++++++---
> 1 file changed, 144 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/RCU/listRCU.txt b/Documentation/RCU/listRCU.txt
> index adb5a3782846..af5bf1bd689c 100644
> --- a/Documentation/RCU/listRCU.txt
> +++ b/Documentation/RCU/listRCU.txt
> @@ -7,8 +7,54 @@ is that all of the required memory barriers are included for you in
> the list macros. This document describes several applications of RCU,
> with the best fits first.
>
> -
> -Example 1: Read-Side Action Taken Outside of Lock, No In-Place Updates
> +Example 1: Read-mostly list: Deferred Destruction
> +
> +A widely used usecase for RCU lists in the kernel is lockless iteration over
> +all processes in the system. task_struct::tasks represents the list node that
> +links all the processes. The list can be traversed in parallel to any list
> +additions or removals.
> +
> +The traversal of the list is done using for_each_process() which is defined by
> +the 2 macros:
> +
> +#define next_task(p) \
> + list_entry_rcu((p)->tasks.next, struct task_struct, tasks)
> +
> +#define for_each_process(p) \
> + for (p = &init_task ; (p = next_task(p)) != &init_task ; )
> +
> +The code traversing the list of all processes typically looks like:
> +rcu_read_lock();
> +for_each_process(p) {
> + /* Do something with p */
> +}
> +rcu_read_unlock();
> +
> +Thes code (simplified) removing a process from the task lists is in
> +release_task():
> +
> +void release_task(struct task_struct *p)
> +{
> + write_lock(&tasklist_lock);
> + list_del_rcu(&p->tasks);
> + write_unlock(&tasklist_lock);
> + call_rcu(&p->rcu, delayed_put_task_struct);
> +}
> +
> +When a process exits, release_task() calls list_del_rcu(&p->tasks) to remove
> +the task from the list of all tasks, under tasklist_lock writer lock
> +protection. The tasklist_lock prevents concurrent list adds/removes from
> +corrupting the list. Readers using for_each_process() are not protected with
> +the tasklist_lock. To prevent readers from appearing to notice changes in the
> +list pointers, the task_struct object is freed only after one more more grace
> +periods elapse (with the help of call_rcu). This deferring of destruction
> +ensures that any readers traversing the list will see valid p->tasks.next
> +pointers and deletion/freeing can happen in parallel to traversal of the list.
> +This pattern is also called an "existence lock" sometimes, since RCU makes sure
> +the object exists in memory as long as readers exist, that are traversing.
> +
> +
> +Example 2: Read-Side Action Taken Outside of Lock, No In-Place Updates
>
> The best applications are cases where, if reader-writer locking were
> used, the read-side lock would be dropped before taking any action
> @@ -32,7 +78,7 @@ implementation of audit_filter_task() might be as follows:
> enum audit_state state;
>
> read_lock(&auditsc_lock);
> - /* Note: audit_netlink_sem held by caller. */
> + /* Note: audit_filter_mutex held by caller. */
> list_for_each_entry(e, &audit_tsklist, list) {
> if (audit_filter_rules(tsk, &e->rule, NULL, &state)) {
> read_unlock(&auditsc_lock);
> @@ -56,7 +102,7 @@ This means that RCU can be easily applied to the read side, as follows:
> enum audit_state state;
>
> rcu_read_lock();
> - /* Note: audit_netlink_sem held by caller. */
> + /* Note: audit_filter_mutex held by caller. */
> list_for_each_entry_rcu(e, &audit_tsklist, list) {
> if (audit_filter_rules(tsk, &e->rule, NULL, &state)) {
> rcu_read_unlock();
> @@ -139,7 +185,7 @@ Following are the RCU equivalents for these two functions:
>
> Normally, the write_lock() and write_unlock() would be replaced by
> a spin_lock() and a spin_unlock(), but in this case, all callers hold
> -audit_netlink_sem, so no additional locking is required. The auditsc_lock
> +audit_filter_mutex, so no additional locking is required. The auditsc_lock
> can therefore be eliminated, since use of RCU eliminates the need for
> writers to exclude readers. Normally, the write_lock() calls would
> be converted into spin_lock() calls.
> @@ -155,7 +201,7 @@ So, when readers can tolerate stale data and when entries are either added
> or deleted, without in-place modification, it is very easy to use RCU!
>
>
> -Example 2: Handling In-Place Updates
> +Example 3: Handling In-Place Updates
>
> The system-call auditing code does not update auditing rules in place.
> However, if it did, reader-writer-locked code to do so might look as
> @@ -171,7 +217,7 @@ otherwise, the added fields would need to be filled in):
> struct audit_newentry *ne;
>
> write_lock(&auditsc_lock);
> - /* Note: audit_netlink_sem held by caller. */
> + /* Note: audit_filter_mutex held by caller. */
> list_for_each_entry(e, list, list) {
> if (!audit_compare_rule(rule, &e->rule)) {
> e->rule.action = newaction;
> @@ -213,13 +259,23 @@ RCU ("read-copy update") its name. The RCU code is as follows:
> return -EFAULT; /* No matching rule */
> }
>
> -Again, this assumes that the caller holds audit_netlink_sem. Normally,
> +Again, this assumes that the caller holds audit_filter_mutex. Normally,
> the reader-writer lock would become a spinlock in this sort of code.
>
> +Another use of this pattern can be found in the openswitch driver's "connection
> +tracking table" code (ct_limit_set()). The table holds connection tracking
> +entries and has a limit on the maximum entries. There is one such table
> +per-zone and hence one "limit" per zone. The zones are mapped to their limits
> +through a hashtable using an RCU-managed hlist for the hash chains. When a new
> +limit is to be set, a new limit object is allocated and ct_limit_set() is
> +called to replace the old limit object with the new one using
> +list_replace_rcu(). The old limit object is then freed after a grace period
> +using kfree_rcu().
> +
>
> -Example 3: Eliminating Stale Data
> +Example 4: Eliminating Stale Data
>
> -The auditing examples above tolerate stale data, as do most algorithms
> +The auditing exampes above tolerates stale data, as do most algorithms
> that are tracking external state. Because there is a delay from the
> time the external state changes before Linux becomes aware of the change,
> additional RCU-induced staleness is normally not a problem.
> @@ -291,6 +347,84 @@ flag under the spinlock as follows:
> }
>
>
> +EXAMPLE 5: Skipping Stale Objects
> +
> +Stale data can also be eliminated for performance reasons since it is pointless
> +to process items in a list, if the object is being destroyed. One such example
> +can be found in the timerfd subsystem. When a CLOCK_REALTIME clock is
> +reprogrammed - for example due to setting of the system time, then all programmed
> +timerfds that depend on this clock get triggered and processes waiting on them
> +to expire are woken up in advance of their scheduled expiry. To facilitate
> +this, all such timers are added to a 'cancel_list' when they are setup in
> +timerfd_setup_cancel:
> +
> +static void timerfd_setup_cancel(struct timerfd_ctx *ctx, int flags)
> +{
> + spin_lock(&ctx->cancel_lock);
> + if ((ctx->clockid == CLOCK_REALTIME &&
> + (flags & TFD_TIMER_ABSTIME) && (flags & TFD_TIMER_CANCEL_ON_SET)) {
> + if (!ctx->might_cancel) {
> + ctx->might_cancel = true;
> + spin_lock(&cancel_lock);
> + list_add_rcu(&ctx->clist, &cancel_list);
> + spin_unlock(&cancel_lock);
> + }
> + }
> + spin_unlock(&ctx->cancel_lock);
> +}
> +
> +When a timerfd is freed (fd is closed), then the might_cancel flag of the
> +timerfd object is cleared, the object removed from the cancel_list and destroyed:
> +
> +int timerfd_release(struct inode *inode, struct file *file)
> +{
> + struct timerfd_ctx *ctx = file->private_data;
> +
> + spin_lock(&ctx->cancel_lock);
> + if (ctx->might_cancel) {
> + ctx->might_cancel = false;
> + spin_lock(&cancel_lock);
> + list_del_rcu(&ctx->clist);
> + spin_unlock(&cancel_lock);
> + }
> + spin_unlock(&ctx->cancel_lock);
> +
> + hrtimer_cancel(&ctx->t.tmr);
> + kfree_rcu(ctx, rcu);
> + return 0;
> +}
> +
> +If the CLOCK_REALTIME clock is set, for example by a time server, the hrtimer
> +framework calls timerfd_clock_was_set() which walks the cancel_list and wakes
> +up processes waiting on the timerfd. While iterating the cancel list, the
> +might_cancel flag is consulted to skip stale objects:
> +
> +void timerfd_clock_was_set(void)
> +{
> + struct timerfd_ctx *ctx;
> + unsigned long flags;
> +
> + rcu_read_lock();
> + list_for_each_entry_rcu(ctx, &cancel_list, clist) {
> + if (!ctx->might_cancel)
> + continue;
> + spin_lock_irqsave(&ctx->wqh.lock, flags);
> + if (ctx->moffs != ktime_mono_to_real(0)) {
> + ctx->moffs = KTIME_MAX;
> + ctx->ticks++;
> + wake_up_locked_poll(&ctx->wqh, EPOLLIN);
> + }
> + spin_unlock_irqrestore(&ctx->wqh.lock, flags);
> + }
> + rcu_read_unlock();
> +}
> +
> +The key point here is, because RCU-traversal of the cancel_list happens while
> +objects are being added and removed to the list, sometimes the traversal can
> +step on an object that has been removed from the list. In this example, it is
> +seen that it is better to skip such objects using a flag.
> +
> +
> Summary
>
> Read-mostly list-based data structures that can tolerate stale data are
> --
> 2.22.0.rc1.311.g5d7573a151-goog
>
^ permalink raw reply
* Re: [PATCH RFC] Rough draft document on merging and rebasing
From: Theodore Ts'o @ 2019-06-01 15:42 UTC (permalink / raw)
To: Jonathan Corbet; +Cc: Linus Torvalds, LKML, linux-doc
In-Reply-To: <20190530135317.3c8d0d7b@lwn.net>
On Thu, May 30, 2019 at 01:53:17PM -0600, Jonathan Corbet wrote:
> +Rebasing
> +========
> +
> +"Rebasing" is the process of changing the history of a series of commits
> +within a repository. At its simplest, a rebase could change the starting
> +point of a patch series from one point to another. Other uses include
> +fixing (or deleting) broken commits, adding tags to commits, or changing
> +the order in which commits are applied. Used properly, rebasing can yield
> +a cleaner and clearer development history; used improperly, it can obscure
> +that history and introduce bugs.
Rebasing is being used in two senses here. The first is where the
diffs don't change (much), but the starting point of the changes is
being changed. The second is where a broken commit is dropped, adding
a signed-off-by, etc.
Both have the property that they can used for good or ill, but the
details when this is an issue can change a bit. More below...
> +There are a few rules of thumb that can help developers to avoid the worst
> +perils of rebasing:
> +
> + - History that has been exposed to the world beyond your private system
> + should not be rebased. Others may have pulled a copy of your tree and
> + built on it; rebasing your tree will create pain for them. If work is
> + in need of rebasing, that is usually a sign that it is not yet ready to
> + be committed to a public repository.
That seems to be a bit too categorical. It's been recommended, and
some people do, push patches to a branch so the zero-day bot will pick
it up and test for potential problems. And broken commits *do* get
dropped from candidate stable kernel releases. And, of course,
there's linux-next, which is constantly getting rebased.
And there have been people who have pushed out RFC patche series onto
a git branch, and publicized it on LKML for the convenience of
reviewers. (Perhaps because there is a patch which is so big it
exceeds the LKML size restrictions.)
I think it's more about whether people know that a branch is
considered unstable from a historical perspective or not. No one
builds on top of linux-next because, well, that would be silly.
Finally, I'm bit concerned about anything which states absolutes,
because there are people who tend to be real stickler for the rules,
and if they see something stated in absolute terms, they fail to
understand that there are exceptions that are well understood, and in
use for years before the existence of the document which is trying to
codify best practices.
> + - Realize the rebasing a patch series changes the environment in which it
> + was developed and, likely, invalidates much of the testing that was
> + done. A rebased patch series should, as a general rule, be treated like
> + new code and retested from the beginning.
In this paragraph, "rebasing" is being used in the change the base
commit on top of which a series of changes were based upon. And it's
what most people think of when they see the word "rebase".
However "git rebase" is also used to rewrite git history when dropping
a bad commit, or fixing things so the branch is bisectable, etc. But
in the definitions above, the word "rebase" was defined to include
both "changing the base of a patch stack", and "rewriting git
history". I wonder if that helps more than it hinders understanding.
At least in my mind, "rebasing" is much more often the wrong thing,
where as "rewriting git history" for a leaf repository can be more
often justifable. And of course, if someone is working on a feature
for which the review and development cycle takes several kernel
releases, I'd claim that "rebasing" in that case always makes sense,
even if they *have* exposed their patch series via git for review /
commenting purposes.
Regards,
- Ted
^ permalink raw reply
* Re: [PATCH v3 2/3] kernel/ucounts: expose count of inotify watches in use
From: Albert Vaca Cintora @ 2019-06-01 18:20 UTC (permalink / raw)
To: Andrew Morton
Cc: rdunlap, mingo, Jan Kara, ebiederm, Nicolas Saenz Julienne,
linux-kernel, corbet, linux-doc, Matthias Brugger
In-Reply-To: <20190531170046.ac2b52d8c4923fdeedf943cc@linux-foundation.org>
On Sat, Jun 1, 2019 at 2:00 AM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Fri, 31 May 2019 21:50:15 +0200 Albert Vaca Cintora <albertvaka@gmail.com> wrote:
>
> > Adds a readonly 'current_inotify_watches' entry to the user sysctl table.
> > The handler for this entry is a custom function that ends up calling
> > proc_dointvec. Said sysctl table already contains 'max_inotify_watches'
> > and it gets mounted under /proc/sys/user/.
> >
> > Inotify watches are a finite resource, in a similar way to available file
> > descriptors. The motivation for this patch is to be able to set up
> > monitoring and alerting before an application starts failing because
> > it runs out of inotify watches.
> >
> > ...
> >
> > --- a/kernel/ucount.c
> > +++ b/kernel/ucount.c
> > @@ -118,6 +118,26 @@ static void put_ucounts(struct ucounts *ucounts)
> > kfree(ucounts);
> > }
> >
> > +#ifdef CONFIG_INOTIFY_USER
> > +int proc_read_inotify_watches(struct ctl_table *table, int write,
> > + void __user *buffer, size_t *lenp, loff_t *ppos)
> > +{
> > + struct ucounts *ucounts;
> > + struct ctl_table fake_table;
>
> hmm.
>
> > + int count = -1;
> > +
> > + ucounts = get_ucounts(current_user_ns(), current_euid());
> > + if (ucounts != NULL) {
> > + count = atomic_read(&ucounts->ucount[UCOUNT_INOTIFY_WATCHES]);
> > + put_ucounts(ucounts);
> > + }
> > +
> > + fake_table.data = &count;
> > + fake_table.maxlen = sizeof(count);
> > + return proc_dointvec(&fake_table, write, buffer, lenp, ppos);
>
> proc_dointvec
> ->do_proc_dointvec
> ->__do_proc_dointvec
> ->proc_first_pos_non_zero_ignore
> ->warn_sysctl_write
> ->pr_warn_once(..., table->procname)
>
> and I think ->procname is uninitialized.
>
> That's from a cursory check. Perhaps other uninitialized members of
> fake_table are accessed, dunno.
>
> we could do
>
> {
> struct ctl_table fake_table = {
> .data = &count,
> .maxlen = sizeof(count),
> };
>
> return proc_dointvec(&fake_table, write, buffer, lenp, ppos);
> }
>
> or whatever. That will cause the pr_warn_once to print "(null)" but
> that's OK I guess.
>
> Are there other places in the kernel which do this temp ctl_table
> trick? If so, what do they do? If not, what is special about this
> code?
>
>
I copied this 'fake_table' trick from proc_do_entropy in
drivers/char/random.c exactly as it is. It is also used in other
places with slight variations.
Note that, since we are creating a read-only proc file,
proc_first_pos_non_zero_ignore is not called from __do_proc_dointvec,
so the uninitialized ->procname is not accessed.
Albert
^ permalink raw reply
* Re: [PATCH 04/22] docs: zh_CN: get rid of basic_profiling.txt
From: Alex Shi @ 2019-06-02 14:36 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Linux Doc Mailing List
Cc: Mauro Carvalho Chehab, linux-kernel, Jonathan Corbet, Harry Wei
In-Reply-To: <81f5848d02cafb986d9145dbff17beb1e4427dea.1559171394.git.mchehab+samsung@kernel.org>
On 2019/5/30 7:23 上午, Mauro Carvalho Chehab wrote:
> Changeset 5700d1974818 ("docs: Get rid of the "basic profiling" guide")
> removed an old basic-profiling.txt file that was not updated over
> the last 11 years and won't reflect the post-perf era.
>
> It makes no sense to keep its translation, so get rid of it too.
>
> Fixes: 5700d1974818 ("docs: Get rid of the "basic profiling" guide")
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> ---
Acked-by: Alex Shi <alex.shi@linux.alibaba.com>
^ permalink raw reply
* Re: [PATCH 13/22] docs: zh_CN: avoid duplicate citation references
From: Alex Shi @ 2019-06-02 15:01 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Linux Doc Mailing List
Cc: Mauro Carvalho Chehab, linux-kernel, Jonathan Corbet, Harry Wei
In-Reply-To: <9d3b9729663f75249b514dd3910309eb418d9e46.1559171394.git.mchehab+samsung@kernel.org>
On 2019/5/30 7:23 上午, Mauro Carvalho Chehab wrote:
> Documentation/process/management-style.rst:35: WARNING: duplicate label decisions, other instance in Documentation/translations/zh_CN/process/management-style.rst
> Documentation/process/programming-language.rst:37: WARNING: duplicate citation c-language, other instance in Documentation/translations/zh_CN/process/programming-language.rst
> Documentation/process/programming-language.rst:38: WARNING: duplicate citation gcc, other instance in Documentation/translations/zh_CN/process/programming-language.rst
> Documentation/process/programming-language.rst:39: WARNING: duplicate citation clang, other instance in Documentation/translations/zh_CN/process/programming-language.rst
> Documentation/process/programming-language.rst:40: WARNING: duplicate citation icc, other instance in Documentation/translations/zh_CN/process/programming-language.rst
> Documentation/process/programming-language.rst:41: WARNING: duplicate citation gcc-c-dialect-options, other instance in Documentation/translations/zh_CN/process/programming-language.rst
> Documentation/process/programming-language.rst:42: WARNING: duplicate citation gnu-extensions, other instance in Documentation/translations/zh_CN/process/programming-language.rst
> Documentation/process/programming-language.rst:43: WARNING: duplicate citation gcc-attribute-syntax, other instance in Documentation/translations/zh_CN/process/programming-language.rst
> Documentation/process/programming-language.rst:44: WARNING: duplicate citation n2049, other instance in Documentation/translations/zh_CN/process/programming-language.rst
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> ---
> .../zh_CN/process/management-style.rst | 4 +--
> .../zh_CN/process/programming-language.rst | 28 +++++++++----------
> 2 files changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/Documentation/translations/zh_CN/process/management-style.rst b/Documentation/translations/zh_CN/process/management-style.rst
> index a181fa56d19e..c6a5bb285797 100644
> --- a/Documentation/translations/zh_CN/process/management-style.rst
> +++ b/Documentation/translations/zh_CN/process/management-style.rst
> @@ -28,7 +28,7 @@ Linux内核管理风格
>
> 不管怎样，这里是：
>
> -.. _decisions:
> +.. _cn_decisions:
>
> 1）决策
> -------
> @@ -108,7 +108,7 @@ Linux内核管理风格
> 但是，为了做好作为内核管理者的准备，最好记住不要烧掉任何桥梁，不要轰炸任何
> 无辜的村民，也不要疏远太多的内核开发人员。事实证明，疏远人是相当容易的，而
> 亲近一个疏远的人是很难的。因此，“疏远”立即属于“不可逆”的范畴，并根据
> -:ref:`decisions` 成为绝不可以做的事情。
> +:ref:`cn_decisions` 成为绝不可以做的事情。
It's good to have.
>
> 这里只有几个简单的规则：
>
> diff --git a/Documentation/translations/zh_CN/process/programming-language.rst b/Documentation/translations/zh_CN/process/programming-language.rst
> index 51fd4ef48ea1..9de9a3108c4d 100644
> --- a/Documentation/translations/zh_CN/process/programming-language.rst
> +++ b/Documentation/translations/zh_CN/process/programming-language.rst
> @@ -8,21 +8,21 @@
> 程序设计语言
> ============
>
> -内核是用C语言 [c-language]_ 编写的。更准确地说，内核通常是用 ``gcc`` [gcc]_
> -在 ``-std=gnu89`` [gcc-c-dialect-options]_ 下编译的：ISO C90的 GNU 方言（
> +内核是用C语言 [cn_c-language]_ 编写的。更准确地说，内核通常是用 ``gcc`` [cn_gcc]_
this change isn't good. cn_gcc will show in docs, it looks wired and confusing for peoples. other changes have the same issue. Could you find other way to fix the warning? or I'd rather tolerant it.
Thanks
Alex
^ permalink raw reply
* [PATCH] crypto: doc - improve the skcipher API example code
From: Eric Biggers @ 2019-06-03 5:44 UTC (permalink / raw)
To: linux-crypto; +Cc: linux-doc
From: Eric Biggers <ebiggers@google.com>
Rewrite the skcipher API example, changing it to encrypt a buffer with
AES-256-XTS. This addresses various problems with the previous example:
- It requests a specific driver "cbc-aes-aesni", which is unusual.
Normally users ask for "cbc(aes)", not a specific driver.
- It encrypts only a single AES block. For the reader, that doesn't
clearly distinguish the "skcipher" API from the "cipher" API.
- Showing how to encrypt something with bare CBC is arguably a poor
choice of example, as it doesn't follow modern crypto trends. Now,
usually authenticated encryption is recommended, in which case the
user would use the AEAD API, not skcipher. Disk encryption is still a
legitimate use for skcipher, but for that usually XTS is recommended.
- Many other bugs and poor coding practices, such as not setting
CRYPTO_TFM_REQ_MAY_SLEEP, unnecessarily allocating a heap buffer for
the IV, unnecessary NULL checks, using a pointless wrapper struct, and
forgetting to set an error code in one case.
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
Documentation/crypto/api-samples.rst | 176 ++++++++++++---------------
1 file changed, 77 insertions(+), 99 deletions(-)
diff --git a/Documentation/crypto/api-samples.rst b/Documentation/crypto/api-samples.rst
index f14afaaf2f324..e923f17bc2bd5 100644
--- a/Documentation/crypto/api-samples.rst
+++ b/Documentation/crypto/api-samples.rst
@@ -4,111 +4,89 @@ Code Examples
Code Example For Symmetric Key Cipher Operation
-----------------------------------------------
-::
-
-
- /* tie all data structures together */
- struct skcipher_def {
- struct scatterlist sg;
- struct crypto_skcipher *tfm;
- struct skcipher_request *req;
- struct crypto_wait wait;
- };
-
- /* Perform cipher operation */
- static unsigned int test_skcipher_encdec(struct skcipher_def *sk,
- int enc)
- {
- int rc;
-
- if (enc)
- rc = crypto_wait_req(crypto_skcipher_encrypt(sk->req), &sk->wait);
- else
- rc = crypto_wait_req(crypto_skcipher_decrypt(sk->req), &sk->wait);
-
- if (rc)
- pr_info("skcipher encrypt returned with result %d\n", rc);
+This code encrypts some data with AES-256-XTS. For sake of example,
+all inputs are random bytes, the encryption is done in-place, and it's
+assumed the code is running in a context where it can sleep.
- return rc;
- }
+::
- /* Initialize and trigger cipher operation */
static int test_skcipher(void)
{
- struct skcipher_def sk;
- struct crypto_skcipher *skcipher = NULL;
- struct skcipher_request *req = NULL;
- char *scratchpad = NULL;
- char *ivdata = NULL;
- unsigned char key[32];
- int ret = -EFAULT;
-
- skcipher = crypto_alloc_skcipher("cbc-aes-aesni", 0, 0);
- if (IS_ERR(skcipher)) {
- pr_info("could not allocate skcipher handle\n");
- return PTR_ERR(skcipher);
- }
-
- req = skcipher_request_alloc(skcipher, GFP_KERNEL);
- if (!req) {
- pr_info("could not allocate skcipher request\n");
- ret = -ENOMEM;
- goto out;
- }
-
- skcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
- crypto_req_done,
- &sk.wait);
-
- /* AES 256 with random key */
- get_random_bytes(&key, 32);
- if (crypto_skcipher_setkey(skcipher, key, 32)) {
- pr_info("key could not be set\n");
- ret = -EAGAIN;
- goto out;
- }
-
- /* IV will be random */
- ivdata = kmalloc(16, GFP_KERNEL);
- if (!ivdata) {
- pr_info("could not allocate ivdata\n");
- goto out;
- }
- get_random_bytes(ivdata, 16);
-
- /* Input data will be random */
- scratchpad = kmalloc(16, GFP_KERNEL);
- if (!scratchpad) {
- pr_info("could not allocate scratchpad\n");
- goto out;
- }
- get_random_bytes(scratchpad, 16);
-
- sk.tfm = skcipher;
- sk.req = req;
-
- /* We encrypt one block */
- sg_init_one(&sk.sg, scratchpad, 16);
- skcipher_request_set_crypt(req, &sk.sg, &sk.sg, 16, ivdata);
- crypto_init_wait(&sk.wait);
-
- /* encrypt data */
- ret = test_skcipher_encdec(&sk, 1);
- if (ret)
- goto out;
-
- pr_info("Encryption triggered successfully\n");
-
+ struct crypto_skcipher *tfm = NULL;
+ struct skcipher_request *req = NULL;
+ u8 *data = NULL;
+ const size_t datasize = 512; /* data size in bytes */
+ struct scatterlist sg;
+ DECLARE_CRYPTO_WAIT(wait);
+ u8 iv[16]; /* AES-256-XTS takes a 16-byte IV */
+ u8 key[64]; /* AES-256-XTS takes a 64-byte key */
+ int err;
+
+ /*
+ * Allocate a tfm (a transformation object) and set the key.
+ *
+ * In real-world use, a tfm and key are typically used for many
+ * encryption/decryption operations. But in this example, we'll just do a
+ * single encryption operation with it (which is not very efficient).
+ */
+
+ tfm = crypto_alloc_skcipher("xts(aes)", 0, 0);
+ if (IS_ERR(tfm)) {
+ pr_err("Error allocating xts(aes) handle: %ld\n", PTR_ERR(tfm));
+ return PTR_ERR(tfm);
+ }
+
+ get_random_bytes(key, sizeof(key));
+ err = crypto_skcipher_setkey(tfm, key, sizeof(key));
+ if (err) {
+ pr_err("Error setting key: %d\n", err);
+ goto out;
+ }
+
+ /* Allocate a request object */
+ req = skcipher_request_alloc(tfm, GFP_KERNEL);
+ if (!req) {
+ err = -ENOMEM;
+ goto out;
+ }
+
+ /* Prepare the input data */
+ data = kmalloc(datasize, GFP_KERNEL);
+ if (!data) {
+ err = -ENOMEM;
+ goto out;
+ }
+ get_random_bytes(data, datasize);
+
+ /* Initialize the IV */
+ get_random_bytes(iv, sizeof(iv));
+
+ /*
+ * Encrypt the data in-place.
+ *
+ * For simplicity, in this example we wait for the request to complete
+ * before proceeding, even if the underlying implementation is asynchronous.
+ *
+ * To decrypt instead of encrypt, just change crypto_skcipher_encrypt() to
+ * crypto_skcipher_decrypt().
+ */
+ sg_init_one(&sg, data, datasize);
+ skcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG |
+ CRYPTO_TFM_REQ_MAY_SLEEP,
+ crypto_req_done, &wait);
+ skcipher_request_set_crypt(req, &sg, &sg, datasize, iv);
+ err = crypto_wait_req(crypto_skcipher_encrypt(req), &wait);
+ if (err) {
+ pr_err("Error encrypting data: %d\n", err);
+ goto out;
+ }
+
+ pr_debug("Encryption was successful\n");
out:
- if (skcipher)
- crypto_free_skcipher(skcipher);
- if (req)
+ crypto_free_skcipher(tfm);
skcipher_request_free(req);
- if (ivdata)
- kfree(ivdata);
- if (scratchpad)
- kfree(scratchpad);
- return ret;
+ kfree(data);
+ return err;
}
--
2.21.0
^ permalink raw reply related
* Re: [PATCH] crypto: doc - improve the skcipher API example code
From: Ard Biesheuvel @ 2019-06-03 6:51 UTC (permalink / raw)
To: Eric Biggers
Cc: open list:HARDWARE RANDOM NUMBER GENERATOR CORE,
Linux Doc Mailing List
In-Reply-To: <20190603054408.5903-1-ebiggers@kernel.org>
On Mon, 3 Jun 2019 at 07:44, Eric Biggers <ebiggers@kernel.org> wrote:
>
> From: Eric Biggers <ebiggers@google.com>
>
> Rewrite the skcipher API example, changing it to encrypt a buffer with
> AES-256-XTS. This addresses various problems with the previous example:
>
> - It requests a specific driver "cbc-aes-aesni", which is unusual.
> Normally users ask for "cbc(aes)", not a specific driver.
>
> - It encrypts only a single AES block. For the reader, that doesn't
> clearly distinguish the "skcipher" API from the "cipher" API.
>
> - Showing how to encrypt something with bare CBC is arguably a poor
> choice of example, as it doesn't follow modern crypto trends. Now,
> usually authenticated encryption is recommended, in which case the
> user would use the AEAD API, not skcipher. Disk encryption is still a
> legitimate use for skcipher, but for that usually XTS is recommended.
>
> - Many other bugs and poor coding practices, such as not setting
> CRYPTO_TFM_REQ_MAY_SLEEP, unnecessarily allocating a heap buffer for
> the IV, unnecessary NULL checks, using a pointless wrapper struct, and
> forgetting to set an error code in one case.
>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> Documentation/crypto/api-samples.rst | 176 ++++++++++++---------------
> 1 file changed, 77 insertions(+), 99 deletions(-)
>
> diff --git a/Documentation/crypto/api-samples.rst b/Documentation/crypto/api-samples.rst
> index f14afaaf2f324..e923f17bc2bd5 100644
> --- a/Documentation/crypto/api-samples.rst
> +++ b/Documentation/crypto/api-samples.rst
> @@ -4,111 +4,89 @@ Code Examples
> Code Example For Symmetric Key Cipher Operation
> -----------------------------------------------
>
> -::
> -
> -
> - /* tie all data structures together */
> - struct skcipher_def {
> - struct scatterlist sg;
> - struct crypto_skcipher *tfm;
> - struct skcipher_request *req;
> - struct crypto_wait wait;
> - };
> -
> - /* Perform cipher operation */
> - static unsigned int test_skcipher_encdec(struct skcipher_def *sk,
> - int enc)
> - {
> - int rc;
> -
> - if (enc)
> - rc = crypto_wait_req(crypto_skcipher_encrypt(sk->req), &sk->wait);
> - else
> - rc = crypto_wait_req(crypto_skcipher_decrypt(sk->req), &sk->wait);
> -
> - if (rc)
> - pr_info("skcipher encrypt returned with result %d\n", rc);
> +This code encrypts some data with AES-256-XTS. For sake of example,
> +all inputs are random bytes, the encryption is done in-place, and it's
> +assumed the code is running in a context where it can sleep.
>
> - return rc;
> - }
> +::
>
> - /* Initialize and trigger cipher operation */
> static int test_skcipher(void)
> {
> - struct skcipher_def sk;
> - struct crypto_skcipher *skcipher = NULL;
> - struct skcipher_request *req = NULL;
> - char *scratchpad = NULL;
> - char *ivdata = NULL;
> - unsigned char key[32];
> - int ret = -EFAULT;
> -
> - skcipher = crypto_alloc_skcipher("cbc-aes-aesni", 0, 0);
> - if (IS_ERR(skcipher)) {
> - pr_info("could not allocate skcipher handle\n");
> - return PTR_ERR(skcipher);
> - }
> -
> - req = skcipher_request_alloc(skcipher, GFP_KERNEL);
> - if (!req) {
> - pr_info("could not allocate skcipher request\n");
> - ret = -ENOMEM;
> - goto out;
> - }
> -
> - skcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
> - crypto_req_done,
> - &sk.wait);
> -
> - /* AES 256 with random key */
> - get_random_bytes(&key, 32);
> - if (crypto_skcipher_setkey(skcipher, key, 32)) {
> - pr_info("key could not be set\n");
> - ret = -EAGAIN;
> - goto out;
> - }
> -
> - /* IV will be random */
> - ivdata = kmalloc(16, GFP_KERNEL);
> - if (!ivdata) {
> - pr_info("could not allocate ivdata\n");
> - goto out;
> - }
> - get_random_bytes(ivdata, 16);
> -
> - /* Input data will be random */
> - scratchpad = kmalloc(16, GFP_KERNEL);
> - if (!scratchpad) {
> - pr_info("could not allocate scratchpad\n");
> - goto out;
> - }
> - get_random_bytes(scratchpad, 16);
> -
> - sk.tfm = skcipher;
> - sk.req = req;
> -
> - /* We encrypt one block */
> - sg_init_one(&sk.sg, scratchpad, 16);
> - skcipher_request_set_crypt(req, &sk.sg, &sk.sg, 16, ivdata);
> - crypto_init_wait(&sk.wait);
> -
> - /* encrypt data */
> - ret = test_skcipher_encdec(&sk, 1);
> - if (ret)
> - goto out;
> -
> - pr_info("Encryption triggered successfully\n");
> -
> + struct crypto_skcipher *tfm = NULL;
> + struct skcipher_request *req = NULL;
> + u8 *data = NULL;
> + const size_t datasize = 512; /* data size in bytes */
> + struct scatterlist sg;
> + DECLARE_CRYPTO_WAIT(wait);
> + u8 iv[16]; /* AES-256-XTS takes a 16-byte IV */
> + u8 key[64]; /* AES-256-XTS takes a 64-byte key */
> + int err;
> +
> + /*
> + * Allocate a tfm (a transformation object) and set the key.
> + *
> + * In real-world use, a tfm and key are typically used for many
> + * encryption/decryption operations. But in this example, we'll just do a
> + * single encryption operation with it (which is not very efficient).
> + */
> +
> + tfm = crypto_alloc_skcipher("xts(aes)", 0, 0);
> + if (IS_ERR(tfm)) {
> + pr_err("Error allocating xts(aes) handle: %ld\n", PTR_ERR(tfm));
> + return PTR_ERR(tfm);
> + }
> +
> + get_random_bytes(key, sizeof(key));
> + err = crypto_skcipher_setkey(tfm, key, sizeof(key));
> + if (err) {
> + pr_err("Error setting key: %d\n", err);
> + goto out;
> + }
> +
> + /* Allocate a request object */
> + req = skcipher_request_alloc(tfm, GFP_KERNEL);
> + if (!req) {
> + err = -ENOMEM;
> + goto out;
> + }
> +
> + /* Prepare the input data */
> + data = kmalloc(datasize, GFP_KERNEL);
> + if (!data) {
> + err = -ENOMEM;
> + goto out;
> + }
> + get_random_bytes(data, datasize);
> +
> + /* Initialize the IV */
> + get_random_bytes(iv, sizeof(iv));
> +
> + /*
> + * Encrypt the data in-place.
> + *
> + * For simplicity, in this example we wait for the request to complete
> + * before proceeding, even if the underlying implementation is asynchronous.
> + *
> + * To decrypt instead of encrypt, just change crypto_skcipher_encrypt() to
> + * crypto_skcipher_decrypt().
> + */
> + sg_init_one(&sg, data, datasize);
> + skcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG |
> + CRYPTO_TFM_REQ_MAY_SLEEP,
> + crypto_req_done, &wait);
> + skcipher_request_set_crypt(req, &sg, &sg, datasize, iv);
> + err = crypto_wait_req(crypto_skcipher_encrypt(req), &wait);
> + if (err) {
> + pr_err("Error encrypting data: %d\n", err);
> + goto out;
> + }
> +
> + pr_debug("Encryption was successful\n");
> out:
> - if (skcipher)
> - crypto_free_skcipher(skcipher);
> - if (req)
> + crypto_free_skcipher(tfm);
> skcipher_request_free(req);
> - if (ivdata)
> - kfree(ivdata);
> - if (scratchpad)
> - kfree(scratchpad);
> - return ret;
> + kfree(data);
> + return err;
> }
>
>
> --
> 2.21.0
>
^ permalink raw reply
* Re: [PATCH 09/22] docs: mark orphan documents as such
From: Christophe Leroy @ 2019-06-03 7:32 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Linux Doc Mailing List
Cc: kvm, Radim Krčmář, Maxime Ripard, dri-devel,
platform-driver-x86, Paul Mackerras, linux-stm32,
Alexandre Torgue, Jonathan Corbet, David Airlie, Andrew Donnellan,
linux-pm, Maarten Lankhorst, Matan Ziv-Av, Mauro Carvalho Chehab,
Daniel Vetter, Sean Paul, linux-arm-kernel, linux-kernel,
Maxime Coquelin, Frederic Barrat, Paolo Bonzini, linuxppc-dev,
Georgi Djakov
In-Reply-To: <e0bf4e767dd5de9189e5993fbec2f4b1bafd2064.1559171394.git.mchehab+samsung@kernel.org>
Le 30/05/2019 à 01:23, Mauro Carvalho Chehab a écrit :
> Sphinx doesn't like orphan documents:
>
> Documentation/accelerators/ocxl.rst: WARNING: document isn't included in any toctree
> Documentation/arm/stm32/overview.rst: WARNING: document isn't included in any toctree
> Documentation/arm/stm32/stm32f429-overview.rst: WARNING: document isn't included in any toctree
> Documentation/arm/stm32/stm32f746-overview.rst: WARNING: document isn't included in any toctree
> Documentation/arm/stm32/stm32f769-overview.rst: WARNING: document isn't included in any toctree
> Documentation/arm/stm32/stm32h743-overview.rst: WARNING: document isn't included in any toctree
> Documentation/arm/stm32/stm32mp157-overview.rst: WARNING: document isn't included in any toctree
> Documentation/gpu/msm-crash-dump.rst: WARNING: document isn't included in any toctree
> Documentation/interconnect/interconnect.rst: WARNING: document isn't included in any toctree
> Documentation/laptops/lg-laptop.rst: WARNING: document isn't included in any toctree
> Documentation/powerpc/isa-versions.rst: WARNING: document isn't included in any toctree
> Documentation/virtual/kvm/amd-memory-encryption.rst: WARNING: document isn't included in any toctree
> Documentation/virtual/kvm/vcpu-requests.rst: WARNING: document isn't included in any toctree
>
> So, while they aren't on any toctree, add :orphan: to them, in order
> to silent this warning.
Are those files really not meant to be included in a toctree ?
Shouldn't we include them in the relevant toctree instead of just
shutting up Sphinx warnings ?
Christophe
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> ---
> Documentation/accelerators/ocxl.rst | 2 ++
> Documentation/arm/stm32/overview.rst | 2 ++
> Documentation/arm/stm32/stm32f429-overview.rst | 2 ++
> Documentation/arm/stm32/stm32f746-overview.rst | 2 ++
> Documentation/arm/stm32/stm32f769-overview.rst | 2 ++
> Documentation/arm/stm32/stm32h743-overview.rst | 2 ++
> Documentation/arm/stm32/stm32mp157-overview.rst | 2 ++
> Documentation/gpu/msm-crash-dump.rst | 2 ++
> Documentation/interconnect/interconnect.rst | 2 ++
> Documentation/laptops/lg-laptop.rst | 2 ++
> Documentation/powerpc/isa-versions.rst | 2 ++
> Documentation/virtual/kvm/amd-memory-encryption.rst | 2 ++
> Documentation/virtual/kvm/vcpu-requests.rst | 2 ++
> 13 files changed, 26 insertions(+)
>
> diff --git a/Documentation/accelerators/ocxl.rst b/Documentation/accelerators/ocxl.rst
> index 14cefc020e2d..b1cea19a90f5 100644
> --- a/Documentation/accelerators/ocxl.rst
> +++ b/Documentation/accelerators/ocxl.rst
> @@ -1,3 +1,5 @@
> +:orphan:
> +
> ========================================================
> OpenCAPI (Open Coherent Accelerator Processor Interface)
> ========================================================
> diff --git a/Documentation/arm/stm32/overview.rst b/Documentation/arm/stm32/overview.rst
> index 85cfc8410798..f7e734153860 100644
> --- a/Documentation/arm/stm32/overview.rst
> +++ b/Documentation/arm/stm32/overview.rst
> @@ -1,3 +1,5 @@
> +:orphan:
> +
> ========================
> STM32 ARM Linux Overview
> ========================
> diff --git a/Documentation/arm/stm32/stm32f429-overview.rst b/Documentation/arm/stm32/stm32f429-overview.rst
> index 18feda97f483..65bbb1c3b423 100644
> --- a/Documentation/arm/stm32/stm32f429-overview.rst
> +++ b/Documentation/arm/stm32/stm32f429-overview.rst
> @@ -1,3 +1,5 @@
> +:orphan:
> +
> STM32F429 Overview
> ==================
>
> diff --git a/Documentation/arm/stm32/stm32f746-overview.rst b/Documentation/arm/stm32/stm32f746-overview.rst
> index b5f4b6ce7656..42d593085015 100644
> --- a/Documentation/arm/stm32/stm32f746-overview.rst
> +++ b/Documentation/arm/stm32/stm32f746-overview.rst
> @@ -1,3 +1,5 @@
> +:orphan:
> +
> STM32F746 Overview
> ==================
>
> diff --git a/Documentation/arm/stm32/stm32f769-overview.rst b/Documentation/arm/stm32/stm32f769-overview.rst
> index 228656ced2fe..f6adac862b17 100644
> --- a/Documentation/arm/stm32/stm32f769-overview.rst
> +++ b/Documentation/arm/stm32/stm32f769-overview.rst
> @@ -1,3 +1,5 @@
> +:orphan:
> +
> STM32F769 Overview
> ==================
>
> diff --git a/Documentation/arm/stm32/stm32h743-overview.rst b/Documentation/arm/stm32/stm32h743-overview.rst
> index 3458dc00095d..c525835e7473 100644
> --- a/Documentation/arm/stm32/stm32h743-overview.rst
> +++ b/Documentation/arm/stm32/stm32h743-overview.rst
> @@ -1,3 +1,5 @@
> +:orphan:
> +
> STM32H743 Overview
> ==================
>
> diff --git a/Documentation/arm/stm32/stm32mp157-overview.rst b/Documentation/arm/stm32/stm32mp157-overview.rst
> index 62e176d47ca7..2c52cd020601 100644
> --- a/Documentation/arm/stm32/stm32mp157-overview.rst
> +++ b/Documentation/arm/stm32/stm32mp157-overview.rst
> @@ -1,3 +1,5 @@
> +:orphan:
> +
> STM32MP157 Overview
> ===================
>
> diff --git a/Documentation/gpu/msm-crash-dump.rst b/Documentation/gpu/msm-crash-dump.rst
> index 757cd257e0d8..240ef200f76c 100644
> --- a/Documentation/gpu/msm-crash-dump.rst
> +++ b/Documentation/gpu/msm-crash-dump.rst
> @@ -1,3 +1,5 @@
> +:orphan:
> +
> =====================
> MSM Crash Dump Format
> =====================
> diff --git a/Documentation/interconnect/interconnect.rst b/Documentation/interconnect/interconnect.rst
> index c3e004893796..56e331dab70e 100644
> --- a/Documentation/interconnect/interconnect.rst
> +++ b/Documentation/interconnect/interconnect.rst
> @@ -1,5 +1,7 @@
> .. SPDX-License-Identifier: GPL-2.0
>
> +:orphan:
> +
> =====================================
> GENERIC SYSTEM INTERCONNECT SUBSYSTEM
> =====================================
> diff --git a/Documentation/laptops/lg-laptop.rst b/Documentation/laptops/lg-laptop.rst
> index aa503ee9b3bc..f2c2ffe31101 100644
> --- a/Documentation/laptops/lg-laptop.rst
> +++ b/Documentation/laptops/lg-laptop.rst
> @@ -1,5 +1,7 @@
> .. SPDX-License-Identifier: GPL-2.0+
>
> +:orphan:
> +
> LG Gram laptop extra features
> =============================
>
> diff --git a/Documentation/powerpc/isa-versions.rst b/Documentation/powerpc/isa-versions.rst
> index 812e20cc898c..66c24140ebf1 100644
> --- a/Documentation/powerpc/isa-versions.rst
> +++ b/Documentation/powerpc/isa-versions.rst
> @@ -1,3 +1,5 @@
> +:orphan:
> +
> CPU to ISA Version Mapping
> ==========================
>
> diff --git a/Documentation/virtual/kvm/amd-memory-encryption.rst b/Documentation/virtual/kvm/amd-memory-encryption.rst
> index 659bbc093b52..33d697ab8a58 100644
> --- a/Documentation/virtual/kvm/amd-memory-encryption.rst
> +++ b/Documentation/virtual/kvm/amd-memory-encryption.rst
> @@ -1,3 +1,5 @@
> +:orphan:
> +
> ======================================
> Secure Encrypted Virtualization (SEV)
> ======================================
> diff --git a/Documentation/virtual/kvm/vcpu-requests.rst b/Documentation/virtual/kvm/vcpu-requests.rst
> index 5feb3706a7ae..c1807a1b92e6 100644
> --- a/Documentation/virtual/kvm/vcpu-requests.rst
> +++ b/Documentation/virtual/kvm/vcpu-requests.rst
> @@ -1,3 +1,5 @@
> +:orphan:
> +
> =================
> KVM VCPU Requests
> =================
>
^ permalink raw reply
* Re: [PATCH 2/3] x86: Move CPU feature test out of uaccess region
From: Marco Elver @ 2019-06-03 9:03 UTC (permalink / raw)
To: H. Peter Anvin
Cc: Peter Zijlstra, Andrey Ryabinin, Dmitry Vyukov,
Alexander Potapenko, Andrey Konovalov, Mark Rutland,
Jonathan Corbet, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
the arch/x86 maintainers, Arnd Bergmann, Josh Poimboeuf,
open list:DOCUMENTATION, LKML, linux-arch, kasan-dev
In-Reply-To: <3B49EF08-147F-451C-AA5B-FC4E1B8568EE@zytor.com>
Thanks for the clarification.
I found that static_cpu_has was replaced by static_cpu_has_safe:
https://lkml.org/lkml/2016/1/24/29 -- so is it fair to assume that
both are equally safe at this point?
I have sent a follow-up patch which uses static_cpu_has:
http://lkml.kernel.org/r/20190531150828.157832-3-elver@google.com
Many thanks!
-- Marco
On Sat, 1 Jun 2019 at 03:13, <hpa@zytor.com> wrote:
>
> On May 31, 2019 2:57:36 AM PDT, Marco Elver <elver@google.com> wrote:
> >On Wed, 29 May 2019 at 16:29, <hpa@zytor.com> wrote:
> >>
> >> 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?
> >
> >I couldn't find any mailing list thread referring to why this doesn't
> >use static_cpu_has, do you have any background?
> >
> >static_cpu_has also solves the UACCESS warning.
> >
> >If you confirm it is safe to change to static_cpu_has(), I will change
> >this patch. Note that I should then also change
> >arch/x86/kernel/signal.c to mirror the change for 32bit (although
> >KASAN is not supported for 32bit x86).
> >
> >Thanks,
> >-- Marco
>
> I believe at some point the intent was that boot_cpu_has() was safer and could be used everywhere.
> --
> Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ 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