* [PATCH v1] arm64: allow building with kcov coverage on ARM64 @ 2016-03-31 13:54 Alexander Potapenko 2016-03-31 14:02 ` Alexander Potapenko 2016-03-31 14:29 ` Mark Rutland 0 siblings, 2 replies; 14+ messages in thread From: Alexander Potapenko @ 2016-03-31 13:54 UTC (permalink / raw) To: dvyukov, catalin.marinas, quentin.casasnovas, will.deacon Cc: linux-kernel, syzkaller, linux-arm-kernel, kcc, akpm Add ARCH_HAS_KCOV to ARM64 config. Disable instrumentation of arch/arm64/lib/delay.c Signed-off-by: Alexander Potapenko <glider@google.com> --- arch/arm64/Kconfig | 1 + arch/arm64/lib/Makefile | 3 +++ 2 files changed, 4 insertions(+) diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 4f43622..c52aa61 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -7,6 +7,7 @@ config ARM64 select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE select ARCH_HAS_ELF_RANDOMIZE select ARCH_HAS_GCOV_PROFILE_ALL + select ARCH_HAS_KCOV select ARCH_HAS_SG_CHAIN select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST select ARCH_USE_CMPXCHG_LOCKREF diff --git a/arch/arm64/lib/Makefile b/arch/arm64/lib/Makefile index c86b790..b407bc1 100644 --- a/arch/arm64/lib/Makefile +++ b/arch/arm64/lib/Makefile @@ -1,3 +1,6 @@ +# Produces uninteresting flaky coverage. +KCOV_INSTRUMENT_delay.o := n + lib-y := bitops.o clear_user.o delay.o copy_from_user.o \ copy_to_user.o copy_in_user.o copy_page.o \ clear_page.o memchr.o memcpy.o memmove.o memset.o \ -- 2.8.0.rc3.226.g39d4020 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v1] arm64: allow building with kcov coverage on ARM64 2016-03-31 13:54 [PATCH v1] arm64: allow building with kcov coverage on ARM64 Alexander Potapenko @ 2016-03-31 14:02 ` Alexander Potapenko 2016-03-31 14:29 ` Mark Rutland 1 sibling, 0 replies; 14+ messages in thread From: Alexander Potapenko @ 2016-03-31 14:02 UTC (permalink / raw) To: Dmitriy Vyukov, catalin.marinas, quentin.casasnovas, will.deacon Cc: LKML, syzkaller, linux-arm-kernel, Kostya Serebryany, Andrew Morton On Thu, Mar 31, 2016 at 3:54 PM, Alexander Potapenko <glider@google.com> wrote: > Add ARCH_HAS_KCOV to ARM64 config. Disable instrumentation of > arch/arm64/lib/delay.c > > Signed-off-by: Alexander Potapenko <glider@google.com> > --- > arch/arm64/Kconfig | 1 + > arch/arm64/lib/Makefile | 3 +++ > 2 files changed, 4 insertions(+) > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index 4f43622..c52aa61 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -7,6 +7,7 @@ config ARM64 > select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE > select ARCH_HAS_ELF_RANDOMIZE > select ARCH_HAS_GCOV_PROFILE_ALL > + select ARCH_HAS_KCOV > select ARCH_HAS_SG_CHAIN > select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST > select ARCH_USE_CMPXCHG_LOCKREF > diff --git a/arch/arm64/lib/Makefile b/arch/arm64/lib/Makefile > index c86b790..b407bc1 100644 > --- a/arch/arm64/lib/Makefile > +++ b/arch/arm64/lib/Makefile > @@ -1,3 +1,6 @@ > +# Produces uninteresting flaky coverage. > +KCOV_INSTRUMENT_delay.o := n > + > lib-y := bitops.o clear_user.o delay.o copy_from_user.o \ > copy_to_user.o copy_in_user.o copy_page.o \ > clear_page.o memchr.o memcpy.o memmove.o memset.o \ > -- > 2.8.0.rc3.226.g39d4020 > Hi all, I've ported the above patch from kernel 3.18, which I was able to successfully run with kcov support on an arm64 device. The original 3.18 patch is at https://chromium-review.googlesource.com/#/c/334560/1, it also touches arch/arm64/kernel/Makefile, which is unnecessary in upstream code, because efi-stub.c has been moved to libstub. Therefore I suspect the above patch should be correct, but I don't have an opportunity to actually test it. Alex -- Alexander Potapenko Software Engineer Google Germany GmbH Erika-Mann-Straße, 33 80636 München Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] arm64: allow building with kcov coverage on ARM64 2016-03-31 13:54 [PATCH v1] arm64: allow building with kcov coverage on ARM64 Alexander Potapenko 2016-03-31 14:02 ` Alexander Potapenko @ 2016-03-31 14:29 ` Mark Rutland 2016-03-31 15:09 ` Alexander Potapenko 1 sibling, 1 reply; 14+ messages in thread From: Mark Rutland @ 2016-03-31 14:29 UTC (permalink / raw) To: Alexander Potapenko Cc: dvyukov, catalin.marinas, quentin.casasnovas, will.deacon, kcc, akpm, syzkaller, linux-kernel, linux-arm-kernel, ard.biesheuvel, marc.zyngier, christoffer.dall Hi, On Thu, Mar 31, 2016 at 03:54:45PM +0200, Alexander Potapenko wrote: > Add ARCH_HAS_KCOV to ARM64 config. Disable instrumentation of > arch/arm64/lib/delay.c Why do we disable instrumentation of delay.c? What exactly does kcov instrumentation imply? Does it require certain data to be mapped or certain functions to be callable while instrumented functions are called? We have some C code that is run outside of the normal kernel context (e.g. EFI stub, KVM hyp code), and I suspect it may be necessary to disable instrumentation for those also. Thanks, Mark. > Signed-off-by: Alexander Potapenko <glider@google.com> > --- > arch/arm64/Kconfig | 1 + > arch/arm64/lib/Makefile | 3 +++ > 2 files changed, 4 insertions(+) > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index 4f43622..c52aa61 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -7,6 +7,7 @@ config ARM64 > select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE > select ARCH_HAS_ELF_RANDOMIZE > select ARCH_HAS_GCOV_PROFILE_ALL > + select ARCH_HAS_KCOV > select ARCH_HAS_SG_CHAIN > select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST > select ARCH_USE_CMPXCHG_LOCKREF > diff --git a/arch/arm64/lib/Makefile b/arch/arm64/lib/Makefile > index c86b790..b407bc1 100644 > --- a/arch/arm64/lib/Makefile > +++ b/arch/arm64/lib/Makefile > @@ -1,3 +1,6 @@ > +# Produces uninteresting flaky coverage. > +KCOV_INSTRUMENT_delay.o := n > + > lib-y := bitops.o clear_user.o delay.o copy_from_user.o \ > copy_to_user.o copy_in_user.o copy_page.o \ > clear_page.o memchr.o memcpy.o memmove.o memset.o \ > -- > 2.8.0.rc3.226.g39d4020 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] arm64: allow building with kcov coverage on ARM64 2016-03-31 14:29 ` Mark Rutland @ 2016-03-31 15:09 ` Alexander Potapenko 2016-03-31 16:00 ` Mark Rutland 0 siblings, 1 reply; 14+ messages in thread From: Alexander Potapenko @ 2016-03-31 15:09 UTC (permalink / raw) To: Mark Rutland Cc: Dmitriy Vyukov, catalin.marinas, quentin.casasnovas, will.deacon, Kostya Serebryany, Andrew Morton, syzkaller, LKML, linux-arm-kernel, ard.biesheuvel, marc.zyngier, christoffer.dall On Thu, Mar 31, 2016 at 4:29 PM, Mark Rutland <mark.rutland@arm.com> wrote: > Hi, > > On Thu, Mar 31, 2016 at 03:54:45PM +0200, Alexander Potapenko wrote: >> Add ARCH_HAS_KCOV to ARM64 config. Disable instrumentation of >> arch/arm64/lib/delay.c > > Why do we disable instrumentation of delay.c? The main purpose of kcov is collecting coverage from syscalls. As far as I understand, coverage of functions from delay.c doesn't deterministically depend on the syscalls being called and their arguments. The initial kcov implementation (https://github.com/torvalds/linux/commit/5c9a8750a6409c63a0f01d51a9024861022f6593) disabled instrumentation of arch/x86/lib/delay.c, so I just copied that chunk. > What exactly does kcov instrumentation imply? Does it require certain > data to be mapped or certain functions to be callable while instrumented > functions are called? Yes, there is __sanitizer_cov_trace_pc() that must be callable. At boot time |current->kcov_mode| zero, so it virtually does nothing. Currently kcov instrumentation is disabled for the following files: arch/x86/boot/* arch/x86/boot/compressed/* arch/x86/entry/vdso/* arch/x86/kernel/* arch/x86/kernel/apic/* arch/x86/kernel/cpu/common.c arch/x86/kernel/cpu/perf_event.c arch/x86/lib/delay.c arch/x86/mm/tlb.c arch/x86/realmode/rm/* Only a handful of the above have corresponding files in arch/arm64: arch/arm64/boot/* arch/arm64/kernel/* arch/arm64/lib/delay.c My patch explicitly disables instrumentation for arch/arm64/lib/delay.c. I never had problems with arch/arm64/boot/* and arch/arm64/kernel/* in the 3.18 kernel, although instrumentation of the corresponding x86 code is claimed to cause boot-time hangs. We can act conservatively and still disable instrumentation for these two dirs just to make sure nothing breaks in the future. > We have some C code that is run outside of the normal kernel context > (e.g. EFI stub, KVM hyp code), and I suspect it may be necessary to > disable instrumentation for those also. EFI stub and a number of other files is already disabled by the initial kcov patch. I understand there might be some code specific to ARM64 that I may have overlooked, so I'd be grateful if someone could try the patch out with the upstream kernel. WBR, Alex > Thanks, > Mark. > >> Signed-off-by: Alexander Potapenko <glider@google.com> >> --- >> arch/arm64/Kconfig | 1 + >> arch/arm64/lib/Makefile | 3 +++ >> 2 files changed, 4 insertions(+) >> >> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig >> index 4f43622..c52aa61 100644 >> --- a/arch/arm64/Kconfig >> +++ b/arch/arm64/Kconfig >> @@ -7,6 +7,7 @@ config ARM64 >> select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE >> select ARCH_HAS_ELF_RANDOMIZE >> select ARCH_HAS_GCOV_PROFILE_ALL >> + select ARCH_HAS_KCOV >> select ARCH_HAS_SG_CHAIN >> select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST >> select ARCH_USE_CMPXCHG_LOCKREF >> diff --git a/arch/arm64/lib/Makefile b/arch/arm64/lib/Makefile >> index c86b790..b407bc1 100644 >> --- a/arch/arm64/lib/Makefile >> +++ b/arch/arm64/lib/Makefile >> @@ -1,3 +1,6 @@ >> +# Produces uninteresting flaky coverage. >> +KCOV_INSTRUMENT_delay.o := n >> + >> lib-y := bitops.o clear_user.o delay.o copy_from_user.o \ >> copy_to_user.o copy_in_user.o copy_page.o \ >> clear_page.o memchr.o memcpy.o memmove.o memset.o \ >> -- >> 2.8.0.rc3.226.g39d4020 >> >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >> -- Alexander Potapenko Software Engineer Google Germany GmbH Erika-Mann-Straße, 33 80636 München Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] arm64: allow building with kcov coverage on ARM64 2016-03-31 15:09 ` Alexander Potapenko @ 2016-03-31 16:00 ` Mark Rutland 2016-03-31 16:33 ` Alexander Potapenko 0 siblings, 1 reply; 14+ messages in thread From: Mark Rutland @ 2016-03-31 16:00 UTC (permalink / raw) To: Alexander Potapenko Cc: Dmitriy Vyukov, catalin.marinas, quentin.casasnovas, will.deacon, Kostya Serebryany, Andrew Morton, syzkaller, LKML, linux-arm-kernel, ard.biesheuvel, marc.zyngier, christoffer.dall On Thu, Mar 31, 2016 at 05:09:29PM +0200, Alexander Potapenko wrote: > On Thu, Mar 31, 2016 at 4:29 PM, Mark Rutland <mark.rutland@arm.com> wrote: > > Hi, > > > > On Thu, Mar 31, 2016 at 03:54:45PM +0200, Alexander Potapenko wrote: > >> Add ARCH_HAS_KCOV to ARM64 config. Disable instrumentation of > >> arch/arm64/lib/delay.c > > > > Why do we disable instrumentation of delay.c? > The main purpose of kcov is collecting coverage from syscalls. As far > as I understand, coverage of functions from delay.c doesn't > deterministically depend on the syscalls being called and their > arguments. > The initial kcov implementation > (https://github.com/torvalds/linux/commit/5c9a8750a6409c63a0f01d51a9024861022f6593) > disabled instrumentation of arch/x86/lib/delay.c, so I just copied > that chunk. > > > What exactly does kcov instrumentation imply? Does it require certain > > data to be mapped or certain functions to be callable while instrumented > > functions are called? > Yes, there is __sanitizer_cov_trace_pc() that must be callable. That will definitely be a problem for the KVM code which is run at a different exception level with a different memory map. For GCOV, KASAN, and UBSAN we simply disable instrumentation of that code [1]. We should be able to do similarly for KCOV. > At boot time |current->kcov_mode| zero, so it virtually does nothing. > > Currently kcov instrumentation is disabled for the following files: > arch/x86/boot/* > arch/x86/boot/compressed/* > arch/x86/entry/vdso/* > arch/x86/realmode/rm/* These are executed outside of the usual kernel context / address space, so excluding these makes sense to me. > arch/x86/kernel/* > arch/x86/kernel/apic/* > arch/x86/kernel/cpu/common.c > arch/x86/kernel/cpu/perf_event.c > arch/x86/lib/delay.c > arch/x86/mm/tlb.c For these, it's not immediately clear to me why instrumentation is disabled, so I don't know whether or not we can instrument the analogous arm64 code. > Only a handful of the above have corresponding files in arch/arm64: > arch/arm64/boot/* > arch/arm64/kernel/* > arch/arm64/lib/delay.c We have arch/arm64/kernel/perf_event.c, and a couple of other files that are directly analogous, even if the paths don't quite line up. > My patch explicitly disables instrumentation for arch/arm64/lib/delay.c. > I never had problems with arch/arm64/boot/* and arch/arm64/kernel/* in > the 3.18 kernel, although instrumentation of the corresponding x86 > code is claimed to cause boot-time hangs. > We can act conservatively and still disable instrumentation for these > two dirs just to make sure nothing breaks in the future. I'd rather that we understood why instrumentation of the above is disabled, such that we can make a sensible decision from the outset. > > We have some C code that is run outside of the normal kernel context > > (e.g. EFI stub, KVM hyp code), and I suspect it may be necessary to > > disable instrumentation for those also. > EFI stub and a number of other files is already disabled by the > initial kcov patch. > I understand there might be some code specific to ARM64 that I may > have overlooked, so I'd be grateful if someone could try the patch out > with the upstream kernel. The only such code that I'm immediately aware of is the hyp-context KVM code, as mentioned above. Thanks, Mark. [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-March/416790.html ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] arm64: allow building with kcov coverage on ARM64 2016-03-31 16:00 ` Mark Rutland @ 2016-03-31 16:33 ` Alexander Potapenko 2016-03-31 16:43 ` Alexander Potapenko 2016-03-31 17:14 ` Mark Rutland 0 siblings, 2 replies; 14+ messages in thread From: Alexander Potapenko @ 2016-03-31 16:33 UTC (permalink / raw) To: Mark Rutland Cc: Dmitriy Vyukov, catalin.marinas, quentin.casasnovas, will.deacon, Kostya Serebryany, Andrew Morton, syzkaller, LKML, linux-arm-kernel, Ard Biesheuvel, marc.zyngier, Christoffer Dall On Thu, Mar 31, 2016 at 6:00 PM, Mark Rutland <mark.rutland@arm.com> wrote: > On Thu, Mar 31, 2016 at 05:09:29PM +0200, Alexander Potapenko wrote: >> On Thu, Mar 31, 2016 at 4:29 PM, Mark Rutland <mark.rutland@arm.com> wrote: >> > Hi, >> > >> > On Thu, Mar 31, 2016 at 03:54:45PM +0200, Alexander Potapenko wrote: >> >> Add ARCH_HAS_KCOV to ARM64 config. Disable instrumentation of >> >> arch/arm64/lib/delay.c >> > >> > Why do we disable instrumentation of delay.c? >> The main purpose of kcov is collecting coverage from syscalls. As far >> as I understand, coverage of functions from delay.c doesn't >> deterministically depend on the syscalls being called and their >> arguments. >> The initial kcov implementation >> (https://github.com/torvalds/linux/commit/5c9a8750a6409c63a0f01d51a9024861022f6593) >> disabled instrumentation of arch/x86/lib/delay.c, so I just copied >> that chunk. >> >> > What exactly does kcov instrumentation imply? Does it require certain >> > data to be mapped or certain functions to be callable while instrumented >> > functions are called? >> Yes, there is __sanitizer_cov_trace_pc() that must be callable. > > That will definitely be a problem for the KVM code which is run at a > different exception level with a different memory map. For GCOV, KASAN, > and UBSAN we simply disable instrumentation of that code [1]. > > We should be able to do similarly for KCOV. Ok, I'll send out the updated patch. >> At boot time |current->kcov_mode| zero, so it virtually does nothing. >> >> Currently kcov instrumentation is disabled for the following files: > >> arch/x86/boot/* >> arch/x86/boot/compressed/* >> arch/x86/entry/vdso/* >> arch/x86/realmode/rm/* > > These are executed outside of the usual kernel context / address space, > so excluding these makes sense to me. > >> arch/x86/kernel/* >> arch/x86/kernel/apic/* >> arch/x86/kernel/cpu/common.c >> arch/x86/kernel/cpu/perf_event.c >> arch/x86/lib/delay.c >> arch/x86/mm/tlb.c > > For these, it's not immediately clear to me why instrumentation is > disabled, so I don't know whether or not we can instrument the analogous > arm64 code. According to the comments in https://github.com/torvalds/linux/commit/5c9a8750a6409c63a0f01d51a9024861022f6593, instrumentation of arch/x86/kernel/apic/* and arch/x86/lib/delay.c leads to non-deterministic coverage, instrumenting others prevent the kernel from booting. >> Only a handful of the above have corresponding files in arch/arm64: >> arch/arm64/boot/* >> arch/arm64/kernel/* >> arch/arm64/lib/delay.c > > We have arch/arm64/kernel/perf_event.c, and a couple of other files that > are directly analogous, even if the paths don't quite line up. Ok, it makes sense to also disable arch/arm64/kernel/perf_event.c then. >> My patch explicitly disables instrumentation for arch/arm64/lib/delay.c. >> I never had problems with arch/arm64/boot/* and arch/arm64/kernel/* in >> the 3.18 kernel, although instrumentation of the corresponding x86 >> code is claimed to cause boot-time hangs. >> We can act conservatively and still disable instrumentation for these >> two dirs just to make sure nothing breaks in the future. > > I'd rather that we understood why instrumentation of the above is > disabled, such that we can make a sensible decision from the outset. > >> > We have some C code that is run outside of the normal kernel context >> > (e.g. EFI stub, KVM hyp code), and I suspect it may be necessary to >> > disable instrumentation for those also. >> EFI stub and a number of other files is already disabled by the >> initial kcov patch. >> I understand there might be some code specific to ARM64 that I may >> have overlooked, so I'd be grateful if someone could try the patch out >> with the upstream kernel. > > The only such code that I'm immediately aware of is the hyp-context KVM > code, as mentioned above. > > Thanks, > Mark. > > [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-March/416790.html -- Alexander Potapenko Software Engineer Google Germany GmbH Erika-Mann-Straße, 33 80636 München Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] arm64: allow building with kcov coverage on ARM64 2016-03-31 16:33 ` Alexander Potapenko @ 2016-03-31 16:43 ` Alexander Potapenko 2016-03-31 17:14 ` Mark Rutland 1 sibling, 0 replies; 14+ messages in thread From: Alexander Potapenko @ 2016-03-31 16:43 UTC (permalink / raw) To: Mark Rutland Cc: Dmitriy Vyukov, catalin.marinas, quentin.casasnovas, will.deacon, Kostya Serebryany, Andrew Morton, syzkaller, LKML, linux-arm-kernel, Ard Biesheuvel, marc.zyngier, Christoffer Dall On Thu, Mar 31, 2016 at 6:33 PM, Alexander Potapenko <glider@google.com> wrote: > On Thu, Mar 31, 2016 at 6:00 PM, Mark Rutland <mark.rutland@arm.com> wrote: >> On Thu, Mar 31, 2016 at 05:09:29PM +0200, Alexander Potapenko wrote: >>> On Thu, Mar 31, 2016 at 4:29 PM, Mark Rutland <mark.rutland@arm.com> wrote: >>> > Hi, >>> > >>> > On Thu, Mar 31, 2016 at 03:54:45PM +0200, Alexander Potapenko wrote: >>> >> Add ARCH_HAS_KCOV to ARM64 config. Disable instrumentation of >>> >> arch/arm64/lib/delay.c >>> > >>> > Why do we disable instrumentation of delay.c? >>> The main purpose of kcov is collecting coverage from syscalls. As far >>> as I understand, coverage of functions from delay.c doesn't >>> deterministically depend on the syscalls being called and their >>> arguments. >>> The initial kcov implementation >>> (https://github.com/torvalds/linux/commit/5c9a8750a6409c63a0f01d51a9024861022f6593) >>> disabled instrumentation of arch/x86/lib/delay.c, so I just copied >>> that chunk. >>> >>> > What exactly does kcov instrumentation imply? Does it require certain >>> > data to be mapped or certain functions to be callable while instrumented >>> > functions are called? >>> Yes, there is __sanitizer_cov_trace_pc() that must be callable. >> >> That will definitely be a problem for the KVM code which is run at a >> different exception level with a different memory map. For GCOV, KASAN, >> and UBSAN we simply disable instrumentation of that code [1]. >> >> We should be able to do similarly for KCOV. > Ok, I'll send out the updated patch. > >>> At boot time |current->kcov_mode| zero, so it virtually does nothing. >>> >>> Currently kcov instrumentation is disabled for the following files: >> >>> arch/x86/boot/* >>> arch/x86/boot/compressed/* >>> arch/x86/entry/vdso/* >>> arch/x86/realmode/rm/* >> >> These are executed outside of the usual kernel context / address space, >> so excluding these makes sense to me. >> >>> arch/x86/kernel/* >>> arch/x86/kernel/apic/* >>> arch/x86/kernel/cpu/common.c >>> arch/x86/kernel/cpu/perf_event.c >>> arch/x86/lib/delay.c >>> arch/x86/mm/tlb.c >> >> For these, it's not immediately clear to me why instrumentation is >> disabled, so I don't know whether or not we can instrument the analogous >> arm64 code. > According to the comments in > https://github.com/torvalds/linux/commit/5c9a8750a6409c63a0f01d51a9024861022f6593, > instrumentation of arch/x86/kernel/apic/* and arch/x86/lib/delay.c > leads to non-deterministic coverage, instrumenting others prevent the > kernel from booting. > >>> Only a handful of the above have corresponding files in arch/arm64: >>> arch/arm64/boot/* >>> arch/arm64/kernel/* >>> arch/arm64/lib/delay.c >> >> We have arch/arm64/kernel/perf_event.c, and a couple of other files that >> are directly analogous, even if the paths don't quite line up. > Ok, it makes sense to also disable arch/arm64/kernel/perf_event.c then. ... and certainly turn off instrumentation for the "couple of other files". By the way, I've just noticed that arch/x86/kernel/cpu/perf_event.c has moved, so the Makefile isn't accurate anymore. >>> My patch explicitly disables instrumentation for arch/arm64/lib/delay.c. >>> I never had problems with arch/arm64/boot/* and arch/arm64/kernel/* in >>> the 3.18 kernel, although instrumentation of the corresponding x86 >>> code is claimed to cause boot-time hangs. >>> We can act conservatively and still disable instrumentation for these >>> two dirs just to make sure nothing breaks in the future. >> >> I'd rather that we understood why instrumentation of the above is >> disabled, such that we can make a sensible decision from the outset. >> >>> > We have some C code that is run outside of the normal kernel context >>> > (e.g. EFI stub, KVM hyp code), and I suspect it may be necessary to >>> > disable instrumentation for those also. >>> EFI stub and a number of other files is already disabled by the >>> initial kcov patch. >>> I understand there might be some code specific to ARM64 that I may >>> have overlooked, so I'd be grateful if someone could try the patch out >>> with the upstream kernel. >> >> The only such code that I'm immediately aware of is the hyp-context KVM >> code, as mentioned above. >> >> Thanks, >> Mark. >> >> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-March/416790.html > > > > -- > Alexander Potapenko > Software Engineer > > Google Germany GmbH > Erika-Mann-Straße, 33 > 80636 München > > Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle > Registergericht und -nummer: Hamburg, HRB 86891 > Sitz der Gesellschaft: Hamburg -- Alexander Potapenko Software Engineer Google Germany GmbH Erika-Mann-Straße, 33 80636 München Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] arm64: allow building with kcov coverage on ARM64 2016-03-31 16:33 ` Alexander Potapenko 2016-03-31 16:43 ` Alexander Potapenko @ 2016-03-31 17:14 ` Mark Rutland 2016-03-31 17:18 ` Alexander Potapenko 1 sibling, 1 reply; 14+ messages in thread From: Mark Rutland @ 2016-03-31 17:14 UTC (permalink / raw) To: Alexander Potapenko Cc: Dmitriy Vyukov, catalin.marinas, quentin.casasnovas, will.deacon, Kostya Serebryany, Andrew Morton, syzkaller, LKML, linux-arm-kernel, Ard Biesheuvel, marc.zyngier, Christoffer Dall On Thu, Mar 31, 2016 at 06:33:24PM +0200, Alexander Potapenko wrote: > On Thu, Mar 31, 2016 at 6:00 PM, Mark Rutland <mark.rutland@arm.com> wrote: > > On Thu, Mar 31, 2016 at 05:09:29PM +0200, Alexander Potapenko wrote: > >> Currently kcov instrumentation is disabled for the following files: > > > >> arch/x86/boot/* > >> arch/x86/boot/compressed/* > >> arch/x86/entry/vdso/* > >> arch/x86/realmode/rm/* > > > > These are executed outside of the usual kernel context / address space, > > so excluding these makes sense to me. > > > >> arch/x86/kernel/* > >> arch/x86/kernel/apic/* > >> arch/x86/kernel/cpu/common.c > >> arch/x86/kernel/cpu/perf_event.c > >> arch/x86/lib/delay.c > >> arch/x86/mm/tlb.c > > > > For these, it's not immediately clear to me why instrumentation is > > disabled, so I don't know whether or not we can instrument the analogous > > arm64 code. > According to the comments in > https://github.com/torvalds/linux/commit/5c9a8750a6409c63a0f01d51a9024861022f6593, > instrumentation of arch/x86/kernel/apic/* and arch/x86/lib/delay.c > leads to non-deterministic coverage, To what extent does determinism matter? Are we just ruling out the worst cases, or is this likely to turn into a whack-a-mole game? Do we exclude clocksources and other driver code? Looking at the arm64 delay timer code, it looks like everything will be inlined (and therefore coverage should be deterministic so long as the delay functions are called deterministically). That said, the same looks basically true of the x86 code, so I guess I've misunderstood. > instrumenting others prevent the kernel from booting. I haven't been able to come up with a scenario whereby kcov would be fatal for the above, so it's difficult to say if we have equivalent problems. For reference, do we have any examples as to why any of these prevent booting? > >> Only a handful of the above have corresponding files in arch/arm64: > >> arch/arm64/boot/* > >> arch/arm64/kernel/* > >> arch/arm64/lib/delay.c > > > > We have arch/arm64/kernel/perf_event.c, and a couple of other files that > > are directly analogous, even if the paths don't quite line up. > Ok, it makes sense to also disable arch/arm64/kernel/perf_event.c then. Potentially, though it really depends on why it was excluded on x86. Some of the arm64 perf code lives in drivers/perf/arm_pmu.c, also. Thanks, Mark. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] arm64: allow building with kcov coverage on ARM64 2016-03-31 17:14 ` Mark Rutland @ 2016-03-31 17:18 ` Alexander Potapenko 2016-04-04 17:30 ` Dmitry Vyukov 0 siblings, 1 reply; 14+ messages in thread From: Alexander Potapenko @ 2016-03-31 17:18 UTC (permalink / raw) To: Mark Rutland, Dmitriy Vyukov Cc: catalin.marinas, quentin.casasnovas, will.deacon, Kostya Serebryany, Andrew Morton, syzkaller, LKML, linux-arm-kernel, Ard Biesheuvel, marc.zyngier, Christoffer Dall On Thu, Mar 31, 2016 at 7:14 PM, Mark Rutland <mark.rutland@arm.com> wrote: > On Thu, Mar 31, 2016 at 06:33:24PM +0200, Alexander Potapenko wrote: >> On Thu, Mar 31, 2016 at 6:00 PM, Mark Rutland <mark.rutland@arm.com> wrote: >> > On Thu, Mar 31, 2016 at 05:09:29PM +0200, Alexander Potapenko wrote: >> >> Currently kcov instrumentation is disabled for the following files: >> > >> >> arch/x86/boot/* >> >> arch/x86/boot/compressed/* >> >> arch/x86/entry/vdso/* >> >> arch/x86/realmode/rm/* >> > >> > These are executed outside of the usual kernel context / address space, >> > so excluding these makes sense to me. >> > >> >> arch/x86/kernel/* >> >> arch/x86/kernel/apic/* >> >> arch/x86/kernel/cpu/common.c >> >> arch/x86/kernel/cpu/perf_event.c >> >> arch/x86/lib/delay.c >> >> arch/x86/mm/tlb.c >> > >> > For these, it's not immediately clear to me why instrumentation is >> > disabled, so I don't know whether or not we can instrument the analogous >> > arm64 code. >> According to the comments in >> https://github.com/torvalds/linux/commit/5c9a8750a6409c63a0f01d51a9024861022f6593, >> instrumentation of arch/x86/kernel/apic/* and arch/x86/lib/delay.c >> leads to non-deterministic coverage, > > To what extent does determinism matter? Are we just ruling out the worst > cases, or is this likely to turn into a whack-a-mole game? I guess we'd better ask Dmitry who excluded these files on x86 and experimented with coverage a lot. Dmitry, can you clarify this, please? > Do we exclude clocksources and other driver code? > > Looking at the arm64 delay timer code, it looks like everything will be > inlined (and therefore coverage should be deterministic so long as the > delay functions are called deterministically). That said, the same looks > basically true of the x86 code, so I guess I've misunderstood. > >> instrumenting others prevent the kernel from booting. > > I haven't been able to come up with a scenario whereby kcov would be > fatal for the above, so it's difficult to say if we have equivalent > problems. > > For reference, do we have any examples as to why any of these prevent > booting? Not sure there's any documentation so far except for the comments in the original kcov patch. >> >> Only a handful of the above have corresponding files in arch/arm64: >> >> arch/arm64/boot/* >> >> arch/arm64/kernel/* >> >> arch/arm64/lib/delay.c >> > >> > We have arch/arm64/kernel/perf_event.c, and a couple of other files that >> > are directly analogous, even if the paths don't quite line up. >> Ok, it makes sense to also disable arch/arm64/kernel/perf_event.c then. > > Potentially, though it really depends on why it was excluded on x86. > > Some of the arm64 perf code lives in drivers/perf/arm_pmu.c, also. > > Thanks, > Mark. -- Alexander Potapenko Software Engineer Google Germany GmbH Erika-Mann-Straße, 33 80636 München Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] arm64: allow building with kcov coverage on ARM64 2016-03-31 17:18 ` Alexander Potapenko @ 2016-04-04 17:30 ` Dmitry Vyukov 2016-04-12 11:17 ` Alexander Potapenko 0 siblings, 1 reply; 14+ messages in thread From: Dmitry Vyukov @ 2016-04-04 17:30 UTC (permalink / raw) To: Alexander Potapenko Cc: Mark Rutland, Catalin Marinas, Quentin Casasnovas, Will Deacon, Kostya Serebryany, Andrew Morton, syzkaller, LKML, linux-arm-kernel, Ard Biesheuvel, marc.zyngier, Christoffer Dall On Thu, Mar 31, 2016 at 7:18 PM, Alexander Potapenko <glider@google.com> wrote: > On Thu, Mar 31, 2016 at 7:14 PM, Mark Rutland <mark.rutland@arm.com> wrote: >> On Thu, Mar 31, 2016 at 06:33:24PM +0200, Alexander Potapenko wrote: >>> On Thu, Mar 31, 2016 at 6:00 PM, Mark Rutland <mark.rutland@arm.com> wrote: >>> > On Thu, Mar 31, 2016 at 05:09:29PM +0200, Alexander Potapenko wrote: >>> >> Currently kcov instrumentation is disabled for the following files: >>> > >>> >> arch/x86/boot/* >>> >> arch/x86/boot/compressed/* >>> >> arch/x86/entry/vdso/* >>> >> arch/x86/realmode/rm/* >>> > >>> > These are executed outside of the usual kernel context / address space, >>> > so excluding these makes sense to me. >>> > >>> >> arch/x86/kernel/* >>> >> arch/x86/kernel/apic/* >>> >> arch/x86/kernel/cpu/common.c >>> >> arch/x86/kernel/cpu/perf_event.c >>> >> arch/x86/lib/delay.c >>> >> arch/x86/mm/tlb.c >>> > >>> > For these, it's not immediately clear to me why instrumentation is >>> > disabled, so I don't know whether or not we can instrument the analogous >>> > arm64 code. >>> According to the comments in >>> https://github.com/torvalds/linux/commit/5c9a8750a6409c63a0f01d51a9024861022f6593, >>> instrumentation of arch/x86/kernel/apic/* and arch/x86/lib/delay.c >>> leads to non-deterministic coverage, >> >> To what extent does determinism matter? Are we just ruling out the worst >> cases, or is this likely to turn into a whack-a-mole game? > I guess we'd better ask Dmitry who excluded these files on x86 and > experimented with coverage a lot. > Dmitry, can you clarify this, please? >> Do we exclude clocksources and other driver code? >> >> Looking at the arm64 delay timer code, it looks like everything will be >> inlined (and therefore coverage should be deterministic so long as the >> delay functions are called deterministically). That said, the same looks >> basically true of the x86 code, so I guess I've misunderstood. >> >>> instrumenting others prevent the kernel from booting. >> >> I haven't been able to come up with a scenario whereby kcov would be >> fatal for the above, so it's difficult to say if we have equivalent >> problems. >> >> For reference, do we have any examples as to why any of these prevent >> booting? > Not sure there's any documentation so far except for the comments in > the original kcov patch. I did not look at all boot crashes and hangs. The low level arch code like interrupts and early bootstrap is not interesting in this setting, so I just bisected down to file level and excluded it. I looked at one crash, though. It was related to setup of permanent per-cpu storage, the kcov callback was emitted into a critical sequence of instructions that switches per-cpu storage from bootstrap to the real one, and access to 'current' faulted in that callback. In general, for the boot issue it's better to exclude files lazily as we discover new issues. Besides the boot issues, other files are excluded for two reasons: 1. non-deterministic coverage (like interrupts and mutex slow paths). 2. excessive coverage, for example memcpy-like loop will produce O(N) coverage since kcov is trace-based. I guess that delay.c falls into this category. We don't need 100% deterministic coverage. I agree that it's not feasible. User-space part of syzkaller (kcov-based fuzzer) tries to work around it with some heuristics. But I've tried to to eliminate some frequent and common sources of non-determinism. I've repeatedly collected coverage from a simple program containing mmap-open-read-close, and eliminated all frequent, large spikes of coverage one by one. Re delay.c: on x86 it is not inlined, and some parts are written in C so disable of instrumentation worked. Is it inlined on arm64? I see at least the following in the c file: void __delay(unsigned long cycles) { cycles_t start = get_cycles(); while ((get_cycles() - start) < cycles) cpu_relax(); } ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] arm64: allow building with kcov coverage on ARM64 2016-04-04 17:30 ` Dmitry Vyukov @ 2016-04-12 11:17 ` Alexander Potapenko 2016-04-13 16:12 ` James Morse 2016-04-13 17:01 ` Mark Rutland 0 siblings, 2 replies; 14+ messages in thread From: Alexander Potapenko @ 2016-04-12 11:17 UTC (permalink / raw) To: Dmitry Vyukov Cc: Mark Rutland, Catalin Marinas, Quentin Casasnovas, Will Deacon, Kostya Serebryany, Andrew Morton, syzkaller, LKML, linux-arm-kernel, Ard Biesheuvel, marc.zyngier, Christoffer Dall On Mon, Apr 4, 2016 at 7:30 PM, Dmitry Vyukov <dvyukov@google.com> wrote: > On Thu, Mar 31, 2016 at 7:18 PM, Alexander Potapenko <glider@google.com> wrote: >> On Thu, Mar 31, 2016 at 7:14 PM, Mark Rutland <mark.rutland@arm.com> wrote: >>> On Thu, Mar 31, 2016 at 06:33:24PM +0200, Alexander Potapenko wrote: >>>> On Thu, Mar 31, 2016 at 6:00 PM, Mark Rutland <mark.rutland@arm.com> wrote: >>>> > On Thu, Mar 31, 2016 at 05:09:29PM +0200, Alexander Potapenko wrote: >>>> >> Currently kcov instrumentation is disabled for the following files: >>>> > >>>> >> arch/x86/boot/* >>>> >> arch/x86/boot/compressed/* >>>> >> arch/x86/entry/vdso/* >>>> >> arch/x86/realmode/rm/* >>>> > >>>> > These are executed outside of the usual kernel context / address space, >>>> > so excluding these makes sense to me. >>>> > >>>> >> arch/x86/kernel/* >>>> >> arch/x86/kernel/apic/* >>>> >> arch/x86/kernel/cpu/common.c >>>> >> arch/x86/kernel/cpu/perf_event.c >>>> >> arch/x86/lib/delay.c >>>> >> arch/x86/mm/tlb.c >>>> > >>>> > For these, it's not immediately clear to me why instrumentation is >>>> > disabled, so I don't know whether or not we can instrument the analogous >>>> > arm64 code. >>>> According to the comments in >>>> https://github.com/torvalds/linux/commit/5c9a8750a6409c63a0f01d51a9024861022f6593, >>>> instrumentation of arch/x86/kernel/apic/* and arch/x86/lib/delay.c >>>> leads to non-deterministic coverage, >>> >>> To what extent does determinism matter? Are we just ruling out the worst >>> cases, or is this likely to turn into a whack-a-mole game? >> I guess we'd better ask Dmitry who excluded these files on x86 and >> experimented with coverage a lot. >> Dmitry, can you clarify this, please? >>> Do we exclude clocksources and other driver code? >>> >>> Looking at the arm64 delay timer code, it looks like everything will be >>> inlined (and therefore coverage should be deterministic so long as the >>> delay functions are called deterministically). That said, the same looks >>> basically true of the x86 code, so I guess I've misunderstood. >>> >>>> instrumenting others prevent the kernel from booting. >>> >>> I haven't been able to come up with a scenario whereby kcov would be >>> fatal for the above, so it's difficult to say if we have equivalent >>> problems. >>> >>> For reference, do we have any examples as to why any of these prevent >>> booting? >> Not sure there's any documentation so far except for the comments in >> the original kcov patch. > > > I did not look at all boot crashes and hangs. The low level arch code > like interrupts and early bootstrap is not interesting in this > setting, so I just bisected down to file level and excluded it. I > looked at one crash, though. It was related to setup of permanent > per-cpu storage, the kcov callback was emitted into a critical > sequence of instructions that switches per-cpu storage from bootstrap > to the real one, and access to 'current' faulted in that callback. In > general, for the boot issue it's better to exclude files lazily as we > discover new issues. > > Besides the boot issues, other files are excluded for two reasons: > 1. non-deterministic coverage (like interrupts and mutex slow paths). > 2. excessive coverage, for example memcpy-like loop will produce O(N) > coverage since kcov is trace-based. I guess that delay.c falls into > this category. > > We don't need 100% deterministic coverage. I agree that it's not > feasible. User-space part of syzkaller (kcov-based fuzzer) tries to > work around it with some heuristics. But I've tried to to eliminate > some frequent and common sources of non-determinism. I've repeatedly > collected coverage from a simple program containing > mmap-open-read-close, and eliminated all frequent, large spikes of > coverage one by one. > > Re delay.c: on x86 it is not inlined, and some parts are written in C > so disable of instrumentation worked. Is it inlined on arm64? I see at > least the following in the c file: > > void __delay(unsigned long cycles) > { > cycles_t start = get_cycles(); > > while ((get_cycles() - start) < cycles) > cpu_relax(); > } Mark, Looks like we haven't reached the consensus on this topic yet. Do you have anything to comment on what Dmitry said? I also wonder if we can, say, land the change to arch/arm64/Kconfig separately from makefile changes that improve the precision or fix certain build configurations. Alex -- Alexander Potapenko Software Engineer Google Germany GmbH Erika-Mann-Straße, 33 80636 München Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] arm64: allow building with kcov coverage on ARM64 2016-04-12 11:17 ` Alexander Potapenko @ 2016-04-13 16:12 ` James Morse 2016-04-13 16:35 ` Alexander Potapenko 2016-04-13 17:01 ` Mark Rutland 1 sibling, 1 reply; 14+ messages in thread From: James Morse @ 2016-04-13 16:12 UTC (permalink / raw) To: Alexander Potapenko Cc: Dmitry Vyukov, Mark Rutland, Catalin Marinas, Quentin Casasnovas, Will Deacon, Kostya Serebryany, Andrew Morton, syzkaller, LKML, linux-arm-kernel, Ard Biesheuvel, marc.zyngier, Christoffer Dall Hi Alex, On 12/04/16 12:17, Alexander Potapenko wrote: > I also wonder if we can, say, land the change to arch/arm64/Kconfig > separately from makefile changes that improve the precision or fix > certain build configurations. (I'm not sure what you mean by precision) It depends which build configurations get broken, for example the first build I tried doesn't boot. I tested the Kconfig change, and added 'KCOV_INSTRUMENT := n' to kvm's two Makefiles[0]. defconfig+KCOV boots fine, and I can start a guest, but if I build with defconfig+KCOV+STACK_TRACER, I get a kernel that fails to boot. It boils down to a loop between ftrace and kcov, I will send a patch. It looks like syzkaller is the only user of this data, and it doesn't appear to support arm64: > 2016/04/13 15:42:55 failed to create instance: qemu stopped: > "kvm" accelerator not found. > No accelerator found! This happens because syzkaller expects to be able to use 'qemu-system-x86_64': > [pid 3670] execve("/usr/bin/qemu-system-x86_64", ["qemu-system-x86_64", > "-hda", "/foo.img", "-snapshot", "-m", "1024", "-net", "nic", "-net", > "user,host=10.0.2.10,hostfwd=tcp:"..., "-nographic", "-enable-kvm", "-numa", > "node,nodeid=0,cpus=0-1", "-numa", "node,nodeid=1,cpus=2-3", ...], qemu-system-x86_64 is installed and works fine, it just doesn't have the expected hardware acceleration... My approximation of your qemu command for arm64 may be useful[1], but I'm not able to hack the go source to fix it! Thanks, James [0] diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile index 122cff482ac4..7d111f06bbf3 100644 --- a/arch/arm64/kvm/Makefile +++ b/arch/arm64/kvm/Makefile @@ -9,6 +9,10 @@ CFLAGS_mmu.o := -I. KVM=../../../virt/kvm ARM=../../../arch/arm/kvm +# Code built here may run at EL2, in which case __sanitizer_cov_trace_pc() will +# not be callable. For now, disable the instrumentation. +KCOV_INSTRUMENT := n + obj-$(CONFIG_KVM_ARM_HOST) += kvm.o obj-$(CONFIG_KVM_ARM_HOST) += hyp/ diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile index 778d0effa2af..1150f8664c85 100644 --- a/arch/arm64/kvm/hyp/Makefile +++ b/arch/arm64/kvm/hyp/Makefile @@ -20,3 +20,4 @@ obj-$(CONFIG_KVM_ARM_HOST) += s2-setup.o GCOV_PROFILE := n KASAN_SANITIZE := n UBSAN_SANITIZE := n +KCOV_INSTRUMENT := n [1] qemu-system-aarch64 -enable-kvm -cpu host -m 1024 -M virt -nographic -kernel ./syzkaller/Image -append "console=ttyAMA0,115200 root=/dev/vda" -drive format=raw,file=/foo.img,media=disk,if=none,cache=writeback,id=root -device virtio-blk-device,drive=root -netdev user,host=10.0.2.10,hostfwd=tcp::23505-:22,id=unet -device virtio-net-device,netdev=unet ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v1] arm64: allow building with kcov coverage on ARM64 2016-04-13 16:12 ` James Morse @ 2016-04-13 16:35 ` Alexander Potapenko 0 siblings, 0 replies; 14+ messages in thread From: Alexander Potapenko @ 2016-04-13 16:35 UTC (permalink / raw) To: James Morse Cc: Dmitry Vyukov, Mark Rutland, Catalin Marinas, Quentin Casasnovas, Will Deacon, Kostya Serebryany, Andrew Morton, syzkaller, LKML, linux-arm-kernel, Ard Biesheuvel, marc.zyngier, Christoffer Dall Hi James, On Wed, Apr 13, 2016 at 6:12 PM, James Morse <james.morse@arm.com> wrote: > Hi Alex, > > On 12/04/16 12:17, Alexander Potapenko wrote: >> I also wonder if we can, say, land the change to arch/arm64/Kconfig >> separately from makefile changes that improve the precision or fix >> certain build configurations. > > (I'm not sure what you mean by precision) > > It depends which build configurations get broken, for example the first build I > tried doesn't boot. > > I tested the Kconfig change, and added 'KCOV_INSTRUMENT := n' to kvm's two > Makefiles[0]. > > defconfig+KCOV boots fine, and I can start a guest, but if I build with > defconfig+KCOV+STACK_TRACER, I get a kernel that fails to boot. It boils down to > a loop between ftrace and kcov, I will send a patch. > > > It looks like syzkaller is the only user of this data, and it doesn't appear to > support arm64: >> 2016/04/13 15:42:55 failed to create instance: qemu stopped: >> "kvm" accelerator not found. >> No accelerator found! syzcaller is able to work with bare-metal Android devices using ADB, but those are usually running ancient kernels. Looks like It's time for me to start cross-compiling and testing an arm64 kernel on a QEMU, so I'll try that out and update the patch. > This happens because syzkaller expects to be able to use 'qemu-system-x86_64': >> [pid 3670] execve("/usr/bin/qemu-system-x86_64", ["qemu-system-x86_64", >> "-hda", "/foo.img", "-snapshot", "-m", "1024", "-net", "nic", "-net", >> "user,host=10.0.2.10,hostfwd=tcp:"..., "-nographic", "-enable-kvm", "-numa", >> "node,nodeid=0,cpus=0-1", "-numa", "node,nodeid=1,cpus=2-3", ...], > > qemu-system-x86_64 is installed and works fine, it just doesn't have the > expected hardware acceleration... > > My approximation of your qemu command for arm64 may be useful[1], but I'm not > able to hack the go source to fix it! > > > > Thanks, > > James > > > > [0] > diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile > index 122cff482ac4..7d111f06bbf3 100644 > --- a/arch/arm64/kvm/Makefile > +++ b/arch/arm64/kvm/Makefile > @@ -9,6 +9,10 @@ CFLAGS_mmu.o := -I. > KVM=../../../virt/kvm > ARM=../../../arch/arm/kvm > > +# Code built here may run at EL2, in which case __sanitizer_cov_trace_pc() will > +# not be callable. For now, disable the instrumentation. > +KCOV_INSTRUMENT := n > + > obj-$(CONFIG_KVM_ARM_HOST) += kvm.o > obj-$(CONFIG_KVM_ARM_HOST) += hyp/ > > diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile > index 778d0effa2af..1150f8664c85 100644 > --- a/arch/arm64/kvm/hyp/Makefile > +++ b/arch/arm64/kvm/hyp/Makefile > @@ -20,3 +20,4 @@ obj-$(CONFIG_KVM_ARM_HOST) += s2-setup.o > GCOV_PROFILE := n > KASAN_SANITIZE := n > UBSAN_SANITIZE := n > +KCOV_INSTRUMENT := n > > > [1] > qemu-system-aarch64 -enable-kvm > -cpu host -m 1024 -M virt -nographic > -kernel ./syzkaller/Image > -append "console=ttyAMA0,115200 root=/dev/vda" > -drive format=raw,file=/foo.img,media=disk,if=none,cache=writeback,id=root > -device virtio-blk-device,drive=root > -netdev user,host=10.0.2.10,hostfwd=tcp::23505-:22,id=unet > -device virtio-net-device,netdev=unet > > -- Alexander Potapenko Software Engineer Google Germany GmbH Erika-Mann-Straße, 33 80636 München Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] arm64: allow building with kcov coverage on ARM64 2016-04-12 11:17 ` Alexander Potapenko 2016-04-13 16:12 ` James Morse @ 2016-04-13 17:01 ` Mark Rutland 1 sibling, 0 replies; 14+ messages in thread From: Mark Rutland @ 2016-04-13 17:01 UTC (permalink / raw) To: Alexander Potapenko Cc: Dmitry Vyukov, Catalin Marinas, Quentin Casasnovas, Will Deacon, Kostya Serebryany, Andrew Morton, syzkaller, LKML, linux-arm-kernel, Ard Biesheuvel, marc.zyngier, Christoffer Dall, james.morse On Tue, Apr 12, 2016 at 01:17:00PM +0200, Alexander Potapenko wrote: > On Mon, Apr 4, 2016 at 7:30 PM, Dmitry Vyukov <dvyukov@google.com> wrote: > > I did not look at all boot crashes and hangs. The low level arch code > > like interrupts and early bootstrap is not interesting in this > > setting, so I just bisected down to file level and excluded it. I > > looked at one crash, though. It was related to setup of permanent > > per-cpu storage, the kcov callback was emitted into a critical > > sequence of instructions that switches per-cpu storage from bootstrap > > to the real one, and access to 'current' faulted in that callback. In > > general, for the boot issue it's better to exclude files lazily as we > > discover new issues. > > > > Besides the boot issues, other files are excluded for two reasons: > > 1. non-deterministic coverage (like interrupts and mutex slow paths). > > 2. excessive coverage, for example memcpy-like loop will produce O(N) > > coverage since kcov is trace-based. I guess that delay.c falls into > > this category. > > > > We don't need 100% deterministic coverage. I agree that it's not > > feasible. User-space part of syzkaller (kcov-based fuzzer) tries to > > work around it with some heuristics. But I've tried to to eliminate > > some frequent and common sources of non-determinism. I've repeatedly > > collected coverage from a simple program containing > > mmap-open-read-close, and eliminated all frequent, large spikes of > > coverage one by one. > > > > Re delay.c: on x86 it is not inlined, and some parts are written in C > > so disable of instrumentation worked. Is it inlined on arm64? I see at > > least the following in the c file: > > > > void __delay(unsigned long cycles) > > { > > cycles_t start = get_cycles(); > > > > while ((get_cycles() - start) < cycles) > > cpu_relax(); > > } > > Mark, > > Looks like we haven't reached the consensus on this topic yet. > Do you have anything to comment on what Dmitry said? I'm still concerned that we only seem to have a coarse understanding of the issues, but I guess that cannot be helped. I'd like to make sure that if there's anything we must inhibit the coverage of for arm64, we have a good, documented (comment or commit message) understanding of why. That allows us to re-evaluate the situation as code changes. Given we don't have much fine-grained knowledge of that sort from x86, it looks like we have to figure that out from scratch. As for deterministic coverage, I guess we have to see what happens and make judgements on a case-by-case basis. > I also wonder if we can, say, land the change to arch/arm64/Kconfig > separately from makefile changes that improve the precision or fix > certain build configurations. I assume that 'precision' here means 'determinism'. I mostly agree with that, though I would like to see the feature working from the point it's merged. i.e. any known boot/runtime failures should be solved now, and as above, we should somehow document why each change is necessary. Changes relating to determinism are a bit different, and should be evaluated separately/subsequently. We may want to annotate those differently, as there may be cases where non-deterministic coverage data is useful (e.g. for something other than syzkaller). Thanks, Mark. ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2016-04-13 17:01 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-03-31 13:54 [PATCH v1] arm64: allow building with kcov coverage on ARM64 Alexander Potapenko 2016-03-31 14:02 ` Alexander Potapenko 2016-03-31 14:29 ` Mark Rutland 2016-03-31 15:09 ` Alexander Potapenko 2016-03-31 16:00 ` Mark Rutland 2016-03-31 16:33 ` Alexander Potapenko 2016-03-31 16:43 ` Alexander Potapenko 2016-03-31 17:14 ` Mark Rutland 2016-03-31 17:18 ` Alexander Potapenko 2016-04-04 17:30 ` Dmitry Vyukov 2016-04-12 11:17 ` Alexander Potapenko 2016-04-13 16:12 ` James Morse 2016-04-13 16:35 ` Alexander Potapenko 2016-04-13 17:01 ` Mark Rutland
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).