Re: [PATCH] ipmi: ipmb: keep device alive across remove

Re: [PATCH] ipmi: ipmb: keep device alive across remove

[Chad Aldreda]

On Wed, 5 Aug 2026, Qing Ming wrote:
> Allocate ipmb_dev explicitly and hold a reference for each open file. Mark
> the device dead before deregistering the miscdevice, wake blocked readers,
> report a hangup to poll, and reject writes after removal.

Tested on mainline d48723711bae (v7.4-rc1-672), where this still applies,
in QEMU with KASAN, lockdep and kmemleak. There is no IPMB hardware
involved: a test-only I2C adapter module with target support binds the
driver and injects requests.

Without the patch, sysfs unbind, delete_device or removing the adapter
with /dev/ipmb-1 open gives slab-use-after-free reports on the next
poll(), read() and write(), and a read() or poll() blocked across an
unbind or delete_device is not woken.

With the patch there are no KASAN, lockdep or kmemleak reports. After
each of the three removals, poll() returns POLLERR|POLLHUP, write()
returns -ENODEV, read() with no requests queued returns -ENODEV, and a
blocked read() or poll() wakes up.

A blocking read() and poll() are still woken by a new request, the
request arrives intact, and an SMBus write completes. These also ran
clean: a write in flight across an unbind (the unbind waits for it, now
on file_mutex rather than the adapter bus lock, and the next write got
-ENODEV), an unbind and bind with the old file still open, closing with
three unread requests queued, probe failures from target registration
and from misc_register(), and 200 unbind/bind cycles against six
processes opening, polling, reading and writing while requests are
injected. The same stress run on the unpatched kernel produces KASAN
reports.

One side effect showed up in testing. ipmb_write() now holds file_mutex
across the transfer, and ipmb_open() takes file_mutex while misc_open()
holds misc_mtx, so a second open of the node during a slow transfer
stalls every misc device open in the system. With a 2 s transfer in the
stub, an open of /dev/cpu_dma_latency took about 1.6 s, against 0.00 s
without the patch. misc_deregister() also takes misc_mtx, and remove()
drops its reference after that, so for lifetime ipmb_open() seems not to
need file_mutex. An open racing removal could then still succeed, and
that file sees the same results as one opened just before removal. With
file_mutex replaced by READ_ONCE(ipmb_dev->dead) the open of
/dev/cpu_dma_latency took 0.00 s and the rest of the tests above stayed
clean. From reading the code only, poll() on the node also waits on
file_mutex during a transfer.

Not tested: real hardware, the i2c-protocol write path (only the SMBus
one ran), adapter removal with a write in flight, read() after removal
with requests still queued (from the code, these are still returned
while poll() reports only POLLERR|POLLHUP), and module unload (the
driver was built in).

The harness and this report were produced with an AI assistant (Claude
Opus 5.5), which also did the earlier runs, including the unpatched and
READ_ONCE() comparisons. I ran the final version on the patched kernel
myself and got the same results. The harness is available on request.

Tested-by: Chad Aldreda <ca@modelmiser.com>

[PATCH] ipmi: ipmb: keep device alive across remove

[Qing Ming]

misc_deregister() prevents new opens but does not close existing files.
ipmb_dev is allocated with devm_kzalloc(), so I2C device removal releases
the object while an open /dev/ipmb-* file can still reach it through its
file operations.

Keeping /dev/ipmb-0 open across I2C removal and then polling the old file
triggers:

  BUG: KASAN: slab-use-after-free in mutex_lock
  ipmb_poll

Allocate ipmb_dev explicitly and hold a reference for each open file. Mark
the device dead before deregistering the miscdevice, wake blocked readers,
report a hangup to poll, and reject writes after removal. Serialize writes
with removal so the I2C client is not used after it has been unregistered.
Release queued requests and the device object after the last reference is
dropped.

Register the I2C target before exposing the miscdevice so a late probe
failure cannot race an open file.

Fixes: 51bd6f291583 ("Add support for IPMB driver")
Cc: stable@vger.kernel.org
Signed-off-by: Qing Ming <a0yami@mailbox.org>
---
 drivers/char/ipmi/ipmb_dev_int.c | 121 +++++++++++++++++++++++++------
 1 file changed, 100 insertions(+), 21 deletions(-)

diff --git a/drivers/char/ipmi/ipmb_dev_int.c b/drivers/char/ipmi/ipmb_dev_int.c
index e4c50d9ae3e1..7f63fa002f18 100644
--- a/drivers/char/ipmi/ipmb_dev_int.c
+++ b/drivers/char/ipmi/ipmb_dev_int.c
@@ -11,6 +11,7 @@
 #include <linux/acpi.h>
 #include <linux/errno.h>
 #include <linux/i2c.h>
+#include <linux/kref.h>
 #include <linux/miscdevice.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
@@ -54,6 +55,7 @@ struct ipmb_request_elem {
 };
 
 struct ipmb_dev {
+	struct kref refcount;
 	struct i2c_client *client;
 	struct miscdevice miscdev;
 	struct ipmb_msg request;
@@ -64,11 +66,26 @@ struct ipmb_dev {
 	wait_queue_head_t wait_queue;
 	struct mutex file_mutex;
 	bool is_i2c_protocol;
+	bool dead;
 };
 
 static inline struct ipmb_dev *to_ipmb_dev(struct file *file)
 {
-	return container_of(file->private_data, struct ipmb_dev, miscdev);
+	return file->private_data;
+}
+
+static void ipmb_dev_release(struct kref *refcount)
+{
+	struct ipmb_dev *ipmb_dev;
+	struct ipmb_request_elem *elem, *tmp;
+
+	ipmb_dev = container_of(refcount, struct ipmb_dev, refcount);
+	list_for_each_entry_safe(elem, tmp, &ipmb_dev->request_queue, list) {
+		list_del(&elem->list);
+		kfree(elem);
+	}
+	kfree(ipmb_dev->miscdev.name);
+	kfree(ipmb_dev);
 }
 
 static ssize_t ipmb_read(struct file *file, char __user *buf, size_t count,
@@ -84,12 +101,17 @@ static ssize_t ipmb_read(struct file *file, char __user *buf, size_t count,
 	spin_lock_irq(&ipmb_dev->lock);
 
 	while (list_empty(&ipmb_dev->request_queue)) {
+		if (READ_ONCE(ipmb_dev->dead)) {
+			spin_unlock_irq(&ipmb_dev->lock);
+			return -ENODEV;
+		}
 		spin_unlock_irq(&ipmb_dev->lock);
 
 		if (file->f_flags & O_NONBLOCK)
 			return -EAGAIN;
 
 		ret = wait_event_interruptible(ipmb_dev->wait_queue,
+				READ_ONCE(ipmb_dev->dead) ||
 				!list_empty(&ipmb_dev->request_queue));
 		if (ret)
 			return ret;
@@ -150,6 +172,12 @@ static ssize_t ipmb_write(struct file *file, const char __user *buf,
 	if (msg[IPMB_MSG_LEN_IDX] < IPMB_REQUEST_LEN_MIN ||
 	    count < (size_t)msg[IPMB_MSG_LEN_IDX] + 1)
 		return -EINVAL;
+	if (mutex_lock_interruptible(&ipmb_dev->file_mutex))
+		return -ERESTARTSYS;
+	if (ipmb_dev->dead) {
+		ret = -ENODEV;
+		goto out_unlock;
+	}
 
 	rq_sa = GET_7BIT_ADDR(msg[RQ_SA_8BIT_IDX]);
 	netf_rq_lun = msg[NETFN_LUN_IDX];
@@ -157,7 +185,8 @@ static ssize_t ipmb_write(struct file *file, const char __user *buf,
 	/* Check i2c block transfer vs smbus */
 	if (ipmb_dev->is_i2c_protocol) {
 		ret = ipmb_i2c_write(ipmb_dev->client, msg, rq_sa);
-		return (ret == 1) ? count : ret;
+		ret = (ret == 1) ? count : ret;
+		goto out_unlock;
 	}
 
 	/*
@@ -166,8 +195,10 @@ static ssize_t ipmb_write(struct file *file, const char __user *buf,
 	 */
 	msg_len = msg[IPMB_MSG_LEN_IDX] - SMBUS_MSG_HEADER_LENGTH;
 	temp_client = kmemdup(ipmb_dev->client, sizeof(*temp_client), GFP_KERNEL);
-	if (!temp_client)
-		return -ENOMEM;
+	if (!temp_client) {
+		ret = -ENOMEM;
+		goto out_unlock;
+	}
 
 	temp_client->addr = rq_sa;
 
@@ -175,7 +206,11 @@ static ssize_t ipmb_write(struct file *file, const char __user *buf,
 					 msg + SMBUS_MSG_IDX_OFFSET);
 	kfree(temp_client);
 
-	return ret < 0 ? ret : count;
+	ret = ret < 0 ? ret : count;
+
+out_unlock:
+	mutex_unlock(&ipmb_dev->file_mutex);
+	return ret;
 }
 
 static __poll_t ipmb_poll(struct file *file, poll_table *wait)
@@ -184,6 +219,10 @@ static __poll_t ipmb_poll(struct file *file, poll_table *wait)
 	__poll_t mask = EPOLLOUT;
 
 	mutex_lock(&ipmb_dev->file_mutex);
+	if (ipmb_dev->dead) {
+		mutex_unlock(&ipmb_dev->file_mutex);
+		return EPOLLERR | EPOLLHUP;
+	}
 	poll_wait(file, &ipmb_dev->wait_queue, wait);
 
 	if (atomic_read(&ipmb_dev->request_queue_len))
@@ -193,11 +232,38 @@ static __poll_t ipmb_poll(struct file *file, poll_table *wait)
 	return mask;
 }
 
+static int ipmb_open(struct inode *inode, struct file *file)
+{
+	struct ipmb_dev *ipmb_dev;
+	int ret = 0;
+
+	ipmb_dev = container_of(file->private_data, struct ipmb_dev, miscdev);
+	mutex_lock(&ipmb_dev->file_mutex);
+	if (ipmb_dev->dead) {
+		ret = -ENODEV;
+	} else {
+		kref_get(&ipmb_dev->refcount);
+		file->private_data = ipmb_dev;
+	}
+	mutex_unlock(&ipmb_dev->file_mutex);
+	return ret;
+}
+
+static int ipmb_release(struct inode *inode, struct file *file)
+{
+	struct ipmb_dev *ipmb_dev = to_ipmb_dev(file);
+
+	kref_put(&ipmb_dev->refcount, ipmb_dev_release);
+	return 0;
+}
+
 static const struct file_operations ipmb_fops = {
 	.owner	= THIS_MODULE,
+	.open	= ipmb_open,
 	.read	= ipmb_read,
 	.write	= ipmb_write,
 	.poll	= ipmb_poll,
+	.release = ipmb_release,
 };
 
 /* Called with ipmb_dev->lock held. */
@@ -305,10 +371,10 @@ static int ipmb_probe(struct i2c_client *client)
 	struct ipmb_dev *ipmb_dev;
 	int ret;
 
-	ipmb_dev = devm_kzalloc(&client->dev, sizeof(*ipmb_dev),
-					GFP_KERNEL);
+	ipmb_dev = kzalloc_obj(struct ipmb_dev);
 	if (!ipmb_dev)
 		return -ENOMEM;
+	kref_init(&ipmb_dev->refcount);
 
 	spin_lock_init(&ipmb_dev->lock);
 	init_waitqueue_head(&ipmb_dev->wait_queue);
@@ -319,38 +385,51 @@ static int ipmb_probe(struct i2c_client *client)
 
 	ipmb_dev->miscdev.minor = MISC_DYNAMIC_MINOR;
 
-	ipmb_dev->miscdev.name = devm_kasprintf(&client->dev, GFP_KERNEL,
-						"%s%d", "ipmb-",
-						client->adapter->nr);
-	if (!ipmb_dev->miscdev.name)
-		return -ENOMEM;
+	ipmb_dev->miscdev.name = kasprintf(GFP_KERNEL, "%s%d", "ipmb-",
+					   client->adapter->nr);
+	if (!ipmb_dev->miscdev.name) {
+		ret = -ENOMEM;
+		goto err_put;
+	}
 
 	ipmb_dev->miscdev.fops = &ipmb_fops;
 	ipmb_dev->miscdev.parent = &client->dev;
-	ret = misc_register(&ipmb_dev->miscdev);
-	if (ret)
-		return ret;
-
 	ipmb_dev->is_i2c_protocol
 		= device_property_read_bool(&client->dev, "i2c-protocol");
 
 	ipmb_dev->client = client;
 	i2c_set_clientdata(client, ipmb_dev);
 	ret = i2c_slave_register(client, ipmb_slave_cb);
-	if (ret) {
-		misc_deregister(&ipmb_dev->miscdev);
-		return ret;
-	}
+	if (ret)
+		goto err_data;
+
+	ret = misc_register(&ipmb_dev->miscdev);
+	if (ret)
+		goto err_slave;
 
 	return 0;
+
+err_slave:
+	i2c_slave_unregister(client);
+err_data:
+	i2c_set_clientdata(client, NULL);
+err_put:
+	kref_put(&ipmb_dev->refcount, ipmb_dev_release);
+	return ret;
 }
 
 static void ipmb_remove(struct i2c_client *client)
 {
 	struct ipmb_dev *ipmb_dev = i2c_get_clientdata(client);
 
-	i2c_slave_unregister(client);
+	mutex_lock(&ipmb_dev->file_mutex);
+	WRITE_ONCE(ipmb_dev->dead, true);
+	mutex_unlock(&ipmb_dev->file_mutex);
 	misc_deregister(&ipmb_dev->miscdev);
+	wake_up_all(&ipmb_dev->wait_queue);
+	i2c_slave_unregister(client);
+	i2c_set_clientdata(client, NULL);
+	kref_put(&ipmb_dev->refcount, ipmb_dev_release);
 }
 
 static const struct i2c_device_id ipmb_id[] = {
-- 
2.53.0