getxpid() parent lookup is broken

getxpid() parent lookup is broken

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1244 bytes --]

I noticed that Tomoyo doesn't build on Alpha because Tomoyo tries to
call sys_getpid() and sys_getppid().  That's a bug in Tomoyo, but when I
looked at why Alpha is different I found that the implementation of
getxpid() hasn't been kept in sync getppid() for other architectures and
is presumably now incorrect.

sys_getppid():
	rcu_read_lock();
	pid = task_tgid_vnr(current->real_parent);
	rcu_read_unlock();

task_tgid_vnr(current->real_parent) expands through various inline
functions to:

pid_nr_ns(current->real_parent->group_leader->pids[PIDTYPE_PID].pid,
          current->nsproxy->pid_ns)

sys_getxpid():
	/* See linux/kernel/timer.c sys_getppid for discussion
	   about this loop.  */
	ldq	$3, TASK_GROUP_LEADER($2)
	ldq	$4, TASK_REAL_PARENT($3)
	ldl	$0, TASK_TGID($2)
1:	ldl	$1, TASK_TGID($4)
#ifdef CONFIG_SMP
	mov	$4, $5
	mb
	ldq	$3, TASK_GROUP_LEADER($2)
	ldq	$4, TASK_REAL_PARENT($3)
	cmpeq	$4, $5, $5
	beq	$5, 1b
#endif

The comment is obviously out-of-date.  This isn't following RCU protocol
and it isn't namespace-aware.  I think it needs to be turned into a
wrapper for the generic code.

Ben.

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: getxpid() parent lookup is broken

[Matt Turner]

On Sun, Sep 26, 2010 at 12:34 AM, Ben Hutchings <ben@decadent.org.uk> wrote:
> I noticed that Tomoyo doesn't build on Alpha because Tomoyo tries to
> call sys_getpid() and sys_getppid().  That's a bug in Tomoyo, but when I
> looked at why Alpha is different I found that the implementation of
> getxpid() hasn't been kept in sync getppid() for other architectures and
> is presumably now incorrect.
>
> sys_getppid():
>        rcu_read_lock();
>        pid = task_tgid_vnr(current->real_parent);
>        rcu_read_unlock();
>
> task_tgid_vnr(current->real_parent) expands through various inline
> functions to:
>
> pid_nr_ns(current->real_parent->group_leader->pids[PIDTYPE_PID].pid,
>          current->nsproxy->pid_ns)
>
> sys_getxpid():
>        /* See linux/kernel/timer.c sys_getppid for discussion
>           about this loop.  */
>        ldq     $3, TASK_GROUP_LEADER($2)
>        ldq     $4, TASK_REAL_PARENT($3)
>        ldl     $0, TASK_TGID($2)
> 1:      ldl     $1, TASK_TGID($4)
> #ifdef CONFIG_SMP
>        mov     $4, $5
>        mb
>        ldq     $3, TASK_GROUP_LEADER($2)
>        ldq     $4, TASK_REAL_PARENT($3)
>        cmpeq   $4, $5, $5
>        beq     $5, 1b
> #endif
>
> The comment is obviously out-of-date.  This isn't following RCU protocol
> and it isn't namespace-aware.  I think it needs to be turned into a
> wrapper for the generic code.
>
> Ben.
>
> --
> Ben Hutchings
> Once a job is fouled up, anything done to improve it makes it worse.

Hi Ben, Al,

I recently looked at this gentoo bug --
https://bugs.gentoo.org/show_bug.cgi?id=405829 and then came across
this email and the two patches. They seem possibly related.

It looks like Al said he thought he had a better way of fixing the
problem, but then I'm not sure if his patches surfaced or not.

Also possibly related, Tobias and Michael have seen some RCU stalls
with recent kernels. Looks like getxpid needs an update?

Thanks,
Matt
--
To unsubscribe from this list: send the line "unsubscribe linux-alpha" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: getxpid() parent lookup is broken

[Al Viro]

On Tue, May 29, 2012 at 12:55:10AM -0400, Matt Turner wrote:

> I recently looked at this gentoo bug --
> https://bugs.gentoo.org/show_bug.cgi?id=405829 and then came across
> this email and the two patches. They seem possibly related.
> 
> It looks like Al said he thought he had a better way of fixing the
> problem, but then I'm not sure if his patches surfaced or not.
> 
> Also possibly related, Tobias and Michael have seen some RCU stalls
> with recent kernels. Looks like getxpid needs an update?

Umm...  Let me see if I can find it...  Here:

alpha: take a bunch of syscalls into osf_sys.c
    
New helper: current_thread_info().  Allows to do a bunch of odd syscalls in C.
While we are at it, there had never been a reason to do osf_getpriority() in
assembler.  We also get "namespace"-aware (read: consistent with getuid(2),
etc.) behaviour from getx?id() syscalls now.
    
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

diff --git a/arch/alpha/include/asm/ptrace.h b/arch/alpha/include/asm/ptrace.h
index fd698a1..b87755a 100644
--- a/arch/alpha/include/asm/ptrace.h
+++ b/arch/alpha/include/asm/ptrace.h
@@ -76,7 +76,10 @@ struct switch_stack {
 #define task_pt_regs(task) \
   ((struct pt_regs *) (task_stack_page(task) + 2*PAGE_SIZE) - 1)
 
-#define force_successful_syscall_return() (task_pt_regs(current)->r0 = 0)
+#define current_pt_regs() \
+  ((struct pt_regs *) ((char *)current_thread_info() + 2*PAGE_SIZE) - 1)
+
+#define force_successful_syscall_return() (current_pt_regs()->r0 = 0)
 
 #endif
 
diff --git a/arch/alpha/kernel/entry.S b/arch/alpha/kernel/entry.S
index 6d159ce..22b0c4d 100644
--- a/arch/alpha/kernel/entry.S
+++ b/arch/alpha/kernel/entry.S
@@ -797,115 +797,6 @@ sys_rt_sigreturn:
 .end sys_rt_sigreturn
 
 	.align	4
-	.globl	sys_sethae
-	.ent	sys_sethae
-sys_sethae:
-	.prologue 0
-	stq	$16, 152($sp)
-	ret
-.end sys_sethae
-
-	.align	4
-	.globl	osf_getpriority
-	.ent	osf_getpriority
-osf_getpriority:
-	lda	$sp, -16($sp)
-	stq	$26, 0($sp)
-	.prologue 0
-
-	jsr	$26, sys_getpriority
-
-	ldq	$26, 0($sp)
-	blt	$0, 1f
-
-	/* Return value is the unbiased priority, i.e. 20 - prio.
-	   This does result in negative return values, so signal
-	   no error by writing into the R0 slot.  */
-	lda	$1, 20
-	stq	$31, 16($sp)
-	subl	$1, $0, $0
-	unop
-
-1:	lda	$sp, 16($sp)
-	ret
-.end osf_getpriority
-
-	.align	4
-	.globl	sys_getxuid
-	.ent	sys_getxuid
-sys_getxuid:
-	.prologue 0
-	ldq	$2, TI_TASK($8)
-	ldq	$3, TASK_CRED($2)
-	ldl	$0, CRED_UID($3)
-	ldl	$1, CRED_EUID($3)
-	stq	$1, 80($sp)
-	ret
-.end sys_getxuid
-
-	.align	4
-	.globl	sys_getxgid
-	.ent	sys_getxgid
-sys_getxgid:
-	.prologue 0
-	ldq	$2, TI_TASK($8)
-	ldq	$3, TASK_CRED($2)
-	ldl	$0, CRED_GID($3)
-	ldl	$1, CRED_EGID($3)
-	stq	$1, 80($sp)
-	ret
-.end sys_getxgid
-
-	.align	4
-	.globl	sys_getxpid
-	.ent	sys_getxpid
-sys_getxpid:
-	.prologue 0
-	ldq	$2, TI_TASK($8)
-
-	/* See linux/kernel/timer.c sys_getppid for discussion
-	   about this loop.  */
-	ldq	$3, TASK_GROUP_LEADER($2)
-	ldq	$4, TASK_REAL_PARENT($3)
-	ldl	$0, TASK_TGID($2)
-1:	ldl	$1, TASK_TGID($4)
-#ifdef CONFIG_SMP
-	mov	$4, $5
-	mb
-	ldq	$3, TASK_GROUP_LEADER($2)
-	ldq	$4, TASK_REAL_PARENT($3)
-	cmpeq	$4, $5, $5
-	beq	$5, 1b
-#endif
-	stq	$1, 80($sp)
-	ret
-.end sys_getxpid
-
-	.align	4
-	.globl	sys_alpha_pipe
-	.ent	sys_alpha_pipe
-sys_alpha_pipe:
-	lda	$sp, -16($sp)
-	stq	$26, 0($sp)
-	.prologue 0
-
-	mov	$31, $17
-	lda	$16, 8($sp)
-	jsr	$26, do_pipe_flags
-
-	ldq	$26, 0($sp)
-	bne	$0, 1f
-
-	/* The return values are in $0 and $20.  */
-	ldl	$1, 12($sp)
-	ldl	$0, 8($sp)
-
-	stq	$1, 80+16($sp)
-1:	lda	$sp, 16($sp)
-	ret
-.end sys_alpha_pipe
-
-	.align	4
 	.globl	sys_execve
 	.ent	sys_execve
 sys_execve:
diff --git a/arch/alpha/kernel/osf_sys.c b/arch/alpha/kernel/osf_sys.c
index 49ee319..3c4e16a 100644
--- a/arch/alpha/kernel/osf_sys.c
+++ b/arch/alpha/kernel/osf_sys.c
@@ -1238,3 +1238,52 @@ SYSCALL_DEFINE3(osf_writev, unsigned long, fd,
 }
 
 #endif
+
+SYSCALL_DEFINE2(osf_getpriority, int, which, int, who)
+{
+	int prio = sys_getpriority(which, who);
+	if (prio >= 0) {
+		/* Return value is the unbiased priority, i.e. 20 - prio.
+		   This does result in negative return values, so signal
+		   no error */
+		force_successful_syscall_return();
+		prio = 20 - prio;
+	}
+	return prio;
+}
+
+SYSCALL_DEFINE0(getxuid)
+{
+	current_pt_regs()->r20 = sys_geteuid();
+	return sys_getuid();
+}
+
+SYSCALL_DEFINE0(getxgid)
+{
+	current_pt_regs()->r20 = sys_getegid();
+	return sys_getgid();
+}
+
+SYSCALL_DEFINE0(getxpid)
+{
+	current_pt_regs()->r20 = sys_getppid();
+	return sys_getpid();
+}
+
+SYSCALL_DEFINE0(alpha_pipe)
+{
+	int fd[2];
+	int res = do_pipe_flags(fd, 0);
+	if (!res) {
+		/* The return values are in $0 and $20.  */
+		current_pt_regs()->r20 = fd[1];
+		res = fd[0];
+	}
+	return res;
+}
+
+SYSCALL_DEFINE1(sethae, unsigned long, val)
+{
+	current_pt_regs()->hae = val;
+	return 0;
+}
diff --git a/arch/alpha/kernel/systbls.S b/arch/alpha/kernel/systbls.S
index e534e1c..97c9030 100644
--- a/arch/alpha/kernel/systbls.S
+++ b/arch/alpha/kernel/systbls.S
@@ -111,7 +111,7 @@ sys_call_table:
 	.quad sys_socket
 	.quad sys_connect
 	.quad sys_accept
-	.quad osf_getpriority			/* 100 */
+	.quad sys_osf_getpriority			/* 100 */
 	.quad sys_send
 	.quad sys_recv
 	.quad sys_sigreturn
diff --git a/kernel/timer.c b/kernel/timer.c
index 6ec7e7e..6885e5f 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -1394,13 +1394,6 @@ SYSCALL_DEFINE1(alarm, unsigned int, seconds)
 
 #endif
 
-#ifndef __alpha__
-
-/*
- * The Alpha uses getxpid, getxuid, and getxgid instead.  Maybe this
- * should be moved into arch/i386 instead?
- */
-
 /**
  * sys_getpid - return the thread group id of the current process
  *
@@ -1456,8 +1449,6 @@ SYSCALL_DEFINE0(getegid)
 	return from_kgid_munged(current_user_ns(), current_egid());
 }
 
-#endif
-
 static void process_timeout(unsigned long __data)
 {
 	wake_up_process((struct task_struct *)__data);

Re: getxpid() parent lookup is broken

[Al Viro]

On Tue, May 29, 2012 at 03:09:43PM +0100, Al Viro wrote:
> On Tue, May 29, 2012 at 12:55:10AM -0400, Matt Turner wrote:
> 
> > I recently looked at this gentoo bug --
> > https://bugs.gentoo.org/show_bug.cgi?id=405829 and then came across
> > this email and the two patches. They seem possibly related.
> > 
> > It looks like Al said he thought he had a better way of fixing the
> > problem, but then I'm not sure if his patches surfaced or not.
> > 
> > Also possibly related, Tobias and Michael have seen some RCU stalls
> > with recent kernels. Looks like getxpid needs an update?
> 
> Umm...  Let me see if I can find it...  Here:
[snip]

BTW, there's another completely untested patch bouncing around in the local
tree - takes kernel_execve() out of entry.S

diff --git a/arch/alpha/kernel/alpha_ksyms.c b/arch/alpha/kernel/alpha_ksyms.c
index d96e742..b77b813 100644
--- a/arch/alpha/kernel/alpha_ksyms.c
+++ b/arch/alpha/kernel/alpha_ksyms.c
@@ -52,7 +52,6 @@ EXPORT_SYMBOL(alpha_write_fp_reg_s);
 
 /* entry.S */
 EXPORT_SYMBOL(kernel_thread);
-EXPORT_SYMBOL(kernel_execve);
 
 /* Networking helper routines. */
 EXPORT_SYMBOL(csum_tcpudp_magic);
diff --git a/arch/alpha/kernel/entry.S b/arch/alpha/kernel/entry.S
index 4074645..a8c9db8 100644
--- a/arch/alpha/kernel/entry.S
+++ b/arch/alpha/kernel/entry.S
@@ -663,58 +663,6 @@ kernel_thread:
 	br	ret_to_kernel
 .end kernel_thread
 
-/*
- * kernel_execve(path, argv, envp)
- */
-	.align	4
-	.globl	kernel_execve
-	.ent	kernel_execve
-kernel_execve:
-	/* We can be called from a module.  */
-	ldgp	$gp, 0($27)
-	lda	$sp, -(32+SIZEOF_PT_REGS+8)($sp)
-	.frame	$sp, 32+SIZEOF_PT_REGS+8, $26, 0
-	stq	$26, 0($sp)
-	stq	$16, 8($sp)
-	stq	$17, 16($sp)
-	stq	$18, 24($sp)
-	.prologue 1
-
-	lda	$16, 32($sp)
-	lda	$17, 0
-	lda	$18, SIZEOF_PT_REGS
-	bsr	$26, memset		!samegp
-
-	/* Avoid the HAE being gratuitously wrong, which would cause us
-	   to do the whole turn off interrupts thing and restore it.  */
-	ldq	$2, alpha_mv+HAE_CACHE
-	stq	$2, 152+32($sp)
-
-	ldq	$16, 8($sp)
-	ldq	$17, 16($sp)
-	ldq	$18, 24($sp)
-	lda	$19, 32($sp)
-	bsr	$26, do_execve		!samegp
-
-	ldq	$26, 0($sp)
-	bne	$0, 1f			/* error! */
-
-	/* Move the temporary pt_regs struct from its current location
-	   to the top of the kernel stack frame.  See copy_thread for
-	   details for a normal process.  */
-	lda	$16, 0x4000 - SIZEOF_PT_REGS($8)
-	lda	$17, 32($sp)
-	lda	$18, SIZEOF_PT_REGS
-	bsr	$26, memmove		!samegp
-
-	/* Take that over as our new stack frame and visit userland!  */
-	lda	$sp, 0x4000 - SIZEOF_PT_REGS($8)
-	br	$31, ret_from_sys_call
-
-1:	lda	$sp, 32+SIZEOF_PT_REGS+8($sp)
-	ret
-.end kernel_execve
-
 \f
 /*
  * Special system calls.  Most of these are special in that they either
diff --git a/arch/alpha/kernel/process.c b/arch/alpha/kernel/process.c
index 153d3fc..d6fde98 100644
--- a/arch/alpha/kernel/process.c
+++ b/arch/alpha/kernel/process.c
@@ -455,3 +455,22 @@ get_wchan(struct task_struct *p)
 	}
 	return pc;
 }
+
+int kernel_execve(const char *path, const char *const argv[], const char *const envp[])
+{
+	/* Avoid the HAE being gratuitously wrong, which would cause us
+	   to do the whole turn off interrupts thing and restore it.  */
+	struct pt_regs regs = {.hae = alpha_mv.hae_cache};
+	int err = do_execve(path, argv, envp, &regs);
+	if (!err) {
+		struct pt_regs *p = current_pt_regs();
+		/* copy regs to normal position and off to userland we go... */
+		*p = regs;
+		__asm__ __volatile__ (
+			"mov	%0, $sp;"
+			"br	$31, ret_from_sys_call"
+			: : "r"(p));
+	}
+	return err;
+}
+EXPORT_SYMBOL(kernel_execve);