[PATCH] arm64: kcsan: Fix kcsan test_barrier fail and panic

[PATCH] arm64: kcsan: Fix kcsan test_barrier fail and panic

[Kefeng Wang]

As "kcsan: Support detecting a subset of missing memory barriers"
introduced KCSAN_STRICT which make kcsan detects more missing memory
barrier, but arm64 don't have KCSAN instrumentation for barriers, so
the new selftest test_barrier() will fail, then panic.

Let's prefix all barriers with __ on arm64, as asm-generic/barriers.h
defined the final instrumented version of these barriers, which will
fix the above issues.

Fixes: dd03762ab608 ("arm64: Enable KCSAN")
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 arch/arm64/include/asm/barrier.h | 12 ++++++------
 include/asm-generic/barrier.h    |  4 ++++
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/include/asm/barrier.h b/arch/arm64/include/asm/barrier.h
index 62217be36217..9760a8d4ed0a 100644
--- a/arch/arm64/include/asm/barrier.h
+++ b/arch/arm64/include/asm/barrier.h
@@ -46,13 +46,13 @@
 #define pmr_sync()	do {} while (0)
 #endif
 
-#define mb()		dsb(sy)
-#define rmb()		dsb(ld)
-#define wmb()		dsb(st)
+#define __mb()		dsb(sy)
+#define __rmb()		dsb(ld)
+#define __wmb()		dsb(st)
 
-#define dma_mb()	dmb(osh)
-#define dma_rmb()	dmb(oshld)
-#define dma_wmb()	dmb(oshst)
+#define __dma_mb()	dmb(osh)
+#define __dma_rmb()	dmb(oshld)
+#define __dma_wmb()	dmb(oshst)
 
 #define io_stop_wc()	dgh()
 
diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h
index fd7e8fbaeef1..18863c50e9ce 100644
--- a/include/asm-generic/barrier.h
+++ b/include/asm-generic/barrier.h
@@ -38,6 +38,10 @@
 #define wmb()	do { kcsan_wmb(); __wmb(); } while (0)
 #endif
 
+#ifdef __dma_mb
+#define dma_mb()	do { kcsan_mb(); __dma_mb(); } while (0)
+#endif
+
 #ifdef __dma_rmb
 #define dma_rmb()	do { kcsan_rmb(); __dma_rmb(); } while (0)
 #endif
-- 
2.27.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: kcsan: Fix kcsan test_barrier fail and panic

[Marco Elver]

On Tue, Apr 26, 2022 at 08:17AM +0000, Kefeng Wang wrote:
> As "kcsan: Support detecting a subset of missing memory barriers"
> introduced KCSAN_STRICT which make kcsan detects more missing memory
> barrier, but arm64 don't have KCSAN instrumentation for barriers, so
> the new selftest test_barrier() will fail, then panic.

Thanks for fixing this - did kcsan_test module pass as well?

> Let's prefix all barriers with __ on arm64, as asm-generic/barriers.h
> defined the final instrumented version of these barriers, which will
> fix the above issues.
> 
> Fixes: dd03762ab608 ("arm64: Enable KCSAN")
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
>  arch/arm64/include/asm/barrier.h | 12 ++++++------
>  include/asm-generic/barrier.h    |  4 ++++
>  2 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/barrier.h b/arch/arm64/include/asm/barrier.h
> index 62217be36217..9760a8d4ed0a 100644
> --- a/arch/arm64/include/asm/barrier.h
> +++ b/arch/arm64/include/asm/barrier.h
> @@ -46,13 +46,13 @@
>  #define pmr_sync()	do {} while (0)
>  #endif
>  
> -#define mb()		dsb(sy)
> -#define rmb()		dsb(ld)
> -#define wmb()		dsb(st)
> +#define __mb()		dsb(sy)
> +#define __rmb()		dsb(ld)
> +#define __wmb()		dsb(st)
>  
> -#define dma_mb()	dmb(osh)
> -#define dma_rmb()	dmb(oshld)
> -#define dma_wmb()	dmb(oshst)
> +#define __dma_mb()	dmb(osh)
> +#define __dma_rmb()	dmb(oshld)
> +#define __dma_wmb()	dmb(oshst)
>  
>  #define io_stop_wc()	dgh()
>  
> diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h
> index fd7e8fbaeef1..18863c50e9ce 100644
> --- a/include/asm-generic/barrier.h
> +++ b/include/asm-generic/barrier.h
> @@ -38,6 +38,10 @@
>  #define wmb()	do { kcsan_wmb(); __wmb(); } while (0)
>  #endif
>  
> +#ifdef __dma_mb
> +#define dma_mb()	do { kcsan_mb(); __dma_mb(); } while (0)
> +#endif
> +

So it looks like arm64 is the only arch that defines dma_mb(). By adding
it to asm-generic, we'd almost be encouraging other architectures to add
it, which I don't know we want.

Documentation/memory-barriers.txt doesn't mention dma_mb() either - so
perhaps dma_mb() doesn't belong in asm-generic/barrier.h, and you could
only change arm64's definition of dma_mb() to add the kcsan_mb().

Preferences? Maybe arch64 maintainers have more background on why arm64
is an anomaly here.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: kcsan: Fix kcsan test_barrier fail and panic

[Mark Rutland]

On Tue, Apr 26, 2022 at 02:10:06PM +0200, Marco Elver wrote:
> On Tue, Apr 26, 2022 at 08:17AM +0000, Kefeng Wang wrote:
> > diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h
> > index fd7e8fbaeef1..18863c50e9ce 100644
> > --- a/include/asm-generic/barrier.h
> > +++ b/include/asm-generic/barrier.h
> > @@ -38,6 +38,10 @@
> >  #define wmb()	do { kcsan_wmb(); __wmb(); } while (0)
> >  #endif
> >  
> > +#ifdef __dma_mb
> > +#define dma_mb()	do { kcsan_mb(); __dma_mb(); } while (0)
> > +#endif
> > +
> 
> So it looks like arm64 is the only arch that defines dma_mb(). By adding
> it to asm-generic, we'd almost be encouraging other architectures to add
> it, which I don't know we want.
> 
> Documentation/memory-barriers.txt doesn't mention dma_mb() either - so
> perhaps dma_mb() doesn't belong in asm-generic/barrier.h, and you could
> only change arm64's definition of dma_mb() to add the kcsan_mb().
> 
> Preferences? Maybe arch64 maintainers have more background on why arm64
> is an anomaly here.

Looking around, there's a single user:

[mark@lakrids:~/src/linux]% git grep -w dma_mb 
arch/arm64/include/asm/barrier.h:#define dma_mb()       dmb(osh)
arch/arm64/include/asm/io.h:#define __iomb()            dma_mb()

[mark@lakrids:~/src/linux]% git grep -w __iomb 
arch/arm64/include/asm/io.h:#define __iomb()            dma_mb()
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c:    __iomb();

... and that was introduced in commit:

  a76a37777f2c936b ("iommu/arm-smmu-v3: Ensure queue is read after updating prod pointer")

... where it is used to ensure that prior (read and write) accesses to
memory by a CPU are ordered w.r.t. a subsequent MMIO write.

That seems like it could be a generic shape of problem (especially for
IOMMUs), even if arm64 is the only architecture with an implementation
today. From my PoV it would weem to make sense as a generic thing, and
should probably be added to Documentation/memory-barriers.txt.

Will, thoughts?

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: kcsan: Fix kcsan test_barrier fail and panic

[Will Deacon]

On Tue, Apr 26, 2022 at 01:50:39PM +0100, Mark Rutland wrote:
> On Tue, Apr 26, 2022 at 02:10:06PM +0200, Marco Elver wrote:
> > On Tue, Apr 26, 2022 at 08:17AM +0000, Kefeng Wang wrote:
> > > diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h
> > > index fd7e8fbaeef1..18863c50e9ce 100644
> > > --- a/include/asm-generic/barrier.h
> > > +++ b/include/asm-generic/barrier.h
> > > @@ -38,6 +38,10 @@
> > >  #define wmb()	do { kcsan_wmb(); __wmb(); } while (0)
> > >  #endif
> > >  
> > > +#ifdef __dma_mb
> > > +#define dma_mb()	do { kcsan_mb(); __dma_mb(); } while (0)
> > > +#endif
> > > +
> > 
> > So it looks like arm64 is the only arch that defines dma_mb(). By adding
> > it to asm-generic, we'd almost be encouraging other architectures to add
> > it, which I don't know we want.
> > 
> > Documentation/memory-barriers.txt doesn't mention dma_mb() either - so
> > perhaps dma_mb() doesn't belong in asm-generic/barrier.h, and you could
> > only change arm64's definition of dma_mb() to add the kcsan_mb().
> > 
> > Preferences? Maybe arch64 maintainers have more background on why arm64
> > is an anomaly here.
> 
> Looking around, there's a single user:
> 
> [mark@lakrids:~/src/linux]% git grep -w dma_mb 
> arch/arm64/include/asm/barrier.h:#define dma_mb()       dmb(osh)
> arch/arm64/include/asm/io.h:#define __iomb()            dma_mb()
> 
> [mark@lakrids:~/src/linux]% git grep -w __iomb 
> arch/arm64/include/asm/io.h:#define __iomb()            dma_mb()
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c:    __iomb();
> 
> ... and that was introduced in commit:
> 
>   a76a37777f2c936b ("iommu/arm-smmu-v3: Ensure queue is read after updating prod pointer")
> 
> ... where it is used to ensure that prior (read and write) accesses to
> memory by a CPU are ordered w.r.t. a subsequent MMIO write.
> 
> That seems like it could be a generic shape of problem (especially for
> IOMMUs), even if arm64 is the only architecture with an implementation
> today. From my PoV it would weem to make sense as a generic thing, and
> should probably be added to Documentation/memory-barriers.txt.
> 
> Will, thoughts?

Given that the read and write variants are generic, making the full-fat
version version as well makes sense to me.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: kcsan: Fix kcsan test_barrier fail and panic

[Kefeng Wang]

On 2022/4/26 20:10, Marco Elver wrote:
> On Tue, Apr 26, 2022 at 08:17AM +0000, Kefeng Wang wrote:
>> As "kcsan: Support detecting a subset of missing memory barriers"
>> introduced KCSAN_STRICT which make kcsan detects more missing memory
>> barrier, but arm64 don't have KCSAN instrumentation for barriers, so
>> the new selftest test_barrier() will fail, then panic.
> Thanks for fixing this - did kcsan_test module pass as well?

Yes， selftest and kcsan_test  passed with gcc11 & clang 14.

...

>> +#ifdef __dma_mb
>> +#define dma_mb()	do { kcsan_mb(); __dma_mb(); } while (0)
>> +#endif
>> +
> So it looks like arm64 is the only arch that defines dma_mb(). By adding
> it to asm-generic, we'd almost be encouraging other architectures to add
> it, which I don't know we want.
>
> Documentation/memory-barriers.txt doesn't mention dma_mb() either - so
> perhaps dma_mb() doesn't belong in asm-generic/barrier.h, and you could
> only change arm64's definition of dma_mb() to add the kcsan_mb().
>
> Preferences? Maybe arch64 maintainers have more background on why arm64
> is an anomaly here.
> .
Let's wait to see aarch64 maintainers's suggestion.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: kcsan: Fix kcsan test_barrier fail and panic

[Mark Rutland]

Hi,

On Tue, Apr 26, 2022 at 08:17:00AM +0000, Kefeng Wang wrote:
> As "kcsan: Support detecting a subset of missing memory barriers"
> introduced KCSAN_STRICT which make kcsan detects more missing memory
> barrier, but arm64 don't have KCSAN instrumentation for barriers, so
> the new selftest test_barrier() will fail, then panic.
> 
> Let's prefix all barriers with __ on arm64, as asm-generic/barriers.h
> defined the final instrumented version of these barriers, which will
> fix the above issues.
> 
> Fixes: dd03762ab608 ("arm64: Enable KCSAN")
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>

I don't think the Fixes tag is necessary given this is a new feature
which depends upon EXPERT, and I'm worried it encourages backporting
this into a kernel where it would be broken, so I'd prefer we drop that
tag.

IIUC when we originially looked at this the instrumentation wasn't safe
and would violate noinstr requirements. Looking at linux/kcsan-checks.h,
the comments block for __KCSAN_BARRIER_TO_SIGNAL_FENCE() say that it
will respect __nokcsan, so it looks like that might be safe now.

It looks like that's the case as of commit:
  
  bd3d5bd1a0ad3864 ("kcsan: Support WEAK_MEMORY with Clang where no objtool support exists")

... which requires clang 14.0.0+.

That looks to have gone in concurrently with dd03762ab608, but is
clearly a prerequisite, so as above I'd strongly prefer we drop the
Fixes tag.

It would be good if we could note that explicitly in the commit message.

Have you eyeballed the generated assembly to verify that this works as
expected for __nokcsan ?

Thanks,
Mark.

> ---
>  arch/arm64/include/asm/barrier.h | 12 ++++++------
>  include/asm-generic/barrier.h    |  4 ++++
>  2 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/barrier.h b/arch/arm64/include/asm/barrier.h
> index 62217be36217..9760a8d4ed0a 100644
> --- a/arch/arm64/include/asm/barrier.h
> +++ b/arch/arm64/include/asm/barrier.h
> @@ -46,13 +46,13 @@
>  #define pmr_sync()	do {} while (0)
>  #endif
>  
> -#define mb()		dsb(sy)
> -#define rmb()		dsb(ld)
> -#define wmb()		dsb(st)
> +#define __mb()		dsb(sy)
> +#define __rmb()		dsb(ld)
> +#define __wmb()		dsb(st)
>  
> -#define dma_mb()	dmb(osh)
> -#define dma_rmb()	dmb(oshld)
> -#define dma_wmb()	dmb(oshst)
> +#define __dma_mb()	dmb(osh)
> +#define __dma_rmb()	dmb(oshld)
> +#define __dma_wmb()	dmb(oshst)
>  
>  #define io_stop_wc()	dgh()
>  
> diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h
> index fd7e8fbaeef1..18863c50e9ce 100644
> --- a/include/asm-generic/barrier.h
> +++ b/include/asm-generic/barrier.h
> @@ -38,6 +38,10 @@
>  #define wmb()	do { kcsan_wmb(); __wmb(); } while (0)
>  #endif
>  
> +#ifdef __dma_mb
> +#define dma_mb()	do { kcsan_mb(); __dma_mb(); } while (0)
> +#endif
> +
>  #ifdef __dma_rmb
>  #define dma_rmb()	do { kcsan_rmb(); __dma_rmb(); } while (0)
>  #endif
> -- 
> 2.27.0
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: kcsan: Fix kcsan test_barrier fail and panic

[Kefeng Wang]

On 2022/4/26 20:42, Mark Rutland wrote:
> Hi,
>
> On Tue, Apr 26, 2022 at 08:17:00AM +0000, Kefeng Wang wrote:
>> As "kcsan: Support detecting a subset of missing memory barriers"
>> introduced KCSAN_STRICT which make kcsan detects more missing memory
>> barrier, but arm64 don't have KCSAN instrumentation for barriers, so
>> the new selftest test_barrier() will fail, then panic.
>>
>> Let's prefix all barriers with __ on arm64, as asm-generic/barriers.h
>> defined the final instrumented version of these barriers, which will
>> fix the above issues.
>>
>> Fixes: dd03762ab608 ("arm64: Enable KCSAN")
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> I don't think the Fixes tag is necessary given this is a new feature
> which depends upon EXPERT, and I'm worried it encourages backporting
> this into a kernel where it would be broken, so I'd prefer we drop that
> tag.
>
> IIUC when we originially looked at this the instrumentation wasn't safe
> and would violate noinstr requirements. Looking at linux/kcsan-checks.h,
> the comments block for __KCSAN_BARRIER_TO_SIGNAL_FENCE() say that it
> will respect __nokcsan, so it looks like that might be safe now.
>
> It looks like that's the case as of commit:
>    
>    bd3d5bd1a0ad3864 ("kcsan: Support WEAK_MEMORY with Clang where no objtool support exists")
>
> ... which requires clang 14.0.0+.
>
> That looks to have gone in concurrently with dd03762ab608, but is
> clearly a prerequisite, so as above I'd strongly prefer we drop the
> Fixes tag.

Sure, the "kcsan: Support detecting a subset of missing memory 
barriers"[1] and

dd03762ab608 "arm64: Enable KCSAN" are both merged in v5.17.  I will 
drop the Fixes tag.

> It would be good if we could note that explicitly in the commit message.
I will add some message into v2.
>
> Have you eyeballed the generated assembly to verify that this works as
> expected for __no_kcsan ?
Look good,  will recheck it.
>
> Thanks,
> Mark.
[1] 
https://patchwork.kernel.org/project/linux-mm/cover/20211130114433.2580590-1-elver@google.com/ 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel