WARNING: syz-executor still has locks held!

WARNING: syz-executor still has locks held!

[syzbot]

Hello,

syzbot found the following crash on:

HEAD commit:    7c98a4261827 Merge tag 'ceph-for-4.20-rc4' of https://gith..
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=12d81015400000
kernel config:  https://syzkaller.appspot.com/x/.config?x=73e2bc0cb6463446
dashboard link: https://syzkaller.appspot.com/bug?extid=b70f2aabc707c69c9239
compiler:       gcc (GCC) 8.0.1 20180413 (experimental)
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=16c9e26d400000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=14e1234d400000

IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+b70f2aabc707c69c9239@syzkaller.appspotmail.com

audit: type=1400 audit(1543059836.603:35): avc:  denied  { map } for   
pid=6203 comm="bash" path="/bin/bash" dev="sda1" ino=1457  
scontext=unconfined_u:system_r:insmod_t:s0-s0:c0.c1023  
tcontext=system_u:object_r:file_t:s0 tclass=file permissive=1
audit: type=1400 audit(1543059843.273:36): avc:  denied  { map } for   
pid=6217 comm="syz-executor552" path="/root/syz-executor552394561"  
dev="sda1" ino=16483 scontext=unconfined_u:system_r:insmod_t:s0-s0:c0.c1023  
tcontext=unconfined_u:object_r:user_home_t:s0 tclass=file permissive=1

====================================
WARNING: syz-executor552/6218 still has locks held!
4.20.0-rc3+ #127 Not tainted
------------------------------------
1 lock held by syz-executor552/6218:
  #0: 000000004754a9eb (&sig->cred_guard_mutex){+.+.}, at:  
prepare_bprm_creds+0x53/0x120 fs/exec.c:1405

stack backtrace:
CPU: 0 PID: 6218 Comm: syz-executor552 Not tainted 4.20.0-rc3+ #127
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS  
Google 01/01/2011
Call Trace:
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0x244/0x39d lib/dump_stack.c:113
  print_held_locks_bug kernel/locking/lockdep.c:4362 [inline]
  debug_check_no_locks_held.cold.49+0x93/0x9f kernel/locking/lockdep.c:4368
  try_to_freeze include/linux/freezer.h:66 [inline]
  freezer_count include/linux/freezer.h:128 [inline]
  freezable_schedule include/linux/freezer.h:173 [inline]
  de_thread fs/exec.c:1115 [inline]
  flush_old_exec+0x1ea2/0x2480 fs/exec.c:1262
  load_elf_binary+0xa39/0x5620 fs/binfmt_elf.c:869
  search_binary_handler+0x17d/0x570 fs/exec.c:1654
  exec_binprm fs/exec.c:1696 [inline]
  __do_execve_file.isra.33+0x1661/0x25d0 fs/exec.c:1820
  do_execveat_common fs/exec.c:1867 [inline]
  do_execveat fs/exec.c:1895 [inline]
  __do_sys_execveat fs/exec.c:1976 [inline]
  __se_sys_execveat fs/exec.c:1968 [inline]
  __x64_sys_execveat+0xed/0x130 fs/exec.c:1968
  do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
  entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x445639
Code: e8 ec b8 02 00 48 83 c4 18 c3 0f 1f 80 00 00 00 00 48 89 f8 48 89 f7  
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff  
ff 0f 83 8b 12 fc ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007f5efaee7d18 EFLAGS: 00000246 ORIG_RAX: 0000000000000142
RAX: ffffffffffffffda RBX: 00000000006dac28 RCX: 0000000000445639
RDX: 0000000000000000 RSI: 0000000020000000 RDI: 0000000000000003
RBP: 00000000006dac20 R08: 0000000000001000 R09: 65732f636f72702f
R10: 0000000000000000 R11: 0000000000000246 R12: 00007f5efaee7d20
R13: 00000000006dac2c R14: 0000000020000280 R15: 00000000006dad2c


---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#bug-status-tracking for how to communicate with  
syzbot.
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches

Re: WARNING: syz-executor still has locks held!

[syzbot]

syzbot has bisected this bug to:

commit c22397888f1eed98cd59f0a88f2a5f6925f80e15
Author: Chanho Min <chanho.min@lge.com>
Date:   Mon Nov 12 03:54:45 2018 +0000

     exec: make de_thread() freezable

bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=14ee6217200000
start commit:   c2239788 exec: make de_thread() freezable
git tree:       upstream
final crash:    https://syzkaller.appspot.com/x/report.txt?x=16ee6217200000
console output: https://syzkaller.appspot.com/x/log.txt?x=12ee6217200000
kernel config:  https://syzkaller.appspot.com/x/.config?x=73e2bc0cb6463446
dashboard link: https://syzkaller.appspot.com/bug?extid=b70f2aabc707c69c9239
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=16c9e26d400000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=14e1234d400000

Reported-by: syzbot+b70f2aabc707c69c9239@syzkaller.appspotmail.com
Fixes: c2239788 ("exec: make de_thread() freezable")

Re: WARNING: syz-executor still has locks held!

[Michal Hocko]

On Wed 20-03-19 05:24:00, syzbot wrote:
> syzbot has bisected this bug to:
> 
> commit c22397888f1eed98cd59f0a88f2a5f6925f80e15
> Author: Chanho Min <chanho.min@lge.com>
> Date:   Mon Nov 12 03:54:45 2018 +0000
> 
>     exec: make de_thread() freezable
> 
> bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=14ee6217200000
> start commit:   c2239788 exec: make de_thread() freezable
> git tree:       upstream
> final crash:    https://syzkaller.appspot.com/x/report.txt?x=16ee6217200000
> console output: https://syzkaller.appspot.com/x/log.txt?x=12ee6217200000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=73e2bc0cb6463446
> dashboard link: https://syzkaller.appspot.com/bug?extid=b70f2aabc707c69c9239
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=16c9e26d400000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=14e1234d400000
> 
> Reported-by: syzbot+b70f2aabc707c69c9239@syzkaller.appspotmail.com
> Fixes: c2239788 ("exec: make de_thread() freezable")

Yes we do hold the cgred mutex while calling freezable_schedule but why
are we getting a warning is not really clear to me. The task should be
hidden from the freezer so why do we warn at all?
-- 
Michal Hocko
SUSE Labs

Re: WARNING: syz-executor still has locks held!

[Oleg Nesterov]

On 03/20, syzbot wrote:
>
> syzbot has bisected this bug to:
> 
> commit c22397888f1eed98cd59f0a88f2a5f6925f80e15
> Author: Chanho Min <chanho.min@lge.com>
> Date:   Mon Nov 12 03:54:45 2018 +0000
>
>     exec: make de_thread() freezable
>
> bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=14ee6217200000
> start commit:   c2239788 exec: make de_thread() freezable
> git tree:       upstream

Yes, this patch was wrong, but it was reverted by a72173ecfc6774cf2d55de9fb2
(Revert "exec: make de_thread() freezable")

> final crash:    https://syzkaller.appspot.com/x/report.txt?x=16ee6217200000
> console output: https://syzkaller.appspot.com/x/log.txt?x=12ee6217200000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=73e2bc0cb6463446
> dashboard link: https://syzkaller.appspot.com/bug?extid=b70f2aabc707c69c9239
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=16c9e26d400000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=14e1234d400000
> 
> Reported-by: syzbot+b70f2aabc707c69c9239@syzkaller.appspotmail.com
> Fixes: c2239788 ("exec: make de_thread() freezable")

Re: WARNING: syz-executor still has locks held!

[Oleg Nesterov]

On 03/20, Michal Hocko wrote:
>
> Yes we do hold the cgred mutex while calling freezable_schedule but why
> are we getting a warning is not really clear to me. The task should be
> hidden from the freezer so why do we warn at all?

try_to_freeze() calls debug_check_no_locks_held() and this makes sense.

Another task can sleep waiting for the same lock; in this case
try_to_freeze_tasks() will fail but it won't blame the lock holder exactly
because it is "hidden from the freezer".

Oleg.

Re: WARNING: syz-executor still has locks held!

[Michal Hocko]

On Wed 20-03-19 14:24:11, Oleg Nesterov wrote:
> On 03/20, Michal Hocko wrote:
> >
> > Yes we do hold the cgred mutex while calling freezable_schedule but why
> > are we getting a warning is not really clear to me. The task should be
> > hidden from the freezer so why do we warn at all?
> 
> try_to_freeze() calls debug_check_no_locks_held() and this makes sense.

Yes it does. But it already ignores PF_NOFREEZE tasks and I fail to see
why is PF_FREEZER_SKIP any different. Yes we know we are holding the
lock and we have discussed that left and right before the patch was
merged but it seems that skipping the task was the only viable option
to fix suspend issues as removing the cgred is way way too complicated.

That being said, I was arguing against the patch initially because it
was a hack but sometimes that is the only way viable path short term.
-- 
Michal Hocko
SUSE Labs

Re: WARNING: syz-executor still has locks held!

[Oleg Nesterov]

On 03/20, Michal Hocko wrote:
>
> On Wed 20-03-19 14:24:11, Oleg Nesterov wrote:
> > On 03/20, Michal Hocko wrote:
> > >
> > > Yes we do hold the cgred mutex while calling freezable_schedule but why
> > > are we getting a warning is not really clear to me. The task should be
> > > hidden from the freezer so why do we warn at all?
> >
> > try_to_freeze() calls debug_check_no_locks_held() and this makes sense.
>
> Yes it does. But it already ignores PF_NOFREEZE tasks and I fail to see
> why is PF_FREEZER_SKIP any different.

But they differ. PF_NOFREEZE is a "sticky" flag for kthreads. Set by default,
cleared by set_freezable() if you want a freezable kthread.

PF_FREEZER_SKIP means that a sleeping freezable task will call try_to_freeze()
right after schedule() returns, so try_to_freeze_tasks() can safely count it as
"already frozen".

> it seems that skipping the task was the only viable option
> to fix suspend issues

Yes, de_thread() should use freezable_schedule(), iow I hope we will reconsider
this (reverted) patch.

> as removing the cgred is way way too complicated.

We need to do this anyway, this leads to other more serious problems...

Oleg.

Re: WARNING: syz-executor still has locks held!

[Michal Hocko]

[Cc Ingo and Chanho Min - the thread starts here
http://lkml.kernel.org/r/0000000000004cdec6058485b2ce@google.com]

On Wed 20-03-19 16:00:54, Oleg Nesterov wrote:
> On 03/20, Michal Hocko wrote:
> >
> > On Wed 20-03-19 14:24:11, Oleg Nesterov wrote:
> > > On 03/20, Michal Hocko wrote:
> > > >
> > > > Yes we do hold the cgred mutex while calling freezable_schedule but why
> > > > are we getting a warning is not really clear to me. The task should be
> > > > hidden from the freezer so why do we warn at all?
> > >
> > > try_to_freeze() calls debug_check_no_locks_held() and this makes sense.
> >
> > Yes it does. But it already ignores PF_NOFREEZE tasks and I fail to see
> > why is PF_FREEZER_SKIP any different.
> 
> But they differ. PF_NOFREEZE is a "sticky" flag for kthreads. Set by default,
> cleared by set_freezable() if you want a freezable kthread.
> 
> PF_FREEZER_SKIP means that a sleeping freezable task will call try_to_freeze()
> right after schedule() returns, so try_to_freeze_tasks() can safely count it as
> "already frozen".

But the fundamental semantic is the same right? Both might be sitting on
locks that might interfere with other tasks and we should be _extra_
careful when using them. In an ideal world, none of them is really
needed.

So my question remains. Can we drop the warning for PF_FREEZER_SKIP
tasks as well?

> > it seems that skipping the task was the only viable option
> > to fix suspend issues
> 
> Yes, de_thread() should use freezable_schedule(), iow I hope we will reconsider
> this (reverted) patch.

As long as we do not have a better solution for the original problem
then revert just because of a pointless warning is not really ideal. I
am wondering why I do not see any of people acking the patch is in CC of
the revert.

> > as removing the cgred is way way too complicated.
> 
> We need to do this anyway, this leads to other more serious problems...

Yes but this is far away and it doesn't really seem like a stable tree
material either and I am pretty sure that people on older kernels would
like to not see suspend failures. Those are annoying as hell.
-- 
Michal Hocko
SUSE Labs

Re: WARNING: syz-executor still has locks held!

[Oleg Nesterov]

On 03/20, Michal Hocko wrote:
>
> [Cc Ingo and Chanho Min - the thread starts here
> http://lkml.kernel.org/r/0000000000004cdec6058485b2ce@google.com]
>
> On Wed 20-03-19 16:00:54, Oleg Nesterov wrote:
> > On 03/20, Michal Hocko wrote:
> > >
> > > On Wed 20-03-19 14:24:11, Oleg Nesterov wrote:
> > > > On 03/20, Michal Hocko wrote:
> > > > >
> > > > > Yes we do hold the cgred mutex while calling freezable_schedule but why
> > > > > are we getting a warning is not really clear to me. The task should be
> > > > > hidden from the freezer so why do we warn at all?
> > > >
> > > > try_to_freeze() calls debug_check_no_locks_held() and this makes sense.
> > >
> > > Yes it does. But it already ignores PF_NOFREEZE tasks and I fail to see
> > > why is PF_FREEZER_SKIP any different.
> >
> > But they differ. PF_NOFREEZE is a "sticky" flag for kthreads. Set by default,
> > cleared by set_freezable() if you want a freezable kthread.
> >
> > PF_FREEZER_SKIP means that a sleeping freezable task will call try_to_freeze()
> > right after schedule() returns, so try_to_freeze_tasks() can safely count it as
> > "already frozen".
>
> But the fundamental semantic is the same right? Both might be sitting on
> locks that might interfere with other tasks and we should be _extra_
> careful when using them. In an ideal world, none of them is really
> needed.

Ah, it seems that we misunderstood each other... see below.

> So my question remains. Can we drop the warning for PF_FREEZER_SKIP
> tasks as well?

But why? It is obviously wrong to call try_to_freeze() with a lock held.

Probably you meant the

	if (!(current->flags & PF_NOFREEZE))

check in try_to_freeze() when you said "already ignores PF_NOFREEZE tasks".

I am not sure we actually need this check, a PF_NOFREEZE kthread shouldn't
call try_to_freeze() at least directly.

However, note that freezing() will return false if PF_NOFREEZE is set, so
try_to_freeze() is nop in this case. Probably this is why PF_NOFREEZE is
also checked before debug_check_no_locks_held().

> > > as removing the cgred is way way too complicated.
> >
> > We need to do this anyway, this leads to other more serious problems...
>
> Yes but this is far away and it doesn't really seem like a stable tree
> material

strace -f can hang ;) so this is the stable material.

Oleg.

Re: WARNING: syz-executor still has locks held!

[Michal Hocko]

On Wed 20-03-19 18:30:42, Oleg Nesterov wrote:
> On 03/20, Michal Hocko wrote:
> >
> > [Cc Ingo and Chanho Min - the thread starts here
> > http://lkml.kernel.org/r/0000000000004cdec6058485b2ce@google.com]
> >
> > On Wed 20-03-19 16:00:54, Oleg Nesterov wrote:
> > > On 03/20, Michal Hocko wrote:
> > > >
> > > > On Wed 20-03-19 14:24:11, Oleg Nesterov wrote:
> > > > > On 03/20, Michal Hocko wrote:
> > > > > >
> > > > > > Yes we do hold the cgred mutex while calling freezable_schedule but why
> > > > > > are we getting a warning is not really clear to me. The task should be
> > > > > > hidden from the freezer so why do we warn at all?
> > > > >
> > > > > try_to_freeze() calls debug_check_no_locks_held() and this makes sense.
> > > >
> > > > Yes it does. But it already ignores PF_NOFREEZE tasks and I fail to see
> > > > why is PF_FREEZER_SKIP any different.
> > >
> > > But they differ. PF_NOFREEZE is a "sticky" flag for kthreads. Set by default,
> > > cleared by set_freezable() if you want a freezable kthread.
> > >
> > > PF_FREEZER_SKIP means that a sleeping freezable task will call try_to_freeze()
> > > right after schedule() returns, so try_to_freeze_tasks() can safely count it as
> > > "already frozen".
> >
> > But the fundamental semantic is the same right? Both might be sitting on
> > locks that might interfere with other tasks and we should be _extra_
> > careful when using them. In an ideal world, none of them is really
> > needed.
> 
> Ah, it seems that we misunderstood each other... see below.
> 
> > So my question remains. Can we drop the warning for PF_FREEZER_SKIP
> > tasks as well?
> 
> But why?

To drop the warning which led to the revert.

> It is obviously wrong to call try_to_freeze() with a lock held.

It is but the question what do we care about more. A task blocking
suspend so that the operation fails or a process being frozen with cgred
held. We have discussed that during the review of the original patch and
concluded that this is the only way for now AFAIR.

> Probably you meant the
> 
> 	if (!(current->flags & PF_NOFREEZE))
> 
> check in try_to_freeze() when you said "already ignores PF_NOFREEZE tasks".

Yes, that is the check I've had in mind.
-- 
Michal Hocko
SUSE Labs

Re: WARNING: syz-executor still has locks held!

[Michal Hocko]

On Wed 20-03-19 20:45:12, Michal Hocko wrote:
> On Wed 20-03-19 18:30:42, Oleg Nesterov wrote:
> > On 03/20, Michal Hocko wrote:
> > >
> > > [Cc Ingo and Chanho Min - the thread starts here
> > > http://lkml.kernel.org/r/0000000000004cdec6058485b2ce@google.com]
> > >
> > > On Wed 20-03-19 16:00:54, Oleg Nesterov wrote:
> > > > On 03/20, Michal Hocko wrote:
> > > > >
> > > > > On Wed 20-03-19 14:24:11, Oleg Nesterov wrote:
> > > > > > On 03/20, Michal Hocko wrote:
> > > > > > >
> > > > > > > Yes we do hold the cgred mutex while calling freezable_schedule but why
> > > > > > > are we getting a warning is not really clear to me. The task should be
> > > > > > > hidden from the freezer so why do we warn at all?
> > > > > >
> > > > > > try_to_freeze() calls debug_check_no_locks_held() and this makes sense.
> > > > >
> > > > > Yes it does. But it already ignores PF_NOFREEZE tasks and I fail to see
> > > > > why is PF_FREEZER_SKIP any different.
> > > >
> > > > But they differ. PF_NOFREEZE is a "sticky" flag for kthreads. Set by default,
> > > > cleared by set_freezable() if you want a freezable kthread.
> > > >
> > > > PF_FREEZER_SKIP means that a sleeping freezable task will call try_to_freeze()
> > > > right after schedule() returns, so try_to_freeze_tasks() can safely count it as
> > > > "already frozen".
> > >
> > > But the fundamental semantic is the same right? Both might be sitting on
> > > locks that might interfere with other tasks and we should be _extra_
> > > careful when using them. In an ideal world, none of them is really
> > > needed.
> > 
> > Ah, it seems that we misunderstood each other... see below.
> > 
> > > So my question remains. Can we drop the warning for PF_FREEZER_SKIP
> > > tasks as well?
> > 
> > But why?
> 
> To drop the warning which led to the revert.

Ble, I should have checked the code more closely. freezer_count does
clear the flag before it goes to the fridge. My bad. So we need
freezable_schedule_unsafe unsafe here to workaround the original problem
and do not trigger the warning.
-- 
Michal Hocko
SUSE Labs

Re: WARNING: syz-executor still has locks held!

[Oleg Nesterov]

On 03/20, Michal Hocko wrote:
>
> So we need
> freezable_schedule_unsafe unsafe here to workaround the original problem
> and do not trigger the warning.

Yes, but note the comment above freezable_schedule_unsafe() ;)

that is why I didn't even try to suggest to use _unsafe() instead of reverting.

And to remind, until we solve the problem with cred_guard_mutex the freezer
can fail anyway although much less likely.

Oleg.

Re: WARNING: syz-executor still has locks held!

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 637 bytes --]

Hi!

> > > > as removing the cgred is way way too complicated.
> > >
> > > We need to do this anyway, this leads to other more serious problems...
> >
> > Yes but this is far away and it doesn't really seem like a stable tree
> > material
> 
> strace -f can hang ;) so this is the stable material.

Stable is only for obvious fixes... and this looks to be far from
simple/obvious.

And yes, strace -f is a bug, but ... not really sufficiently serious one.
									
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: WARNING: syz-executor still has locks held!

[Michal Hocko]

On Thu 21-03-19 18:28:33, Oleg Nesterov wrote:
> On 03/20, Michal Hocko wrote:
> >
> > So we need
> > freezable_schedule_unsafe unsafe here to workaround the original problem
> > and do not trigger the warning.
> 
> Yes, but note the comment above freezable_schedule_unsafe() ;)
> 
> that is why I didn't even try to suggest to use _unsafe() instead of reverting.

Maybe I have misunderstood the discussion back then but we _knew_ that
the lock is held and considered that an acceptable compromise to
workaround the issue. And that is where the freezable_schedule_unsafe
can be used AFAIU.
 
> And to remind, until we solve the problem with cred_guard_mutex the freezer
> can fail anyway although much less likely.

Sure but this seems much less likely as the original patch seemed to
resolve the failing suspend.

So unless there is something to really handle cred_guard_mutex now, can
we get back to the workaround and silence the lockdep splat and move on
please?
-- 
Michal Hocko
SUSE Labs