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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
