Re: [PATCH 1/1] tty: disassociate_ctty() sends the extra SIGCONT

[Peter Hurley]

On 09/22/2013 04:03 PM, Oleg Nesterov wrote:
> On 09/21, Peter Hurley wrote:
> > On 09/21/2013 02:34 PM, Oleg Nesterov wrote:
> > > 		do_each_pid_task(tty->session, PIDTYPE_SID, p) {
> > > 			spin_lock_irq(&p->sighand->siglock);
> > > 			if (p->signal->tty == tty) {
> > > 				p->signal->tty = NULL;
> > > 				/* We defer the dereferences outside fo
> > > 				   the tasklist lock */
> > > 				refs++;
> > > 			}
> > > 			if (!p->signal->leader) {
> > > 				spin_unlock_irq(&p->sighand->siglock);
> > > 				continue;
> > > 			}
> > > 			__group_send_sig_info(SIGHUP, SEND_SIG_PRIV, p);
> > > 			__group_send_sig_info(SIGCONT, SEND_SIG_PRIV, p);
> > > 			put_pid(p->signal->tty_old_pgrp);  /* A noop */
> > > 			spin_lock(&tty->ctrl_lock);
> > > 			tty_pgrp = get_pid(tty->pgrp);
> > >
> > > I guess this can happen only once, so we could even add WARN_ON(tty_pgrp)
> > > before get_pid(). But this look confusing, as if we can do get_pid()
> > > multiple times and leak tty->pgrp.
> > >
> > > 			if (tty->pgrp)
> > > 				p->signal->tty_old_pgrp = get_pid(tty->pgrp);
> > >
> > > else? We already did put_pid(tty_old_pgrp), we should clear it.
> > >
> > > IOW, do you think the patch below makes sense or I missed something?
> > > Just curious.
> >
> > The code block you're referring to only executes once because there is
> > only one session leader.
>
> I understand, and I even mentioned this above.

Ah, ok. I didn't realize the earlier patch was a cleanup attempt and
not fixing something.

> My point was, this _looks_ confusing, and the patch I sent makes it
> more clean.

I agree that this looks confusing, but I'm not sure I agree that your earlier
patch makes it cleaner; maybe a code comment stating that the block only
executes once for the session leader would be more appropriate.

Also, I put the get_pid() in the siglock critical section to prevent
unsafe racing between hangup and ioctl(TIOCSCTTY).

> And what about ->tty_old_pgrp? I still think that at least the patch
> below makes sense. If tty->pgrp == NULL is not possible here (I do
> not know), then why do we check?

tty->pgrp can be NULL here if the session leader is dropping the
controlling terminal association via no_tty(). But in this
case ->tty_old_pgrp will also be NULL.

This race should probably be eliminated by claiming the tty_lock()
in no_tty(), so that it doesn't race with __tty_hangup() at all.

[NB: The other possibility, a second hangup, is no longer possible.]

> Otherwise ->tty_old_pgrp != NULL looks  certainly wrong after put_pid().

I agree this looks fairly suspect; so does

   			put_pid(p->signal->tty_old_pgrp);  /* A noop */

not because of the comment, but because tty_old_pgrp cannot be non-NULL
here:
1. The session leader's tty_old_pgrp is only assigned non-NULL if its
   controlling terminal is hung up.
2. The tty cannot be hung up more than once.
3. If the session leader changes the controlling tty via ioctl(TIOCSCTTY),
   __proc_set_tty() will put_pid(tty_old_pgrp) and reset it to NULL.
   [so tty_old_pgrp is NULL on a subsequent hangup of the new controlling tty].
4. If the session leader drops the controlling terminal association
   via ioctl(TIOCNOTTY), disassociate_tty() will put_pid(tty_old_pgrp)
   and reset it to NULL. [Assuming the race mentioned above is fixed,
   then there is no controlling tty to hangup.]

What about replacing
   			put_pid(p->signal->tty_old_pgrp);  /* A noop */
with
			WARN_ON(p->signal->tty_old_pgrp);
?

And fixing the FIXME in no_tty()?

Regards,
Peter Hurley

> --- x/drivers/tty/tty_io.c
> +++ x/drivers/tty/tty_io.c
> @@ -569,8 +569,7 @@ static int tty_signal_session_leader(str
>   			put_pid(p->signal->tty_old_pgrp);  /* A noop */
>   			spin_lock(&tty->ctrl_lock);
>   			tty_pgrp = get_pid(tty->pgrp);
> -			if (tty->pgrp)
> -				p->signal->tty_old_pgrp = get_pid(tty->pgrp);
> +			p->signal->tty_old_pgrp = get_pid(tty->pgrp);
>   			spin_unlock(&tty->ctrl_lock);
>   			spin_unlock_irq(&p->sighand->siglock);
>   		} while_each_pid_task(tty->session, PIDTYPE_SID, p);


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/