* [PATCH] Make dmi_name_in_vendors more focused
@ 2011-11-04 9:26 Jean Delvare
2011-11-09 22:25 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Jean Delvare @ 2011-11-04 9:26 UTC (permalink / raw)
To: LKML; +Cc: Andi Kleen, Jesse Barnes, Andrew Morton
The current implementation of dmi_name_in_vendors() is an invitation
to lazy coding and false positives [1]. Searching for a string in 8
different DMI fields is something nobody should ever need to do. You
know what you're looking for, so you should know where to look. strstr
isn't fast, especially when it fails, so we should avoid calling it
when it just can't succeed.
Looking at the current users of the function, it seems clear to me
that they are looking for a system or board vendor name, so let's
limit dmi_name_in_vendors to these two DMI fields. This much better
matches the function name, BTW.
[1] We currently have code looking for short names in DMI data, such
as "IBM" or "ASUS". I let you guess what will happen the day other
vendors ship products named, for example, "SCHREIBMEISTER" or "PEGASUS".
Signed-off-by: Jean Delvare <khali@linux-fr.org>
Cc: Andi Kleen <andi@firstfloor.org>
---
This patch was already sent on 2011-05-15. I thought Andrew had picked
it up but apparently not, otherwise it should already be upstream by
now.
drivers/firmware/dmi_scan.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
--- linux-2.6.39-rc7.orig/drivers/firmware/dmi_scan.c 2011-03-15 22:52:48.000000000 +0100
+++ linux-2.6.39-rc7/drivers/firmware/dmi_scan.c 2011-05-15 15:05:29.000000000 +0200
@@ -585,14 +585,12 @@ int dmi_name_in_serial(const char *str)
}
/**
- * dmi_name_in_vendors - Check if string is anywhere in the DMI vendor information.
+ * dmi_name_in_vendors - Check if string is in the DMI system or board vendor name
* @str: Case sensitive Name
*/
int dmi_name_in_vendors(const char *str)
{
- static int fields[] = { DMI_BIOS_VENDOR, DMI_BIOS_VERSION, DMI_SYS_VENDOR,
- DMI_PRODUCT_NAME, DMI_PRODUCT_VERSION, DMI_BOARD_VENDOR,
- DMI_BOARD_NAME, DMI_BOARD_VERSION, DMI_NONE };
+ static int fields[] = { DMI_SYS_VENDOR, DMI_BOARD_VENDOR, DMI_NONE };
int i;
for (i = 0; fields[i] != DMI_NONE; i++) {
int f = fields[i];
--
Jean Delvare
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Make dmi_name_in_vendors more focused
2011-11-04 9:26 [PATCH] Make dmi_name_in_vendors more focused Jean Delvare
@ 2011-11-09 22:25 ` Andrew Morton
2011-11-10 3:35 ` Jesse Barnes
2011-11-10 8:13 ` Jean Delvare
0 siblings, 2 replies; 6+ messages in thread
From: Andrew Morton @ 2011-11-09 22:25 UTC (permalink / raw)
To: Jean Delvare; +Cc: LKML, Andi Kleen, Jesse Barnes
On Fri, 4 Nov 2011 10:26:47 +0100
Jean Delvare <khali@linux-fr.org> wrote:
> The current implementation of dmi_name_in_vendors() is an invitation
> to lazy coding and false positives [1]. Searching for a string in 8
> different DMI fields is something nobody should ever need to do. You
> know what you're looking for, so you should know where to look. strstr
> isn't fast, especially when it fails, so we should avoid calling it
> when it just can't succeed.
>
> Looking at the current users of the function, it seems clear to me
> that they are looking for a system or board vendor name, so let's
> limit dmi_name_in_vendors to these two DMI fields. This much better
> matches the function name, BTW.
>
> [1] We currently have code looking for short names in DMI data, such
> as "IBM" or "ASUS". I let you guess what will happen the day other
> vendors ship products named, for example, "SCHREIBMEISTER" or "PEGASUS".
>
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> Cc: Andi Kleen <andi@firstfloor.org>
> ---
> This patch was already sent on 2011-05-15. I thought Andrew had picked
> it up but apparently not, otherwise it should already be upstream by
> now.
I did merge it in May and have carried it in -mm (and hence in
linux-next) since then. I have sent it to Jesse at least twice, with
no effect.
I'll send it to Linus for 3.3-rc1, OK?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Make dmi_name_in_vendors more focused
2011-11-09 22:25 ` Andrew Morton
@ 2011-11-10 3:35 ` Jesse Barnes
2011-11-10 8:07 ` Jean Delvare
2011-11-10 8:13 ` Jean Delvare
1 sibling, 1 reply; 6+ messages in thread
From: Jesse Barnes @ 2011-11-10 3:35 UTC (permalink / raw)
To: Andrew Morton; +Cc: Jean Delvare, LKML, Andi Kleen
On Wed, 9 Nov 2011 14:25:57 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:
> On Fri, 4 Nov 2011 10:26:47 +0100
> Jean Delvare <khali@linux-fr.org> wrote:
>
> > The current implementation of dmi_name_in_vendors() is an invitation
> > to lazy coding and false positives [1]. Searching for a string in 8
> > different DMI fields is something nobody should ever need to do. You
> > know what you're looking for, so you should know where to look.
> > strstr isn't fast, especially when it fails, so we should avoid
> > calling it when it just can't succeed.
> >
> > Looking at the current users of the function, it seems clear to me
> > that they are looking for a system or board vendor name, so let's
> > limit dmi_name_in_vendors to these two DMI fields. This much better
> > matches the function name, BTW.
> >
> > [1] We currently have code looking for short names in DMI data, such
> > as "IBM" or "ASUS". I let you guess what will happen the day other
> > vendors ship products named, for example, "SCHREIBMEISTER" or
> > "PEGASUS".
> >
> > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> > Cc: Andi Kleen <andi@firstfloor.org>
> > ---
> > This patch was already sent on 2011-05-15. I thought Andrew had
> > picked it up but apparently not, otherwise it should already be
> > upstream by now.
>
> I did merge it in May and have carried it in -mm (and hence in
> linux-next) since then. I have sent it to Jesse at least twice, with
> no effect.
I don't generally look after DMI stuff... I can queue it though if you
want. My only worry is potential breakage for existing DMI matches,
but it looks like Jean already did the audit for that.
Jesse
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Make dmi_name_in_vendors more focused
2011-11-10 3:35 ` Jesse Barnes
@ 2011-11-10 8:07 ` Jean Delvare
0 siblings, 0 replies; 6+ messages in thread
From: Jean Delvare @ 2011-11-10 8:07 UTC (permalink / raw)
To: Jesse Barnes; +Cc: Andrew Morton, LKML, Andi Kleen
On Thu, 10 Nov 2011 04:35:20 +0100, Jesse Barnes wrote:
> On Wed, 9 Nov 2011 14:25:57 -0800
> Andrew Morton <akpm@linux-foundation.org> wrote:
>
> > On Fri, 4 Nov 2011 10:26:47 +0100
> > Jean Delvare <khali@linux-fr.org> wrote:
> > > This patch was already sent on 2011-05-15. I thought Andrew had
> > > picked it up but apparently not, otherwise it should already be
> > > upstream by now.
> >
> > I did merge it in May and have carried it in -mm (and hence in
> > linux-next) since then. I have sent it to Jesse at least twice, with
> > no effect.
>
> I don't generally look after DMI stuff...
Indeed I'm not sure why Jesse would take this, it's not PCI-related in
any way.
> I can queue it though if you
> want. My only worry is potential breakage for existing DMI matches,
> but it looks like Jean already did the audit for that.
Yes I did, both at the time of the original submission and again when
resending.
--
Jean Delvare
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Make dmi_name_in_vendors more focused
2011-11-09 22:25 ` Andrew Morton
2011-11-10 3:35 ` Jesse Barnes
@ 2011-11-10 8:13 ` Jean Delvare
2011-11-10 8:28 ` Andrew Morton
1 sibling, 1 reply; 6+ messages in thread
From: Jean Delvare @ 2011-11-10 8:13 UTC (permalink / raw)
To: Andrew Morton; +Cc: LKML, Andi Kleen, Jesse Barnes
On Wed, 9 Nov 2011 14:25:57 -0800, Andrew Morton wrote:
> On Fri, 4 Nov 2011 10:26:47 +0100
> Jean Delvare <khali@linux-fr.org> wrote:
>
> > The current implementation of dmi_name_in_vendors() is an invitation
> > to lazy coding and false positives [1]. Searching for a string in 8
> > different DMI fields is something nobody should ever need to do. You
> > know what you're looking for, so you should know where to look. strstr
> > isn't fast, especially when it fails, so we should avoid calling it
> > when it just can't succeed.
> >
> > Looking at the current users of the function, it seems clear to me
> > that they are looking for a system or board vendor name, so let's
> > limit dmi_name_in_vendors to these two DMI fields. This much better
> > matches the function name, BTW.
> >
> > [1] We currently have code looking for short names in DMI data, such
> > as "IBM" or "ASUS". I let you guess what will happen the day other
> > vendors ship products named, for example, "SCHREIBMEISTER" or "PEGASUS".
> >
> > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> > Cc: Andi Kleen <andi@firstfloor.org>
> > ---
> > This patch was already sent on 2011-05-15. I thought Andrew had picked
> > it up but apparently not, otherwise it should already be upstream by
> > now.
>
> I did merge it in May and have carried it in -mm (and hence in
> linux-next) since then. I have sent it to Jesse at least twice, with
> no effect.
>
> I'll send it to Linus for 3.3-rc1, OK?
If you want, sure. Note however that it might be less risky to send it
now, as I just audited the code. By 3.3-rc1, new callers might have
appeared and nobody may remember to audit again. Committing this kind
of change right after rc1 seems better than during the merge window, as
it lets us ensure that no new use case sneaked it.
--
Jean Delvare
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-11-10 8:23 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-11-04 9:26 [PATCH] Make dmi_name_in_vendors more focused Jean Delvare
2011-11-09 22:25 ` Andrew Morton
2011-11-10 3:35 ` Jesse Barnes
2011-11-10 8:07 ` Jean Delvare
2011-11-10 8:13 ` Jean Delvare
2011-11-10 8:28 ` Andrew Morton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox