linux-kernel - Re: [PATCH 0/1] exit: kill signal_struct->quick

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: <20240610152902.GC20640@redhat.com>
Date: Mon, 10 Jun 2024 17:29:02 +0200
From: Oleg Nesterov <oleg@...hat.com>
To: "Eric W. Biederman" <ebiederm@...ssion.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>, Tejun Heo <tj@...nel.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/1] exit: kill signal_struct->quick_threads

Hi Eric, thanks for looking at this.

Let me answer your questions out-of-order. But, before anything else,
do you see anything wrong in 1/1 ?

On 06/10, Eric W. Biederman wrote:
>
> May I ask which direction you are coming at this from?  Are you trying
> to reduce the cost of do_exit?  Are you interested in untangling the
> mess that is exiting threads in a process?

I am trying to understand why do we need another counter.

And, I'd like to cleanup the usage of task->signal->live, I think it
should be avoided (if possible) when task != current. IIRC, we even
discussed this some time ago but I can't find any reference.

See also another thread about css_task_iter_advance().

> > Eric, I can't understand why the commit ("signal: Guarantee that
> > SIGNAL_GROUP_EXIT is set on process exit") added the new
> > quick_threads counter. And why, if we forget about --quick_threads,
> > synchronize_group_exit() has to take siglock unconditionally.
> > Did I miss something obvious?
>
> At a minimum it is the exact same locking as everywhere else that sets
> signal->flags, signal->group_exit_code, and signal->group_stop_count
> uses.
>
> So it would probably require some significant reason to not use
> the same locking and complicate reasoning about the code.  I suspect
> setting those values without siglock held is likely to lead to
> interesting races.

I guess I was not clear. Of course, SIGNAL_GROUP_EXIT must be always
set under ->siglock. But I think synchronize_group_exit() can just
return if SIGNAL_GROUP_EXIT is already set? If nothing else, this is
what do_group_exit() does.

Or I misunderstood you?

> That is where signal->quick_threads comes from.  In the work it is a
> part of I wind up moving the decrement up much sooner to the point where
> individual threads decide to exit.  The decrement of signal->live comes
> much too late to be useful in that context.

And that is why this patch moves the decrement of signal->live to the
start of do_exit().

> It is also part of me wanting to be able to uniformly use
> SIGNAL_GROUP_EXIT and signal->group_exit_code when talking about the
> process state, and p->exit_code when talking about the per task state.

Agreed,

> At the moment I am staring at wait_task_zombie and trying to understand
> how:
>
> 	status = (p->signal->flags & SIGNAL_GROUP_EXIT)
> 		? p->signal->group_exit_code : p->exit_code;
>
> works without any locks or barriers.

Agreed, at first glance this looks worrying without siglock... I'll try
to take a look, perhaps we can simply kill the SIGNAL_GROUP_EXIT check,
not sure.

But this patch should not make any difference ?

Oleg.