| Message ID | 20221122102125.142075-13-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | serial: qcom-geni-serial: implement support for SE DMA | expand |
On Tue, Nov 22, 2022 at 11:21:22AM +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > 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. thanks, greg k-h
On Tue, Nov 22, 2022 at 5:58 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Tue, Nov 22, 2022 at 11:21:22AM +0100, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > 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@gmail.com/T/
diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst index 56082265e8e5..5d07a8c1eadb 100644 --- a/Documentation/driver-api/driver-model/devres.rst +++ b/Documentation/driver-api/driver-model/devres.rst @@ -436,6 +436,9 @@ RTC SERDEV devm_serdev_device_open() +SERIAL + devm_uart_add_one_port() + SLAVE DMA ENGINE devm_acpi_dma_controller_register() diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c index 179ee199df34..005190b279b3 100644 --- a/drivers/tty/serial/serial_core.c +++ b/drivers/tty/serial/serial_core.c @@ -3217,6 +3217,42 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport) } EXPORT_SYMBOL(uart_remove_one_port); +struct uart_port_devres { + struct uart_driver *drv; + struct uart_port *port; +}; + +static void devm_uart_remove_one_port(struct device *dev, void *data) +{ + struct uart_port_devres *res = data; + + uart_remove_one_port(res->drv, res->port); +} + +int devm_uart_add_one_port(struct device *dev, + struct uart_driver *drv, struct uart_port *port) +{ + struct uart_port_devres *res; + int ret; + + res = devres_alloc(devm_uart_remove_one_port, sizeof(*res), GFP_KERNEL); + if (!res) + return -1; + + ret = uart_add_one_port(drv, port); + if (ret) { + devres_free(res); + return -1; + } + + res->drv = drv; + res->port = port; + devres_add(dev, res); + + return 0; +} +EXPORT_SYMBOL(devm_uart_add_one_port); + /** * uart_match_port - are the two ports equivalent? * @port1: first port diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h index d657f2a42a7b..d0911f04706e 100644 --- a/include/linux/serial_core.h +++ b/include/linux/serial_core.h @@ -771,6 +771,12 @@ int uart_remove_one_port(struct uart_driver *reg, struct uart_port *port); bool uart_match_port(const struct uart_port *port1, const struct uart_port *port2); +/* + * UART devres + */ +int devm_uart_add_one_port(struct device *dev, + struct uart_driver *drv, struct uart_port *port); + /* * Power Management */