linux-kernel - Re: [PATCH 0/2] introduce __next_thread(), change next

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: <87y1hzs2e4.fsf@email.froward.int.ebiederm.org>
Date:   Fri, 25 Aug 2023 08:00:19 -0500
From:   "Eric W. Biederman" <ebiederm@...ssion.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Oleg Nesterov <oleg@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>, peterz@...hat.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/2] introduce __next_thread(), change next_thread()

Linus Torvalds <torvalds@...ux-foundation.org> writes:

> On Thu, 24 Aug 2023 at 07:32, Oleg Nesterov <oleg@...hat.com> wrote:
>>
>> After document-while_each_thread-change-first_tid-to-use-for_each_thread.patch
>> in mm tree + this series
>
> Looking at your patch 2/2, I started looking at users ("Maybe we
> *want* NULL for the end case, and make next_thread() and __next_thread
> be the same?").
>
> One of the main users is while_each_thread(), which certainly wants
> that NULL case, both for an easier loop condition, but also because
> the only user that uses the 't' pointer after the loop is
> fs/proc/base.c, which wants it to be NULL.

Sort of.

I have found 3 loops that want to loop through all of the threads of
a process starting with the current thread. 

The loop in do_wait.
The loop finding the thread to signal in complete_signal.
The loop in retarget_shared_pending finding which threads
to wake up.

For the signal case that is just quality of implementation,
and starting somewhere else would just decrease that quality.

For the loop in do_wait it is a correctness issue that the code
starts first with the threads own children before looking for
children of other threads.

There are 4 users of next_thread outside of while_each_thread.
- next_tid -- wants NULL
- task_group_seq_get_next -- same as next_tid
- __exit_signal -- wants any thread on the list after __unhash_process
- complete_signal -- wants the same loop as do_wait.

> And kernel/bpf/task_iter.c seems to *expect* NULL at the end?
>
> End result: if you're changing next_thread() anyway, please just
> change it to be a completely new thing that returns NULL at the end,
> which is what everybody really seems to want, and don't add a new
> __next_thread() helper. Ok?

So I would say Oleg please build the helper that do_wait wants
and use it in do_wait, complete_signal, and retarget_shared_pending.

Change the rest of the loops can use for_each_thread (skipping
the current task if needed) or for_each_process_thread.

Change __exit_signal to use signal->group_leader instead of next_thread.

Change next_thread to be your __next_thread, and update the 2 callers
appropriately.

Eric