linux-kernel - Re: [RFC][PATCH 2/5] signals: Introduce per-thread siglock and action rwlock

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]

Message-ID: <20110418164513.GA25930@redhat.com>
Date:	Mon, 18 Apr 2011 18:45:13 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Matt Fleming <matt@...sole-pimps.org>
Cc:	Tejun Heo <tj@...nel.org>, linux-kernel@...r.kernel.org,
	Thomas Gleixner <tglx@...utronix.de>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	"H. Peter Anvin" <hpa@...or.com>,
	Matt Fleming <matt.fleming@...ux.intel.com>,
	Chris Metcalf <cmetcalf@...era.com>
Subject: Re: [RFC][PATCH 2/5] signals: Introduce per-thread siglock and
	action rwlock

On 04/16, Matt Fleming wrote:
>
> On Thu, 14 Apr 2011 21:00:12 +0200
> Oleg Nesterov <oleg@...hat.com> wrote:
>
> > > Is there a reason that a short-term reference counter isn't used to
> > > prevent this, instead of taking the siglock?
> >
> > Well, sighand->count is the reference counter. The problem is, ->sighand
> > is not per-process, we can share it with abother CLONE_SIGHAND process
> > and de_thread() can change ->sighand during exec.
>
> What I meant was, a reference to say "You can't change/free ->sighand
> because I'm reading/modifying it". So you'd have two new functions,
> get_sighand() and put_sighand(), which would protect the sighand from
> changing while you were looking at it. Obviously, you'd still need to
> see if sighand = NULL, but you wouldn't need to grab the shared
> siglock.
>
> Note how this is different from sighand->count. sighand->count is a
> much longer term reference which stops it being freed while a task is
> using it, kinda like a "Don't free _MY_ sighand" reference, whereas
> what I'm talking about is a "I'm touching YOUR sighand, so don't
> change/free it" reference, e.g. a short term ref for when we're
> operating on a target task. It could be that the two references can
> really be just one atomic_t, I would have to write the code to figure
> that out.

Can't understand...

OK, someone does get_sighand(). Now, what de_thread() should do if it
wants to change ->sighand?

And I don't really understand the point. You can read *sighand lockless.
But you need some per-CLONE_SIGHAND lock if you want to modify it anyway.

> Now, at the moment that suggestion just seems like needless overhead
> because siglock already provides the features we want. But, my problem
> with siglock is,
>
> 	1. It needs to be acquired to stop a task passing through
> 	__exit_signal().
>
> 	2. It protects bits of signal_struct and that struct is getting
> 	pretty bloated and siglock is being used to protect lots of
> 	different things.

Yes, this is the main problem: it is overused.

We need the better locking. Honestly, _personally_ I do not really care
about scalability (but perhaps I should) when it comes to signals, but
there are other problems. And, apart from the already mentioned problems
with signals-from-irq, I think the main problem is tasklist_lock in
do_wait/exit/etc pathes.

And we still have the problems with signals which should be fixed.
de_thread() can miss a signal, vfork() should be interruptible,
do_coredump() should be interruptible. But first of all we need to
define better the behaviour of explicit SIGKILL and what it means
after exit_signals(). This should be fixed first, I think.

> 	3. I suspect most people find the rules of ->sighand pretty
> 	confusing. Just look at
>
> 		arch/tile/kernel/hardwall.c:do_hardwall_trap()
>
> 	the use of siglock there looks buggy to me.

Indeed, I agree. It shouldn't use __group_send_sig_info() at all.
I'll send the patch. Nobody outside of signal code should play with
->sighand, this is almost always wrong.

There is another problem, historically we have a lot, a lot of send-signal
helpers, but you can never find the right one. And the naming sucks.

> Do you have any recollection of the cleanups? signal_struct needs to be
> put on a diet for sure.

I was going to remove ->sighand from fs/proc first, probably I should
try to resend these patches... Then we should remove the "sighand != NULL"
checks, we need the new helper, and btw it should be used instead of
pid_alive(). Then something else... boring ;)

Oleg.

--
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/