linux-kernel - Re: [PATCH] powercap: RAPL: Add Power Limit4 support for Meteor Lake SoC

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]

Message-ID: <28ead36b-2d9e-1a36-6f4e-04684e420260@intel.com>
Date:   Thu, 11 May 2023 07:55:08 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     Sumeet Pawnikar <sumeet.r.pawnikar@...el.com>, rafael@...nel.org,
        srinivas.pandruvada@...ux.intel.com, rui.zhang@...el.com,
        linux-pm@...r.kernel.org
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH] powercap: RAPL: Add Power Limit4 support for Meteor Lake
 SoC

On 2/15/23 04:32, Sumeet Pawnikar wrote:
> diff --git a/drivers/powercap/intel_rapl_msr.c b/drivers/powercap/intel_rapl_msr.c
> index bc6adda58883..a27673706c3d 100644
> --- a/drivers/powercap/intel_rapl_msr.c
> +++ b/drivers/powercap/intel_rapl_msr.c
> @@ -143,6 +143,8 @@ static const struct x86_cpu_id pl4_support_ids[] = {
>  	{ X86_VENDOR_INTEL, 6, INTEL_FAM6_ALDERLAKE_N, X86_FEATURE_ANY },
>  	{ X86_VENDOR_INTEL, 6, INTEL_FAM6_RAPTORLAKE, X86_FEATURE_ANY },
>  	{ X86_VENDOR_INTEL, 6, INTEL_FAM6_RAPTORLAKE_P, X86_FEATURE_ANY },
> +	{ X86_VENDOR_INTEL, 6, INTEL_FAM6_METEORLAKE, X86_FEATURE_ANY },
> +	{ X86_VENDOR_INTEL, 6, INTEL_FAM6_METEORLAKE_L, X86_FEATURE_ANY },
>  	{}
>  };

Sumeet, could you _please_ go take a close look at 'struct x86_cpu_id'?

> struct x86_cpu_id {
>         __u16 vendor;
>         __u16 family;
>         __u16 model;
>         __u16 steppings;
>         __u16 feature;  /* bit index */
>         kernel_ulong_t driver_data;
> };

You might also want to very carefully count the fields in the structure.
 Which field is being initialized to X86_FEATURE_ANY?  Is it:

 a. ->feature
 b. ->steppings
 c. ->model

How could this _possibly_ work, you ask yourself?  Well, you lucked out:

#define X86_FAMILY_ANY   0
#define X86_MODEL_ANY    0
#define X86_STEPPING_ANY 0
#define X86_FEATURE_ANY  0

so, you actually accidentally *explicitly* specified a 0 for ->steppings
*AND* accidentally *implicitly* specified a 0 for ->feature.

... and you did this in at least five separate commits over four years.

Why does this matter?  Because some hapless maintainer might take your
code, copy it, and then s/X86_FEATURE_ANY/X86_FEATURE_FOO/ and then
scratch their head for an hour as to why it doesn't work.

Could you please fix this up?  As penance, you could even fix the _ANY
defines so that people can't do this accidentally any longer.