linux-kernel - Re: [PATCH] Revert "exec: make de_thread() freezable (was: Re: Linux 4.20-rc4)

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]

Date:   Mon, 03 Dec 2018 13:00:41 +0100
From:   "Rafael J. Wysocki" <rjw@...ysocki.net>
To:     Ingo Molnar <mingo@...nel.org>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Linux List Kernel Mailing <linux-kernel@...r.kernel.org>,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        Chanho Min <chanho.min@....com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <a.p.zijlstra@...llo.nl>,
        Oleg Nesterov <oleg@...hat.com>, Pavel Machek <pavel@....cz>,
        Michal Hocko <mhocko@...e.com>
Subject: Re: [PATCH] Revert "exec: make de_thread() freezable (was: Re: Linux 4.20-rc4)

On Monday, December 3, 2018 8:47:00 AM CET Ingo Molnar wrote:
> 
> * Linus Torvalds <torvalds@...ux-foundation.org> wrote:
> 
> > The patch stats this week look a little bit more normal than last tim,
> > probably simply because it's also a normal-sized rc4 rather than the
> > unusually small rc3.
> 
> So there's a new regression in v4.20-rc4, my desktop produces this 
> lockdep splat:
> 
> [ 1772.588771] WARNING: pkexec/4633 still has locks held!
> [ 1772.588773] 4.20.0-rc4-custom-00213-g93a49841322b #1 Not tainted
> [ 1772.588775] ------------------------------------
> [ 1772.588776] 1 lock held by pkexec/4633:
> [ 1772.588778]  #0: 00000000ed85fbf8 (&sig->cred_guard_mutex){+.+.}, at: prepare_bprm_creds+0x2a/0x70
> [ 1772.588786] stack backtrace:
> [ 1772.588789] CPU: 7 PID: 4633 Comm: pkexec Not tainted 4.20.0-rc4-custom-00213-g93a49841322b #1
> [ 1772.588792] Call Trace:
> [ 1772.588800]  dump_stack+0x85/0xcb
> [ 1772.588803]  flush_old_exec+0x116/0x890
> [ 1772.588807]  ? load_elf_phdrs+0x72/0xb0
> [ 1772.588809]  load_elf_binary+0x291/0x1620
> [ 1772.588815]  ? sched_clock+0x5/0x10
> [ 1772.588817]  ? search_binary_handler+0x6d/0x240
> [ 1772.588820]  search_binary_handler+0x80/0x240
> [ 1772.588823]  load_script+0x201/0x220
> [ 1772.588825]  search_binary_handler+0x80/0x240
> [ 1772.588828]  __do_execve_file.isra.32+0x7d2/0xa60
> [ 1772.588832]  ? strncpy_from_user+0x40/0x180
> [ 1772.588835]  __x64_sys_execve+0x34/0x40
> [ 1772.588838]  do_syscall_64+0x60/0x1c0
> 
> The warning gets triggered by an ancient lockdep check in the freezer:
> 
> (gdb) list *0xffffffff812ece06
> 0xffffffff812ece06 is in flush_old_exec (./include/linux/freezer.h:57).
> 52	 * DO NOT ADD ANY NEW CALLERS OF THIS FUNCTION
> 53	 * If try_to_freeze causes a lockdep warning it means the caller may deadlock
> 54	 */
> 55	static inline bool try_to_freeze_unsafe(void)
> 56	{
> 57		might_sleep();
> 58		if (likely(!freezing(current)))
> 59			return false;
> 60		return __refrigerator(false);
> 61	}
> 
> I reviewed the ->cred_guard_mutex code, and the mutex is held across all 
> of exec() - and we always did this.
> 
> But there's this recent -rc4 commit:
> 
> > Chanho Min (1):
> >       exec: make de_thread() freezable
> 
>   c22397888f1e: exec: make de_thread() freezable
> 
> I believe this commit is bogus, you cannot call try_to_freeze() from 
> de_thread(), because it's holding the ->cred_guard_mutex.
> 
> Also, I'm 3 times grumpy:

Well, sorry about that.

>  #1: I think this commit was never tested with lockdep turned on, as I 
>      think splat this should trigger 100% of the time with the ELF 
>      binfmt loader.

It has been there in my local builds/tests, so I must have overlooked the
splat.

>  #2: Less than 4 days passed between the commit on Nov 19 and it being 
>      upstream by Nov 23. *Please* give it more testing if you change 
>      something as central as fs/exec.c ...

It has been under review for quite a bit more time though and I had hoped
that there would be some reports from linux-next coverage if there were
problems with it, but nothig has been reported till now.

>  #3  Also, please also proof-read changelogs before committing them:

I do that as a rule and I tend to fix up such things in changelogs.

Not sure why I didn't do that this time.  Because of travel perhaps.

>      >>  The casue is that de_thread() sleeps in TASK_UNINTERRUPTIBLE waiting for
>      >>  [...]
>      >>
>      >>  In our machine, it causes freeze timeout as bellows.
> 
>      That's *three* typos in a single commit:
> 
>         s/casue/cause
>         s/In our/On our
>         s/bellows/below
> 
>      ...
> 
> Grump! :-)
> 
> Note that I haven't tested the revert yet, but the code and the breakage 
> looks pretty obvious. (I'll boot the revert, will follow up if that 
> didn't solve the problem.)

I can queue up a revert unless anyone beats me to that.

Thanks,
Rafael