On Mon, Sep 22, 2025 at 08:42:23PM +0200, Greg Kroah-Hartman wrote:
On Mon, Sep 22, 2025 at 02:40:10PM -0300, Jason Gunthorpe wrote:
On Mon, Sep 22, 2025 at 05:55:43PM +0200, Danilo Krummrich wrote:
I fully agree with that, in C there is indeed no value of a revocable type when subsystems can guarantee "sane unregistration semantics".
Indeed, I agree with your message. If I look at the ec_cros presented here, there is no reason for any revocable. In fact it already seems like an abuse of the idea.
The cros_ec.c spawns a MFD with a platform_device of "cros-ec-chardev" that is alreay properly lifetime controlled - the platform is already removed during the remove of the cros_ec.c.
So, there is no need for a revocable that spans two drivers like this!
It's a very common pattern, you have a struct device, and a userspace access, and need to "split" them apart, as you say below. This logic here handles that pattern.
This is two struct devices, two device drivers and a fops. There is no reason to trigger revoke from the parent driver to a child driver, that's just mis-layering and obfuscation.
It already subtly relies on the parent not triggering revoke until the child is removed because it added this to the cros-ec-chardev driver:
@@ -166,7 +183,12 @@ static int cros_ec_chardev_open(struct inode *inode, struct file *filp) struct cros_ec_dev *ec_dev = dev_get_drvdata(mdev->parent);
if (!priv) return -ENOMEM;
- priv->ec_dev = ec_dev; + priv->ec_dev_rev = revocable_alloc(ec_dev->revocable_provider);
It would UAF if the revoke is triggered by the parent driver at the wrong time.
So why is the parent involved at all? Why does it have to violate modularity?
See the many talks about this in the past at Plumbers and other conferences, this isn't a new thing, and it isn't quite as simple as I think you are making it out to be to solve.
There are more complex situations, but this isn't one of them. All this needs is to fence the file operations of a single cdev. I've solved it enough times now to know exactly how simple this really is..
struct chardev_data {
struct device dev; struct cdev cdev;
Woah, no, that is totally wrong.
Sigh. I'm sure we've had this exchange before. Please re-read the documentation for cdev_device_add():
* This function should be used whenever the struct cdev and the * struct device are members of the same structure whose lifetime is * managed by the struct device. ^^^^^^^^^^^^
We created this helper specifically to clean up the refcount bugs around cdev usage. It supports the above pattern which is the natural and easy way to use cdev.
And really, let's not abuse cdev anymore than it currently is please. There's a reason that miscdevice returns just a pointer. Right now cdev can be used in 3-4 different ways, let's not come up with another way to abuse that api :)
It is true there are many abuses, but converting cdev users to use cdev_device_add() is the right and best option IMHO.
miscdev is not a good option in cases like this because you don't get a nice natural kref around the memory, among other limitations.
There is no performance issue here, but there is lifetime rule logic here that I really really do not want to have to "open code" for every user. revokable gives this to us in a simple way that we can "know" is being used correctly.
I strongly prefer Laurent's direction to add a helper file_operations that automatically wraps all ops in a lock.
I think all this series does is create a weirdly named lock that drivers still have to open code.
The fact the very first proposed user is already abusing it to needlessly violate driver modularity is really depressing. Let's not create another devm :\
Jason