linux-kernel - Re: epoll oops.

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: <20131015154838.GA32271@redhat.com>
Date:	Tue, 15 Oct 2013 17:48:38 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Dave Jones <davej@...hat.com>
Cc:	Linux Kernel <linux-kernel@...r.kernel.org>,
	Al Viro <viro@...iv.linux.org.uk>,
	Davide Libenzi <davidel@...ilserver.org>,
	Eric Wong <normalperson@...t.net>,
	Pekka Enberg <penberg@...helsinki.fi>,
	Peter Hurley <peter@...leysoftware.com>
Subject: Re: epoll oops.

> Hmm? There might be other cases..

Yes.

Dave, perhaps you have vmcore? I have no idea if this is possible or
not, but perhaps you can look at eventpoll_release_file's frame and
print file->f_op ?

On 10/14, Linus Torvalds wrote:
>
> [ Adding Pekka to verify the SLAB_DESTROY_BY_RCU semantics

Just in case, we depend on SLAB_DESTROY_BY_RCU anyway, and ->sighand
in particular. lock_task_sighand() equally depends on it.

> Ok, Oleg, going back to that whole thread, I think that old bug went like this:

Yes, yes, thanks, I do remember what this patch does and why. Just
I forgot everything about eventpoll.c, I tried to read it only once
to fix that bug.

>  (b) signalfd is special, and it does a
>
>         poll_wait(file, &current->sighand->signalfd_wqh);
>
>      which means that the wait-queue isn't associated with the file
> lifetime at all. It cleans it up with signalfd_cleanup() if the signal
> handlers are removed. Normal (non-epoll) handling is safe, because
> "current->sighand" obviously cannot go away as long as the current
> thread (doing the polling) is in its poll/select handling.

Yes. and, just in case, the main problem is that sighand has no any
connection with the file. Unlike, say, tty which uses ->private_data.

>  (c) as a result, epoll and exit() can race, since the normal epoll
> cleanup() is serialized by the file being closed, and we're missing
> that for the case of sighand going away.

Yes. Before that 971316f0503a hack epoll can't even know if the task
which did signalfd_poll() exits and frees the active signalfd_wqh.
If for example that task forked a child before exit.

And the whole RCU logic is only needed if exit/ep_remove_wait_queue
actually race with each other.

> Agreed so far? Ugly, ugly, ugly,

Yes, ugly, agreed. d80e731ecab4 even tries to docunent that this all
is the hack.

> And it looks like it should work.

Yes... I tried to read this all again, and so far I do not see
anything wrong... signalfd_cleanup()->waitqueue_active() looks fine
too, afaics. We do not need to clear ->whead unconditionally, the
only caller of ep_ptable_queue_proc() is signalfd_poll(), and we are
the last thread which can use this ->sighand.

> Peter? Does a tty hangup end up actually possibly freeing the tty
> struct? Looking at it, I'm starting to think that it only affects
> f_op, and the "struct tty" stays around, in which case this is all
> fine.

Of course I can't answer, but at first glance file_tty() should not go
away in this case... If nothing else, tty_release() expects tty != NULL,
and it seems that priv->tty is never changed.

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/