linux-kernel - Re: [PATCH 0/1] futex: check PF_KTHREAD rather than !p->mm to filter out kthreads

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20150206170425.GA7493@redhat.com>
Date:	Fri, 6 Feb 2015 18:04:25 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	Darren Hart <darren@...art.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Jerome Marchand <jmarchan@...hat.com>,
	Larry Woodman <lwoodman@...hat.com>,
	Mateusz Guzik <mguzik@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/1] futex: check PF_KTHREAD rather than !p->mm to
	filter out kthreads

Peter, I am spamming you again and again, but I didn't even start the
patches. It turns out I can do nothing until devconf.cz finishes next
week.

On 02/06, Peter Zijlstra wrote:
>
> On Thu, Feb 05, 2015 at 07:10:14PM +0100, Oleg Nesterov wrote:
>
> > So I think that in this case we either need to recheck that *uaddr is still the
> > same (and turn -ESRCH into -EAGAIN otherwise), or change handle_futex_death() to
> > serialize with X so that it can proceed and attach pi_state.
> >
> > No?
>
> I _think_ you're right, doing -ESRCH is wrong without first looking to
> see if uval changed and gained an FUTEX_OWNER_DIED.

OK, thanks.

> I don't think making handle_futex_death() wait on hb lock works because
> of the -EAGAIN loop releasing that lock.

I think this should work... EAGAIN loop will either notice the change in
*uaddr or it will attach to pi list successfully. But please ignore, even
if I am right I do not like this change too.

And there is another thing which looks like design bug to me. I understand
that it is too late and pointless to complain, probably we can't change the
current behaviour, but I simply can't resist...

Suppose that a task T takes a PI futex (non-robust) and exits. Another task
does futex(FUTEX_LOCK_PI).

Now. if futex() is called after T exits it returns -ESRCH, this is correct.
But if it is called before, it succeeds while (I think) it should not.
fixup_owner() treats pi_state->owner == NULL pretty much as "unlocked".

IOW,
	#include <stdio.h>
	#include <unistd.h>
	#include <sys/syscall.h>
	#include <sys/wait.h>
	#include <assert.h>

	#define FUTEX_LOCK_PI	6

	int main(void)
	{
		int mutex, pid, err;

		pid = fork();
		if (!pid) {
			sleep(1);
			return 0;
		}

		mutex = pid;
		err = syscall(__NR_futex, &mutex, FUTEX_LOCK_PI, 0,0,0);
		printf("err=%d %x -> %x %m\n", err, pid, mutex);

		assert(wait(NULL) == pid);
		return 0;
	}

I don't understand why syscall(FUTEX_LOCK_PI) succeeds in this case.
To me it should fail with -ESRCH, this would be much more consistent
imho.

And this means that "PI" implies "robust" to some degree. OK, may be
this is fine. But if this is fine, why we can't do the same if, say,
futex_find_get_task() returns NULL ?

To me the rule should be simple. If the owner dies the next LOCK_PI
should succeed if and only if the futex was robust.

Or at least this should not depend on timing.

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/