[PATCH] posix-cpu-timers: don't abuse lock_task_sighand() in handle_posix_cpu_timers()

[PATCH] posix-cpu-timers: don't abuse lock_task_sighand() in handle_posix_cpu_timers()

[Oleg Nesterov]

After the commit f90fff1e152d ("posix-cpu-timers: fix race between
handle_posix_cpu_timers() and posix_cpu_timer_del()"), tsk->sighand
is stable in handle_posix_cpu_timers(), so it can use the plain
spin_lock_irqsave(&tsk->sighand->siglock).

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/time/posix-cpu-timers.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c
index a7d3e8229c4b..d73d31c7994f 100644
--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -1357,8 +1357,11 @@ static void handle_posix_cpu_timers(struct task_struct *tsk)
 	unsigned long flags, start;
 	LIST_HEAD(firing);
 
-	if (!lock_task_sighand(tsk, &flags))
-		return;
+	/*
+	 * tsk is current and ->sighand is stable, see the
+	 * tsk->exit_state check in run_posix_cpu_timers()
+	 */
+	spin_lock_irqsave(&tsk->sighand->siglock, flags);
 
 	do {
 		/*
@@ -1418,7 +1421,7 @@ static void handle_posix_cpu_timers(struct task_struct *tsk)
 	 * that gets the timer lock before we do will give it up and
 	 * spin until we've taken care of that timer below.
 	 */
-	unlock_task_sighand(tsk, &flags);
+	spin_unlock_irqrestore(&tsk->sighand->siglock, flags);
 
 	/*
 	 * Now that all the timers on our list have the firing flag,
-- 
2.52.0

Re: [PATCH] posix-cpu-timers: don't abuse lock_task_sighand() in handle_posix_cpu_timers()

[Bradley Morgan]

hi oleg! I'm new to reviewing, so feel free to criticize.

This patch does LGTM,

Something else I'd like to bring up though, this file has already
regressed once, 0bdd2ed4138e (dropping the exit_state check)

And then f90fff1e152d re adding it.

Would a WARN_ON_ONCE(tsk != current) be worth adding? 

Not a blocker or anything, just a question!

For this patch, 

Reviewed-by: Bradley Morgan <include@grrlz.net>
Thanks!

Re: [PATCH] posix-cpu-timers: don't abuse lock_task_sighand() in handle_posix_cpu_timers()

[Oleg Nesterov]

On 07/04, Bradley Morgan wrote:
>
> Something else I'd like to bring up though, this file has already
> regressed once, 0bdd2ed4138e (dropping the exit_state check)
>
> And then f90fff1e152d re adding it.
>
> Would a WARN_ON_ONCE(tsk != current) be worth adding?

Well may be, up to Thomas...

But note that the problem was not that tsk != current. The problem was
that tsk->sighand could go away after unlock_task_sighand(tsk), even if
tsk == current.

> Reviewed-by: Bradley Morgan <include@grrlz.net>

Thanks,

Oleg.

Re: [PATCH] posix-cpu-timers: don't abuse lock_task_sighand() in handle_posix_cpu_timers()

[Oleg Nesterov]

Hi Bradley,

FYI, this is another private message.

On 07/04, Bradley Morgan wrote:
>
> hi oleg! I'm new to reviewing, so feel free to criticize.

This is fine...

But if you want to have a "real" practice, you can try to review V2 from Eric:

	[PATCH v2 00/14] Short circuit delivery for coredump signals
	https://lore.kernel.org/all/877bnb4uyw.fsf_-_@email.froward.int.ebiederm.org/

I won't be able to take a look until the middle of the next week.

> Would a WARN_ON_ONCE(tsk != current) be worth adding?

See my reply on lkml. But since this message is private...

IOW. tsk->sighand must be stable here or we have more problems. Without this
patch the "if (!lock_task_sighand(tsk, &flags))" check looks as if we can handle
the case were ->sighand is not stable, but this is not true.

And. This patch is trivial, but it connects to other (under discussion, nontrivial)
changes related to the wrong usage of lock_task_sighand() in posix-cpu-timers.c.

> For this patch,
>
> Reviewed-by: Bradley Morgan <include@grrlz.net>

Thanks again,

Oleg.

Re: [PATCH] posix-cpu-timers: don't abuse lock_task_sighand() in handle_posix_cpu_timers()

[Bradley Morgan]

On July 4, 2026 6:05:20 PM GMT+01:00, Oleg Nesterov <oleg@redhat.com>
wrote:
>Hi Bradley,
>
>FYI, this is another private message.

It's not.


>On 07/04, Bradley Morgan wrote:
>>
>> hi oleg! I'm new to reviewing, so feel free to criticize.
>
>This is fine...
>
>But if you want to have a "real" practice, you can try to review V2 from
>Eric:
>
>	[PATCH v2 00/14] Short circuit delivery for coredump signals
>	https://lore.kernel.org/all/877bnb4uyw.fsf_-_@email.froward.int.ebiederm.org/
>
>I won't be able to take a look until the middle of the next week.
>
>> Would a WARN_ON_ONCE(tsk != current) be worth adding?
>
>See my reply on lkml. But since this message is private...
>
>IOW. tsk->sighand must be stable here or we have more problems. Without
>this
>patch the "if (!lock_task_sighand(tsk, &flags))" check looks as if we can
>handle
>the case were ->sighand is not stable, but this is not true.
>
>And. This patch is trivial, but it connects to other (under discussion,
>nontrivial)
>changes related to the wrong usage of lock_task_sighand() in
>posix-cpu-timers.c.
>
>> For this patch,
>>
>> Reviewed-by: Bradley Morgan <include@grrlz.net>
>
>Thanks again,
>
>Oleg.
>
>

Thanks!

Re: [PATCH] posix-cpu-timers: don't abuse lock_task_sighand() in handle_posix_cpu_timers()

[Oleg Nesterov]

On 07/04, Oleg Nesterov wrote:
>
> Hi Bradley,
>
> FYI, this is another private message.

Damn ;)

FYI, it wasn't private. I've pressed 'g' by mistake.

Oleg.

> On 07/04, Bradley Morgan wrote:
> >
> > hi oleg! I'm new to reviewing, so feel free to criticize.
> 
> This is fine...
> 
> But if you want to have a "real" practice, you can try to review V2 from Eric:
> 
> 	[PATCH v2 00/14] Short circuit delivery for coredump signals
> 	https://lore.kernel.org/all/877bnb4uyw.fsf_-_@email.froward.int.ebiederm.org/
> 
> I won't be able to take a look until the middle of the next week.
> 
> > Would a WARN_ON_ONCE(tsk != current) be worth adding?
> 
> See my reply on lkml. But since this message is private...
> 
> IOW. tsk->sighand must be stable here or we have more problems. Without this
> patch the "if (!lock_task_sighand(tsk, &flags))" check looks as if we can handle
> the case were ->sighand is not stable, but this is not true.
> 
> And. This patch is trivial, but it connects to other (under discussion, nontrivial)
> changes related to the wrong usage of lock_task_sighand() in posix-cpu-timers.c.
> 
> > For this patch,
> >
> > Reviewed-by: Bradley Morgan <include@grrlz.net>
> 
> Thanks again,
> 
> Oleg.

Re: [PATCH] posix-cpu-timers: don't abuse lock_task_sighand() in handle_posix_cpu_timers()

[Bradley Morgan]

On July 4, 2026 6:05:20 PM GMT+01:00, Oleg Nesterov <oleg@redhat.com>
wrote:
>Hi Bradley,
>
>FYI, this is another private message.
>
>On 07/04, Bradley Morgan wrote:
>>
>> hi oleg! I'm new to reviewing, so feel free to criticize.
>
>This is fine...
>
>But if you want to have a "real" practice, you can try to review V2 from
>Eric:
>
>	[PATCH v2 00/14] Short circuit delivery for coredump signals
>	https://lore.kernel.org/all/877bnb4uyw.fsf_-_@email.froward.int.ebiederm.org/

If you CC me, I'll review anything! :)


>I won't be able to take a look until the middle of the next week.
>
>> Would a WARN_ON_ONCE(tsk != current) be worth adding?
>
>See my reply on lkml. But since this message is private...
>
>IOW. tsk->sighand must be stable here or we have more problems. Without
>this
>patch the "if (!lock_task_sighand(tsk, &flags))" check looks as if we can
>handle
>the case were ->sighand is not stable, but this is not true.
>
>And. This patch is trivial, but it connects to other (under discussion,
>nontrivial)
>changes related to the wrong usage of lock_task_sighand() in
>posix-cpu-timers.c.
>
>> For this patch,
>>
>> Reviewed-by: Bradley Morgan <include@grrlz.net>
>
>Thanks again,
>
>Oleg.
>
>

Thanks!

[tip: timers/core] posix-cpu-timers: Don't abuse lock_task_sighand() in handle_posix_cpu_timers()

[tip-bot2 for Oleg Nesterov]

The following commit has been merged into the timers/core branch of tip:

Commit-ID:     a73d7f98e41a96d6e1bcb0e731ab185d9d67878e
Gitweb:        https://git.kernel.org/tip/a73d7f98e41a96d6e1bcb0e731ab185d9d67878e
Author:        Oleg Nesterov <oleg@redhat.com>
AuthorDate:    Sat, 04 Jul 2026 13:09:36 +02:00
Committer:     Thomas Gleixner <tglx@kernel.org>
CommitterDate: Sun, 05 Jul 2026 12:15:22 +02:00

posix-cpu-timers: Don't abuse lock_task_sighand() in handle_posix_cpu_timers()

After commit f90fff1e152d ("posix-cpu-timers: fix race between
handle_posix_cpu_timers() and posix_cpu_timer_del()"), tsk->sighand is
stable in handle_posix_cpu_timers(), so it can use the plain
spin_lock_irqsave(&tsk->sighand->siglock).

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Thomas Gleixner <tglx@kernel.org>
Reviewed-by: Bradley Morgan <include@grrlz.net>
Link: https://patch.msgid.link/akjp8AGpY8eJG5I1@redhat.com
---
 kernel/time/posix-cpu-timers.c |  9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c
index 5e633d8..c8a9b52 100644
--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -1300,8 +1300,11 @@ static void handle_posix_cpu_timers(struct task_struct *tsk)
 	unsigned long flags, start;
 	LIST_HEAD(firing);
 
-	if (!lock_task_sighand(tsk, &flags))
-		return;
+	/*
+	 * tsk is current and ->sighand is stable, see the
+	 * tsk->exit_state check in run_posix_cpu_timers()
+	 */
+	spin_lock_irqsave(&tsk->sighand->siglock, flags);
 
 	do {
 		/*
@@ -1361,7 +1364,7 @@ static void handle_posix_cpu_timers(struct task_struct *tsk)
 	 * that gets the timer lock before we do will give it up and
 	 * spin until we've taken care of that timer below.
 	 */
-	unlock_task_sighand(tsk, &flags);
+	spin_unlock_irqrestore(&tsk->sighand->siglock, flags);
 
 	/*
 	 * Now that all the timers on our list have the firing flag,