Add explicit KCSAN checks for bitops.
Signed-off-by: Marco Elver <[email protected]>
---
The same patch was previously sent, but at that point the updated bitops
instrumented infrastructure was not yet in mainline:
http://lkml.kernel.org/r/[email protected]
Note that test_bit() is an atomic bitop, and KCSAN treats it as such,
although it is in the non-atomic header. Currently it cannot be moved:
http://lkml.kernel.org/r/[email protected]
---
include/asm-generic/bitops/instrumented-atomic.h | 7 +++++++
include/asm-generic/bitops/instrumented-lock.h | 5 +++++
include/asm-generic/bitops/instrumented-non-atomic.h | 8 ++++++++
3 files changed, 20 insertions(+)
diff --git a/include/asm-generic/bitops/instrumented-atomic.h b/include/asm-generic/bitops/instrumented-atomic.h
index 18ce3c9e8eec..eb3abf7e5c08 100644
--- a/include/asm-generic/bitops/instrumented-atomic.h
+++ b/include/asm-generic/bitops/instrumented-atomic.h
@@ -12,6 +12,7 @@
#define _ASM_GENERIC_BITOPS_INSTRUMENTED_ATOMIC_H
#include <linux/kasan-checks.h>
+#include <linux/kcsan-checks.h>
/**
* set_bit - Atomically set a bit in memory
@@ -26,6 +27,7 @@
static inline void set_bit(long nr, volatile unsigned long *addr)
{
kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ kcsan_check_atomic_write(addr + BIT_WORD(nr), sizeof(long));
arch_set_bit(nr, addr);
}
@@ -39,6 +41,7 @@ static inline void set_bit(long nr, volatile unsigned long *addr)
static inline void clear_bit(long nr, volatile unsigned long *addr)
{
kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ kcsan_check_atomic_write(addr + BIT_WORD(nr), sizeof(long));
arch_clear_bit(nr, addr);
}
@@ -55,6 +58,7 @@ static inline void clear_bit(long nr, volatile unsigned long *addr)
static inline void change_bit(long nr, volatile unsigned long *addr)
{
kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ kcsan_check_atomic_write(addr + BIT_WORD(nr), sizeof(long));
arch_change_bit(nr, addr);
}
@@ -68,6 +72,7 @@ static inline void change_bit(long nr, volatile unsigned long *addr)
static inline bool test_and_set_bit(long nr, volatile unsigned long *addr)
{
kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ kcsan_check_atomic_write(addr + BIT_WORD(nr), sizeof(long));
return arch_test_and_set_bit(nr, addr);
}
@@ -81,6 +86,7 @@ static inline bool test_and_set_bit(long nr, volatile unsigned long *addr)
static inline bool test_and_clear_bit(long nr, volatile unsigned long *addr)
{
kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ kcsan_check_atomic_write(addr + BIT_WORD(nr), sizeof(long));
return arch_test_and_clear_bit(nr, addr);
}
@@ -94,6 +100,7 @@ static inline bool test_and_clear_bit(long nr, volatile unsigned long *addr)
static inline bool test_and_change_bit(long nr, volatile unsigned long *addr)
{
kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ kcsan_check_atomic_write(addr + BIT_WORD(nr), sizeof(long));
return arch_test_and_change_bit(nr, addr);
}
diff --git a/include/asm-generic/bitops/instrumented-lock.h b/include/asm-generic/bitops/instrumented-lock.h
index ec53fdeea9ec..2c80dca31e27 100644
--- a/include/asm-generic/bitops/instrumented-lock.h
+++ b/include/asm-generic/bitops/instrumented-lock.h
@@ -12,6 +12,7 @@
#define _ASM_GENERIC_BITOPS_INSTRUMENTED_LOCK_H
#include <linux/kasan-checks.h>
+#include <linux/kcsan-checks.h>
/**
* clear_bit_unlock - Clear a bit in memory, for unlock
@@ -23,6 +24,7 @@
static inline void clear_bit_unlock(long nr, volatile unsigned long *addr)
{
kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ kcsan_check_atomic_write(addr + BIT_WORD(nr), sizeof(long));
arch_clear_bit_unlock(nr, addr);
}
@@ -38,6 +40,7 @@ static inline void clear_bit_unlock(long nr, volatile unsigned long *addr)
static inline void __clear_bit_unlock(long nr, volatile unsigned long *addr)
{
kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ kcsan_check_write(addr + BIT_WORD(nr), sizeof(long));
arch___clear_bit_unlock(nr, addr);
}
@@ -53,6 +56,7 @@ static inline void __clear_bit_unlock(long nr, volatile unsigned long *addr)
static inline bool test_and_set_bit_lock(long nr, volatile unsigned long *addr)
{
kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ kcsan_check_atomic_write(addr + BIT_WORD(nr), sizeof(long));
return arch_test_and_set_bit_lock(nr, addr);
}
@@ -72,6 +76,7 @@ static inline bool
clear_bit_unlock_is_negative_byte(long nr, volatile unsigned long *addr)
{
kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ kcsan_check_atomic_write(addr + BIT_WORD(nr), sizeof(long));
return arch_clear_bit_unlock_is_negative_byte(nr, addr);
}
/* Let everybody know we have it. */
diff --git a/include/asm-generic/bitops/instrumented-non-atomic.h b/include/asm-generic/bitops/instrumented-non-atomic.h
index 95ff28d128a1..8479af8b3309 100644
--- a/include/asm-generic/bitops/instrumented-non-atomic.h
+++ b/include/asm-generic/bitops/instrumented-non-atomic.h
@@ -12,6 +12,7 @@
#define _ASM_GENERIC_BITOPS_INSTRUMENTED_NON_ATOMIC_H
#include <linux/kasan-checks.h>
+#include <linux/kcsan-checks.h>
/**
* __set_bit - Set a bit in memory
@@ -25,6 +26,7 @@
static inline void __set_bit(long nr, volatile unsigned long *addr)
{
kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ kcsan_check_write(addr + BIT_WORD(nr), sizeof(long));
arch___set_bit(nr, addr);
}
@@ -40,6 +42,7 @@ static inline void __set_bit(long nr, volatile unsigned long *addr)
static inline void __clear_bit(long nr, volatile unsigned long *addr)
{
kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ kcsan_check_write(addr + BIT_WORD(nr), sizeof(long));
arch___clear_bit(nr, addr);
}
@@ -55,6 +58,7 @@ static inline void __clear_bit(long nr, volatile unsigned long *addr)
static inline void __change_bit(long nr, volatile unsigned long *addr)
{
kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ kcsan_check_write(addr + BIT_WORD(nr), sizeof(long));
arch___change_bit(nr, addr);
}
@@ -69,6 +73,7 @@ static inline void __change_bit(long nr, volatile unsigned long *addr)
static inline bool __test_and_set_bit(long nr, volatile unsigned long *addr)
{
kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ kcsan_check_write(addr + BIT_WORD(nr), sizeof(long));
return arch___test_and_set_bit(nr, addr);
}
@@ -83,6 +88,7 @@ static inline bool __test_and_set_bit(long nr, volatile unsigned long *addr)
static inline bool __test_and_clear_bit(long nr, volatile unsigned long *addr)
{
kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ kcsan_check_write(addr + BIT_WORD(nr), sizeof(long));
return arch___test_and_clear_bit(nr, addr);
}
@@ -97,6 +103,7 @@ static inline bool __test_and_clear_bit(long nr, volatile unsigned long *addr)
static inline bool __test_and_change_bit(long nr, volatile unsigned long *addr)
{
kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ kcsan_check_write(addr + BIT_WORD(nr), sizeof(long));
return arch___test_and_change_bit(nr, addr);
}
@@ -108,6 +115,7 @@ static inline bool __test_and_change_bit(long nr, volatile unsigned long *addr)
static inline bool test_bit(long nr, const volatile unsigned long *addr)
{
kasan_check_read(addr + BIT_WORD(nr), sizeof(long));
+ kcsan_check_atomic_read(addr + BIT_WORD(nr), sizeof(long));
return arch_test_bit(nr, addr);
}
--
2.25.0.rc1.283.g88dfdc4193-goog
On Wed, Jan 15, 2020 at 5:58 PM Marco Elver <[email protected]> wrote:
> * set_bit - Atomically set a bit in memory
> @@ -26,6 +27,7 @@
> static inline void set_bit(long nr, volatile unsigned long *addr)
> {
> kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> + kcsan_check_atomic_write(addr + BIT_WORD(nr), sizeof(long));
> arch_set_bit(nr, addr);
> }
It looks like you add a kcsan_check_atomic_write or kcsan_check_write directly
next to almost any instance of kasan_check_write().
Are there any cases where we actually just need one of the two but not the
other? If not, maybe it's better to rename the macro and have it do both things
as needed?
Arnd
On Wed, Jan 15, 2020 at 8:51 PM Marco Elver <[email protected]> wrote:
>
> On Wed, 15 Jan 2020 at 20:27, Arnd Bergmann <[email protected]> wrote:
> >
> > On Wed, Jan 15, 2020 at 5:58 PM Marco Elver <[email protected]> wrote:
> > > * set_bit - Atomically set a bit in memory
> > > @@ -26,6 +27,7 @@
> > > static inline void set_bit(long nr, volatile unsigned long *addr)
> > > {
> > > kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> > > + kcsan_check_atomic_write(addr + BIT_WORD(nr), sizeof(long));
> > > arch_set_bit(nr, addr);
> > > }
> >
> > It looks like you add a kcsan_check_atomic_write or kcsan_check_write directly
> > next to almost any instance of kasan_check_write().
> >
> > Are there any cases where we actually just need one of the two but not the
> > other? If not, maybe it's better to rename the macro and have it do both things
> > as needed?
>
> Do you mean adding an inline helper at the top of each bitops header
> here, similar to what we did for atomic-instrumented? Happy to do
> that if it improves readability.
I was thinking of treewide wrappers, given that there are only a couple of files
calling kasan_check_write():
$ git grep -wl kasan_check_write
arch/arm64/include/asm/barrier.h
arch/arm64/include/asm/uaccess.h
arch/x86/include/asm/uaccess_64.h
include/asm-generic/atomic-instrumented.h
include/asm-generic/bitops/instrumented-atomic.h
include/asm-generic/bitops/instrumented-lock.h
include/asm-generic/bitops/instrumented-non-atomic.h
include/linux/kasan-checks.h
include/linux/uaccess.h
lib/iov_iter.c
lib/strncpy_from_user.c
lib/usercopy.c
scripts/atomic/gen-atomic-instrumented.sh
Are there any that really just want kasan_check_write() but not one
of the kcsan checks?
Arnd
On Wed, 15 Jan 2020 at 20:27, Arnd Bergmann <[email protected]> wrote:
>
> On Wed, Jan 15, 2020 at 5:58 PM Marco Elver <[email protected]> wrote:
> > * set_bit - Atomically set a bit in memory
> > @@ -26,6 +27,7 @@
> > static inline void set_bit(long nr, volatile unsigned long *addr)
> > {
> > kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> > + kcsan_check_atomic_write(addr + BIT_WORD(nr), sizeof(long));
> > arch_set_bit(nr, addr);
> > }
>
> It looks like you add a kcsan_check_atomic_write or kcsan_check_write directly
> next to almost any instance of kasan_check_write().
>
> Are there any cases where we actually just need one of the two but not the
> other? If not, maybe it's better to rename the macro and have it do both things
> as needed?
Do you mean adding an inline helper at the top of each bitops header
here, similar to what we did for atomic-instrumented? Happy to do
that if it improves readability.
Thanks,
-- Marco
On Wed, 15 Jan 2020 at 20:55, Arnd Bergmann <[email protected]> wrote:
>
> On Wed, Jan 15, 2020 at 8:51 PM Marco Elver <[email protected]> wrote:
> >
> > On Wed, 15 Jan 2020 at 20:27, Arnd Bergmann <[email protected]> wrote:
> > >
> > > On Wed, Jan 15, 2020 at 5:58 PM Marco Elver <[email protected]> wrote:
> > > > * set_bit - Atomically set a bit in memory
> > > > @@ -26,6 +27,7 @@
> > > > static inline void set_bit(long nr, volatile unsigned long *addr)
> > > > {
> > > > kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
> > > > + kcsan_check_atomic_write(addr + BIT_WORD(nr), sizeof(long));
> > > > arch_set_bit(nr, addr);
> > > > }
> > >
> > > It looks like you add a kcsan_check_atomic_write or kcsan_check_write directly
> > > next to almost any instance of kasan_check_write().
> > >
> > > Are there any cases where we actually just need one of the two but not the
> > > other? If not, maybe it's better to rename the macro and have it do both things
> > > as needed?
> >
> > Do you mean adding an inline helper at the top of each bitops header
> > here, similar to what we did for atomic-instrumented? Happy to do
> > that if it improves readability.
>
> I was thinking of treewide wrappers, given that there are only a couple of files
> calling kasan_check_write():
>
> $ git grep -wl kasan_check_write
> arch/arm64/include/asm/barrier.h
> arch/arm64/include/asm/uaccess.h
> arch/x86/include/asm/uaccess_64.h
> include/asm-generic/atomic-instrumented.h
> include/asm-generic/bitops/instrumented-atomic.h
> include/asm-generic/bitops/instrumented-lock.h
> include/asm-generic/bitops/instrumented-non-atomic.h
> include/linux/kasan-checks.h
> include/linux/uaccess.h
> lib/iov_iter.c
> lib/strncpy_from_user.c
> lib/usercopy.c
> scripts/atomic/gen-atomic-instrumented.sh
>
> Are there any that really just want kasan_check_write() but not one
> of the kcsan checks?
If I understood correctly, this suggestion would amount to introducing
a new header, e.g. 'ksan-checks.h', that provides unified generic
checks. For completeness, we will also need to consider reads. Since
KCSAN provides 4 check variants ({read,write} x {plain,atomic}), we
will need 4 generic check variants.
I certainly do not feel comfortable blindly introducing kcsan_checks
in all places where we have kasan_checks, but it may be worthwhile
adding this infrastructure and starting with atomic-instrumented and
bitops-instrumented wrappers. The other locations you list above would
need to be evaluated on a case-by-case basis to check if we want to
report data races for those accesses.
As a minor data point, {READ,WRITE}_ONCE in compiler.h currently only
has kcsan_checks and not kasan_checks.
My personal preference would be to keep the various checks explicit,
clearly opting into either KCSAN and/or KASAN. Since I do not think
it's obvious if we want both for the existing and potentially new
locations (in future), the potential for error by blindly using a
generic 'ksan_check' appears worse than potentially adding a dozen
lines or so.
Let me know if you'd like to proceed with 'ksan-checks.h'.
Thanks,
-- Marco
On Wed, Jan 15, 2020 at 9:50 PM Marco Elver <[email protected]> wrote:
> On Wed, 15 Jan 2020 at 20:55, Arnd Bergmann <[email protected]> wrote:
> > On Wed, Jan 15, 2020 at 8:51 PM Marco Elver <[email protected]> wrote:
> > > On Wed, 15 Jan 2020 at 20:27, Arnd Bergmann <[email protected]> wrote:
> > Are there any that really just want kasan_check_write() but not one
> > of the kcsan checks?
>
> If I understood correctly, this suggestion would amount to introducing
> a new header, e.g. 'ksan-checks.h', that provides unified generic
> checks. For completeness, we will also need to consider reads. Since
> KCSAN provides 4 check variants ({read,write} x {plain,atomic}), we
> will need 4 generic check variants.
Yes, that was the idea.
> I certainly do not feel comfortable blindly introducing kcsan_checks
> in all places where we have kasan_checks, but it may be worthwhile
> adding this infrastructure and starting with atomic-instrumented and
> bitops-instrumented wrappers. The other locations you list above would
> need to be evaluated on a case-by-case basis to check if we want to
> report data races for those accesses.
I think the main question to answer is whether it is more likely to go
wrong because we are missing checks when one caller accidentally
only has one but not the other, or whether they go wrong because
we accidentally check both when we should only be checking one.
My guess would be that the first one is more likely to happen, but
the second one is more likely to cause problems when it happens.
> As a minor data point, {READ,WRITE}_ONCE in compiler.h currently only
> has kcsan_checks and not kasan_checks.
Right. This is because we want an explicit "atomic" check for kcsan
but we want to have the function inlined for kasan, right?
> My personal preference would be to keep the various checks explicit,
> clearly opting into either KCSAN and/or KASAN. Since I do not think
> it's obvious if we want both for the existing and potentially new
> locations (in future), the potential for error by blindly using a
> generic 'ksan_check' appears worse than potentially adding a dozen
> lines or so.
>
> Let me know if you'd like to proceed with 'ksan-checks.h'.
Could you have a look at the files I listed and see if there are any
other examples that probably a different set of checks between the
two, besides the READ_ONCE() example?
If you can't find any, I would prefer having the simpler interface
with just one set of annotations.
Arnd
On Fri, 17 Jan 2020 at 13:25, Arnd Bergmann <[email protected]> wrote:
>
> On Wed, Jan 15, 2020 at 9:50 PM Marco Elver <[email protected]> wrote:
> > On Wed, 15 Jan 2020 at 20:55, Arnd Bergmann <[email protected]> wrote:
> > > On Wed, Jan 15, 2020 at 8:51 PM Marco Elver <[email protected]> wrote:
> > > > On Wed, 15 Jan 2020 at 20:27, Arnd Bergmann <[email protected]> wrote:
> > > Are there any that really just want kasan_check_write() but not one
> > > of the kcsan checks?
> >
> > If I understood correctly, this suggestion would amount to introducing
> > a new header, e.g. 'ksan-checks.h', that provides unified generic
> > checks. For completeness, we will also need to consider reads. Since
> > KCSAN provides 4 check variants ({read,write} x {plain,atomic}), we
> > will need 4 generic check variants.
>
> Yes, that was the idea.
>
> > I certainly do not feel comfortable blindly introducing kcsan_checks
> > in all places where we have kasan_checks, but it may be worthwhile
> > adding this infrastructure and starting with atomic-instrumented and
> > bitops-instrumented wrappers. The other locations you list above would
> > need to be evaluated on a case-by-case basis to check if we want to
> > report data races for those accesses.
>
> I think the main question to answer is whether it is more likely to go
> wrong because we are missing checks when one caller accidentally
> only has one but not the other, or whether they go wrong because
> we accidentally check both when we should only be checking one.
>
> My guess would be that the first one is more likely to happen, but
> the second one is more likely to cause problems when it happens.
Right, I guess both have trade-offs.
> > As a minor data point, {READ,WRITE}_ONCE in compiler.h currently only
> > has kcsan_checks and not kasan_checks.
>
> Right. This is because we want an explicit "atomic" check for kcsan
> but we want to have the function inlined for kasan, right?
Yes, correct.
> > My personal preference would be to keep the various checks explicit,
> > clearly opting into either KCSAN and/or KASAN. Since I do not think
> > it's obvious if we want both for the existing and potentially new
> > locations (in future), the potential for error by blindly using a
> > generic 'ksan_check' appears worse than potentially adding a dozen
> > lines or so.
> >
> > Let me know if you'd like to proceed with 'ksan-checks.h'.
>
> Could you have a look at the files I listed and see if there are any
> other examples that probably a different set of checks between the
> two, besides the READ_ONCE() example?
All the user-copy related code should probably have kcsan_checks as well.
> If you can't find any, I would prefer having the simpler interface
> with just one set of annotations.
That's fair enough. I'll prepare a v2 series that first introduces the
new header, and then applies it to the locations that seem obvious
candidates for having both checks.
Thanks,
-- Marco
On Fri, 17 Jan 2020 at 14:14, Marco Elver <[email protected]> wrote:
>
> On Fri, 17 Jan 2020 at 13:25, Arnd Bergmann <[email protected]> wrote:
> >
> > On Wed, Jan 15, 2020 at 9:50 PM Marco Elver <[email protected]> wrote:
> > > On Wed, 15 Jan 2020 at 20:55, Arnd Bergmann <[email protected]> wrote:
> > > > On Wed, Jan 15, 2020 at 8:51 PM Marco Elver <[email protected]> wrote:
> > > > > On Wed, 15 Jan 2020 at 20:27, Arnd Bergmann <[email protected]> wrote:
> > > > Are there any that really just want kasan_check_write() but not one
> > > > of the kcsan checks?
> > >
> > > If I understood correctly, this suggestion would amount to introducing
> > > a new header, e.g. 'ksan-checks.h', that provides unified generic
> > > checks. For completeness, we will also need to consider reads. Since
> > > KCSAN provides 4 check variants ({read,write} x {plain,atomic}), we
> > > will need 4 generic check variants.
> >
> > Yes, that was the idea.
> >
> > > I certainly do not feel comfortable blindly introducing kcsan_checks
> > > in all places where we have kasan_checks, but it may be worthwhile
> > > adding this infrastructure and starting with atomic-instrumented and
> > > bitops-instrumented wrappers. The other locations you list above would
> > > need to be evaluated on a case-by-case basis to check if we want to
> > > report data races for those accesses.
> >
> > I think the main question to answer is whether it is more likely to go
> > wrong because we are missing checks when one caller accidentally
> > only has one but not the other, or whether they go wrong because
> > we accidentally check both when we should only be checking one.
> >
> > My guess would be that the first one is more likely to happen, but
> > the second one is more likely to cause problems when it happens.
>
> Right, I guess both have trade-offs.
>
> > > As a minor data point, {READ,WRITE}_ONCE in compiler.h currently only
> > > has kcsan_checks and not kasan_checks.
> >
> > Right. This is because we want an explicit "atomic" check for kcsan
> > but we want to have the function inlined for kasan, right?
>
> Yes, correct.
>
> > > My personal preference would be to keep the various checks explicit,
> > > clearly opting into either KCSAN and/or KASAN. Since I do not think
> > > it's obvious if we want both for the existing and potentially new
> > > locations (in future), the potential for error by blindly using a
> > > generic 'ksan_check' appears worse than potentially adding a dozen
> > > lines or so.
> > >
> > > Let me know if you'd like to proceed with 'ksan-checks.h'.
> >
> > Could you have a look at the files I listed and see if there are any
> > other examples that probably a different set of checks between the
> > two, besides the READ_ONCE() example?
>
> All the user-copy related code should probably have kcsan_checks as well.
>
> > If you can't find any, I would prefer having the simpler interface
> > with just one set of annotations.
>
> That's fair enough. I'll prepare a v2 series that first introduces the
> new header, and then applies it to the locations that seem obvious
> candidates for having both checks.
I've sent a new patch series which introduces instrumented.h:
http://lkml.kernel.org/r/[email protected]
Thanks,
-- Marco
On Mon, Jan 20, 2020 at 3:23 PM Marco Elver <[email protected]> wrote:
> On Fri, 17 Jan 2020 at 14:14, Marco Elver <[email protected]> wrote:
> > On Fri, 17 Jan 2020 at 13:25, Arnd Bergmann <[email protected]> wrote:
> > > On Wed, Jan 15, 2020 at 9:50 PM Marco Elver <[email protected]> wrote:
> > > If you can't find any, I would prefer having the simpler interface
> > > with just one set of annotations.
> >
> > That's fair enough. I'll prepare a v2 series that first introduces the
> > new header, and then applies it to the locations that seem obvious
> > candidates for having both checks.
>
> I've sent a new patch series which introduces instrumented.h:
> http://lkml.kernel.org/r/[email protected]
Looks good to me, feel free to add
Acked-by: Arnd Bergmann <[email protected]>
if you are merging this through your own tree or someone else's,
or let me know if I should put it into the asm-generic git tree.
Arnd
On Mon, 20 Jan 2020 at 15:40, Arnd Bergmann <[email protected]> wrote:
>
> On Mon, Jan 20, 2020 at 3:23 PM Marco Elver <[email protected]> wrote:
> > On Fri, 17 Jan 2020 at 14:14, Marco Elver <[email protected]> wrote:
> > > On Fri, 17 Jan 2020 at 13:25, Arnd Bergmann <[email protected]> wrote:
> > > > On Wed, Jan 15, 2020 at 9:50 PM Marco Elver <[email protected]> wrote:
>
> > > > If you can't find any, I would prefer having the simpler interface
> > > > with just one set of annotations.
> > >
> > > That's fair enough. I'll prepare a v2 series that first introduces the
> > > new header, and then applies it to the locations that seem obvious
> > > candidates for having both checks.
> >
> > I've sent a new patch series which introduces instrumented.h:
> > http://lkml.kernel.org/r/[email protected]
>
> Looks good to me, feel free to add
>
> Acked-by: Arnd Bergmann <[email protected]>
>
> if you are merging this through your own tree or someone else's,
> or let me know if I should put it into the asm-generic git tree.
Thank you! It seems there is still some debate around the user-copy
instrumentation.
The main question we have right now is if we should add pre/post hooks
for them. Although in the version above I added KCSAN checks after the
user-copies, it seems maybe we want it before. I personally don't have
a strong preference, and wanted to err on the side of being more
conservative.
If I send a v2, and it now turns out we do all the instrumentation
before the user-copies for KASAN and KCSAN, then we have a bunch of
empty hooks. However, for KMSAN we need the post-hook, at least for
copy_from_user. Do you mind a bunch of empty functions to provide
pre/post hooks for user-copies? Could the post-hooks be generally
useful for something else?
Thanks,
-- Marco
On Mon, Jan 20, 2020 at 4:11 PM Marco Elver <[email protected]> wrote:
> On Mon, 20 Jan 2020 at 15:40, Arnd Bergmann <[email protected]> wrote:
> > On Mon, Jan 20, 2020 at 3:23 PM Marco Elver <[email protected]> wrote:
> > > On Fri, 17 Jan 2020 at 14:14, Marco Elver <[email protected]> wrote:
> > > > On Fri, 17 Jan 2020 at 13:25, Arnd Bergmann <[email protected]> wrote:
> > > > > On Wed, Jan 15, 2020 at 9:50 PM Marco Elver <[email protected]> wrote:
> >
> > > > > If you can't find any, I would prefer having the simpler interface
> > > > > with just one set of annotations.
> > > >
> > > > That's fair enough. I'll prepare a v2 series that first introduces the
> > > > new header, and then applies it to the locations that seem obvious
> > > > candidates for having both checks.
> > >
> > > I've sent a new patch series which introduces instrumented.h:
> > > http://lkml.kernel.org/r/[email protected]
> >
> > Looks good to me, feel free to add
> >
> > Acked-by: Arnd Bergmann <[email protected]>
> >
> > if you are merging this through your own tree or someone else's,
> > or let me know if I should put it into the asm-generic git tree.
>
> Thank you! It seems there is still some debate around the user-copy
> instrumentation.
>
> The main question we have right now is if we should add pre/post hooks
> for them. Although in the version above I added KCSAN checks after the
> user-copies, it seems maybe we want it before. I personally don't have
> a strong preference, and wanted to err on the side of being more
> conservative.
>
> If I send a v2, and it now turns out we do all the instrumentation
> before the user-copies for KASAN and KCSAN, then we have a bunch of
> empty hooks. However, for KMSAN we need the post-hook, at least for
> copy_from_user. Do you mind a bunch of empty functions to provide
> pre/post hooks for user-copies? Could the post-hooks be generally
> useful for something else?
I'd prefer not to add any empty hooks, let's do that once they
are actually used.
Arnd
On Mon, 20 Jan 2020 at 20:03, Arnd Bergmann <[email protected]> wrote:
>
> On Mon, Jan 20, 2020 at 4:11 PM Marco Elver <[email protected]> wrote:
> > On Mon, 20 Jan 2020 at 15:40, Arnd Bergmann <[email protected]> wrote:
> > > On Mon, Jan 20, 2020 at 3:23 PM Marco Elver <[email protected]> wrote:
> > > > On Fri, 17 Jan 2020 at 14:14, Marco Elver <[email protected]> wrote:
> > > > > On Fri, 17 Jan 2020 at 13:25, Arnd Bergmann <[email protected]> wrote:
> > > > > > On Wed, Jan 15, 2020 at 9:50 PM Marco Elver <[email protected]> wrote:
> > >
> > > > > > If you can't find any, I would prefer having the simpler interface
> > > > > > with just one set of annotations.
> > > > >
> > > > > That's fair enough. I'll prepare a v2 series that first introduces the
> > > > > new header, and then applies it to the locations that seem obvious
> > > > > candidates for having both checks.
> > > >
> > > > I've sent a new patch series which introduces instrumented.h:
> > > > http://lkml.kernel.org/r/[email protected]
> > >
> > > Looks good to me, feel free to add
> > >
> > > Acked-by: Arnd Bergmann <[email protected]>
> > >
> > > if you are merging this through your own tree or someone else's,
> > > or let me know if I should put it into the asm-generic git tree.
> >
> > Thank you! It seems there is still some debate around the user-copy
> > instrumentation.
> >
> > The main question we have right now is if we should add pre/post hooks
> > for them. Although in the version above I added KCSAN checks after the
> > user-copies, it seems maybe we want it before. I personally don't have
> > a strong preference, and wanted to err on the side of being more
> > conservative.
> >
> > If I send a v2, and it now turns out we do all the instrumentation
> > before the user-copies for KASAN and KCSAN, then we have a bunch of
> > empty hooks. However, for KMSAN we need the post-hook, at least for
> > copy_from_user. Do you mind a bunch of empty functions to provide
> > pre/post hooks for user-copies? Could the post-hooks be generally
> > useful for something else?
>
> I'd prefer not to add any empty hooks, let's do that once they
> are actually used.
I hope I found a solution to the various constraints:
http://lkml.kernel.org/r/[email protected]
I removed your Acks from the patches that were changed in v2. Please
have another look.
Re tree: Once people are happy with the patches, since this depends on
KCSAN it'll probably have to go through Paul's -rcu tree, since KCSAN
is not yet in mainline (currently only in -rcu, -tip, and -next).
Thanks,
-- Marco