When comparing 'model name' fields in /proc/cpuinfo it was noticed that
a simple test comparing the model name fields was failing. After some
quick investigation it was noticed that the model name fields were actually
different -- processor 0's model name field had trailing white space removed,
while the other processors did not.
Another way of seeing this behaviour is to convert spaces into underscores
in the output of /proc/cpuinfo,
[thetango@prarit ~]# grep "^model name" /proc/cpuinfo | uniq -c | sed 's/\ /_/g'
______1_model_name :_AMD_Opteron(TM)_Processor_6272
_____63_model_name :_AMD_Opteron(TM)_Processor_6272_________________
which shows two different model name fields even though they should be the
same.
This occurs because the kernel calls strim() on cpu 0's x86_model_id field
to output a pretty message to the console in print_cpu_info(), and as a
result truncates the whitespace at the end of the x86_model_id field.
The x86_model_id field should be the same for the same processors. This
patch uses string functions to remove both leading and trailing whitespace
in the x86_model_id field. As a result the print_cpu_info() output looks
like
smpboot: CPU0: AMD Opteron(TM) Processor 6272 (fam: 15, model: 01, stepping: 02)
and the x86_model_id field is correct on all processors on AMD platforms
[thetango@prarit ~]# grep "^model name" /proc/cpuinfo | uniq -c | sed 's/\ /_/g'
_____64_model_name :_AMD_Opteron(TM)_Processor_6272
and the functionality is correct on an Intel box:
[thetango@prarit2]# grep "^model name" /proc/cpuinfo | uniq -c | sed 's/\ /_/g'
____144_model_name :_Intel(R)_Xeon(R)_CPU_E7-8890_v3_@_2.50GHz
Signed-off-by: Prarit Bhargava <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: [email protected]
Cc: Andy Lutomirski <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Denys Vlasenko <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Igor Mammedov <[email protected]>
Cc: Fenghua Yu <[email protected]>
Cc: Brian Gerst <[email protected]>
---
arch/x86/kernel/cpu/common.c | 17 ++++-------------
1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index a62cf04..9405c1e 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -419,7 +419,6 @@ static const struct cpu_dev *cpu_devs[X86_VENDOR_NUM] = {};
static void get_model_name(struct cpuinfo_x86 *c)
{
unsigned int *v;
- char *p, *q;
if (c->extended_cpuid_level < 0x80000004)
return;
@@ -431,18 +430,10 @@ static void get_model_name(struct cpuinfo_x86 *c)
c->x86_model_id[48] = 0;
/*
- * Intel chips right-justify this string for some dumb reason;
- * undo that brain damage:
+ * Remove leading whitespace on Intel processors and trailing
+ * whitespace on AMD processors.
*/
- p = q = &c->x86_model_id[0];
- while (*p == ' ')
- p++;
- if (p != q) {
- while (*p)
- *q++ = *p++;
- while (q <= &c->x86_model_id[48])
- *q++ = '\0'; /* Zero-pad the rest */
- }
+ strlcpy(c->x86_model_id, strim(c->x86_model_id), 48);
}
void cpu_detect_cache_sizes(struct cpuinfo_x86 *c)
@@ -1122,7 +1113,7 @@ void print_cpu_info(struct cpuinfo_x86 *c)
printk(KERN_CONT "%s ", vendor);
if (c->x86_model_id[0])
- printk(KERN_CONT "%s", strim(c->x86_model_id));
+ printk(KERN_CONT "%s", c->x86_model_id);
else
printk(KERN_CONT "%d86", c->x86);
--
1.7.9.3
On Tue, May 19, 2015 at 11:43:30AM -0400, Prarit Bhargava wrote:
> When comparing 'model name' fields in /proc/cpuinfo it was noticed that
> a simple test comparing the model name fields was failing. After some
> quick investigation it was noticed that the model name fields were actually
> different -- processor 0's model name field had trailing white space removed,
> while the other processors did not.
>
> Another way of seeing this behaviour is to convert spaces into underscores
> in the output of /proc/cpuinfo,
>
> [thetango@prarit ~]# grep "^model name" /proc/cpuinfo | uniq -c | sed 's/\ /_/g'
> ______1_model_name :_AMD_Opteron(TM)_Processor_6272
> _____63_model_name :_AMD_Opteron(TM)_Processor_6272_________________
>
> which shows two different model name fields even though they should be the
> same.
>
> This occurs because the kernel calls strim() on cpu 0's x86_model_id field
> to output a pretty message to the console in print_cpu_info(), and as a
> result truncates the whitespace at the end of the x86_model_id field.
>
> The x86_model_id field should be the same for the same processors. This
> patch uses string functions to remove both leading and trailing whitespace
> in the x86_model_id field. As a result the print_cpu_info() output looks
> like
>
> smpboot: CPU0: AMD Opteron(TM) Processor 6272 (fam: 15, model: 01, stepping: 02)
>
> and the x86_model_id field is correct on all processors on AMD platforms
>
> [thetango@prarit ~]# grep "^model name" /proc/cpuinfo | uniq -c | sed 's/\ /_/g'
> _____64_model_name :_AMD_Opteron(TM)_Processor_6272
>
> and the functionality is correct on an Intel box:
>
> [thetango@prarit2]# grep "^model name" /proc/cpuinfo | uniq -c | sed 's/\ /_/g'
> ____144_model_name :_Intel(R)_Xeon(R)_CPU_E7-8890_v3_@_2.50GHz
>
> Signed-off-by: Prarit Bhargava <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: "H. Peter Anvin" <[email protected]>
> Cc: [email protected]
> Cc: Andy Lutomirski <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Denys Vlasenko <[email protected]>
> Cc: Dave Hansen <[email protected]>
> Cc: Igor Mammedov <[email protected]>
> Cc: Fenghua Yu <[email protected]>
> Cc: Brian Gerst <[email protected]>
> ---
> arch/x86/kernel/cpu/common.c | 17 ++++-------------
> 1 file changed, 4 insertions(+), 13 deletions(-)
Applied, thanks.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
On Tue, May 19, 2015 at 11:43 AM, Prarit Bhargava <[email protected]> wrote:
> When comparing 'model name' fields in /proc/cpuinfo it was noticed that
> a simple test comparing the model name fields was failing. After some
> quick investigation it was noticed that the model name fields were actually
> different -- processor 0's model name field had trailing white space removed,
> while the other processors did not.
>
> Another way of seeing this behaviour is to convert spaces into underscores
> in the output of /proc/cpuinfo,
>
> [thetango@prarit ~]# grep "^model name" /proc/cpuinfo | uniq -c | sed 's/\ /_/g'
> ______1_model_name :_AMD_Opteron(TM)_Processor_6272
> _____63_model_name :_AMD_Opteron(TM)_Processor_6272_________________
>
> which shows two different model name fields even though they should be the
> same.
>
> This occurs because the kernel calls strim() on cpu 0's x86_model_id field
> to output a pretty message to the console in print_cpu_info(), and as a
> result truncates the whitespace at the end of the x86_model_id field.
>
> The x86_model_id field should be the same for the same processors. This
> patch uses string functions to remove both leading and trailing whitespace
> in the x86_model_id field. As a result the print_cpu_info() output looks
> like
>
> smpboot: CPU0: AMD Opteron(TM) Processor 6272 (fam: 15, model: 01, stepping: 02)
>
> and the x86_model_id field is correct on all processors on AMD platforms
>
> [thetango@prarit ~]# grep "^model name" /proc/cpuinfo | uniq -c | sed 's/\ /_/g'
> _____64_model_name :_AMD_Opteron(TM)_Processor_6272
>
> and the functionality is correct on an Intel box:
>
> [thetango@prarit2]# grep "^model name" /proc/cpuinfo | uniq -c | sed 's/\ /_/g'
> ____144_model_name :_Intel(R)_Xeon(R)_CPU_E7-8890_v3_@_2.50GHz
>
> Signed-off-by: Prarit Bhargava <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: "H. Peter Anvin" <[email protected]>
> Cc: [email protected]
> Cc: Andy Lutomirski <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Denys Vlasenko <[email protected]>
> Cc: Dave Hansen <[email protected]>
> Cc: Igor Mammedov <[email protected]>
> Cc: Fenghua Yu <[email protected]>
> Cc: Brian Gerst <[email protected]>
> ---
> arch/x86/kernel/cpu/common.c | 17 ++++-------------
> 1 file changed, 4 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index a62cf04..9405c1e 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -419,7 +419,6 @@ static const struct cpu_dev *cpu_devs[X86_VENDOR_NUM] = {};
> static void get_model_name(struct cpuinfo_x86 *c)
> {
> unsigned int *v;
> - char *p, *q;
>
> if (c->extended_cpuid_level < 0x80000004)
> return;
> @@ -431,18 +430,10 @@ static void get_model_name(struct cpuinfo_x86 *c)
> c->x86_model_id[48] = 0;
>
> /*
> - * Intel chips right-justify this string for some dumb reason;
> - * undo that brain damage:
> + * Remove leading whitespace on Intel processors and trailing
> + * whitespace on AMD processors.
> */
> - p = q = &c->x86_model_id[0];
> - while (*p == ' ')
> - p++;
> - if (p != q) {
> - while (*p)
> - *q++ = *p++;
> - while (q <= &c->x86_model_id[48])
> - *q++ = '\0'; /* Zero-pad the rest */
> - }
> + strlcpy(c->x86_model_id, strim(c->x86_model_id), 48);
> }
Using strlcpy in this manner could fail if it does larger than byte
copies and they overlap. I would instead allocate a temp buffer on
the stack:
unsigned char model_id[49];
v = (unsigned int *)model_id;
...
strlcpy(c->x86_model_id, strim(model_id), 48);
--
Brian Gerst
On Tue, May 19, 2015 at 01:25:59PM -0400, Brian Gerst wrote:
> Using strlcpy in this manner could fail if it does larger than byte
> copies and they overlap.
Why?
AFAICT, strlcpy() calls memcpy() and memcpy should handle overlapping
buffers just fine.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
On May 19, 2015 11:13 AM, "Borislav Petkov" <[email protected]> wrote:
>
> On Tue, May 19, 2015 at 01:25:59PM -0400, Brian Gerst wrote:
> > Using strlcpy in this manner could fail if it does larger than byte
> > copies and they overlap.
>
> Why?
>
> AFAICT, strlcpy() calls memcpy() and memcpy should handle overlapping
> buffers just fine.
Are you thinking of memmove?
--Andy
On Tue, May 19, 2015 at 11:44:41AM -0700, Andy Lutomirski wrote:
> On May 19, 2015 11:13 AM, "Borislav Petkov" <[email protected]> wrote:
> >
> > On Tue, May 19, 2015 at 01:25:59PM -0400, Brian Gerst wrote:
> > > Using strlcpy in this manner could fail if it does larger than byte
> > > copies and they overlap.
> >
> > Why?
> >
> > AFAICT, strlcpy() calls memcpy() and memcpy should handle overlapping
> > buffers just fine.
>
> Are you thinking of memmove?
I guess I'm trying to find out why don't we have a BIG FAT WARNING over
memcpy saying not to use it with overlapping buffers and larger than
byte sizes. Or maybe this is something everyone, except me, just knows
and that's a "Doh, Boris, of course!".
Btw, can we still avoid using the temporary buffer and use strncpy()
instead? AFAICT, that does byte copies, from looking at the asm.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
On Tue, May 19, 2015 at 12:22 PM, Borislav Petkov <[email protected]> wrote:
> On Tue, May 19, 2015 at 11:44:41AM -0700, Andy Lutomirski wrote:
>> On May 19, 2015 11:13 AM, "Borislav Petkov" <[email protected]> wrote:
>> >
>> > On Tue, May 19, 2015 at 01:25:59PM -0400, Brian Gerst wrote:
>> > > Using strlcpy in this manner could fail if it does larger than byte
>> > > copies and they overlap.
>> >
>> > Why?
>> >
>> > AFAICT, strlcpy() calls memcpy() and memcpy should handle overlapping
>> > buffers just fine.
>>
>> Are you thinking of memmove?
>
> I guess I'm trying to find out why don't we have a BIG FAT WARNING over
> memcpy saying not to use it with overlapping buffers and larger than
> byte sizes. Or maybe this is something everyone, except me, just knows
> and that's a "Doh, Boris, of course!".
>
> Btw, can we still avoid using the temporary buffer and use strncpy()
> instead? AFAICT, that does byte copies, from looking at the asm.
It's not just chunk size; it's the direction. If the dest starts
after the source but overlaps it and you copy forwards, then you can
clobber the end of the source before you read it. memmove is
specifically intended to avoid this.
Would it be possible to just use memmove directly?
--Andy
iOn Tue, 2015-05-19 at 13:16 -0700, Andy Lutomirski wrote:
> On Tue, May 19, 2015 at 12:22 PM, Borislav Petkov <[email protected]> wrote:
> > On Tue, May 19, 2015 at 11:44:41AM -0700, Andy Lutomirski wrote:
> >> On May 19, 2015 11:13 AM, "Borislav Petkov" <[email protected]> wrote:
> >> >
> >> > On Tue, May 19, 2015 at 01:25:59PM -0400, Brian Gerst wrote:
> >> > > Using strlcpy in this manner could fail if it does larger than byte
> >> > > copies and they overlap.
> >> >
> >> > Why?
I think this is traditionally handled by specifying that
the strcpy strings may not overlap, so the suggested
+ strlcpy(c->x86_model_id, strim(c->x86_model_id), 48);
isn't good code.
A temporary intermediate buffer is required.
On Tue, 2015-05-19 at 13:26 -0700, Joe Perches wrote:
> iOn Tue, 2015-05-19 at 13:16 -0700, Andy Lutomirski wrote:
> > On Tue, May 19, 2015 at 12:22 PM, Borislav Petkov <[email protected]> wrote:
> > > On Tue, May 19, 2015 at 11:44:41AM -0700, Andy Lutomirski wrote:
> > >> On May 19, 2015 11:13 AM, "Borislav Petkov" <[email protected]> wrote:
> > >> >
> > >> > On Tue, May 19, 2015 at 01:25:59PM -0400, Brian Gerst wrote:
> > >> > > Using strlcpy in this manner could fail if it does larger than byte
> > >> > > copies and they overlap.
> > >> >
> > >> > Why?
>
> I think this is traditionally handled by specifying that
> the strcpy strings may not overlap, so the suggested
>
> + strlcpy(c->x86_model_id, strim(c->x86_model_id), 48);
>
> isn't good code.
>
> A temporary intermediate buffer is required.
Or memmove. (duh)
On Tue, May 19, 2015 at 01:16:23PM -0700, Andy Lutomirski wrote:
> It's not just chunk size; it's the direction. If the dest starts
> after the source but overlaps it and you copy forwards, then you can
> clobber the end of the source before you read it. memmove is
Some things should simply be done solely in userspace :-)
> specifically intended to avoid this.
>
> Would it be possible to just use memmove directly?
Yeah.
Prarit, care to send a v2?
Thanks.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
On 05/19/2015 04:31 PM, Borislav Petkov wrote:
> On Tue, May 19, 2015 at 01:16:23PM -0700, Andy Lutomirski wrote:
>> It's not just chunk size; it's the direction. If the dest starts
>> after the source but overlaps it and you copy forwards, then you can
>> clobber the end of the source before you read it. memmove is
>
> Some things should simply be done solely in userspace :-)
>
>> specifically intended to avoid this.
>>
>> Would it be possible to just use memmove directly?
>
> Yeah.
>
> Prarit, care to send a v2?
Yep, I can do that. It'll have to wait until I get into the office in the
morning so I can retest.
P.
>
> Thanks.
>
* Prarit Bhargava <[email protected]> wrote:
> arch/x86/kernel/cpu/common.c | 17 ++++-------------
> 1 file changed, 4 insertions(+), 13 deletions(-)
So I saw this title:
[PATCH] x86, cpuinfo x86_model_id whitespace cleanup
... and in an early morning deconcentrated state was skipped the
changelog and was looking for a whitespace coding style cleanup:
> @@ -431,18 +430,10 @@ static void get_model_name(struct cpuinfo_x86 *c)
> c->x86_model_id[48] = 0;
>
> /*
> - * Intel chips right-justify this string for some dumb reason;
> - * undo that brain damage:
> + * Remove leading whitespace on Intel processors and trailing
> + * whitespace on AMD processors.
> */
> - p = q = &c->x86_model_id[0];
> - while (*p == ' ')
> - p++;
> - if (p != q) {
> - while (*p)
> - *q++ = *p++;
> - while (q <= &c->x86_model_id[48])
> - *q++ = '\0'; /* Zero-pad the rest */
> - }
> + strlcpy(c->x86_model_id, strim(c->x86_model_id), 48);
> }
Which this clearly isnt!
Fortunately before complaining about that I read the changelog as
well, and realized that the 'whitespace cleanup' is done to
/proc/cpuinfo ABI output.
Could you please make the title less ambiguous, so that sleepy kernel
developers get the right idea what the patch looks like, from the
title alone? Git shortlogs will vastly improve as well.
Something like:
[PATCH] x86/cpu: Strip leading and trailing spaces from the /proc/cpuinfo CPU model field
... or so would work very well for me!
Thanks,
Ingo
On 05/20/2015 02:34 AM, Ingo Molnar wrote:
>
> * Prarit Bhargava <[email protected]> wrote:
>
>> arch/x86/kernel/cpu/common.c | 17 ++++-------------
>> 1 file changed, 4 insertions(+), 13 deletions(-)
>
> So I saw this title:
>
> [PATCH] x86, cpuinfo x86_model_id whitespace cleanup
>
> ... and in an early morning deconcentrated state was skipped the
> changelog and was looking for a whitespace coding style cleanup:
>
>> @@ -431,18 +430,10 @@ static void get_model_name(struct cpuinfo_x86 *c)
>> c->x86_model_id[48] = 0;
>>
>> /*
>> - * Intel chips right-justify this string for some dumb reason;
>> - * undo that brain damage:
>> + * Remove leading whitespace on Intel processors and trailing
>> + * whitespace on AMD processors.
>> */
>> - p = q = &c->x86_model_id[0];
>> - while (*p == ' ')
>> - p++;
>> - if (p != q) {
>> - while (*p)
>> - *q++ = *p++;
>> - while (q <= &c->x86_model_id[48])
>> - *q++ = '\0'; /* Zero-pad the rest */
>> - }
>> + strlcpy(c->x86_model_id, strim(c->x86_model_id), 48);
>> }
>
> Which this clearly isnt!
>
> Fortunately before complaining about that I read the changelog as
> well, and realized that the 'whitespace cleanup' is done to
> /proc/cpuinfo ABI output.
>
> Could you please make the title less ambiguous, so that sleepy kernel
> developers get the right idea what the patch looks like, from the
> title alone? Git shortlogs will vastly improve as well.
>
> Something like:
>
> [PATCH] x86/cpu: Strip leading and trailing spaces from the /proc/cpuinfo CPU model field
>
> ... or so would work very well for me!
:) Sorry Ingo -- I'll definitely clean that up in the next version.
P.
>
> Thanks,
>
> Ingo
>
Commit-ID: ee098e1aed67715f0ce4651813d0c33ab3a56e0b
Gitweb: http://git.kernel.org/tip/ee098e1aed67715f0ce4651813d0c33ab3a56e0b
Author: Borislav Petkov <[email protected]>
AuthorDate: Mon, 1 Jun 2015 12:06:57 +0200
Committer: Ingo Molnar <[email protected]>
CommitDate: Tue, 2 Jun 2015 10:38:11 +0200
x86/cpu: Trim model ID whitespace
We did try trimming whitespace surrounding the 'model name'
field in /proc/cpuinfo since reportedly some userspace uses it
in string comparisons and there were discrepancies:
[thetango@prarit ~]# grep "^model name" /proc/cpuinfo | uniq -c | sed 's/\ /_/g'
______1_model_name :_AMD_Opteron(TM)_Processor_6272
_____63_model_name :_AMD_Opteron(TM)_Processor_6272_________________
However, there were issues with overlapping buffers, string
sizes and non-byte-sized copies in the previous proposed
solutions; see Link tags below for the whole farce.
So, instead of diddling with this more, let's simply extend what
was there originally with trimming any present trailing
whitespace. Final result is really simple and obvious.
Testing with the most insane model IDs qemu can generate, looks
good:
.model_id = " My funny model ID CPU ",
______4_model_name :_My_funny_model_ID_CPU
.model_id = "My funny model ID CPU ",
______4_model_name :_My_funny_model_ID_CPU
.model_id = " My funny model ID CPU",
______4_model_name :_My_funny_model_ID_CPU
.model_id = " ",
______4_model_name :__
.model_id = "",
______4_model_name :_15/02
Signed-off-by: Borislav Petkov <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Brian Gerst <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Denys Vlasenko <[email protected]>
Cc: Fenghua Yu <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: Igor Mammedov <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/kernel/cpu/common.c | 22 +++++++++++++++++-----
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 41a8e9c..351197c 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -5,6 +5,7 @@
#include <linux/module.h>
#include <linux/percpu.h>
#include <linux/string.h>
+#include <linux/ctype.h>
#include <linux/delay.h>
#include <linux/sched.h>
#include <linux/init.h>
@@ -419,6 +420,7 @@ static const struct cpu_dev *cpu_devs[X86_VENDOR_NUM] = {};
static void get_model_name(struct cpuinfo_x86 *c)
{
unsigned int *v;
+ char *p, *q, *s;
if (c->extended_cpuid_level < 0x80000004)
return;
@@ -429,11 +431,21 @@ static void get_model_name(struct cpuinfo_x86 *c)
cpuid(0x80000004, &v[8], &v[9], &v[10], &v[11]);
c->x86_model_id[48] = 0;
- /*
- * Remove leading whitespace on Intel processors and trailing
- * whitespace on AMD processors.
- */
- memmove(c->x86_model_id, strim(c->x86_model_id), 48);
+ /* Trim whitespace */
+ p = q = s = &c->x86_model_id[0];
+
+ while (*p == ' ')
+ p++;
+
+ while (*p) {
+ /* Note the last non-whitespace index */
+ if (!isspace(*p))
+ s = q;
+
+ *q++ = *p++;
+ }
+
+ *(s + 1) = '\0';
}
void cpu_detect_cache_sizes(struct cpuinfo_x86 *c)