Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1370801 > unrolled thread

Re: [PATCH v1] arm64: allow building with kcov coverage on ARM64

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v1] arm64: allow building with kcov coverage on ARM64 Dmitry Vyukov <dvyukov@google.com> - 2016-04-04 19:40 +0200
    Re: [PATCH v1] arm64: allow building with kcov coverage on ARM64 Alexander Potapenko <glider@google.com> - 2016-04-12 13:20 +0200
      Re: [PATCH v1] arm64: allow building with kcov coverage on ARM64 James Morse <james.morse@arm.com> - 2016-04-13 18:20 +0200
        Re: [PATCH v1] arm64: allow building with kcov coverage on ARM64 Alexander Potapenko <glider@google.com> - 2016-04-13 18:40 +0200
      Re: [PATCH v1] arm64: allow building with kcov coverage on ARM64 Mark Rutland <mark.rutland@arm.com> - 2016-04-13 19:10 +0200

#1370801 — Re: [PATCH v1] arm64: allow building with kcov coverage on ARM64

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();
}

[toc] | [next] | [standalone]

#1376712

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

[toc] | [prev] | [next] | [standalone]

#1378087

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

[toc] | [prev] | [next] | [standalone]

#1378099

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

[toc] | [prev] | [next] | [standalone]

#1378103

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web