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

Groups > linux.kernel > #1453128 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH] kernel/fork: fix CLONE_CHILD_CLEARTID regression in nscd Michal Hocko <mhocko@kernel.org> - 2016-08-01 12:50 +0200
    Re: [RFC PATCH] kernel/fork: fix CLONE_CHILD_CLEARTID regression in  nscd Oleg Nesterov <oleg@redhat.com> - 2016-08-03 23:10 +0200

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

From: Michal Hocko <mhocko@suse.com>

fec1d0115240 ("[PATCH] Disable CLONE_CHILD_CLEARTID for abnormal exit")
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.  Now, when nscd
uses a non-persistent database, clients that have it mapped keep
thinking the database is being updated by nscd, when in fact nscd has
created a new (anonymous) one (for non-persistent databases it uses an
unlinked file as backend).

The original proposal for the CLONE_CHILD_CLEARTID change claimed
(https://lkml.org/lkml/2006/10/25/233):
"
The NPTL library uses the CLONE_CHILD_CLEARTID flag on clone() syscalls
on behalf of pthread_create() library calls.  This feature is used to
request that the kernel clear the thread-id in user space (at an address
provided in the syscall) when the thread disassociates itself from the
address space, which is done in mm_release().

Unfortunately, when a multi-threaded process incurs a core dump (such as
from a SIGSEGV), the core-dumping thread sends SIGKILL signals to all of
the other threads, which then proceed to clear their user-space tids
before synchronizing in exit_mm() with the start of core dumping.  This
misrepresents the state of process's address space at the time of the
SIGSEGV and makes it more difficult for someone to debug NPTL and glibc
problems (misleading him/her to conclude that the threads had gone away
before the fault).

The fix below is to simply avoid the CLONE_CHILD_CLEARTID action if a
core dump has been initiated.
"

The resulting patch from Roland (https://lkml.org/lkml/2006/10/26/269)
seems to have a larger scope than the original patch asked for. It seems
that limitting the scope of the check to core dumping should work for
SIGSEGV issue describe above. We should also check for vfork because
this is killable since d68b46fe16ad ("vfork: make it killable").

[Changelog partly based on Andreas' description]
Fixes: fec1d0115240 ("[PATCH] Disable CLONE_CHILD_CLEARTID for abnormal exit")
Tested-by:  William Preston <wpreston@suse.com>
Cc: Roland McGrath <roland@hack.frob.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Andreas Schwab <schwab@suse.com>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---

Hi,
the issue has been reported by Andreas https://lkml.org/lkml/2015/8/20/459
almost one year ago without any response. This is my attempt to fix the issue
and the testing confirms that nscd doesn't complain with this patch applied
but I have hard time to think through all the consequences, to be honest so
I am sending this as an RFC.

 kernel/fork.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index 52e725d4a866..0c1184473c3e 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -913,14 +913,12 @@ void mm_release(struct task_struct *tsk, struct mm_struct *mm)
 	deactivate_mm(tsk, mm);
 
 	/*
-	 * If we're exiting normally, clear a user-space tid field if
-	 * requested.  We leave this alone when dying by signal, to leave
-	 * the value intact in a core dump, and to save the unnecessary
-	 * trouble, say, a killed vfork parent shouldn't touch this mm.
-	 * Userland only wants this done for a sys_exit.
+	 * Signal userspace if we're not exiting with a core dump
+	 * or a killed vfork parent which shouldn't touch this mm.
 	 */
 	if (tsk->clear_child_tid) {
-		if (!(tsk->flags & PF_SIGNALED) &&
+		if (!(tsk->flags & PF_SIGNALED && tsk->vfork_done) &&
+		    !(tsk->signal->flags & SIGNAL_GROUP_COREDUMP) &&
 		    atomic_read(&mm->mm_users) > 1) {
 			/*
 			 * We don't check the error code - if userspace has
-- 
2.8.1

[toc] | [next] | [standalone]

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

sorry for delay, I am travelling till the end of the week.

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

almost 10 years ago ;)

> 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?

> 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) &&

?

And I think PF_SIGNALED must die in any case... but this is off-topic.

Oleg.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web