some more optimizations#574
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds several hardening and behavior-gating changes across thermald’s config parsing, sysfs writes, D-Bus entrypoints, and Intel platform detection, along with a new RAPL “restore-on-exit” mechanism.
Changes:
- Add shared validation helpers (thermal object names, finite numeric ranges, validated config open) and apply them across D-Bus, engine, and zone user-config paths.
- Harden XML parsing by using a validated open path + restrictive libxml2 parse options and rejecting any config that includes a DTD.
- Tighten platform and power-control behavior: gate select Intel CPUs to adaptive mode and add RAPL limit restoration support; restrict sysfs writes to resolved paths under
/sys/.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/thermald.h | Adds global flag for adaptive perf enable. |
| src/main.cpp | Defines/sets adaptive_perf_enable when --adaptive is used. |
| src/thd_platform_intel.cpp | Adds adaptive-only CPU gating and removes /dev/mem MMIO workaround code. |
| src/thd_util.h | Declares new validation + validated XML-open helpers. |
| src/thd_util.cpp | Implements name validation, finite-range checks, and validated XML file opening. |
| src/thd_parse.cpp | Switches to validated XML FD open, restrictive parse flags, and DTD rejection. |
| src/thd_features_parse.cpp | Same validated XML FD open + restrictive parsing + DTD rejection for features config. |
| src/thd_cdev_order_parser.cpp | Same validated XML FD open + restrictive parsing + DTD rejection for cdev-order config. |
| src/thd_sys_fs.cpp | Adds validated sysfs write open that resolves paths and restricts writes to /sys. |
| src/thd_zone.cpp | Validates zone type names before using them to build runtime config filenames. |
| src/thd_engine.cpp | Validates user-provided names and virtual sensor parameters before acting. |
| src/thd_dbus_interface.cpp | Adds D-Bus input validation for names and virtual sensor numeric params. |
| src/thd_cdev_rapl.h | Adds private register_for_restoration() hook. |
| src/thd_cdev_rapl.cpp | Calls restoration registration during RAPL cdev update. |
| src/thd_cdev_rapl_restore.cpp | New file: global registry + atexit handler to restore RAPL limits. |
| Makefile.am | Adds new RAPL restore source file to build. |
| README.txt | Adds 2.5.12-rc1 release notes entry. |
| configure.ac | Updates version to 2.5.12-rc1. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1
to
+3
| /* | ||
| * cthd_cdev_rapl.cpp: thermal cooling class implementation | ||
| * using RAPL |
Comment on lines
+84
to
+105
| void cthd_sysfs_cdev_rapl::register_for_restoration() { | ||
| std::lock_guard<std::mutex> lock(registry_mutex); | ||
|
|
||
| // Register atexit handler on first call | ||
| if (!atexit_registered) { | ||
| std::atexit(restore_rapl_limits); | ||
| atexit_registered = true; | ||
| thd_log_info("Registered RAPL power limit restoration handler\n"); | ||
| } | ||
|
|
||
| rapl_restore_info info; | ||
| info.sysfs_path = cdev_sysfs.get_base_path(); | ||
| info.constraint_index = constraint_index; | ||
| info.power_limit = rapl_read_pl1(); | ||
| info.time_window = rapl_read_time_window(); | ||
| info.enable_status = rapl_read_enable_status(); | ||
|
|
||
| restore_registry.push_back(info); | ||
|
|
||
| thd_log_info("Registered RAPL %s: PL1=%d uW, window=%d us, enable=%d\n", | ||
| info.sysfs_path.c_str(), info.power_limit, info.time_window, info.enable_status); | ||
| } |
Comment on lines
+236
to
+237
| if (!is_valid_thermal_object_name(zone_name)) | ||
| return FALSE; |
Comment on lines
+256
to
+257
| if (!is_valid_thermal_object_name(zone_name)) | ||
| return FALSE; |
Comment on lines
295
to
+300
| g_assert(obj != nullptr); | ||
| if (!is_valid_thermal_object_name(name)) | ||
| return FALSE; | ||
| if (!is_valid_finite_value(slope, -1000.0, 1000.0) | ||
| || !is_valid_finite_value(intercept, -1000.0, 1000.0)) | ||
| return FALSE; |
Comment on lines
+492
to
+493
| if (!is_valid_thermal_object_name(zone_name)) | ||
| return FALSE; |
f1cd936 to
3b965d6
Compare
Comment on lines
114
to
+121
| extern std::unique_ptr<cthd_engine> thd_engine; | ||
| extern int thd_poll_interval; | ||
| extern bool thd_ignore_default_control; | ||
| extern bool workaround_enabled; | ||
| extern bool disable_active_power; | ||
| extern bool ignore_critical; | ||
| extern bool power_floor_enable; | ||
|
|
||
| extern bool adaptive_perf_enable; |
Comment on lines
+47
to
+51
| bool is_valid_thermal_object_name(const std::string &name); | ||
| bool is_valid_finite_value(double value, double min_val, double max_val); | ||
|
|
||
| int open_validated_xml_file(const std::string &filename, bool require_root_owner = true); | ||
|
|
Comment on lines
379
to
+383
| if (rapl_sysfs_valid()) | ||
| return THD_ERROR; | ||
|
|
||
| register_for_restoration(); | ||
|
|
Comment on lines
+39
to
+41
| static constexpr int thd_xml_parse_options = XML_PARSE_NONET | XML_PARSE_NOERROR | ||
| | XML_PARSE_NOWARNING; | ||
|
|
3b965d6 to
0211c85
Compare
Comment on lines
356
to
360
| if (adaptive) { | ||
| adaptive_perf_enable = true; | ||
| ret = thd_engine_create_adaptive_engine((bool) ignore_cpuid_check, (bool) test_mode); | ||
| if (ret != THD_SUCCESS) { | ||
| thd_log_info("--adaptive option failed on this platform\n"); |
Comment on lines
+43
to
+45
| int fd = ::open(resolved_path, O_WRONLY | O_NOFOLLOW); | ||
| if (fd < 0) | ||
| return -errno; |
Comment on lines
+46
to
+50
| void restore_rapl_limits() { | ||
| std::lock_guard<std::mutex> lock(registry_mutex); | ||
|
|
||
| thd_log_info("Restoring %zu RAPL power limits on exit\n", | ||
| restore_registry.size()); |
0211c85 to
306e1a3
Compare
Comment on lines
+46
to
+55
| void restore_rapl_limits() { | ||
| std::unique_lock<std::mutex> lock(registry_mutex, std::try_to_lock); | ||
| if (!lock.owns_lock()) | ||
| return; | ||
|
|
||
| thd_log_info("Restoring %zu RAPL power limits on exit\n", | ||
| restore_registry.size()); | ||
|
|
||
| for (const auto& info : restore_registry) { | ||
| csys_fs sysfs(info.sysfs_path); |
Comment on lines
83
to
+87
| std::string p = base_path + path; | ||
| int fd = ::open(p.c_str(), O_WRONLY | O_NOFOLLOW); | ||
| int fd = open_validated_sysfs_write_fd(p); | ||
| if (fd < 0) { | ||
| thd_log_info("sysfs write failed %s\n", p.c_str()); | ||
| return -errno; | ||
| thd_log_info("sysfs write failed %s\n", path.c_str()); | ||
| return fd; |
Comment on lines
89
to
93
| int ret = ::write(fd, buf.c_str(), buf.size()); | ||
| if (ret < 0) { | ||
| ret = -errno; | ||
| thd_log_info("sysfs write failed %s\n", p.c_str()); | ||
| thd_log_info("sysfs write failed %s\n", path.c_str()); | ||
| } |
Comment on lines
101
to
+105
| std::string p = base_path + path; | ||
| int fd = ::open(p.c_str(), O_WRONLY | O_NOFOLLOW); | ||
| int fd = open_validated_sysfs_write_fd(p); | ||
| if (fd < 0) { | ||
| thd_log_info("sysfs write failed %s\n", p.c_str()); | ||
| return -errno; | ||
| thd_log_info("sysfs write failed %s\n", path.c_str()); | ||
| return fd; |
Comment on lines
107
to
110
| if (::lseek(fd, position, SEEK_CUR) == -1) { | ||
| thd_log_info("sysfs write failed %s\n", p.c_str()); | ||
| thd_log_info("sysfs write failed %s\n", path.c_str()); | ||
| close(fd); | ||
| return -errno; |
Comment on lines
112
to
115
| int ret = ::write(fd, &data, sizeof(data)); | ||
| if (ret < 0) | ||
| thd_log_info("sysfs write failed %s\n", p.c_str()); | ||
| thd_log_info("sysfs write failed %s\n", path.c_str()); | ||
| close(fd); |
Contributor
Author
There was a problem hiding this comment.
This was intentional. Don't comment again.
Capture current RAPL PL1 power, time window and enabled status at startup and restore on exit. Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
--adaptive option is enabled for a while. For newer platform only support this mode starting from Lunar Lake. Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
We have proper driver interface. The direct /dev/mem access is not required. Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
306e1a3 to
82ca371
Compare
Add an API open_validated_xml_file(), which can be used by all XML files opens. Also add API to sanitize zone names, which can only contain [A-Za-z0-9_.- ' '] Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
82ca371 to
f3b92ae
Compare
Comment on lines
+39
to
+46
| doc = xmlReadFile(filename.c_str(), nullptr, 0); | ||
| int fd = open_validated_xml_file(filename); | ||
| if (fd < 0) | ||
| return THD_ERROR; |
Comment on lines
+39
to
+41
| static constexpr int thd_xml_parse_options = XML_PARSE_NONET | XML_PARSE_NOERROR | ||
| | XML_PARSE_NOWARNING; | ||
|
|
Comment on lines
42
to
46
| typedef struct { | ||
| unsigned int family; | ||
| unsigned int model; | ||
| unsigned int adaptive_only; | ||
| } supported_ids_t; |
Use open_validated_xml_file() and check for XML files without DTD. Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Check for canonicalized sysfs paths rooted under /sys. Also reduce full path name prints in errors. Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Use is_valid_thermal_object_name() to check for valid ASCII file names. Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Add new files to Android build. Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Refer to README.txt for details. Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
f3b92ae to
3944dcf
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.