Re: [RESEND PATCH 0/2] tty: serial: add and use a managed variant of uart_add_one_port()

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Thu, Dec 29, 2022 at 05:19:46PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Resending rebased on top of v6.2-rc1
> 
> --
> 
> This series adds a managed variant of uart_add_one_port() and uses it in the
> qcom-geni-serial driver.
> 
> I've been asked by Greg to send it separately and he didn't seem to be
> impressed by the proposition of adding devres interfaces to the tty layer
> in general. I can only assume it has something to do with the ongoing
> discussion about the supposed danger of using devres interfaces in conjunction
> with exporting character devices to user-space.

That is correct.

> The bug in question can be triggered by opening a device file, unbinding the
> driver that exported it and then calling any of the system calls on the
> associated file descriptor.
> 
> After some testing I noticed that many subsystems are indeed either crashing
> or deadlocking in the above situation. I've sent patches that attempt to fix
> the GPIO and I2C subsystems[1][2]. Neither of these issues have anything to
> do with devres and all to do with the fact that certain resources are freed
> on driver unbind and others need to live for as long as the character device
> exists. More details on that in the cover letters and commit messages in the
> links.
> 
> I'd like to point out that the serial code is immune to this issue as before
> every operation, the serial core takes the port lock and checks the uart
> state. If the device no longer exists (when the uart port is removed, the
> pointer to uart_port inside uart_state is to NULL), it gracefully returns
> -ENODEV to user-space.
> 
> Please consider applying the patches in the series as devres is the easiest
> way to lessen the burden on driver developers when dealing with complex error
> paths and resource leaks. The general rule for devres is: if it can be freed
> in .remove() then it can be managed by devres, which is the case for this new
> helper.

Overall you are adding more code to the kernel than removing, so how is
this a win?  Perhaps if other drivers were converted over to this new
function then I would be more inclined to be able to accept it.

But as-is, with only one user, it's a non-starter, sorry.

thanks,

greg k-h