On Fri, Sep 12, 2025 at 04:26:56PM +0300, Laurent Pinchart wrote:
On Fri, Sep 12, 2025 at 08:49:30PM +0800, Tzung-Bi Shih wrote:
On Fri, Sep 12, 2025 at 11:24:10AM +0200, Bartosz Golaszewski wrote:
On Fri, 12 Sept 2025 at 11:09, Krzysztof Kozlowski wrote:
On 12/09/2025 10:17, Tzung-Bi Shih wrote:
Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Bartosz Golaszewski bartosz.golaszewski@linaro.org Cc: Wolfram Sang wsa+renesas@sang-engineering.com
Thanks for the work. Just a note, please start using b4, so above Cc will be propagated to all patches. Folks above received only the cover letter...
Thank you for bringing this to my attention. I wasn't aware of that and will ensure this is handled correctly in the future.
Thanks to Krzysztof for making me aware of this. Could you please Cc my brgl@bgdev.pl address on the next iteration.
Sure, will do.
I haven't looked into the details yet but the small size of the first patch strikes me as odd. The similar changes I did for GPIO were quite big and they were designed just for a single sub-system.
During the talk you reference, after I suggested a library like this, Greg KH can be heard saying: do this for two big subsystems so that you're sure it's a generic solution. Here you're only using it in a single driver which makes me wonder if we can actually use it to improve bigger offenders, like for example I2C, or even replace the custom, SRCU-based solution in GPIO we have now. Have you considered at least doing a PoC in a wider kernel framework?
Yes, I'm happy to take this on.
To help me get started, could you please point me to some relevant code locations? Also, could you let me know if any specific physical devices will be needed for testing?
One interesting test would be to move the logic to the cdev layer. The use-after-free problem isn't specific to one type of character device, and so shouldn't require a fix in every driver instantiating a cdev directly (or indirectly). See [1] for a previous attempt to handle this at the V4L2 level and [2] for an attempt to handle it at the cdev level.
In [1], two new functions named video_device_enter() and video_device_exit() flag the beginning and end of protected code sections. The equivalent in [2] is the manual get/put of cdev->qactive, and if I understand things correctly, your series creates a REVOCABLE() macro to do the same. I'm sure we'll bikesheed about names at some point, but for the time being, what I'd like to see if this being done in fs/char_dev.c to cover all entry points from userspace at the cdev level.
We then have video_device_unplug() in [1], which I think is more or less the equivalent of revocable_provider_free(). I don't think we'll be able to hide this completely from drivers, at least not in all cases. We should however design the API to make it easy for drivers, likely with subsystem-specific wrappers.
What I have in mind is roughly the following:
Protect all access to the cdev from userspace with enter/exit calls that flag if a call is in progress. This can be done with explicit function calls, or with a scope guard as in your series.
At .remove() time, start by flagging that the device is being removed. That has to be an explicit call from drivers I believe, likely using subsystem-specific wrappers to simplify things.
Once the device is marked as being removed, all enter() calls should fail at the cdev level.
In .remove(), proceed to perform driver-specific operations that will stop the device and wake up any userspace task blocked on a syscall protected by enter()/remove(). This isn't needed for drivers/subsystems that don't provide any blocking API, but is required otherwise.
Unregister, still in .remove(), the cdev (likely through subsystem-specific APIs in most cases). This should block until all protected sections have exited.
The cdev is now unregistered, can't be opened anymore, and any new syscall on any opened file handle will return an error. The driver's .remove() function can proceed to free data, there won't be any UAF caused by userspace.
[1] implemented this fairly naively with flags and spinlocks. An RCU-based implementation is probably more efficient, even if I don't know how performance-sensitive all this is.
Does this align with your design, and do you think you could give a try at pushing revocable resource handling to the cdev level ?
On a separate note, I'm not sure "revocable" is the right name here. I believe a revocable resource API is needed, and well-named, for in-kernel consumers (e.g. drivers consuming a GPIO or clock). For the userspace syscalls racing with .remove(), I don't think we're dealing with "revocable resources". Now, if a "revocable resources" API were to support the in-kernel users, and be usable as a building block to fix the cdev issue, I would have nothing against it, but the "revocable" name should be internal in that case, used in the cdev layer only, and not exposed to drivers (or even subsystem helpers that should wrap cdev functions instead).
I think the name makes sense as it matches up with how things are working (the backend structure is "revoked"), but naming is tough :)
I have no objection moving this to the cdev api, BUT given that 'struct cdev' is embedded everywhere, I don't think it's going to be a simple task, but rather have to be done one-driver-at-a-time like the patch in this series does it.
And that's fine, we have "interns" that we can set loose on this type of code conversions, I think we just need to wrap the access to the cdev with this api, which will take a bit of rewriting in many drivers.
Anyway, just my thought, if someone else can see how this could drop into the core cdev code without any changes needed, that would be great, but I don't see it at the moment. cdev is just too "raw" for that.
thanks,
greg k-h