linux-kernel - Re: proc_flush

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: <87po78trjm.fsf@xmission.com>
Date:   Thu, 21 Dec 2017 02:26:53 -0600
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     Dave Jones <davej@...emonkey.org.uk>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Al Viro <viro@...iv.linux.org.uk>,
        Linux Kernel <linux-kernel@...r.kernel.org>,
        syzkaller-bugs@...glegroups.com, Gargi Sharma <gs051095@...il.com>,
        Alexey Dobriyan <adobriyan@...il.com>,
        Oleg Nesterov <oleg@...hat.com>,
        Rik van Riel <riel@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: proc_flush_task oops

Dave Jones <davej@...emonkey.org.uk> writes:

> On Wed, Dec 20, 2017 at 12:25:52PM -0600, Eric W. Biederman wrote:
>> >  > 
>  > >  > If the warning triggers it means the bug is in alloc_pid and somehow
>  > >  > something has gotten past the is_child_reaper check.
>  > >
>  > > You're onto something.
>  > >
>  > I am not seeing where things go wrong, but that puts the recent pid bitmap, bit
>  > hash to idr change in the suspect zone.
>  > 
>  > Can you try reverting that change:
>  > 
>  > e8cfbc245e24 ("pid: remove pidhash")
>  > 95846ecf9dac ("pid: replace pid bitmap implementation with IDR API")
>  > 
>  > While keeping the warning in place so we can see if this fixes the
>  > allocation problem?
>
> So I can't trigger this any more with those reverted.  I seem to hit a
> bunch of other long-standing bugs first.  I'll keep running it
> overnight, but it looks like this is where the problem lies.

I would really like to hear from the people who made this change if they
are interested in tracking down this failure.

It might be as simple as the locking changed enough that the locking
instrumentation is now slowing things down, and opening up an old race.

I have stared at this code, and written some test programs and I can't
see what is going on.  alloc_pid by design and in implementation (as far
as I can see) is always single threaded when allocating the first pid
in a pid namespace.  idr_init always initialized idr_next to 0.

So how we can get past:

	if (unlikely(is_child_reaper(pid))) {
		if (pid_ns_prepare_proc(ns)) {
			disable_pid_allocation(ns);
			goto out_free;
		}
	}

with proc_mnt still set to NULL is a mystery to me.

Is there any chance the idr code doesn't always return the lowest valid
free number?  So init gets assigned something other than 1?

Eric