| 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: <CAJZ5v0hroRuGQ5N42Z8=yFVXiJPdid3wJrHoKqr2BZVx=sfnBQ@mail.gmail.com>
Date: Sat, 27 Jul 2019 15:10:00 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Stephen Boyd <swboyd@...omium.org>
Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Linux PM <linux-pm@...r.kernel.org>,
Tri Vo <trong@...roid.com>,
Kalesh Singh <kaleshsingh@...gle.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Ravi Chandra Sadineni <ravisadineni@...omium.org>
Subject: Re: [PATCH] PM / wakeup: Avoid dev_name collisions in wakeup class
On Sat, Jul 27, 2019 at 3:11 AM Stephen Boyd <swboyd@...omium.org> wrote:
>
> If a device is wakeup capable and the driver calls device_wakeup_init()
> on it during probe and then userspace writes 'enabled' to that device's
> power/wakeup file in sysfs we'll try to create the same named wakeup
> device in sysfs. The kernel will complain about duplicate file names.
>
> sysfs: cannot create duplicate filename '/devices/virtual/wakeup/1-1.1'
> kobject_add_internal failed for 1-1.1 with -EEXIST, don't try to register things with the same name in the same directory.
>
> It may be advantageous to not write 'enabled' to the wakeup file (see
> wakeup_store()) from userspace for these devices because we allocate
> devices and register them and then throw them all away later on if the
> device driver has already initialized the wakeup attribute. The
> implementation currently tries to avoid taking locks here so it seems
> best to optimize that path in a separate patch.
>
> Let's rename the wakeup class devices as 'wakeupN' with an IDA that's
> simple enough to just return some sort of number. In addition, let's
> make the device registering the wakeup the parent and include a 'name'
> attribute in case userspace wants to figure out the type of wakeup it is
> (in the case of virtual wakeups) or the device associated with the
> wakeup. This makes it easier for userspace to go from /sys/class/wakeup
> to a place in the device hierarchy where the wakeup is generated from
> like an input device.
>
> Cc: Tri Vo <trong@...roid.com>
> Cc: Kalesh Singh <kaleshsingh@...gle.com>
> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Cc: Ravi Chandra Sadineni <ravisadineni@...omium.org>
> Signed-off-by: Stephen Boyd <swboyd@...omium.org>
I'd rather change the commit that introduced this issue which is only
in linux-next for now.
> ---
> drivers/acpi/device_pm.c | 2 +-
> drivers/base/power/wakeup.c | 8 +++++---
> drivers/base/power/wakeup_stats.c | 31 ++++++++++++++++++++++++++-----
> fs/eventpoll.c | 4 ++--
> include/linux/pm_wakeup.h | 12 ++++++++----
> kernel/power/autosleep.c | 2 +-
> kernel/power/wakelock.c | 2 +-
> kernel/time/alarmtimer.c | 2 +-
> 8 files changed, 45 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> index 28cffaaf9d82..0863be1e42d6 100644
> --- a/drivers/acpi/device_pm.c
> +++ b/drivers/acpi/device_pm.c
> @@ -495,7 +495,7 @@ acpi_status acpi_add_pm_notifier(struct acpi_device *adev, struct device *dev,
> goto out;
>
> mutex_lock(&acpi_pm_notifier_lock);
> - adev->wakeup.ws = wakeup_source_register(dev_name(&adev->dev));
> + adev->wakeup.ws = wakeup_source_register(&adev->dev, dev_name(&adev->dev));
> adev->wakeup.context.dev = dev;
> adev->wakeup.context.func = func;
> adev->wakeup.flags.notifier_present = true;
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index 2b8def0ea59f..7ba242b49831 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -201,15 +201,17 @@ EXPORT_SYMBOL_GPL(wakeup_source_remove);
> /**
> * wakeup_source_register - Create wakeup source and add it to the list.
> * @name: Name of the wakeup source to register.
> + * @dev: Device wakeup source is associated with (or NULL if virtual)
> */
> -struct wakeup_source *wakeup_source_register(const char *name)
> +struct wakeup_source *wakeup_source_register(struct device *dev,
> + const char *name)
> {
> struct wakeup_source *ws;
> int ret;
>
> ws = wakeup_source_create(name);
> if (ws) {
> - ret = wakeup_source_sysfs_add(ws);
> + ret = wakeup_source_sysfs_add(dev, ws);
> if (ret) {
> kfree_const(ws->name);
> kfree(ws);
> @@ -273,7 +275,7 @@ int device_wakeup_enable(struct device *dev)
> if (pm_suspend_target_state != PM_SUSPEND_ON)
> dev_dbg(dev, "Suspicious %s() during system transition!\n", __func__);
>
> - ws = wakeup_source_register(dev_name(dev));
> + ws = wakeup_source_register(dev, dev_name(dev));
> if (!ws)
> return -ENOMEM;
>
> diff --git a/drivers/base/power/wakeup_stats.c b/drivers/base/power/wakeup_stats.c
> index 9c01150f1213..927cc84d3392 100644
> --- a/drivers/base/power/wakeup_stats.c
> +++ b/drivers/base/power/wakeup_stats.c
> @@ -7,8 +7,9 @@
> * Copyright (c) 2019 Google Inc.
> */
>
> -#include <linux/slab.h>
> +#include <linux/idr.h>
> #include <linux/kdev_t.h>
> +#include <linux/slab.h>
>
> #include "power.h"
>
> @@ -80,6 +81,15 @@ static ssize_t last_change_ms_show(struct device *dev,
> }
> static DEVICE_ATTR_RO(last_change_ms);
>
> +static ssize_t name_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct wakeup_source *ws = dev_get_drvdata(dev);
> +
> + return sprintf(buf, "%s\n", ws->name);
> +}
> +static DEVICE_ATTR_RO(name);
> +
> static ssize_t prevent_suspend_time_ms_show(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> @@ -96,6 +106,7 @@ static ssize_t prevent_suspend_time_ms_show(struct device *dev,
> static DEVICE_ATTR_RO(prevent_suspend_time_ms);
>
> static struct attribute *wakeup_source_attrs[] = {
> + &dev_attr_name.attr,
> &dev_attr_active_count.attr,
> &dev_attr_event_count.attr,
> &dev_attr_wakeup_count.attr,
> @@ -109,18 +120,27 @@ static struct attribute *wakeup_source_attrs[] = {
> };
> ATTRIBUTE_GROUPS(wakeup_source);
>
> +static DEFINE_IDA(wakeup_ida);
> +
> /**
> * wakeup_source_sysfs_add - Add wakeup_source attributes to sysfs.
> * @ws: Wakeup source to be added in sysfs.
> + * @parent: Device wakeup source is associated with (or NULL if virtual)
> */
> -int wakeup_source_sysfs_add(struct wakeup_source *ws)
> +int wakeup_source_sysfs_add(struct device *parent, struct wakeup_source *ws)
> {
> struct device *dev;
>
> - dev = device_create_with_groups(wakeup_class, NULL, MKDEV(0, 0), ws,
> - wakeup_source_groups, "%s", ws->name);
> - if (IS_ERR(dev))
> + ws->id = ida_simple_get(&wakeup_ida, 0, 0, GFP_KERNEL);
> + if (ws->id < 0)
> + return ws->id;
> +
> + dev = device_create_with_groups(wakeup_class, parent, MKDEV(0, 0), ws,
> + wakeup_source_groups, "wakeup%d", ws->id);
> + if (IS_ERR(dev)) {
> + ida_simple_remove(&wakeup_ida, ws->id);
> return PTR_ERR(dev);
> + }
>
> ws->dev = dev;
> return 0;
> @@ -134,6 +154,7 @@ EXPORT_SYMBOL_GPL(wakeup_source_sysfs_add);
> void wakeup_source_sysfs_remove(struct wakeup_source *ws)
> {
> device_unregister(ws->dev);
> + ida_simple_remove(&wakeup_ida, ws->id);
> }
> EXPORT_SYMBOL_GPL(wakeup_source_sysfs_remove);
>
> diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> index d7f1f5011fac..c4159bcc05d9 100644
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
> @@ -1459,13 +1459,13 @@ static int ep_create_wakeup_source(struct epitem *epi)
> struct wakeup_source *ws;
>
> if (!epi->ep->ws) {
> - epi->ep->ws = wakeup_source_register("eventpoll");
> + epi->ep->ws = wakeup_source_register(NULL, "eventpoll");
> if (!epi->ep->ws)
> return -ENOMEM;
> }
>
> name = epi->ffd.file->f_path.dentry->d_name.name;
> - ws = wakeup_source_register(name);
> + ws = wakeup_source_register(NULL, name);
>
> if (!ws)
> return -ENOMEM;
> diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h
> index 500f9cfe2db8..822e74f45384 100644
> --- a/include/linux/pm_wakeup.h
> +++ b/include/linux/pm_wakeup.h
> @@ -41,6 +41,7 @@ struct wake_irq;
> */
> struct wakeup_source {
> const char *name;
> + int id;
> struct list_head entry;
> spinlock_t lock;
> struct wake_irq *wakeirq;
> @@ -88,7 +89,8 @@ extern struct wakeup_source *wakeup_source_create(const char *name);
> extern void wakeup_source_destroy(struct wakeup_source *ws);
> extern void wakeup_source_add(struct wakeup_source *ws);
> extern void wakeup_source_remove(struct wakeup_source *ws);
> -extern struct wakeup_source *wakeup_source_register(const char *name);
> +extern struct wakeup_source *wakeup_source_register(struct device *dev,
> + const char *name);
> extern void wakeup_source_unregister(struct wakeup_source *ws);
> extern int device_wakeup_enable(struct device *dev);
> extern int device_wakeup_disable(struct device *dev);
> @@ -128,7 +130,8 @@ static inline void wakeup_source_add(struct wakeup_source *ws) {}
>
> static inline void wakeup_source_remove(struct wakeup_source *ws) {}
>
> -static inline struct wakeup_source *wakeup_source_register(const char *name)
> +static inline struct wakeup_source *wakeup_source_register(struct device *dev,
> + const char *name)
> {
> return NULL;
> }
> @@ -186,12 +189,13 @@ static inline void pm_wakeup_dev_event(struct device *dev, unsigned int msec,
> #ifdef CONFIG_PM_SLEEP_STATS
>
> /* drivers/base/power/wakeup_stats.c */
> -int wakeup_source_sysfs_add(struct wakeup_source *ws);
> +int wakeup_source_sysfs_add(struct device *parent, struct wakeup_source *ws);
> void wakeup_source_sysfs_remove(struct wakeup_source *ws);
>
> #else /* !CONFIG_PM_SLEEP_STATS */
>
> -static inline int wakeup_source_sysfs_add(struct wakeup_source *ws)
> +static inline int wakeup_source_sysfs_add(struct device *parent,
> + struct wakeup_source *ws)
> {
> return 0;
> }
> diff --git a/kernel/power/autosleep.c b/kernel/power/autosleep.c
> index 41e83a779e19..9af5a50d3489 100644
> --- a/kernel/power/autosleep.c
> +++ b/kernel/power/autosleep.c
> @@ -116,7 +116,7 @@ int pm_autosleep_set_state(suspend_state_t state)
>
> int __init pm_autosleep_init(void)
> {
> - autosleep_ws = wakeup_source_register("autosleep");
> + autosleep_ws = wakeup_source_register(NULL, "autosleep");
> if (!autosleep_ws)
> return -ENOMEM;
>
> diff --git a/kernel/power/wakelock.c b/kernel/power/wakelock.c
> index 32726da3d6e6..826fcd97647a 100644
> --- a/kernel/power/wakelock.c
> +++ b/kernel/power/wakelock.c
> @@ -192,7 +192,7 @@ static struct wakelock *wakelock_lookup_add(const char *name, size_t len,
> wl->ws.name = wl->name;
> wl->ws.last_time = ktime_get();
>
> - ret = wakeup_source_sysfs_add(&wl->ws);
> + ret = wakeup_source_sysfs_add(NULL, &wl->ws);
> if (ret) {
> kfree(wl->name);
> kfree(wl);
> diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
> index 57518efc3810..93b382d9701c 100644
> --- a/kernel/time/alarmtimer.c
> +++ b/kernel/time/alarmtimer.c
> @@ -97,7 +97,7 @@ static int alarmtimer_rtc_add_device(struct device *dev,
> if (!device_may_wakeup(rtc->dev.parent))
> return -1;
>
> - __ws = wakeup_source_register("alarmtimer");
> + __ws = wakeup_source_register(dev, "alarmtimer");
>
> spin_lock_irqsave(&rtcdev_lock, flags);
> if (!rtcdev) {
>
> base-commit: 0c826a07dd696d99784c68ec1e8def4399cc4a0b
> --
> Sent by a computer through tubes
>
Powered by blists - more mailing lists