RAS: two fixlets

PATCH In review Borislav Petkov 4 participants 7 messages Oct 2, 2017 – Oct 7, 2017

Subsystems (2)

x86 RAS infrastructure

lore

Share & tools

lore

Borislav Petkov

author maintainer

2017-10-02 09:28 UTC #0 lore raw

cover RAS: two fixlets

From: Borislav Petkov <[email protected]>

Hi guys,

just minor stuff right now. Please queue.

Thx.

Borislav Petkov (1):
x86/mce: Hide mca_cfg

Nicolas Iooss (1):
RAS/CEC: Use the right length for "cec_disable"

 arch/x86/include/asm/mce.h                | 1 -
 arch/x86/kernel/cpu/mcheck/mce-internal.h | 7 +++++++
 arch/x86/kernel/cpu/mcheck/mce_amd.c      | 2 ++
 drivers/ras/cec.c                         | 2 +-
 4 files changed, 10 insertions(+), 2 deletions(-)

Signature · 1 line

2.13.0

Borislav Petkov

author maintainer

2017-10-02 09:29 UTC #1 lore raw

PATCH 1/2 RAS/CEC: Use the right length for "cec_disable"

From: Nicolas Iooss <[email protected]>

parse_cec_param() compares a string with "cec_disable" using only 7 characters of the 11-character-long string. Fix the length.

Signed-off-by Nicolas Iooss <[email protected]>

Fixes 011d82611172 ("RAS: Add a Corrected Errors Collector")

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Borislav Petkov <[email protected]>

---
 drivers/ras/cec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/ras/cec.c +1 -1

index d0e5d6ee882c..586c296d1538 100644
@@ -523,7 +523,7 @@ int __init parse_cec_param(char *str)
 	if (*str == '=')
 		str++;
 
-	if (!strncmp(str, "cec_disable", 7))
+	if (!strncmp(str, "cec_disable", 11))
 		ce_arr.disabled = 1;
 	else
 		return 0;

Signature · 1 line

2.13.0

Thomas Gleixner

2017-10-02 15:43 UTC #2 lore raw

Re: [PATCH 1/2] RAS/CEC: Use the right length for "cec_disable"

Quoted 18 lines · Borislav Petkov

From: Nicolas Iooss <[email protected]>
parse_cec_param() compares a string with "cec_disable" using only 7 characters of the 11-character-long string. Fix the length.
Signed-off-by Nicolas Iooss <[email protected]>
Fixes 011d82611172 ("RAS: Add a Corrected Errors Collector")
Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by Borislav Petkov <[email protected]>
---
 drivers/ras/cec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/ras/cec.c +0 -0
index d0e5d6ee882c..586c296d1538 100644

drivers/ras/cec.c int __init parse_cec_param(char *str) · line 526

Quoted 5 lines

drivers/ras/cec.c +1 -1

@@ -523,7 +523,7 @@ int __init parse_cec_param(char *str)
 	if (*str == '=')
 		str++;
 
-	if (!strncmp(str, "cec_disable", 7))
+	if (!strncmp(str, "cec_disable", 11))

This kind of issue happens over and over. So if you really want to use strncmp() then this should be:

#define CEC_DISABLE "cec_disable"

if (!strncmp(str, CEC_DISABLE, strlen(CEC_DISABLE))

or we get a proper helper for that. Though in case of comparing some string against a constant string strncmp() has no real advantage over strcmp() as the comparison is guaranteed to be bound by the string constant.

Thanks,

tglx

Borislav Petkov

author maintainer

2017-10-03 09:04 UTC #3 lore raw

Re: [PATCH 1/2] RAS/CEC: Use the right length for "cec_disable"

Quoted 28 lines · Thomas Gleixner

Quoted 18 lines · Borislav Petkov
From: Nicolas Iooss <[email protected]>
parse_cec_param() compares a string with "cec_disable" using only 7 characters of the 11-character-long string. Fix the length.
Signed-off-by Nicolas Iooss <[email protected]>
Fixes 011d82611172 ("RAS: Add a Corrected Errors Collector")
Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by Borislav Petkov <[email protected]>
---
 drivers/ras/cec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/ras/cec.c +0 -0
index d0e5d6ee882c..586c296d1538 100644
drivers/ras/cec.c int __init parse_cec_param(char *str) · line 526
Quoted 5 lines
drivers/ras/cec.c +1 -1
@@ -523,7 +523,7 @@ int __init parse_cec_param(char *str)
 	if (*str == '=')
 		str++;
 
-	if (!strncmp(str, "cec_disable", 7))
+	if (!strncmp(str, "cec_disable", 11))
This kind of issue happens over and over. So if you really want to use strncmp() then this should be:

Quoted 7 lines

#define CEC_DISABLE "cec_disable"
if (!strncmp(str, CEC_DISABLE, strlen(CEC_DISABLE))
or we get a proper helper for that. Though in case of comparing some string against a constant string strncmp() has no real advantage over strcmp() as the comparison is guaranteed to be bound by the string constant.

Right.

Nicolas, wanna address that?

Thx.

Signature · 4 lines

Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Nicolas Iooss

2017-10-07 14:38 UTC #4 lore raw

Re: [PATCH 1/2] RAS/CEC: Use the right length for "cec_disable"

Quoted 37 lines · Borislav Petkov

Quoted 28 lines · Thomas Gleixner
Quoted 18 lines · Borislav Petkov
From: Nicolas Iooss <[email protected]>
parse_cec_param() compares a string with "cec_disable" using only 7 characters of the 11-character-long string. Fix the length.
Signed-off-by Nicolas Iooss <[email protected]>
Fixes 011d82611172 ("RAS: Add a Corrected Errors Collector")
Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by Borislav Petkov <[email protected]>
---
 drivers/ras/cec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/ras/cec.c +0 -0
index d0e5d6ee882c..586c296d1538 100644
drivers/ras/cec.c int __init parse_cec_param(char *str) · line 526
Quoted 5 lines
drivers/ras/cec.c +1 -1
@@ -523,7 +523,7 @@ int __init parse_cec_param(char *str)
    if (*str == '=')
            str++;
 
-   if (!strncmp(str, "cec_disable", 7))
+   if (!strncmp(str, "cec_disable", 11))
This kind of issue happens over and over. So if you really want to use strncmp() then this should be:
Quoted 7 lines
#define CEC_DISABLE "cec_disable"
if (!strncmp(str, CEC_DISABLE, strlen(CEC_DISABLE))
or we get a proper helper for that. Though in case of comparing some string against a constant string strncmp() has no real advantage over strcmp() as the comparison is guaranteed to be bound by the string constant.

Quoted 5 lines

Right.
Nicolas, wanna address that?
Thx.

Hi, the only difference I see between strncmp+strlen and strcmp is that the first option compares a prefix ("does the string begin with cec_disable?") and the second one checks that strings are equals. In parse_cec_param() it does not seem to matter.
I have seen that Thomas Gleixner has already committed a modified patch which uses strcmp(), which looks good to me.

Thanks!
Nicolas

Borislav Petkov

author maintainer

2017-10-02 09:28 UTC #5 lore raw

PATCH 2/2 x86/mce: Hide mca_cfg

In reply to Borislav Petkov

From: Borislav Petkov <[email protected]>

Now that lguest is gone, put it in the internal header which should be used only by MCA/RAS code.

Add missing header guards while at it.

No functionality change.

Signed-off-by Borislav Petkov <[email protected]>

---
 arch/x86/include/asm/mce.h                | 1 -
 arch/x86/kernel/cpu/mcheck/mce-internal.h | 7 +++++++
 arch/x86/kernel/cpu/mcheck/mce_amd.c      | 2 ++
 3 files changed, 9 insertions(+), 1 deletion(-)

arch/x86/include/asm/mce.h +0 -1

index 181264989db5..8edac1de2e35 100644
@@ -187,7 +187,6 @@ struct mca_msr_regs {
 
 extern struct mce_vendor_flags mce_flags;
 
-extern struct mca_config mca_cfg;
 extern struct mca_msr_regs msr_ops;
 
 enum mce_notifier_prios {

arch/x86/kernel/cpu/mcheck/mce-internal.h +7 -0

index 098530a93bb7..debb974fd17d 100644
@@ -1,3 +1,6 @@
+#ifndef __X86_MCE_INTERNAL_H__
+#define __X86_MCE_INTERNAL_H__
+
 #include <linux/device.h>
 #include <asm/mce.h>

@@ -108,3 +111,7 @@ static inline void mce_work_trigger(void)	{ }
 static inline void mce_register_injector_chain(struct notifier_block *nb)	{ }
 static inline void mce_unregister_injector_chain(struct notifier_block *nb)	{ }
 #endif
+
+extern struct mca_config mca_cfg;
+
+#endif /* __X86_MCE_INTERNAL_H__ */

arch/x86/kernel/cpu/mcheck/mce_amd.c +2 -0

index 40e28ed77fbf..486f640b02ef 100644
@@ -28,6 +28,8 @@
 #include <asm/msr.h>
 #include <asm/trace/irq_vectors.h>
 
+#include "mce-internal.h"
+
 #define NR_BLOCKS         5
 #define THRESHOLD_MAX     0xFFF
 #define INT_TYPE_APIC     0x00020000

Signature · 1 line

2.13.0

tip-bot for Borislav Petkov

merge bot

2017-10-05 12:29 UTC #6 lore raw

[tip:ras/urgent] x86/mce: Hide mca_cfg

x86/mce: Hide mca_cfg

Commit-ID:  262e681183ddcdb24d64a2f993e41a226adcec29
Gitweb:     https://git.kernel.org/tip/262e681183ddcdb24d64a2f993e41a226adcec29
Author:     Borislav Petkov <[email protected]>
AuthorDate: Mon, 2 Oct 2017 11:28:36 +0200
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Thu, 5 Oct 2017 14:23:06 +0200

x86/mce: Hide mca_cfg

Now that lguest is gone, put it in the internal header which should be used only by MCA/RAS code.

Add missing header guards while at it.

No functional change.

Signed-off-by Borislav Petkov <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

Link https://lkml.kernel.org/r/[email protected] open here

---
 arch/x86/include/asm/mce.h                | 1 -
 arch/x86/kernel/cpu/mcheck/mce-internal.h | 7 +++++++
 arch/x86/kernel/cpu/mcheck/mce_amd.c      | 2 ++
 3 files changed, 9 insertions(+), 1 deletion(-)

arch/x86/include/asm/mce.h +0 -1

index 1812649..8edac1d 100644
@@ -187,7 +187,6 @@ struct mca_msr_regs {
 
 extern struct mce_vendor_flags mce_flags;
 
-extern struct mca_config mca_cfg;
 extern struct mca_msr_regs msr_ops;
 
 enum mce_notifier_prios {

arch/x86/kernel/cpu/mcheck/mce-internal.h +7 -0

index 098530a..debb974 100644
@@ -1,3 +1,6 @@
+#ifndef __X86_MCE_INTERNAL_H__
+#define __X86_MCE_INTERNAL_H__
+
 #include <linux/device.h>
 #include <asm/mce.h>

@@ -108,3 +111,7 @@ static inline void mce_work_trigger(void)	{ }
 static inline void mce_register_injector_chain(struct notifier_block *nb)	{ }
 static inline void mce_unregister_injector_chain(struct notifier_block *nb)	{ }
 #endif
+
+extern struct mca_config mca_cfg;
+
+#endif /* __X86_MCE_INTERNAL_H__ */

arch/x86/kernel/cpu/mcheck/mce_amd.c +2 -0

index 40e28ed..486f640 100644
@@ -28,6 +28,8 @@
 #include <asm/msr.h>
 #include <asm/trace/irq_vectors.h>
 
+#include "mce-internal.h"
+
 #define NR_BLOCKS         5
 #define THRESHOLD_MAX     0xFFF
 #define INT_TYPE_APIC     0x00020000