| 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
| ||
|
Message-ID: <4499f49f-e846-66c6-3017-deb5bc63c31f@huawei.com>
Date: Wed, 11 Dec 2024 15:41:44 +0800
From: "lihuisong (C)" <lihuisong@...wei.com>
To: Guenter Roeck <linux@...ck-us.net>
CC: <jdelvare@...e.com>, <liuyonglong@...wei.com>, <zhanjie9@...ilicon.com>,
<zhenglifeng1@...wei.com>, <linux-kernel@...r.kernel.org>,
<linux-hwmon@...r.kernel.org>
Subject: Re: [PATCH v1 1/4] hwmon: (acpi_power_meter) Fix using uninitialized
variables
Hi Guenter,
Can you take a look at my following reply? Looking forward to your reply.
/Huisong
在 2024/11/27 11:43, lihuisong (C) 写道:
> Hi Guenter，
>
> How about the modification as below? But driver doesn't know what the
> time is to set resource->power_alarm to false.
>
> 在 2024/11/27 0:19, Guenter Roeck 写道:
>> On 11/25/24 23:03, lihuisong (C) wrote:
>>>
>>> 在 2024/11/26 12:04, Guenter Roeck 写道:
>>>> On 11/25/24 17:56, lihuisong (C) wrote:
>>>>> Hi Guente,
>>>>>
>>>>> Thanks for your timely review.
>>>>>
>>>>> 在 2024/11/26 0:03, Guenter Roeck 写道:
>>>>>> On 11/25/24 01:34, Huisong Li wrote:
>>>>>>> The 'power1_alarm' attribute uses the 'power' and 'cap' in the
>>>>>>> acpi_power_meter_resource structure. However, these two fields
>>>>>>> are just
>>>>>>> updated when user query 'power' and 'cap' attribute, or hardware
>>>>>>> enforced
>>>>>>> limit. If user directly query the 'power1_alarm' attribute
>>>>>>> without queryng
>>>>>>> above two attributes, driver will use the uninitialized
>>>>>>> variables to judge.
>>>>>>> In addition, the 'power1_alarm' attribute needs to update power
>>>>>>> and cap to
>>>>>>> show the real state.
>>>>>>>
>>>>>>> Signed-off-by: Huisong Li <lihuisong@...wei.com>
>>>>>>> ---
>>>>>>> drivers/hwmon/acpi_power_meter.c | 10 ++++++++++
>>>>>>> 1 file changed, 10 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/hwmon/acpi_power_meter.c
>>>>>>> b/drivers/hwmon/acpi_power_meter.c
>>>>>>> index 2f1c9d97ad21..4c3314e35d30 100644
>>>>>>> --- a/drivers/hwmon/acpi_power_meter.c
>>>>>>> +++ b/drivers/hwmon/acpi_power_meter.c
>>>>>>> @@ -396,6 +396,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,6 +426,13 @@ static ssize_t show_val(struct device *dev,
>>>>>>> val = 0;
>>>>>>> break;
>>>>>>> case 6:
>>>>>>> + ret = update_meter(resource);
>>>>>>> + if (ret)
>>>>>>> + return ret;
>>>>>>> + ret = update_cap(resource);
>>>>>>> + if (ret)
>>>>>>> + return ret;
>>>>>>> +
>>>>>>> if (resource->power > resource->cap)
>>>>>>> val = 1;
>>>>>>> else
>>>>>>
>>>>>>
>>>>>> While technically correct, the implementation of this attribute
>>>>>> defeats its
>>>>>> purpose. It is supposed to reflect the current status as reported
>>>>>> by the
>>>>>> hardware. A real fix would be to use the associated notification
>>>>>> to set or
>>>>>> reset a status flag, and to report the current value of that flag
>>>>>> as reported
>>>>>> by the hardware.
>>>>> I know what you mean.
>>>>> The Notify(power_meter, 0x83) is supposed to meet your proposal IIUC.
>>>>> It's good, but it depands on hardware support notification.
>>>>>>
>>>>>> If there is no notification support, the attribute should not
>>>>>> even exist,
>>>>>> unless there is a means to retrieve its value from ACPI (the
>>>>>> status itself,
>>>>>> not by comparing temperature values).
>>>>> Currently, the 'power1_alarm' attribute is created just when
>>>>> platform support the power meter meassurement(bit0 of the
>>>>> supported capabilities in _PMC).
>>>>> And it doesn't see if the platform support notifications.
>>>>> From the current implementation of this driver, this sysfs can
>>>>> also reflect the status by comparing power and cap,
>>>>> which is good to the platform that support hardware limit from
>>>>> some out-of-band mechanism but doesn't support any notification.
>>>>>
>>>>
>>>> The point is that this can also be done from userspace. Hardware
>>>> monitoring drivers
>>>> are supposed to provide hardware attributes, not software
>>>> attributes derived from it.
>>>>
>>> So this 'power1_alarm' attribute can be exposed when platform
>>> supports hardware enforced limit and notifcations when the hardware
>>> limit is enforced, right?
>>> If so, we have to change the condition that driver creates this
>>> sysfs interface.
>>
>> This isn't about enforcing anything, it is about reporting an alarm
>> if the power consumed exceeds the maximum configured.
>>
> -->
>
> index 2f1c9d97ad21..b436ebd863e6
> --- 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,21 @@ static ssize_t show_val(struct device *dev,
> val = 0;
> break;
> case 6:
> - if (resource->power > resource->cap)
> - val = 1;
> - else
> - val = 0;
> + /* report alarm status based on the notification if
> support. */
> + if (resource->caps.flags & POWER_METER_CAN_NOTIFY) {
> + val = resource->power_alarm;
> + } else {
> + ret = update_meter(resource);
> + if (ret)
> + return ret;
> + ret = update_cap(resource);
> + if (ret)
> + return ret;
> + if (resource->power > resource->cap)
> + val = 1;
> + else
> + val = 0;
> + }
> break;
> case 7:
> case 8:
> @@ -853,6 +868,7 @@ static void acpi_power_meter_notify(struct
> acpi_device *device, u32 event)
> sysfs_notify(&device->dev.kobj, NULL,
> POWER_AVG_INTERVAL_NAME);
> break;
> case METER_NOTIFY_CAPPING:
> + resource->power_alarm = true;
> sysfs_notify(&device->dev.kobj, NULL, POWER_ALARM_NAME);
> dev_info(&device->dev, "Capping in progress.\n");
> break;
>
>> .
>
> .
Powered by blists - more mailing lists