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

Groups > linux.kernel > #1642106 > unrolled thread

[PATCH 1/8] move compat wait4 and waitid next to native variants

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

  [PATCH 1/8] move compat wait4 and waitid next to native variants Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-16 00:40 +0200
    [PATCH 3/8] kernel_wait4()/kernel_waitid(): delay copying status to userland Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-16 00:40 +0200
    [PATCH 4/8] waitid(2): leave copyout of siginfo to syscall itself Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-16 00:40 +0200
      Re: [PATCH 4/8] waitid(2): leave copyout of siginfo to syscall itself Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-16 01:10 +0200
        Re: [PATCH 4/8] waitid(2): leave copyout of siginfo to syscall itself Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-16 01:50 +0200
          Re: [PATCH 4/8] waitid(2): leave copyout of siginfo to syscall itself ebiederm@xmission.com (Eric W. Biederman) - 2017-05-17 22:00 +0200

#1642106 — [PATCH 1/8] move compat wait4 and waitid next to native variants

From: Al Viro <viro@zeniv.linux.org.uk>

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 kernel/compat.c | 66 ------------------------------------------------------
 kernel/exit.c   | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+), 66 deletions(-)

diff --git a/kernel/compat.c b/kernel/compat.c
index 933bcb31ae10..b4cdba6bbd02 100644
--- a/kernel/compat.c
+++ b/kernel/compat.c
@@ -543,72 +543,6 @@ int put_compat_rusage(const struct rusage *r, struct compat_rusage __user *ru)
 	return 0;
 }
 
-COMPAT_SYSCALL_DEFINE4(wait4,
-	compat_pid_t, pid,
-	compat_uint_t __user *, stat_addr,
-	int, options,
-	struct compat_rusage __user *, ru)
-{
-	if (!ru) {
-		return sys_wait4(pid, stat_addr, options, NULL);
-	} else {
-		struct rusage r;
-		int ret;
-		unsigned int status;
-		mm_segment_t old_fs = get_fs();
-
-		set_fs (KERNEL_DS);
-		ret = sys_wait4(pid,
-				(stat_addr ?
-				 (unsigned int __user *) &status : NULL),
-				options, (struct rusage __user *) &r);
-		set_fs (old_fs);
-
-		if (ret > 0) {
-			if (put_compat_rusage(&r, ru))
-				return -EFAULT;
-			if (stat_addr && put_user(status, stat_addr))
-				return -EFAULT;
-		}
-		return ret;
-	}
-}
-
-COMPAT_SYSCALL_DEFINE5(waitid,
-		int, which, compat_pid_t, pid,
-		struct compat_siginfo __user *, uinfo, int, options,
-		struct compat_rusage __user *, uru)
-{
-	siginfo_t info;
-	struct rusage ru;
-	long ret;
-	mm_segment_t old_fs = get_fs();
-
-	memset(&info, 0, sizeof(info));
-
-	set_fs(KERNEL_DS);
-	ret = sys_waitid(which, pid, (siginfo_t __user *)&info, options,
-			 uru ? (struct rusage __user *)&ru : NULL);
-	set_fs(old_fs);
-
-	if ((ret < 0) || (info.si_signo == 0))
-		return ret;
-
-	if (uru) {
-		/* sys_waitid() overwrites everything in ru */
-		if (COMPAT_USE_64BIT_TIME)
-			ret = copy_to_user(uru, &ru, sizeof(ru));
-		else
-			ret = put_compat_rusage(&ru, uru);
-		if (ret)
-			return -EFAULT;
-	}
-
-	BUG_ON(info.si_code & __SI_MASK);
-	info.si_code |= __SI_CHLD;
-	return copy_siginfo_to_user32(uinfo, &info);
-}
-
 static int compat_get_user_cpu_mask(compat_ulong_t __user *user_mask_ptr,
 				    unsigned len, struct cpumask *new_mask)
 {
diff --git a/kernel/exit.c b/kernel/exit.c
index 516acdb0e0ec..197878103b25 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -62,6 +62,7 @@
 #include <linux/kcov.h>
 #include <linux/random.h>
 #include <linux/rcuwait.h>
+#include <linux/compat.h>
 
 #include <linux/uaccess.h>
 #include <asm/unistd.h>
@@ -1735,3 +1736,71 @@ SYSCALL_DEFINE3(waitpid, pid_t, pid, int __user *, stat_addr, int, options)
 }
 
 #endif
+
+#ifdef CONFIG_COMPAT
+COMPAT_SYSCALL_DEFINE4(wait4,
+	compat_pid_t, pid,
+	compat_uint_t __user *, stat_addr,
+	int, options,
+	struct compat_rusage __user *, ru)
+{
+	if (!ru) {
+		return sys_wait4(pid, stat_addr, options, NULL);
+	} else {
+		struct rusage r;
+		int ret;
+		unsigned int status;
+		mm_segment_t old_fs = get_fs();
+
+		set_fs (KERNEL_DS);
+		ret = sys_wait4(pid,
+				(stat_addr ?
+				 (unsigned int __user *) &status : NULL),
+				options, (struct rusage __user *) &r);
+		set_fs (old_fs);
+
+		if (ret > 0) {
+			if (put_compat_rusage(&r, ru))
+				return -EFAULT;
+			if (stat_addr && put_user(status, stat_addr))
+				return -EFAULT;
+		}
+		return ret;
+	}
+}
+
+COMPAT_SYSCALL_DEFINE5(waitid,
+		int, which, compat_pid_t, pid,
+		struct compat_siginfo __user *, uinfo, int, options,
+		struct compat_rusage __user *, uru)
+{
+	siginfo_t info;
+	struct rusage ru;
+	long ret;
+	mm_segment_t old_fs = get_fs();
+
+	memset(&info, 0, sizeof(info));
+
+	set_fs(KERNEL_DS);
+	ret = sys_waitid(which, pid, (siginfo_t __user *)&info, options,
+			 uru ? (struct rusage __user *)&ru : NULL);
+	set_fs(old_fs);
+
+	if ((ret < 0) || (info.si_signo == 0))
+		return ret;
+
+	if (uru) {
+		/* sys_waitid() overwrites everything in ru */
+		if (COMPAT_USE_64BIT_TIME)
+			ret = copy_to_user(uru, &ru, sizeof(ru));
+		else
+			ret = put_compat_rusage(&ru, uru);
+		if (ret)
+			return -EFAULT;
+	}
+
+	BUG_ON(info.si_code & __SI_MASK);
+	info.si_code |= __SI_CHLD;
+	return copy_siginfo_to_user32(uinfo, &info);
+}
+#endif
-- 
2.11.0

[toc] | [next] | [standalone]

#1642108 — [PATCH 3/8] kernel_wait4()/kernel_waitid(): delay copying status to userland

From: Al Viro <viro@zeniv.linux.org.uk>

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 kernel/exit.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index b3c141a4432f..574e6b04f838 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1002,7 +1002,7 @@ struct wait_opts {
 	struct pid		*wo_pid;
 
 	struct siginfo __user	*wo_info;
-	int __user		*wo_stat;
+	int			wo_stat;
 	struct rusage		*wo_rusage;
 
 	wait_queue_t		child_wait;
@@ -1189,8 +1189,7 @@ static int wait_task_zombie(struct wait_opts *wo, struct task_struct *p)
 	retval = 0;
 	status = (p->signal->flags & SIGNAL_GROUP_EXIT)
 		? p->signal->group_exit_code : p->exit_code;
-	if (!retval && wo->wo_stat)
-		retval = put_user(status, wo->wo_stat);
+	wo->wo_stat = status;
 
 	infop = wo->wo_info;
 	if (!retval && infop)
@@ -1322,8 +1321,7 @@ static int wait_task_stopped(struct wait_opts *wo,
 	if (wo->wo_rusage)
 		getrusage(p, RUSAGE_BOTH, wo->wo_rusage);
 	retval = 0;
-	if (!retval && wo->wo_stat)
-		retval = put_user((exit_code << 8) | 0x7f, wo->wo_stat);
+	wo->wo_stat = (exit_code << 8) | 0x7f;
 
 	infop = wo->wo_info;
 	if (!retval && infop)
@@ -1383,12 +1381,9 @@ static int wait_task_continued(struct wait_opts *wo, struct task_struct *p)
 	if (!wo->wo_info) {
 		if (wo->wo_rusage)
 			getrusage(p, RUSAGE_BOTH, wo->wo_rusage);
-		retval = 0;
 		put_task_struct(p);
-		if (!retval && wo->wo_stat)
-			retval = put_user(0xffff, wo->wo_stat);
-		if (!retval)
-			retval = pid;
+		wo->wo_stat = 0xffff;
+		retval = pid;
 	} else {
 		retval = wait_noreap_copyout(wo, p, pid, uid,
 					     CLD_CONTINUED, SIGCONT);
@@ -1662,7 +1657,6 @@ static long kernel_waitid(int which, pid_t upid, struct siginfo __user *infop,
 	wo.wo_pid	= pid;
 	wo.wo_flags	= options;
 	wo.wo_info	= infop;
-	wo.wo_stat	= NULL;
 	wo.wo_rusage	= ru;
 	ret = do_wait(&wo);
 
@@ -1734,10 +1728,12 @@ static long kernel_wait4(pid_t upid, int __user *stat_addr,
 	wo.wo_pid	= pid;
 	wo.wo_flags	= options | WEXITED;
 	wo.wo_info	= NULL;
-	wo.wo_stat	= stat_addr;
+	wo.wo_stat	= 0;
 	wo.wo_rusage	= ru;
 	ret = do_wait(&wo);
 	put_pid(pid);
+	if (ret > 0 && stat_addr && put_user(wo.wo_stat, stat_addr))
+		ret = -EFAULT;
 
 	return ret;
 }
-- 
2.11.0

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

#1642109 — [PATCH 4/8] waitid(2): leave copyout of siginfo to syscall itself

From: Al Viro <viro@zeniv.linux.org.uk>

have kernel_waitid() collect the information needed for siginfo into
a small structure (waitid_info) passed to it; deal with copyout in
sys_waitid()/compat_sys_waitid().

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 kernel/exit.c | 168 ++++++++++++++++++++++------------------------------------
 1 file changed, 64 insertions(+), 104 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index 574e6b04f838..9ea7c0d4cb37 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -996,12 +996,19 @@ SYSCALL_DEFINE1(exit_group, int, error_code)
 	return 0;
 }
 
+struct waitid_info {
+	pid_t pid;
+	uid_t uid;
+	int status;
+	int why;
+};
+
 struct wait_opts {
 	enum pid_type		wo_type;
 	int			wo_flags;
 	struct pid		*wo_pid;
 
-	struct siginfo __user	*wo_info;
+	struct waitid_info	*wo_info;
 	int			wo_stat;
 	struct rusage		*wo_rusage;
 
@@ -1053,8 +1060,7 @@ eligible_child(struct wait_opts *wo, bool ptrace, struct task_struct *p)
 static int wait_noreap_copyout(struct wait_opts *wo, struct task_struct *p,
 				pid_t pid, uid_t uid, int why, int status)
 {
-	struct siginfo __user *infop;
-	int retval = 0;
+	struct waitid_info *infop;
 
 	if (wo->wo_rusage)
 		getrusage(p, RUSAGE_BOTH, wo->wo_rusage);
@@ -1062,22 +1068,12 @@ static int wait_noreap_copyout(struct wait_opts *wo, struct task_struct *p,
 	put_task_struct(p);
 	infop = wo->wo_info;
 	if (infop) {
-		if (!retval)
-			retval = put_user(SIGCHLD, &infop->si_signo);
-		if (!retval)
-			retval = put_user(0, &infop->si_errno);
-		if (!retval)
-			retval = put_user((short)why, &infop->si_code);
-		if (!retval)
-			retval = put_user(pid, &infop->si_pid);
-		if (!retval)
-			retval = put_user(uid, &infop->si_uid);
-		if (!retval)
-			retval = put_user(status, &infop->si_status);
+		infop->why = why;
+		infop->pid = pid;
+		infop->uid = uid;
+		infop->status = status;
 	}
-	if (!retval)
-		retval = pid;
-	return retval;
+	return pid;
 }
 
 /*
@@ -1088,10 +1084,10 @@ static int wait_noreap_copyout(struct wait_opts *wo, struct task_struct *p,
  */
 static int wait_task_zombie(struct wait_opts *wo, struct task_struct *p)
 {
-	int state, retval, status;
+	int state, status;
 	pid_t pid = task_pid_vnr(p);
 	uid_t uid = from_kuid_munged(current_user_ns(), task_uid(p));
-	struct siginfo __user *infop;
+	struct waitid_info *infop;
 
 	if (!likely(wo->wo_flags & WEXITED))
 		return 0;
@@ -1186,36 +1182,22 @@ static int wait_task_zombie(struct wait_opts *wo, struct task_struct *p)
 
 	if (wo->wo_rusage)
 		getrusage(p, RUSAGE_BOTH, wo->wo_rusage);
-	retval = 0;
 	status = (p->signal->flags & SIGNAL_GROUP_EXIT)
 		? p->signal->group_exit_code : p->exit_code;
 	wo->wo_stat = status;
 
 	infop = wo->wo_info;
-	if (!retval && infop)
-		retval = put_user(SIGCHLD, &infop->si_signo);
-	if (!retval && infop)
-		retval = put_user(0, &infop->si_errno);
-	if (!retval && infop) {
-		int why;
-
+	if (infop) {
 		if ((status & 0x7f) == 0) {
-			why = CLD_EXITED;
-			status >>= 8;
+			infop->why = CLD_EXITED;
+			infop->status = status >> 8;
 		} else {
-			why = (status & 0x80) ? CLD_DUMPED : CLD_KILLED;
-			status &= 0x7f;
+			infop->why = (status & 0x80) ? CLD_DUMPED : CLD_KILLED;
+			infop->status = status & 0x7f;
 		}
-		retval = put_user((short)why, &infop->si_code);
-		if (!retval)
-			retval = put_user(status, &infop->si_status);
+		infop->pid = pid;
+		infop->uid = uid;
 	}
-	if (!retval && infop)
-		retval = put_user(pid, &infop->si_pid);
-	if (!retval && infop)
-		retval = put_user(uid, &infop->si_uid);
-	if (!retval)
-		retval = pid;
 
 	if (state == EXIT_TRACE) {
 		write_lock_irq(&tasklist_lock);
@@ -1232,7 +1214,7 @@ static int wait_task_zombie(struct wait_opts *wo, struct task_struct *p)
 	if (state == EXIT_DEAD)
 		release_task(p);
 
-	return retval;
+	return pid;
 }
 
 static int *task_stopped_code(struct task_struct *p, bool ptrace)
@@ -1268,8 +1250,8 @@ static int *task_stopped_code(struct task_struct *p, bool ptrace)
 static int wait_task_stopped(struct wait_opts *wo,
 				int ptrace, struct task_struct *p)
 {
-	struct siginfo __user *infop;
-	int retval, exit_code, *p_code, why;
+	struct waitid_info *infop;
+	int exit_code, *p_code, why;
 	uid_t uid = 0; /* unneeded, required by compiler */
 	pid_t pid;
 
@@ -1320,28 +1302,19 @@ static int wait_task_stopped(struct wait_opts *wo,
 
 	if (wo->wo_rusage)
 		getrusage(p, RUSAGE_BOTH, wo->wo_rusage);
-	retval = 0;
 	wo->wo_stat = (exit_code << 8) | 0x7f;
 
 	infop = wo->wo_info;
-	if (!retval && infop)
-		retval = put_user(SIGCHLD, &infop->si_signo);
-	if (!retval && infop)
-		retval = put_user(0, &infop->si_errno);
-	if (!retval && infop)
-		retval = put_user((short)why, &infop->si_code);
-	if (!retval && infop)
-		retval = put_user(exit_code, &infop->si_status);
-	if (!retval && infop)
-		retval = put_user(pid, &infop->si_pid);
-	if (!retval && infop)
-		retval = put_user(uid, &infop->si_uid);
-	if (!retval)
-		retval = pid;
+	if (infop) {
+		infop->why = why;
+		infop->status = exit_code;
+		infop->pid = pid;
+		infop->uid = uid;
+	}
 	put_task_struct(p);
 
-	BUG_ON(!retval);
-	return retval;
+	BUG_ON(!pid);
+	return pid;
 }
 
 /*
@@ -1618,7 +1591,7 @@ static long do_wait(struct wait_opts *wo)
 	return retval;
 }
 
-static long kernel_waitid(int which, pid_t upid, struct siginfo __user *infop,
+static long kernel_waitid(int which, pid_t upid, struct waitid_info *infop,
 			  int options, struct rusage *ru)
 {
 	struct wait_opts wo;
@@ -1660,27 +1633,8 @@ static long kernel_waitid(int which, pid_t upid, struct siginfo __user *infop,
 	wo.wo_rusage	= ru;
 	ret = do_wait(&wo);
 
-	if (ret > 0) {
+	if (ret > 0)
 		ret = 0;
-	} else if (infop) {
-		/*
-		 * For a WNOHANG return, clear out all the fields
-		 * we would set so the user can easily tell the
-		 * difference.
-		 */
-		if (!ret)
-			ret = put_user(0, &infop->si_signo);
-		if (!ret)
-			ret = put_user(0, &infop->si_errno);
-		if (!ret)
-			ret = put_user(0, &infop->si_code);
-		if (!ret)
-			ret = put_user(0, &infop->si_pid);
-		if (!ret)
-			ret = put_user(0, &infop->si_uid);
-		if (!ret)
-			ret = put_user(0, &infop->si_status);
-	}
 
 	put_pid(pid);
 	return ret;
@@ -1690,12 +1644,24 @@ SYSCALL_DEFINE5(waitid, int, which, pid_t, upid, struct siginfo __user *,
 		infop, int, options, struct rusage __user *, ru)
 {
 	struct rusage r;
-	long err = kernel_waitid(which, upid, infop, options, ru ? &r : NULL);
+	struct waitid_info info = {.status = 0};
+	long err = kernel_waitid(which, upid, &info, options, ru ? &r : NULL);
 
 	if (!err) {
 		if (ru && copy_to_user(ru, &r, sizeof(struct rusage)))
 			return -EFAULT;
 	}
+	if (!infop)
+		return err;
+
+	if (put_user(err ? 0 : SIGCHLD, &infop->si_signo) ||
+	    put_user(0, &infop->si_errno) ||
+	    put_user((short)info.why, &infop->si_code) ||
+	    put_user(info.pid, &infop->si_pid) ||
+	    put_user(info.uid, &infop->si_uid) ||
+	    put_user(info.status, &infop->si_status))
+		err = -EFAULT;
+
 	return err;
 }
 
@@ -1785,33 +1751,27 @@ COMPAT_SYSCALL_DEFINE5(waitid,
 		struct compat_siginfo __user *, uinfo, int, options,
 		struct compat_rusage __user *, uru)
 {
-	siginfo_t info;
 	struct rusage ru;
-	long ret;
-	mm_segment_t old_fs = get_fs();
-
-	memset(&info, 0, sizeof(info));
+	struct waitid_info info = {.status = 0};
+	long err = kernel_waitid(which, pid, &info, options, uru ? &ru : NULL);
 
-	set_fs(KERNEL_DS);
-	ret = kernel_waitid(which, pid, (siginfo_t __user *)&info, options,
-			 uru ? &ru : NULL);
-	set_fs(old_fs);
-
-	if ((ret < 0) || (info.si_signo == 0))
-		return ret;
-
-	if (uru) {
-		/* sys_waitid() overwrites everything in ru */
+	if (!err && uru) {
+		/* kernel_waitid() overwrites everything in ru */
 		if (COMPAT_USE_64BIT_TIME)
-			ret = copy_to_user(uru, &ru, sizeof(ru));
+			err = copy_to_user(uru, &ru, sizeof(ru));
 		else
-			ret = put_compat_rusage(&ru, uru);
-		if (ret)
+			err = put_compat_rusage(&ru, uru);
+		if (err)
 			return -EFAULT;
 	}
 
-	BUG_ON(info.si_code & __SI_MASK);
-	info.si_code |= __SI_CHLD;
-	return copy_siginfo_to_user32(uinfo, &info);
+	if (put_user(err ? 0 : SIGCHLD, &uinfo->si_signo) ||
+	    put_user(0, &uinfo->si_errno) ||
+	    put_user((short)info.why, &uinfo->si_code) ||
+	    put_user(info.pid, &uinfo->si_pid) ||
+	    put_user(info.uid, &uinfo->si_uid) ||
+	    put_user(info.status, &uinfo->si_status))
+		err = -EFAULT;
+	return err;
 }
 #endif
-- 
2.11.0

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

#1642114 — Re: [PATCH 4/8] waitid(2): leave copyout of siginfo to syscall itself

On Mon, May 15, 2017 at 3:37 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> +struct waitid_info {
> +       pid_t pid;
> +       uid_t uid;
> +       int status;
> +       int why;
> +};

Ugh. Could we please just name those with what they are actually used for?

Even if you hate the "si_" previx for some reason, I really don't see
why we'd continue call it "why", when it's written to "si_code"

Yes, yes, I see the historical reason, and how "si_code" is just the
low 16 bits of "why", and the high 16 bits is something else.

But now that there is a structure for that, could we not just make
that explicit in the structure instead? Those games with "why" look
really odd.

So I can see why you'd like to keep this patch as "minimal
conversion", but it would be really nice to have a followup patch that
gets rid of the odd "why" games.

             Linus

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

#1642131 — Re: [PATCH 4/8] waitid(2): leave copyout of siginfo to syscall itself

On Mon, May 15, 2017 at 04:06:49PM -0700, Linus Torvalds wrote:
> On Mon, May 15, 2017 at 3:37 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > +struct waitid_info {
> > +       pid_t pid;
> > +       uid_t uid;
> > +       int status;
> > +       int why;
> > +};
> 
> Ugh. Could we please just name those with what they are actually used for?
> 
> Even if you hate the "si_" previx for some reason, I really don't see
> why we'd continue call it "why", when it's written to "si_code"
> 
> Yes, yes, I see the historical reason, and how "si_code" is just the
> low 16 bits of "why", and the high 16 bits is something else.

__SI_CHLD, and AFAICS it only matters for copy_siginfo_to_user() and its
relatives - basically, "how much of kernel-side struct siginfo do we have
initialized"...

> But now that there is a structure for that, could we not just make
> that explicit in the structure instead? Those games with "why" look
> really odd.

OK...

> So I can see why you'd like to keep this patch as "minimal
> conversion", but it would be really nice to have a followup patch that
> gets rid of the odd "why" games.

The thing is, we lack convenient defines for those constants.  We could
turn this "why" thing into u16 si_code, but then gcc will scream about
integer constant truncation ;-/  Suggestions?

BTW, I wonder if making those stores conditional is actually a win -
sure, for put_user() it used to be, but for plain stores...  Not sure.

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

#1643606 — Re: [PATCH 4/8] waitid(2): leave copyout of siginfo to syscall itself

Al Viro <viro@ZenIV.linux.org.uk> writes:

> On Mon, May 15, 2017 at 04:06:49PM -0700, Linus Torvalds wrote:
>> On Mon, May 15, 2017 at 3:37 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>> >
>> > +struct waitid_info {
>> > +       pid_t pid;
>> > +       uid_t uid;
>> > +       int status;
>> > +       int why;
>> > +};
>> 
>> Ugh. Could we please just name those with what they are actually used for?
>> 
>> Even if you hate the "si_" previx for some reason, I really don't see
>> why we'd continue call it "why", when it's written to "si_code"
>> 
>> Yes, yes, I see the historical reason, and how "si_code" is just the
>> low 16 bits of "why", and the high 16 bits is something else.
>
> __SI_CHLD, and AFAICS it only matters for copy_siginfo_to_user() and its
> relatives - basically, "how much of kernel-side struct siginfo do we have
> initialized"...
>
>> But now that there is a structure for that, could we not just make
>> that explicit in the structure instead? Those games with "why" look
>> really odd.
>
> OK...
>
>> So I can see why you'd like to keep this patch as "minimal
>> conversion", but it would be really nice to have a followup patch that
>> gets rid of the odd "why" games.
>
> The thing is, we lack convenient defines for those constants.  We could
> turn this "why" thing into u16 si_code, but then gcc will scream about
> integer constant truncation ;-/  Suggestions?

Hmm.  Given the small size of that thing I think I would just
include embed waidtid_info in wait_opts as you have done earlier with wo_stat;

Which would mean at the end of do_wait you can just do:

	wo->wo_info.si_code &= 0xffff;

Neither SI_TIMER nor SI_MESGQ are being used so sign extension
is not needed.

As for getting magic out of the upper bits of si_code I suspect
we can just switch on si_signo and then for the realtime signals
si_code to get the layout in copy_siginfo_to_user.

The compiler just uses a binary tree of jumps so I don't expect the code
generated for copy_signinfo_to_user would be much worse and I do expect
with the magic taken out the logic of the code would be easier to
understand.

Alternately we could use some kind of ksiginfo that does not take up 128
bytes (except in the case of rt_sigqueueinfo) and being kernel internal
only has a format that is easy to decode for copy_siginfo_to_user.

> BTW, I wonder if making those stores conditional is actually a win -
> sure, for put_user() it used to be, but for plain stores...  Not sure.

My guess would be that storing to a small structure on the stack would
be almost free, and with all of the fields unconditionally present in
struct wait_opts the code could compute the values directly into wo
without needing intermediate varriables.    Which ought to make
the code easier to understand and maintain if not cycle by cycle faster.

Eric

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web