getxpid() parent lookup is broken

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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.

index fd698a1..b87755a 100644
@@ -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
 

index e534e1c..97c9030 100644
@@ -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

index 6ec7e7e..6885e5f 100644
@@ -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);

[snip]

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

index d96e742..b77b813 100644
@@ -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);

index 153d3fc..d6fde98 100644
@@ -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);