linux-kernel - Re: [PATCH/RFC] signal races/bugs, losing TIF

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 4 Jun 2007 18:44:00 -0700 (PDT)
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Benjamin Herrenschmidt <benh@...nel.crashing.org>
cc:	Linux Kernel list <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Paul Mackerras <paulus@...ba.org>
Subject: Re: [PATCH/RFC] signal races/bugs, losing TIF_SIGPENDING and other
 woes

On Tue, 5 Jun 2007, Benjamin Herrenschmidt wrote:
> 
>  - something calls recalc_sigpending_tsk() on thread A (for example,
>    something try to sends it S2 which is blocked). There is no longer
>    an active signal and thus TIF_SIGPENDING is cleared on thread A

I agree. That's unquestionably a bug. We should *never* clear sigpending 
for somebody else.

I have to say, your patch looks pretty ugly though. It also looks like 
it's rife to be subtly buggy (ie somebody calls "recalc_sigpending_tsk()" 
with "current" and doesn't realize the subtle rule.

So I'd rather just make it more explicit, and simple, and just have 
something really simple like the appended instead..

If we want to be fancier (and avoid the unnecessary compare for the cases 
where we statically know that we're calling it with "t" being "current"), 
we could make it an inline function and factor things out a bit, but I'm 
not sure how worthwhile that really is.

I also wonder if we should just remove the "clear_tsk_thread_flag()" thing 
*entirely*, and do it only in the do_sigal() path. THAT, however, is a 
much bigger change. This one-liner seems the trivial and most obvious 
patch.

Comments?

		Linus

---
 kernel/signal.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index acdfc05..61abd8d 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -105,7 +105,8 @@ static int recalc_sigpending_tsk(struct task_struct *t)
 		set_tsk_thread_flag(t, TIF_SIGPENDING);
 		return 1;
 	}
-	clear_tsk_thread_flag(t, TIF_SIGPENDING);
+	if (t == current)
+		clear_tsk_thread_flag(t, TIF_SIGPENDING);
 	return 0;
 }

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/