Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964873AbbDPIqW (ORCPT ); Thu, 16 Apr 2015 04:46:22 -0400 Received: from aserp1040.oracle.com ([141.146.126.69]:26228 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752642AbbDPIqP (ORCPT ); Thu, 16 Apr 2015 04:46:15 -0400 Message-ID: <552F76E6.8030104@oracle.com> Date: Thu, 16 Apr 2015 16:46:30 +0800 From: Zhenzhong Duan Reply-To: zhenzhong.duan@oracle.com Organization: Oracle User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Jean Delvare CC: LKML , Ben Hutchings , Artem Savkov , Ivan Khoronzhuk , Matt Fleming Subject: Re: [PATCH] firmware: dmi_scan: Fix ordering of product_uuid References: <20150415110222.194867c1@endymion.delvare> <552F5535.9010302@oracle.com> <1429168164.4386.12.camel@chaos.site> In-Reply-To: <1429168164.4386.12.camel@chaos.site> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-Source-IP: userv0021.oracle.com [156.151.31.71] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5915 Lines: 141 On 2015/4/16 15:09, Jean Delvare wrote: > Hi zduan, > > Thanks for your reply. > > Le Thursday 16 April 2015 à 14:22 +0800, Zhenzhong Duan a écrit : >> On 2015/4/15 17:02, Jean Delvare wrote: >>> In function dmi_present(), dmi_walk_early() calls dmi_table(), which >>> calls dmi_decode(), which ultimately calls dmi_save_uuid(). This last >>> function makes a decision based on the value of global variable >>> dmi_ver. The problem is that this variable is set right _after_ >>> dmi_walk_early() returns. So dmi_save_uuid() always sees dmi_ver == 0 >>> regardless of the actual version implemented. >>> >>> This causes /sys/class/dmi/id/product_uuid to always use the old >>> ordering even on systems implementing DMI/SMBIOS 2.6 or later, which >>> should use the new ordering. >>> >>> This is broken since kernel v3.8 for legacy DMI implementations and >>> since kernel v3.10 for SMBIOS 2 implementations. SMBIOS 3 >>> implementations with the 64-bit entry point are not affected. >>> >>> The first breakage does not matter much as in practice legacy DMI >>> implementations are always for versions older than 2.6, which is when >>> the UUID ordering changed. The second breakage is more problematic as >>> it affects the vast majority of x86 systems manufactured since 2009. >>> >>> Signed-off-by: Jean Delvare >>> Fixes: 9f9c9cbb6057 ("drivers/firmware/dmi_scan.c: fetch dmi version from SMBIOS if it exists") >> I think above line should be removed as dmi_ver is set before >> dmi_walk_early with the commit, see below clip. >> We did get right UUID order with SMBIOS 2.6 per customer test. > I bet your customers tested only with recent SMBIOS implementations that > have the _SM_ entry point. They did not test on systems with only legacy > _DMI_ entry points. As I said above, odds are that such systems would > implement a version of the specification older than 2.6 anyway, so the > bug wouldn't trigger. In fact, I have considered the fact that legacy DMI is always older than 2.6 when design. > > I agree that 9f9c9cbb6057 is not problematic in practice and this is why > I wrote that the fix is only needed for kernels v3.10+, not v3.8+. But I > think it is still interesting to document the first commit which > introduced the bug. I'm pretty sure that the second faulty commit would > not have been faulty if the first commit had been correct. After all, > that second commit aligned the _SM_ code path on the _DMI_ code path, > without realizing that the latter had a bug. Ok, just keep it > >> +static int __init smbios_present(const char __iomem *p) >> +{ >> + u8 buf[32]; >> + int offset = 0; >> + >> + memcpy_fromio(buf, p, 32); >> + if ((buf[5] < 32) && dmi_checksum(buf, buf[5])) { >> + dmi_ver = (buf[6] << 8) + buf[7]; > But look at the _DMI_ code path: > > static int __init dmi_present(const char __iomem *p) > { > (...) > if (dmi_walk_early(dmi_decode) == 0) { > if (dmi_ver) > pr_info("SMBIOS %d.%d present.\n", > dmi_ver >> 8, dmi_ver & 0xFF); > else { > dmi_ver = (buf[14] & 0xF0) << 4 | > (buf[14] & 0x0F); > pr_info("Legacy DMI %d.%d present.\n", > dmi_ver >> 8, dmi_ver & 0xFF); > } > dmi_dump_ids(); > return 0; > } > > Here dmi_ver may be set _after_ dmi_walk_early is called. > >>> Fixes: 79bae42d51a5 ("dmi_scan: refactor dmi_scan_machine(), {smbios,dmi}_present()") >>> Cc: Zhenzhong Duan >>> Cc: Ben Hutchings >>> Cc: Artem Savkov >>> Cc: Ivan Khoronzhuk >>> Cc: Matt Fleming >>> Cc: stable@vger.kernel.org [v3.10+] >>> --- >>> drivers/firmware/dmi_scan.c | 7 ++++--- >>> 1 file changed, 4 insertions(+), 3 deletions(-) >>> >>> --- linux-4.0.orig/drivers/firmware/dmi_scan.c 2015-04-13 00:12:50.000000000 +0200 >>> +++ linux-4.0/drivers/firmware/dmi_scan.c 2015-04-15 10:24:37.556994240 +0200 >>> @@ -499,18 +499,19 @@ static int __init dmi_present(const u8 * >>> buf += 16; >>> >>> if (memcmp(buf, "_DMI_", 5) == 0 && dmi_checksum(buf, 15)) { >>> + if (smbios_ver) >>> + dmi_ver = smbios_ver; >>> + else >>> + dmi_ver = (buf[14] & 0xF0) << 4 | (buf[14] & 0x0F); >>> dmi_num = get_unaligned_le16(buf + 12); >>> dmi_len = get_unaligned_le16(buf + 6); >>> dmi_base = get_unaligned_le32(buf + 8); >>> >>> if (dmi_walk_early(dmi_decode) == 0) { >>> if (smbios_ver) { >>> - dmi_ver = smbios_ver; >>> pr_info("SMBIOS %d.%d present.\n", >>> dmi_ver >> 8, dmi_ver & 0xFF); >>> } else { >>> - dmi_ver = (buf[14] & 0xF0) << 4 | >>> - (buf[14] & 0x0F); >>> pr_info("Legacy DMI %d.%d present.\n", >>> dmi_ver >> 8, dmi_ver & 0xFF); >>> } >>> >>> >> The basic idea is right, but you ignore the case dmi_walk_early may >> fail, though looks impossible when bootup. >> >> Better to add below for robust. >> >> @@ -521,6 +521,6 @@ static int __init dmi_present(const u8 * >> >> return 0; >> } >> } >> + dmi_ver = 0; >> return 1; >> } >> > What is the value of this? dmi_ver will never be accessed after this > point anyway, as far as I can see. Same as above, future commit may not realize you bring this faulty when they want to use dmi_ver. zduan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/