linux-kernel - Re: [Regression] cached max_state breaks ACPI processor cooling device

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: <1d21b45a453518cfbabd827d46c14bd7ce9ef9d7.camel@intel.com>
Date:   Sat, 25 Feb 2023 05:34:28 +0000
From:   "Zhang, Rui" <rui.zhang@...el.com>
To:     "viresh.kumar@...aro.org" <viresh.kumar@...aro.org>,
        "Wysocki, Rafael J" <rafael.j.wysocki@...el.com>,
        "daniel.lezcano@...aro.org" <daniel.lezcano@...aro.org>
CC:     "srinivas.pandruvada@...ux.intel.com" 
        <srinivas.pandruvada@...ux.intel.com>,
        "Wang, Quanxian" <quanxian.wang@...el.com>,
        "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [Regression] cached max_state breaks ACPI processor cooling
 device

On Sat, 2023-02-25 at 13:29 +0800, Zhang Rui wrote:
> Hi, All,
> 
> Starting from commit c408b3d1d9bb("thermal: Validate new state in
> cur_state_store()") and commit a365105c685c("thermal: sysfs: Reuse
> cdev->max_state"), the cdev->get_max_state() is evaluated only once
> during cooling device registration.
> 
> This is done to fix the below Smatch static checker warning:
> 	drivers/thermal/thermal_sysfs.c:656
> thermal_cooling_device_stats_update()
> 	warn: potential integer overflow from user 'stats->state *
> stats->max_states + new_state'
> reported here https://lore.kernel.org/all/Y0ltRJRjO7AkawvE@kili/.
> 
> But this actually breaks cooling devices which could have dynamic max
> cooling state, like ACPI processor cooling device.
> 
> acpi_processor_driver_init
> 	driver_register(&acpi_processor_driver);
> 		acpi_processor_start
> 			acpi_processor_thermal_init
> 				thermal_cooling_device_register
> 					processor_get_max_state
> 	acpi_processor_cpufreq_init = true
> The driver doesn't count cpufreq as cooling state until
> acpi_processor_cpufreq_init is set later.
> 
> As a result, without the commits above,
> /sys/class/thermal/cooling_device10/cur_state:0
> /sys/class/thermal/cooling_device10/max_state:3
> /sys/class/thermal/cooling_device10/type:Processor
> after the commits above, 
> /sys/class/thermal/cooling_device10/cur_state:0
> /sys/class/thermal/cooling_device10/max_state:0
> /sys/class/thermal/cooling_device10/type:Processor

Forgot to mention that this problem is

Reported-by: Wang, Quanxian <quanxian.wang@...el.com>

thanks,
rui
> 
> In order to fix this, there are something worth clarification IMO.
> 1. should we support dynamic max_state or not?
>    ACPI processor cooling state is a combination of processor p-
> states
>    and t-states.
>    t-states are static, but p-states can vary depends on processor 
>    frequency driver loaded or not.
>    I'm not sure if there is any other user like this, but still this
>    is a valid use case of dynamic max_state.
> 2. how to handle dynamic max_state for cooling device statistics in
>    sysfs?
>    IMO, when max_state changes, the definition of previous cooling
>    device is changed as well, thus we should abandon the previous
>    statistics and restart counting.
> 3. anything else needs to be handled when max_state changes?
>    Say, as the definition of each cooling state is changed when
>    max_state changes, should we invalidate and re-update the
>    thermal instances of this cdev in each thermal zone device?
> 4. how to detect/handle max cooling states changes?
>    Should we do this as before, which invokes .get_max_state()
>    everywhere and do updates when necessary , or
>    a. cache max_state like we do today
>    b. introduce a new API for max_state change
>    c. invoke the new API in the cooling device driver explicitly when
>       max_state changes
>    d. update cached max_state value, statistics sysfs and
>       thermal_instances in the API
>    e. remove .get_max_state() callback as we register and update the
>       max_state with a fixed value each time.
> 
> thanks,
> rui
> 
> 
>