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

Groups > linux.kernel > #1610518 > unrolled thread

Re: [PATCH] lkdtm: turn off kcov for lkdtm_rodata_do_nothing:

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] lkdtm: turn off kcov for lkdtm_rodata_do_nothing: Dmitry Vyukov <dvyukov@google.com> - 2017-03-28 12:10 +0200
    Re: [PATCH] lkdtm: turn off kcov for lkdtm_rodata_do_nothing: Mark Rutland <mark.rutland@arm.com> - 2017-03-28 12:40 +0200
      Re: [PATCH] lkdtm: turn off kcov for lkdtm_rodata_do_nothing: Arnd Bergmann <arnd@arndb.de> - 2017-03-28 15:50 +0200
    Re: [PATCH] lkdtm: turn off kcov for lkdtm_rodata_do_nothing: Kees Cook <keescook@chromium.org> - 2017-03-28 19:00 +0200
      Re: [PATCH] lkdtm: turn off kcov for lkdtm_rodata_do_nothing: Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-03-28 19:20 +0200

#1610518 — Re: [PATCH] lkdtm: turn off kcov for lkdtm_rodata_do_nothing:

On Tue, Mar 28, 2017 at 11:57 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> I ran into a link error on ARM64 for lkdtm_rodata_do_nothing:
>
> drivers/misc/built-in.o: In function `lkdtm_rodata_do_nothing':
> :(.rodata+0x68c8): relocation truncated to fit: R_AARCH64_CALL26 against symbol `__sanitizer_cov_trace_pc' defined in .text section in kernel/built-in.o
>
> I did not analyze this further, but my theory is that we would need a trampoline
> to call __sanitizer_cov_trace_pc(), but the linker (correctly) only adds trampolines
> for callers in executable sections.
>
> Disabling KCOV for this one file avoids the build failure with no
> other practical downsides I can think of.
>
> The problem can only happen on kernels that contain both kcov and
> lkdtm, so if we want to backport this, it should be in the earliest
> version that has both (v4.8).
>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: Kees Cook <keescook@chromium.org>
> Fixes: 5c9a8750a640 ("kernel: add kcov code coverage")
> Fixes: 9a49a528dcf3 ("lkdtm: add function for testing .rodata section")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/misc/Makefile | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 4925ea8e1952..7a321047bfbe 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -63,6 +63,8 @@ lkdtm-$(CONFIG_LKDTM)         += lkdtm_perms.o
>  lkdtm-$(CONFIG_LKDTM)          += lkdtm_rodata_objcopy.o
>  lkdtm-$(CONFIG_LKDTM)          += lkdtm_usercopy.o
>
> +KCOV_INSTRUMENT_lkdtm_rodata.o := n
> +
>  OBJCOPYFLAGS :=
>  OBJCOPYFLAGS_lkdtm_rodata_objcopy.o := \
>                         --set-section-flags .text=alloc,readonly \
> --
> 2.9.0


Acked-by: Dmitry Vyukov <dvyukov@google.com>

[toc] | [next] | [standalone]

#1610550

Hi,

On Tue, Mar 28, 2017 at 12:00:15PM +0200, Dmitry Vyukov wrote:
> On Tue, Mar 28, 2017 at 11:57 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > I ran into a link error on ARM64 for lkdtm_rodata_do_nothing:
> >
> > drivers/misc/built-in.o: In function `lkdtm_rodata_do_nothing':
> > :(.rodata+0x68c8): relocation truncated to fit: R_AARCH64_CALL26 against symbol `__sanitizer_cov_trace_pc' defined in .text section in kernel/built-in.o
> >
> > I did not analyze this further, but my theory is that we would need a trampoline
> > to call __sanitizer_cov_trace_pc(), but the linker (correctly) only adds trampolines
> > for callers in executable sections.

For reference, is this a "large" config, e.g. allyesconfig?

I'm aware that at least as recently as GCC 6 there were issues with
veneer generation for calls across sections (which I personally saw with
calls from .init.text to .text) when the kernel was sufficiently large.

FWIW, I have no problem building a v4.11-rc3 kernel with both KCOV and
LKDTM using the Linaro 15.08 aarch64-linux-gnu GCC 5 toolchain. Which
toolchain are you using?

No strong feelings on this patch, but it may be indicative of a larger
problem, and we probably don't want to play whack-a-mole to fix
relocation truncation more generally.

Thanks,
Mark.

> > Disabling KCOV for this one file avoids the build failure with no
> > other practical downsides I can think of.
> >
> > The problem can only happen on kernels that contain both kcov and
> > lkdtm, so if we want to backport this, it should be in the earliest
> > version that has both (v4.8).
> >
> > Cc: Dmitry Vyukov <dvyukov@google.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Fixes: 5c9a8750a640 ("kernel: add kcov code coverage")
> > Fixes: 9a49a528dcf3 ("lkdtm: add function for testing .rodata section")
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > ---
> >  drivers/misc/Makefile | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> > index 4925ea8e1952..7a321047bfbe 100644
> > --- a/drivers/misc/Makefile
> > +++ b/drivers/misc/Makefile
> > @@ -63,6 +63,8 @@ lkdtm-$(CONFIG_LKDTM)         += lkdtm_perms.o
> >  lkdtm-$(CONFIG_LKDTM)          += lkdtm_rodata_objcopy.o
> >  lkdtm-$(CONFIG_LKDTM)          += lkdtm_usercopy.o
> >
> > +KCOV_INSTRUMENT_lkdtm_rodata.o := n
> > +
> >  OBJCOPYFLAGS :=
> >  OBJCOPYFLAGS_lkdtm_rodata_objcopy.o := \
> >                         --set-section-flags .text=alloc,readonly \
> > --
> > 2.9.0
> 
> 
> Acked-by: Dmitry Vyukov <dvyukov@google.com>

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

#1610931

On Tue, Mar 28, 2017 at 12:22 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi,
>
> On Tue, Mar 28, 2017 at 12:00:15PM +0200, Dmitry Vyukov wrote:
>> On Tue, Mar 28, 2017 at 11:57 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > I ran into a link error on ARM64 for lkdtm_rodata_do_nothing:
>> >
>> > drivers/misc/built-in.o: In function `lkdtm_rodata_do_nothing':
>> > :(.rodata+0x68c8): relocation truncated to fit: R_AARCH64_CALL26 against symbol `__sanitizer_cov_trace_pc' defined in .text section in kernel/built-in.o
>> >
>> > I did not analyze this further, but my theory is that we would need a trampoline
>> > to call __sanitizer_cov_trace_pc(), but the linker (correctly) only adds trampolines
>> > for callers in executable sections.
>
> For reference, is this a "large" config, e.g. allyesconfig?

It's a randconfig build, and I'm guessing that it's large, but I have
not checked
the size of the sections. This was the first time I ran into it after months of
building randconfig kernels without anything having triggered it recently.

> I'm aware that at least as recently as GCC 6 there were issues with
> veneer generation for calls across sections (which I personally saw with
> calls from .init.text to .text) when the kernel was sufficiently large.

We have had problems with veneers no arm32 in the past, and ld definitely
doesn't create them for calls from inside of non-executable sections, so
the typical fix was to put all code that needs veneers into executable sections.

Clearly that cannot work for this particular case.

> FWIW, I have no problem building a v4.11-rc3 kernel with both KCOV and
> LKDTM using the Linaro 15.08 aarch64-linux-gnu GCC 5 toolchain. Which
> toolchain are you using?

This is a gcc-7.0.1 snapshot from last week, and a somewhat older
binutils: GNU ld (GNU Binutils) 2.27.51.20161215

> No strong feelings on this patch, but it may be indicative of a larger
> problem, and we probably don't want to play whack-a-mole to fix
> relocation truncation more generally.

I have not seen this problem in allyesconfig or any other randconfig build,
so it's certainly likely that some other setting must be there to trigger this.

I've recreated the problem using an allyesconfig build with this change
to turn off -mcmodel=large:

@@ -283,7 +283,6 @@
 CONFIG_HAVE_IRQ_TIME_ACCOUNTING=y
 CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE=y
 CONFIG_HAVE_ARCH_HUGE_VMAP=y
-CONFIG_HAVE_MOD_ARCH_SPECIFIC=y
 CONFIG_MODULES_USE_ELF_RELA=y
 CONFIG_ARCH_HAS_ELF_RANDOMIZE=y
 CONFIG_HAVE_ARCH_MMAP_RND_BITS=y
@@ -529,7 +528,7 @@
 CONFIG_ARM64_ERRATUM_832075=y
 CONFIG_ARM64_ERRATUM_834220=y
 CONFIG_ARM64_ERRATUM_845719=y
-CONFIG_ARM64_ERRATUM_843419=y
+# CONFIG_ARM64_ERRATUM_843419 is not set
 CONFIG_CAVIUM_ERRATUM_22375=y
 CONFIG_CAVIUM_ERRATUM_23144=y
 CONFIG_CAVIUM_ERRATUM_23154=y
@@ -643,10 +642,7 @@
 # ARMv8.2 architectural features
 #
 CONFIG_ARM64_UAO=y
-CONFIG_ARM64_MODULE_CMODEL_LARGE=y
-CONFIG_ARM64_MODULE_PLTS=y
-CONFIG_RELOCATABLE=y
-CONFIG_RANDOMIZE_BASE=y
+# CONFIG_RANDOMIZE_BASE is not set

 #
 # Boot options

     Arnd

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

#1611222

On Tue, Mar 28, 2017 at 3:00 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Tue, Mar 28, 2017 at 11:57 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> I ran into a link error on ARM64 for lkdtm_rodata_do_nothing:
>>
>> drivers/misc/built-in.o: In function `lkdtm_rodata_do_nothing':
>> :(.rodata+0x68c8): relocation truncated to fit: R_AARCH64_CALL26 against symbol `__sanitizer_cov_trace_pc' defined in .text section in kernel/built-in.o
>>
>> I did not analyze this further, but my theory is that we would need a trampoline
>> to call __sanitizer_cov_trace_pc(), but the linker (correctly) only adds trampolines
>> for callers in executable sections.

Yup, this function should be inherently uncallable (which is what
lkdtm is testing), so it makes sense to turn off kcov for it.

>>
>> Disabling KCOV for this one file avoids the build failure with no
>> other practical downsides I can think of.
>>
>> The problem can only happen on kernels that contain both kcov and
>> lkdtm, so if we want to backport this, it should be in the earliest
>> version that has both (v4.8).
>>
>> Cc: Dmitry Vyukov <dvyukov@google.com>
>> Cc: Kees Cook <keescook@chromium.org>
>> Fixes: 5c9a8750a640 ("kernel: add kcov code coverage")
>> Fixes: 9a49a528dcf3 ("lkdtm: add function for testing .rodata section")
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>>  drivers/misc/Makefile | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
>> index 4925ea8e1952..7a321047bfbe 100644
>> --- a/drivers/misc/Makefile
>> +++ b/drivers/misc/Makefile
>> @@ -63,6 +63,8 @@ lkdtm-$(CONFIG_LKDTM)         += lkdtm_perms.o
>>  lkdtm-$(CONFIG_LKDTM)          += lkdtm_rodata_objcopy.o
>>  lkdtm-$(CONFIG_LKDTM)          += lkdtm_usercopy.o
>>
>> +KCOV_INSTRUMENT_lkdtm_rodata.o := n
>> +
>>  OBJCOPYFLAGS :=
>>  OBJCOPYFLAGS_lkdtm_rodata_objcopy.o := \
>>                         --set-section-flags .text=alloc,readonly \
>> --
>> 2.9.0
>
>
> Acked-by: Dmitry Vyukov <dvyukov@google.com>

Acked-by: Kees Cook <keescook@chromium.org>

Greg, can you please add this to the drivers/misc tree when you get a chance?

Thanks!

-Kees

-- 
Kees Cook
Pixel Security

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

#1611231

On Tue, Mar 28, 2017 at 09:57:34AM -0700, Kees Cook wrote:
> On Tue, Mar 28, 2017 at 3:00 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
> > On Tue, Mar 28, 2017 at 11:57 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> >> I ran into a link error on ARM64 for lkdtm_rodata_do_nothing:
> >>
> >> drivers/misc/built-in.o: In function `lkdtm_rodata_do_nothing':
> >> :(.rodata+0x68c8): relocation truncated to fit: R_AARCH64_CALL26 against symbol `__sanitizer_cov_trace_pc' defined in .text section in kernel/built-in.o
> >>
> >> I did not analyze this further, but my theory is that we would need a trampoline
> >> to call __sanitizer_cov_trace_pc(), but the linker (correctly) only adds trampolines
> >> for callers in executable sections.
> 
> Yup, this function should be inherently uncallable (which is what
> lkdtm is testing), so it makes sense to turn off kcov for it.
> 
> >>
> >> Disabling KCOV for this one file avoids the build failure with no
> >> other practical downsides I can think of.
> >>
> >> The problem can only happen on kernels that contain both kcov and
> >> lkdtm, so if we want to backport this, it should be in the earliest
> >> version that has both (v4.8).
> >>
> >> Cc: Dmitry Vyukov <dvyukov@google.com>
> >> Cc: Kees Cook <keescook@chromium.org>
> >> Fixes: 5c9a8750a640 ("kernel: add kcov code coverage")
> >> Fixes: 9a49a528dcf3 ("lkdtm: add function for testing .rodata section")
> >> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> >> ---
> >>  drivers/misc/Makefile | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> >> index 4925ea8e1952..7a321047bfbe 100644
> >> --- a/drivers/misc/Makefile
> >> +++ b/drivers/misc/Makefile
> >> @@ -63,6 +63,8 @@ lkdtm-$(CONFIG_LKDTM)         += lkdtm_perms.o
> >>  lkdtm-$(CONFIG_LKDTM)          += lkdtm_rodata_objcopy.o
> >>  lkdtm-$(CONFIG_LKDTM)          += lkdtm_usercopy.o
> >>
> >> +KCOV_INSTRUMENT_lkdtm_rodata.o := n
> >> +
> >>  OBJCOPYFLAGS :=
> >>  OBJCOPYFLAGS_lkdtm_rodata_objcopy.o := \
> >>                         --set-section-flags .text=alloc,readonly \
> >> --
> >> 2.9.0
> >
> >
> > Acked-by: Dmitry Vyukov <dvyukov@google.com>
> 
> Acked-by: Kees Cook <keescook@chromium.org>
> 
> Greg, can you please add this to the drivers/misc tree when you get a chance?

Will do.

thanks,

greg k-h

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web