linux-kernel - Re: [PATCH v2 1/3] hwmon: (acpi_power_meter) Fix uninitialized variables

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: <d1d2382f-f618-a318-579e-8318057fbf15@huawei.com>
Date: Thu, 9 Jan 2025 09:27:24 +0800
From: "lihuisong (C)" <lihuisong@...wei.com>
To: Guenter Roeck <linux@...ck-us.net>, <linux-hwmon@...r.kernel.org>
CC: <linux-kernel@...r.kernel.org>, <jdelvare@...e.com>,
	<liuyonglong@...wei.com>, <zhanjie9@...ilicon.com>, <zhenglifeng1@...wei.com>
Subject: Re: [PATCH v2 1/3] hwmon: (acpi_power_meter) Fix uninitialized
 variables


在 2025/1/8 22:45, Guenter Roeck 写道:
> On 1/7/25 03:33, Huisong Li wrote:
>> The 'power1_alarm' attribute uses the 'power' and 'cap' in the
>> acpi_power_meter_resource structure. Currently, these two fields are 
>> just
>> updated when user query 'power' and 'cap' attribute. If user directly 
>> query
>> the 'power1_alarm' attribute without queryng above two attributes, 
>> driver
>> will use uninitialized variables to judge.
>>
>> So this patch adds the setting of alarm state and update 'cap' in the
>> notification callback and update 'power' and 'cap' if needed to show the
>> real alarm state.
>>
>> Signed-off-by: Huisong Li <lihuisong@...wei.com>
>> ---
>>   drivers/hwmon/acpi_power_meter.c | 27 +++++++++++++++++++++++----
>>   1 file changed, 23 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/hwmon/acpi_power_meter.c 
>> b/drivers/hwmon/acpi_power_meter.c
>> index 2f1c9d97ad21..0b337d4fb212 100644
>> --- a/drivers/hwmon/acpi_power_meter.c
>> +++ b/drivers/hwmon/acpi_power_meter.c
>> @@ -84,6 +84,7 @@ struct acpi_power_meter_resource {
>>       u64        power;
>>       u64        cap;
>>       u64        avg_interval;
>> +    bool        power_alarm;
>>       int            sensors_valid;
>>       unsigned long        sensors_last_updated;
>>       struct sensor_device_attribute    sensors[NUM_SENSORS];
>> @@ -396,6 +397,9 @@ static ssize_t show_val(struct device *dev,
>>       struct acpi_device *acpi_dev = to_acpi_device(dev);
>>       struct acpi_power_meter_resource *resource = 
>> acpi_dev->driver_data;
>>       u64 val = 0;
>> +    int ret;
>> +
>> +    guard(mutex)(&resource->lock);
>>         switch (attr->index) {
>>       case 0:
>> @@ -423,10 +427,17 @@ static ssize_t show_val(struct device *dev,
>>               val = 0;
>>           break;
>>       case 6:
>> -        if (resource->power > resource->cap)
>> -            val = 1;
>> -        else
>> -            val = 0;
>> +        ret = update_meter(resource);
>> +        if (ret)
>> +            return ret;
>> +        /* need to update cap if not to support the notification. */
>> +        if (!(resource->caps.flags & POWER_METER_CAN_NOTIFY)) {
>> +            ret = update_cap(resource);
>> +            if (ret)
>> +                return ret;
>> +        }
>> +        val = resource->power_alarm || resource->power > resource->cap;
>> +        resource->power_alarm = resource->power > resource->cap ? 
>> true : false;
>
> The result of the comparison is already a boolean. ? : is unnecessary.
Right, will fix in v3.
>
>>           break;
>>       case 7:
>>       case 8:
>> @@ -847,12 +858,20 @@ static void acpi_power_meter_notify(struct 
>> acpi_device *device, u32 event)
>>           sysfs_notify(&device->dev.kobj, NULL, POWER_AVERAGE_NAME);
>>           break;
>>       case METER_NOTIFY_CAP:
>> +        mutex_lock(&resource->lock);
>> +        res = update_cap(resource);
>> +        if (res)
>> +            dev_err(&device->dev, "update cap failed when capping 
>> value is changed.\n");
>
> Please make this dev_err_once() to avoid logging noise if the problem
> is persistent.

will fix in v3.

Thanks.

>
>> + mutex_unlock(&resource->lock);
>>           sysfs_notify(&device->dev.kobj, NULL, POWER_CAP_NAME);
>>           break;
>>       case METER_NOTIFY_INTERVAL:
>>           sysfs_notify(&device->dev.kobj, NULL, 
>> POWER_AVG_INTERVAL_NAME);
>>           break;
>>       case METER_NOTIFY_CAPPING:
>> +        mutex_lock(&resource->lock);
>> +        resource->power_alarm = true;
>> +        mutex_unlock(&resource->lock);
>>           sysfs_notify(&device->dev.kobj, NULL, POWER_ALARM_NAME);
>>           dev_info(&device->dev, "Capping in progress.\n");
>>           break;
>
> .