* Re: [PATCH v8 1/7] iommu: enhance IOMMU default DMA mode build options
From: Leizhen (ThunderTown) @ 2019-05-31 10:03 UTC (permalink / raw)
To: John Garry, Jean-Philippe Brucker, Robin Murphy, Will Deacon,
Joerg Roedel, Jonathan Corbet, linux-doc, Sebastian Ott,
Gerald Schaefer, Martin Schwidefsky, Heiko Carstens,
Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Tony Luck, Fenghua Yu, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H . Peter Anvin, David Woodhouse, iommu,
linux-kernel, linux-s390, linuxppc-dev, x86, linux-ia64
Cc: Hanjun Guo, Linuxarm
In-Reply-To: <645bd526-4eb0-4a36-2dda-023f009247ab@huawei.com>
On 2019/5/30 20:20, John Garry wrote:
> On 30/05/2019 04:48, Zhen Lei wrote:
>> First, add build option IOMMU_DEFAULT_{LAZY|STRICT}, so that we have the
>> opportunity to set {lazy|strict} mode as default at build time. Then put
>> the three config options in an choice, make people can only choose one of
>> the three at a time.
>>
>
> Since this was not picked up, but modulo (somtimes same) comments below:
>
> Reviewed-by: John Garry <john.garry@huawei.com>
>
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>> ---
>> drivers/iommu/Kconfig | 42 +++++++++++++++++++++++++++++++++++-------
>> drivers/iommu/iommu.c | 3 ++-
>> 2 files changed, 37 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
>> index 83664db5221df02..d6a1a45f80ffbf5 100644
>> --- a/drivers/iommu/Kconfig
>> +++ b/drivers/iommu/Kconfig
>> @@ -75,17 +75,45 @@ config IOMMU_DEBUGFS
>> debug/iommu directory, and then populate a subdirectory with
>> entries as required.
>>
>> -config IOMMU_DEFAULT_PASSTHROUGH
>> - bool "IOMMU passthrough by default"
>> +choice
>> + prompt "IOMMU default DMA mode"
>> depends on IOMMU_API
>> - help
>> - Enable passthrough by default, removing the need to pass in
>> - iommu.passthrough=on or iommu=pt through command line. If this
>> - is enabled, you can still disable with iommu.passthrough=off
>> - or iommu=nopt depending on the architecture.
>> + default IOMMU_DEFAULT_STRICT
>> + help
>> + This option allows IOMMU DMA mode to be chose at build time, to
>
> As before:
> /s/chose/chosen/, /s/allows IOMMU/allows an IOMMU/
I'm sorry that the previous version was not modified.
>
>> + override the default DMA mode of each ARCHs, removing the need to
>
> Again, as before:
> ARCHs should be singular
OK
>
>> + pass in kernel parameters through command line. You can still use
>> + ARCHs specific boot options to override this option again.
>> +
>> +config IOMMU_DEFAULT_PASSTHROUGH
>> + bool "passthrough"
>> + help
>> + In this mode, the DMA access through IOMMU without any addresses
>> + translation. That means, the wrong or illegal DMA access can not
>> + be caught, no error information will be reported.
>>
>> If unsure, say N here.
>>
>> +config IOMMU_DEFAULT_LAZY
>> + bool "lazy"
>> + help
>> + Support lazy mode, where for every IOMMU DMA unmap operation, the
>> + flush operation of IOTLB and the free operation of IOVA are deferred.
>> + They are only guaranteed to be done before the related IOVA will be
>> + reused.
>
> why no advisory on how to set if unsure?
Because the LAZY and STRICT have their own advantages and disadvantages.
Should I say: If unsure, keep the default。
>
>> +
>> +config IOMMU_DEFAULT_STRICT
>> + bool "strict"
>> + help
>> + For every IOMMU DMA unmap operation, the flush operation of IOTLB and
>> + the free operation of IOVA are guaranteed to be done in the unmap
>> + function.
>> +
>> + This mode is safer than the two above, but it maybe slower in some
>> + high performace scenarios.
>
> and here?
>
>> +
>> +endchoice
>> +
>> config OF_IOMMU
>> def_bool y
>> depends on OF && IOMMU_API
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 67ee6623f9b2a4d..56bce221285b15f 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -43,7 +43,8 @@
>> #else
>> static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_DMA;
>> #endif
>> -static bool iommu_dma_strict __read_mostly = true;
>> +static bool iommu_dma_strict __read_mostly =
>> + IS_ENABLED(CONFIG_IOMMU_DEFAULT_STRICT);
>>
>> struct iommu_group {
>> struct kobject kobj;
>>
>
>
>
> .
>
^ permalink raw reply
* Re: [PATCH v8 1/7] iommu: enhance IOMMU default DMA mode build options
From: John Garry @ 2019-05-31 10:42 UTC (permalink / raw)
To: Leizhen (ThunderTown), Jean-Philippe Brucker, Robin Murphy,
Will Deacon, Joerg Roedel, Jonathan Corbet, linux-doc,
Sebastian Ott, Gerald Schaefer, Martin Schwidefsky,
Heiko Carstens, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Tony Luck, Fenghua Yu, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H . Peter Anvin, David Woodhouse,
iommu, linux-kernel, linux-s390, linuxppc-dev, x86, linux-ia64
Cc: Hanjun Guo, Linuxarm
In-Reply-To: <030bafab-58f5-8bb1-0533-2977d6e138b2@huawei.com>
>>> -config IOMMU_DEFAULT_PASSTHROUGH
>>> - bool "IOMMU passthrough by default"
>>> +choice
>>> + prompt "IOMMU default DMA mode"
>>> depends on IOMMU_API
>>> - help
>>> - Enable passthrough by default, removing the need to pass in
>>> - iommu.passthrough=on or iommu=pt through command line. If this
>>> - is enabled, you can still disable with iommu.passthrough=off
>>> - or iommu=nopt depending on the architecture.
>>> + default IOMMU_DEFAULT_STRICT
>>> + help
>>> + This option allows IOMMU DMA mode to be chose at build time, to
>>
>> As before:
>> /s/chose/chosen/, /s/allows IOMMU/allows an IOMMU/
> I'm sorry that the previous version was not modified.
>
>>
>>> + override the default DMA mode of each ARCHs, removing the need to
>>
>> Again, as before:
>> ARCHs should be singular
> OK
>
>>
>>> + pass in kernel parameters through command line. You can still use
>>> + ARCHs specific boot options to override this option again.
*
>>> +
>>> +config IOMMU_DEFAULT_PASSTHROUGH
>>> + bool "passthrough"
>>> + help
>>> + In this mode, the DMA access through IOMMU without any addresses
>>> + translation. That means, the wrong or illegal DMA access can not
>>> + be caught, no error information will be reported.
>>>
>>> If unsure, say N here.
>>>
>>> +config IOMMU_DEFAULT_LAZY
>>> + bool "lazy"
>>> + help
>>> + Support lazy mode, where for every IOMMU DMA unmap operation, the
>>> + flush operation of IOTLB and the free operation of IOVA are deferred.
>>> + They are only guaranteed to be done before the related IOVA will be
>>> + reused.
>>
>> why no advisory on how to set if unsure?
> Because the LAZY and STRICT have their own advantages and disadvantages.
>
> Should I say: If unsure, keep the default。
Maybe. So you could put this in the help for the choice, * above, and
remove the advisory on IOMMU_DEFAULT_PASSTHROUGH.
However the maintainer may have a different view.
Thanks,
John
>
>>
>>> +
>>> +config IOMMU_DEFAULT_STRICT
>>> + bool "strict"
>>> + help
>>> + For every IOMMU DMA unmap operation, the flush operation of IOTLB and
>>> + the free operation of IOVA are guaranteed to be done in the unmap
>>> + function.
>>> +
>>> + This mode is safer than the two above, but it maybe slower in some
>>> + high performace scenarios.
>>
>> and here?
^ permalink raw reply
* Re: [PATCH 11/22] gpu: amdgpu: fix broken amdgpu_dma_buf.c references
From: Christian König @ 2019-05-31 14:00 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Linux Doc Mailing List
Cc: Jonathan Corbet, David Airlie, linux-kernel, dri-devel,
Mauro Carvalho Chehab, Maxime Ripard, Sean Paul
In-Reply-To: <f7378a751557277eab6f37f3f5692cf5f1aff8c6.1559171394.git.mchehab+samsung@kernel.org>
Am 30.05.19 um 01:23 schrieb Mauro Carvalho Chehab:
> This file was renamed, but docs weren't updated accordingly.
>
> WARNING: kernel-doc './scripts/kernel-doc -rst -enable-lineno -function PRIME Buffer Sharing ./drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c' failed with return code 1
> WARNING: kernel-doc './scripts/kernel-doc -rst -enable-lineno -internal ./drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c' failed with return code 2
>
> Fixes: 988076cd8c5c ("drm/amdgpu: rename amdgpu_prime.[ch] into amdgpu_dma_buf.[ch]")
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
Reviewed-by: Christian König <christian.koenig@amd.com>
> ---
> Documentation/gpu/amdgpu.rst | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/gpu/amdgpu.rst b/Documentation/gpu/amdgpu.rst
> index a740e491dfcc..a15199b1b02e 100644
> --- a/Documentation/gpu/amdgpu.rst
> +++ b/Documentation/gpu/amdgpu.rst
> @@ -37,10 +37,10 @@ Buffer Objects
> PRIME Buffer Sharing
> --------------------
>
> -.. kernel-doc:: drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> +.. kernel-doc:: drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> :doc: PRIME Buffer Sharing
>
> -.. kernel-doc:: drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> +.. kernel-doc:: drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> :internal:
>
> MMU Notifier
^ permalink raw reply
* [PATCH] docs: clk: fix struct syntax
From: Luca Ceresoli @ 2019-05-31 14:30 UTC (permalink / raw)
To: linux-doc; +Cc: Jonathan Corbet, linux-kernel, Luca Ceresoli
The clk_foo_ops struct example has syntax errors. Fix it so it can be
copy-pasted and used more easily.
Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
---
Documentation/driver-api/clk.rst | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/Documentation/driver-api/clk.rst b/Documentation/driver-api/clk.rst
index 593cca5058b1..3cad45d14187 100644
--- a/Documentation/driver-api/clk.rst
+++ b/Documentation/driver-api/clk.rst
@@ -175,9 +175,9 @@ the following::
To take advantage of your data you'll need to support valid operations
for your clk::
- struct clk_ops clk_foo_ops {
- .enable = &clk_foo_enable;
- .disable = &clk_foo_disable;
+ struct clk_ops clk_foo_ops = {
+ .enable = &clk_foo_enable,
+ .disable = &clk_foo_disable,
};
Implement the above functions using container_of::
--
2.21.0
^ permalink raw reply related
* Re: [PATCH 11/22] gpu: amdgpu: fix broken amdgpu_dma_buf.c references
From: Alex Deucher @ 2019-05-31 15:03 UTC (permalink / raw)
To: Christian Koenig
Cc: Mauro Carvalho Chehab, Linux Doc Mailing List, Jonathan Corbet,
David Airlie, LKML, Maling list - DRI developers,
Mauro Carvalho Chehab, Maxime Ripard, Sean Paul
In-Reply-To: <bf8163be-eb1f-f060-1c5a-405bc6d4c8c5@gmail.com>
On Fri, May 31, 2019 at 10:00 AM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> Am 30.05.19 um 01:23 schrieb Mauro Carvalho Chehab:
> > This file was renamed, but docs weren't updated accordingly.
> >
> > WARNING: kernel-doc './scripts/kernel-doc -rst -enable-lineno -function PRIME Buffer Sharing ./drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c' failed with return code 1
> > WARNING: kernel-doc './scripts/kernel-doc -rst -enable-lineno -internal ./drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c' failed with return code 2
> >
> > Fixes: 988076cd8c5c ("drm/amdgpu: rename amdgpu_prime.[ch] into amdgpu_dma_buf.[ch]")
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
>
> Reviewed-by: Christian König <christian.koenig@amd.com>
>
Applied. thanks!
Alex
> > ---
> > Documentation/gpu/amdgpu.rst | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/gpu/amdgpu.rst b/Documentation/gpu/amdgpu.rst
> > index a740e491dfcc..a15199b1b02e 100644
> > --- a/Documentation/gpu/amdgpu.rst
> > +++ b/Documentation/gpu/amdgpu.rst
> > @@ -37,10 +37,10 @@ Buffer Objects
> > PRIME Buffer Sharing
> > --------------------
> >
> > -.. kernel-doc:: drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> > +.. kernel-doc:: drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> > :doc: PRIME Buffer Sharing
> >
> > -.. kernel-doc:: drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c
> > +.. kernel-doc:: drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> > :internal:
> >
> > MMU Notifier
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* [PATCH v3 2/3] x86: Use static_cpu_has in uaccess region to avoid instrumentation
From: Marco Elver @ 2019-05-31 15:08 UTC (permalink / raw)
To: peterz, aryabinin, dvyukov, glider, andreyknvl, mark.rutland, hpa
Cc: corbet, tglx, mingo, bp, x86, arnd, jpoimboe, linux-doc,
linux-kernel, linux-arch, kasan-dev, Marco Elver
In-Reply-To: <20190531150828.157832-1-elver@google.com>
This patch is a pre-requisite for enabling KASAN bitops instrumentation;
using static_cpu_has instead of boot_cpu_has avoids instrumentation of
test_bit inside the uaccess region. With instrumentation, the KASAN
check would otherwise be flagged by objtool.
For consistency, kernel/signal.c was changed to mirror this change,
however, is never instrumented with KASAN (currently unsupported under
x86 32bit).
Signed-off-by: Marco Elver <elver@google.com>
Suggested-by: H. Peter Anvin <hpa@zytor.com>
---
Changes in v3:
* Use static_cpu_has instead of moving boot_cpu_has outside uaccess
region.
Changes in v2:
* Replaces patch: 'tools/objtool: add kasan_check_* to uaccess
whitelist'
---
arch/x86/ia32/ia32_signal.c | 2 +-
arch/x86/kernel/signal.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
index 629d1ee05599..1cee10091b9f 100644
--- a/arch/x86/ia32/ia32_signal.c
+++ b/arch/x86/ia32/ia32_signal.c
@@ -358,7 +358,7 @@ int ia32_setup_rt_frame(int sig, struct ksignal *ksig,
put_user_ex(ptr_to_compat(&frame->uc), &frame->puc);
/* Create the ucontext. */
- if (boot_cpu_has(X86_FEATURE_XSAVE))
+ if (static_cpu_has(X86_FEATURE_XSAVE))
put_user_ex(UC_FP_XSTATE, &frame->uc.uc_flags);
else
put_user_ex(0, &frame->uc.uc_flags);
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 364813cea647..52eb1d551aed 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -391,7 +391,7 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
put_user_ex(&frame->uc, &frame->puc);
/* Create the ucontext. */
- if (boot_cpu_has(X86_FEATURE_XSAVE))
+ if (static_cpu_has(X86_FEATURE_XSAVE))
put_user_ex(UC_FP_XSTATE, &frame->uc.uc_flags);
else
put_user_ex(0, &frame->uc.uc_flags);
--
2.22.0.rc1.257.g3120a18244-goog
^ permalink raw reply related
* [PATCH v3 0/3] Bitops instrumentation for KASAN
From: Marco Elver @ 2019-05-31 15:08 UTC (permalink / raw)
To: peterz, aryabinin, dvyukov, glider, andreyknvl, mark.rutland, hpa
Cc: corbet, tglx, mingo, bp, x86, arnd, jpoimboe, linux-doc,
linux-kernel, linux-arch, kasan-dev, Marco Elver
Previous version of this patch series and discussion can be found here:
http://lkml.kernel.org/r/20190529141500.193390-1-elver@google.com
Marco Elver (3):
lib/test_kasan: Add bitops tests
x86: Use static_cpu_has in uaccess region to avoid instrumentation
asm-generic, x86: Add bitops instrumentation for KASAN
Documentation/core-api/kernel-api.rst | 2 +-
arch/x86/ia32/ia32_signal.c | 2 +-
arch/x86/include/asm/bitops.h | 189 ++++------------
arch/x86/kernel/signal.c | 2 +-
include/asm-generic/bitops-instrumented.h | 263 ++++++++++++++++++++++
lib/test_kasan.c | 75 +++++-
6 files changed, 376 insertions(+), 157 deletions(-)
create mode 100644 include/asm-generic/bitops-instrumented.h
--
2.22.0.rc1.257.g3120a18244-goog
^ permalink raw reply
* [PATCH v3 1/3] lib/test_kasan: Add bitops tests
From: Marco Elver @ 2019-05-31 15:08 UTC (permalink / raw)
To: peterz, aryabinin, dvyukov, glider, andreyknvl, mark.rutland, hpa
Cc: corbet, tglx, mingo, bp, x86, arnd, jpoimboe, linux-doc,
linux-kernel, linux-arch, kasan-dev, Marco Elver
In-Reply-To: <20190531150828.157832-1-elver@google.com>
This adds bitops tests to the test_kasan module. In a follow-up patch,
support for bitops instrumentation will be added.
Signed-off-by: Marco Elver <elver@google.com>
---
Changes in v3:
* Use kzalloc instead of kmalloc.
* Use sizeof(*bits).
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 related
* [PATCH v3 3/3] asm-generic, x86: Add bitops instrumentation for KASAN
From: Marco Elver @ 2019-05-31 15:08 UTC (permalink / raw)
To: peterz, aryabinin, dvyukov, glider, andreyknvl, mark.rutland, hpa
Cc: corbet, tglx, mingo, bp, x86, arnd, jpoimboe, linux-doc,
linux-kernel, linux-arch, kasan-dev, Marco Elver
In-Reply-To: <20190531150828.157832-1-elver@google.com>
This adds a new header to asm-generic to allow optionally instrumenting
architecture-specific asm implementations of bitops.
This change includes the required change for x86 as reference and
changes the kernel API doc to point to bitops-instrumented.h instead.
Rationale: the functions in x86's bitops.h are no longer the kernel API
functions, but instead the arch_ prefixed functions, which are then
instrumented via bitops-instrumented.h.
Other architectures can similarly add support for asm implementations of
bitops.
The documentation text 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.
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 related
* 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
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