linux-kernel - Re: [PATCH 1/5] Split wait_noreap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 21 May 2009 16:12:50 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Ingo Molnar <mingo@...e.hu>
Cc:	Vitaly Mayatskikh <v.mayatskih@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Roland McGrath <roland@...hat.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/5] Split wait_noreap_copyout()

On 05/20, Ingo Molnar wrote:
>
> * Vitaly Mayatskikh <v.mayatskih@...il.com> wrote:
>
> > 2. Fix copy_wait_opts_to_user to old behaviour by something like:
> >
> > 	if (!retval && (infop || WNOWAIT)) {
> >
> > What's your opinion?
>
> I'd suggest a variant of 2: keep this large-ish patch an equivalent
> transformation - i.e. an impact: cleanup type of change.
>
> Then queue up a patch that removes this quirk.

Yes, this would be the best option.

The problem is, it is not trivial to keep the current behaviour and
make the patch which looks like a cleanup, not uglification.

copy_wait_opts_to_user() needs the new "called_from_wait_task_continued"
argument, and it should do

	if (called_from_wait_task_continued && !wo->wo_info)
		return -EFAULT;

Or we should add

	if (!infop && WNOWAIT)
		return -EFAULT;

to all callers except wait_task_continued().


Roland thinks that "-EFAULT -> success" change is acceptable, and I think
the same. So, to me the best option is just  change the changelog of this
patch and that is all.

Or. We can make a trivial patch which adds the behavior change first:

	Changelog: always accept the NULL infop, because it is not
	possible to understand the current behaviour ;)

	User-visible change! needs Acks!

	--- a/kernel/exit.c
	+++ b/kernel/exit.c
	@@ -1126,24 +1126,26 @@ static int eligible_child(struct wait_op
	 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 = wo->wo_rusage
			? getrusage(p, RUSAGE_BOTH, wo->wo_rusage) : 0;
	-
		put_task_struct(p);
	-	infop = wo->wo_info;
	-	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);
	+
	+	if (wo->wo_info) {
	+		struct siginfo __user *infop = wo->wo_info;
	+
	+		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);
	+	}
		if (!retval)
			retval = pid;
		return retval;

And then redo Vitaly's patches on top of this change.

What do you and Vitaly think?

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/