Re: [PATCH v10 4/9] cgroup: cgroup v2 freezer

[Roman Gushchin <guro@fb.com>]

On Wed, Apr 24, 2019 at 05:46:19PM +0200, Oleg Nesterov wrote:
> On 04/22, Roman Gushchin wrote:
> >
> > > > Hm, it might work too, but I'm not sure I like it more. IMO, the best option
> > > > is to have a single cgroup_leave_frozen(true) in signal.c, it's just simpler.
> > > > If a user changed the desired state of cgroup twice, there is no need to avoid
> > > > state transitions. Or maybe I don't see it yet.
> > >
> > > Then why do we need cgroup_leave_frozen(false) in wait_for_vfork_done() ? How
> > > does it differ from get_signal() ?
> >
> > We need it because sleeping in vfork is a special state which we want to
> > account as frozen. And if the parent process wakes up while the cgroup is frozen
> > (because of the child death, for example), we want to push it into the "proper"
> > frozen state without changing the state of the cgroup.
> 
> Again, I do not see how vfork() differs from get_signal() in this respect.
> 
> Let me provide another example. A TASK_STOPPED task reacts to SIGCONT and
> returns to get_signal(), current->frozen is true.
> 
> If this races with CGRP_FREEZE, the task should not return to user-space,
> just like vfork(). I see no difference.
> 
> They differ in that wait_for_vfork_done() should guarentee TIF_SIGPENDING
> in this case, but this is another story...

Right, I agree.

> 
> >
> > > If nothing else. Suppose that wait_for_vfork_done() calls leave(false) and this
> > > races with freezer, CGRP_FREEZE is already set but JOBCTL_TRAP_FREEZE is not.
> > >
> > > This sets TIF_SIGPENDING to ensure the task won't return to user mode, thus it
> > > calls get_signal().
> > >
> > > get_signal() doesn't see JOBCTL_TRAP_FREEZE, it notices ->frozen == T and does
> > > cgroup_leave_frozen(true) which clears ->frozen.
> > >
> > > Then the task calls dequeue_signal(), clears TIF_SIGPENDING and returns to user
> > > mode?
> >
> > Got it, a good catch! So if the freezer races with vfork() completion, we might
> > have a spurious frozen->unfrozen->frozen transition of the cgroup state.
> >
> > Switching to cgroup_leave_frozen(false) seems to solve it, but I'm slightly
> > concerned that we're basically putting the task in a busy loop between
> > the setting CGRP_FREEZE and setting TRAP_FREEZE.
> 
> Yes, yes. Didn't I say I dislike the new ->frozen check in recalc() ? ;)
> 
> OK, how about the ABSOLUTELY UNTESTED patch below? For the start.

It looks good to me (and all freezer selftests pass).

Just to be sure, is it a solution to avoid the busy loop in the signal handling
loop, right? Because it doesn't allow to drop the ->frozen check from recalc().

The JOBCTL_TRAP_FREEZE check without siglock initially looked dangerous to me,
but after some thoughts I didn't find any case when it's wrong.

Do you prefer me to master a patch or to do it by yourself?

Thank you!

Roman