Re: [PATCH] kernel panic, pty.c: remove direct call to tty_wakup in pty_write with better commit message

[Andre Naujoks <nautsch2@gmail.com>]

Thanks for the pointer. Since your patch is from April, does that mean, 
that we cannot expect it to hit 3.11?


 From 6cbfeb6cb9bbe7455cddbf162a7b158e1debd578 Mon Sep 17 00:00:00 2001
From: Andre Naujoks <nautsch2@googlemail.com>
Date: Tue, 2 Jul 2013 22:11:33 +0200
Subject: [PATCH] Remove the tty_wakeup call inside pty_write

The call to tty_wakeup can cause a recursive loop and therefore a
kernel oops when pty_write is called again from the ldisc wakeup
function but there is not enough room for all data at once.

Signed-off-by: Andre Naujoks <nautsch2@googlemail.com>
---
  drivers/tty/pty.c | 4 +---
  1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index abfd990..cae4e4f 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -125,10 +125,8 @@ static int pty_write(struct tty_struct *tty, const 
unsigned char *buf, int c)
  		/* Stuff the data into the input queue of the other end */
  		c = tty_insert_flip_string(to->port, buf, c);
  		/* And shovel */
-		if (c) {
+		if (c)
  			tty_flip_buffer_push(to->port);
-			tty_wakeup(tty);
-		}
  	}
  	return c;
  }
-- 
1.8.3.1



On 02.07.2013 20:59, Peter Hurley wrote:
> On 07/01/2013 10:49 AM, Andre Naujoks wrote:
>> Hello.
>>
>> This patch removes the direct call to tty_wakeup in pty_write. I have
>> not noticed any drawbacks with this but I am not familiar with the pty
>> driver at all. I think what happens is a recursive loop,
>> write_wakeup->write->write_wakeup ...
>>
>> The documentation for the tty interface forbids this direct call:
>>
>> (from Documentation/serial/tty.txt)
>> write_wakeup()  - May be called at any point between open and close.
>>        The TTY_DO_WRITE_WAKEUP flag indicates if a call
>>        is needed but always races versus calls. Thus the
>>        ldisc must be careful about setting order and to
>>        handle unexpected calls. Must not sleep.
>>
>>        The driver is forbidden from calling this directly
>>        from the ->write call from the ldisc as the ldisc
>>        is permitted to call the driver write method from
>>        this function. In such a situation defer it.
>>
>>
>>
>> The direct call caused a reproducable kernel panic (see bottom of this
>> mail) for me with the following setup:
>>
>> - using can-utils from git://gitorious.org/linux-can/can-utils.git
>>    slcan_attach and cangen are used
>>
>> - create a network link between two serial CAN interfaces with:
>>    $ socat PTY,link=/tmp/slcan0,raw TCP4-LISTEN:50000 &
>>    $ socat TCP4:localhost:50000 PTY,link=/tmp/slcan1,raw &
>>    $ slcan_attach /tmp/slcan0
>>    $ slcan_attach /tmp/slcan1
>>    $ ip link set slcan0 up
>>    $ ip link set slcan1 up
>>
>> - produce a kernel panic by overloading the CAN interfaces:
>>    $ cangen slcan0 -g0
>>
>>
>> Please keep me in CC. I am not subscribed to the list.
>> If I can provide any more information, I will be glad to do so.
>>
>> This is the patch. It applies to the current linux master branch:
>
> An identical patch is in Greg's queue for linux-next:
>    'tty: Remove extra wakeup from pty write() path'
>
> That patch's commit message details why tty_wakeup() is unnecessary,
> but does not foresee or document the SLIP ldisc write()/write_wakeup()
> recursion.
>
> Since this fix will now likely go back through stable, the commit
> message should include a description of the recursion, so that Greg can
> merge the commit messages.
>
> Separately, the stack trace for the WARN and the oops implicates
> the network stack alone. Maybe there is some other problem?
>
> Regards,
> Peter Hurley
>
>
>