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;
MIME-Version: 1.0
References: <20230807085203.819772-1-david@readahead.eu> <CAJqdLroQE1sU2uOG018Ezyome1sQb89c_mdSdvOCpM7=XNt+xg@mail.gmail.com>
 <24357f20-4354-4b44-b3f7-4cb29dcfb8b4@app.fastmail.com>
In-Reply-To: <24357f20-4354-4b44-b3f7-4cb29dcfb8b4@app.fastmail.com>
From:   Alexander Mikhalitsyn <alexander@mihalicyn.com>
Date:   Mon, 7 Aug 2023 11:31:06 +0200
Message-ID: <CAJqdLrrD41chNmoWX5kqxYp=fVN3qgzeRb86NEFKhD0gSUtYJw@mail.gmail.com>
Subject: Re: [PATCH] pid: allow pidfds for reaped tasks
To:     David Rheinsberg <david@readahead.eu>
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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Aug 7, 2023 at 11:12=E2=80=AFAM David Rheinsberg <david@readahead.e=
u> wrote:
>
> 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@readahe=
ad.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 t=
hem for
> >> +        * reaped thread-group leaders.
> >> +        */
> >> +       if (pid_has_task(pid, PIDTYPE_PID) && !pid_has_task(pid, PIDTY=
PE_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 the=
 perspective of the unlocked reader)? I don't think it is a compatibility i=
ssue, because the same issue existed before the patch. But it might indeed =
be required to avoid spurious EINVAL _while_ the target process is reaped.

Yes, that was my thought. At the same time we can see that
__unhash_process() function at first detaches PIDTYPE_PID and then
PIDTYPE_TGID.
But without having any kind of memory barrier (and locks are also
implicit memory barriers) we can't be sure that inconsistency won't
happen here.

>
> It would be unfortunate if we need that. Because it is really not require=
d for AF_UNIX or fanotify (they guarantee that they always deal with TGIDs)=
. So maybe the correct call is to just drop pidfd_prepare() and always 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 b=
etter to document that the caller of __pidfd_prepare() needs to ensure the =
source is/was a TGID?

Do you think that taking read_lock(&tasklist_lock) can cause any
issues with contention on it?
IMHO, read_lock should be safe as we are taking it for a short period of ti=
me.

But anyways, I'm not insisting on that. I've just wanted to point this
out to discuss with you and folks.

Kind regards,
Alex

>
> Thanks
> David