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  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]
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

Powered by blists - more mailing lists