Re: [PATCH v2 12/15] tty: serial: provide devm_uart_add_one_port()

[Bartosz Golaszewski]

On Tue, Nov 22, 2022 at 5:58 PM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Tue, Nov 22, 2022 at 11:21:22AM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> >
> > Provide a devres variant of uart_add_one_port() that removes the managed
> > port at device detach.
>
> Ick, no, please let's keep devm away from the serial layer like this.
>
> I would need a LOT of justification for why this is needed, not just one
> driver that wants to use it, sorry.
>
> Please do not make this part of the series for this driver, but do it
> separately, later, if you still want it.
>

Hey Greg,

I sent out a v3 without devres changes.

It's not just one driver that would use it, rather: it would be the
first user and others could be converted later.

I was wondering if your reaction ("Ick, no") comes from the general
aversion to devres certain developers express? I'm asking because as a
matter of fact, I've been just recently going through the various
discussions about the supposed harm in using devres helpers and also
watched Laurent's presentation[1] from this year's Linux Plumbers.
While the issues described are real and easy to reproduce, I struggle
to understand how they're caused by devres and not by the driver model
in general. Specifically: the question that keeps popping up in my
head is: how are the use-after-free issues fixed by ditching devres in
favor of manually freeing resources in .remove()?

I'm not the only one to be asking that, for instance in this[2]
discussion Matti Vaittinen asked the same question and I can't find a
convincing answer in this thread. Instead there are some incorrect
statements, like claims that the order in which managed resources are
released is not guaranteed - it is: resources are released in reverse
(as they should).

It's true that devres_release_all() is called after the remove()
callback returns AND driver_sysfs_remove() is called so when replacing
kmalloc() with devm_kmalloc(), there is some functional change but
let's imagine the following patch:

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 3dda62503102..0046062828a3 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -527,7 +527,6 @@ static DEVICE_ATTR_RO(state_synced);

 static void device_unbind_cleanup(struct device *dev)
 {
-    devres_release_all(dev);
     arch_teardown_dma_ops(dev);
     kfree(dev->dma_range_map);
     dev->dma_range_map = NULL;
@@ -548,6 +547,8 @@ static void device_remove(struct device *dev)
         dev->bus->remove(dev);
     else if (dev->driver->remove)
         dev->driver->remove(dev);
+
+    devres_release_all(dev);
 }

 static int call_driver_probe(struct device *dev, struct device_driver *drv)

This seems to work just fine when tested (and I would argue it would
be slightly more correct so I may send it as a separate patch). Now
the devres resources are freed in reverse order IMMEDIATELY after
.remove(). How would moving anything into .remove() fix the
use-after-free problem described?

We have the same use-after-free issue with the GPIO character device
and while we keep the struct device associated with it alive until the
last reference is dropped, the user-space can crash the kernel by
calling any of the GPIO ioctl()s once the device has been unbound. I'm
looking into subsystem-level solutions to that but devres has nothing
to do with that (I tested that with gpio-sim just to make sure).

Anyway, it's most likely a subject for a separate discussion. Just let
me know what kind of justification for devres other than "it lessens
the burden on developers to keep track of resources and simplifies
error paths" you'd like to see.

Best Regards,
Bartosz

[1] https://www.youtube.com/watch?v=kW8LHWlJPTU
[2] https://lore.kernel.org/lkml/cover.1660292316.git.mazziesaccount@xxxxxxxxx/T/