On Monday, December 3, 2018 8:47:00 AM CET Ingo Molnar wrote: > > * Linus Torvalds <[email protected]> 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
