Ticket #693 (closed defect: fixed)
Invalid timeval passed to setitmer()
| Reported by: | mail@… | Owned by: | tick@… |
|---|---|---|---|
| Priority: | normal | Milestone: | |
| Component: | gsmd | Version: | current svn head |
| Severity: | minor | Keywords: | |
| Cc: | buglog@…, jserv@… | Blocked By: | |
| Blocking: | Estimated Completion (week): | ||
| HasPatchForReview: | PatchReviewResult: | ||
| Reproducible: |
Description
A system clock step (e.g. caused by NTP synchronization) can cause gsmd/timer.c
to pass a negative time interval to setitmer():
Aug 7 16:53:18 fic-gta01 daemon.info ntpd[1100]: synchronized to 192.168.0.200,
stratum 3
Aug 7 17:15:29 fic-gta01 daemon.notice ntpd[1100]: time reset +70.331006 s
Aug 7 17:15:54 fic-gta01 user.warn kernel: setitimer: gsmd (pid = 1152)
provided invalid timeval it_value: tv_sec = -69 tv_usec = 899874
Aug 7 17:15:54 fic-gta01 user.warn kernel: setitimer: gsmd (pid = 1152)
provided invalid timeval it_value: tv_sec = -69 tv_usec = 899478
There is code which anticipates this condition:
if (tv_sub(&diff, &min, &now) < 0) {
/* FIXME: run expired timer callbacks */
/* we cannot run timers from here since we might be
- called from register_timer() within check_n_run() */
/* FIXME: restart with next minimum timer */
goto retry;
}
however tv_sub() currently has an unconditional return value of 0, so this block
will never be executed.
I haven't yet looked at the gsmd timer code in detail, but it might help in some
places to use clock_gettime(CLOCK_MONOTONIC,...) rather than gettimeofday() for
interval-related calculations.
Attachments
Change History
Changed 6 years ago by tick@…
- Attachment timeout.patch added
timer with retry (setSystem time, racing condition)
comment:3 Changed 6 years ago by tick@…
- Status changed from new to closed
- Resolution set to fixed
After verify thr properities of setitimer. setitimer works well even when system
is changing.
And so that the log shows that it's a racing condition: when registering a
timer, gsmd get the expire time and then the current time changed before
calculate the diff(calc_next_expireation). And so that the diff happened at past.
If a system is too busy or changing system, this kind of errors may happen, so
that a second chance may needed. Because "diff" is calculated from the closest
timeout, it is reasonable to assume this one is closest timer.
comment:4 Changed 6 years ago by tick@…
- Status changed from closed to reopened
- Resolution fixed deleted
waiting for some comments.
comment:5 Changed 6 years ago by tick@…
When calculating the next expiration, the timer should not be expired already.
But if so, we may need to figure why and what should we do to deal with this.
In this case, we see the system is changed forward and so that the expired time
in timer is expired.
In other case, we may found that if a system is too busy and context switch
quickly, this may also happened.
In my patch (id=339), when calculating the next expiration and found that the
oldest one has been expired, I give it a second try. That is, pretending this
timer is registering at this moment, and setting timer with the interval that
user set.
The first thing we should figure out is "what should we do when timer is expired?"
In my opinion, this is an abnormal condition, and we should try to set another
alarm to call those expired timer.
In the case of changing system time, gsmd should wait for a moment and so that
reset the timer is suit for this case.
However, in the case of system too busy, it may need to send SIGALRM immediately.
In order to make gsmd robust, these two conditions should be considered well.
In that patch id=339, there is a weakness:
If timer A registered at 00:00 and interval is 15 sec, and timer B registered at
00:13 and interval is 3 sec.
If the system time changed at 00:14 from 00:14 to 00:20, both timer A and B are
expired.
In this patch, A will be selected and another 15 sec will be waited. That is B
will wait for extra 13 sec.
Therefore, I have to reject my patch (id=339)
A better way to handle this abnormal condition is to find out the latest
registering time, and recalculate all the expired timer. Pretending the current
time is the latest registering time, and recalculate all the expiration of
expired timer.
Changed 6 years ago by tick@…
- Attachment timeout.2.patch added
give expired timer a second chance with recalculate expiration
