linux-kernel - Re: [PATCH v2] exit: perform randomness and pid work without tasklist

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: <8734gybmxv.fsf@email.froward.int.ebiederm.org>
Date: Fri, 31 Jan 2025 17:09:16 -0600
From: "Eric W. Biederman" <ebiederm@...ssion.com>
To: Mateusz Guzik <mjguzik@...il.com>
Cc: brauner@...nel.org,  oleg@...hat.com,  akpm@...ux-foundation.org,
  linux-mm@...ck.org,  linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] exit: perform randomness and pid work without
 tasklist_lock

Mateusz Guzik <mjguzik@...il.com> writes:

> On Fri, Jan 31, 2025 at 9:56 PM Eric W. Biederman <ebiederm@...ssion.com> wrote:
>> Moving proc_flush_pid inside of tasklist_lock is a bad idea.
>
> The patch does not make such a change though.
>
> The call is still performed without the lock, but it also dodges the
> additional refcount dance (and notably eliminates an atomic from an
> area protected by tasklist_lock).

My mistake I saw you had moved it up, but I had missed just how
far.

It is still a bad idea to move it early, as that has caused problems
with lingering proc entries for reaped task clogging up the dcache.

>> It is wrong that attach_pid/detach_pid can be performed without the
>> tasklist_lock.  There are reasonable guarantees provided by the posix
>> standard that the set of processes sent a signal is the set of
>> processes at a point in time.  The tasklist_lock is how we provide
>> those guarantees currently.
>>
>
> I don't see anything calling these without the lock and neither my
> patch nor a follow up about pids suggest anything of the sort.

No.  You simply said fork called attach_pid without the lock and
your description implied it was safe to call attach_pid/detach_pid
without the lock.

>> There are two more layers to pids.  The pid number allocation of
>> alloc_pid/free_pid, and the struct pid layer maintained by get_pid,
>> put_pid.  Those two layers don't need the tasklist_lock.
>>
>>
>> It is safe to move free_pid out of tasklist_lock.  I am not certain
>> how sane it is.
>>
>
> Where is the sanity problem here? AFAICS this just delays some wakeups
> in the worst case.

At the end of the day it might be a sensible way to go.  I just haven't
found the arguments to convince my gut of that yet.  It is important for
me at least to convince my gut because it usually notices problems
before the rest of me does.

Eric