| 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: <20251208133126.00006756@huawei.com>
Date: Mon, 8 Dec 2025 13:31:26 +0000
From: Jonathan Cameron <jonathan.cameron@...wei.com>
To: Pengjie Zhang <zhangpengjie2@...wei.com>, <linuxarm@...wei.com>
CC: <myungjoo.ham@...sung.com>, <kyungmin.park@...sung.com>,
<cw00.choi@...sung.com>, <linux-pm@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <zhanjie9@...ilicon.com>,
<zhenglifeng1@...wei.com>, <lihuisong@...wei.com>, <yubowen8@...wei.com>,
<linhongye@...artners.com>, <jonathan.cameron@...wei.com>,
<wangzhi12@...wei.com>
Subject: Re: [PATCH v4] PM / devfreq: use _visible attribute to replace
create/remove_sysfs_files()
On Fri, 5 Dec 2025 16:37:24 +0800
Pengjie Zhang <zhangpengjie2@...wei.com> 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,
Been a while since I looked at this series (sorry about that!)
A few trivial things inline and one suggestion for a possible follow up
patch.
None are significant enough that I'll look again at this so:
Reviewed-by: Jonathan Cameron <jonathan.cameron@...wei.com>
> ---
> changes in v4:
> -Remove the DEFINE_SYSFS_GROUP_VISIBLE macro and directly set the is_visible function.
> -Remove unnecessary ret variables
> Link to v3:https://lore.kernel.org/lkml/20251107031706.1698396-1-zhangpengjie2@huawei.com/
>
> changes in v3:
> - Use guard() to simplify lock acquisition and destruction.
> - Eliminate redundant checks for df.
> Link to v2:https://lore.kernel.org/lkml/20251028022458.2824872-1-zhangpengjie2@huawei.com/
>
> Changes in v2:
> - Fix one problem reported by the kernel test robot.
> - Redirect all error paths in timer_store() to out to ensure locks are not
> left unReleased.
> Link to v1:https://lore.kernel.org/lkml/20251025135238.3576861-1-zhangpengjie2@huawei.com/
>
> drivers/devfreq/devfreq.c | 99 +++++++++++++++++++++++----------------
> 1 file changed, 58 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 2e8d01d47f69..afcfc1be2e64 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -38,6 +38,7 @@
>
> static struct class *devfreq_class;
> static struct dentry *devfreq_debugfs;
> +static const struct attribute_group gov_attr_group;
>
> /*
> * devfreq core provides delayed work based load monitoring helper
> @@ -785,11 +786,6 @@ static void devfreq_dev_release(struct device *dev)
> kfree(devfreq);
> }
>
> -static void create_sysfs_files(struct devfreq *devfreq,
> - const struct devfreq_governor *gov);
> -static void remove_sysfs_files(struct devfreq *devfreq,
> - const struct devfreq_governor *gov);
> -
> /**
> * devfreq_add_device() - Add devfreq feature to the device
> * @dev: the device to add devfreq feature.
> @@ -956,7 +952,10 @@ struct devfreq *devfreq_add_device(struct device *dev,
> __func__);
> goto err_init;
> }
> - create_sysfs_files(devfreq, devfreq->governor);
> +
> + err = sysfs_update_group(&devfreq->dev.kobj, &gov_attr_group);
> + if (err)
> + goto err_init;
>
> list_add(&devfreq->node, &devfreq_list);
>
> @@ -998,9 +997,7 @@ int devfreq_remove_device(struct devfreq *devfreq)
> if (devfreq->governor) {
This file adopts (at least in first case I looked at) convention of no {}
when a single call is made in if blocks (even when it is multiline).
Coding standard has never been particularly clear on this corner case
so normally I'd advise following local style.
> devfreq->governor->event_handler(devfreq,
> DEVFREQ_GOV_STOP, NULL);
> - remove_sysfs_files(devfreq, devfreq->governor);
> }
> -
Technically an unrelated change that I'd be temped to undo just to slightly
reduce patch size.
> device_unregister(&devfreq->dev);
>
> return 0;
> @@ -1460,7 +1457,6 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
> __func__, df->governor->name, ret);
> goto out;
> }
> - remove_sysfs_files(df, df->governor);
>
> /*
> * Start the new governor and create the specific sysfs files
> @@ -1489,7 +1485,7 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
> * Create the sysfs files for the new governor. But if failed to start
> * the new governor, restore the sysfs files of previous governor.
> */
> - create_sysfs_files(df, df->governor);
> + ret = sysfs_update_group(&df->dev.kobj, &gov_attr_group);
Not directly relevant to this patch, but this function smells like one
that would significantly benefit from guard() and early returns in error paths.
>
> out:
> mutex_unlock(&devfreq_list_lock);
> @@ -1807,14 +1803,16 @@ static struct attribute *devfreq_attrs[] = {
> &dev_attr_trans_stat.attr,
> NULL,
> };
> @@ -1847,7 +1848,10 @@ static ssize_t timer_show(struct device *dev,
> {
> struct devfreq *df = to_devfreq(dev);
>
> - if (!df->profile)
> + guard(mutex)(&devfreq_list_lock);
For these I'd be tempted to add a trivial comment about the race they
are protecting against. I think it is the window between setting
the IS_SUPPORTED_ATTR() and isvisible being called.
> +
> + if (!df->profile || !df->governor ||
> + !IS_SUPPORTED_ATTR(df->governor->attrs, TIMER))
> return -EINVAL;
>
> return sprintf(buf, "%s\n", timer_name[df->profile->timer]);
> @@ -1861,7 +1865,10 @@ static ssize_t timer_store(struct device *dev, struct device_attribute *attr,
> int timer = -1;
> int ret = 0, i;
>
> - if (!df->governor || !df->profile)
> + guard(mutex)(&devfreq_list_lock);
> +
> + if (!df->governor || !df->profile ||
> + !IS_SUPPORTED_ATTR(df->governor->attrs, TIMER))
> return -EINVAL;
>
> ret = sscanf(buf, "%16s", str_timer);
Powered by blists - more mailing lists