[PATCH 1/2] powerpc/64s: Work around spurious warning on old gccs with -fsanitize-coverage

Some older gccs (<GCC 7), when invoked with -fsanitize-coverage=trace-pc,
cause a spurious uninitialised variable warning in dt_cpu_ftrs.c:

  arch/powerpc/kernel/dt_cpu_ftrs.c: In function ‘cpufeatures_process_feature’:
  arch/powerpc/kernel/dt_cpu_ftrs.c:686:7: warning: ‘m’ may be used uninitialized in this function [-Wmaybe-uninitialized]
    if (m->cpu_ftr_bit_mask)

An upcoming patch will enable support for kcov, which requires
-fsanitize-coverage=trace-pc.

Work around this by explicitly initialising m to NULL.

Signed-off-by: Andrew Donnellan <redacted>
---
 arch/powerpc/kernel/dt_cpu_ftrs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
index 8be3721d9302..2192b2114513 100644
--- a/arch/powerpc/kernel/dt_cpu_ftrs.c
+++ b/arch/powerpc/kernel/dt_cpu_ftrs.c

@@ -658,7 +658,7 @@ static void __init cpufeatures_setup_start(u32 isa)
 
 static bool __init cpufeatures_process_feature(struct dt_cpu_feature *f)
 {
-	const struct dt_cpu_feature_match *m;
+	const struct dt_cpu_feature_match *m = NULL;
 	bool known = false;
 	int i;
 

-- 
2.11.0

kcov provides kernel coverage data that's useful for fuzzing tools like
syzkaller.

Wire up kcov support on powerpc. Disable kcov instrumentation on the same
files where we currently disable gcov and UBSan instrumentation.

Signed-off-by: Andrew Donnellan <redacted>
Acked-by: Dmitry Vyukov <dvyukov@google.com>
---
 arch/powerpc/Kconfig                | 1 +
 arch/powerpc/kernel/Makefile        | 7 ++++++-
 arch/powerpc/kernel/trace/Makefile  | 3 ++-
 arch/powerpc/kernel/vdso32/Makefile | 1 +
 arch/powerpc/kernel/vdso64/Makefile | 1 +
 arch/powerpc/xmon/Makefile          | 1 +
 6 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 2890d36eb531..d3698dae0e60 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig

@@ -134,6 +134,7 @@ config PPC
 	select ARCH_HAS_ELF_RANDOMIZE
 	select ARCH_HAS_FORTIFY_SOURCE
 	select ARCH_HAS_GCOV_PROFILE_ALL
+	select ARCH_HAS_KCOV
 	select ARCH_HAS_PHYS_TO_DMA
 	select ARCH_HAS_PMEM_API                if PPC64
 	select ARCH_HAS_PTE_SPECIAL

diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index cb7f0bb9ee71..961f44eabb65 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile

@@ -142,16 +142,21 @@ endif
 obj-$(CONFIG_EPAPR_PARAVIRT)	+= epapr_paravirt.o epapr_hcalls.o
 obj-$(CONFIG_KVM_GUEST)		+= kvm.o kvm_emul.o
 
-# Disable GCOV & sanitizers in odd or sensitive code
+# Disable GCOV, KCOV & sanitizers in odd or sensitive code
 GCOV_PROFILE_prom_init.o := n
+KCOV_INSTRUMENT_prom_init.o := n
 UBSAN_SANITIZE_prom_init.o := n
 GCOV_PROFILE_machine_kexec_64.o := n
+KCOV_INSTRUMENT_machine_kexec_64.o := n
 UBSAN_SANITIZE_machine_kexec_64.o := n
 GCOV_PROFILE_machine_kexec_32.o := n
+KCOV_INSTRUMENT_machine_kexec_32.o := n
 UBSAN_SANITIZE_machine_kexec_32.o := n
 GCOV_PROFILE_kprobes.o := n
+KCOV_INSTRUMENT_kprobes.o := n
 UBSAN_SANITIZE_kprobes.o := n
 GCOV_PROFILE_kprobes-ftrace.o := n
+KCOV_INSTRUMENT_kprobes-ftrace.o := n
 UBSAN_SANITIZE_kprobes-ftrace.o := n
 UBSAN_SANITIZE_vdso.o := n
 

diff --git a/arch/powerpc/kernel/trace/Makefile b/arch/powerpc/kernel/trace/Makefile
index b1725ad3e13d..858503775c58 100644
--- a/arch/powerpc/kernel/trace/Makefile
+++ b/arch/powerpc/kernel/trace/Makefile

@@ -23,6 +23,7 @@ obj-$(CONFIG_TRACING)			+= trace_clock.o
 obj-$(CONFIG_PPC64)			+= $(obj64-y)
 obj-$(CONFIG_PPC32)			+= $(obj32-y)
 
-# Disable GCOV & sanitizers in odd or sensitive code
+# Disable GCOV, KCOV & sanitizers in odd or sensitive code
 GCOV_PROFILE_ftrace.o := n
+KCOV_INSTRUMENT_ftrace.o := n
 UBSAN_SANITIZE_ftrace.o := n

diff --git a/arch/powerpc/kernel/vdso32/Makefile b/arch/powerpc/kernel/vdso32/Makefile
index 50112d4473bb..ce199f6e4256 100644
--- a/arch/powerpc/kernel/vdso32/Makefile
+++ b/arch/powerpc/kernel/vdso32/Makefile

@@ -23,6 +23,7 @@ targets := $(obj-vdso32) vdso32.so vdso32.so.dbg
 obj-vdso32 := $(addprefix $(obj)/, $(obj-vdso32))
 
 GCOV_PROFILE := n
+KCOV_INSTRUMENT := n
 UBSAN_SANITIZE := n
 
 ccflags-y := -shared -fno-common -fno-builtin

diff --git a/arch/powerpc/kernel/vdso64/Makefile b/arch/powerpc/kernel/vdso64/Makefile
index 69cecb346269..28e7d112aa2f 100644
--- a/arch/powerpc/kernel/vdso64/Makefile
+++ b/arch/powerpc/kernel/vdso64/Makefile

@@ -9,6 +9,7 @@ targets := $(obj-vdso64) vdso64.so vdso64.so.dbg
 obj-vdso64 := $(addprefix $(obj)/, $(obj-vdso64))
 
 GCOV_PROFILE := n
+KCOV_INSTRUMENT := n
 UBSAN_SANITIZE := n
 
 ccflags-y := -shared -fno-common -fno-builtin

diff --git a/arch/powerpc/xmon/Makefile b/arch/powerpc/xmon/Makefile
index 878f9c1d3615..3050f9323254 100644
--- a/arch/powerpc/xmon/Makefile
+++ b/arch/powerpc/xmon/Makefile

@@ -5,6 +5,7 @@
 subdir-ccflags-y := $(call cc-disable-warning, builtin-requires-header)
 
 GCOV_PROFILE := n
+KCOV_INSTRUMENT := n
 UBSAN_SANITIZE := n
 
 # Disable ftrace for the entire directory

-- 
2.11.0

On Thu, Feb 07, 2019 at 04:33:23PM +1100, Andrew Donnellan wrote:

Some older gccs (<GCC 7), when invoked with -fsanitize-coverage=trace-pc,
cause a spurious uninitialised variable warning in dt_cpu_ftrs.c:

  arch/powerpc/kernel/dt_cpu_ftrs.c: In function ‘cpufeatures_process_feature’:
  arch/powerpc/kernel/dt_cpu_ftrs.c:686:7: warning: ‘m’ may be used uninitialized in this function [-Wmaybe-uninitialized]
    if (m->cpu_ftr_bit_mask)

It seems to me the warning is correct?  If enable_unknown is false and no
cpu_feature is found, it will in

	if (m->cpu_ftr_bit_mask)
		cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;

enable random features (whatever was last in the table), or indeed access
via NULL if the table is length 0?  So maybe this should be

	if (known && m->cpu_ftr_bit_mask)
		cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;

instead?  (The code would be much clearer if all the known vs. !known
codepath was fully separated here).


Segher

On 7/2/19 5:37 pm, Segher Boessenkool wrote:

On Thu, Feb 07, 2019 at 04:33:23PM +1100, Andrew Donnellan wrote:

quoted

Some older gccs (<GCC 7), when invoked with -fsanitize-coverage=trace-pc,
cause a spurious uninitialised variable warning in dt_cpu_ftrs.c:

   arch/powerpc/kernel/dt_cpu_ftrs.c: In function ‘cpufeatures_process_feature’:
   arch/powerpc/kernel/dt_cpu_ftrs.c:686:7: warning: ‘m’ may be used uninitialized in this function [-Wmaybe-uninitialized]
     if (m->cpu_ftr_bit_mask)

It seems to me the warning is correct?  If enable_unknown is false and no
cpu_feature is found, it will in

	if (m->cpu_ftr_bit_mask)
		cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;

enable random features (whatever was last in the table), or indeed access
via NULL if the table is length 0?  So maybe this should be

	if (known && m->cpu_ftr_bit_mask)
		cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;

instead?  (The code would be much clearer if all the known vs. !known
codepath was fully separated here).

The table is never length 0, it's a statically defined array.

Segher

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited

On Thu, Feb 07, 2019 at 05:59:48PM +1100, Andrew Donnellan wrote:

On 7/2/19 5:37 pm, Segher Boessenkool wrote:

quoted

On Thu, Feb 07, 2019 at 04:33:23PM +1100, Andrew Donnellan wrote:

quoted

Some older gccs (<GCC 7), when invoked with -fsanitize-coverage=trace-pc,
cause a spurious uninitialised variable warning in dt_cpu_ftrs.c:

  arch/powerpc/kernel/dt_cpu_ftrs.c: In function 
  ‘cpufeatures_process_feature’:
  arch/powerpc/kernel/dt_cpu_ftrs.c:686:7: warning: ‘m’ may be used 
  uninitialized in this function [-Wmaybe-uninitialized]
    if (m->cpu_ftr_bit_mask)

It seems to me the warning is correct?  If enable_unknown is false and no
cpu_feature is found, it will in

if (m->cpu_ftr_bit_mask)
	cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;

enable random features (whatever was last in the table), or indeed access
via NULL if the table is length 0?  So maybe this should be

if (known && m->cpu_ftr_bit_mask)
	cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;

instead?  (The code would be much clearer if all the known vs. !known
codepath was fully separated here).

The table is never length 0, it's a statically defined array.

Sure, and presumably that is why newer GCC doesn't warn.  But what about
the other point?  Is this code ever correct?  Enabling random features
(in cur_cpu_spec->cpu_features) when the name isn't found seems wrong.


Segher

(+ Nick)

On 7/2/19 6:49 pm, Segher Boessenkool wrote:

On Thu, Feb 07, 2019 at 05:59:48PM +1100, Andrew Donnellan wrote:

quoted

On 7/2/19 5:37 pm, Segher Boessenkool wrote:

quoted

On Thu, Feb 07, 2019 at 04:33:23PM +1100, Andrew Donnellan wrote:

quoted

Some older gccs (<GCC 7), when invoked with -fsanitize-coverage=trace-pc,
cause a spurious uninitialised variable warning in dt_cpu_ftrs.c:

   arch/powerpc/kernel/dt_cpu_ftrs.c: In function
   ‘cpufeatures_process_feature’:
   arch/powerpc/kernel/dt_cpu_ftrs.c:686:7: warning: ‘m’ may be used
   uninitialized in this function [-Wmaybe-uninitialized]
     if (m->cpu_ftr_bit_mask)

It seems to me the warning is correct?  If enable_unknown is false and no
cpu_feature is found, it will in

	if (m->cpu_ftr_bit_mask)
		cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;

enable random features (whatever was last in the table), or indeed access
via NULL if the table is length 0?  So maybe this should be

	if (known && m->cpu_ftr_bit_mask)
		cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;

instead?  (The code would be much clearer if all the known vs. !known
codepath was fully separated here).

The table is never length 0, it's a statically defined array.

Sure, and presumably that is why newer GCC doesn't warn.  But what about
the other point?  Is this code ever correct?  Enabling random features
(in cur_cpu_spec->cpu_features) when the name isn't found seems wrong.

Now that I'm replying without being 2 minutes before a meeting :)

The warning is still spurious, but the logic looks very suspicious.

I think your solution looks correct, though the whole function could be 
cleaned up a bit.

I also notice that if enable_unknown == false, then I think an unknown 
feature will still print "enabling" and return true, which seems wrong.

How does something like the following look, which I could send instead 
and will probably solve the spurious warnings issues anyway?

diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c 

b/arch/powerpc/kernel/dt_cpu_ftrs.c
index 2192b2114513..0f13048dc0dd 100644

--- a/arch/powerpc/kernel/dt_cpu_ftrs.c
+++ b/arch/powerpc/kernel/dt_cpu_ftrs.c

@@ -658,40 +658,31 @@ static void __init cpufeatures_setup_start(u32 isa)

  static bool __init cpufeatures_process_feature(struct dt_cpu_feature *f)
  {
-       const struct dt_cpu_feature_match *m = NULL;
-       bool known = false;
+       const struct dt_cpu_feature_match *m;
         int i;

         for (i = 0; i < ARRAY_SIZE(dt_cpu_feature_match_table); i++) {
                 m = &dt_cpu_feature_match_table[i];
                 if (!strcmp(f->name, m->name)) {
-                       known = true;
-                       if (m->enable(f))
-                               break;
-
-                       pr_info("not enabling: %s (disabled or 

unsupported by kernel)\n",
-                               f->name);
-                       return false;
-               }
-       }
-
-       if (!known && enable_unknown) {
-               if (!feat_try_enable_unknown(f)) {
-                       pr_info("not enabling: %s (unknown and 
unsupported by kernel)\n",
-                               f->name);
-                       return false;
+                       if (!m->enable(f)) {
+                               pr_info("not enabling: %s (disabled or 
unsupported by kernel)\n",
+                                       f->name);
+                               return false;
+                       }
+                       pr_debug("enabling: %s\n", f->name);
+                       cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;
+                       return true;
                 }
         }

-       if (m->cpu_ftr_bit_mask)
-               cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;
-
-       if (known)
-               pr_debug("enabling: %s\n", f->name);
-       else
+       if (enable_unknown && feat_try_enable_unknown(f)) {
                 pr_debug("enabling: %s (unknown)\n", f->name);
-
-       return true;
+               return true;
+       } else {
+               pr_info("not enabling: %s (unknown and unsupported by 
kernel)\n",
+                       f->name);
+               return false;
+       }
  }

  static __init void cpufeatures_cpu_quirks(void)


-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited

Andrew Donnellan [off-list ref] writes:

(+ Nick)

On 7/2/19 6:49 pm, Segher Boessenkool wrote:

quoted

On Thu, Feb 07, 2019 at 05:59:48PM +1100, Andrew Donnellan wrote:

quoted

On 7/2/19 5:37 pm, Segher Boessenkool wrote:

quoted

On Thu, Feb 07, 2019 at 04:33:23PM +1100, Andrew Donnellan wrote:

quoted

Some older gccs (<GCC 7), when invoked with -fsanitize-coverage=trace-pc,
cause a spurious uninitialised variable warning in dt_cpu_ftrs.c:

   arch/powerpc/kernel/dt_cpu_ftrs.c: In function
   ‘cpufeatures_process_feature’:
   arch/powerpc/kernel/dt_cpu_ftrs.c:686:7: warning: ‘m’ may be used
   uninitialized in this function [-Wmaybe-uninitialized]
     if (m->cpu_ftr_bit_mask)

It seems to me the warning is correct?  If enable_unknown is false and no
cpu_feature is found, it will in

	if (m->cpu_ftr_bit_mask)
		cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;

enable random features (whatever was last in the table), or indeed access
via NULL if the table is length 0?  So maybe this should be

	if (known && m->cpu_ftr_bit_mask)
		cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;

instead?  (The code would be much clearer if all the known vs. !known
codepath was fully separated here).

The table is never length 0, it's a statically defined array.

Sure, and presumably that is why newer GCC doesn't warn.  But what about
the other point?  Is this code ever correct?  Enabling random features
(in cur_cpu_spec->cpu_features) when the name isn't found seems wrong.

Now that I'm replying without being 2 minutes before a meeting :)

The warning is still spurious, but the logic looks very suspicious.

I think your solution looks correct, though the whole function could be 
cleaned up a bit.

I also notice that if enable_unknown == false, then I think an unknown 
feature will still print "enabling" and return true, which seems wrong.

How does something like the following look, which I could send instead 
and will probably solve the spurious warnings issues anyway?

I'd prefer a minimal fix that we can backport. How about:

diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
index 8be3721d9302..a1acccd25839 100644
--- a/arch/powerpc/kernel/dt_cpu_ftrs.c
+++ b/arch/powerpc/kernel/dt_cpu_ftrs.c

@@ -675,12 +675,10 @@ static bool __init cpufeatures_process_feature(struct dt_cpu_feature *f)
 		}
 	}
 
-	if (!known && enable_unknown) {
-		if (!feat_try_enable_unknown(f)) {
-			pr_info("not enabling: %s (unknown and unsupported by kernel)\n",
-				f->name);
-			return false;
-		}
+	if (!known && (!enable_unknown || !feat_try_enable_unknown(f))) {
+		pr_info("not enabling: %s (unknown and unsupported by kernel)\n",
+			f->name);
+		return false;
 	}
 
 	if (m->cpu_ftr_bit_mask)

cheers

On 8/2/19 2:02 pm, Michael Ellerman wrote:> I'd prefer a minimal fix 
that we can backport. How about:

Sure, I can send a v2 with this fix and your signoff?

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited

On Fri, Feb 08, 2019 at 02:02:24PM +1100, Michael Ellerman wrote:

   		cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;

This still set the wrong mask here, which is the bug you're trying to fix.
It should only do this if "known", afaics.


Segher

On 9/2/19 2:41 am, Segher Boessenkool wrote:

On Fri, Feb 08, 2019 at 02:02:24PM +1100, Michael Ellerman wrote:

quoted

diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
index 8be3721d9302..a1acccd25839 100644
--- a/arch/powerpc/kernel/dt_cpu_ftrs.c
+++ b/arch/powerpc/kernel/dt_cpu_ftrs.c

@@ -675,12 +675,10 @@ static bool __init cpufeatures_process_feature(struct dt_cpu_feature *f)
  		}
  	}
  
-	if (!known && enable_unknown) {
-		if (!feat_try_enable_unknown(f)) {
-			pr_info("not enabling: %s (unknown and unsupported by kernel)\n",
-				f->name);
-			return false;
-		}
+	if (!known && (!enable_unknown || !feat_try_enable_unknown(f))) {
+		pr_info("not enabling: %s (unknown and unsupported by kernel)\n",
+			f->name);
+		return false;
  	}
  
  	if (m->cpu_ftr_bit_mask)

    		cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;

This still set the wrong mask here, which is the bug you're trying to fix.
It should only do this if "known", afaics.

I've got a v2 ready to send which fixes both things.

Segher

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited