| 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: <1402fe0a-13ff-4e6c-bcea-660acb383177@huawei.com>
Date: Thu, 4 Dec 2025 16:29:08 +0800
From: "zhangpengjie (A)" <zhangpengjie2@...wei.com>
To: Jie Zhan <zhanjie9@...ilicon.com>, <myungjoo.ham@...sung.com>,
<kyungmin.park@...sung.com>, <cw00.choi@...sung.com>
CC: <linux-pm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<zhenglifeng1@...wei.com>, <lihuisong@...wei.com>, <yubowen8@...wei.com>,
<linhongye@...artners.com>, <linuxarm@...wei.com>,
<jonathan.cameron@...wei.com>
Subject: Re: [PATCH v3] PM / devfreq: use _visible attribute to replace
create/remove_sysfs_files()
hello，Jie
On 12/4/2025 3:36 PM, Jie Zhan wrote:
> On 11/7/2025 11:17 AM, Pengjie Zhang wrote:
>> Previously, non-generic attributes (polling_interval, timer) used separate
>> create/delete logic, leading to race conditions during concurrent access in
>> creation/deletion. Multi-threaded operations also caused inconsistencies
>> between governor capabilities and attribute states.
>>
>> 1.Use is_visible + sysfs_update_group() to unify management of these
>> attributes, eliminating creation/deletion races.
>> 2.Add locks and validation to these attributes, ensuring consistency
>> between current governor capabilities and attribute operations in
>> multi-threaded environments.
>>
>> Signed-off-by: Pengjie Zhang <zhangpengjie2@...wei.com>
> Hi Pengjie,
>
> The motivation looks fine but the implementation of gov_group_visible() and
> gov_attr_visible() doesn't work right in my view.
>
> See comments below.
>
> Thanks!
> Jie
>> /*
>> * devfreq core provides delayed work based load monitoring helper
>> @@ -785,11 +786,6 @@ static void devfreq_dev_release(struct device *dev)
>> kfree(devfreq);
>> }
>>
>>
>> static ssize_t polling_interval_show(struct device *dev,
>> struct device_attribute *attr, char *buf)
>> {
>> struct devfreq *df = to_devfreq(dev);
>> + int ret;
> No need of this if restore the last line.
>>
>> - if (!df->profile)
>> + guard(mutex)(&devfreq_list_lock);
>> +
>> + if (!df->profile || !df->governor ||
>> + !IS_SUPPORTED_ATTR(df->governor->attrs, POLLING_INTERVAL))
>> return -EINVAL;
>>
>> - return sprintf(buf, "%d\n", df->profile->polling_ms);
> It's fine to keep this line.
>> + ret = sprintf(buf, "%d\n", df->profile->polling_ms);
>> +
>> + return ret;
>> }
>>
>>
>>
>> -/* Create the specific sysfs files which depend on each governor. */
>> -static void create_sysfs_files(struct devfreq *devfreq,
>> - const struct devfreq_governor *gov)
>> +static umode_t gov_attr_visible(struct kobject *kobj,
>> + struct attribute *attr, int n)
>> {
>> - if (IS_SUPPORTED_ATTR(gov->attrs, POLLING_INTERVAL))
>> - CREATE_SYSFS_FILE(devfreq, polling_interval);
>> - if (IS_SUPPORTED_ATTR(gov->attrs, TIMER))
>> - CREATE_SYSFS_FILE(devfreq, timer);
>> + struct device *dev = kobj_to_dev(kobj);
>> + struct devfreq *df = to_devfreq(dev);
>> +
>> + if (!df->governor || !df->governor->attrs)
>> + return 0;
>> +
>> + if (IS_SUPPORTED_ATTR(df->governor->attrs, POLLING_INTERVAL))
>> + return attr->mode;
>> + if (IS_SUPPORTED_ATTR(df->governor->attrs, TIMER))
>> + return attr->mode;
> This would expose both 'timer' and 'polling_interval' if either of them is
> supported, which is wrong.
ok，i see，may be
if (attr == &dev_attr_polling_interval.attr) {
if (IS_SUPPORTED_ATTR(df->governor->attrs, POLLING_INTERVAL))
return attr->mode;
return 0;
}
if (attr == &dev_attr_timer.attr) {
if (IS_SUPPORTED_ATTR(df->governor->attrs, TIMER))
return attr->mode;
return 0;
}
>> +
>> + return 0;
>> }
>>
>> -/* Remove the specific sysfs files which depend on each governor. */
>> -static void remove_sysfs_files(struct devfreq *devfreq,
>> - const struct devfreq_governor *gov)
>> +static bool gov_group_visible(struct kobject *kobj)
>> {
>> - if (IS_SUPPORTED_ATTR(gov->attrs, POLLING_INTERVAL))
>> - sysfs_remove_file(&devfreq->dev.kobj,
>> - &dev_attr_polling_interval.attr);
>> - if (IS_SUPPORTED_ATTR(gov->attrs, TIMER))
>> - sysfs_remove_file(&devfreq->dev.kobj, &dev_attr_timer.attr);
>> + struct device *dev = kobj_to_dev(kobj);
>> +
>> + if (!dev)
>> + return false;
> kobj_to_dev(kobj) can't fail. This check is unnecessary.
>> +
>> + return true;
>> }
>> +DEFINE_SYSFS_GROUP_VISIBLE(gov);
> The implementation of gov_group_visible() and gov_attr_visible() doesn't
> seem to comply with the design of DEFINE_SYSFS_GROUP_VISIBLE().
>
> DEFINE_SYSFS_GROUP_VISIBLE is defined as:
>
> #define DEFINE_SYSFS_GROUP_VISIBLE(name) \
> static inline umode_t sysfs_group_visible_##name( \
> struct kobject *kobj, struct attribute *attr, int n) \
> { \
> if (n == 0 && !name##_group_visible(kobj)) \
> return SYSFS_GROUP_INVISIBLE; \
> return name##_attr_visible(kobj, attr, n); \
> }
>
> That means:
> _group_visible controls whether to hide all the attrs in this group;
> _attr_visible further decides whether to show each attr.
>
> Hence,
> 1. gov_group_visible() should check if any attr in 'governor_attrs' should
> be present, i.e. checking df->governor->attrs.
> 2. gov_attr_visible identifies which attr it is and checks whether the
> governor supports it.
>
> Neither is done in the above code, so this should be updated.
we can move the check|if (!df->governor || !df->governor->attrs)|
from|gov_attr_visible|to|gov_group_visible|.
thanks !
Powered by blists - more mailing lists