linux-kernel - Re: [PATCH v1 1/4] dmi: Introduce dmi_get_bios

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 27 Feb 2018 10:14:22 +0100
From:   Jean Delvare <jdelvare@...e.de>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Bjorn Helgaas <bhelgaas@...gle.com>, linux-pci@...r.kernel.org,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        linux-acpi@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/4] dmi: Introduce dmi_get_bios_year() helper

Hi Andy,

On Thu, 22 Feb 2018 14:59:20 +0200, Andy Shevchenko wrote:
> It's used in several places and more users may come.
> By using this helper they may create a slightly cleaner code.

In general I'm not a big fan of arbitrary API shortcuts. I won't object
if you think it's a good one to have, however I'm a bit concerned about
the silent handling of the "error case", which could cause unexpected
default decisions as seen in patch 2/4 of this series. The fact that
dmi_get_bios_year() returns 0 if there is no DMI BIOS date provided
should at least be documented, to limit the risk.

I also wonder if dmi_get_date() itself couldn't be optimized a bit if
it turns out that most callers are only interested in the year.
Currently it will parse the whole string even if the caller isn't
interested in the month and day.

The fact that dmi_get_date() returns true even if it couldn't parse the
date string at all is also strange, although unrelated with your
current work.

> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> ---
>  include/linux/dmi.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/include/linux/dmi.h b/include/linux/dmi.h
> index 46e151172d95..241c27008c70 100644
> --- a/include/linux/dmi.h
> +++ b/include/linux/dmi.h
> @@ -147,4 +147,11 @@ static inline const struct dmi_system_id *
>  
>  #endif
>  
> +static inline int dmi_get_bios_year(void)
> +{
> +	int year;
> +	dmi_get_date(DMI_BIOS_DATE, &year, NULL, NULL);
> +	return year;
> +}

checkpatch complains:

WARNING: Missing a blank line after declarations
#61: FILE: include/linux/dmi.h:153:
+	int year;
+	dmi_get_date(DMI_BIOS_DATE, &year, NULL, NULL);

And I would tend to agree. Just because it is an inline function in a
header file doesn't mean we don't stick to the usual coding style
policy.

> +
>  #endif	/* __DMI_H__ */

-- 
Jean Delvare
SUSE L3 Support