Re: [PATCH] kernel: Prevent prctl(PR_SET_PDEATHSIG) from racing with parent process exit

[Christian Brauner <brauner@kernel.org>]

On Tue, Sep 23, 2025 at 03:39:06PM +0200, Mateusz Guzik wrote:
> On Tue, Sep 23, 2025 at 2:05 PM Oleg Nesterov <oleg@redhat.com> wrote:
> > As you correctly pointed out, forget_original_parent/prctl lack the necessary
> > barries. So lets add the barriers instead of abusing tasklist? As for sys_prctl(),
> > I think that ret-to-user-mode + enter-the-kernel-mode should act as a full
> > barrier, so it only needs WRITE_ONCE()...
> >
> 
> So I looked over this and I think I see why you are not eager to fix
> the problem to begin with. ;)
> 
> I agree with reluctance to take tasklist lock to handle
> PR_SET_PDEATHSIG, but I wonder if in practice this is used rarely
> enough that the lock trip would not be a problem? It avoids any
> modifications to the exit codepath.

Unprivileged userspace can now just call that prctl() in a loop to
hammer on tasklist_lock. Already possible with the subreaper stuff but I
think we don't need to add more. At least for the subreaper stuff it has
an actual meaningful use and not just plugs a theoretical problem.

> 
> By barriers I presume you meant smp_mb() between
> RCU_INIT_POINTER(t->real_parent, reaper) and
> READ_ONCE(t->pdeath_signal) in forget_original_parent. That's very
> nasty as the full fence is quite expensive. This could be done with
> just one fence for the entire call by iterating the list twice, but
> that's still preferably avoided.
> 
> > Or perhaps user-space can do something else to sync with the exiting parent
> > instead of using getppid() ?
> >
> 
> I never put any thought concerning this mechanism, I do think it
> nicely showcases the prctl at hand is kind of crap. The non-crap
> version would pass the PID you think your parent is, so that you do
> this race-free. I don't know if makes any sense to add this.

I see that stuff already made it into the tree. I strongly dislike
adding more tasklist_lock usage. All for a theoretical race with limited
impact.

If I'd seen this sooner I would have NAKed this out of existence.

> 
> I'm wondering if the fact that tasklist is write-locked in that code
> path could be utilized to synchronize this in a matter other than
> taking it.
> 
> pseudo-code wise, something like this:
> WRITE_ONCE(me->pdeath_signal, arg2);
> /* publish the above store and load the lock after */
> smb_mb();
> /* here spin waiting until tasklist_lock is not write-locked */
> smb_rmb();
> 
> Unless I'm missing something this should provide the guarantee you see
> the updated parent, if any.
> 
> I don't see a routine to do it though and knowing memory barriers
> there might be some bullshit hiding there making this not work, so not
> my first choice unless someone with more memory barrier clue can chime
> in.