Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1461045 > unrolled thread

Re: [RFC PATCH] kernel/fork: fix CLONE_CHILD_CLEARTID regression in nscd

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC PATCH] kernel/fork: fix CLONE_CHILD_CLEARTID regression in  nscd Michal Hocko <mhocko@kernel.org> - 2016-08-12 11:50 +0200
    Re: [RFC PATCH] kernel/fork: fix CLONE_CHILD_CLEARTID regression in  nscd Michal Hocko <mhocko@kernel.org> - 2016-08-19 15:30 +0200
      Re: [RFC PATCH] kernel/fork: fix CLONE_CHILD_CLEARTID regression         in nscd Oleg Nesterov <oleg@redhat.com> - 2016-08-23 17:30 +0200
        Re: [RFC PATCH] kernel/fork: fix CLONE_CHILD_CLEARTID regression in  nscd Michal Hocko <mhocko@kernel.org> - 2016-08-23 18:20 +0200

#1461045 — Re: [RFC PATCH] kernel/fork: fix CLONE_CHILD_CLEARTID regression in nscd

On Wed 03-08-16 23:08:04, Oleg Nesterov wrote:
> sorry for delay, I am travelling till the end of the week.

Same here...

> On 08/01, Michal Hocko wrote:
> >
> > fec1d0115240 ("[PATCH] Disable CLONE_CHILD_CLEARTID for abnormal exit")
> 
> almost 10 years ago ;)

Yes, it's been a while... I guess nscd doesn't enable persistent host
caching by default. I just know that our customer wanted to enable this
feature to find out it doesn't work properly. At least that is my
understanding.

> > has caused a subtle regression in nscd which uses CLONE_CHILD_CLEARTID
> > to clear the nscd_certainly_running flag in the shared databases, so
> > that the clients are notified when nscd is restarted.
> 
> So iiuc with this patch nscd_certainly_running should be cleared even if
> ncsd was killed by !sig_kernel_coredump() signal, right?

Yes.

> > We should also check for vfork because
> > this is killable since d68b46fe16ad ("vfork: make it killable").
> 
> Hmm, why? Can't understand... In any case this check doesn't look right, the
> comment says "a killed vfork parent" while tsk->vfork_done != NULL means it
> is a vforked child.
> 
> So if we want this change, why we can't simply do
> 
> 	-	if (!(tsk->flags & PF_SIGNALED) &&
> 	+	if (!(tsk->signal->flags & SIGNAL_GROUP_COREDUMP) &&
> 
> ?

This is what I had initially. But then the comment above the check made
me worried that the parent of vforked child might get confused if the
flag is cleared. I might have completely misunderstood the point of the
comment though. So if you believe that vfork_done check is incorrect I
can drop it. It shouldn't have any effect on the nscd usecase AFAIU.

Thanks!

-- 
Michal Hocko
SUSE Labs

[toc] | [next] | [standalone]

#1466407

On Fri 12-08-16 11:41:13, Michal Hocko wrote:
> On Wed 03-08-16 23:08:04, Oleg Nesterov wrote:
> > sorry for delay, I am travelling till the end of the week.
> 
> Same here...
> 
> > On 08/01, Michal Hocko wrote:
[...]
> > > We should also check for vfork because
> > > this is killable since d68b46fe16ad ("vfork: make it killable").
> > 
> > Hmm, why? Can't understand... In any case this check doesn't look right, the
> > comment says "a killed vfork parent" while tsk->vfork_done != NULL means it
> > is a vforked child.
> > 
> > So if we want this change, why we can't simply do
> > 
> > 	-	if (!(tsk->flags & PF_SIGNALED) &&
> > 	+	if (!(tsk->signal->flags & SIGNAL_GROUP_COREDUMP) &&
> > 
> > ?
> 
> This is what I had initially. But then the comment above the check made
> me worried that the parent of vforked child might get confused if the
> flag is cleared. I might have completely misunderstood the point of the
> comment though. So if you believe that vfork_done check is incorrect I
> can drop it. It shouldn't have any effect on the nscd usecase AFAIU.

So should I drop the vfork check and repost or we do not care about this
"regression" and declare nscd broken because it relies on a behavior
which is not in fact guaranteed by the kernel?
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1468636 — Re: [RFC PATCH] kernel/fork: fix CLONE_CHILD_CLEARTID regression in nscd

On 08/19, Michal Hocko wrote:
>
> On Fri 12-08-16 11:41:13, Michal Hocko wrote:
> > On Wed 03-08-16 23:08:04, Oleg Nesterov wrote:
> > >
> > > So if we want this change, why we can't simply do
> > >
> > > 	-	if (!(tsk->flags & PF_SIGNALED) &&
> > > 	+	if (!(tsk->signal->flags & SIGNAL_GROUP_COREDUMP) &&
> > >
> > > ?
> >
> > This is what I had initially. But then the comment above the check made
> > me worried that the parent of vforked child might get confused if the
> > flag is cleared.

I don't think the child can be confused... At least I can't imagine how
this can happen.

Anyway, I objected because the tsk->vfork != NULL check was wrong, in this
case this tsk is vforke'd child, not parent.

> So should I drop the vfork check and repost

Probably yes. At least the SIGNAL_GROUP_COREDUMP will match the comment.

> or we do not care about this
> "regression"

Honestly, I do not know ;) Personally, I am always scared when it comes
to the subtle changes like this, you can never know what can be broken.
And note that it can be broken 10 years later, like it happened with
nscd ;)

But if you send the s/PF_SIGNALED/SIGNAL_GROUP_COREDUMP/ change I will
ack it ;) Even if it won't really fix this nscd problem (imo), because
I guess nscd wants to reset ->clear_child_tid even if the signal was
sig_kernel_coredump().

Oleg.

[toc] | [prev] | [next] | [standalone]

#1468682

On Tue 23-08-16 17:27:11, Oleg Nesterov wrote:
> On 08/19, Michal Hocko wrote:
[...]
> > or we do not care about this
> > "regression"
> 
> Honestly, I do not know ;) Personally, I am always scared when it comes
> to the subtle changes like this, you can never know what can be broken.

If _you_ are scarred (after so many years of permanent exposure to this
code) then try to imagine how I am scarred when touching anything in
this area...

> And note that it can be broken 10 years later, like it happened with
> nscd ;)
> 
> But if you send the s/PF_SIGNALED/SIGNAL_GROUP_COREDUMP/ change I will
> ack it ;)

OK, I will repost

> Even if it won't really fix this nscd problem (imo), because
> I guess nscd wants to reset ->clear_child_tid even if the signal was
> sig_kernel_coredump().

Come on, have you ever seen this fine piece of software crashing?
But more seriously, I wouldn't give a damn because nscd is usually the
first thing I disable on my systems but there seem to be people who
would like to use this persistence thingy and even service restart will
break it. So I think we should plug this hole.

Anyway thanks for your review and feedback. As always it is really
appreciated!
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web