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] [day] [month] [year] [list]

Message-ID: <CAGudoHGG6HRP5Cs4p0bHKApt1nTQFjX105Y5J=xmMUzybiCU+Q@mail.gmail.com>
Date: Sat, 1 Feb 2025 15:03:52 +0100
From: Mateusz Guzik <mjguzik@...il.com>
To: "Eric W. Biederman" <ebiederm@...ssion.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

On Sat, Feb 1, 2025 at 12:09 AM Eric W. Biederman <ebiederm@...ssion.com> wrote:
>
> 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.
>

I would argue the time window to find the about-to-be-whacked task is
not big, but this part is not important enough for me to push for it.
So I'm going to drop this bit for now.

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

Huh, indeed that's how it reads like. That's very poorly stated at best, my bad.

The key was *allocating* a pid happens without the tasklist_lock (but
with pidmap_lock) and the part which gets rid of it (detach_pid ->
free_pid) operates under both.

As you can see the patch keeps detach_pid inside the
tasklist_lock-protected area.

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

There is definitely no rush.

I'm going to cook v3 if only just for fun.
-- 
Mateusz Guzik <mjguzik gmail.com>