c/r of pdeath

c/r of pdeath

[Serge E. Hallyn]

Hi Oren,

commit 9a45e26c0aabda6a94e2ac620befd8ee12a7363d adds
reset of pdeath_signal.  It does so unconditionally.  I
don't think that's safe.  Perhaps if pdeath_signal is
anything other than 0, it should only be restored if
the task is capable(CAP_KILL)?

-serge

Re: c/r of pdeath

[Oren Laadan]

Serge E. Hallyn wrote:
> Hi Oren,
> 
> commit 9a45e26c0aabda6a94e2ac620befd8ee12a7363d adds
> reset of pdeath_signal.  It does so unconditionally.  I
> don't think that's safe.  Perhaps if pdeath_signal is
> anything other than 0, it should only be restored if
> the task is capable(CAP_KILL)?

Hmmm... maybe I'm missing something here, but --

pdeath_signal indicates that the process wishes to receive
a signal, not to send one. It may change through prctl()
without requiring any capabilities from the caller. Finally
it is reset at fork/clone.

So at worse it will kill the specific task that holds it ?

--

As a side note - for a brief moment I worried that it may
break restart with zombies, if the to-be-zombie process has
a child that already restarted (including pdeath_signal) and
then exits, then the child will receive a signal unwillingly.

I then realized that it's safe as long as we restore parents
before their children. In turn this depends on the checkpoint
order, which indeed operates this way.

Otherwise we would have needed set this to all processes
after all zombies indeed have terminated - which means another
sync point at restart, or a sweep by coordinator on all tasks.

Oren.

Re: c/r of pdeath

[Serge E. Hallyn]

Quoting Oren Laadan (orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org):
> 
> 
> Serge E. Hallyn wrote:
> > Hi Oren,
> > 
> > commit 9a45e26c0aabda6a94e2ac620befd8ee12a7363d adds
> > reset of pdeath_signal.  It does so unconditionally.  I
> > don't think that's safe.  Perhaps if pdeath_signal is
> > anything other than 0, it should only be restored if
> > the task is capable(CAP_KILL)?
> 
> Hmmm... maybe I'm missing something here, but --

Nope, you're not.  I was thinking wrong.

> pdeath_signal indicates that the process wishes to receive
> a signal, not to send one. It may change through prctl()
> without requiring any capabilities from the caller. Finally
> it is reset at fork/clone.
> 
> So at worse it will kill the specific task that holds it ?
> 
> --
> 
> As a side note - for a brief moment I worried that it may
> break restart with zombies, if the to-be-zombie process has
> a child that already restarted (including pdeath_signal) and
> then exits, then the child will receive a signal unwillingly.
> 
> I then realized that it's safe as long as we restore parents
> before their children. In turn this depends on the checkpoint
> order, which indeed operates this way.
> 
> Otherwise we would have needed set this to all processes
> after all zombies indeed have terminated - which means another
> sync point at restart, or a sweep by coordinator on all tasks.
> 
> Oren.

Re: c/r of pdeath

[Oren Laadan]

Oren Laadan wrote:
> 
> Serge E. Hallyn wrote:
>> Hi Oren,
>>
>> commit 9a45e26c0aabda6a94e2ac620befd8ee12a7363d adds
>> reset of pdeath_signal.  It does so unconditionally.  I
>> don't think that's safe.  Perhaps if pdeath_signal is
>> anything other than 0, it should only be restored if
>> the task is capable(CAP_KILL)?
> 
> Hmmm... maybe I'm missing something here, but --
> 
> pdeath_signal indicates that the process wishes to receive
> a signal, not to send one. It may change through prctl()
> without requiring any capabilities from the caller. Finally
> it is reset at fork/clone.
> 
> So at worse it will kill the specific task that holds it ?
> 
> --
> 
> As a side note - for a brief moment I worried that it may
> break restart with zombies, if the to-be-zombie process has
> a child that already restarted (including pdeath_signal) and
> then exits, then the child will receive a signal unwillingly.
> 
> I then realized that it's safe as long as we restore parents
> before their children. In turn this depends on the checkpoint
> order, which indeed operates this way.

Bahh... silly me -
it's handled by commit efd1403a4606e0d6bd84299dab0b74792531c712
"c/r: introduce PF_RESTARTING, and skip notification on exit"

Oren.