Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757592AbbBFQX3 (ORCPT ); Fri, 6 Feb 2015 11:23:29 -0500 Received: from forward-corp1g.mail.yandex.net ([95.108.253.251]:33886 "EHLO forward-corp1g.mail.yandex.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756196AbbBFQXG (ORCPT ); Fri, 6 Feb 2015 11:23:06 -0500 Authentication-Results: smtpcorp1m.mail.yandex.net; dkim=pass header.i=@yandex-team.ru Subject: [PATCH 2/2] kernel/fork: handle put_user errors for CLONE_PARENT_SETTID From: Konstantin Khlebnikov To: linux-api@vger.kernel.org, Andrew Morton , Linus Torvalds , linux-kernel@vger.kernel.org Cc: Roman Gushchin , Nikita Vetoshkin , Oleg Nesterov , Pavel Emelyanov Date: Fri, 06 Feb 2015 19:23:03 +0300 Message-ID: <20150206162303.18031.37408.stgit@buzz> In-Reply-To: <20150206162301.18031.32251.stgit@buzz> References: <20150206162301.18031.32251.stgit@buzz> User-Agent: StGit/0.17.1-dirty MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3380 Lines: 84 Handling of flag CLONE_PARENT_SETTID has the same problem: error returned from put_user() is ignored. Glibc completely relies on that feature and uses value returned from syscall only for error checking. Kernels older than v2.6.24 handled that correctly but check has been removed in commit 30e49c263e36 ("pid namespaces: allow cloning of new namespace"). Commit message tells nothing about reason. I guess that was fix for commit 425fb2b4bf5d ("pid namespaces: move alloc_pid() lower in copy_process()") which breaks this logic: after it kernel writes parent pid as child pid. This patch moves related put_user() from do_fork() back into copy_process() where it was before and where error can be handled. Another problem is that before v2.6.24 CLONE_PARENT_SETTID stored child pid both in parent and child memory. Documentation in man clone(2) also tells so. In v2.6.24 put_user() was placed after copy_mm(), now only parent sees it. LTP test which should check that is buggy too: it clones child with CLONE_VM. It seems nobody have noticed this for seven years, probably we should leave it as is and document existing behavior. Signed-off-by: Konstantin Khlebnikov --- kernel/fork.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/kernel/fork.c b/kernel/fork.c index f71305d..1ea2eae 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -1194,6 +1194,7 @@ init_task_pid(struct task_struct *task, enum pid_type type, struct pid *pid) static struct task_struct *copy_process(unsigned long clone_flags, unsigned long stack_start, unsigned long stack_size, + int __user *parent_tidptr, int __user *child_tidptr, struct pid *pid, int trace) @@ -1416,6 +1417,12 @@ static struct task_struct *copy_process(unsigned long clone_flags, goto bad_fork_cleanup_io; } + if (clone_flags & CLONE_PARENT_SETTID) { + retval = put_user(pid_vnr(pid), parent_tidptr); + if (retval) + goto bad_fork_free_pid; + } + p->set_child_tid = (clone_flags & CLONE_CHILD_SETTID) ? child_tidptr : NULL; /* * Clear TID on mm_release()? @@ -1617,7 +1624,7 @@ static inline void init_idle_pids(struct pid_link *links) struct task_struct *fork_idle(int cpu) { struct task_struct *task; - task = copy_process(CLONE_VM, 0, 0, NULL, &init_struct_pid, 0); + task = copy_process(CLONE_VM, 0, 0, NULL, NULL, &init_struct_pid, 0); if (!IS_ERR(task)) { init_idle_pids(task->pids); init_idle(task, cpu); @@ -1661,7 +1668,7 @@ long do_fork(unsigned long clone_flags, } p = copy_process(clone_flags, stack_start, stack_size, - child_tidptr, NULL, trace); + parent_tidptr, child_tidptr, NULL, trace); /* * Do this prior waking up the new thread - the thread pointer * might get invalid after that point, if the thread exits quickly. @@ -1675,9 +1682,6 @@ long do_fork(unsigned long clone_flags, pid = get_task_pid(p, PIDTYPE_PID); nr = pid_vnr(pid); - if (clone_flags & CLONE_PARENT_SETTID) - put_user(nr, parent_tidptr); - if (clone_flags & CLONE_VFORK) { p->vfork_done = &vfork; init_completion(&vfork); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/