| 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: <20231103061522.1268637-9-andrew@codeconstruct.com.au>
Date: Fri, 3 Nov 2023 16:45:20 +1030
From: Andrew Jeffery <andrew@...econstruct.com.au>
To: minyard@....org, openipmi-developer@...ts.sourceforge.net
Cc: Andrew Jeffery <andrew@...econstruct.com.au>,
linux-kernel@...r.kernel.org, Jonathan.Cameron@...wei.com,
aladyshev22@...il.com, jk@...econstruct.com.au
Subject: [PATCH 08/10] ipmi: kcs_bmc: Track clients in core
I ran out of spoons before I could come up with a better client tracking
scheme back in the original refactoring series:
https://lore.kernel.org/all/20210608104757.582199-1-andrew@aj.id.au/
Jonathan prodded Konstantin about the issue in a review of Konstantin's
MCTP patches[1], prompting an attempt to clean it up.
[1]: https://lore.kernel.org/all/20230929120835.0000108e@Huawei.com/
Prevent client modules from having to track their own instances by
requiring they return a pointer to a client object from their
add_device() implementation. We can then track this in the core, and
provide it as the argument to the remove_device() implementation to save
the client module from further work. The usual container_of() pattern
gets the client module access to its private data.
Signed-off-by: Andrew Jeffery <andrew@...econstruct.com.au>
---
drivers/char/ipmi/kcs_bmc.c | 68 ++++++++++++++++-----------
drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 42 ++++-------------
drivers/char/ipmi/kcs_bmc_client.h | 35 ++++++++++----
drivers/char/ipmi/kcs_bmc_device.h | 4 +-
drivers/char/ipmi/kcs_bmc_serio.c | 43 +++++------------
5 files changed, 88 insertions(+), 104 deletions(-)
diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
index d70e503041bd..203cb73faa91 100644
--- a/drivers/char/ipmi/kcs_bmc.c
+++ b/drivers/char/ipmi/kcs_bmc.c
@@ -19,6 +19,7 @@
static DEFINE_MUTEX(kcs_bmc_lock);
static LIST_HEAD(kcs_bmc_devices);
static LIST_HEAD(kcs_bmc_drivers);
+static LIST_HEAD(kcs_bmc_clients);
/* Consumer data access */
@@ -129,25 +130,27 @@ void kcs_bmc_disable_device(struct kcs_bmc_client *client)
}
EXPORT_SYMBOL(kcs_bmc_disable_device);
-int kcs_bmc_add_device(struct kcs_bmc_device *kcs_bmc)
+int kcs_bmc_add_device(struct kcs_bmc_device *dev)
{
+ struct kcs_bmc_client *client;
struct kcs_bmc_driver *drv;
int error = 0;
- int rc;
- spin_lock_init(&kcs_bmc->lock);
- kcs_bmc->client = NULL;
+ spin_lock_init(&dev->lock);
+ dev->client = NULL;
mutex_lock(&kcs_bmc_lock);
- list_add(&kcs_bmc->entry, &kcs_bmc_devices);
+ list_add(&dev->entry, &kcs_bmc_devices);
list_for_each_entry(drv, &kcs_bmc_drivers, entry) {
- rc = drv->ops->add_device(kcs_bmc);
- if (!rc)
- continue;
-
- dev_err(kcs_bmc->dev, "Failed to add chardev for KCS channel %d: %d",
- kcs_bmc->channel, rc);
- error = rc;
+ client = drv->ops->add_device(drv, dev);
+ if (IS_ERR(client)) {
+ error = PTR_ERR(client);
+ dev_err(dev->dev,
+ "Failed to add chardev for KCS channel %d: %d",
+ dev->channel, error);
+ break;
+ }
+ list_add(&client->entry, &kcs_bmc_clients);
}
mutex_unlock(&kcs_bmc_lock);
@@ -155,31 +158,37 @@ int kcs_bmc_add_device(struct kcs_bmc_device *kcs_bmc)
}
EXPORT_SYMBOL(kcs_bmc_add_device);
-void kcs_bmc_remove_device(struct kcs_bmc_device *kcs_bmc)
+void kcs_bmc_remove_device(struct kcs_bmc_device *dev)
{
- struct kcs_bmc_driver *drv;
+ struct kcs_bmc_client *curr, *tmp;
mutex_lock(&kcs_bmc_lock);
- list_del(&kcs_bmc->entry);
- list_for_each_entry(drv, &kcs_bmc_drivers, entry) {
- drv->ops->remove_device(kcs_bmc);
+ list_for_each_entry_safe(curr, tmp, &kcs_bmc_clients, entry) {
+ if (curr->dev == dev) {
+ list_del(&curr->entry);
+ curr->drv->ops->remove_device(curr);
+ }
}
+ list_del(&dev->entry);
mutex_unlock(&kcs_bmc_lock);
}
EXPORT_SYMBOL(kcs_bmc_remove_device);
void kcs_bmc_register_driver(struct kcs_bmc_driver *drv)
{
- struct kcs_bmc_device *kcs_bmc;
- int rc;
+ struct kcs_bmc_client *client;
+ struct kcs_bmc_device *dev;
mutex_lock(&kcs_bmc_lock);
list_add(&drv->entry, &kcs_bmc_drivers);
- list_for_each_entry(kcs_bmc, &kcs_bmc_devices, entry) {
- rc = drv->ops->add_device(kcs_bmc);
- if (rc)
- dev_err(kcs_bmc->dev, "Failed to add driver for KCS channel %d: %d",
- kcs_bmc->channel, rc);
+ list_for_each_entry(dev, &kcs_bmc_devices, entry) {
+ client = drv->ops->add_device(drv, dev);
+ if (IS_ERR(client)) {
+ dev_err(dev->dev, "Failed to add driver for KCS channel %d: %ld",
+ dev->channel, PTR_ERR(client));
+ continue;
+ }
+ list_add(&client->entry, &kcs_bmc_clients);
}
mutex_unlock(&kcs_bmc_lock);
}
@@ -187,13 +196,16 @@ EXPORT_SYMBOL(kcs_bmc_register_driver);
void kcs_bmc_unregister_driver(struct kcs_bmc_driver *drv)
{
- struct kcs_bmc_device *kcs_bmc;
+ struct kcs_bmc_client *curr, *tmp;
mutex_lock(&kcs_bmc_lock);
- list_del(&drv->entry);
- list_for_each_entry(kcs_bmc, &kcs_bmc_devices, entry) {
- drv->ops->remove_device(kcs_bmc);
+ list_for_each_entry_safe(curr, tmp, &kcs_bmc_clients, entry) {
+ if (curr->drv == drv) {
+ list_del(&curr->entry);
+ drv->ops->remove_device(curr);
+ }
}
+ list_del(&drv->entry);
mutex_unlock(&kcs_bmc_lock);
}
EXPORT_SYMBOL(kcs_bmc_unregister_driver);
diff --git a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
index 98f231f24c26..9fca31f8c7c2 100644
--- a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
+++ b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
@@ -71,8 +71,6 @@ enum kcs_ipmi_errors {
#define KCS_ZERO_DATA 0
struct kcs_bmc_ipmi {
- struct list_head entry;
-
struct kcs_bmc_client client;
spinlock_t lock;
@@ -462,27 +460,24 @@ static const struct file_operations kcs_bmc_ipmi_fops = {
.unlocked_ioctl = kcs_bmc_ipmi_ioctl,
};
-static DEFINE_SPINLOCK(kcs_bmc_ipmi_instances_lock);
-static LIST_HEAD(kcs_bmc_ipmi_instances);
-
-static int kcs_bmc_ipmi_add_device(struct kcs_bmc_device *kcs_bmc)
+static struct kcs_bmc_client *
+kcs_bmc_ipmi_add_device(struct kcs_bmc_driver *drv, struct kcs_bmc_device *dev)
{
struct kcs_bmc_ipmi *priv;
int rc;
priv = kzalloc(sizeof(*priv), GFP_KERNEL);
if (!priv)
- return -ENOMEM;
+ return ERR_PTR(ENOMEM);
spin_lock_init(&priv->lock);
mutex_init(&priv->mutex);
init_waitqueue_head(&priv->queue);
- priv->client.dev = kcs_bmc;
- priv->client.ops = &kcs_bmc_ipmi_client_ops;
+ kcs_bmc_client_init(&priv->client, &kcs_bmc_ipmi_client_ops, drv, dev);
priv->miscdev.minor = MISC_DYNAMIC_MINOR;
- priv->miscdev.name = kasprintf(GFP_KERNEL, "%s%u", DEVICE_NAME, kcs_bmc->channel);
+ priv->miscdev.name = kasprintf(GFP_KERNEL, "%s%u", DEVICE_NAME, dev->channel);
if (!priv->miscdev.name) {
rc = -ENOMEM;
goto cleanup_priv;
@@ -496,13 +491,9 @@ static int kcs_bmc_ipmi_add_device(struct kcs_bmc_device *kcs_bmc)
goto cleanup_miscdev_name;
}
- spin_lock_irq(&kcs_bmc_ipmi_instances_lock);
- list_add(&priv->entry, &kcs_bmc_ipmi_instances);
- spin_unlock_irq(&kcs_bmc_ipmi_instances_lock);
+ pr_info("Initialised IPMI client for channel %d\n", dev->channel);
- pr_info("Initialised IPMI client for channel %d\n", kcs_bmc->channel);
-
- return 0;
+ return &priv->client;
cleanup_miscdev_name:
kfree(priv->miscdev.name);
@@ -510,25 +501,12 @@ static int kcs_bmc_ipmi_add_device(struct kcs_bmc_device *kcs_bmc)
cleanup_priv:
kfree(priv);
- return rc;
+ return ERR_PTR(rc);
}
-static void kcs_bmc_ipmi_remove_device(struct kcs_bmc_device *kcs_bmc)
+static void kcs_bmc_ipmi_remove_device(struct kcs_bmc_client *client)
{
- struct kcs_bmc_ipmi *priv = NULL, *pos;
-
- spin_lock_irq(&kcs_bmc_ipmi_instances_lock);
- list_for_each_entry(pos, &kcs_bmc_ipmi_instances, entry) {
- if (pos->client.dev == kcs_bmc) {
- priv = pos;
- list_del(&pos->entry);
- break;
- }
- }
- spin_unlock_irq(&kcs_bmc_ipmi_instances_lock);
-
- if (!priv)
- return;
+ struct kcs_bmc_ipmi *priv = client_to_kcs_bmc_ipmi(client);
misc_deregister(&priv->miscdev);
kcs_bmc_disable_device(&priv->client);
diff --git a/drivers/char/ipmi/kcs_bmc_client.h b/drivers/char/ipmi/kcs_bmc_client.h
index 1251e9669bcd..1c6c812d6edc 100644
--- a/drivers/char/ipmi/kcs_bmc_client.h
+++ b/drivers/char/ipmi/kcs_bmc_client.h
@@ -8,16 +8,7 @@
#include "kcs_bmc.h"
-struct kcs_bmc_driver_ops {
- int (*add_device)(struct kcs_bmc_device *kcs_bmc);
- void (*remove_device)(struct kcs_bmc_device *kcs_bmc);
-};
-
-struct kcs_bmc_driver {
- struct list_head entry;
-
- const struct kcs_bmc_driver_ops *ops;
-};
+struct kcs_bmc_driver;
struct kcs_bmc_client_ops {
irqreturn_t (*event)(struct kcs_bmc_client *client);
@@ -26,7 +17,31 @@ struct kcs_bmc_client_ops {
struct kcs_bmc_client {
const struct kcs_bmc_client_ops *ops;
+ struct kcs_bmc_driver *drv;
struct kcs_bmc_device *dev;
+ struct list_head entry;
+};
+
+struct kcs_bmc_driver_ops {
+ struct kcs_bmc_client *(*add_device)(struct kcs_bmc_driver *drv,
+ struct kcs_bmc_device *dev);
+ void (*remove_device)(struct kcs_bmc_client *client);
+};
+
+static inline void kcs_bmc_client_init(struct kcs_bmc_client *client,
+ const struct kcs_bmc_client_ops *ops,
+ struct kcs_bmc_driver *drv,
+ struct kcs_bmc_device *dev)
+{
+ client->ops = ops;
+ client->drv = drv;
+ client->dev = dev;
+}
+
+struct kcs_bmc_driver {
+ struct list_head entry;
+
+ const struct kcs_bmc_driver_ops *ops;
};
void kcs_bmc_register_driver(struct kcs_bmc_driver *drv);
diff --git a/drivers/char/ipmi/kcs_bmc_device.h b/drivers/char/ipmi/kcs_bmc_device.h
index 17c572f25c54..ca2b5dc2031d 100644
--- a/drivers/char/ipmi/kcs_bmc_device.h
+++ b/drivers/char/ipmi/kcs_bmc_device.h
@@ -16,7 +16,7 @@ struct kcs_bmc_device_ops {
};
irqreturn_t kcs_bmc_handle_event(struct kcs_bmc_device *kcs_bmc);
-int kcs_bmc_add_device(struct kcs_bmc_device *kcs_bmc);
-void kcs_bmc_remove_device(struct kcs_bmc_device *kcs_bmc);
+int kcs_bmc_add_device(struct kcs_bmc_device *dev);
+void kcs_bmc_remove_device(struct kcs_bmc_device *dev);
#endif
diff --git a/drivers/char/ipmi/kcs_bmc_serio.c b/drivers/char/ipmi/kcs_bmc_serio.c
index 0a68c76da955..3cfda39506f6 100644
--- a/drivers/char/ipmi/kcs_bmc_serio.c
+++ b/drivers/char/ipmi/kcs_bmc_serio.c
@@ -13,8 +13,6 @@
#include "kcs_bmc_client.h"
struct kcs_bmc_serio {
- struct list_head entry;
-
struct kcs_bmc_client client;
struct serio *port;
@@ -64,10 +62,8 @@ static void kcs_bmc_serio_close(struct serio *port)
kcs_bmc_disable_device(&priv->client);
}
-static DEFINE_SPINLOCK(kcs_bmc_serio_instances_lock);
-static LIST_HEAD(kcs_bmc_serio_instances);
-
-static int kcs_bmc_serio_add_device(struct kcs_bmc_device *kcs_bmc)
+static struct kcs_bmc_client *
+kcs_bmc_serio_add_device(struct kcs_bmc_driver *drv, struct kcs_bmc_device *dev)
{
struct kcs_bmc_serio *priv;
struct serio *port;
@@ -75,12 +71,12 @@ static int kcs_bmc_serio_add_device(struct kcs_bmc_device *kcs_bmc)
priv = kzalloc(sizeof(*priv), GFP_KERNEL);
if (!priv)
- return -ENOMEM;
+ return ERR_PTR(ENOMEM);
/* Use kzalloc() as the allocation is cleaned up with kfree() via serio_unregister_port() */
port = kzalloc(sizeof(*port), GFP_KERNEL);
if (!port) {
- rc = -ENOMEM;
+ rc = ENOMEM;
goto cleanup_priv;
}
@@ -88,45 +84,28 @@ static int kcs_bmc_serio_add_device(struct kcs_bmc_device *kcs_bmc)
port->open = kcs_bmc_serio_open;
port->close = kcs_bmc_serio_close;
port->port_data = priv;
- port->dev.parent = kcs_bmc->dev;
+ port->dev.parent = dev->dev;
spin_lock_init(&priv->lock);
priv->port = port;
- priv->client.dev = kcs_bmc;
- priv->client.ops = &kcs_bmc_serio_client_ops;
- spin_lock_irq(&kcs_bmc_serio_instances_lock);
- list_add(&priv->entry, &kcs_bmc_serio_instances);
- spin_unlock_irq(&kcs_bmc_serio_instances_lock);
+ kcs_bmc_client_init(&priv->client, &kcs_bmc_serio_client_ops, drv, dev);
serio_register_port(port);
- pr_info("Initialised serio client for channel %d\n", kcs_bmc->channel);
+ pr_info("Initialised serio client for channel %d\n", dev->channel);
- return 0;
+ return &priv->client;
cleanup_priv:
kfree(priv);
- return rc;
+ return ERR_PTR(rc);
}
-static void kcs_bmc_serio_remove_device(struct kcs_bmc_device *kcs_bmc)
+static void kcs_bmc_serio_remove_device(struct kcs_bmc_client *client)
{
- struct kcs_bmc_serio *priv = NULL, *pos;
-
- spin_lock_irq(&kcs_bmc_serio_instances_lock);
- list_for_each_entry(pos, &kcs_bmc_serio_instances, entry) {
- if (pos->client.dev == kcs_bmc) {
- priv = pos;
- list_del(&pos->entry);
- break;
- }
- }
- spin_unlock_irq(&kcs_bmc_serio_instances_lock);
-
- if (!priv)
- return;
+ struct kcs_bmc_serio *priv = client_to_kcs_bmc_serio(client);
/* kfree()s priv->port via put_device() */
serio_unregister_port(priv->port);
--
2.39.2
Powered by blists - more mailing lists