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

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?

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/2] SIGWINCH problem with terminal apps still alive

[Adam Tlałka]

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

Welcome,

generally speaking terminal resize could be made from vc or other
terminal device module code, from terminal ioctl on tty master
or tty slave. As we read the code we will see that the problem is to
use a proper mutex. Also we should use a one variable in which we set
terminal sizes. So in case of tty master and slave we have tty and
real_tty structures. TIOCSWINSZ and TIOCGWINSZ ioctls could be
called on tty and real_tty at the same time. To avoid race condition
we should use a mutex. But this must be the same mutex in all cases
- tty or real_tty. For the best we should also use the same
winsize variable. Proposed previously code change only simplifies
the code and eliminates SIGWINCH signal race but an app could read
terminal sizes at any time so this patch not closes all race
possibilities. So I propose a patch in which we have some code
movements and always use real_tty->termios_mutex and also
real_tty->winsize. In no pty case tty == real_tty so it works properly
as before and in pty situaction we use the same mutex and variable so
it removes all race conditions according to access to winsize now.

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

Regards

-- 
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.27_tty_io_2.patch --]
[-- Type: text/x-patch, Size: 1719 bytes --]

--- drivers/char/tty_io_orig.c	2008-10-10 05:37:30.000000000 +0200
+++ drivers/char/tty_io.c	2008-10-12 13:37:11.000000000 +0200
@@ -2524,27 +2524,26 @@ int tty_do_resize(struct tty_struct *tty
 
 	/* For a PTY we need to lock the tty side */
 	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);
+	flags = memcmp(ws, &real_tty->winsize, sizeof(*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);
+		if (pgrp)
+			kill_pgrp(pgrp, SIGWINCH, 1);
+		if (rpgrp != pgrp && rpgrp)
+			kill_pgrp(rpgrp, SIGWINCH, 1);
 
-	put_pid(pgrp);
-	put_pid(rpgrp);
+		put_pid(pgrp);
+		put_pid(rpgrp);
+	}
 
-	tty->winsize = *ws;
-	real_tty->winsize = *ws;
-done:
-	mutex_unlock(&real_tty->termios_mutex);
 	return 0;
 }
 
@@ -2996,7 +2995,7 @@ long tty_ioctl(struct file *file, unsign
 	case TIOCSTI:
 		return tiocsti(tty, p);
 	case TIOCGWINSZ:
-		return tiocgwinsz(tty, p);
+		return tiocgwinsz(real_tty, p);
 	case TIOCSWINSZ:
 		return tiocswinsz(tty, real_tty, p);
 	case TIOCCONS:

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

[Alan Cox]

O> real_tty structures. TIOCSWINSZ and TIOCGWINSZ ioctls could be
> called on tty and real_tty at the same time. To avoid race condition

No they can't. Would you please bother to spend five minutes actually
reading the source code and following through your assumptions to see if
they make sense before posting.

tiocgwinsz is never called for the pty side of a pty pair.

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

[Adam Tlałka]

Sun, 12 Oct 2008 15:22:00 +0100 - Alan Cox <alan@lxorguk.ukuu.org.uk>:

> O> real_tty structures. TIOCSWINSZ and TIOCGWINSZ ioctls could be
> > called on tty and real_tty at the same time. To avoid race condition
> 
> No they can't. Would you please bother to spend five minutes actually
> reading the source code and following through your assumptions to see
> if they make sense before posting.
> 
> tiocgwinsz is never called for the pty side of a pty pair.

I've read the code. The race problem with xterm or other pty using
program in 2.6.26 appeared because one app called ioctl(TIOCSWINSZ) on
the master side while other read winsize (TIOCGWINSZ) using client side
(slave). So in one ioctl() call tty == master and in other tty ==
real_tty. Of course we can have the opposite situaction so terminal app
is using ioctl(TIOCSWINSZ) on its side (slave) and xterm is using ioctl
on its side to know to which size resize itself. Not working now as I
tested but possible.

Anyway I think that you miss the point. Why using
real_tty->termios_mutex instead of tty->termios_mutex in tty_do_resize
called from tiocswins() so from ioctl(TIOCSWINSZ) closes the race. If as
you said tiocgwinsz is called on tty and not real_tty then
tty->termios_mutex should be valid here.
Mutexes work and it is not a scheduler problem as I wrongly assumed.
The scheduler just exposed this problem doing an app switch.
It's just wrong mutex used.

Look at the  tty_ioctl(struct file *file, unsigned int cmd, unsigned
long arg) in tty_io.c.

	tty = (struct tty_struct *)file->private_data;

so if you calling ioctl on master side we have tty = master
and on client side tty = real_tty in ioctl entry;
next

	real_tty = tty;
	if (tty->driver->type == TTY_DRIVER_TYPE_PTY &&
            tty->driver->subtype == PTY_TYPE_MASTER)
                real_tty = tty->link;

if tty is the master one we set real_tty but in case of client side
tty == real_tty already so real_tty points to the same structure.

So it seems that tty->termios_mutex could point to different
location in different calls but real_tty->termios_mutex always points
to the same location.
 
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/2] SIGWINCH problem with terminal apps still alive

[Alan Cox]

> I've read the code. The race problem with xterm or other pty using

Clearly not.

> program in 2.6.26 appeared because one app called ioctl(TIOCSWINSZ) on
> the master side while other read winsize (TIOCGWINSZ) using client side
> (slave). So in one ioctl() call tty == master and in other tty ==
> real_tty. Of course we can have the opposite situaction so terminal app

Then we end up with both using real_tty.

> Anyway I think that you miss the point. Why using
> real_tty->termios_mutex instead of tty->termios_mutex in tty_do_resize

To avoid deadlocks if you took both as you updated both structures.

> So it seems that tty->termios_mutex could point to different
> location in different calls but real_tty->termios_mutex always points
> to the same location.

You've finally got there - we always work off real_tty. That is why we
can safely use the mutex on the real_tty side.

Alan

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

[Adam Tlałka]

Sun, 12 Oct 2008 19:03:12 +0100 - Alan Cox <alan@lxorguk.ukuu.org.uk>:
> Then we end up with both using real_tty.
> 
> > Anyway I think that you miss the point. Why using
> > real_tty->termios_mutex instead of tty->termios_mutex in
> > tty_do_resize
> 
> To avoid deadlocks if you took both as you updated both structures.

Actually to avoid race and not valid winsize reading because
ioctl(TIOCGWINSZ) is done with tty == real_tty so we have to use this
mutex and not the other one.
What is more because we are using real_tty we could safely use only
real_tty->winsize and forget about master tty->winsize.
 
> > So it seems that tty->termios_mutex could point to different
> > location in different calls but real_tty->termios_mutex always
> > points to the same location.
> 
> You've finally got there - we always work off real_tty. That is why we
> can safely use the mutex on the real_tty side.

typically ioctl(,TIOCSWINSZ,) is called on master side but
ioctl(,TIOCGWINSZ,) is called on slave side so tty is not the same in
both calls inside ioctl() in tty_io.c. Only real_tty variable has the
same value for the same master/slave pair. So we must use real_tty
mutex all the time because we not always work from the same side IMHO.

That is why I propose usage of real_tty in ioctl() handling:

case TIOCGWINSZ:
                return tiocgwinsz(real_tty, p);


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/2] SIGWINCH problem with terminal apps still alive

[Alan Cox]

> That is why I propose usage of real_tty in ioctl() handling:
> 
> case TIOCGWINSZ:
>                 return tiocgwinsz(real_tty, p);

Congratulations we've finally got there - and that is exactly the code in
the current tty tree.

Alan

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

[Bodo Eggert]

On Sun, 12 Oct 2008, Alan Cox wrote:

> > That is why I propose usage of real_tty in ioctl() handling:
> > 
> > case TIOCGWINSZ:
> >                 return tiocgwinsz(real_tty, p);
> 
> Congratulations we've finally got there - and that is exactly the code in
> the current tty tree.

These patches are missing from 2.6.27. There you have:

    case TIOCGWINSZ:
        return tiocgwinsz(tty, p);
-- 
     WAR IS PEACE
  FREEDOM IS SLAVERY
IGNORANCE IS STRENGTH

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

[Alan Cox]

On Mon, 13 Oct 2008 11:59:52 +0200 (CEST)
Bodo Eggert <7eggert@gmx.de> wrote:

> On Sun, 12 Oct 2008, Alan Cox wrote:
> 
> > > That is why I propose usage of real_tty in ioctl() handling:
> > > 
> > > case TIOCGWINSZ:
> > >                 return tiocgwinsz(real_tty, p);
> > 
> > Congratulations we've finally got there - and that is exactly the code in
> > the current tty tree.
> 
> These patches are missing from 2.6.27. There you have:
> 
>     case TIOCGWINSZ:
>         return tiocgwinsz(tty, p);

Yes I know. The rules for the stable tree are that it has to go upstream
first. It has not gone upstream so it cannot go into the stable tree yet.
As and when Linus takes the relevant changes I will send a set to the
stable tree.

Alan

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

[Bodo Eggert]

On Mon, 13 Oct 2008, Alan Cox wrote:
> Bodo Eggert <7eggert@gmx.de> wrote:
> > On Sun, 12 Oct 2008, Alan Cox wrote:

> > > > That is why I propose usage of real_tty in ioctl() handling:
> > > > 
> > > > case TIOCGWINSZ:
> > > >                 return tiocgwinsz(real_tty, p);
> > > 
> > > Congratulations we've finally got there - and that is exactly the code in
> > > the current tty tree.
> > 
> > These patches are missing from 2.6.27. There you have:
> > 
> >     case TIOCGWINSZ:
> >         return tiocgwinsz(tty, p);
> 
> Yes I know. The rules for the stable tree are that it has to go upstream
> first. It has not gone upstream so it cannot go into the stable tree yet.
> As and when Linus takes the relevant changes I will send a set to the
> stable tree.

Nice to hear. I misunderstood the waiting for 2.6.27 part as in "Will be 
fixed there" ... until I saw your patchset.
-- 
You must be the arithmetic man -- you add trouble, subtract pleasure, divide
attention, and multiply ignorance. You have an inferiority complex -- and
it's fully justified. You used to be arrogant and obnoxious. Now you are
just the opposite. You are obnoxious and arrogant. -- Harry Murphy in a.2.hz

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

[Adam Tlałka]

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

Hello,

I am sending sligtly corrected patch.
In case of ioctl(,TIOCSWINSZ,) on pty side we should send signal to
master side too. I've tested it on modified unix putty version
and it works properly.

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

Maybe we could optimize code more and not call 
get_pid() and put_pid() if they are not needed - in case
where there is no master/slave pair.

Regards

-- 
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.27_tty_io_3.patch --]
[-- Type: text/x-patch, Size: 1879 bytes --]

--- tty_io_orig.c	2008-10-10 05:37:30.000000000 +0200
+++ tty_io.c	2008-10-14 14:28:50.000000000 +0200
@@ -2522,29 +2522,33 @@ int tty_do_resize(struct tty_struct *tty
 	struct pid *pgrp, *rpgrp;
 	unsigned long flags;
 
-	/* For a PTY we need to lock the tty side */
+        /* in case of resize ioctl on slave */
+	if ((tty == real_tty)
+	    && tty->driver->type == TTY_DRIVER_TYPE_PTY)
+		tty = tty->link;
+	
+	/* for a PTY we need to lock the tty side */
 	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;
+	flags = memcmp(ws, &real_tty->winsize, sizeof(*ws));
 	real_tty->winsize = *ws;
-done:
 	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;
 }
 
@@ -2996,7 +3000,7 @@ long tty_ioctl(struct file *file, unsign
 	case TIOCSTI:
 		return tiocsti(tty, p);
 	case TIOCGWINSZ:
-		return tiocgwinsz(tty, p);
+		return tiocgwinsz(real_tty, p);
 	case TIOCSWINSZ:
 		return tiocswinsz(tty, real_tty, p);
 	case TIOCCONS:

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

[Adam Tlałka]

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

> Hello,
> 
> I am sending sligtly corrected patch.
> In case of ioctl(,TIOCSWINSZ,) on pty side we should send signal to
> master side too. I've tested it on modified unix putty version
> and it works properly.
> 
> Signed-off-by: Adam Tla/lka <atlka@pg.gda.pl>
> 
> Maybe we could optimize code more and not call 
> get_pid() and put_pid() if they are not needed - in case
> where there is no master/slave pair.

Not it is after small changes and proper path in patch.

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

Regards

-- 
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.27_tty_io_4.patch --]
[-- Type: text/x-patch, Size: 1908 bytes --]

--- drivers/char/tty_io_orig.c	2008-10-10 05:37:30.000000000 +0200
+++ drivers/char/tty_io.c	2008-10-14 15:58:54.000000000 +0200
@@ -2522,29 +2522,32 @@ int tty_do_resize(struct tty_struct *tty
 	struct pid *pgrp, *rpgrp;
 	unsigned long flags;
 
-	/* For a PTY we need to lock the tty side */
+        /* in case of resize ioctl on slave */
+	if ((tty == real_tty) && (tty->driver->type == TTY_DRIVER_TYPE_PTY))
+		tty = tty->link;
+	
+	/* for a PTY we need to lock the tty side */
 	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;
+	flags = memcmp(ws, &real_tty->winsize, sizeof(*ws));
 	real_tty->winsize = *ws;
-done:
 	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);
+			put_pid(pgrp);
+		}
+		if (rpgrp != pgrp && rpgrp){
+			kill_pgrp(rpgrp, SIGWINCH, 1);
+			put_pid(rpgrp);
+		}
+	}
+	
 	return 0;
 }
 
@@ -2996,7 +2999,7 @@ long tty_ioctl(struct file *file, unsign
 	case TIOCSTI:
 		return tiocsti(tty, p);
 	case TIOCGWINSZ:
-		return tiocgwinsz(tty, p);
+		return tiocgwinsz(real_tty, p);
 	case TIOCSWINSZ:
 		return tiocswinsz(tty, real_tty, p);
 	case TIOCCONS:

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

[Adam Tlałka]

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

Hello,

actual pty ioctl(,TIOCSWINSZ,) handling is broken IMHO.
I we do that action on normal tty (console for example)
some resize is done or not and resulting size variables are updated
and signal generated. In case of pty ioctl() on slave side it just sets
pty size variables, generates SIGWINCH, but terminal is not changed so
a terminal app will go crazy now. I propose changes which lead to more
consistent handling:

1. set in non pty master/slave case: no changes
2. set in pty master case: we update all sizes and do SIGWINCH on slave
side
3. set in pty slave  case: we update only master variable and do
SIGWINCH on master side
4. read: master reads master variable  while slave reads slave variable

Now if xterm resizes itself then a program on slave gets its signal
but if this program sets terminal sizes by ioctl then only xterm gets
the SIGWINCH signal and could read desired sizes by ioctl and then
resize itself and set valid sizes on slave side by another ioctl() call.
If it not supports this method then there will be no changes on slave
side. I think that it is more proper so on the slave side we will see
always actual values and if terminal resizes we will get SIGWINCH. 

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

-- 
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.27_tty_io_5.patch --]
[-- Type: text/x-patch, Size: 3077 bytes --]

--- drivers/char/tty_io_orig.c	2008-10-10 05:37:30.000000000 +0200
+++ drivers/char/tty_io.c	2008-10-16 11:25:53.000000000 +0200
@@ -2490,17 +2490,17 @@ static int tiocsti(struct tty_struct *tt
  *
  *	Copies the kernel idea of the window size into the user buffer.
  *
- *	Locking: tty->termios_mutex is taken to ensure the winsize data
+ *	Locking: real_tty->termios_mutex is taken to ensure the winsize data
  *		is consistent.
  */
 
-static int tiocgwinsz(struct tty_struct *tty, struct winsize __user *arg)
+static int tiocgwinsz(struct tty_struct *tty, struct tty_struct *real_tty, struct winsize __user *arg)
 {
 	int err;
 
-	mutex_lock(&tty->termios_mutex);
+	mutex_lock(&real_tty->termios_mutex);
 	err = copy_to_user(arg, &tty->winsize, sizeof(*arg));
-	mutex_unlock(&tty->termios_mutex);
+	mutex_unlock(&real_tty->termios_mutex);
 
 	return err ? -EFAULT: 0;
 }
@@ -2519,32 +2519,31 @@ static int tiocgwinsz(struct tty_struct 
 int tty_do_resize(struct tty_struct *tty, struct tty_struct *real_tty,
 					struct winsize *ws)
 {
-	struct pid *pgrp, *rpgrp;
+	struct pid *pgrp;
 	unsigned long flags;
 
-	/* For a PTY we need to lock the tty side */
+	/* for a PTY we need to lock the tty side */
 	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);
-
+	flags = memcmp(ws, &real_tty->winsize, sizeof(*ws));
+	if (tty != real_tty){ /* master side */
+		tty->winsize = *ws;
+		tty = real_tty;
+	} else if (tty->driver->type == TTY_DRIVER_TYPE_PTY){
+		tty = tty->link;
+	}
 	tty->winsize = *ws;
-	real_tty->winsize = *ws;
-done:
 	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);
+		spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+		if (pgrp){
+			kill_pgrp(pgrp, SIGWINCH, 1);
+			put_pid(pgrp);
+		}
+	}
 	return 0;
 }
 
@@ -2570,9 +2569,12 @@ static int tiocswinsz(struct tty_struct 
 	if (copy_from_user(&tmp_ws, arg, sizeof(*arg)))
 		return -EFAULT;
 
-	if (tty->ops->resize)
+	if (tty->ops->resize){
+		if ((tty == real_tty)
+		    && (tty->driver->type == TTY_DRIVER_TYPE_PTY))
+			tty = tty->link;
 		return tty->ops->resize(tty, real_tty, &tmp_ws);
-	else
+	} else
 		return tty_do_resize(tty, real_tty, &tmp_ws);
 }
 
@@ -2996,7 +2998,7 @@ long tty_ioctl(struct file *file, unsign
 	case TIOCSTI:
 		return tiocsti(tty, p);
 	case TIOCGWINSZ:
-		return tiocgwinsz(tty, p);
+		return tiocgwinsz(tty, real_tty, p);
 	case TIOCSWINSZ:
 		return tiocswinsz(tty, real_tty, p);
 	case TIOCCONS:

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

[Alan Cox]

> and signal generated. In case of pty ioctl() on slave side it just sets
> pty size variables, generates SIGWINCH, but terminal is not changed so
> a terminal app will go crazy now. I propose changes which lead to more
> consistent handling:

It sets the tty and pty side variables.

> Now if xterm resizes itself then a program on slave gets its signal
> but if this program sets terminal sizes by ioctl then only xterm gets
> the SIGWINCH signal and could read desired sizes by ioctl and then
> resize itself and set valid sizes on slave side by another ioctl() call.
> If it not supports this method then there will be no changes on slave
> side. I think that it is more proper so on the slave side we will see
> always actual values and if terminal resizes we will get SIGWINCH. 

The current and historic behaviour is I believe correct and matches other
Unixes.

Your patch doesn't really seem to make a lot of sense either. You add pty
special cases in places they are not needed and you pass various extra
arguments to functions that don't need them.

I did actually have a glance at the pty signalling question a couple of
days ago while further tidying up the default resize logic - see the
ttydev tree. I'm cautious about changing the signal behaviour however
without having a hard look to see whether any other Unixen has that
behaviour currently as we may risk breaking stuff.

Alan

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

[Adam Tlałka]

Welcome,
Thu, 16 Oct 2008 11:52:02 +0100 - Alan Cox <alan@lxorguk.ukuu.org.uk>:

> > Now if xterm resizes itself then a program on slave gets its signal
> > but if this program sets terminal sizes by ioctl then only xterm
> > gets the SIGWINCH signal and could read desired sizes by ioctl and
> > then resize itself and set valid sizes on slave side by another
> > ioctl() call. If it not supports this method then there will be no
> > changes on slave side. I think that it is more proper so on the
> > slave side we will see always actual values and if terminal resizes
> > we will get SIGWINCH. 
> 
> The current and historic behaviour is I believe correct and matches
> other Unixes.

I don't know what it means ,,correct'' here. stty(,TIOCSWINSZ,) on pty
slave in its correct behaviour is unusable and leads to application
confuse and sometimes crash.

> Your patch doesn't really seem to make a lot of sense either. You add
> pty special cases in places they are not needed and you pass various
> extra arguments to functions that don't need them.

I do not agree. Adding real_tty arg to tiocgwinsz() is for purpose
of using always the same mutex but read winsize depending on call
side (master or slave). So master could get what it should set and
slave reads always actual terminal values corresponding to real
terminal sizes.
 
> I did actually have a glance at the pty signalling question a couple
> of days ago while further tidying up the default resize logic - see
> the ttydev tree. I'm cautious about changing the signal behaviour
> however without having a hard look to see whether any other Unixen
> has that behaviour currently as we may risk breaking stuff.

Other Unixes seem to have the ,,correct'' not quite usable behaviour ;).
If we want to stick to this my previous patch - [PATCH 0/4] seems to be
suitable.
It only corrects situaction with mutex usage and sending signal to the
master side in case of ioctl(,TIOCSWINS,) done on the slave side.
So a teminal emulator can get signal too and react. But 1. an app can
get its signal before terminal emulator while it is not really resized
yet; 2. a terminal emulator does not support SIGWINCH so we have no
resize but variable is updated and signal generated. So it is not usable
anymore till terminal emulator resizes by itself - but if it changes
sizes to values retuned by ioctl(,TIOCGWINSZ,) there will be no
SIGWINCH send to the app. So you have to manually force it to repaint
its screen so it looks correctly.

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/5] SIGWINCH problem with terminal apps still alive

[Adam Tlałka]

Welcome,

If we want to be more consistent with console behaviour the ptm/pty
behaviour should be similar. In my opinion it could be done
be using some kind of callback function which could be registered by
the program which rules ptm. Then it could be activated through
tty->ops->resize from tiocswinsz in tty_io. The default will be not set
so we get current typical behaviour.

We could also use some default function which calls tty_do_resize() if
called from master side and does nothing if called from slave side
so there is no change because real terminal sizes are not changed.
The terminal emulator program should register its callback
function which leads to proper resize and then signal generating by
ioctl(ptm,TIOCSWINSZ,). It is different from current behaviour but more
usable and sane IMHO.

The current ,,proper'' behaviour is inconsistent. For example you can
do "stty rows 0" on console and there is no change at all and no error
reporting too. With pty we get SIGWINCH signal and variable rows is
set to 0. Of course the real physical terminal is not changed at all.


Regards

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