* race in timerobj
@ 2020-12-01 10:26 Ronny Meeus
2020-12-01 11:06 ` Jan Kiszka
0 siblings, 1 reply; 14+ messages in thread
From: Ronny Meeus @ 2020-12-01 10:26 UTC (permalink / raw)
To: xenomai
Hello Xenomai community,
it looks like we have a race condition in the timer object handling.
The scope of the below mentioned issue is the alarm interface of the
alchemy skin:
int rt_alarm_start(RT_ALARM *alarm, RTIME value, RTIME interval)
The documentation mentions that this start can be called also on an
already running timer:
"This service overrides any previous setup of the expiry date and
reload interval for the given alarm."
In the timer server code (see file lib/copperplate/timerobj.c):
static void *timerobj_server (void *arg))
I see the timer being re-inserted in the timeout list in case of a
periodic timer.
write_lock_nocancel(&svlock);
...
if (interval.tv_sec > 0 || interval.tv_nsec > 0) {
timespec_add(&tmobj->itspec.it_value, &value, &interval);
timerobj_enqueue(tmobj);
}
write_unlock(&svlock);
tmobj->handler(tmobj);
write_lock_nocancel(&svlock);
}
This re-insert is done with the svlock taken but the timer specific
lock is not taken.
In the start on the other hand I see:
int timerobj_start(struct timerobj *tmobj,
void (*handler)(struct timerobj *tmobj),
struct itimerspec *it) /* lock held, dropped */
{
tmobj->handler = handler;
tmobj->itspec = *it;
...
write_lock_nocancel(&svlock);
So the itspec is updated with only the timerobj lock taken.
If the timeout value is changed via the timerobj_start while the timer is
under processing by the timer server, we can enter an endless loop (at
least that is what we see sporadically)
Does this make sense?
Best regards,
Ronny
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: race in timerobj
2020-12-01 10:26 race in timerobj Ronny Meeus
@ 2020-12-01 11:06 ` Jan Kiszka
2020-12-01 12:08 ` Ronny Meeus
0 siblings, 1 reply; 14+ messages in thread
From: Jan Kiszka @ 2020-12-01 11:06 UTC (permalink / raw)
To: Ronny Meeus, xenomai
On 01.12.20 11:26, Ronny Meeus via Xenomai wrote:
> Hello Xenomai community,
>
> it looks like we have a race condition in the timer object handling.
> The scope of the below mentioned issue is the alarm interface of the
> alchemy skin:
> int rt_alarm_start(RT_ALARM *alarm, RTIME value, RTIME interval)
>
> The documentation mentions that this start can be called also on an
> already running timer:
> "This service overrides any previous setup of the expiry date and
> reload interval for the given alarm."
>
> In the timer server code (see file lib/copperplate/timerobj.c):
> static void *timerobj_server (void *arg))
> I see the timer being re-inserted in the timeout list in case of a
> periodic timer.
>
> write_lock_nocancel(&svlock);
> ...
> if (interval.tv_sec > 0 || interval.tv_nsec > 0) {
> timespec_add(&tmobj->itspec.it_value, &value, &interval);
> timerobj_enqueue(tmobj);
> }
> write_unlock(&svlock);
> tmobj->handler(tmobj);
> write_lock_nocancel(&svlock);
> }
>
> This re-insert is done with the svlock taken but the timer specific
> lock is not taken.
>
> In the start on the other hand I see:
>
> int timerobj_start(struct timerobj *tmobj,
> void (*handler)(struct timerobj *tmobj),
> struct itimerspec *it) /* lock held, dropped */
> {
> tmobj->handler = handler;
> tmobj->itspec = *it;
> ...
> write_lock_nocancel(&svlock);
>
> So the itspec is updated with only the timerobj lock taken.
> If the timeout value is changed via the timerobj_start while the timer is
> under processing by the timer server, we can enter an endless loop (at
> least that is what we see sporadically)
>
> Does this make sense?
Yes, at least to me. Patch welcome.
Note, though, that updating the handler will remain inherently racy.
Jan
--
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: race in timerobj
2020-12-01 11:06 ` Jan Kiszka
@ 2020-12-01 12:08 ` Ronny Meeus
2020-12-01 13:51 ` Philippe Gerum
0 siblings, 1 reply; 14+ messages in thread
From: Ronny Meeus @ 2020-12-01 12:08 UTC (permalink / raw)
To: Jan Kiszka; +Cc: xenomai
Op di 1 dec. 2020 om 12:06 schreef Jan Kiszka <jan.kiszka@siemens.com>:
>
> On 01.12.20 11:26, Ronny Meeus via Xenomai wrote:
> > Hello Xenomai community,
> >
> > it looks like we have a race condition in the timer object handling.
> > The scope of the below mentioned issue is the alarm interface of the
> > alchemy skin:
> > int rt_alarm_start(RT_ALARM *alarm, RTIME value, RTIME interval)
> >
> > The documentation mentions that this start can be called also on an
> > already running timer:
> > "This service overrides any previous setup of the expiry date and
> > reload interval for the given alarm."
> >
> > In the timer server code (see file lib/copperplate/timerobj.c):
> > static void *timerobj_server (void *arg))
> > I see the timer being re-inserted in the timeout list in case of a
> > periodic timer.
> >
> > write_lock_nocancel(&svlock);
> > ...
> > if (interval.tv_sec > 0 || interval.tv_nsec > 0) {
> > timespec_add(&tmobj->itspec.it_value, &value, &interval);
> > timerobj_enqueue(tmobj);
> > }
> > write_unlock(&svlock);
> > tmobj->handler(tmobj);
> > write_lock_nocancel(&svlock);
> > }
> >
> > This re-insert is done with the svlock taken but the timer specific
> > lock is not taken.
> >
> > In the start on the other hand I see:
> >
> > int timerobj_start(struct timerobj *tmobj,
> > void (*handler)(struct timerobj *tmobj),
> > struct itimerspec *it) /* lock held, dropped */
> > {
> > tmobj->handler = handler;
> > tmobj->itspec = *it;
> > ...
> > write_lock_nocancel(&svlock);
> >
> > So the itspec is updated with only the timerobj lock taken.
> > If the timeout value is changed via the timerobj_start while the timer is
> > under processing by the timer server, we can enter an endless loop (at
> > least that is what we see sporadically)
> >
> > Does this make sense?
>
> Yes, at least to me. Patch welcome.
Jan,
this is the patch we are currently testing with (note that the line numbers
might not match since we are not at the latest revision).
diff --git a/lib/copperplate/timerobj.c b/lib/copperplate/timerobj.c
--- a/lib/copperplate/timerobj.c
+++ b/lib/copperplate/timerobj.c
@@ -165,7 +165,8 @@ static void *timerobj_server(void *arg)
timerobj_enqueue(tmobj);
}
write_unlock(&svlock);
- tmobj->handler(tmobj);
+ if (tmobj->handler)
+ tmobj->handler(tmobj);
write_lock_nocancel(&svlock);
}
@@ -232,10 +233,14 @@ int timerobj_start(struct timerobj *tmob
void (*handler)(struct timerobj *tmobj),
struct itimerspec *it) /* lock held, dropped */
{
+ write_lock_nocancel(&svlock);
+
+ if (pvholder_linked(&tmobj->next))
+ pvlist_remove_init(&tmobj->next);
+
tmobj->handler = handler;
tmobj->itspec = *it;
- write_lock_nocancel(&svlock);
timerobj_enqueue(tmobj);
write_unlock(&svlock);
timerobj_unlock(tmobj);
>
> Note, though, that updating the handler will remain inherently racy.
>
> Jan
>
> --
> Siemens AG, T RDA IOT
> Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: race in timerobj
2020-12-01 12:08 ` Ronny Meeus
@ 2020-12-01 13:51 ` Philippe Gerum
2020-12-04 10:41 ` Ronny Meeus
0 siblings, 1 reply; 14+ messages in thread
From: Philippe Gerum @ 2020-12-01 13:51 UTC (permalink / raw)
To: Ronny Meeus; +Cc: Jan Kiszka, xenomai
Ronny Meeus via Xenomai <xenomai@xenomai.org> writes:
> Op di 1 dec. 2020 om 12:06 schreef Jan Kiszka <jan.kiszka@siemens.com>:
>>
>> On 01.12.20 11:26, Ronny Meeus via Xenomai wrote:
>> > Hello Xenomai community,
>> >
>> > it looks like we have a race condition in the timer object handling.
>> > The scope of the below mentioned issue is the alarm interface of the
>> > alchemy skin:
>> > int rt_alarm_start(RT_ALARM *alarm, RTIME value, RTIME interval)
>> >
>> > The documentation mentions that this start can be called also on an
>> > already running timer:
>> > "This service overrides any previous setup of the expiry date and
>> > reload interval for the given alarm."
>> >
>> > In the timer server code (see file lib/copperplate/timerobj.c):
>> > static void *timerobj_server (void *arg))
>> > I see the timer being re-inserted in the timeout list in case of a
>> > periodic timer.
>> >
>> > write_lock_nocancel(&svlock);
>> > ...
>> > if (interval.tv_sec > 0 || interval.tv_nsec > 0) {
>> > timespec_add(&tmobj->itspec.it_value, &value, &interval);
>> > timerobj_enqueue(tmobj);
>> > }
>> > write_unlock(&svlock);
>> > tmobj->handler(tmobj);
>> > write_lock_nocancel(&svlock);
>> > }
>> >
>> > This re-insert is done with the svlock taken but the timer specific
>> > lock is not taken.
>> >
>> > In the start on the other hand I see:
>> >
>> > int timerobj_start(struct timerobj *tmobj,
>> > void (*handler)(struct timerobj *tmobj),
>> > struct itimerspec *it) /* lock held, dropped */
>> > {
>> > tmobj->handler = handler;
>> > tmobj->itspec = *it;
>> > ...
>> > write_lock_nocancel(&svlock);
>> >
>> > So the itspec is updated with only the timerobj lock taken.
>> > If the timeout value is changed via the timerobj_start while the timer is
>> > under processing by the timer server, we can enter an endless loop (at
>> > least that is what we see sporadically)
>> >
>> > Does this make sense?
>>
>> Yes, at least to me. Patch welcome.
>
> Jan,
>
> this is the patch we are currently testing with (note that the line numbers
> might not match since we are not at the latest revision).
>
> diff --git a/lib/copperplate/timerobj.c b/lib/copperplate/timerobj.c
> --- a/lib/copperplate/timerobj.c
> +++ b/lib/copperplate/timerobj.c
> @@ -165,7 +165,8 @@ static void *timerobj_server(void *arg)
> timerobj_enqueue(tmobj);
> }
> write_unlock(&svlock);
> - tmobj->handler(tmobj);
> + if (tmobj->handler)
> + tmobj->handler(tmobj);
> write_lock_nocancel(&svlock);
> }
>
> @@ -232,10 +233,14 @@ int timerobj_start(struct timerobj *tmob
> void (*handler)(struct timerobj *tmobj),
> struct itimerspec *it) /* lock held, dropped */
> {
> + write_lock_nocancel(&svlock);
> +
> + if (pvholder_linked(&tmobj->next))
> + pvlist_remove_init(&tmobj->next);
> +
> tmobj->handler = handler;
> tmobj->itspec = *it;
>
> - write_lock_nocancel(&svlock);
> timerobj_enqueue(tmobj);
> write_unlock(&svlock);
> timerobj_unlock(tmobj);
>
>
>>
>> Note, though, that updating the handler will remain inherently racy.
>>
Good catch. Also, we would need a consistent view of the
(value,interval,handler) triplet for any time wrt concurrent start/stop
updates, all accessed under server lock. We would not want a new value
being copied to be used with an old interval and/or handler for
instance.
There is also a locking imbalance to be fixed on error in
timerobj_start().
e.g. (proudly untested).
diff --git a/lib/copperplate/timerobj.c b/lib/copperplate/timerobj.c
index cbfcda566..08cc0d3b9 100644
--- a/lib/copperplate/timerobj.c
+++ b/lib/copperplate/timerobj.c
@@ -98,6 +98,7 @@ static int server_prologue(void *arg)
static void *timerobj_server(void *arg)
{
+ void (*handler)(struct timerobj *tmobj);
struct timespec now, value, interval;
struct timerobj *tmobj, *tmp;
sigset_t set;
@@ -120,17 +121,18 @@ static void *timerobj_server(void *arg)
pvlist_for_each_entry_safe(tmobj, tmp, &svtimers, next) {
value = tmobj->itspec.it_value;
+ interval = tmobj->itspec.it_interval;
+ handler = tmobj->handler;
if (timespec_after(&value, &now))
break;
pvlist_remove_init(&tmobj->next);
- interval = tmobj->itspec.it_interval;
if (interval.tv_sec > 0 || interval.tv_nsec > 0) {
timespec_add(&tmobj->itspec.it_value,
&value, &interval);
timerobj_enqueue(tmobj);
}
write_unlock(&svlock);
- tmobj->handler(tmobj);
+ handler(tmobj);
write_lock_nocancel(&svlock);
}
@@ -218,8 +220,7 @@ int timerobj_start(struct timerobj *tmobj,
void (*handler)(struct timerobj *tmobj),
struct itimerspec *it) /* lock held, dropped */
{
- tmobj->handler = handler;
- tmobj->itspec = *it;
+ int ret = 0;
/*
* We hold the queue lock long enough to prevent the timer
@@ -232,14 +233,23 @@ int timerobj_start(struct timerobj *tmobj,
*/
write_lock_nocancel(&svlock);
- if (__RT(timer_settime(tmobj->timer, TIMER_ABSTIME, it, NULL)))
- return __bt(-errno);
+ if (pvholder_linked(&tmobj->next))
+ pvlist_remove_init(&tmobj->next);
+
+ tmobj->handler = handler;
+ tmobj->itspec = *it;
+
+ if (__RT(timer_settime(tmobj->timer, TIMER_ABSTIME, it, NULL))) {
+ ret = __bt(-errno);
+ goto fail;
+ }
timerobj_enqueue(tmobj);
+fail:
write_unlock(&svlock);
timerobj_unlock(tmobj);
- return 0;
+ return ret;
}
int timerobj_stop(struct timerobj *tmobj) /* lock held, dropped */
@@ -251,10 +261,9 @@ int timerobj_stop(struct timerobj *tmobj) /* lock held, dropped */
if (pvholder_linked(&tmobj->next))
pvlist_remove_init(&tmobj->next);
- write_unlock(&svlock);
-
__RT(timer_settime(tmobj->timer, 0, &itimer_stop, NULL));
tmobj->handler = NULL;
+ write_unlock(&svlock);
timerobj_unlock(tmobj);
return 0;
--
Philippe.
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: race in timerobj
2020-12-01 13:51 ` Philippe Gerum
@ 2020-12-04 10:41 ` Ronny Meeus
2020-12-04 15:29 ` Philippe Gerum
0 siblings, 1 reply; 14+ messages in thread
From: Ronny Meeus @ 2020-12-04 10:41 UTC (permalink / raw)
To: Philippe Gerum; +Cc: Jan Kiszka, xenomai
Op di 1 dec. 2020 om 14:51 schreef Philippe Gerum <rpm@xenomai.org>:
>
>
> Ronny Meeus via Xenomai <xenomai@xenomai.org> writes:
>
> > Op di 1 dec. 2020 om 12:06 schreef Jan Kiszka <jan.kiszka@siemens.com>:
> >>
> >> On 01.12.20 11:26, Ronny Meeus via Xenomai wrote:
> >> > Hello Xenomai community,
> >> >
> >> > it looks like we have a race condition in the timer object handling.
> >> > The scope of the below mentioned issue is the alarm interface of the
> >> > alchemy skin:
> >> > int rt_alarm_start(RT_ALARM *alarm, RTIME value, RTIME interval)
> >> >
> >> > The documentation mentions that this start can be called also on an
> >> > already running timer:
> >> > "This service overrides any previous setup of the expiry date and
> >> > reload interval for the given alarm."
> >> >
> >> > In the timer server code (see file lib/copperplate/timerobj.c):
> >> > static void *timerobj_server (void *arg))
> >> > I see the timer being re-inserted in the timeout list in case of a
> >> > periodic timer.
> >> >
> >> > write_lock_nocancel(&svlock);
> >> > ...
> >> > if (interval.tv_sec > 0 || interval.tv_nsec > 0) {
> >> > timespec_add(&tmobj->itspec.it_value, &value, &interval);
> >> > timerobj_enqueue(tmobj);
> >> > }
> >> > write_unlock(&svlock);
> >> > tmobj->handler(tmobj);
> >> > write_lock_nocancel(&svlock);
> >> > }
> >> >
> >> > This re-insert is done with the svlock taken but the timer specific
> >> > lock is not taken.
> >> >
> >> > In the start on the other hand I see:
> >> >
> >> > int timerobj_start(struct timerobj *tmobj,
> >> > void (*handler)(struct timerobj *tmobj),
> >> > struct itimerspec *it) /* lock held, dropped */
> >> > {
> >> > tmobj->handler = handler;
> >> > tmobj->itspec = *it;
> >> > ...
> >> > write_lock_nocancel(&svlock);
> >> >
> >> > So the itspec is updated with only the timerobj lock taken.
> >> > If the timeout value is changed via the timerobj_start while the timer is
> >> > under processing by the timer server, we can enter an endless loop (at
> >> > least that is what we see sporadically)
> >> >
> >> > Does this make sense?
> >>
> >> Yes, at least to me. Patch welcome.
> >
> > Jan,
> >
> > this is the patch we are currently testing with (note that the line numbers
> > might not match since we are not at the latest revision).
> >
> > diff --git a/lib/copperplate/timerobj.c b/lib/copperplate/timerobj.c
> > --- a/lib/copperplate/timerobj.c
> > +++ b/lib/copperplate/timerobj.c
> > @@ -165,7 +165,8 @@ static void *timerobj_server(void *arg)
> > timerobj_enqueue(tmobj);
> > }
> > write_unlock(&svlock);
> > - tmobj->handler(tmobj);
> > + if (tmobj->handler)
> > + tmobj->handler(tmobj);
> > write_lock_nocancel(&svlock);
> > }
> >
> > @@ -232,10 +233,14 @@ int timerobj_start(struct timerobj *tmob
> > void (*handler)(struct timerobj *tmobj),
> > struct itimerspec *it) /* lock held, dropped */
> > {
> > + write_lock_nocancel(&svlock);
> > +
> > + if (pvholder_linked(&tmobj->next))
> > + pvlist_remove_init(&tmobj->next);
> > +
> > tmobj->handler = handler;
> > tmobj->itspec = *it;
> >
> > - write_lock_nocancel(&svlock);
> > timerobj_enqueue(tmobj);
> > write_unlock(&svlock);
> > timerobj_unlock(tmobj);
> >
> >
> >>
> >> Note, though, that updating the handler will remain inherently racy.
> >>
>
> Good catch. Also, we would need a consistent view of the
> (value,interval,handler) triplet for any time wrt concurrent start/stop
> updates, all accessed under server lock. We would not want a new value
> being copied to be used with an old interval and/or handler for
> instance.
>
> There is also a locking imbalance to be fixed on error in
> timerobj_start().
>
> e.g. (proudly untested).
>
> diff --git a/lib/copperplate/timerobj.c b/lib/copperplate/timerobj.c
> index cbfcda566..08cc0d3b9 100644
> --- a/lib/copperplate/timerobj.c
> +++ b/lib/copperplate/timerobj.c
> @@ -98,6 +98,7 @@ static int server_prologue(void *arg)
>
> static void *timerobj_server(void *arg)
> {
> + void (*handler)(struct timerobj *tmobj);
> struct timespec now, value, interval;
> struct timerobj *tmobj, *tmp;
> sigset_t set;
> @@ -120,17 +121,18 @@ static void *timerobj_server(void *arg)
>
> pvlist_for_each_entry_safe(tmobj, tmp, &svtimers, next) {
> value = tmobj->itspec.it_value;
> + interval = tmobj->itspec.it_interval;
> + handler = tmobj->handler;
> if (timespec_after(&value, &now))
> break;
> pvlist_remove_init(&tmobj->next);
> - interval = tmobj->itspec.it_interval;
> if (interval.tv_sec > 0 || interval.tv_nsec > 0) {
> timespec_add(&tmobj->itspec.it_value,
> &value, &interval);
> timerobj_enqueue(tmobj);
> }
> write_unlock(&svlock);
> - tmobj->handler(tmobj);
> + handler(tmobj);
> write_lock_nocancel(&svlock);
> }
>
> @@ -218,8 +220,7 @@ int timerobj_start(struct timerobj *tmobj,
> void (*handler)(struct timerobj *tmobj),
> struct itimerspec *it) /* lock held, dropped */
> {
> - tmobj->handler = handler;
> - tmobj->itspec = *it;
> + int ret = 0;
>
> /*
> * We hold the queue lock long enough to prevent the timer
> @@ -232,14 +233,23 @@ int timerobj_start(struct timerobj *tmobj,
> */
> write_lock_nocancel(&svlock);
>
> - if (__RT(timer_settime(tmobj->timer, TIMER_ABSTIME, it, NULL)))
> - return __bt(-errno);
> + if (pvholder_linked(&tmobj->next))
> + pvlist_remove_init(&tmobj->next);
> +
> + tmobj->handler = handler;
> + tmobj->itspec = *it;
> +
> + if (__RT(timer_settime(tmobj->timer, TIMER_ABSTIME, it, NULL))) {
> + ret = __bt(-errno);
> + goto fail;
> + }
>
> timerobj_enqueue(tmobj);
> +fail:
> write_unlock(&svlock);
> timerobj_unlock(tmobj);
>
> - return 0;
> + return ret;
> }
>
> int timerobj_stop(struct timerobj *tmobj) /* lock held, dropped */
> @@ -251,10 +261,9 @@ int timerobj_stop(struct timerobj *tmobj) /* lock held, dropped */
> if (pvholder_linked(&tmobj->next))
> pvlist_remove_init(&tmobj->next);
>
> - write_unlock(&svlock);
> -
> __RT(timer_settime(tmobj->timer, 0, &itimer_stop, NULL));
> tmobj->handler = NULL;
> + write_unlock(&svlock);
> timerobj_unlock(tmobj);
>
> return 0;
> --
Philippe,
we tested with the patch you proposed and the problem seem to be resolved.
I think this is a good improvement.
Best regards,
Ronny
> Philippe.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: race in timerobj
2020-12-04 10:41 ` Ronny Meeus
@ 2020-12-04 15:29 ` Philippe Gerum
2021-01-22 7:57 ` Ronny Meeus
0 siblings, 1 reply; 14+ messages in thread
From: Philippe Gerum @ 2020-12-04 15:29 UTC (permalink / raw)
To: Ronny Meeus; +Cc: Jan Kiszka, xenomai
Ronny Meeus <ronny.meeus@gmail.com> writes:
> Op di 1 dec. 2020 om 14:51 schreef Philippe Gerum <rpm@xenomai.org>:
>>
>>
>> Ronny Meeus via Xenomai <xenomai@xenomai.org> writes:
>>
>> > Op di 1 dec. 2020 om 12:06 schreef Jan Kiszka <jan.kiszka@siemens.com>:
>> >>
>> >> On 01.12.20 11:26, Ronny Meeus via Xenomai wrote:
>> >> > Hello Xenomai community,
>> >> >
>> >> > it looks like we have a race condition in the timer object handling.
>> >> > The scope of the below mentioned issue is the alarm interface of the
>> >> > alchemy skin:
>> >> > int rt_alarm_start(RT_ALARM *alarm, RTIME value, RTIME interval)
>> >> >
>> >> > The documentation mentions that this start can be called also on an
>> >> > already running timer:
>> >> > "This service overrides any previous setup of the expiry date and
>> >> > reload interval for the given alarm."
>> >> >
>> >> > In the timer server code (see file lib/copperplate/timerobj.c):
>> >> > static void *timerobj_server (void *arg))
>> >> > I see the timer being re-inserted in the timeout list in case of a
>> >> > periodic timer.
>> >> >
>> >> > write_lock_nocancel(&svlock);
>> >> > ...
>> >> > if (interval.tv_sec > 0 || interval.tv_nsec > 0) {
>> >> > timespec_add(&tmobj->itspec.it_value, &value, &interval);
>> >> > timerobj_enqueue(tmobj);
>> >> > }
>> >> > write_unlock(&svlock);
>> >> > tmobj->handler(tmobj);
>> >> > write_lock_nocancel(&svlock);
>> >> > }
>> >> >
>> >> > This re-insert is done with the svlock taken but the timer specific
>> >> > lock is not taken.
>> >> >
>> >> > In the start on the other hand I see:
>> >> >
>> >> > int timerobj_start(struct timerobj *tmobj,
>> >> > void (*handler)(struct timerobj *tmobj),
>> >> > struct itimerspec *it) /* lock held, dropped */
>> >> > {
>> >> > tmobj->handler = handler;
>> >> > tmobj->itspec = *it;
>> >> > ...
>> >> > write_lock_nocancel(&svlock);
>> >> >
>> >> > So the itspec is updated with only the timerobj lock taken.
>> >> > If the timeout value is changed via the timerobj_start while the timer is
>> >> > under processing by the timer server, we can enter an endless loop (at
>> >> > least that is what we see sporadically)
>> >> >
>> >> > Does this make sense?
>> >>
>> >> Yes, at least to me. Patch welcome.
>> >
>> > Jan,
>> >
>> > this is the patch we are currently testing with (note that the line numbers
>> > might not match since we are not at the latest revision).
>> >
>> > diff --git a/lib/copperplate/timerobj.c b/lib/copperplate/timerobj.c
>> > --- a/lib/copperplate/timerobj.c
>> > +++ b/lib/copperplate/timerobj.c
>> > @@ -165,7 +165,8 @@ static void *timerobj_server(void *arg)
>> > timerobj_enqueue(tmobj);
>> > }
>> > write_unlock(&svlock);
>> > - tmobj->handler(tmobj);
>> > + if (tmobj->handler)
>> > + tmobj->handler(tmobj);
>> > write_lock_nocancel(&svlock);
>> > }
>> >
>> > @@ -232,10 +233,14 @@ int timerobj_start(struct timerobj *tmob
>> > void (*handler)(struct timerobj *tmobj),
>> > struct itimerspec *it) /* lock held, dropped */
>> > {
>> > + write_lock_nocancel(&svlock);
>> > +
>> > + if (pvholder_linked(&tmobj->next))
>> > + pvlist_remove_init(&tmobj->next);
>> > +
>> > tmobj->handler = handler;
>> > tmobj->itspec = *it;
>> >
>> > - write_lock_nocancel(&svlock);
>> > timerobj_enqueue(tmobj);
>> > write_unlock(&svlock);
>> > timerobj_unlock(tmobj);
>> >
>> >
>> >>
>> >> Note, though, that updating the handler will remain inherently racy.
>> >>
>>
>> Good catch. Also, we would need a consistent view of the
>> (value,interval,handler) triplet for any time wrt concurrent start/stop
>> updates, all accessed under server lock. We would not want a new value
>> being copied to be used with an old interval and/or handler for
>> instance.
>>
>> There is also a locking imbalance to be fixed on error in
>> timerobj_start().
>>
>> e.g. (proudly untested).
>>
>> diff --git a/lib/copperplate/timerobj.c b/lib/copperplate/timerobj.c
>> index cbfcda566..08cc0d3b9 100644
>> --- a/lib/copperplate/timerobj.c
>> +++ b/lib/copperplate/timerobj.c
>> @@ -98,6 +98,7 @@ static int server_prologue(void *arg)
>>
>> static void *timerobj_server(void *arg)
>> {
>> + void (*handler)(struct timerobj *tmobj);
>> struct timespec now, value, interval;
>> struct timerobj *tmobj, *tmp;
>> sigset_t set;
>> @@ -120,17 +121,18 @@ static void *timerobj_server(void *arg)
>>
>> pvlist_for_each_entry_safe(tmobj, tmp, &svtimers, next) {
>> value = tmobj->itspec.it_value;
>> + interval = tmobj->itspec.it_interval;
>> + handler = tmobj->handler;
>> if (timespec_after(&value, &now))
>> break;
>> pvlist_remove_init(&tmobj->next);
>> - interval = tmobj->itspec.it_interval;
>> if (interval.tv_sec > 0 || interval.tv_nsec > 0) {
>> timespec_add(&tmobj->itspec.it_value,
>> &value, &interval);
>> timerobj_enqueue(tmobj);
>> }
>> write_unlock(&svlock);
>> - tmobj->handler(tmobj);
>> + handler(tmobj);
>> write_lock_nocancel(&svlock);
>> }
>>
>> @@ -218,8 +220,7 @@ int timerobj_start(struct timerobj *tmobj,
>> void (*handler)(struct timerobj *tmobj),
>> struct itimerspec *it) /* lock held, dropped */
>> {
>> - tmobj->handler = handler;
>> - tmobj->itspec = *it;
>> + int ret = 0;
>>
>> /*
>> * We hold the queue lock long enough to prevent the timer
>> @@ -232,14 +233,23 @@ int timerobj_start(struct timerobj *tmobj,
>> */
>> write_lock_nocancel(&svlock);
>>
>> - if (__RT(timer_settime(tmobj->timer, TIMER_ABSTIME, it, NULL)))
>> - return __bt(-errno);
>> + if (pvholder_linked(&tmobj->next))
>> + pvlist_remove_init(&tmobj->next);
>> +
>> + tmobj->handler = handler;
>> + tmobj->itspec = *it;
>> +
>> + if (__RT(timer_settime(tmobj->timer, TIMER_ABSTIME, it, NULL))) {
>> + ret = __bt(-errno);
>> + goto fail;
>> + }
>>
>> timerobj_enqueue(tmobj);
>> +fail:
>> write_unlock(&svlock);
>> timerobj_unlock(tmobj);
>>
>> - return 0;
>> + return ret;
>> }
>>
>> int timerobj_stop(struct timerobj *tmobj) /* lock held, dropped */
>> @@ -251,10 +261,9 @@ int timerobj_stop(struct timerobj *tmobj) /* lock held, dropped */
>> if (pvholder_linked(&tmobj->next))
>> pvlist_remove_init(&tmobj->next);
>>
>> - write_unlock(&svlock);
>> -
>> __RT(timer_settime(tmobj->timer, 0, &itimer_stop, NULL));
>> tmobj->handler = NULL;
>> + write_unlock(&svlock);
>> timerobj_unlock(tmobj);
>>
>> return 0;
>> --
>
> Philippe,
>
> we tested with the patch you proposed and the problem seem to be resolved.
> I think this is a good improvement.
>
> Best regards,
> Ronny
>
>> Philippe.
Ok, thanks for testing. Now pushing a patch referring to your original
post with the preliminary fix.
--
Philippe.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: race in timerobj
2020-12-04 15:29 ` Philippe Gerum
@ 2021-01-22 7:57 ` Ronny Meeus
2021-01-23 7:40 ` Ronny Meeus
0 siblings, 1 reply; 14+ messages in thread
From: Ronny Meeus @ 2021-01-22 7:57 UTC (permalink / raw)
To: Philippe Gerum; +Cc: Jan Kiszka, xenomai
Op vr 4 dec. 2020 om 16:29 schreef Philippe Gerum <rpm@xenomai.org>:
>
>
> Ronny Meeus <ronny.meeus@gmail.com> writes:
>
> > Op di 1 dec. 2020 om 14:51 schreef Philippe Gerum <rpm@xenomai.org>:
> >>
> >>
> >> Ronny Meeus via Xenomai <xenomai@xenomai.org> writes:
> >>
> >> > Op di 1 dec. 2020 om 12:06 schreef Jan Kiszka <jan.kiszka@siemens.com>:
> >> >>
> >> >> On 01.12.20 11:26, Ronny Meeus via Xenomai wrote:
> >> >> > Hello Xenomai community,
> >> >> >
> >> >> > it looks like we have a race condition in the timer object handling.
> >> >> > The scope of the below mentioned issue is the alarm interface of the
> >> >> > alchemy skin:
> >> >> > int rt_alarm_start(RT_ALARM *alarm, RTIME value, RTIME interval)
> >> >> >
> >> >> > The documentation mentions that this start can be called also on an
> >> >> > already running timer:
> >> >> > "This service overrides any previous setup of the expiry date and
> >> >> > reload interval for the given alarm."
> >> >> >
> >> >> > In the timer server code (see file lib/copperplate/timerobj.c):
> >> >> > static void *timerobj_server (void *arg))
> >> >> > I see the timer being re-inserted in the timeout list in case of a
> >> >> > periodic timer.
> >> >> >
> >> >> > write_lock_nocancel(&svlock);
> >> >> > ...
> >> >> > if (interval.tv_sec > 0 || interval.tv_nsec > 0) {
> >> >> > timespec_add(&tmobj->itspec.it_value, &value, &interval);
> >> >> > timerobj_enqueue(tmobj);
> >> >> > }
> >> >> > write_unlock(&svlock);
> >> >> > tmobj->handler(tmobj);
> >> >> > write_lock_nocancel(&svlock);
> >> >> > }
> >> >> >
> >> >> > This re-insert is done with the svlock taken but the timer specific
> >> >> > lock is not taken.
> >> >> >
> >> >> > In the start on the other hand I see:
> >> >> >
> >> >> > int timerobj_start(struct timerobj *tmobj,
> >> >> > void (*handler)(struct timerobj *tmobj),
> >> >> > struct itimerspec *it) /* lock held, dropped */
> >> >> > {
> >> >> > tmobj->handler = handler;
> >> >> > tmobj->itspec = *it;
> >> >> > ...
> >> >> > write_lock_nocancel(&svlock);
> >> >> >
> >> >> > So the itspec is updated with only the timerobj lock taken.
> >> >> > If the timeout value is changed via the timerobj_start while the timer is
> >> >> > under processing by the timer server, we can enter an endless loop (at
> >> >> > least that is what we see sporadically)
> >> >> >
> >> >> > Does this make sense?
> >> >>
> >> >> Yes, at least to me. Patch welcome.
> >> >
> >> > Jan,
> >> >
> >> > this is the patch we are currently testing with (note that the line numbers
> >> > might not match since we are not at the latest revision).
> >> >
> >> > diff --git a/lib/copperplate/timerobj.c b/lib/copperplate/timerobj.c
> >> > --- a/lib/copperplate/timerobj.c
> >> > +++ b/lib/copperplate/timerobj.c
> >> > @@ -165,7 +165,8 @@ static void *timerobj_server(void *arg)
> >> > timerobj_enqueue(tmobj);
> >> > }
> >> > write_unlock(&svlock);
> >> > - tmobj->handler(tmobj);
> >> > + if (tmobj->handler)
> >> > + tmobj->handler(tmobj);
> >> > write_lock_nocancel(&svlock);
> >> > }
> >> >
> >> > @@ -232,10 +233,14 @@ int timerobj_start(struct timerobj *tmob
> >> > void (*handler)(struct timerobj *tmobj),
> >> > struct itimerspec *it) /* lock held, dropped */
> >> > {
> >> > + write_lock_nocancel(&svlock);
> >> > +
> >> > + if (pvholder_linked(&tmobj->next))
> >> > + pvlist_remove_init(&tmobj->next);
> >> > +
> >> > tmobj->handler = handler;
> >> > tmobj->itspec = *it;
> >> >
> >> > - write_lock_nocancel(&svlock);
> >> > timerobj_enqueue(tmobj);
> >> > write_unlock(&svlock);
> >> > timerobj_unlock(tmobj);
> >> >
> >> >
> >> >>
> >> >> Note, though, that updating the handler will remain inherently racy.
> >> >>
> >>
> >> Good catch. Also, we would need a consistent view of the
> >> (value,interval,handler) triplet for any time wrt concurrent start/stop
> >> updates, all accessed under server lock. We would not want a new value
> >> being copied to be used with an old interval and/or handler for
> >> instance.
> >>
> >> There is also a locking imbalance to be fixed on error in
> >> timerobj_start().
> >>
> >> e.g. (proudly untested).
> >>
> >> diff --git a/lib/copperplate/timerobj.c b/lib/copperplate/timerobj.c
> >> index cbfcda566..08cc0d3b9 100644
> >> --- a/lib/copperplate/timerobj.c
> >> +++ b/lib/copperplate/timerobj.c
> >> @@ -98,6 +98,7 @@ static int server_prologue(void *arg)
> >>
> >> static void *timerobj_server(void *arg)
> >> {
> >> + void (*handler)(struct timerobj *tmobj);
> >> struct timespec now, value, interval;
> >> struct timerobj *tmobj, *tmp;
> >> sigset_t set;
> >> @@ -120,17 +121,18 @@ static void *timerobj_server(void *arg)
> >>
> >> pvlist_for_each_entry_safe(tmobj, tmp, &svtimers, next) {
> >> value = tmobj->itspec.it_value;
> >> + interval = tmobj->itspec.it_interval;
> >> + handler = tmobj->handler;
> >> if (timespec_after(&value, &now))
> >> break;
> >> pvlist_remove_init(&tmobj->next);
> >> - interval = tmobj->itspec.it_interval;
> >> if (interval.tv_sec > 0 || interval.tv_nsec > 0) {
> >> timespec_add(&tmobj->itspec.it_value,
> >> &value, &interval);
> >> timerobj_enqueue(tmobj);
> >> }
> >> write_unlock(&svlock);
> >> - tmobj->handler(tmobj);
> >> + handler(tmobj);
> >> write_lock_nocancel(&svlock);
> >> }
> >>
> >> @@ -218,8 +220,7 @@ int timerobj_start(struct timerobj *tmobj,
> >> void (*handler)(struct timerobj *tmobj),
> >> struct itimerspec *it) /* lock held, dropped */
> >> {
> >> - tmobj->handler = handler;
> >> - tmobj->itspec = *it;
> >> + int ret = 0;
> >>
> >> /*
> >> * We hold the queue lock long enough to prevent the timer
> >> @@ -232,14 +233,23 @@ int timerobj_start(struct timerobj *tmobj,
> >> */
> >> write_lock_nocancel(&svlock);
> >>
> >> - if (__RT(timer_settime(tmobj->timer, TIMER_ABSTIME, it, NULL)))
> >> - return __bt(-errno);
> >> + if (pvholder_linked(&tmobj->next))
> >> + pvlist_remove_init(&tmobj->next);
> >> +
> >> + tmobj->handler = handler;
> >> + tmobj->itspec = *it;
> >> +
> >> + if (__RT(timer_settime(tmobj->timer, TIMER_ABSTIME, it, NULL))) {
> >> + ret = __bt(-errno);
> >> + goto fail;
> >> + }
> >>
> >> timerobj_enqueue(tmobj);
> >> +fail:
> >> write_unlock(&svlock);
> >> timerobj_unlock(tmobj);
> >>
> >> - return 0;
> >> + return ret;
> >> }
> >>
> >> int timerobj_stop(struct timerobj *tmobj) /* lock held, dropped */
> >> @@ -251,10 +261,9 @@ int timerobj_stop(struct timerobj *tmobj) /* lock held, dropped */
> >> if (pvholder_linked(&tmobj->next))
> >> pvlist_remove_init(&tmobj->next);
> >>
> >> - write_unlock(&svlock);
> >> -
> >> __RT(timer_settime(tmobj->timer, 0, &itimer_stop, NULL));
> >> tmobj->handler = NULL;
> >> + write_unlock(&svlock);
> >> timerobj_unlock(tmobj);
> >>
> >> return 0;
> >> --
> >
> > Philippe,
> >
> > we tested with the patch you proposed and the problem seem to be resolved.
> > I think this is a good improvement.
> >
> > Best regards,
> > Ronny
> >
> >> Philippe.
>
> Ok, thanks for testing. Now pushing a patch referring to your original
> post with the preliminary fix.
Hello
In the same context we have hit another issue.
It looks like there is a problem when a timer is deleted/stopped from within
the handler callback, the mechanism to traverse the list does not behave
correctly anymore.
pvlist_for_each_entry_safe(tmobj, tmp, &svtimers, next) {
value = tmobj->itspec.it_value;
In the above code, "tmp" is used to reference the next entry to be processed
in the next iteration of the loop. When in the callback, this next timer is
changed or deleted, the reference is not valid anymore.
I do not see immedialy a generic a solution for the pvlist.
For the timer-server specifically, we could always start from the first element
since we will remove from the head and possibly reinsert again in the list.
Please comment.
Best regards,
Ronny
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: race in timerobj
2021-01-22 7:57 ` Ronny Meeus
@ 2021-01-23 7:40 ` Ronny Meeus
2021-01-25 6:49 ` Jan Kiszka
0 siblings, 1 reply; 14+ messages in thread
From: Ronny Meeus @ 2021-01-23 7:40 UTC (permalink / raw)
To: Philippe Gerum; +Cc: Jan Kiszka, xenomai
Op vr 22 jan. 2021 om 08:57 schreef Ronny Meeus <ronny.meeus@gmail.com>:
> Op vr 4 dec. 2020 om 16:29 schreef Philippe Gerum <rpm@xenomai.org>:
> >
> >
> > Ronny Meeus <ronny.meeus@gmail.com> writes:
> >
> > > Op di 1 dec. 2020 om 14:51 schreef Philippe Gerum <rpm@xenomai.org>:
> > >>
> > >>
> > >> Ronny Meeus via Xenomai <xenomai@xenomai.org> writes:
> > >>
> > >> > Op di 1 dec. 2020 om 12:06 schreef Jan Kiszka <
> jan.kiszka@siemens.com>:
> > >> >>
> > >> >> On 01.12.20 11:26, Ronny Meeus via Xenomai wrote:
> > >> >> > Hello Xenomai community,
> > >> >> >
> > >> >> > it looks like we have a race condition in the timer object
> handling.
> > >> >> > The scope of the below mentioned issue is the alarm interface of
> the
> > >> >> > alchemy skin:
> > >> >> > int rt_alarm_start(RT_ALARM *alarm, RTIME value, RTIME interval)
> > >> >> >
> > >> >> > The documentation mentions that this start can be called also on
> an
> > >> >> > already running timer:
> > >> >> > "This service overrides any previous setup of the expiry date and
> > >> >> > reload interval for the given alarm."
> > >> >> >
> > >> >> > In the timer server code (see file lib/copperplate/timerobj.c):
> > >> >> > static void *timerobj_server (void *arg))
> > >> >> > I see the timer being re-inserted in the timeout list in case of
> a
> > >> >> > periodic timer.
> > >> >> >
> > >> >> > write_lock_nocancel(&svlock);
> > >> >> > ...
> > >> >> > if (interval.tv_sec > 0 || interval.tv_nsec > 0) {
> > >> >> > timespec_add(&tmobj->itspec.it_value, &value, &interval);
> > >> >> > timerobj_enqueue(tmobj);
> > >> >> > }
> > >> >> > write_unlock(&svlock);
> > >> >> > tmobj->handler(tmobj);
> > >> >> > write_lock_nocancel(&svlock);
> > >> >> > }
> > >> >> >
> > >> >> > This re-insert is done with the svlock taken but the timer
> specific
> > >> >> > lock is not taken.
> > >> >> >
> > >> >> > In the start on the other hand I see:
> > >> >> >
> > >> >> > int timerobj_start(struct timerobj *tmobj,
> > >> >> > void (*handler)(struct timerobj *tmobj),
> > >> >> > struct itimerspec *it) /* lock held, dropped */
> > >> >> > {
> > >> >> > tmobj->handler = handler;
> > >> >> > tmobj->itspec = *it;
> > >> >> > ...
> > >> >> > write_lock_nocancel(&svlock);
> > >> >> >
> > >> >> > So the itspec is updated with only the timerobj lock taken.
> > >> >> > If the timeout value is changed via the timerobj_start while the
> timer is
> > >> >> > under processing by the timer server, we can enter an endless
> loop (at
> > >> >> > least that is what we see sporadically)
> > >> >> >
> > >> >> > Does this make sense?
> > >> >>
> > >> >> Yes, at least to me. Patch welcome.
> > >> >
> > >> > Jan,
> > >> >
> > >> > this is the patch we are currently testing with (note that the line
> numbers
> > >> > might not match since we are not at the latest revision).
> > >> >
> > >> > diff --git a/lib/copperplate/timerobj.c b/lib/copperplate/timerobj.c
> > >> > --- a/lib/copperplate/timerobj.c
> > >> > +++ b/lib/copperplate/timerobj.c
> > >> > @@ -165,7 +165,8 @@ static void *timerobj_server(void *arg)
> > >> > timerobj_enqueue(tmobj);
> > >> > }
> > >> > write_unlock(&svlock);
> > >> > - tmobj->handler(tmobj);
> > >> > + if (tmobj->handler)
> > >> > + tmobj->handler(tmobj);
> > >> > write_lock_nocancel(&svlock);
> > >> > }
> > >> >
> > >> > @@ -232,10 +233,14 @@ int timerobj_start(struct timerobj *tmob
> > >> > void (*handler)(struct timerobj *tmobj),
> > >> > struct itimerspec *it) /* lock held, dropped */
> > >> > {
> > >> > + write_lock_nocancel(&svlock);
> > >> > +
> > >> > + if (pvholder_linked(&tmobj->next))
> > >> > + pvlist_remove_init(&tmobj->next);
> > >> > +
> > >> > tmobj->handler = handler;
> > >> > tmobj->itspec = *it;
> > >> >
> > >> > - write_lock_nocancel(&svlock);
> > >> > timerobj_enqueue(tmobj);
> > >> > write_unlock(&svlock);
> > >> > timerobj_unlock(tmobj);
> > >> >
> > >> >
> > >> >>
> > >> >> Note, though, that updating the handler will remain inherently
> racy.
> > >> >>
> > >>
> > >> Good catch. Also, we would need a consistent view of the
> > >> (value,interval,handler) triplet for any time wrt concurrent
> start/stop
> > >> updates, all accessed under server lock. We would not want a new value
> > >> being copied to be used with an old interval and/or handler for
> > >> instance.
> > >>
> > >> There is also a locking imbalance to be fixed on error in
> > >> timerobj_start().
> > >>
> > >> e.g. (proudly untested).
> > >>
> > >> diff --git a/lib/copperplate/timerobj.c b/lib/copperplate/timerobj.c
> > >> index cbfcda566..08cc0d3b9 100644
> > >> --- a/lib/copperplate/timerobj.c
> > >> +++ b/lib/copperplate/timerobj.c
> > >> @@ -98,6 +98,7 @@ static int server_prologue(void *arg)
> > >>
> > >> static void *timerobj_server(void *arg)
> > >> {
> > >> + void (*handler)(struct timerobj *tmobj);
> > >> struct timespec now, value, interval;
> > >> struct timerobj *tmobj, *tmp;
> > >> sigset_t set;
> > >> @@ -120,17 +121,18 @@ static void *timerobj_server(void *arg)
> > >>
> > >> pvlist_for_each_entry_safe(tmobj, tmp, &svtimers,
> next) {
> > >> value = tmobj->itspec.it_value;
> > >> + interval = tmobj->itspec.it_interval;
> > >> + handler = tmobj->handler;
> > >> if (timespec_after(&value, &now))
> > >> break;
> > >> pvlist_remove_init(&tmobj->next);
> > >> - interval = tmobj->itspec.it_interval;
> > >> if (interval.tv_sec > 0 || interval.tv_nsec >
> 0) {
> > >> timespec_add(&tmobj->itspec.it_value,
> > >> &value, &interval);
> > >> timerobj_enqueue(tmobj);
> > >> }
> > >> write_unlock(&svlock);
> > >> - tmobj->handler(tmobj);
> > >> + handler(tmobj);
> > >> write_lock_nocancel(&svlock);
> > >> }
> > >>
> > >> @@ -218,8 +220,7 @@ int timerobj_start(struct timerobj *tmobj,
> > >> void (*handler)(struct timerobj *tmobj),
> > >> struct itimerspec *it) /* lock held, dropped */
> > >> {
> > >> - tmobj->handler = handler;
> > >> - tmobj->itspec = *it;
> > >> + int ret = 0;
> > >>
> > >> /*
> > >> * We hold the queue lock long enough to prevent the timer
> > >> @@ -232,14 +233,23 @@ int timerobj_start(struct timerobj *tmobj,
> > >> */
> > >> write_lock_nocancel(&svlock);
> > >>
> > >> - if (__RT(timer_settime(tmobj->timer, TIMER_ABSTIME, it,
> NULL)))
> > >> - return __bt(-errno);
> > >> + if (pvholder_linked(&tmobj->next))
> > >> + pvlist_remove_init(&tmobj->next);
> > >> +
> > >> + tmobj->handler = handler;
> > >> + tmobj->itspec = *it;
> > >> +
> > >> + if (__RT(timer_settime(tmobj->timer, TIMER_ABSTIME, it,
> NULL))) {
> > >> + ret = __bt(-errno);
> > >> + goto fail;
> > >> + }
> > >>
> > >> timerobj_enqueue(tmobj);
> > >> +fail:
> > >> write_unlock(&svlock);
> > >> timerobj_unlock(tmobj);
> > >>
> > >> - return 0;
> > >> + return ret;
> > >> }
> > >>
> > >> int timerobj_stop(struct timerobj *tmobj) /* lock held, dropped */
> > >> @@ -251,10 +261,9 @@ int timerobj_stop(struct timerobj *tmobj) /*
> lock held, dropped */
> > >> if (pvholder_linked(&tmobj->next))
> > >> pvlist_remove_init(&tmobj->next);
> > >>
> > >> - write_unlock(&svlock);
> > >> -
> > >> __RT(timer_settime(tmobj->timer, 0, &itimer_stop, NULL));
> > >> tmobj->handler = NULL;
> > >> + write_unlock(&svlock);
> > >> timerobj_unlock(tmobj);
> > >>
> > >> return 0;
> > >> --
> > >
> > > Philippe,
> > >
> > > we tested with the patch you proposed and the problem seem to be
> resolved.
> > > I think this is a good improvement.
> > >
> > > Best regards,
> > > Ronny
> > >
> > >> Philippe.
> >
> > Ok, thanks for testing. Now pushing a patch referring to your original
> > post with the preliminary fix.
>
>
> Hello
>
> In the same context we have hit another issue.
> It looks like there is a problem when a timer is deleted/stopped from
> within
> the handler callback, the mechanism to traverse the list does not behave
> correctly anymore.
>
> pvlist_for_each_entry_safe(tmobj, tmp, &svtimers, next) {
> value = tmobj->itspec.it_value;
>
> In the above code, "tmp" is used to reference the next entry to be
> processed
> in the next iteration of the loop. When in the callback, this next timer is
> changed or deleted, the reference is not valid anymore.
>
> I do not see immedialy a generic a solution for the pvlist.
> For the timer-server specifically, we could always start from the first
> element
> since we will remove from the head and possibly reinsert again in the list.
>
To work around the issue in the timer server we have adapter the code like
below.
Instead of the pvlist_for_each_entry_safe we use:
while (1) {
if (pvlist_empty(&svtimers) != 0) {
break;
}
tmobj = pvlist_first_entry(&svtimers, typeof(*tmobj), next);
This seems to solve our issue since there is no next pointer anymore
which can become invalid.
> Please comment.
>
> Best regards,
> Ronny
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: race in timerobj
2021-01-23 7:40 ` Ronny Meeus
@ 2021-01-25 6:49 ` Jan Kiszka
2021-01-25 14:27 ` Ronny Meeus
0 siblings, 1 reply; 14+ messages in thread
From: Jan Kiszka @ 2021-01-25 6:49 UTC (permalink / raw)
To: Ronny Meeus, Philippe Gerum; +Cc: xenomai
On 23.01.21 08:40, Ronny Meeus wrote:
>
>
> Op vr 22 jan. 2021 om 08:57 schreef Ronny Meeus <ronny.meeus@gmail.com
> <mailto:ronny.meeus@gmail.com>>:
>
> Op vr 4 dec. 2020 om 16:29 schreef Philippe Gerum <rpm@xenomai.org
> <mailto:rpm@xenomai.org>>:
> >
> >
> > Ronny Meeus <ronny.meeus@gmail.com <mailto:ronny.meeus@gmail.com>>
> writes:
> >
> > > Op di 1 dec. 2020 om 14:51 schreef Philippe Gerum
> <rpm@xenomai.org <mailto:rpm@xenomai.org>>:
> > >>
> > >>
> > >> Ronny Meeus via Xenomai <xenomai@xenomai.org
> <mailto:xenomai@xenomai.org>> writes:
> > >>
> > >> > Op di 1 dec. 2020 om 12:06 schreef Jan Kiszka
> <jan.kiszka@siemens.com <mailto:jan.kiszka@siemens.com>>:
> > >> >>
> > >> >> On 01.12.20 11:26, Ronny Meeus via Xenomai wrote:
> > >> >> > Hello Xenomai community,
> > >> >> >
> > >> >> > it looks like we have a race condition in the timer object
> handling.
> > >> >> > The scope of the below mentioned issue is the alarm
> interface of the
> > >> >> > alchemy skin:
> > >> >> > int rt_alarm_start(RT_ALARM *alarm, RTIME value, RTIME
> interval)
> > >> >> >
> > >> >> > The documentation mentions that this start can be called
> also on an
> > >> >> > already running timer:
> > >> >> > "This service overrides any previous setup of the expiry
> date and
> > >> >> > reload interval for the given alarm."
> > >> >> >
> > >> >> > In the timer server code (see file
> lib/copperplate/timerobj.c):
> > >> >> > static void *timerobj_server (void *arg))
> > >> >> > I see the timer being re-inserted in the timeout list in
> case of a
> > >> >> > periodic timer.
> > >> >> >
> > >> >> > write_lock_nocancel(&svlock);
> > >> >> > ...
> > >> >> > if (interval.tv_sec > 0 || interval.tv_nsec > 0) {
> > >> >> > timespec_add(&tmobj->itspec.it_value, &value, &interval);
> > >> >> > timerobj_enqueue(tmobj);
> > >> >> > }
> > >> >> > write_unlock(&svlock);
> > >> >> > tmobj->handler(tmobj);
> > >> >> > write_lock_nocancel(&svlock);
> > >> >> > }
> > >> >> >
> > >> >> > This re-insert is done with the svlock taken but the timer
> specific
> > >> >> > lock is not taken.
> > >> >> >
> > >> >> > In the start on the other hand I see:
> > >> >> >
> > >> >> > int timerobj_start(struct timerobj *tmobj,
> > >> >> > void (*handler)(struct timerobj *tmobj),
> > >> >> > struct itimerspec *it) /* lock held, dropped */
> > >> >> > {
> > >> >> > tmobj->handler = handler;
> > >> >> > tmobj->itspec = *it;
> > >> >> > ...
> > >> >> > write_lock_nocancel(&svlock);
> > >> >> >
> > >> >> > So the itspec is updated with only the timerobj lock taken.
> > >> >> > If the timeout value is changed via the timerobj_start
> while the timer is
> > >> >> > under processing by the timer server, we can enter an
> endless loop (at
> > >> >> > least that is what we see sporadically)
> > >> >> >
> > >> >> > Does this make sense?
> > >> >>
> > >> >> Yes, at least to me. Patch welcome.
> > >> >
> > >> > Jan,
> > >> >
> > >> > this is the patch we are currently testing with (note that
> the line numbers
> > >> > might not match since we are not at the latest revision).
> > >> >
> > >> > diff --git a/lib/copperplate/timerobj.c
> b/lib/copperplate/timerobj.c
> > >> > --- a/lib/copperplate/timerobj.c
> > >> > +++ b/lib/copperplate/timerobj.c
> > >> > @@ -165,7 +165,8 @@ static void *timerobj_server(void *arg)
> > >> > timerobj_enqueue(tmobj);
> > >> > }
> > >> > write_unlock(&svlock);
> > >> > - tmobj->handler(tmobj);
> > >> > + if (tmobj->handler)
> > >> > + tmobj->handler(tmobj);
> > >> > write_lock_nocancel(&svlock);
> > >> > }
> > >> >
> > >> > @@ -232,10 +233,14 @@ int timerobj_start(struct timerobj *tmob
> > >> > void (*handler)(struct timerobj *tmobj),
> > >> > struct itimerspec *it) /* lock held,
> dropped */
> > >> > {
> > >> > + write_lock_nocancel(&svlock);
> > >> > +
> > >> > + if (pvholder_linked(&tmobj->next))
> > >> > + pvlist_remove_init(&tmobj->next);
> > >> > +
> > >> > tmobj->handler = handler;
> > >> > tmobj->itspec = *it;
> > >> >
> > >> > - write_lock_nocancel(&svlock);
> > >> > timerobj_enqueue(tmobj);
> > >> > write_unlock(&svlock);
> > >> > timerobj_unlock(tmobj);
> > >> >
> > >> >
> > >> >>
> > >> >> Note, though, that updating the handler will remain
> inherently racy.
> > >> >>
> > >>
> > >> Good catch. Also, we would need a consistent view of the
> > >> (value,interval,handler) triplet for any time wrt concurrent
> start/stop
> > >> updates, all accessed under server lock. We would not want a
> new value
> > >> being copied to be used with an old interval and/or handler for
> > >> instance.
> > >>
> > >> There is also a locking imbalance to be fixed on error in
> > >> timerobj_start().
> > >>
> > >> e.g. (proudly untested).
> > >>
> > >> diff --git a/lib/copperplate/timerobj.c
> b/lib/copperplate/timerobj.c
> > >> index cbfcda566..08cc0d3b9 100644
> > >> --- a/lib/copperplate/timerobj.c
> > >> +++ b/lib/copperplate/timerobj.c
> > >> @@ -98,6 +98,7 @@ static int server_prologue(void *arg)
> > >>
> > >> static void *timerobj_server(void *arg)
> > >> {
> > >> + void (*handler)(struct timerobj *tmobj);
> > >> struct timespec now, value, interval;
> > >> struct timerobj *tmobj, *tmp;
> > >> sigset_t set;
> > >> @@ -120,17 +121,18 @@ static void *timerobj_server(void *arg)
> > >>
> > >> pvlist_for_each_entry_safe(tmobj, tmp,
> &svtimers, next) {
> > >> value = tmobj->itspec.it_value;
> > >> + interval = tmobj->itspec.it_interval;
> > >> + handler = tmobj->handler;
> > >> if (timespec_after(&value, &now))
> > >> break;
> > >> pvlist_remove_init(&tmobj->next);
> > >> - interval = tmobj->itspec.it_interval;
> > >> if (interval.tv_sec > 0 ||
> interval.tv_nsec > 0) {
> > >>
> timespec_add(&tmobj->itspec.it_value,
> > >> &value, &interval);
> > >> timerobj_enqueue(tmobj);
> > >> }
> > >> write_unlock(&svlock);
> > >> - tmobj->handler(tmobj);
> > >> + handler(tmobj);
> > >> write_lock_nocancel(&svlock);
> > >> }
> > >>
> > >> @@ -218,8 +220,7 @@ int timerobj_start(struct timerobj *tmobj,
> > >> void (*handler)(struct timerobj *tmobj),
> > >> struct itimerspec *it) /* lock held, dropped */
> > >> {
> > >> - tmobj->handler = handler;
> > >> - tmobj->itspec = *it;
> > >> + int ret = 0;
> > >>
> > >> /*
> > >> * We hold the queue lock long enough to prevent the timer
> > >> @@ -232,14 +233,23 @@ int timerobj_start(struct timerobj *tmobj,
> > >> */
> > >> write_lock_nocancel(&svlock);
> > >>
> > >> - if (__RT(timer_settime(tmobj->timer, TIMER_ABSTIME, it,
> NULL)))
> > >> - return __bt(-errno);
> > >> + if (pvholder_linked(&tmobj->next))
> > >> + pvlist_remove_init(&tmobj->next);
> > >> +
> > >> + tmobj->handler = handler;
> > >> + tmobj->itspec = *it;
> > >> +
> > >> + if (__RT(timer_settime(tmobj->timer, TIMER_ABSTIME, it,
> NULL))) {
> > >> + ret = __bt(-errno);
> > >> + goto fail;
> > >> + }
> > >>
> > >> timerobj_enqueue(tmobj);
> > >> +fail:
> > >> write_unlock(&svlock);
> > >> timerobj_unlock(tmobj);
> > >>
> > >> - return 0;
> > >> + return ret;
> > >> }
> > >>
> > >> int timerobj_stop(struct timerobj *tmobj) /* lock held, dropped */
> > >> @@ -251,10 +261,9 @@ int timerobj_stop(struct timerobj *tmobj)
> /* lock held, dropped */
> > >> if (pvholder_linked(&tmobj->next))
> > >> pvlist_remove_init(&tmobj->next);
> > >>
> > >> - write_unlock(&svlock);
> > >> -
> > >> __RT(timer_settime(tmobj->timer, 0, &itimer_stop, NULL));
> > >> tmobj->handler = NULL;
> > >> + write_unlock(&svlock);
> > >> timerobj_unlock(tmobj);
> > >>
> > >> return 0;
> > >> --
> > >
> > > Philippe,
> > >
> > > we tested with the patch you proposed and the problem seem to be
> resolved.
> > > I think this is a good improvement.
> > >
> > > Best regards,
> > > Ronny
> > >
> > >> Philippe.
> >
> > Ok, thanks for testing. Now pushing a patch referring to your original
> > post with the preliminary fix.
>
>
> Hello
>
> In the same context we have hit another issue.
> It looks like there is a problem when a timer is deleted/stopped
> from within
> the handler callback, the mechanism to traverse the list does not behave
> correctly anymore.
>
> pvlist_for_each_entry_safe(tmobj, tmp, &svtimers, next) {
> value = tmobj->itspec.it_value;
>
> In the above code, "tmp" is used to reference the next entry to be
> processed
> in the next iteration of the loop. When in the callback, this next
> timer is
> changed or deleted, the reference is not valid anymore.
>
> I do not see immedialy a generic a solution for the pvlist.
> For the timer-server specifically, we could always start from the
> first element
> since we will remove from the head and possibly reinsert again in
> the list.
>
>
> To work around the issue in the timer server we have adapter the code
> like below.
> Instead of the pvlist_for_each_entry_safe we use:
>
> while (1) {
> if (pvlist_empty(&svtimers) != 0) {
> break;
> }
> tmobj = pvlist_first_entry(&svtimers, typeof(*tmobj), next);
>
> This seems to solve our issue since there is no next pointer anymore
> which can become invalid.
>
This looks reasonable to me: every expired times will be removed from
the list or re-appended after the current date.
Could you translate this to a patch with appropriate commit message?
TIA,
Jan
--
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: race in timerobj
2021-01-25 6:49 ` Jan Kiszka
@ 2021-01-25 14:27 ` Ronny Meeus
2021-01-25 14:36 ` Jan Kiszka
0 siblings, 1 reply; 14+ messages in thread
From: Ronny Meeus @ 2021-01-25 14:27 UTC (permalink / raw)
To: Jan Kiszka; +Cc: Philippe Gerum, xenomai
commit b38a39bcd758872201e71c07a24d8b5e7f26c3ac
Author: Ronny Meeus <ronny.meeus@gmail.com>
Date: Mon Jan 25 15:11:52 2021 +0100
We observe an issue that the timer-list gets corrupted resulting in an
endless loop executed by the timer-server thread.
During the processing of the timeout list, a pointer to the next timer
to be handled is kept in the tmp stack variable.
Just before calling the timer handler of the current timer the lock on
the timer list is released giving other threads the opportunity to
change the list.
If the timer currently referenced by tmp is deleted, we and up with an
invalid node (next pointer pointing to itself) and this will result in
an endless loop of the timer server.
Test code is not available but I have seen this issue in our real
production code and after applying this path, the issue is solved.
The patch basically changes the timer server logic to always start
from the beginning of the list since when a timer is processed, it is
either removed (one-shot) or reinserted in a different location in the
list.
The processing of the list will stop anyhow if all timers that need
to expire up to "now" are handled.
diff --git a/lib/copperplate/timerobj.c b/lib/copperplate/timerobj.c
index 08cc0d3..b6c6abb 100644
--- a/lib/copperplate/timerobj.c
+++ b/lib/copperplate/timerobj.c
@@ -100,7 +100,7 @@ static void *timerobj_server(void *arg)
{
void (*handler)(struct timerobj *tmobj);
struct timespec now, value, interval;
- struct timerobj *tmobj, *tmp;
+ struct timerobj *tmobj;
sigset_t set;
int sig, ret;
@@ -119,7 +119,12 @@ static void *timerobj_server(void *arg)
__RT(clock_gettime(CLOCK_COPPERPLATE, &now));
- pvlist_for_each_entry_safe(tmobj, tmp, &svtimers, next) {
+ for (;;) {
+ if (pvlist_empty(&svtimers)) {
+ break;
+ }
+ tmobj = pvlist_first_entry(&svtimers,
typeof(*tmobj), next);
+
value = tmobj->itspec.it_value;
interval = tmobj->itspec.it_interval;
handler = tmobj->handler;
Op ma 25 jan. 2021 om 07:49 schreef Jan Kiszka <jan.kiszka@siemens.com>:
>
> On 23.01.21 08:40, Ronny Meeus wrote:
> >
> >
> > Op vr 22 jan. 2021 om 08:57 schreef Ronny Meeus <ronny.meeus@gmail.com
> > <mailto:ronny.meeus@gmail.com>>:
> >
> > Op vr 4 dec. 2020 om 16:29 schreef Philippe Gerum <rpm@xenomai.org
> > <mailto:rpm@xenomai.org>>:
> > >
> > >
> > > Ronny Meeus <ronny.meeus@gmail.com <mailto:ronny.meeus@gmail.com>>
> > writes:
> > >
> > > > Op di 1 dec. 2020 om 14:51 schreef Philippe Gerum
> > <rpm@xenomai.org <mailto:rpm@xenomai.org>>:
> > > >>
> > > >>
> > > >> Ronny Meeus via Xenomai <xenomai@xenomai.org
> > <mailto:xenomai@xenomai.org>> writes:
> > > >>
> > > >> > Op di 1 dec. 2020 om 12:06 schreef Jan Kiszka
> > <jan.kiszka@siemens.com <mailto:jan.kiszka@siemens.com>>:
> > > >> >>
> > > >> >> On 01.12.20 11:26, Ronny Meeus via Xenomai wrote:
> > > >> >> > Hello Xenomai community,
> > > >> >> >
> > > >> >> > it looks like we have a race condition in the timer object
> > handling.
> > > >> >> > The scope of the below mentioned issue is the alarm
> > interface of the
> > > >> >> > alchemy skin:
> > > >> >> > int rt_alarm_start(RT_ALARM *alarm, RTIME value, RTIME
> > interval)
> > > >> >> >
> > > >> >> > The documentation mentions that this start can be called
> > also on an
> > > >> >> > already running timer:
> > > >> >> > "This service overrides any previous setup of the expiry
> > date and
> > > >> >> > reload interval for the given alarm."
> > > >> >> >
> > > >> >> > In the timer server code (see file
> > lib/copperplate/timerobj.c):
> > > >> >> > static void *timerobj_server (void *arg))
> > > >> >> > I see the timer being re-inserted in the timeout list in
> > case of a
> > > >> >> > periodic timer.
> > > >> >> >
> > > >> >> > write_lock_nocancel(&svlock);
> > > >> >> > ...
> > > >> >> > if (interval.tv_sec > 0 || interval.tv_nsec > 0) {
> > > >> >> > timespec_add(&tmobj->itspec.it_value, &value, &interval);
> > > >> >> > timerobj_enqueue(tmobj);
> > > >> >> > }
> > > >> >> > write_unlock(&svlock);
> > > >> >> > tmobj->handler(tmobj);
> > > >> >> > write_lock_nocancel(&svlock);
> > > >> >> > }
> > > >> >> >
> > > >> >> > This re-insert is done with the svlock taken but the timer
> > specific
> > > >> >> > lock is not taken.
> > > >> >> >
> > > >> >> > In the start on the other hand I see:
> > > >> >> >
> > > >> >> > int timerobj_start(struct timerobj *tmobj,
> > > >> >> > void (*handler)(struct timerobj *tmobj),
> > > >> >> > struct itimerspec *it) /* lock held, dropped */
> > > >> >> > {
> > > >> >> > tmobj->handler = handler;
> > > >> >> > tmobj->itspec = *it;
> > > >> >> > ...
> > > >> >> > write_lock_nocancel(&svlock);
> > > >> >> >
> > > >> >> > So the itspec is updated with only the timerobj lock taken.
> > > >> >> > If the timeout value is changed via the timerobj_start
> > while the timer is
> > > >> >> > under processing by the timer server, we can enter an
> > endless loop (at
> > > >> >> > least that is what we see sporadically)
> > > >> >> >
> > > >> >> > Does this make sense?
> > > >> >>
> > > >> >> Yes, at least to me. Patch welcome.
> > > >> >
> > > >> > Jan,
> > > >> >
> > > >> > this is the patch we are currently testing with (note that
> > the line numbers
> > > >> > might not match since we are not at the latest revision).
> > > >> >
> > > >> > diff --git a/lib/copperplate/timerobj.c
> > b/lib/copperplate/timerobj.c
> > > >> > --- a/lib/copperplate/timerobj.c
> > > >> > +++ b/lib/copperplate/timerobj.c
> > > >> > @@ -165,7 +165,8 @@ static void *timerobj_server(void *arg)
> > > >> > timerobj_enqueue(tmobj);
> > > >> > }
> > > >> > write_unlock(&svlock);
> > > >> > - tmobj->handler(tmobj);
> > > >> > + if (tmobj->handler)
> > > >> > + tmobj->handler(tmobj);
> > > >> > write_lock_nocancel(&svlock);
> > > >> > }
> > > >> >
> > > >> > @@ -232,10 +233,14 @@ int timerobj_start(struct timerobj *tmob
> > > >> > void (*handler)(struct timerobj *tmobj),
> > > >> > struct itimerspec *it) /* lock held,
> > dropped */
> > > >> > {
> > > >> > + write_lock_nocancel(&svlock);
> > > >> > +
> > > >> > + if (pvholder_linked(&tmobj->next))
> > > >> > + pvlist_remove_init(&tmobj->next);
> > > >> > +
> > > >> > tmobj->handler = handler;
> > > >> > tmobj->itspec = *it;
> > > >> >
> > > >> > - write_lock_nocancel(&svlock);
> > > >> > timerobj_enqueue(tmobj);
> > > >> > write_unlock(&svlock);
> > > >> > timerobj_unlock(tmobj);
> > > >> >
> > > >> >
> > > >> >>
> > > >> >> Note, though, that updating the handler will remain
> > inherently racy.
> > > >> >>
> > > >>
> > > >> Good catch. Also, we would need a consistent view of the
> > > >> (value,interval,handler) triplet for any time wrt concurrent
> > start/stop
> > > >> updates, all accessed under server lock. We would not want a
> > new value
> > > >> being copied to be used with an old interval and/or handler for
> > > >> instance.
> > > >>
> > > >> There is also a locking imbalance to be fixed on error in
> > > >> timerobj_start().
> > > >>
> > > >> e.g. (proudly untested).
> > > >>
> > > >> diff --git a/lib/copperplate/timerobj.c
> > b/lib/copperplate/timerobj.c
> > > >> index cbfcda566..08cc0d3b9 100644
> > > >> --- a/lib/copperplate/timerobj.c
> > > >> +++ b/lib/copperplate/timerobj.c
> > > >> @@ -98,6 +98,7 @@ static int server_prologue(void *arg)
> > > >>
> > > >> static void *timerobj_server(void *arg)
> > > >> {
> > > >> + void (*handler)(struct timerobj *tmobj);
> > > >> struct timespec now, value, interval;
> > > >> struct timerobj *tmobj, *tmp;
> > > >> sigset_t set;
> > > >> @@ -120,17 +121,18 @@ static void *timerobj_server(void *arg)
> > > >>
> > > >> pvlist_for_each_entry_safe(tmobj, tmp,
> > &svtimers, next) {
> > > >> value = tmobj->itspec.it_value;
> > > >> + interval = tmobj->itspec.it_interval;
> > > >> + handler = tmobj->handler;
> > > >> if (timespec_after(&value, &now))
> > > >> break;
> > > >> pvlist_remove_init(&tmobj->next);
> > > >> - interval = tmobj->itspec.it_interval;
> > > >> if (interval.tv_sec > 0 ||
> > interval.tv_nsec > 0) {
> > > >>
> > timespec_add(&tmobj->itspec.it_value,
> > > >> &value, &interval);
> > > >> timerobj_enqueue(tmobj);
> > > >> }
> > > >> write_unlock(&svlock);
> > > >> - tmobj->handler(tmobj);
> > > >> + handler(tmobj);
> > > >> write_lock_nocancel(&svlock);
> > > >> }
> > > >>
> > > >> @@ -218,8 +220,7 @@ int timerobj_start(struct timerobj *tmobj,
> > > >> void (*handler)(struct timerobj *tmobj),
> > > >> struct itimerspec *it) /* lock held, dropped */
> > > >> {
> > > >> - tmobj->handler = handler;
> > > >> - tmobj->itspec = *it;
> > > >> + int ret = 0;
> > > >>
> > > >> /*
> > > >> * We hold the queue lock long enough to prevent the timer
> > > >> @@ -232,14 +233,23 @@ int timerobj_start(struct timerobj *tmobj,
> > > >> */
> > > >> write_lock_nocancel(&svlock);
> > > >>
> > > >> - if (__RT(timer_settime(tmobj->timer, TIMER_ABSTIME, it,
> > NULL)))
> > > >> - return __bt(-errno);
> > > >> + if (pvholder_linked(&tmobj->next))
> > > >> + pvlist_remove_init(&tmobj->next);
> > > >> +
> > > >> + tmobj->handler = handler;
> > > >> + tmobj->itspec = *it;
> > > >> +
> > > >> + if (__RT(timer_settime(tmobj->timer, TIMER_ABSTIME, it,
> > NULL))) {
> > > >> + ret = __bt(-errno);
> > > >> + goto fail;
> > > >> + }
> > > >>
> > > >> timerobj_enqueue(tmobj);
> > > >> +fail:
> > > >> write_unlock(&svlock);
> > > >> timerobj_unlock(tmobj);
> > > >>
> > > >> - return 0;
> > > >> + return ret;
> > > >> }
> > > >>
> > > >> int timerobj_stop(struct timerobj *tmobj) /* lock held, dropped */
> > > >> @@ -251,10 +261,9 @@ int timerobj_stop(struct timerobj *tmobj)
> > /* lock held, dropped */
> > > >> if (pvholder_linked(&tmobj->next))
> > > >> pvlist_remove_init(&tmobj->next);
> > > >>
> > > >> - write_unlock(&svlock);
> > > >> -
> > > >> __RT(timer_settime(tmobj->timer, 0, &itimer_stop, NULL));
> > > >> tmobj->handler = NULL;
> > > >> + write_unlock(&svlock);
> > > >> timerobj_unlock(tmobj);
> > > >>
> > > >> return 0;
> > > >> --
> > > >
> > > > Philippe,
> > > >
> > > > we tested with the patch you proposed and the problem seem to be
> > resolved.
> > > > I think this is a good improvement.
> > > >
> > > > Best regards,
> > > > Ronny
> > > >
> > > >> Philippe.
> > >
> > > Ok, thanks for testing. Now pushing a patch referring to your original
> > > post with the preliminary fix.
> >
> >
> > Hello
> >
> > In the same context we have hit another issue.
> > It looks like there is a problem when a timer is deleted/stopped
> > from within
> > the handler callback, the mechanism to traverse the list does not behave
> > correctly anymore.
> >
> > pvlist_for_each_entry_safe(tmobj, tmp, &svtimers, next) {
> > value = tmobj->itspec.it_value;
> >
> > In the above code, "tmp" is used to reference the next entry to be
> > processed
> > in the next iteration of the loop. When in the callback, this next
> > timer is
> > changed or deleted, the reference is not valid anymore.
> >
> > I do not see immedialy a generic a solution for the pvlist.
> > For the timer-server specifically, we could always start from the
> > first element
> > since we will remove from the head and possibly reinsert again in
> > the list.
> >
> >
> > To work around the issue in the timer server we have adapter the code
> > like below.
> > Instead of the pvlist_for_each_entry_safe we use:
> >
> > while (1) {
> > if (pvlist_empty(&svtimers) != 0) {
> > break;
> > }
> > tmobj = pvlist_first_entry(&svtimers, typeof(*tmobj), next);
> >
> > This seems to solve our issue since there is no next pointer anymore
> > which can become invalid.
> >
>
> This looks reasonable to me: every expired times will be removed from
> the list or re-appended after the current date.
>
> Could you translate this to a patch with appropriate commit message?
>
> TIA,
> Jan
>
> --
> Siemens AG, T RDA IOT
> Corporate Competence Center Embedded Linux
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: race in timerobj
2021-01-25 14:27 ` Ronny Meeus
@ 2021-01-25 14:36 ` Jan Kiszka
2021-01-25 15:10 ` Ronny Meeus
0 siblings, 1 reply; 14+ messages in thread
From: Jan Kiszka @ 2021-01-25 14:36 UTC (permalink / raw)
To: Ronny Meeus; +Cc: Philippe Gerum, xenomai
On 25.01.21 15:27, Ronny Meeus wrote:
> commit b38a39bcd758872201e71c07a24d8b5e7f26c3ac
> Author: Ronny Meeus <ronny.meeus@gmail.com>
> Date: Mon Jan 25 15:11:52 2021 +0100
>
> We observe an issue that the timer-list gets corrupted resulting in an
> endless loop executed by the timer-server thread.
>
> During the processing of the timeout list, a pointer to the next timer
> to be handled is kept in the tmp stack variable.
> Just before calling the timer handler of the current timer the lock on
> the timer list is released giving other threads the opportunity to
> change the list.
> If the timer currently referenced by tmp is deleted, we and up with an
> invalid node (next pointer pointing to itself) and this will result in
> an endless loop of the timer server.
>
> Test code is not available but I have seen this issue in our real
> production code and after applying this path, the issue is solved.
>
> The patch basically changes the timer server logic to always start
> from the beginning of the list since when a timer is processed, it is
> either removed (one-shot) or reinserted in a different location in the
> list.
> The processing of the list will stop anyhow if all timers that need
> to expire up to "now" are handled.
>
DCO (https://developercertificate.org) via signed-off-by missing.
> diff --git a/lib/copperplate/timerobj.c b/lib/copperplate/timerobj.c
> index 08cc0d3..b6c6abb 100644
> --- a/lib/copperplate/timerobj.c
> +++ b/lib/copperplate/timerobj.c
> @@ -100,7 +100,7 @@ static void *timerobj_server(void *arg)
> {
> void (*handler)(struct timerobj *tmobj);
> struct timespec now, value, interval;
> - struct timerobj *tmobj, *tmp;
> + struct timerobj *tmobj;
> sigset_t set;
> int sig, ret;
>
> @@ -119,7 +119,12 @@ static void *timerobj_server(void *arg)
>
> __RT(clock_gettime(CLOCK_COPPERPLATE, &now));
>
> - pvlist_for_each_entry_safe(tmobj, tmp, &svtimers, next) {
> + for (;;) {
> + if (pvlist_empty(&svtimers)) {
How about "while (!pvlist_empty(&svtimers)) ..."?
> + break;
> + }
> + tmobj = pvlist_first_entry(&svtimers,
> typeof(*tmobj), next);
> +
And then your mail client rewraps the email. If possible, disable this,
otherwise use git send-email.
> value = tmobj->itspec.it_value;
> interval = tmobj->itspec.it_interval;
> handler = tmobj->handler;
>
>
Thanks,
Jan
--
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: race in timerobj
2021-01-25 14:36 ` Jan Kiszka
@ 2021-01-25 15:10 ` Ronny Meeus
2021-01-25 15:26 ` Jan Kiszka
2021-01-25 15:56 ` Philippe Gerum
0 siblings, 2 replies; 14+ messages in thread
From: Ronny Meeus @ 2021-01-25 15:10 UTC (permalink / raw)
To: Jan Kiszka; +Cc: Philippe Gerum, xenomai
Jan,
This is my last attempt since I do not want to waste more time on this issue.
Feel free to change whatever you want to the patch.
I put my gmail in the correct mode but it keeps on wrapping
the text and git email also doesn't work (we do not use git overhere)
Best regards,
Ronny
commit 352b0355f617bfbb38f9bd7b1f7d74967f44c857
Author: Ronny Meeus <ronny.meeus@gmail.com>
Date: Mon Jan 25 15:11:52 2021 +0100
copperplate/timerobj: fix corrupted timer list.
We observe an issue that the timer-list gets corrupted resulting in an
endless loop executed by the timer-server thread.
During the processing of the timeout list, a pointer to the next timer
to be handled is kept in the tmp stack variable.
Just before calling the timer handler of the current timer the lock on
the timer list is released giving other threads to change the list.
If the timer currently referenced by tmp is deleted, we end up with an
invalid node (next pointer pointing to itself) and this will result in
an endless loop of the timer server.
Test code is not available but I have seen this issue in our real
production code and after applying this path, the issue is solved.
The patch basically changes the timer server logic to always start
from the beginning of the list since when a timer is processed, it is
either removed (one-shot) or reinserted in a different location in the
list.
The processing of the list will stop anyhow if all timers that need
to expire up to "now" are handled.
Signed-off-by: Ronny Meeus <ronny.meeus@gmail.com>
diff --git a/lib/copperplate/timerobj.c b/lib/copperplate/timerobj.c
index 08cc0d3..96ad2e5 100644
--- a/lib/copperplate/timerobj.c
+++ b/lib/copperplate/timerobj.c
@@ -100,7 +100,7 @@ static void *timerobj_server(void *arg)
{
void (*handler)(struct timerobj *tmobj);
struct timespec now, value, interval;
- struct timerobj *tmobj, *tmp;
+ struct timerobj *tmobj;
sigset_t set;
int sig, ret;
@@ -119,7 +119,9 @@ static void *timerobj_server(void *arg)
__RT(clock_gettime(CLOCK_COPPERPLATE, &now));
- pvlist_for_each_entry_safe(tmobj, tmp, &svtimers, next) {
+ while (!pvlist_empty(&svtimers)) {
+ tmobj = pvlist_first_entry(&svtimers,
typeof(*tmobj), next);
+
value = tmobj->itspec.it_value;
interval = tmobj->itspec.it_interval;
handler = tmobj->handler;
Op ma 25 jan. 2021 om 15:36 schreef Jan Kiszka <jan.kiszka@siemens.com>:
>
> On 25.01.21 15:27, Ronny Meeus wrote:
> > commit b38a39bcd758872201e71c07a24d8b5e7f26c3ac
> > Author: Ronny Meeus <ronny.meeus@gmail.com>
> > Date: Mon Jan 25 15:11:52 2021 +0100
> >
> > We observe an issue that the timer-list gets corrupted resulting in an
> > endless loop executed by the timer-server thread.
> >
> > During the processing of the timeout list, a pointer to the next timer
> > to be handled is kept in the tmp stack variable.
> > Just before calling the timer handler of the current timer the lock on
> > the timer list is released giving other threads the opportunity to
> > change the list.
> > If the timer currently referenced by tmp is deleted, we and up with an
> > invalid node (next pointer pointing to itself) and this will result in
> > an endless loop of the timer server.
> >
> > Test code is not available but I have seen this issue in our real
> > production code and after applying this path, the issue is solved.
> >
> > The patch basically changes the timer server logic to always start
> > from the beginning of the list since when a timer is processed, it is
> > either removed (one-shot) or reinserted in a different location in the
> > list.
> > The processing of the list will stop anyhow if all timers that need
> > to expire up to "now" are handled.
> >
>
> DCO (https://developercertificate.org) via signed-off-by missing.
>
> > diff --git a/lib/copperplate/timerobj.c b/lib/copperplate/timerobj.c
> > index 08cc0d3..b6c6abb 100644
> > --- a/lib/copperplate/timerobj.c
> > +++ b/lib/copperplate/timerobj.c
> > @@ -100,7 +100,7 @@ static void *timerobj_server(void *arg)
> > {
> > void (*handler)(struct timerobj *tmobj);
> > struct timespec now, value, interval;
> > - struct timerobj *tmobj, *tmp;
> > + struct timerobj *tmobj;
> > sigset_t set;
> > int sig, ret;
> >
> > @@ -119,7 +119,12 @@ static void *timerobj_server(void *arg)
> >
> > __RT(clock_gettime(CLOCK_COPPERPLATE, &now));
> >
> > - pvlist_for_each_entry_safe(tmobj, tmp, &svtimers, next) {
> > + for (;;) {
> > + if (pvlist_empty(&svtimers)) {
>
> How about "while (!pvlist_empty(&svtimers)) ..."?
>
> > + break;
> > + }
> > + tmobj = pvlist_first_entry(&svtimers,
> > typeof(*tmobj), next);
> > +
>
> And then your mail client rewraps the email. If possible, disable this,
> otherwise use git send-email.
>
> > value = tmobj->itspec.it_value;
> > interval = tmobj->itspec.it_interval;
> > handler = tmobj->handler;
> >
> >
>
> Thanks,
> Jan
>
> --
> Siemens AG, T RDA IOT
> Corporate Competence Center Embedded Linux
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: race in timerobj
2021-01-25 15:10 ` Ronny Meeus
@ 2021-01-25 15:26 ` Jan Kiszka
2021-01-25 15:56 ` Philippe Gerum
1 sibling, 0 replies; 14+ messages in thread
From: Jan Kiszka @ 2021-01-25 15:26 UTC (permalink / raw)
To: Ronny Meeus, Philippe Gerum; +Cc: xenomai
On 25.01.21 16:10, Ronny Meeus wrote:
> Jan,
>
> This is my last attempt since I do not want to waste more time on this issue.
> Feel free to change whatever you want to the patch.
> I put my gmail in the correct mode but it keeps on wrapping
> the text and git email also doesn't work (we do not use git overhere)
That's why there is git send-email, to by-pass unsuitable interfaces
(including web browsers).
Thanks anyway, I've hand-edited the patch to make it apply.
Philippe, any feedback from you on the approach?
Jan
>
> Best regards,
> Ronny
>
> commit 352b0355f617bfbb38f9bd7b1f7d74967f44c857
> Author: Ronny Meeus <ronny.meeus@gmail.com>
> Date: Mon Jan 25 15:11:52 2021 +0100
>
> copperplate/timerobj: fix corrupted timer list.
>
> We observe an issue that the timer-list gets corrupted resulting in an
> endless loop executed by the timer-server thread.
>
> During the processing of the timeout list, a pointer to the next timer
> to be handled is kept in the tmp stack variable.
> Just before calling the timer handler of the current timer the lock on
> the timer list is released giving other threads to change the list.
> If the timer currently referenced by tmp is deleted, we end up with an
> invalid node (next pointer pointing to itself) and this will result in
> an endless loop of the timer server.
>
> Test code is not available but I have seen this issue in our real
> production code and after applying this path, the issue is solved.
>
> The patch basically changes the timer server logic to always start
> from the beginning of the list since when a timer is processed, it is
> either removed (one-shot) or reinserted in a different location in the
> list.
> The processing of the list will stop anyhow if all timers that need
> to expire up to "now" are handled.
>
> Signed-off-by: Ronny Meeus <ronny.meeus@gmail.com>
>
> diff --git a/lib/copperplate/timerobj.c b/lib/copperplate/timerobj.c
> index 08cc0d3..96ad2e5 100644
> --- a/lib/copperplate/timerobj.c
> +++ b/lib/copperplate/timerobj.c
> @@ -100,7 +100,7 @@ static void *timerobj_server(void *arg)
> {
> void (*handler)(struct timerobj *tmobj);
> struct timespec now, value, interval;
> - struct timerobj *tmobj, *tmp;
> + struct timerobj *tmobj;
> sigset_t set;
> int sig, ret;
>
> @@ -119,7 +119,9 @@ static void *timerobj_server(void *arg)
>
> __RT(clock_gettime(CLOCK_COPPERPLATE, &now));
>
> - pvlist_for_each_entry_safe(tmobj, tmp, &svtimers, next) {
> + while (!pvlist_empty(&svtimers)) {
> + tmobj = pvlist_first_entry(&svtimers,
> typeof(*tmobj), next);
> +
> value = tmobj->itspec.it_value;
> interval = tmobj->itspec.it_interval;
> handler = tmobj->handler;
>
>
> Op ma 25 jan. 2021 om 15:36 schreef Jan Kiszka <jan.kiszka@siemens.com>:
>>
>> On 25.01.21 15:27, Ronny Meeus wrote:
>>> commit b38a39bcd758872201e71c07a24d8b5e7f26c3ac
>>> Author: Ronny Meeus <ronny.meeus@gmail.com>
>>> Date: Mon Jan 25 15:11:52 2021 +0100
>>>
>>> We observe an issue that the timer-list gets corrupted resulting in an
>>> endless loop executed by the timer-server thread.
>>>
>>> During the processing of the timeout list, a pointer to the next timer
>>> to be handled is kept in the tmp stack variable.
>>> Just before calling the timer handler of the current timer the lock on
>>> the timer list is released giving other threads the opportunity to
>>> change the list.
>>> If the timer currently referenced by tmp is deleted, we and up with an
>>> invalid node (next pointer pointing to itself) and this will result in
>>> an endless loop of the timer server.
>>>
>>> Test code is not available but I have seen this issue in our real
>>> production code and after applying this path, the issue is solved.
>>>
>>> The patch basically changes the timer server logic to always start
>>> from the beginning of the list since when a timer is processed, it is
>>> either removed (one-shot) or reinserted in a different location in the
>>> list.
>>> The processing of the list will stop anyhow if all timers that need
>>> to expire up to "now" are handled.
>>>
>>
>> DCO (https://developercertificate.org) via signed-off-by missing.
>>
>>> diff --git a/lib/copperplate/timerobj.c b/lib/copperplate/timerobj.c
>>> index 08cc0d3..b6c6abb 100644
>>> --- a/lib/copperplate/timerobj.c
>>> +++ b/lib/copperplate/timerobj.c
>>> @@ -100,7 +100,7 @@ static void *timerobj_server(void *arg)
>>> {
>>> void (*handler)(struct timerobj *tmobj);
>>> struct timespec now, value, interval;
>>> - struct timerobj *tmobj, *tmp;
>>> + struct timerobj *tmobj;
>>> sigset_t set;
>>> int sig, ret;
>>>
>>> @@ -119,7 +119,12 @@ static void *timerobj_server(void *arg)
>>>
>>> __RT(clock_gettime(CLOCK_COPPERPLATE, &now));
>>>
>>> - pvlist_for_each_entry_safe(tmobj, tmp, &svtimers, next) {
>>> + for (;;) {
>>> + if (pvlist_empty(&svtimers)) {
>>
>> How about "while (!pvlist_empty(&svtimers)) ..."?
>>
>>> + break;
>>> + }
>>> + tmobj = pvlist_first_entry(&svtimers,
>>> typeof(*tmobj), next);
>>> +
>>
>> And then your mail client rewraps the email. If possible, disable this,
>> otherwise use git send-email.
>>
>>> value = tmobj->itspec.it_value;
>>> interval = tmobj->itspec.it_interval;
>>> handler = tmobj->handler;
>>>
>>>
>>
>> Thanks,
>> Jan
>>
>> --
>> Siemens AG, T RDA IOT
>> Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: race in timerobj
2021-01-25 15:10 ` Ronny Meeus
2021-01-25 15:26 ` Jan Kiszka
@ 2021-01-25 15:56 ` Philippe Gerum
1 sibling, 0 replies; 14+ messages in thread
From: Philippe Gerum @ 2021-01-25 15:56 UTC (permalink / raw)
To: Ronny Meeus; +Cc: Jan Kiszka, xenomai
Ronny Meeus <ronny.meeus@gmail.com> writes:
> Jan,
>
> This is my last attempt since I do not want to waste more time on this issue.
> Feel free to change whatever you want to the patch.
> I put my gmail in the correct mode but it keeps on wrapping
> the text and git email also doesn't work (we do not use git overhere)
>
> Best regards,
> Ronny
>
> commit 352b0355f617bfbb38f9bd7b1f7d74967f44c857
> Author: Ronny Meeus <ronny.meeus@gmail.com>
> Date: Mon Jan 25 15:11:52 2021 +0100
>
> copperplate/timerobj: fix corrupted timer list.
>
> We observe an issue that the timer-list gets corrupted resulting in an
> endless loop executed by the timer-server thread.
>
> During the processing of the timeout list, a pointer to the next timer
> to be handled is kept in the tmp stack variable.
> Just before calling the timer handler of the current timer the lock on
> the timer list is released giving other threads to change the list.
> If the timer currently referenced by tmp is deleted, we end up with an
> invalid node (next pointer pointing to itself) and this will result in
> an endless loop of the timer server.
>
> Test code is not available but I have seen this issue in our real
> production code and after applying this path, the issue is solved.
>
> The patch basically changes the timer server logic to always start
> from the beginning of the list since when a timer is processed, it is
> either removed (one-shot) or reinserted in a different location in the
> list.
> The processing of the list will stop anyhow if all timers that need
> to expire up to "now" are handled.
>
> Signed-off-by: Ronny Meeus <ronny.meeus@gmail.com>
>
> diff --git a/lib/copperplate/timerobj.c b/lib/copperplate/timerobj.c
> index 08cc0d3..96ad2e5 100644
> --- a/lib/copperplate/timerobj.c
> +++ b/lib/copperplate/timerobj.c
> @@ -100,7 +100,7 @@ static void *timerobj_server(void *arg)
> {
> void (*handler)(struct timerobj *tmobj);
> struct timespec now, value, interval;
> - struct timerobj *tmobj, *tmp;
> + struct timerobj *tmobj;
> sigset_t set;
> int sig, ret;
>
> @@ -119,7 +119,9 @@ static void *timerobj_server(void *arg)
>
> __RT(clock_gettime(CLOCK_COPPERPLATE, &now));
>
> - pvlist_for_each_entry_safe(tmobj, tmp, &svtimers, next) {
> + while (!pvlist_empty(&svtimers)) {
> + tmobj = pvlist_first_entry(&svtimers,
> typeof(*tmobj), next);
> +
> value = tmobj->itspec.it_value;
> interval = tmobj->itspec.it_interval;
> handler = tmobj->handler;
>
>
Ack, thanks.
--
Philippe.
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-01-25 15:56 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-01 10:26 race in timerobj Ronny Meeus
2020-12-01 11:06 ` Jan Kiszka
2020-12-01 12:08 ` Ronny Meeus
2020-12-01 13:51 ` Philippe Gerum
2020-12-04 10:41 ` Ronny Meeus
2020-12-04 15:29 ` Philippe Gerum
2021-01-22 7:57 ` Ronny Meeus
2021-01-23 7:40 ` Ronny Meeus
2021-01-25 6:49 ` Jan Kiszka
2021-01-25 14:27 ` Ronny Meeus
2021-01-25 14:36 ` Jan Kiszka
2021-01-25 15:10 ` Ronny Meeus
2021-01-25 15:26 ` Jan Kiszka
2021-01-25 15:56 ` Philippe Gerum
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.