Messages in this thread | | | From | Alexander Mikhalitsyn <> | Date | Mon, 7 Aug 2023 11:31:06 +0200 | Subject | Re: [PATCH] pid: allow pidfds for reaped tasks |
| |
On Mon, Aug 7, 2023 at 11:12 AM David Rheinsberg <david@readahead.eu> wrote: > > Hi > > On Mon, Aug 7, 2023, at 11:01 AM, Alexander Mikhalitsyn wrote: > > On Mon, Aug 7, 2023 at 10:52 AM David Rheinsberg <david@readahead.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, PIDTYPE_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 issue, 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 required 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 better 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 time.
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
| |