Re: [PATCH 1/1] tty: disassociate_ctty() sends the extra SIGCONT
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(>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(>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(>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(>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(>ctrl_lock);
spin_unlock_irq(>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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] tty: disassociate_ctty() sends the extra SIGCONT
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] tty: disassociate_ctty() sends the extra SIGCONT
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(>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(>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(>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.
My point was, this _looks_ confusing, and the patch I sent makes it
more clean.
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? Otherwise ->tty_old_pgrp != NULL looks
certainly wrong after put_pid().
Oleg.
--- 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(>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(>ctrl_lock);
spin_unlock_irq(>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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] tty: disassociate_ctty() sends the extra SIGCONT
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.
My point was, this _looks_ confusing, and the patch I sent makes it
more clean.
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? Otherwise -tty_old_pgrp != NULL looks
certainly wrong after put_pid().
Oleg.
--- 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] tty: disassociate_ctty() sends the extra SIGCONT
On 09/21/2013 02:34 PM, Oleg Nesterov wrote:
Peter, sorry for delay, I was sick.
On 09/17, Peter Hurley wrote:
On 09/15/2013 11:50 AM, Oleg Nesterov wrote:
Put the "!on_exit" check back to restore the old behaviour.
Cc: sta...@vger.kernel.org # v3.10+
Signed-off-by: Oleg Nesterov
Reported-by: Karel Srot
Reviewed-by: Peter Hurley
Thanks!
Can I ask the question? tty_signal_session_leader() is probably fine,
but it _looks_ buggy or at least confusing to me.
do_each_pid_task(tty->session, PIDTYPE_SID, p) {
spin_lock_irq(>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(>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(>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.
Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] tty: disassociate_ctty() sends the extra SIGCONT
Peter, sorry for delay, I was sick.
On 09/17, Peter Hurley wrote:
>
> On 09/15/2013 11:50 AM, Oleg Nesterov wrote:
>
>> Put the "!on_exit" check back to restore the old behaviour.
>>
>> Cc: sta...@vger.kernel.org # v3.10+
>> Signed-off-by: Oleg Nesterov
>> Reported-by: Karel Srot
>
> Reviewed-by: Peter Hurley
Thanks!
Can I ask the question? tty_signal_session_leader() is probably fine,
but it _looks_ buggy or at least confusing to me.
do_each_pid_task(tty->session, PIDTYPE_SID, p) {
spin_lock_irq(>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(>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(>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.
Oleg.
--- x/drivers/tty/tty_io.c
+++ x/drivers/tty/tty_io.c
@@ -548,7 +548,7 @@ static int tty_signal_session_leader(str
{
struct task_struct *p;
int refs = 0;
- struct pid *tty_pgrp = NULL;
+ struct pid *tty_pgrp = tty_get_pgrp(tty);
read_lock(_lock);
if (tty->session) {
@@ -560,18 +560,12 @@ static int tty_signal_session_leader(str
the tasklist lock */
refs++;
}
- if (!p->signal->leader) {
- spin_unlock_irq(>sighand->siglock);
- continue;
+ if (p->signal->leader) {
+ __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 */
+ p->signal->tty_old_pgrp = get_pid(tty_pgrp);
}
- __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(>ctrl_lock);
- tty_pgrp = get_pid(tty->pgrp);
- if (tty->pgrp)
- p->signal->tty_old_pgrp = get_pid(tty->pgrp);
- spin_unlock(>ctrl_lock);
spin_unlock_irq(>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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] tty: disassociate_ctty() sends the extra SIGCONT
Peter, sorry for delay, I was sick.
On 09/17, Peter Hurley wrote:
On 09/15/2013 11:50 AM, Oleg Nesterov wrote:
Put the !on_exit check back to restore the old behaviour.
Cc: sta...@vger.kernel.org # v3.10+
Signed-off-by: Oleg Nesterov o...@redhat.com
Reported-by: Karel Srot ks...@redhat.com
Reviewed-by: Peter Hurley pe...@hurleysoftware.com
Thanks!
Can I ask the question? tty_signal_session_leader() is probably fine,
but it _looks_ buggy or at least confusing to me.
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.
Oleg.
--- x/drivers/tty/tty_io.c
+++ x/drivers/tty/tty_io.c
@@ -548,7 +548,7 @@ static int tty_signal_session_leader(str
{
struct task_struct *p;
int refs = 0;
- struct pid *tty_pgrp = NULL;
+ struct pid *tty_pgrp = tty_get_pgrp(tty);
read_lock(tasklist_lock);
if (tty-session) {
@@ -560,18 +560,12 @@ static int tty_signal_session_leader(str
the tasklist lock */
refs++;
}
- if (!p-signal-leader) {
- spin_unlock_irq(p-sighand-siglock);
- continue;
+ if (p-signal-leader) {
+ __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 */
+ p-signal-tty_old_pgrp = get_pid(tty_pgrp);
}
- __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);
- if (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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] tty: disassociate_ctty() sends the extra SIGCONT
On 09/21/2013 02:34 PM, Oleg Nesterov wrote:
Peter, sorry for delay, I was sick.
On 09/17, Peter Hurley wrote:
On 09/15/2013 11:50 AM, Oleg Nesterov wrote:
Put the !on_exit check back to restore the old behaviour.
Cc: sta...@vger.kernel.org # v3.10+
Signed-off-by: Oleg Nesterov o...@redhat.com
Reported-by: Karel Srot ks...@redhat.com
Reviewed-by: Peter Hurley pe...@hurleysoftware.com
Thanks!
Can I ask the question? tty_signal_session_leader() is probably fine,
but it _looks_ buggy or at least confusing to me.
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.
Regards,
Peter Hurley
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] tty: disassociate_ctty() sends the extra SIGCONT
On 09/15/2013 11:50 AM, Oleg Nesterov wrote:
Starting from v3.10 (probably f91e2590 "tty: Signal foreground
group processes in hangup") disassociate_ctty() sends SIGCONT
if tty && on_exit. This breaks LSB test-suite, in particular
test8 in _exit.c and test40 in sigcon5.c.
Yes, this regression was introduced by me in that commit.
The effect of the regression is that ptys will receive a
SIGCONT when, in similar circumstances, ttys would not.
The fact that two test vectors accidentally tripped over
this regression suggests that some other apps may as well.
Thanks for catching this.
Put the "!on_exit" check back to restore the old behaviour.
Cc: sta...@vger.kernel.org # v3.10+
Signed-off-by: Oleg Nesterov
Reported-by: Karel Srot
Reviewed-by: Peter Hurley
---
drivers/tty/tty_io.c |3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index a9355ce..3a1a01a 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -854,7 +854,8 @@ void disassociate_ctty(int on_exit)
struct pid *tty_pgrp = tty_get_pgrp(tty);
if (tty_pgrp) {
kill_pgrp(tty_pgrp, SIGHUP, on_exit);
- kill_pgrp(tty_pgrp, SIGCONT, on_exit);
+ if (!on_exit)
+ kill_pgrp(tty_pgrp, SIGCONT, on_exit);
put_pid(tty_pgrp);
}
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] tty: disassociate_ctty() sends the extra SIGCONT
On 09/17/2013 03:41 PM, Carlos O'Donell wrote: On 09/16/2013 06:16 PM, Peter Hurley wrote: On 09/15/2013 11:50 AM, Oleg Nesterov wrote: Starting from v3.10 (probably f91e2590 "tty: Signal foreground group processes in hangup") disassociate_ctty() sends SIGCONT if tty && on_exit. This breaks LSB test-suite, in particular test8 in _exit.c and test40 in sigcon5.c. Put the "!on_exit" check back to restore the old behaviour. Cc: sta...@vger.kernel.org # v3.10+ Signed-off-by: Oleg Nesterov Reported-by: Karel Srot Although I confirmed your results with a new unit test, I'd like to review the source code for the reported tests. Where can grab the source for the LSB tests, _exit.c and sigcon5.c? Direct links would be appreciated. wget ftp://ftp.linuxfoundation.org/pub/lsb/test_suites/released-4.1/source/runtime/lsb-test-core-4.1.15-1.src.rpm rpm2cpio lsb-test-core-4.1.15-1.src.rpm | cpio -idmv tar zxvf lsb-test-core-4.1.15.tar.gz tar zxvf lts_vsx-pcts-4.1.15.tgz Source is at: ./tset/POSIX.os/procprim/_exit/_exit.c ./tset/POSIX.os/procprim/sigconcept/sigcon5.c Hi Carlos, Thanks for the links. In all failures the tests are checking for SIGHUP to be sent to a foreground process. It would appear that the additional signal confuses the test. Exactly what semantics should be followed do not seem to be clearly covered by any standards. Therefore it is likely just as valid to say that the LSB tests need to be more robust in the face of the additional signal. I have little experience when it comes this particular area of the kernel or expected behaviour from other OSs. The _exit.c:test8() and sigcon5.c:test40() look correct to me. The fact that they are fragile IMO is a good thing; I could easily envision an app's signal handling being equally fragile. Although SUSv3 & v4 don't preclude the extra SIGCONT, in this specific case, a pty should receive the same signals, in the same order as a regular tty. Thanks again, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] tty: disassociate_ctty() sends the extra SIGCONT
On 09/16/2013 06:16 PM, Peter Hurley wrote: > On 09/15/2013 11:50 AM, Oleg Nesterov wrote: >> Starting from v3.10 (probably f91e2590 "tty: Signal foreground >> group processes in hangup") disassociate_ctty() sends SIGCONT >> if tty && on_exit. This breaks LSB test-suite, in particular >> test8 in _exit.c and test40 in sigcon5.c. >> >> Put the "!on_exit" check back to restore the old behaviour. >> >> Cc: sta...@vger.kernel.org # v3.10+ >> Signed-off-by: Oleg Nesterov >> Reported-by: Karel Srot > > Although I confirmed your results with a new unit test, > I'd like to review the source code for the reported tests. > Where can grab the source for the LSB tests, _exit.c and sigcon5.c? > Direct links would be appreciated. wget ftp://ftp.linuxfoundation.org/pub/lsb/test_suites/released-4.1/source/runtime/lsb-test-core-4.1.15-1.src.rpm rpm2cpio lsb-test-core-4.1.15-1.src.rpm | cpio -idmv tar zxvf lsb-test-core-4.1.15.tar.gz tar zxvf lts_vsx-pcts-4.1.15.tgz Source is at: ./tset/POSIX.os/procprim/_exit/_exit.c ./tset/POSIX.os/procprim/sigconcept/sigcon5.c In all failures the tests are checking for SIGHUP to be sent to a foreground process. It would appear that the additional signal confuses the test. Exactly what semantics should be followed do not seem to be clearly covered by any standards. Therefore it is likely just as valid to say that the LSB tests need to be more robust in the face of the additional signal. I have little experience when it comes this particular area of the kernel or expected behaviour from other OSs. Cheers, Carlos. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] tty: disassociate_ctty() sends the extra SIGCONT
On 09/16/2013 06:16 PM, Peter Hurley wrote: On 09/15/2013 11:50 AM, Oleg Nesterov wrote: Starting from v3.10 (probably f91e2590 tty: Signal foreground group processes in hangup) disassociate_ctty() sends SIGCONT if tty on_exit. This breaks LSB test-suite, in particular test8 in _exit.c and test40 in sigcon5.c. Put the !on_exit check back to restore the old behaviour. Cc: sta...@vger.kernel.org # v3.10+ Signed-off-by: Oleg Nesterov o...@redhat.com Reported-by: Karel Srot ks...@redhat.com Although I confirmed your results with a new unit test, I'd like to review the source code for the reported tests. Where can grab the source for the LSB tests, _exit.c and sigcon5.c? Direct links would be appreciated. wget ftp://ftp.linuxfoundation.org/pub/lsb/test_suites/released-4.1/source/runtime/lsb-test-core-4.1.15-1.src.rpm rpm2cpio lsb-test-core-4.1.15-1.src.rpm | cpio -idmv tar zxvf lsb-test-core-4.1.15.tar.gz tar zxvf lts_vsx-pcts-4.1.15.tgz Source is at: ./tset/POSIX.os/procprim/_exit/_exit.c ./tset/POSIX.os/procprim/sigconcept/sigcon5.c In all failures the tests are checking for SIGHUP to be sent to a foreground process. It would appear that the additional signal confuses the test. Exactly what semantics should be followed do not seem to be clearly covered by any standards. Therefore it is likely just as valid to say that the LSB tests need to be more robust in the face of the additional signal. I have little experience when it comes this particular area of the kernel or expected behaviour from other OSs. Cheers, Carlos. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] tty: disassociate_ctty() sends the extra SIGCONT
On 09/17/2013 03:41 PM, Carlos O'Donell wrote: On 09/16/2013 06:16 PM, Peter Hurley wrote: On 09/15/2013 11:50 AM, Oleg Nesterov wrote: Starting from v3.10 (probably f91e2590 tty: Signal foreground group processes in hangup) disassociate_ctty() sends SIGCONT if tty on_exit. This breaks LSB test-suite, in particular test8 in _exit.c and test40 in sigcon5.c. Put the !on_exit check back to restore the old behaviour. Cc: sta...@vger.kernel.org # v3.10+ Signed-off-by: Oleg Nesterov o...@redhat.com Reported-by: Karel Srot ks...@redhat.com Although I confirmed your results with a new unit test, I'd like to review the source code for the reported tests. Where can grab the source for the LSB tests, _exit.c and sigcon5.c? Direct links would be appreciated. wget ftp://ftp.linuxfoundation.org/pub/lsb/test_suites/released-4.1/source/runtime/lsb-test-core-4.1.15-1.src.rpm rpm2cpio lsb-test-core-4.1.15-1.src.rpm | cpio -idmv tar zxvf lsb-test-core-4.1.15.tar.gz tar zxvf lts_vsx-pcts-4.1.15.tgz Source is at: ./tset/POSIX.os/procprim/_exit/_exit.c ./tset/POSIX.os/procprim/sigconcept/sigcon5.c Hi Carlos, Thanks for the links. In all failures the tests are checking for SIGHUP to be sent to a foreground process. It would appear that the additional signal confuses the test. Exactly what semantics should be followed do not seem to be clearly covered by any standards. Therefore it is likely just as valid to say that the LSB tests need to be more robust in the face of the additional signal. I have little experience when it comes this particular area of the kernel or expected behaviour from other OSs. The _exit.c:test8() and sigcon5.c:test40() look correct to me. The fact that they are fragile IMO is a good thing; I could easily envision an app's signal handling being equally fragile. Although SUSv3 v4 don't preclude the extra SIGCONT, in this specific case, a pty should receive the same signals, in the same order as a regular tty. Thanks again, Peter Hurley -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] tty: disassociate_ctty() sends the extra SIGCONT
On 09/15/2013 11:50 AM, Oleg Nesterov wrote:
Starting from v3.10 (probably f91e2590 tty: Signal foreground
group processes in hangup) disassociate_ctty() sends SIGCONT
if tty on_exit. This breaks LSB test-suite, in particular
test8 in _exit.c and test40 in sigcon5.c.
Yes, this regression was introduced by me in that commit.
The effect of the regression is that ptys will receive a
SIGCONT when, in similar circumstances, ttys would not.
The fact that two test vectors accidentally tripped over
this regression suggests that some other apps may as well.
Thanks for catching this.
Put the !on_exit check back to restore the old behaviour.
Cc: sta...@vger.kernel.org # v3.10+
Signed-off-by: Oleg Nesterov o...@redhat.com
Reported-by: Karel Srot ks...@redhat.com
Reviewed-by: Peter Hurley pe...@hurleysoftware.com
---
drivers/tty/tty_io.c |3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index a9355ce..3a1a01a 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -854,7 +854,8 @@ void disassociate_ctty(int on_exit)
struct pid *tty_pgrp = tty_get_pgrp(tty);
if (tty_pgrp) {
kill_pgrp(tty_pgrp, SIGHUP, on_exit);
- kill_pgrp(tty_pgrp, SIGCONT, on_exit);
+ if (!on_exit)
+ kill_pgrp(tty_pgrp, SIGCONT, on_exit);
put_pid(tty_pgrp);
}
}
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] tty: disassociate_ctty() sends the extra SIGCONT
On 09/15/2013 11:50 AM, Oleg Nesterov wrote: Starting from v3.10 (probably f91e2590 "tty: Signal foreground group processes in hangup") disassociate_ctty() sends SIGCONT if tty && on_exit. This breaks LSB test-suite, in particular test8 in _exit.c and test40 in sigcon5.c. Put the "!on_exit" check back to restore the old behaviour. Cc: sta...@vger.kernel.org # v3.10+ Signed-off-by: Oleg Nesterov Reported-by: Karel Srot Although I confirmed your results with a new unit test, I'd like to review the source code for the reported tests. Where can grab the source for the LSB tests, _exit.c and sigcon5.c? Direct links would be appreciated. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] tty: disassociate_ctty() sends the extra SIGCONT
On 09/15/2013 11:50 AM, Oleg Nesterov wrote: Starting from v3.10 (probably f91e2590 tty: Signal foreground group processes in hangup) disassociate_ctty() sends SIGCONT if tty on_exit. This breaks LSB test-suite, in particular test8 in _exit.c and test40 in sigcon5.c. Put the !on_exit check back to restore the old behaviour. Cc: sta...@vger.kernel.org # v3.10+ Signed-off-by: Oleg Nesterov o...@redhat.com Reported-by: Karel Srot ks...@redhat.com Although I confirmed your results with a new unit test, I'd like to review the source code for the reported tests. Where can grab the source for the LSB tests, _exit.c and sigcon5.c? Direct links would be appreciated. Regards, Peter Hurley -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1] tty: disassociate_ctty() sends the extra SIGCONT
Starting from v3.10 (probably f91e2590 "tty: Signal foreground
group processes in hangup") disassociate_ctty() sends SIGCONT
if tty && on_exit. This breaks LSB test-suite, in particular
test8 in _exit.c and test40 in sigcon5.c.
Put the "!on_exit" check back to restore the old behaviour.
Cc: sta...@vger.kernel.org # v3.10+
Signed-off-by: Oleg Nesterov
Reported-by: Karel Srot
---
drivers/tty/tty_io.c |3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index a9355ce..3a1a01a 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -854,7 +854,8 @@ void disassociate_ctty(int on_exit)
struct pid *tty_pgrp = tty_get_pgrp(tty);
if (tty_pgrp) {
kill_pgrp(tty_pgrp, SIGHUP, on_exit);
- kill_pgrp(tty_pgrp, SIGCONT, on_exit);
+ if (!on_exit)
+ kill_pgrp(tty_pgrp, SIGCONT, on_exit);
put_pid(tty_pgrp);
}
}
--
1.5.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1] tty: disassociate_ctty() sends the extra SIGCONT
Starting from v3.10 (probably f91e2590 tty: Signal foreground
group processes in hangup) disassociate_ctty() sends SIGCONT
if tty on_exit. This breaks LSB test-suite, in particular
test8 in _exit.c and test40 in sigcon5.c.
Put the !on_exit check back to restore the old behaviour.
Cc: sta...@vger.kernel.org # v3.10+
Signed-off-by: Oleg Nesterov o...@redhat.com
Reported-by: Karel Srot ks...@redhat.com
---
drivers/tty/tty_io.c |3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index a9355ce..3a1a01a 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -854,7 +854,8 @@ void disassociate_ctty(int on_exit)
struct pid *tty_pgrp = tty_get_pgrp(tty);
if (tty_pgrp) {
kill_pgrp(tty_pgrp, SIGHUP, on_exit);
- kill_pgrp(tty_pgrp, SIGCONT, on_exit);
+ if (!on_exit)
+ kill_pgrp(tty_pgrp, SIGCONT, on_exit);
put_pid(tty_pgrp);
}
}
--
1.5.5.1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
