Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: id2994666:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-624-g7714e4406d-fm-20230801.001-g7714e440
Mime-Version: 1.0
Message-Id: <24357f20-4354-4b44-b3f7-4cb29dcfb8b4@app.fastmail.com>
In-Reply-To: <CAJqdLroQE1sU2uOG018Ezyome1sQb89c_mdSdvOCpM7=XNt+xg@mail.gmail.com>
References: <20230807085203.819772-1-david@readahead.eu>
 <CAJqdLroQE1sU2uOG018Ezyome1sQb89c_mdSdvOCpM7=XNt+xg@mail.gmail.com>
Date:   Mon, 07 Aug 2023 11:12:11 +0200
From:   "David Rheinsberg" <david@readahead.eu>
To:     "Alexander Mikhalitsyn" <alexander@mihalicyn.com>
Cc:     linux-kernel@vger.kernel.org,
        "Christian Brauner" <brauner@kernel.org>,
        "Jan Kara" <jack@suse.cz>, "Kees Cook" <keescook@chromium.org>,
        "Luca Boccassi" <bluca@debian.org>
Subject: Re: [PATCH] pid: allow pidfds for reaped tasks
Content-Type: text/plain;charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi

On Mon, Aug 7, 2023, at 11:01 AM, Alexander Mikhalitsyn wrote:
> On Mon, Aug 7, 2023 at 10:52=E2=80=AFAM David Rheinsberg <david@readah=
ead.eu> wrote:
[...]
>>  int pidfd_prepare(struct pid *pid, unsigned int flags, struct file *=
*ret)
>>  {
>> -       if (!pid || !pid_has_task(pid, PIDTYPE_TGID))
>> +       if (!pid)
>> +               return -EINVAL;
>> +
>> +       /*
>> +        * Non thread-group leaders cannot have pidfds, but we allow =
them for
>> +        * reaped thread-group leaders.
>> +        */
>> +       if (pid_has_task(pid, PIDTYPE_PID) && !pid_has_task(pid, PIDT=
YPE_TGID))
>>                 return -EINVAL;
>
> Hi David!
>
> As far as I understand, __unhash_process is always called with a
> tasklist_lock held for writing.
> Don't we need to take tasklist_lock for reading here to guarantee
> consistency between
> pid_has_task(pid, PIDTYPE_PID) and pid_has_task(pid, PIDTYPE_TGID)
> return values?

You mean PIDTYPE_TGID being cleared before PIDTYPE_PID (at least from th=
e perspective of the unlocked reader)? I don't think it is a compatibili=
ty issue, because the same issue existed before the patch. But it might =
indeed be required to avoid spurious EINVAL _while_ the target process i=
s reaped.

It would be unfortunate if we need that. Because it is really not requir=
ed for AF_UNIX or fanotify (they guarantee that they always deal with TG=
IDs). So maybe the correct call is to just drop pidfd_prepare() and alwa=
ys use __pidfd_prepare()? So far the safety-measures of pidfd_prepare() =
introduced two races I already mentioned in the commit-message. So maybe=
 it is just better to document that the caller of __pidfd_prepare() need=
s to ensure the source is/was a TGID?

Thanks
David