From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758225AbcAKF30 (ORCPT ); Mon, 11 Jan 2016 00:29:26 -0500 Received: from mail-pa0-f44.google.com ([209.85.220.44]:36069 "EHLO mail-pa0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758132AbcAKF3Y (ORCPT ); Mon, 11 Jan 2016 00:29:24 -0500 Subject: Re: [PATCH 0/8] Fix unsafe uart port access To: Greg Kroah-Hartman References: <1449966992-4033-1-git-send-email-peter@hurleysoftware.com> Cc: Jiri Slaby , linux-kernel@vger.kernel.org From: Peter Hurley Message-ID: <56933DB2.2000208@hurleysoftware.com> Date: Sun, 10 Jan 2016 21:29:22 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <1449966992-4033-1-git-send-email-peter@hurleysoftware.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 12/12/2015 04:36 PM, Peter Hurley wrote: > Hi Greg, > > The serial core has always intended to allow uart drivers to detach and > unload, even while ttys are open and running. Since the serial core is > the actual tty driver, it acts as a proxy for the uart driver, which > allows the uart driver to remove the port (which hangs up the tty) and > then unload. > > However, all of this is broken. > > Firstly, there is no mechanism for blocking the uart port removal until > current operations are complete. The tty core does not, and never has, > serialized ioctl() or any other tty operation other than open/close > with hangup. > > Secondly, uart_register_driver() directly binds data from the uart driver > with the tty core, rather than providing proxy copies _without taking > module references to the uart driver_. This produces some spectacular > results if the tty is in-use when the uart driver unloads. > > Thirdly, uart_unregister_driver() immediately destroys the tty port > despite that it may be in use, and will continue to be for the > lifetime of the tty, which is unbounded (userspace may retain open > ttys forever). > > This series addresses the first problem of ensuring safe uart port > dereferencing with concurrent uart port removal. Two distinct > mechanisms are used to provide the necessary safe dereference: the > tty_port mutex and RCU. > > By serializing the uart port detach (ie, reset to NULL) with the > tty_port mutex, existing sections of the serial core that already > hold the port mutex are guaranteed the uart port detach will not > be concurrent, and so can simply test for NULL value before first > dereference. > > Most of the existing serial core that holds the port mutex is > ioctl-related and so can return -EIO as if the tty has been hungup > (which it has been). > > Other portions of the serial core that sleep (eg. uart_wait_until_sent()) > also now claim the port mutex to prevent concurrent uart port detach, > and thus protect the reference. The port mutex is dropped for the > actual sleep, retaken on wakeup and the uart port is re-checked > for NULL. > > For portions of the serial core that don't sleep, RCU is used to > defer actual uart port teardown after detach (with synchronize_rcu()) > until the current holders of rcu_read_lock() are complete. > > The xmit buffer operations that don't modify the buffer indexes -- > uart_write_room() and uart_chars_in_buffer() -- just report the state > of the xmit buffer anyway if the uart port has been removed, despite > that the xmit buffer is no longer lockable (the lock is in the > uart_port that is now gone). > > Xmit buffer operations that do modify the buffer indexes are aborted. > > Extra care is taken with the close/hangup/shutdown paths since this > must continue to perform other work even if the uart port has been > removed (for example, if the uart port was /dev/console and so will > only be closed when the file descriptor is released). > > I do have a plan for the second and third problems but this series > does not implement those. > > Regards, > > Peter Hurley (8): > serial: core: Fold __uart_put_char() into caller > serial: core: Fold do_uart_get_info() into caller > serial: core: Use tty->index for port # in debug messages Hi Greg, I split the first three patches above and submitted those in the "Misc serial cleanups" v2 series. For the remaining patches below, I need to rework how to guarantee the lifespan of uart port during throttle/unthrottle, and then rebase the following patches. Regards Peter Hurley > serial: core: Take port mutex for send_xchar() tty method > serial: core: Expand port mutex section in uart_poll_init() > serial: core: Prevent unsafe uart port access, part 1 > serial: core: Prevent unsafe uart port access, part 2 > serial: core: Prevent unsafe uart port access, part 3 > > drivers/tty/serial/8250/8250_port.c | 6 +- > drivers/tty/serial/serial_core.c | 547 +++++++++++++++++++++++------------- > 2 files changed, 355 insertions(+), 198 deletions(-) >