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

[Oliver Pinter]

[add stable]

On 7/21/08, Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
> On 07/20, Roland McGrath wrote:
>>
>> > Yes, thanks, I see. But does it have any meaning for the user-space?
>> [si_sys_private]
>>
>> No, it's not part of the user ABI.  It's not even copied out (see
>> copy_siginfo_to_user).
>
> Heh, I didn't know, thanks.
>
>> > Let me repeat. Can't we make a simple fix for now for this nasty and
>> > ancient bug, before we do the more clever changes,
>>
>> You do need to clear si_overrun there to be correct in the usual case
>> (not already queued).
>
> Indeed, I missed that. Can't we do this in send_sigqueue() ?
>
>> It would be a perfectly fine and worthwhile optimization/cleanup on its
>> own just to move all the initialization of sigq->info (everything but
>> si_sys_private) to alloc_posix_timer.
>
> Yes, we can do this in sys_timer_create(). But this is not very trivial to
> do without uglifying the code futher, note this "if (timer_event_spec)".
> And we can't do this after "->it_process = process", the timer is already
> visible to sys_timer_settime().
>
>> Even if it's a fine stopgap, I'm not comfortable calling this a real
>> "fix".
>> ...
>> I don't find it easy to be sure there aren't more bad
>> problems caused by trying to re-send the same sigqueue entry.
>
> Yes, yes, I agree. I propose this change as a first step only.
>
>> It seems likely this is the good choice for the stable branch.
>
> So, what do you and Mark think about the patch below?
>
>> > The thread which does dequeue_signal() continues and re-schedules the
>> > timer while ->sigq is queued. Then it copies sigq->info to the user
>> > space.
>>
>> The thread that dequeued the first timer signal had ceased all reference
>> to sigq by the time it unlocked siglock.  When its do_schedule_next_timer
>> call gets it_lock, it can do bookkeeping in struct k_itimer to figure out
>> what posix_timer_event or timer_settime has done lately.
>
> Yes, this should work.
>
> Oleg.
>
> --- a/kernel/posix-timers.c
> +++ b/kernel/posix-timers.c
> @@ -298,12 +298,10 @@ 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;
> @@ -435,6 +433,7 @@ static struct k_itimer * alloc_posix_tim
>  		kmem_cache_free(posix_timers_cache, tmr);
>  		tmr = NULL;
>  	}
> +	memset(&tmr->sigq->info, 0, sizeof(siginfo_t));
>  	return tmr;
>  }
>
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -1310,6 +1310,7 @@ int send_sigqueue(struct sigqueue *q, st
>  		q->info.si_overrun++;
>  		goto out;
>  	}
> +	q->info.si_overrun = 0;
>
>  	signalfd_notify(t, sig);
>  	pending = group ? &t->signal->shared_pending : &t->pending;
>
> --
> 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/
>


-- 
Thanks,
Oliver
--
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/