linux-kernel - Re: [RESEND PATCH 0/2] tty: serial: add and use a managed variant of uart_add_one

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Y8lPRiEbUfkjS/P/@kroah.com>
Date:   Thu, 19 Jan 2023 15:10:14 +0100
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Bartosz Golaszewski <brgl@...ev.pl>
Cc:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...ainline.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Vinod Koul <vkoul@...nel.org>, Alex Elder <elder@...nel.org>,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-serial@...r.kernel.org,
        Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [RESEND PATCH 0/2] tty: serial: add and use a managed variant of
 uart_add_one_port()

On Thu, Dec 29, 2022 at 05:19:46PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@...aro.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