[PATCH 0/0] SIGWINCH problem with terminal apps

[PATCH 0/0] SIGWINCH problem with terminal apps

[Adam Tlałka]

[-- Attachment #1: Type: text/plain, Size: 1561 bytes --]

Welcome,

I've observed then very often a X11 terminal app is not getting proper
window sizes afer terminal resize operation. This could be seen with
mc, jed, vim or other curses and not curses aware apps.

I wrote a simple program which just does nothing but uses SIGWINCH
handler so I can observe values reported by ioctl(1,TIOCGWINS,&ws) call
inside my signal handler. What is interesting that from time to time it
obtain unchanged values. It means values which were valid just before
terminal resize.

In drivers/char/vt.c and drivers/char/tty_io.c variables
vc->vc_tty->winsize and tty->winsize , real_tty->winsize are updated
after kill_pgrp(pgrp, SIGWINCH, 1) calls. I am not very familiar with
mutex design and how it corresponds to kill_pgrp() kernel function but
it seems that locking is not working here as we expect. An app can read
tty winsize data through ioctl() call in SIGWINCH handler and obtain
uchanged values.

So as a quick solution I made patches which move mentioned updates
before kill_pgrp() calls. As I tested modified kernel there is no
observed effect now. So I send patchs.

There are some places where kill_pgrp() call is used and some variable
is changed after it. It should be considered if this code is always
working properly or some race scheduler condition exists.

Signed-off-by: Adam Tla/lka <atlka@pg.gda.pl>

-- 
Adam Tlałka       mailto:atlka@pg.gda.pl    ^v^ ^v^ ^v^
System  & Network Administration Group       - - - ~~~~~~
Computer Center, Gdańsk University of Technology, Poland

[-- Attachment #2: 2.6.26.2_tty_io.patch --]
[-- Type: text/x-patch, Size: 624 bytes --]

--- drivers/char/tty_io_orig.c	2008-10-06 11:03:39.000000000 +0200
+++ drivers/char/tty_io.c	2008-10-06 11:20:54.000000000 +0200
@@ -3021,6 +3021,9 @@ static int tiocswinsz(struct tty_struct 
 	rpgrp = get_pid(real_tty->pgrp);
 	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
 
+	tty->winsize = tmp_ws;
+	real_tty->winsize = tmp_ws;
+
 	if (pgrp)
 		kill_pgrp(pgrp, SIGWINCH, 1);
 	if (rpgrp != pgrp && rpgrp)
@@ -3028,9 +3031,6 @@ static int tiocswinsz(struct tty_struct 
 
 	put_pid(pgrp);
 	put_pid(rpgrp);
-
-	tty->winsize = tmp_ws;
-	real_tty->winsize = tmp_ws;
 done:
 	mutex_unlock(&tty->termios_mutex);
 	return 0;

[-- Attachment #3: 2.6.26.2_vt.patch --]
[-- Type: text/x-patch, Size: 493 bytes --]

--- drivers/char/vt_orig.c	2008-10-06 11:01:26.000000000 +0200
+++ drivers/char/vt.c	2008-10-06 11:59:48.000000000 +0200
@@ -921,11 +921,11 @@ int vc_resize(struct vc_data *vc, unsign
 		if ((ws.ws_row != cws->ws_row || ws.ws_col != cws->ws_col))
 			pgrp = get_pid(vc->vc_tty->pgrp);
 		spin_unlock_irq(&vc->vc_tty->ctrl_lock);
+		*cws = ws;
 		if (pgrp) {
 			kill_pgrp(vc->vc_tty->pgrp, SIGWINCH, 1);
 			put_pid(pgrp);
 		}
-		*cws = ws;
 		mutex_unlock(&vc->vc_tty->termios_mutex);
 	}
 

Re: [PATCH 0/0] SIGWINCH problem with terminal apps

[Alan Cox]

> So as a quick solution I made patches which move mentioned updates
> before kill_pgrp() calls. As I tested modified kernel there is no
> observed effect now. So I send patchs.

NAK - this might happen to make the race miss on your box but it's not a
fix of any kind.

> There are some places where kill_pgrp() call is used and some variable
> is changed after it. It should be considered if this code is always
> working properly or some race scheduler condition exists.

The code was never race free, the scheduler change made the problem show
up more. Later 2.6.27-rc has patches that use the termios lock across
TIOCG/SWINSZ and deal with the problem properly.

Alan

Re: [PATCH 0/0] SIGWINCH problem with terminal apps

[Adam Tlałka]

Hi,

Mon, 6 Oct 2008 14:13:06 +0100 - Alan Cox <alan@lxorguk.ukuu.org.uk>:
> > So as a quick solution I made patches which move mentioned updates
> > before kill_pgrp() calls. As I tested modified kernel there is no
> > observed effect now. So I send patchs.
> 
> NAK - this might happen to make the race miss on your box but it's
> not a fix of any kind.

It depends. If mutexes are not working properly only in case of signal
sending then moving variables modification before kill_pgrp() call
could be a quite good enough solution too. Mutexes seems to be faster
and more efficient then semaphores for example.
 
> The code was never race free, the scheduler change made the problem
> show up more. Later 2.6.27-rc has patches that use the termios lock
> across TIOCG/SWINSZ and deal with the problem properly.

Maybe but what with older versions. Anyway the problem is if mutexes
are usable here or not.
 
Regards

-- 
Adam Tlałka

Re: [PATCH 0/0] SIGWINCH problem with terminal apps

[Alan Cox]

> > show up more. Later 2.6.27-rc has patches that use the termios lock
> > across TIOCG/SWINSZ and deal with the problem properly.
> 
> Maybe but what with older versions. Anyway the problem is if mutexes
> are usable here or not.

Well if mutexes don't work your kernel will eat your computer fairly
rapidly so I think we may safely conclude that mutex locks work

Re: [PATCH 0/0] SIGWINCH problem with terminal apps

[Adam Tlałka]

Mon, 6 Oct 2008 23:14:34 +0100 - Alan Cox <alan@lxorguk.ukuu.org.uk>:

> > > show up more. Later 2.6.27-rc has patches that use the termios
> > > lock across TIOCG/SWINSZ and deal with the problem properly.
> > 
> > Maybe but what with older versions. Anyway the problem is if mutexes
> > are usable here or not.
> 
> Well if mutexes don't work your kernel will eat your computer fairly
> rapidly so I think we may safely conclude that mutex locks work
> 

OK, so if this race problem raises only while kill_pgrp() is
used the proposed patch eliminates this problem. Of course we should
change code in all places where pgrp_kill() is used in conjuction
with mutexes and some internal variables are modified.
What do you think about it?

Regards

-- 
Adam Tlałka       mailto:atlka@pg.gda.pl    ^v^ ^v^ ^v^
System  & Network Administration Group       - - - ~~~~~~
Computer Center, Gdańsk University of Technology, Poland

[PATCH 0/1] SIGWINCH problem with terminal apps still alive

[Adam Tlałka]

[-- Attachment #1: Type: text/plain, Size: 680 bytes --]

Welcome,

now we have 2.6.26.6 kernel and still terminal resize leads to
undesired effects. It is very inconvenient to wait for 2.6.27 for
corrections.

As Alan Cox previously said mutexes generally work but as we can
observe in case of kill_pgrp() call inside mutex lock we got
race because of rescheduling so lock is not working here.
Rearanging code so the variable change is placed before kill_pgrp()
call removes mentioned race situaction.   

Signed-off-by: Adam Tla/lka <atlka@pg.gda.pl>

I strongly suggest to patch actual 2.6.26.x kernel to remove this very
nasty pts behaviour.

Regards

-- 
Adam Tlałka       mailto:atlka@pg.gda.pl    ^v^ ^v^ ^v^

[-- Attachment #2: 2.6.26.6_tty_io.patch --]
[-- Type: text/x-patch, Size: 639 bytes --]

--- drivers/char/tty_io_orig.c	2008-10-10 02:30:18.000000000 +0200
+++ drivers/char/tty_io.c	2008-10-10 02:33:38.000000000 +0200
@@ -3014,6 +3014,9 @@ static int tiocswinsz(struct tty_struct 
 		}
 	}
 #endif
+	tty->winsize = tmp_ws;
+	real_tty->winsize = tmp_ws;
+
 	/* Get the PID values and reference them so we can
 	   avoid holding the tty ctrl lock while sending signals */
 	spin_lock_irqsave(&tty->ctrl_lock, flags);
@@ -3028,9 +3031,6 @@ static int tiocswinsz(struct tty_struct 
 
 	put_pid(pgrp);
 	put_pid(rpgrp);
-
-	tty->winsize = tmp_ws;
-	real_tty->winsize = tmp_ws;
 done:
 	mutex_unlock(&tty->termios_mutex);
 	return 0;

[-- Attachment #3: 2.6.26.6_vt.patch --]
[-- Type: text/x-patch, Size: 493 bytes --]

--- drivers/char/vt_orig.c	2008-10-10 02:12:40.000000000 +0200
+++ drivers/char/vt.c	2008-10-10 02:31:28.000000000 +0200
@@ -921,11 +921,11 @@ int vc_resize(struct vc_data *vc, unsign
 		if ((ws.ws_row != cws->ws_row || ws.ws_col != cws->ws_col))
 			pgrp = get_pid(vc->vc_tty->pgrp);
 		spin_unlock_irq(&vc->vc_tty->ctrl_lock);
+		*cws = ws;
 		if (pgrp) {
 			kill_pgrp(vc->vc_tty->pgrp, SIGWINCH, 1);
 			put_pid(pgrp);
 		}
-		*cws = ws;
 		mutex_unlock(&vc->vc_tty->termios_mutex);
 	}
 

Re: [PATCH 0/1] SIGWINCH problem with terminal apps still alive

[Adam Tlałka]

Welcome,

Fri, 10 Oct 2008 03:12:34 +0200 - Adam Tlałka <atlka@pg.gda.pl>:
> now we have 2.6.26.6 kernel and still terminal resize leads to
> undesired effects. It is very inconvenient to wait for 2.6.27 for
> corrections.

Very funny, I've posted my patch just before 2.6.27 appeared so now it
seems obsolete. Only argument for it now is the cleaner and more
optimized code. Why?
Now we have two distant places where we use ws and tty->winsize
variables:

(from drivers/char/tty_io.c:tty_do_resize())

	mutex_lock(&real_tty->termios_mutex);
=>      if (!memcmp(ws, &tty->winsize, sizeof(*ws)))
                goto done;
        /* Get the PID values and reference them so we can
           avoid holding the tty ctrl lock while sending signals */
        spin_lock_irqsave(&tty->ctrl_lock, flags);
        pgrp = get_pid(tty->pgrp);
        rpgrp = get_pid(real_tty->pgrp);
        spin_unlock_irqrestore(&tty->ctrl_lock, flags);

        if (pgrp)
                kill_pgrp(pgrp, SIGWINCH, 1);
        if (rpgrp != pgrp && rpgrp)
                kill_pgrp(rpgrp, SIGWINCH, 1);

        put_pid(pgrp);
        put_pid(rpgrp);

=>      tty->winsize = *ws;
=>      real_tty->winsize = *ws;
done:
	mutex_unlock(&real_tty->termios_mutex);
 
Rearanged code

	mutex_lock(&real_tty->termios_mutex);
=>      if (!memcmp(ws, &tty->winsize, sizeof(*ws)))
                goto done;
=>	tty->winsize = *ws;
=>	real_tty->winsize = *ws;

        /* Get the PID values and reference them so we can
           avoid holding the tty ctrl lock while sending signals */
        spin_lock_irqsave(&tty->ctrl_lock, flags);
        pgrp = get_pid(tty->pgrp);
        rpgrp = get_pid(real_tty->pgrp);
        spin_unlock_irqrestore(&tty->ctrl_lock, flags);

        if (pgrp)
                kill_pgrp(pgrp, SIGWINCH, 1);
        if (rpgrp != pgrp && rpgrp)
                kill_pgrp(rpgrp, SIGWINCH, 1);

        put_pid(pgrp);
        put_pid(rpgrp);

done:
	mutex_unlock(&real_tty->termios_mutex);

is more logical and grouping acces to the same variable in one place
mean that gcc can better optimize outputed machine code. So it should
be a bit faster.
If some app gets SIGWINCH and will be waiting on this mutex so in case
of rearanged code it will be waiting shorter because variables setting
is done before signal generation.

Regards

-- 
Adam Tlałka       mailto:atlka@pg.gda.pl    ^v^ ^v^ ^v^
System  & Network Administration Group       - - - ~~~~~~
Computer Center, Gdańsk University of Technology, Poland

Re: [PATCH 0/1] SIGWINCH problem with terminal apps still alive

[Alan Cox]

On Fri, 10 Oct 2008 03:12:34 +0200
Adam Tlałka <atlka@pg.gda.pl> wrote:

> Welcome,
> 
> now we have 2.6.26.6 kernel and still terminal resize leads to
> undesired effects. It is very inconvenient to wait for 2.6.27 for
> corrections.
> 
> As Alan Cox previously said mutexes generally work but as we can
> observe in case of kill_pgrp() call inside mutex lock we got
> race because of rescheduling so lock is not working here.
> Rearanging code so the variable change is placed before kill_pgrp()
> call removes mentioned race situaction.   
> 
> Signed-off-by: Adam Tla/lka <atlka@pg.gda.pl>
> 
> I strongly suggest to patch actual 2.6.26.x kernel to remove this very
> nasty pts behaviour.

NAK again

Moving the copies around simply moves the race, it might be that it fixes
your box and unfixes other peoples.

Re: [PATCH 0/1] SIGWINCH problem with terminal apps still alive

[Adam Tlałka]

Fri, 10 Oct 2008 10:29:06 +0100 - Alan Cox <alan@lxorguk.ukuu.org.uk>:

> On Fri, 10 Oct 2008 03:12:34 +0200
> Adam Tlałka <atlka@pg.gda.pl> wrote:
> 
> > Welcome,
> > 
> > now we have 2.6.26.6 kernel and still terminal resize leads to
> > undesired effects. It is very inconvenient to wait for 2.6.27 for
> > corrections.
> > 
> > As Alan Cox previously said mutexes generally work but as we can
> > observe in case of kill_pgrp() call inside mutex lock we got
> > race because of rescheduling so lock is not working here.
> > Rearanging code so the variable change is placed before kill_pgrp()
> > call removes mentioned race situaction.   
> > 
> > Signed-off-by: Adam Tla/lka <atlka@pg.gda.pl>
> > 
> > I strongly suggest to patch actual 2.6.26.x kernel to remove this
> > very nasty pts behaviour.
> 
> NAK again
> 
> Moving the copies around simply moves the race, it might be that it
> fixes your box and unfixes other peoples.
> 

I don't think so. Race appears because of kill_pgrp() call which
generates SIGWINCH so it leads to reschedule and ioctl() which reads
termios sizes before they are updated - from time to time. So if we
update variables before signal generation there will be no race.
Moving the point of variables update eliminates
possibility of reading old values. So even if after kill_pgrp() the
other process will not lock here on this mutex values obtained will be
sane.

Whats more we could protect by mutex variable only test and change
operations and it stil will work correctly.

Because now we have 2.6.27 I tested this kind of code in
tty_io.c(tty_do_resize):

	struct pid *pgrp, *rpgrp;                                                                                    
        unsigned long flags;                                                                                         

/* For a PTY we need to lock the tty side */                                                                 
        mutex_lock(&real_tty->termios_mutex);                                                                        
        if ((flags = memcmp(ws, &tty->winsize, sizeof(*ws)))){                                                       
                tty->winsize = *ws;                                                                                  
                real_tty->winsize = *ws;                                                                             
        }                                                                                                            
        mutex_unlock(&real_tty->termios_mutex);                                                                      
        if (flags){                                                                                                  
                /* Get the PID values and reference them so we can                                                   
                   avoid holding the tty ctrl lock while sending signals */                                          
                spin_lock_irqsave(&tty->ctrl_lock, flags);                                                           
                pgrp = get_pid(tty->pgrp);                                                                           
                rpgrp = get_pid(real_tty->pgrp);                                                                     
                spin_unlock_irqrestore(&tty->ctrl_lock, flags);                                                      
                                                                                                                     
                if (pgrp)                                                                                            
                        kill_pgrp(pgrp, SIGWINCH, 1);                                                                
                if (rpgrp != pgrp && rpgrp)                                                                          
                        kill_pgrp(rpgrp, SIGWINCH, 1);                                                               
                                                                                                                     
                put_pid(pgrp);                                                                                       
                put_pid(rpgrp);                                                                                      
        }                                                                                                            
                                                                                                                     
        return 0;


So it works, and change of tty->winsize and real_tty->winsize are protected .
Why another process should wait more if winsize is already properly set?

Regards

-- 
Adam Tlałka       mailto:atlka@pg.gda.pl    ^v^ ^v^ ^v^
System  & Network Administration Group       - - - ~~~~~~
Computer Center, Gdańsk University of Technology, Poland

Re: [PATCH 0/1] SIGWINCH problem with terminal apps still alive

[Adam Tlałka]

Fri, 10 Oct 2008 12:35:17 +0200 - Adam Tlałka <atlka@pg.gda.pl>:

> Fri, 10 Oct 2008 10:29:06 +0100 - Alan Cox <alan@lxorguk.ukuu.org.uk>:
> > 
> > NAK again
> > 
> > Moving the copies around simply moves the race, it might be that it
> > fixes your box and unfixes other peoples.
> > 
> 
> I don't think so. Race appears because of kill_pgrp() call which
> generates SIGWINCH so it leads to reschedule and ioctl() which reads
> termios sizes before they are updated - from time to time. So if we
> update variables before signal generation there will be no race.
> Moving the point of variables update eliminates
> possibility of reading old values. So even if after kill_pgrp() the
> other process will not lock here on this mutex values obtained will be
> sane.
> 
> Whats more we could protect by mutex variable only test and change
> operations and it stil will work correctly.
> 
> Because now we have 2.6.27 I tested this kind of code in
> tty_io.c(tty_do_resize):
> 
> ...
> 
> So it works, and change of tty->winsize and real_tty->winsize are
> protected . Why another process should wait more if winsize is
> already properly set?

Next if we want to speed up our code in case of resize we could remove
one of two comparizons so values always be updated in tty_io.c(tty_do_resize):

	struct pid *pgrp, *rpgrp;
        unsigned long flags;

	/* For a PTY we need to lock the tty side */                                                                 
        mutex_lock(&real_tty->termios_mutex);                                                                        
        flags = memcmp(ws, &tty->winsize, sizeof(*ws));
        tty->winsize = *ws;
        real_tty->winsize = *ws;
        mutex_unlock(&real_tty->termios_mutex);                                                                      
        if (flags){                                                                                                  
                /* Get the PID values and reference them so we can                                                   
                   avoid holding the tty ctrl lock while sending signals */                                          
                spin_lock_irqsave(&tty->ctrl_lock, flags);                                                           
                pgrp = get_pid(tty->pgrp);                                                                           
                rpgrp = get_pid(real_tty->pgrp);                                                                     
                spin_unlock_irqrestore(&tty->ctrl_lock, flags);                                                      
                                                                                                                     
                if (pgrp)                                                                                            
                        kill_pgrp(pgrp, SIGWINCH, 1);                                                                
                if (rpgrp != pgrp && rpgrp)                                                                          
                        kill_pgrp(rpgrp, SIGWINCH, 1);                                                               
                                                                                                                     
                put_pid(pgrp);                                                                                       
                put_pid(rpgrp);                                                                                      
        }                                                                                                            
                                                                                                                     
        return 0;

We could assume that ioctl which sets the same values is rather rare
so we want faster code in case of changes. Presented above code for 
kernel 2.6.27 works quit nicely and I can't observe any bad effect of it.
Anyway we can prove on paper by time diagrams that there will be no races
according to update and reading winsize variables.

Regards

-- 
Adam Tlałka       mailto:atlka@pg.gda.pl    ^v^ ^v^ ^v^
System  & Network Administration Group       - - - ~~~~~~
Computer Center, Gdańsk University of Technology, Poland

Re: [PATCH 0/1] SIGWINCH problem with terminal apps still alive

[Alan Cox]

> Alan, do you agree? Or is it required to take both locks at the same time?
> If it is, in which order?

I don't agree. Please read the code more carefully. In paticular note
that TIOCGWINSZ is driven off the tty side of any tty/pty pair. That we
set the pty size termios bits is really a curiousity of history.

Alan

Re: [PATCH 0/1] SIGWINCH problem with terminal apps still alive

[Bodo Eggert]

Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> Adam Tla?ka <atlka@pg.gda.pl> wrote:

>> now we have 2.6.26.6 kernel and still terminal resize leads to
>> undesired effects. It is very inconvenient to wait for 2.6.27 for
>> corrections.

You'll have to wait some longer, since it still has this bug.

>> As Alan Cox previously said mutexes generally work but as we can
>> observe in case of kill_pgrp() call inside mutex lock we got
>> race because of rescheduling so lock is not working here.
>> Rearanging code so the variable change is placed before kill_pgrp()
>> call removes mentioned race situaction.

> NAK again
> 
> Moving the copies around simply moves the race, it might be that it fixes
> your box and unfixes other peoples.

This patch does not move around any race, but it works around a locking issue
by making sure you are the hedgehog racing the rabbit.

However, you are right in spotting that there must be something wrong with
the resulting code. It does (still) modify both tty and the real_tty while
only holding one lock. Besides that, it depends on tty->mutex to prevent
reading the old values because real_tty->mutex is held.

Adam, since you are working on this issue, I'd suggest you modify the source
to take both locks, one at a time, while setting the new values (lock
tty->mutex, compare tty->ws, possibly set ws, unlock, lock real_tty, ...).

Alan, do you agree? Or is it required to take both locks at the same time?
If it is, in which order?