Re: [PATCH] posix-timers: Do not modify an already queued timer signal

[Oleg Nesterov <oleg@tv-sign.ru>]

On 07/18, Mark McLoughlin wrote:
>
> On Thu, 2008-07-17 at 17:55 +0400, Oleg Nesterov wrote:
>
> > I forgot (if ever knew ;) this code completely, but can't we make a simpler
> > fix? posix_timer_event() can check list_empty() lockless,
> >
> >         posix_timer_event()
> >         {
> >                 if (!list_emtpy(sigq->list))
> >                         return 0;
> >
> >                 ... fill and send ->sigq...
> >         }
>
> Well, one issue with this is that we need to set the si_private supplied
> to posix_timer_event() on the queued siginfo. See updated version of the
> original patch below.
>
> So, for that reason, we can't currently do it lockless.
>
> Now, I've spent a while looking at the it_requeue_pending code and I
> can't fully satisfy myself that we need it to be a modification counter
> that we match up via si_sys_private. Do you know why this is needed? It
> seems to me that it could be seriously simplified.

No, I don't understand what does si_sys_private mean. In fact I don't even
understand what should we do with info.si_overrun in this corner case.

We have the active timer, the app does sys_timer_settime() which changes the
timer. This looks like creating the new timer which "inherits" ->it_id and
->it_sigev_value. But the queued siginfo is connected to the "old" timer...
OK, I just don't understand this all.

> Subject: [PATCH] posix-timers: Do not modify an already queued timer signal
>
> When a timer fires, posix_timer_event() zeroes out its
> pre-allocated siginfo structure, initialises it and then
> queues up the signal with send_sigqueue().
>
> However, we may have previously queued up this signal, in
> which case we only want to increment si_overrun and
> re-initialising the siginfo structure is incorrect.
>
> Also, since we are modifying an already queued signal
> without the protection of the sighand spinlock, we may also
> race with e.g. collect_signal() causing it to fail to find
> a signal on the pending list because it happens to look at
> the siginfo struct after it was zeroed and before it was
> re-initialised.
>
> The race was observed with a modified kvm-userspace when
> running a guest under heavy network load. When it occurs,
> KVM never sees another SIGALRM signal because although
> the signal is queued up the appropriate bit is never set
> in the pending mask. Manually sending the process a SIGALRM
> kicks it out of this state.

Please update the changelog to explain how it is possible to hit the
already queued siginfo.

> -int posix_timer_event(struct k_itimer *timr,int si_private)
> +int posix_timer_event(struct k_itimer *timr, int si_private)
>  {
> -	memset(&timr->sigq->info, 0, sizeof(siginfo_t));
> -	timr->sigq->info.si_sys_private = si_private;
> -	/* Send signal to the process that owns this timer.*/
> +	siginfo_t info;
>
> -	timr->sigq->info.si_signo = timr->it_sigev_signo;
> -	timr->sigq->info.si_errno = 0;
> -	timr->sigq->info.si_code = SI_TIMER;
> -	timr->sigq->info.si_tid = timr->it_id;
> -	timr->sigq->info.si_value = timr->it_sigev_value;
> +	memset(&info, 0, sizeof(siginfo_t));
> +
> +	info.si_sys_private = si_private;
> +	info.si_signo = timr->it_sigev_signo;
> +	info.si_errno = 0;
> +	info.si_code = SI_TIMER;
> +	info.si_tid = timr->it_id;
> +	info.si_value = timr->it_sigev_value;
>
>  	if (timr->it_sigev_notify & SIGEV_THREAD_ID) {
>  		struct task_struct *leader;
> -		int ret = send_sigqueue(timr->sigq, timr->it_process, 0);
> +		int ret = send_sigqueue(timr->sigq, &info, timr->it_process, 0);

I think this is a bit overkill. Note that (unless I missed something)
posix_timer_event() populates timr->sigq->info with the same numbers
every time, so afaics we can do

	--- kernel/posix-timers.c
	+++ kernel/posix-timers.c
	@@ -298,19 +298,14 @@ void do_schedule_next_timer(struct sigin
	 
	 int posix_timer_event(struct k_itimer *timr,int si_private)
	 {
	-	memset(&timr->sigq->info, 0, sizeof(siginfo_t));
	-	timr->sigq->info.si_sys_private = si_private;
	-	/* Send signal to the process that owns this timer.*/
	-
		timr->sigq->info.si_signo = timr->it_sigev_signo;
	-	timr->sigq->info.si_errno = 0;
		timr->sigq->info.si_code = SI_TIMER;
		timr->sigq->info.si_tid = timr->it_id;
		timr->sigq->info.si_value = timr->it_sigev_value;
	 
		if (timr->it_sigev_notify & SIGEV_THREAD_ID) {
			struct task_struct *leader;
	-		int ret = send_sigqueue(timr->sigq, timr->it_process, 0);
	+		int ret = send_sigqueue(timr->sigq, si_private, timr->it_process, 0);
	 
			if (likely(ret >= 0))
				return ret;
	@@ -321,7 +316,7 @@ int posix_timer_event(struct k_itimer *t
			timr->it_process = leader;
		}
	 
	-	return send_sigqueue(timr->sigq, timr->it_process, 1);
	+	return send_sigqueue(timr->sigq, si_private, timr->it_process, 1);
	 }
	 EXPORT_SYMBOL_GPL(posix_timer_event);
	 
	@@ -435,6 +430,7 @@ static struct k_itimer * alloc_posix_tim
			kmem_cache_free(posix_timers_cache, tmr);
			tmr = NULL;
		}
	+	memset(&timr->sigq->info, 0, sizeof(siginfo_t));
		return tmr;
	 }
	 
	--- kernel/signal.c
	+++ kernel/signal.c
	@@ -1283,7 +1283,7 @@ void sigqueue_free(struct sigqueue *q)
			__sigqueue_free(q);
	 }
	 
	-int send_sigqueue(struct sigqueue *q, struct task_struct *t, int group)
	+int send_sigqueue(struct sigqueue *q, int si_private, struct task_struct *t, int group)
	 {
		int sig = q->info.si_signo;
		struct sigpending *pending;
	@@ -1300,6 +1300,8 @@ int send_sigqueue(struct sigqueue *q, st
		if (!prepare_signal(sig, t))
			goto out;
	 
	+	q->info.si_sys_private = info->si_sys_private;
	+
		ret = 0;
		if (unlikely(!list_empty(&q->list))) {
			/*

But can't we do a simpler change?

	--- kernel/posix-timers.c
	+++ kernel/posix-timers.c
	@@ -298,7 +298,6 @@ void do_schedule_next_timer(struct sigin
	 
	 int posix_timer_event(struct k_itimer *timr,int si_private)
	 {
	-	memset(&timr->sigq->info, 0, sizeof(siginfo_t));
		timr->sigq->info.si_sys_private = si_private;
		/* Send signal to the process that owns this timer.*/
	 
	@@ -435,6 +434,7 @@ static struct k_itimer * alloc_posix_tim
			kmem_cache_free(posix_timers_cache, tmr);
			tmr = NULL;
		}
	+	memset(&timr->sigq->info, 0, sizeof(siginfo_t));
		return tmr;
	 }

Yes, if sigq->info is queued, it can be dequeued right after
".si_sys_private = si_private" and before we send the signal. As I said,
I don't know what si_sys_private means for the user-level, is this bad?

Note that the we can't race with do_schedule_next_timer(), the timer is
locked.

Thoughts?

Oleg.