linux-kernel - Re: [patch V2 02/13] USB: serial: keyspan_pda: Replace in

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20201025165647.GR26280@localhost>
Date:   Sun, 25 Oct 2020 17:56:47 +0100
From:   Johan Hovold <johan@...nel.org>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        "Ahmed S. Darwish" <a.darwish@...utronix.de>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Johan Hovold <johan@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-usb@...r.kernel.org,
        Thomas Winischhofer <thomas@...ischhofer.net>,
        Mathias Nyman <mathias.nyman@...el.com>,
        Valentina Manea <valentina.manea.m@...il.com>,
        Shuah Khan <shuah@...nel.org>,
        Alan Stern <stern@...land.harvard.edu>,
        linux-omap@...r.kernel.org, Kukjin Kim <kgene@...nel.org>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        linux-arm-kernel@...ts.infradead.org,
        linux-samsung-soc@...r.kernel.org, Felipe Balbi <balbi@...nel.org>,
        Duncan Sands <duncan.sands@...e.fr>
Subject: Re: [patch V2 02/13] USB: serial: keyspan_pda: Replace
 in_interrupt() usage

On Mon, Oct 19, 2020 at 12:06:31PM +0200, Thomas Gleixner wrote:
> keyspan_pda_write() uses in_interrupt() to check whether it is safe to
> invoke functions which might sleep.
> 
> The usage of in_interrupt() in drivers is phased out and Linus clearly
> requested that code which changes behaviour depending on context should
> either be seperated or the context be conveyed in an argument passed by the
> caller, which usually knows the context.
> 
> Aside of that it does not cover all contexts which cannot sleep,
> e.g. preempt disabled regions which cannot be reliably detected on all
> kernel configurations.
> 
> With the current printk() implementation console->write() can be invoked
> from almost any context. The upcoming rework of the console core will
> provide thread context for console drivers which require to sleep.
> 
> For now, restrict the room query which can sleep to tty writes which happen
> from preemptible task context.
> 
> The usability for dmesg output is limited anyway because it's almost
> guaranteed to drop the 'LF' which is submitted after the dmesg line because
> the device supports only one transfer on flight. Same for any printk()
> which is coming in before the previous transfer has been done.
> 
> This new restriction does not make it worse than before, but it makes the
> condition correct under all circumstances.
> 
> Signed-off-by: Ahmed S. Darwish <a.darwish@...utronix.de>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> Cc: Johan Hovold <johan@...nel.org>
> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Cc: linux-usb@...r.kernel.org
> 
> ---
>  drivers/usb/serial/keyspan_pda.c |   10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> --- a/drivers/usb/serial/keyspan_pda.c
> +++ b/drivers/usb/serial/keyspan_pda.c
> @@ -477,10 +477,12 @@ static int keyspan_pda_write(struct tty_
>  
>  	count = (count > port->bulk_out_size) ? port->bulk_out_size : count;
>  
> -	/* Check if we might overrun the Tx buffer.   If so, ask the
> -	   device how much room it really has.  This is done only on
> -	   scheduler time, since usb_control_msg() sleeps. */
> -	if (count > priv->tx_room && !in_interrupt()) {
> +	/*
> +	 * Check if we might overrun the Tx buffer. If so, ask the device
> +	 * how much room it really has. This can only be invoked for tty
> +	 * usage because the console write can't sleep.
> +	 */
> +	if (count > priv->tx_room && tty) {
>  		u8 *room;
>  
>  		room = kmalloc(1, GFP_KERNEL);

There's a ton of issues with this driver, but this is arguably making
things worse. A line discipline may call write() from just about any
context so we cannot rely on tty being non-NULL here (e.g. PPP).

I've prepared a series fixing up the driver's write implementation
that does away with this room check in the write path instead.

Johan