| 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: <CAJZ5v0i8HNj5ztnt2K=QqL6eCUCEFbcDgAt2AWWhG4UwHL_EuQ@mail.gmail.com>
Date: Tue, 14 Jan 2025 18:13:48 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Kurt Borja <kuurtb@...il.com>, platform-driver-x86@...r.kernel.org,
"Rafael J. Wysocki" <rafael@...nel.org>, Len Brown <lenb@...nel.org>, linux-acpi@...r.kernel.org,
LKML <linux-kernel@...r.kernel.org>,
Mario Limonciello <mario.limonciello@....com>, Armin Wolf <W_Armin@....de>,
Joshua Grisham <josh@...huagrisham.com>, "Derek J. Clark" <derekjohn.clark@...il.com>,
Hans de Goede <hdegoede@...hat.com>, Maximilian Luz <luzmaximilian@...il.com>,
"Lee, Chun-Yi" <jlee@...e.com>, Shyam Sundar S K <Shyam-sundar.S-k@....com>,
Corentin Chary <corentin.chary@...il.com>, "Luke D. Jones" <luke@...nes.dev>,
Lyndon Sanche <lsanche@...deno.ca>, Ike Panhc <ike.pan@...onical.com>,
Henrique de Moraes Holschuh <hmh@....eng.br>, Mark Pearson <mpearson-lenovo@...ebb.ca>,
Alexis Belmonte <alexbelm48@...il.com>, Ai Chao <aichao@...inos.cn>, Gergo Koteles <soyer@....hu>,
Dell.Client.Kernel@...l.com, ibm-acpi-devel@...ts.sourceforge.net
Subject: Re: [PATCH v2 15/18] ACPI: platform_profile: Remove
platform_profile_handler from exported symbols
On Tue, Jan 14, 2025 at 5:55 PM Ilpo Järvinen
<ilpo.jarvinen@...ux.intel.com> wrote:
>
> On Tue, 14 Jan 2025, Kurt Borja wrote:
>
> > In order to protect the platform_profile_handler from API consumers,
> > allocate it in platform_profile_register() and modify it's signature
> > accordingly.
> >
> > Remove the platform_profile_handler from all consumer drivers and
> > replace them with a pointer to the class device, which is
> > now returned from platform_profile_register().
> >
> > Replace *pprof with a pointer to the class device in the rest of
> > exported symbols.
> >
> > Signed-off-by: Kurt Borja <kuurtb@...il.com>
> > ---
> > drivers/acpi/platform_profile.c | 87 ++++++++++++-------
> > .../surface/surface_platform_profile.c | 11 ++-
> > drivers/platform/x86/acer-wmi.c | 18 ++--
> > drivers/platform/x86/amd/pmf/pmf.h | 2 +-
> > drivers/platform/x86/amd/pmf/sps.c | 17 ++--
> > drivers/platform/x86/asus-wmi.c | 20 ++---
> > drivers/platform/x86/dell/alienware-wmi.c | 9 +-
> > drivers/platform/x86/dell/dell-pc.c | 22 ++---
> > drivers/platform/x86/hp/hp-wmi.c | 19 ++--
> > drivers/platform/x86/ideapad-laptop.c | 14 +--
> > .../platform/x86/inspur_platform_profile.c | 9 +-
> > drivers/platform/x86/thinkpad_acpi.c | 14 ++-
> > include/linux/platform_profile.h | 12 ++-
> > 13 files changed, 125 insertions(+), 129 deletions(-)
> >
> > diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> > index 34e22b006ccc..2fae5e2fc962 100644
> > --- a/drivers/acpi/platform_profile.c
> > +++ b/drivers/acpi/platform_profile.c
> > @@ -4,6 +4,7 @@
> >
> > #include <linux/acpi.h>
> > #include <linux/bits.h>
> > +#include <linux/cleanup.h>
> > #include <linux/init.h>
> > #include <linux/kdev_t.h>
> > #include <linux/mutex.h>
> > @@ -213,9 +214,17 @@ static struct attribute *profile_attrs[] = {
> > };
> > ATTRIBUTE_GROUPS(profile);
> >
> > +static void pprof_device_release(struct device *dev)
> > +{
> > + struct platform_profile_handler *pprof = to_pprof_handler(dev);
> > +
> > + kfree(pprof);
> > +}
> > +
> > static const struct class platform_profile_class = {
> > .name = "platform-profile",
> > .dev_groups = profile_groups,
> > + .dev_release = pprof_device_release,
> > };
> >
> > /**
> > @@ -409,10 +418,10 @@ static const struct attribute_group platform_profile_group = {
> > .is_visible = profile_class_is_visible,
> > };
> >
> > -void platform_profile_notify(struct platform_profile_handler *pprof)
> > +void platform_profile_notify(struct device *dev)
> > {
> > scoped_cond_guard(mutex_intr, return, &profile_lock) {
> > - _notify_class_profile(&pprof->class_dev, NULL);
> > + _notify_class_profile(dev, NULL);
> > }
> > sysfs_notify(acpi_kobj, NULL, "platform_profile");
> > }
> > @@ -461,40 +470,51 @@ int platform_profile_cycle(void)
> > }
> > EXPORT_SYMBOL_GPL(platform_profile_cycle);
> >
> > -int platform_profile_register(struct platform_profile_handler *pprof, void *drvdata)
> > +struct device *platform_profile_register(struct device *dev, const char *name,
> > + void *drvdata,
> > + const struct platform_profile_ops *ops)
> > {
> > + int minor;
> > int err;
> >
> > - /* Sanity check the profile handler */
> > - if (!pprof || !pprof->ops->profile_set || !pprof->ops->profile_get ||
> > - !pprof->ops->probe) {
> > + /* Sanity check */
> > + if (!dev || !name || !ops || !ops->profile_get ||
> > + !ops->profile_set || !ops->probe) {
> > pr_err("platform_profile: handler is invalid\n");
> > - return -EINVAL;
> > + return ERR_PTR(-EINVAL);
> > }
> >
> > - err = pprof->ops->probe(drvdata, pprof->choices);
> > + struct platform_profile_handler *pprof __free(kfree) = kzalloc(
> > + sizeof(*pprof), GFP_KERNEL);
> > + if (!pprof)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + err = ops->probe(drvdata, pprof->choices);
> > if (err < 0)
> > - return err;
> > + return ERR_PTR(err);
> >
> > if (bitmap_empty(pprof->choices, PLATFORM_PROFILE_LAST)) {
> > pr_err("platform_profile: no available profiles\n");
> > - return -EINVAL;
> > + return ERR_PTR(-EINVAL);
> > }
> >
> > guard(mutex)(&profile_lock);
> >
> > /* create class interface for individual handler */
> > - pprof->minor = ida_alloc(&platform_profile_ida, GFP_KERNEL);
> > - if (pprof->minor < 0)
> > - return pprof->minor;
> > + minor = ida_alloc(&platform_profile_ida, GFP_KERNEL);
> > + if (minor < 0)
> > + return ERR_PTR(minor);
> >
> > + pprof->name = name;
> > + pprof->ops = ops;
> > + pprof->minor = minor;
> > pprof->class_dev.class = &platform_profile_class;
> > - pprof->class_dev.parent = pprof->dev;
> > + pprof->class_dev.parent = dev;
> > dev_set_drvdata(&pprof->class_dev, drvdata);
> > dev_set_name(&pprof->class_dev, "platform-profile-%d", pprof->minor);
> > err = device_register(&pprof->class_dev);
> > if (err) {
> > - put_device(&pprof->class_dev);
> > + put_device(&no_free_ptr(pprof)->class_dev);
> > goto cleanup_ida;
> > }
> >
> > @@ -504,20 +524,21 @@ int platform_profile_register(struct platform_profile_handler *pprof, void *drvd
> > if (err)
> > goto cleanup_cur;
> >
> > - return 0;
> > + return &no_free_ptr(pprof)->class_dev;
> >
> > cleanup_cur:
> > - device_unregister(&pprof->class_dev);
> > + device_unregister(&no_free_ptr(pprof)->class_dev);
>
> I don't like how this is architected.
>
> IMO, no_free_ptr() should not appear on error/rollback paths. The pointer
> is going to be freed despite the code just told it's not going to be
> freed, which sends conflicting signals. Obviously, it is because this
> function has relinquished its ownership of the pointer but as is it seems
> a dangerous/confusing pattern.
I agree.
Powered by blists - more mailing lists