[PATCH 1/3] signals: sigqueue_free: don't free sigqueue if it is queued

[PATCH 1/3] signals: sigqueue_free: don't free sigqueue if it is queued

[Oleg Nesterov]

Currently sigqueue_free() removes sigqueue from list, but doesn't cancel the
pending signal. This is not consistent, the task should either receive the
"full" signal along with siginfo_t, or it shouldn't receive the signal at all.

Change sigqueue_free() to clear SIGQUEUE_PREALLOC but leave sigqueue on list
if it is queued.

This patch doesn't change the behaviour of sys_timer_delete() and friends,
just makes it more correct and allows us to introduce other SIGQUEUE_ flags
passed to the receiver.

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

--- 25/kernel/signal.c~4_SF_DONT_REMOVE	2008-05-17 16:22:07.000000000 +0400
+++ 25/kernel/signal.c	2008-05-17 17:14:04.000000000 +0400
@@ -1240,18 +1240,22 @@ void sigqueue_free(struct sigqueue *q)
 
 	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
 	/*
-	 * If the signal is still pending remove it from the
-	 * pending queue. We must hold ->siglock while testing
-	 * q->list to serialize with collect_signal() or with
+	 * We must hold ->siglock while testing q->list
+	 * to serialize with collect_signal() or with
 	 * __exit_signal()->flush_sigqueue().
 	 */
 	spin_lock_irqsave(lock, flags);
+	q->flags &= ~SIGQUEUE_PREALLOC;
+	/*
+	 * If it is queued it will be freed when dequeued,
+	 * like the "regular" sigqueue.
+	 */
 	if (!list_empty(&q->list))
-		list_del_init(&q->list);
+		q = NULL;
 	spin_unlock_irqrestore(lock, flags);
 
-	q->flags &= ~SIGQUEUE_PREALLOC;
-	__sigqueue_free(q);
+	if (q)
+		__sigqueue_free(q);
 }
 
 int send_sigqueue(struct sigqueue *q, struct task_struct *t, int group)

Re: [PATCH 1/3] signals: sigqueue_free: don't free sigqueue if it is queued

[Roland McGrath]

> Currently sigqueue_free() removes sigqueue from list, but doesn't cancel the
> pending signal. This is not consistent, the task should either receive the
> "full" signal along with siginfo_t, or it shouldn't receive the signal at all.

Agreed.

> Change sigqueue_free() to clear SIGQUEUE_PREALLOC but leave sigqueue on list
> if it is queued.
> 
> This patch doesn't change the behaviour of sys_timer_delete() and friends,
> just makes it more correct and allows us to introduce other SIGQUEUE_ flags
> passed to the receiver.

To clarify, this certainly does change the behavior.  
There are two changes.  

Firstly, a pending timer-firing signal currently gets its siginfo_t
zeroed out synchronously by timer_delete and now will have its info
preserved.  That change alone is a potential problem for userland, so
it should not go in without the following changes to prevent userland
from seeing the signal at all.  (Currently userland may see a spurious
signal, but its si_code and si_value don't indicate a timer firing.
With the correct info, userland might try to use a pointer from
si_value that was freed around the time it called timer_delete.)

Second, a pending timer-firing signal currently stops counting towards
the RLIMIT_SIGPENDING limit immediately upon a timer_delete call and
now will keep counting toward that limit until it gets dequeued and
discarded.  This change is not necessarily a problem.  (POSIX does not
specify how we decide when resources are too short to create a new
timer or queue a signal.)  But it deserves mention somewhere.
Applications can just get fixed to always unblock the signal number or
flush old signals out with sigwait, if they are averse to timer
signals with intact siginfo_t arriving after timer_delete.

For this reason, I don't think this set of changes should be
considered for any -stable branch.


Thanks,
Roland

Re: [PATCH 1/3] signals: sigqueue_free: don't free sigqueue if it is queued

[Oleg Nesterov]

On 05/20, Roland McGrath wrote:
>
> > This patch doesn't change the behaviour of sys_timer_delete() and friends,
> > just makes it more correct and allows us to introduce other SIGQUEUE_ flags
> > passed to the receiver.
>
> To clarify, this certainly does change the behavior.
> There are two changes.
>
> Firstly, a pending timer-firing signal currently gets its siginfo_t
> zeroed out synchronously by timer_delete and now will have its info
> preserved.  That change alone is a potential problem for userland, so
> it should not go in without the following changes to prevent userland
> from seeing the signal at all.  (Currently userland may see a spurious
> signal, but its si_code and si_value don't indicate a timer firing.
> With the correct info, userland might try to use a pointer from
> si_value that was freed around the time it called timer_delete.)

Yes, yes, I meant doesn't change the behaviour in a sense that the
signal is still "visible" to the application.

> Second, a pending timer-firing signal currently stops counting towards
> the RLIMIT_SIGPENDING limit immediately upon a timer_delete call and
> now will keep counting toward that limit until it gets dequeued and
> discarded.  This change is not necessarily a problem.  (POSIX does not
> specify how we decide when resources are too short to create a new
> timer or queue a signal.)  But it deserves mention somewhere.
> Applications can just get fixed to always unblock the signal number or
> flush old signals out with sigwait, if they are averse to timer
> signals with intact siginfo_t arriving after timer_delete.

Yes, thanks, I didn't think about this.

> For this reason, I don't think this set of changes should be
> considered for any -stable branch.

Yes sure.


> > 	q->flags |= SIGQUEUE_CANCELLED;
> > 	spin_lock_irqsave(lock, flags);
> > 	q->flags &= ~SIGQUEUE_PREALLOC;
>
> Just make it:
>
> 	spin_lock_irqsave(lock, flags);
> 	q->flags |= SIGQUEUE_CANCELLED;
> 	q->flags &= ~SIGQUEUE_PREALLOC;
>
> and we needn't wax philosophical about the meaning of locking rules.  That
> patch would have my ACK, but I concur with Linus about the undesireability
> of the plain = version.

OK, will do tomorrow, but...

Oh well. I just realized SIGQUEUE_CANCELLED breaks sys_sigpending() ?

Oleg.

Re: [PATCH 1/3] signals: sigqueue_free: don't free sigqueue if it is queued

[Roland McGrath]

> Oh well. I just realized SIGQUEUE_CANCELLED breaks sys_sigpending() ?

Yes, it does.  Well, POSIX says after timer_delete "the disposition of
pending signals for the deleted timer is unspecified".  So perhaps one can
say that "unspecified" here can include sigpending() says it's pending but
it will disappear when delivered or accepted (means sigwait()).  But it's a
bit of a stretch.

Just properly removing the sigqueue entry and fixing the pending set is
looking pretty good.  Why was it we didn't do that?


Thanks,
Roland

Re: [PATCH 1/3] signals: sigqueue_free: don't free sigqueue if it is queued

[Linus Torvalds]

On Wed, 21 May 2008, Roland McGrath wrote:
> 
> Just properly removing the sigqueue entry and fixing the pending set is
> looking pretty good.  Why was it we didn't do that?

I thought we didn't even know which queue it was pending on if it was 
already on a thread-local queue. So we could remove the entry, but I 
always objected to the games with the pending bit.

Just removing the entry I'm ok with, it was the (pointless and misleading) 
use of recalc_sigpending() that started the whole discussion. The fact 
that we then also have that "which signal is pending" bit in front of the 
queue was something that came up later.

		Linus

Re: [PATCH 1/3] signals: sigqueue_free: don't free sigqueue if it is queued

[Roland McGrath]

> I thought we didn't even know which queue it was pending on if it was 
> already on a thread-local queue. So we could remove the entry, but I 
> always objected to the games with the pending bit.

Removing the entry without fixing the pending set is the bug we're trying
to fix.  That's what it does now, and it's wrong.

Oleg had a patch that marked the sigqueue entry with whether it was on the
shared queue or not.  The caller in timer_delete knows which thread it is
when it's on a thread queue, and whether it's on the shared queue.  So it
could be the caller's responsibility to know, i.e. its sigqueue_free call
matches its send_sigqueue call.


Thanks,
Roland

Re: [PATCH 1/3] signals: sigqueue_free: don't free sigqueue if it is queued

[Linus Torvalds]

On Wed, 21 May 2008, Roland McGrath wrote:
> 
> Removing the entry without fixing the pending set is the bug we're trying
> to fix.  That's what it does now, and it's wrong.

Well, I think we could/should look at the bigger picture.

The whole (and only!) reason for this problem in the first place is that 
the generic signal-handling code is simply not designed for signals going 
away. Because they simply don't. This whole sys_timer_delete() thing is 
purely based on that problem.

So we have a few options here, I think.

One is the approach that Oleg has taken, which is to try to remove the 
signals. Quite frankly, I don't much like it, because it's against all the 
normal signal handler behaviour. And being against all the normal signal 
handler behaviour, it violates the assumptions we have about signals being 
sticky, which is why it then has problems with both the task sigpending 
bit and the per-signal pending bits.

We can continue with that approach, but judging by the issues, I suspect 
it basically involves having to do back-pointers from the signal info to 
the queues they are on, and even then we'd always have the issues with 
code that simply assumes that signals are sticky. The simple fact is, we 
have lots of system call code that does

	if (signal_pending())
		return -ERESTARTNOHAND;

etc, so even if we don't actually *take* the signal, a cancelled signal 
would still result in a spurious restart or worse - an EINTR.

But there are other approaches. For example, afaik, this really is more 
about execve() than about anything else. How about we just do a special 
case in "flush_signal_handlers()" - rather than try to make it an issue of 
releasing the POSIX timer.

We know those posix timer things are special. We know they go away at 
execve() time. There's this unlucky race condition that only happens at 
execve time, and only because we must flush the pending signal handlers in 
a way that we normally *don't* flush any other signals.

So we could easily make the POSIX timer code just mark the signals it 
sends, and then at execve() time (in "flush_signal_handlers()") we walk 
the signal queues of that thread and get rid of those signal entries.

And at *that* point it is trivial to clear re-calculate the pending bit 
entirely for that thread, because we know there is nothing else going on.

But doing it in general, when there may be multiple active threads, and 
one of them does an "timer_delete()" system call, that's actually hard. 
The other threads may be doing other things, and may validly have that 
timer pending.

Hmm?

I bet there are other approaches to this. But I think "sigqueue_free()" is 
fundamentally hard to fix to do what we want to do, without making signals 
do something that they normally don't have to do.

		Linus

Re: [PATCH 1/3] signals: sigqueue_free: don't free sigqueue if it is queued

[Roland McGrath]

The timer_delete/sigqueue_free behavior predates my involvement with the
code.  I think it is fine to leave pending signals queued.  (POSIX makes it
unspecified for deleted timers.  A robust application cannot even assume
that they will or won't be removed consistently on one running system.)

What I think everyone agrees is wrong in the abstract is the status quo,
where (effectively) signals stay queued but with zeroed siginfo_t values.
(This violates POSIX.)  If there is a signal, its siginfo_t must be intact.

A robust application has to drain any pending timer signals by unblocking
the signal or using sigwait.  This is true if signals might remain
properly queued as POSIX permits.  It's also true of existing Linux
kernels, where a bogus infoless signal remains pending.

There is also the caveat I mentioned before about RLIMIT_SIGPENDING.
An existing dumb application might do many iterations of timer_create,
timer_settime, (timer fires), timer_delete, all with the timer signal
blocked, and not expect that it can hit its rlimit and not be able to
create more timers until it drains those signals.  I don't mind breaking
any such application.  It's dumb.

As I read it, POSIX leaves it the same unspecified what is the
disposition of pending timer signals at the time of the implicit timer
deletion at exec.  But people sure find it surprising on exec.

So maybe this (wholly untested)?
The last hunk could go alone (just keep queued) before the rest (exec flush).


Thanks,
Roland


diff --git a/kernel/signal.c b/kernel/signal.c
index 72bb4f5..0000000 100644  
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -205,18 +205,25 @@ static void __sigqueue_free(struct sigqu
 	kmem_cache_free(sigqueue_cachep, q);
 }
 
-void flush_sigqueue(struct sigpending *queue)
+static void __flush_sigqueue(struct sigpending *queue, int timers)
 {
 	struct sigqueue *q;
 
 	sigemptyset(&queue->signal);
 	while (!list_empty(&queue->list)) {
 		q = list_entry(queue->list.next, struct sigqueue , list);
+		if (timers && q->info.si_code != SI_TIMER)
+			continue;
 		list_del_init(&q->list);
 		__sigqueue_free(q);
 	}
 }
 
+void flush_sigqueue(struct sigpending *queue)
+{
+	__flush_sigqueue(queue, 0);
+}
+
 /*
  * Flush all pending signals for a task.
  */
@@ -243,6 +250,7 @@ void ignore_signals(struct task_struct *
 
 /*
  * Flush all handlers for a task.
+ * Also flush all SI_TIMER signals from the queues.
  */
 
 void
@@ -257,6 +265,9 @@ flush_signal_handlers(struct task_struct
 		sigemptyset(&ka->sa.sa_mask);
 		ka++;
 	}
+
+	__flush_sigqueue(&t->pending, 1);
+	__flush_sigqueue(&t->signal->shared_pending, 1);
 }
 
 int unhandled_signal(struct task_struct *tsk, int sig)
@@ -1240,17 +1251,18 @@ void sigqueue_free(struct sigqueue *q)
 
 	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
 	/*
-	 * If the signal is still pending remove it from the
-	 * pending queue. We must hold ->siglock while testing
+	 * If the signal is still pending, leave it to be dequeued.
+	 * We must hold ->siglock while testing
 	 * q->list to serialize with collect_signal().
 	 */
 	spin_lock_irqsave(lock, flags);
+	q->flags &= ~SIGQUEUE_PREALLOC;
 	if (!list_empty(&q->list))
-		list_del_init(&q->list);
+		q = NULL;
 	spin_unlock_irqrestore(lock, flags);
 
-	q->flags &= ~SIGQUEUE_PREALLOC;
-	__sigqueue_free(q);
+	if (q)
+		__sigqueue_free(q);
 }
 
 int send_sigqueue(struct sigqueue *q, struct task_struct *t, int group)

Re: [PATCH 1/3] signals: sigqueue_free: don't free sigqueue if it is queued

[Oleg Nesterov]

On 05/21, Roland McGrath wrote:
>
> -void flush_sigqueue(struct sigpending *queue)
> +static void __flush_sigqueue(struct sigpending *queue, int timers)
>  {
>  	struct sigqueue *q;
>  
>  	sigemptyset(&queue->signal);
>  	while (!list_empty(&queue->list)) {
>  		q = list_entry(queue->list.next, struct sigqueue , list);
> +		if (timers && q->info.si_code != SI_TIMER)
> +			continue;
>  		list_del_init(&q->list);
>  		__sigqueue_free(q);
>  	}
>  }

This is not enough. Again, we remove and free sigqueue but don't discard
the pending signal. (and we must take into account other rt signals with
the same si_signo if we want to discard the signal).

Oh, this problem is unexpectedly nasty. It is trivial and minor, we can
solve it in may ways, but personally I can't find a simple/clean way.

Let's look at my first attempt,

	http://marc.info/?l=linux-kernel&m=120888210417700

the patch was "almost" correct.
We can add the "bool cancel" parameter to sigqueue_free(), true when
called from exec (or exit_itimers). In that case SIGQUEUE_SHARED_PENDING
is enough: the pending signal was either sent to current, or it is group
wide. Not nice too of course, but afaics a bit simpler. Actually, the
patch exists: http://marc.info/?l=linux-kernel&m=120888210417698

What do you think?

(instead of SIGQUEUE_SHARED_PENDING, we can encode "struct sigpending *"
 in q->flags, but this is really awful and I agree with Linus on the
 EINTR/etc issues).

I'll try to think more on Weekend.

Oleg.

Re: [PATCH 1/3] signals: sigqueue_free: don't free sigqueue if it is queued

[Oleg Nesterov]

On 05/22, Oleg Nesterov wrote:
>
> On 05/21, Roland McGrath wrote:
> >
> > -void flush_sigqueue(struct sigpending *queue)
> > +static void __flush_sigqueue(struct sigpending *queue, int timers)
> >  {
> >  	struct sigqueue *q;
> >  
> >  	sigemptyset(&queue->signal);
> >  	while (!list_empty(&queue->list)) {
> >  		q = list_entry(queue->list.next, struct sigqueue , list);
> > +		if (timers && q->info.si_code != SI_TIMER)
> > +			continue;
> >  		list_del_init(&q->list);
> >  		__sigqueue_free(q);
> >  	}
> >  }
> 
> This is not enough. Again, we remove and free sigqueue but don't discard
> the pending signal. (and we must take into account other rt signals with
> the same si_signo if we want to discard the signal).
> 
> Oh, this problem is unexpectedly nasty. It is trivial and minor, we can
> solve it in may ways, but personally I can't find a simple/clean way.
> 
> Let's look at my first attempt,
> 
> 	http://marc.info/?l=linux-kernel&m=120888210417700
> 
> the patch was "almost" correct.
> We can add the "bool cancel" parameter to sigqueue_free(), true when
> called from exec (or exit_itimers). In that case SIGQUEUE_SHARED_PENDING
> is enough: the pending signal was either sent to current, or it is group
> wide. Not nice too of course, but afaics a bit simpler.

I take my words back. It is not simpler. How about

	void xxx(struct sigpending *pending)
	{
		struct sigqueue *q;
		sigset_t drop, retain;

		sigemptyset(&drop);
		sigemptyset(&retain);

		list_for_each_entry_safe(q) {
			int sig = q->info.si_signo;

			// it is better to add another SIGQUEUE_ flag...
			if (q->info.si_code == SI_TIMER) {
				list_del_init(&q->list);
				__sigqueue_free(q);
				sigaddset(&drop, sig);
			} else
				sigaddset(&retain, sig);
		}

		// pseudo code
		pending->signal &= ~(drop & ~retain);
	}

?

This helper is called somewhere near flush_signal_handlers() or
de_thread()->exit_itimers().

We still need the "sigqueue_free: don't free sigqueue if it is queued"
patch of course.

Oleg.

Re: [PATCH 1/3] signals: sigqueue_free: don't free sigqueue if it is queued

[Oleg Nesterov]

On 05/22, Roland McGrath wrote:
>
> > This is not enough. Again, we remove and free sigqueue but don't discard
> > the pending signal. (and we must take into account other rt signals with
> > the same si_signo if we want to discard the signal).
> 
> Right, of course.  The sigset_t collecting while checking the queue is the
> only way to do it.  Note that you need to make it:
> 
> 	if (q->info.si_code == SI_TIMER && sig >= SIGRTMIN)
> 
> because a SI_TIMER could be using a legacy_queue signal number.

Argh ;)

I hoped you will not notice this. Because this is a separate problem
which needs a separate discussion.

Please note that send_sigqueue() does not check if legacy_queue signal
is already pending. The recent changes in signal.c carefully preserve
this peculiarity. This is the question I was going to ask 1000 times
but forgot all the time.

So. Let's suppose that (say) SIGHUP is pending, and the posix timer
sends SIGHUP too. In that case the new SIGHUP is added, and if the
previous one was infoless the new one "hides" it: collect_signal()
won't see 2 distinct signals, it will find only 1 signal and clear
the bit in sigpending->signal.

So I think we shouldn't check "sig >= SIGRTMIN", but perhaps send_signal()
should be fixed? Anyway, I believe this is a separate problem.

Oleg.

Re: [PATCH 1/3] signals: sigqueue_free: don't free sigqueue if it is queued

[Roland McGrath]

> This is not enough. Again, we remove and free sigqueue but don't discard
> the pending signal. (and we must take into account other rt signals with
> the same si_signo if we want to discard the signal).

Right, of course.  The sigset_t collecting while checking the queue is the
only way to do it.  Note that you need to make it:

	if (q->info.si_code == SI_TIMER && sig >= SIGRTMIN)

because a SI_TIMER could be using a legacy_queue signal number.  In that
case there might have been a second non-timer signal pending with that
number too, which must not get cleared.  Since the only siginfo_t still
recorded is the SI_TIMER one, better to leave it queued than turn it into
an infoless signal, seems to me.


Thanks,
Roland

Re: [PATCH 1/3] signals: sigqueue_free: don't free sigqueue if it is queued

[Roland McGrath]

> So. Let's suppose that (say) SIGHUP is pending, and the posix timer
> sends SIGHUP too. In that case the new SIGHUP is added, and if the
> previous one was infoless the new one "hides" it: collect_signal()
> won't see 2 distinct signals, it will find only 1 signal and clear
> the bit in sigpending->signal.

That is correct behavior.  Signals < SIGRTMIN do not queue.  (POSIX says
it's implementation-defined whether they do, and in Linux they don't.)
When a signal does not queue, it's unspecified whether the the one that
is ever actually seen is the first one or the last one (or any in between),
so the siginfo_t details showing the second one in your example is fine.


Thanks,
Roland