posix timer freeze after some random time, under pthread create/destroy load

posix timer freeze after some random time, under pthread create/destroy load

[Anthony Mallet]

[-- Attachment #1: message body text --]
[-- Type: text/plain, Size: 1818 bytes --]

Hi,

I'm facing an issue with posix timers configured to send SIGALRM
signal upon expiry. The symptom is that the timer randomly freezes
(the signal handler not triggered anymore). After analysis, this happens
in combination with pthreads creation / destruction.

I have attached a test case that can reliably reproduce my issue on
affected kernels. It involves creating a timer that increments a
global counter at each tick, while the main thread is spawning and
destroying other threads. At some point, the counter gets stalled. In
the context of this test case, I do heavy thread creation and
destruction, so that the issue triggers almost immediately. Regarding
the real-world issue, it happens in the context of aio(7) work, which
also involves thread creation and destruction but presumably at a much
lower rate, and the issue consequently triggers much less often.

I could reproduce the issue reliably with mainline kernels from 6.4
to 6.11 (included), and on several distributions, different hardware
and glibc versions. Kernels earlier than 6.3 (included) do not exhibit
the problem at all.

Once the issue triggers, simply resetting the timer (with
timer_settime(2)) makes it work again, until next
stall. timer_gettime(2) does not show garbage and the values are still
as expected. Only the signal handler is not called. Manually sending
SIGALRM with raise(SIGALRM) also works and invokes the signal handler
as expected.

Also note that using setitimer(2) instead of a posix timer does not
show any problem with the same test program.

Before filling a proper bug report, I wanted to have your opinion
about this. This e-mail is already probably too long for an
introduction, but I can of course provide you with any missing detail
that you would deem necessary.

Thanks for you attention,
Anthony Mallet


[-- Attachment #2: timer.c --]
[-- Type: text/plain, Size: 2906 bytes --]

/* Public domain - Anthony Mallet on Mon Nov  4 2024 */

#include <err.h>
#include <errno.h>
#include <pthread.h>
#include <signal.h>
#include <stdint.h>
#include <stdio.h>
#include <time.h>

static volatile int ticks;

/* SIGALRM handler */
void
tick(int arg)
{
  (void)arg; /* unused */

  /* global counter - even if access is not atomic, we don't care here as the
   * exact value is not used, only the fact that the value changes is relevant
   */
  ticks++;
}

/* thread forking thread */
void *
thr(void *arg)
{
  pthread_attr_t attr;
  pthread_t t;
  (void)arg; /* unused */

  /* spawn a new thread in detached state so that we don't grow too much */
  pthread_attr_init(&attr);
  pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);
  if (pthread_create(&t, &attr, thr, NULL))
    err(2, "pthread_create");

  return NULL;
}

int
main()
{
  int hz = 1000; /* 1kHz timer - the higher, the faster the issue happens */

  struct sigaction act;
  struct itimerspec tv;
  struct timespec pts, ts, rem;
  sigset_t sigset;
  timer_t timer;
  int i, c1, c2;

  /* SIGALRM handler */
  act.sa_handler = tick;
  sigemptyset(&act.sa_mask);
  act.sa_flags = 0;
  if (sigaction(SIGALRM, &act, NULL) == -1)
    err(2, "sigaction");

  sigemptyset(&sigset);
  sigaddset(&sigset, SIGALRM);
  if (pthread_sigmask(SIG_UNBLOCK, &sigset, NULL) == -1)
    err(2, "pthread_sigmask");


  /* SIGALRM timer at 'hz' frequency */
  if (timer_create(CLOCK_REALTIME, NULL, &timer) == -1)
    err(2, "timer_create");

  tv.it_interval.tv_nsec = 1000000000/hz;
  tv.it_interval.tv_sec = 0;
  tv.it_value = tv.it_interval;


  /* thread forking threads - this is an issue spotted on ubuntu-22.04 and
   * 24.04, as well as other distributions, that affects timer signal
   * delivrery. This seems to affect kernels from 6.4 to 6.11 inclusive. */
  thr(NULL);


  /* start timer */
  if (timer_settime(timer, 0, &tv, NULL) == -1)
    err(2, "timer_settime");

  /* 100 periods delay */
  pts.tv_sec = 0;
  pts.tv_nsec = tv.it_interval.tv_nsec * 100; /* 100ms */
  while(pts.tv_nsec >= 1000000000) {
    pts.tv_nsec -= 1000000000;
    pts.tv_sec++;
  }
  /* for 1s */
  for (i = 0; i < 10; i++) {
    ts = pts;
    c1 = ticks;
    while (nanosleep(&ts, &rem) != 0 && errno == EINTR) ts = rem;
    c2 = ticks;

    if (c1 == c2) {
      /* the counter is stuck, SIGALRM not firing anymore */
      fprintf(stderr, "SIGALRM issue after %d ticks\n", c1);

      /* just resetting the timer at this point makes it work again: */
      /* timer_settime(timer, 0, &tv, NULL); */
      /* (the issue will trigger again after some time) */

      /* also note that timer_gettime(timer, &tv) will show both correct
       * tv.it_interval and tv.it_value changing normally */

      /* manually sending SIGALRM also still works: */
      /* raise(SIGALRM); */

      return 2;
    }
  }

  printf("OK, no issue\n");
  return 0;
}

Re: posix timer freeze after some random time, under pthread create/destroy load

[Frederic Weisbecker]

Hi Anthony,

Le Wed, Nov 06, 2024 at 10:29:44PM +0100, Anthony Mallet a écrit :
> Hi,
> 
> I'm facing an issue with posix timers configured to send SIGALRM
> signal upon expiry. The symptom is that the timer randomly freezes
> (the signal handler not triggered anymore). After analysis, this happens
> in combination with pthreads creation / destruction.
> 
> I have attached a test case that can reliably reproduce my issue on
> affected kernels. It involves creating a timer that increments a
> global counter at each tick, while the main thread is spawning and
> destroying other threads. At some point, the counter gets stalled. In
> the context of this test case, I do heavy thread creation and
> destruction, so that the issue triggers almost immediately. Regarding
> the real-world issue, it happens in the context of aio(7) work, which
> also involves thread creation and destruction but presumably at a much
> lower rate, and the issue consequently triggers much less often.
> 
> I could reproduce the issue reliably with mainline kernels from 6.4
> to 6.11 (included), and on several distributions, different hardware
> and glibc versions. Kernels earlier than 6.3 (included) do not exhibit
> the problem at all.
> 
> Once the issue triggers, simply resetting the timer (with
> timer_settime(2)) makes it work again, until next
> stall. timer_gettime(2) does not show garbage and the values are still
> as expected. Only the signal handler is not called. Manually sending
> SIGALRM with raise(SIGALRM) also works and invokes the signal handler
> as expected.
> 
> Also note that using setitimer(2) instead of a posix timer does not
> show any problem with the same test program.
> 
> Before filling a proper bug report, I wanted to have your opinion
> about this. This e-mail is already probably too long for an
> introduction, but I can of course provide you with any missing detail
> that you would deem necessary.
> 
> Thanks for you attention,
> Anthony Mallet

Thanks a lot for your report and the very helpful reliable reproducer.

I think this started with commit:

bcb7ee79029d (posix-timers: Prefer delivery of signals to the current thread)

The problem is that if the current task is exiting and has already been reaped,
its sighand pointer isn't there anymore. And so the signal is ignored even
though it should be queued to and handled by the thread group that has other
live threads to take care of it.

Can you test the following patch? I'm cooking another patch with changelog for
upstream that has seen recent changes in this area.

diff --git a/kernel/signal.c b/kernel/signal.c
index 8f6330f0e9ca..4cadee618d4b 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1984,7 +1984,8 @@ int send_sigqueue(struct sigqueue *q, struct pid *pid, enum pid_type type)
 	t = pid_task(pid, type);
 	if (!t)
 		goto ret;
-	if (type != PIDTYPE_PID && same_thread_group(t, current))
+	if (type != PIDTYPE_PID && same_thread_group(t, current) &&
+	    !(current->flags & PF_EXITING))
 		t = current;
 	if (!likely(lock_task_sighand(t, &flags)))
 		goto ret;

Re: posix timer freeze after some random time, under pthread create/destroy load

[Oleg Nesterov]

On 11/21, Frederic Weisbecker wrote:
>
> I think this started with commit:
>
> bcb7ee79029d (posix-timers: Prefer delivery of signals to the current thread)
>
> The problem is that if the current task is exiting and has already been reaped,
> its sighand pointer isn't there anymore.

Thanks...

This can only happen if the exiting task has already passed exit_notify() which
sets exit_state. So I'd suggest to check current->exit_state instead of PF_EXITING
in the patch below.

Oleg.

> And so the signal is ignored even
> though it should be queued to and handled by the thread group that has other
> live threads to take care of it.
>
> Can you test the following patch? I'm cooking another patch with changelog for
> upstream that has seen recent changes in this area.
>
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 8f6330f0e9ca..4cadee618d4b 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -1984,7 +1984,8 @@ int send_sigqueue(struct sigqueue *q, struct pid *pid, enum pid_type type)
>  	t = pid_task(pid, type);
>  	if (!t)
>  		goto ret;
> -	if (type != PIDTYPE_PID && same_thread_group(t, current))
> +	if (type != PIDTYPE_PID && same_thread_group(t, current) &&
> +	    !(current->flags & PF_EXITING))
>  		t = current;
>  	if (!likely(lock_task_sighand(t, &flags)))
>  		goto ret;
>
>

Re: posix timer freeze after some random time, under pthread create/destroy load

[Frederic Weisbecker]

Le Fri, Nov 22, 2024 at 09:24:07AM +0100, Oleg Nesterov a écrit :
> On 11/21, Frederic Weisbecker wrote:
> >
> > I think this started with commit:
> >
> > bcb7ee79029d (posix-timers: Prefer delivery of signals to the current thread)
> >
> > The problem is that if the current task is exiting and has already been reaped,
> > its sighand pointer isn't there anymore.
> 
> Thanks...
> 
> This can only happen if the exiting task has already passed exit_notify() which
> sets exit_state. So I'd suggest to check current->exit_state instead of PF_EXITING
> in the patch below.
> 
> Oleg.

Right, I don't mind either way, though if it's past PF_EXITING,
complete_signal() -> wants_signal() will defer to another thread anyway, right?
Due to retarget_shared_pending() being called after the flag being set...

Thanks.

> 
> > And so the signal is ignored even
> > though it should be queued to and handled by the thread group that has other
> > live threads to take care of it.
> >
> > Can you test the following patch? I'm cooking another patch with changelog for
> > upstream that has seen recent changes in this area.
> >
> > diff --git a/kernel/signal.c b/kernel/signal.c
> > index 8f6330f0e9ca..4cadee618d4b 100644
> > --- a/kernel/signal.c
> > +++ b/kernel/signal.c
> > @@ -1984,7 +1984,8 @@ int send_sigqueue(struct sigqueue *q, struct pid *pid, enum pid_type type)
> >  	t = pid_task(pid, type);
> >  	if (!t)
> >  		goto ret;
> > -	if (type != PIDTYPE_PID && same_thread_group(t, current))
> > +	if (type != PIDTYPE_PID && same_thread_group(t, current) &&
> > +	    !(current->flags & PF_EXITING))
> >  		t = current;
> >  	if (!likely(lock_task_sighand(t, &flags)))
> >  		goto ret;
> >
> >
> 

Re: posix timer freeze after some random time, under pthread create/destroy load

[Oleg Nesterov]

On 11/22, Frederic Weisbecker wrote:
>
> Le Fri, Nov 22, 2024 at 09:24:07AM +0100, Oleg Nesterov a écrit :
> > On 11/21, Frederic Weisbecker wrote:
> > >
> > > I think this started with commit:
> > >
> > > bcb7ee79029d (posix-timers: Prefer delivery of signals to the current thread)
> > >
> > > The problem is that if the current task is exiting and has already been reaped,
> > > its sighand pointer isn't there anymore.
> >
> > Thanks...
> >
> > This can only happen if the exiting task has already passed exit_notify() which
> > sets exit_state. So I'd suggest to check current->exit_state instead of PF_EXITING
> > in the patch below.
> >
> > Oleg.
>
> Right, I don't mind either way,

Me too, so feel free to ignore,

> though if it's past PF_EXITING,
> complete_signal() -> wants_signal() will defer to another thread anyway, right?

Right. So I think it would be better to rely on complete_signal() in this
case even if the current logic is very simple and dumb.

> Due to retarget_shared_pending() being called after the flag being set...

Yes. Whatever we do send_sigqueue/complete_signal can choose an exiting thread
which doesn't have PF_EXITING yet, in this case retarget_shared_pending() from
that thread will pick another target for signal_wake_up/TIF_SIGPENDING.

Thanks!

Oleg.

Re: posix timer freeze after some random time, under pthread create/destroy load

[Frederic Weisbecker]

Le Fri, Nov 22, 2024 at 12:49:50PM +0100, Oleg Nesterov a écrit :
> On 11/22, Frederic Weisbecker wrote:
> >
> > Le Fri, Nov 22, 2024 at 09:24:07AM +0100, Oleg Nesterov a écrit :
> > > On 11/21, Frederic Weisbecker wrote:
> > > >
> > > > I think this started with commit:
> > > >
> > > > bcb7ee79029d (posix-timers: Prefer delivery of signals to the current thread)
> > > >
> > > > The problem is that if the current task is exiting and has already been reaped,
> > > > its sighand pointer isn't there anymore.
> > >
> > > Thanks...
> > >
> > > This can only happen if the exiting task has already passed exit_notify() which
> > > sets exit_state. So I'd suggest to check current->exit_state instead of PF_EXITING
> > > in the patch below.
> > >
> > > Oleg.
> >
> > Right, I don't mind either way,
> 
> Me too, so feel free to ignore,
> 
> > though if it's past PF_EXITING,
> > complete_signal() -> wants_signal() will defer to another thread anyway, right?
> 
> Right. So I think it would be better to rely on complete_signal() in this
> case even if the current logic is very simple and dumb.

Just to make sure I understand correctly, this means you'd prefer to keep
the PF_EXITING test?

> 
> > Due to retarget_shared_pending() being called after the flag being set...
> 
> Yes. Whatever we do send_sigqueue/complete_signal can choose an exiting thread
> which doesn't have PF_EXITING yet, in this case retarget_shared_pending() from
> that thread will pick another target for signal_wake_up/TIF_SIGPENDING.

Right.

Thanks.

> 
> Thanks!
> 
> Oleg.
> 

Re: posix timer freeze after some random time, under pthread create/destroy load

[Oleg Nesterov]

On 11/22, Frederic Weisbecker wrote:
>
> Le Fri, Nov 22, 2024 at 12:49:50PM +0100, Oleg Nesterov a écrit :
> > On 11/22, Frederic Weisbecker wrote:
> > >
> > >
> > > Right, I don't mind either way,
> >
> > Me too, so feel free to ignore,
> >
> > > though if it's past PF_EXITING,
> > > complete_signal() -> wants_signal() will defer to another thread anyway, right?
> >
> > Right. So I think it would be better to rely on complete_signal() in this
> > case even if the current logic is very simple and dumb.
>
> Just to make sure I understand correctly, this means you'd prefer to keep
> the PF_EXITING test?

No, sorry for confusion ;)

I'd prefer to check t->exit_state in send_sigqueue() and let complete_signal()
pick another thread if "t->flags & PF_EXITING" is already set.

But I am fine either way, up to you.

I guess we can even avoid the additional check altogether, something like below.
Again, up to you. Your approach looks simpler and doesn't need more comments.

Oleg.

--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1966,7 +1966,7 @@ int send_sigqueue(struct sigqueue *q, struct pid *pid, enum pid_type type)
 {
 	int sig = q->info.si_signo;
 	struct sigpending *pending;
-	struct task_struct *t;
+	struct task_struct *g, *t;
 	unsigned long flags;
 	int ret, result;
 
@@ -1989,12 +1989,12 @@ int send_sigqueue(struct sigqueue *q, struct pid *pid, enum pid_type type)
 	 * the same thread group as the target process, which avoids
 	 * unnecessarily waking up a potentially idle task.
 	 */
-	t = pid_task(pid, type);
-	if (!t)
+	g = t = pid_task(pid, type);
+	if (!g)
 		goto ret;
 	if (type != PIDTYPE_PID && same_thread_group(t, current))
 		t = current;
-	if (!likely(lock_task_sighand(t, &flags)))
+	if (!likely(lock_task_sighand(g, &flags)))
 		goto ret;
 
 	ret = 1; /* the signal is ignored */
@@ -2022,7 +2022,7 @@ int send_sigqueue(struct sigqueue *q, struct pid *pid, enum pid_type type)
 	result = TRACE_SIGNAL_DELIVERED;
 out:
 	trace_signal_generate(sig, &q->info, t, type != PIDTYPE_PID, result);
-	unlock_task_sighand(t, &flags);
+	unlock_task_sighand(g, &flags);
 ret:
 	rcu_read_unlock();
 	return ret;

Re: posix timer freeze after some random time, under pthread create/destroy load

[Frederic Weisbecker]

Le Fri, Nov 22, 2024 at 01:38:17PM +0100, Oleg Nesterov a écrit :
> On 11/22, Frederic Weisbecker wrote:
> >
> > Le Fri, Nov 22, 2024 at 12:49:50PM +0100, Oleg Nesterov a écrit :
> > > On 11/22, Frederic Weisbecker wrote:
> > > >
> > > >
> > > > Right, I don't mind either way,
> > >
> > > Me too, so feel free to ignore,
> > >
> > > > though if it's past PF_EXITING,
> > > > complete_signal() -> wants_signal() will defer to another thread anyway, right?
> > >
> > > Right. So I think it would be better to rely on complete_signal() in this
> > > case even if the current logic is very simple and dumb.
> >
> > Just to make sure I understand correctly, this means you'd prefer to keep
> > the PF_EXITING test?
> 
> No, sorry for confusion ;)
> 
> I'd prefer to check t->exit_state in send_sigqueue() and let complete_signal()
> pick another thread if "t->flags & PF_EXITING" is already set.
> 
> But I am fine either way, up to you.

Ok I'm good with t->exit_state, I'm cooking that.

> 
> I guess we can even avoid the additional check altogether, something like below.
> Again, up to you. Your approach looks simpler and doesn't need more comments.
> 
> Oleg.
> 
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -1966,7 +1966,7 @@ int send_sigqueue(struct sigqueue *q, struct pid *pid, enum pid_type type)
>  {
>  	int sig = q->info.si_signo;
>  	struct sigpending *pending;
> -	struct task_struct *t;
> +	struct task_struct *g, *t;
>  	unsigned long flags;
>  	int ret, result;
>  
> @@ -1989,12 +1989,12 @@ int send_sigqueue(struct sigqueue *q, struct pid *pid, enum pid_type type)
>  	 * the same thread group as the target process, which avoids
>  	 * unnecessarily waking up a potentially idle task.
>  	 */
> -	t = pid_task(pid, type);
> -	if (!t)
> +	g = t = pid_task(pid, type);
> +	if (!g)
>  		goto ret;
>  	if (type != PIDTYPE_PID && same_thread_group(t, current))
>  		t = current;
> -	if (!likely(lock_task_sighand(t, &flags)))
> +	if (!likely(lock_task_sighand(g, &flags)))
>  		goto ret;
>  
>  	ret = 1; /* the signal is ignored */
> @@ -2022,7 +2022,7 @@ int send_sigqueue(struct sigqueue *q, struct pid *pid, enum pid_type type)
>  	result = TRACE_SIGNAL_DELIVERED;
>  out:
>  	trace_signal_generate(sig, &q->info, t, type != PIDTYPE_PID, result);
> -	unlock_task_sighand(t, &flags);
> +	unlock_task_sighand(g, &flags);
>  ret:
>  	rcu_read_unlock();
>  	return ret;
> 

That's nice! But with the recent changes in this area, the target pick-up
logic has moved to a separate function posixtimer_get_target() which makes
this trick a bit more difficult.

I'll stick to exit_state for now.

Thanks a lot for your insight!