Re: [PATCH v5 5/7] revocable: Add fops replacement

[Tzung-Bi Shih <tzungbi@kernel.org>]

On Thu, Dec 11, 2025 at 02:46:08PM +0000, Tzung-Bi Shih wrote:
> On Thu, Dec 11, 2025 at 10:43:06PM +0900, Laurent Pinchart wrote:
> > On Thu, Dec 11, 2025 at 05:36:57PM +0900, Wolfram Sang wrote:
> > > 
> > > > > Isn't there even prototype code from Dan Williams?
> > > > > 
> > > > > "[PATCH 1/3] cdev: Finish the cdev api with queued mode support"
> > > > > 
> > > > > https://lkml.org/lkml/2021/1/20/997
> > > > 
> > > > I mentioned that in my LPC talk in 2022 :-) I think we should merge that
> > > > (or a rebased, possibly improved version of it). I've meant to try
> > > > plumbing that series in V4L2 but couldn't find the time so far.
> > > 
> > > Yes, you mentioned it in 2022 but maybe not everyone in this thread is
> > > right now aware of it ;) The patch above got changes requested. I talked
> > > to Dan very briefly about it at Maintainers Summit 2023 and he was also
> > > open (back then) to pick it up again.
> > 
> > After discussing with Tzung-Bi today after his presentation (thank you
> > Tzung-Bi for your time, it helped me understand the problem you're
> > facing better), I wonder if this series is fixing the issue in the right
> > place.
> 
> Thank you for your time too for providing me some more context.
> 
> > At the core of the problem is a devm_kzalloc() call to allocate
> > driver-specific data. That data structure is then referenced from a
> > cdev, which can dereference is after it gets freed. It seems that
> > reference-counting the data structure instead of using devm_kzalloc()
> > could be a better solution.
> 
> After discussing with you, I recalled this was one of my previous attempts.
> See the series [1] and Greg's feedback [2].
> 
> I want to provide some more context about the cdev level solution.  I failed
> to do so for misc device [3] mainly because all misc devices share a same
> cdev [4].  If one of the misc device drivers "revoke" the cdev, all other
> drivers stop working.
> 
> I'm not saying we shouldn't seek for cdev level solution.  But at least it
> doesn't work for misc device.  Still need some other ways for misc devices.
> 
> [1] https://lore.kernel.org/chrome-platform/20250721044456.2736300-8-tzungbi@kernel.org/
> [2] https://lore.kernel.org/chrome-platform/2025072114-unifier-screen-1594@gregkh/
> [3] https://lore.kernel.org/chrome-platform/aQ1xfHuyg1y8eJQ_@google.com/
> [4] https://elixir.bootlin.com/linux/v6.17/source/drivers/char/misc.c#L299

Continuing the context, the subsystem level solution for misc device without
revocable could be more or less like the following patch.  Observed 2 main
issues of it:

1. Because it tries to synchronize the misc device and open files, it has a
   big lock between them.  misc_deregister() needs to wait for all open files.
   I think this is a common issue shared by "replacing file operations"
   approaches.  All file operations are considered as critical sections.

2. It doesn't stop existing open files.  UAF still happens when the dangling
   FD tries to access the miscdevice (which should have been freed).

diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index 726516fb0a3b..0ce415da10c2 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -115,6 +116,89 @@ static const struct seq_operations misc_seq_ops = {
 };
 #endif

+static struct miscdevice *find_miscdevice(int minor)
+{
+	struct miscdevice *c;
+
+	list_for_each_entry(c, &misc_list, list)
+		if (c->minor == minor)
+			return c;
+	return NULL;
+}
+
+static __poll_t misc_some_poll(struct file *filp, poll_table *wait)
+{
+	struct miscdevice *c;
+
+	c = find_miscdevice(iminor(filp->f_inode));
+	if (!c)
+		return -ENODEV;
+	if (!c->fops->poll)
+		return 0;
+
+	guard(mutex)(&c->some_lock);
+	if (!c->registered)
+		return -ENODEV;
+	return c->fops->poll(filp, wait);
+}
+
+static const struct file_operations misc_some_fops = {
+	.poll = misc_some_poll,
+	.read = misc_some_read,
+	.unlocked_ioctl = misc_some_ioctl,
+	.release = misc_some_release,
+};

@@ -161,6 +245,7 @@ static int misc_open(struct inode *inode, struct file *file)
 	replace_fops(file, new_fops);
 	if (file->f_op->open)
 		err = file->f_op->open(inode, file);
+	file->f_op = &misc_some_fops;
 fail:
 	mutex_unlock(&misc_mtx);
 	return err;
@@ -262,6 +347,8 @@ int misc_register(struct miscdevice *misc)
 		goto out;
 	}
 
+	mutex_init(&misc->some_lock);
+	misc->registered = true;
 	/*
 	 * Add it to the front, so that later devices can "override"
 	 * earlier defaults
@@ -283,6 +370,9 @@ EXPORT_SYMBOL(misc_register);
 
 void misc_deregister(struct miscdevice *misc)
 {
+	scoped_guard(mutex, &misc->some_lock)
+		misc->registered = false;
+
 	mutex_lock(&misc_mtx);
 	list_del_init(&misc->list);
 	device_destroy(&misc_class, MKDEV(MISC_MAJOR, misc->minor));

diff --git a/include/linux/miscdevice.h b/include/linux/miscdevice.h
index 7d0aa718499c..3b42cf273f97 100644
--- a/include/linux/miscdevice.h
+++ b/include/linux/miscdevice.h
@@ -92,6 +92,8 @@ struct miscdevice {
 	const struct attribute_group **groups;
 	const char *nodename;
 	umode_t mode;
+	struct mutex some_lock;
+	bool registered;
 };