linux-kernel - Re: [RFC 1/3] pidfd: allow pidfd_open() on non-thread-group leaders

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: <ZXN2u2oJl1Z6FTqt@tycho.pizza>
Date:   Fri, 8 Dec 2023 13:04:11 -0700
From:   Tycho Andersen <tycho@...ho.pizza>
To:     Christian Brauner <brauner@...nel.org>
Cc:     Oleg Nesterov <oleg@...hat.com>,
        "Eric W . Biederman" <ebiederm@...ssion.com>,
        linux-kernel@...r.kernel.org, linux-api@...r.kernel.org,
        Tycho Andersen <tandersen@...flix.com>,
        Jan Kara <jack@...e.cz>, linux-fsdevel@...r.kernel.org,
        Joel Fernandes <joel@...lfernandes.org>
Subject: Re: [RFC 1/3] pidfd: allow pidfd_open() on non-thread-group leaders

On Thu, Dec 07, 2023 at 10:25:09PM +0100, Christian Brauner wrote:
> > If these concerns are correct
> 
> So, ok. I misremebered this. The scenario I had been thinking of is
> basically the following.
> 
> We have a thread-group with thread-group leader 1234 and a thread with
> 4567 in that thread-group. Assume current thread-group leader is tsk1
> and the non-thread-group leader is tsk2. tsk1 uses struct pid *tg_pid
> and tsk2 uses struct pid *t_pid. The struct pids look like this after
> creation of both thread-group leader tsk1 and thread tsk2:
> 
> 	TGID 1234				TID 4567 
> 	tg_pid[PIDTYPE_PID]  = tsk1		t_pid[PIDTYPE_PID]  = tsk2
> 	tg_pid[PIDTYPE_TGID] = tsk1		t_pid[PIDTYPE_TGID] = NULL
> 
> IOW, tsk2's struct pid has never been used as a thread-group leader and
> thus PIDTYPE_TGID is NULL. Now assume someone does create pidfds for
> tsk1 and for tsk2:
> 	
> 	tg_pidfd = pidfd_open(tsk1)		t_pidfd = pidfd_open(tsk2)
> 	-> tg_pidfd->private_data = tg_pid	-> t_pidfd->private_data = t_pid
> 
> So we stash away struct pid *tg_pid for a pidfd_open() on tsk1 and we
> stash away struct pid *t_pid for a pidfd_open() on tsk2.
> 
> If we wait on that task via P_PIDFD we get:
> 
> 				/* waiting through pidfd */
> 	waitid(P_PIDFD, tg_pidfd)		waitid(P_PIDFD, t_pidfd)
> 	tg_pid[PIDTYPE_TGID] == tsk1		t_pid[PIDTYPE_TGID] == NULL
> 	=> succeeds				=> fails
> 
> Because struct pid *tg_pid is used a thread-group leader struct pid we
> can wait on that tsk1. But we can't via the non-thread-group leader
> pidfd because the struct pid *t_pid has never been used as a
> thread-group leader.
> 
> Now assume, t_pid exec's and the struct pids are transfered. IIRC, we
> get:
> 
> 	tg_pid[PIDTYPE_PID]   = tsk2		t_pid[PIDTYPE_PID]   = tsk1
> 	tg_pid[PIDTYPE_TGID]  = tsk2		t_pid[PIDTYPE_TGID]  = NULL
> 
> If we wait on that task via P_PIDFD we get:
> 	
> 				/* waiting through pidfd */
> 	waitid(P_PIDFD, tg_pidfd)		waitid(P_PIDFD, t_pid)
> 	tg_pid[PIDTYPE_TGID] == tsk2		t_pid[PIDTYPE_TGID] == NULL
> 	=> succeeds				=> fails
> 
> Which is what we want. So effectively this should all work and I
> misremembered the struct pid linkage. So afaict we don't even have a
> problem here which is great.

It sounds like we need some tests for waitpid() directly though, to
ensure the semantics stay stable. I can add those and send a v3,
assuming the location of do_notify_pidfd() looks ok to you in v2:

https://lore.kernel.org/all/20231207170946.130823-1-tycho@tycho.pizza/

Tycho