Re: [PATCH] copy_process(): Move fd_install() out of sighand->siglock critical section

[Christian Brauner]

On Tue, Feb 08, 2022 at 07:07:41PM +0000, Al Viro wrote:
> On Tue, Feb 08, 2022 at 01:51:35PM -0500, Waiman Long wrote:
> > On 2/8/22 13:16, Al Viro wrote:
> > > On Tue, Feb 08, 2022 at 11:39:12AM -0500, Waiman Long wrote:
> > > 
> > > > One way to solve this problem is to move the fd_install() call out of
> > > > the sighand->siglock critical section.
> > > > 
> > > > Before commit 6fd2fe494b17 ("copy_process(): don't use ksys_close()
> > > > on cleanups"), the pidfd installation was done without holding both
> > > > the task_list lock and the sighand->siglock. Obviously, holding these
> > > > two locks are not really needed to protect the fd_install() call.
> > > > So move the fd_install() call down to after the releases of both locks.
> > > 	Umm... That assumes we can delay it that far.  IOW, that nothing
> > > relies upon having pidfd observable in /proc/*/fd as soon as the child
> > > becomes visible there in the first place.
> > > 
> > > 	What warranties are expected from CLONE_PIDFD wrt observation of
> > > child's descriptor table?
> > > 
> > I think the fd_install() call can be moved after the release of
> > sighand->siglock but before the release the tasklist_lock. Will that be good
> > enough?
> 
> Looks like it should, but I'd rather hear from the CLONE_PIDFD authors first...
> Christian, could you comment on that?

Sorry, I'm on vacation right now until the 17th so I'm a bit mia right
now.

Short story is, I think it's fine to move the fd_install() later.

We explicitly did not give such a strong guarantee for /proc/*/fd as
we figured that's unpleasant to do cleanly and really not required, i.e.
there's no obvious use-case for that apart from really weird
corner-cases that I think we can fend off.

(For the most part we expect people that use pidfd to almost not rely on
proc. When they access proc they likely want get metadata about the
process - something which cannot yet be done using pidfds - and that's
usually only relevant later in the lifecycle of a process.)

Originally the code installed the fd before grabbing tasklist lock and
siglock but as you rightly pointed out back then this gets us into
ksys_close() territory which is yucky. So I think this was an oversight
when we fixed that.

I can pick that patch up now. Thanks for all the reviews.