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: <87iky5k2yi.fsf@email.froward.int.ebiederm.org>
Date: Tue, 18 Jun 2024 22:48:21 -0500
From: "Eric W. Biederman" <ebiederm@...ssion.com>
To: Oleg Nesterov <oleg@...hat.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

Oleg Nesterov <oleg@...hat.com> writes:

> On 06/15, Eric W. Biederman wrote:
>>
>> Oleg Nesterov <oleg@...hat.com> writes:
>>
>> > Eric, do you agree with this patch or not?
>>
>> I really don't.
>
> OK, I won't insist.
>
> If nothing else, I failed to remove the usage of signal->live in cgroup.c
> and this patch (supposed to be a cleanup) can slightly affect the cgroup
> iterators.
>
> But. You didn't mention cgroups, so lets forget cgroups to simplify this
> discussion.
>
> So. I would like to see at least ONE _technical_ objection to this patch.
>
> Once again, I was worried about this cleanup from the very beginning, that is
> why I asked you to review. Thanks for taking a look. But could you help me to
> understand what exactly you don't like?
>
>> I think skipping some work if SIGNAL_GROUP_EXIT is already
>> set is not necessarily wrong.
>
> OK, so you seem to agree with this part after all.
>
>> I think we need the quick_threads count,
>
> And so far I fail to understand this,
>
>> and related cleanups.
>> I was hoping to be able to post a patchset with this reply
>> to explain things, but it looks like that is still a couple
>> of days off.
>
> OK. This looks as if I need to wait for your cleanups to understand
> why do we need quick_threads even if we move atomic_dec_and_test(live)
> to the very start of do_exit().
>
> Okay, I'll have to wait.

I am just about to post it.

The patchset should be be good enough to answer what I think needs to
happen with quick_threads.  I don't know if it is good enough to merge
yet, I resurrected, rebased against v6.10-rc3, reviewed and compiled it
but I haven't actually tested the code.

> And. To me the current placement of atomic_dec_and_test(signal->live)
> looks "random" no matter what. Even if we forget about cgroups.

Ah yes sorting out signal->live.

The history shows the placement was one of convenience just putting
it where it should go.

The big issue is the code does not have a piece of cleanup code that
runs once there are no more users of signal_struct.  So we get
``process'' level cleanup all over the place.

The function __exit_signal(tsk) when "thread_group_leader(tsk) == true"
which implies "thread_group_empty(tsk) == true" is about as close as we
get.

I had a patchset where I was working to remove the concept of
thread_group_leaders.  I remember by moving the code between do_exit and
__exit_signal I could make things a lot cleaner.  I would have to dig it
out to see what I was really able to accomplish.

I also remember that the cgroup iterators where somewhat hopeless when
there were zombie thread group leaders.

Eric