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

2018-12-06 Thread Chanho Min
> >
> > I am sorry for the reverting this patch. It's also my fault that
> > I didn't check lockdep. But, We decided to keep this patch in our product.
> > Freeze fail is a real problem we've had for the last two years,
> > whereas lockdep's notice is not a real problem.
> > We hope this issue will be resolved soon.
> 
> I guess it makes sense for your usage.
> 
> How often do you see the failures without the patch?
Very rare, it happens about 1 in 1000 suspends.

Chanho,



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

2018-12-06 Thread Pavel Machek
Hi!

> > On 12/04, Ingo Molnar wrote:
> > >
> > > * Oleg Nesterov  wrote:
> > >
> > > > we really need to narrow the (huge) scope of ->cred_guard_mutex in exec
> > paths.
> > > >
> > > > my attempt to fix this was nacked, and nobody suggested a better 
> > > > solution
> > so far.
> > >
> > > Any link to your patch and the NAK?
> > 
> > See https://lore.kernel.org/lkml/20170213141452.ga30...@redhat.com/
> > 
> > No questions, the patch wasn't pretty. And imo we need to rework the 
> > security
> > hooks in the long term.
> > 
> > Oleg.
> 
> I am sorry for the reverting this patch. It's also my fault that
> I didn't check lockdep. But, We decided to keep this patch in our product.
> Freeze fail is a real problem we've had for the last two years,
> whereas lockdep's notice is not a real problem.
> We hope this issue will be resolved soon.

I guess it makes sense for your usage.

How often do you see the failures without the patch?
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature


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

2018-12-06 Thread Chanho Min
> From: Oleg Nesterov [mailto:o...@redhat.com]
> Sent: Wednesday, December 05, 2018 11:36 PM
> To: Ingo Molnar
> Cc: Linus Torvalds; Linux List Kernel Mailing; Rafael J. Wysocki; Chanho Min;
> Thomas Gleixner; Peter Zijlstra; Pavel Machek; Michal Hocko
> Subject: Re: [PATCH] Revert "exec: make de_thread() freezable (was: Re: Linux
> 4.20-rc4)
> 
> Ingo, et al,
> 
> Sorry, I am sick and can't participate this discussion right now,
> 
> On 12/04, Ingo Molnar wrote:
> >
> > * Oleg Nesterov  wrote:
> >
> > > we really need to narrow the (huge) scope of ->cred_guard_mutex in exec
> paths.
> > >
> > > my attempt to fix this was nacked, and nobody suggested a better solution
> so far.
> >
> > Any link to your patch and the NAK?
> 
> See https://lore.kernel.org/lkml/20170213141452.ga30...@redhat.com/
> 
> No questions, the patch wasn't pretty. And imo we need to rework the security
> hooks in the long term.
> 
> Oleg.

I am sorry for the reverting this patch. It's also my fault that
I didn't check lockdep. But, We decided to keep this patch in our product.
Freeze fail is a real problem we've had for the last two years,
whereas lockdep's notice is not a real problem.
We hope this issue will be resolved soon.

Special thanks to Oleg,
Chanho



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

2018-12-05 Thread Oleg Nesterov
Ingo, et al,

Sorry, I am sick and can't participate this discussion right now,

On 12/04, Ingo Molnar wrote:
>
> * Oleg Nesterov  wrote:
>
> > we really need to narrow the (huge) scope of ->cred_guard_mutex in exec 
> > paths.
> >
> > my attempt to fix this was nacked, and nobody suggested a better solution 
> > so far.
>
> Any link to your patch and the NAK?

See https://lore.kernel.org/lkml/20170213141452.ga30...@redhat.com/

No questions, the patch wasn't pretty. And imo we need to rework the security
hooks in the long term.

Oleg.



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

2018-12-04 Thread Linus Torvalds
On Tue, Dec 4, 2018 at 11:49 AM Linus Torvalds
 wrote:
>
> because honestly, the *only* reason we hold on to that lock is for the
> insane and not really interesting case of "somebody tried to use
> ptrace to change the creds in-flight during the exec".

No, sorry, me confused. Not somebody trying to change them, it's just
ptrace_attach() trying to change _our_ state during this sequence, and
relying on it all being atomic.

So taking a ref is unnecessary and pointless. It's not the creds that
change, it's that we really want to delay ptrace_attach().

We could maybe set that "we're busy now" flag, and have
ptrace_attach() do something like

if (task_is_busy(task)) {
sched_yield();
return -ERESTARTSYS;
}

or something like that.

  Linus


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

2018-12-04 Thread Pavel Machek
On Tue 2018-12-04 09:31:11, Linus Torvalds wrote:
> On Tue, Dec 4, 2018 at 1:58 AM Michal Hocko  wrote:
> >
> > AFAIU both suspend and hibernation require the system to enter quiescent
> > state with no task potentially interfering with suspended devices. And
> > in this particular case those de-thread-ed threads will certainly not
> > interfere so silencing the lockdep sounds like a reasonable workaround.
> 
> I still think it would be better to simply not freeze killed user processes.
> 
> We already  have things like
> 
> if (test_tsk_thread_flag(p, TIF_MEMDIE))
> return false;
> 
> exactly because we do not want to freeze processes that are about to
> die due to being killed. Very similar situation: we don't want to
> freeze those processes, because doing so would halt them from freeing
> the resources that may be needed for suspend or hibernate.
> 
> How about something like we set PF_NOFREEZE when we set PF_EXITING? At
> that point we've pretty much turned into a kernel thread, no?

I'd be careful about that. Exiting task needs to write to disk (space
of unlinked but open files needs to be freed), so we can't just ignore
them.

And given that ptrace example (where it deadlocks w/o freezer anywhere
nearby), I'd say attempt to simplify the locking should be made, first.

Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature


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

2018-12-04 Thread Linus Torvalds
On Tue, Dec 4, 2018 at 11:33 AM Linus Torvalds
 wrote:
>
> Looking at this, I'm agreeing that ot would be better to just try to
> narrow down the cred_guard_mutex use a lot.

Ho humm. This is a crazy idea, but I don't see why it wouldn't work.

How about we:

 - stop holding on to cred_guard_mutex entirely in the exec path

and instead just do:

 - prepare_bprm_creds takes a ref to our old creds, and saves it off in the bprm

 - security_bprm_{committing,committed}_creds() can do it's "is this a
valid transition" using the saved-off old creds instead of the current
creds

because honestly, the *only* reason we hold on to that lock is for the
insane and not really interesting case of "somebody tried to use
ptrace to change the creds in-flight during the exec".

Or maybe we could just add a task state flag that says "in exec, you
can't modify the creds in this window, because we're about to switch
to new creds".

Again, no *normal* situation will even notice or care, I think. We
hold the cred lock purely to make sure that the sequence from
prepare_exec_creds -> install_exec_creds is "atomic" wrt credentials,
and it already is for all the normal cases since this is all inside a
single execve system call.

   Linus


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

2018-12-04 Thread Pavel Machek
On Tue 2018-12-04 10:33:10, Ingo Molnar wrote:
> 
> * Michal Hocko  wrote:
> 
> > I dunno. I do not use hibernation. I am a heavy user of the suspend 
> > though. I s2ram all the time. And I have certainly experienced cases 
> > where suspend has failed and I onlyi found out later when I've picked 
> > up my laptop from my heat up bag. Nothing fatal has resulted from that 
> > but this is certainly annoying.
> 
> Hm, so I (mistakenly) thought freezing was mostly limited to hibernation 
> and to a few weird cases when in flight DMA must not be suspended - but 
> I'm wrong and in practice we always freeze tasks during s2ram, right?
> 
> And indeed:
> 
>  config SUSPEND_FREEZER
> bool "Enable freezer for suspend to RAM/standby" \
> if ARCH_WANTS_FREEZER_CONTROL || BROKEN
> depends on SUSPEND
> default y
> 
> which is essentially always enabled on x86.
> 
> TIL ...

pavel@amd:~$ wtf til
Gee...  I don't know what til means...
?

> s2ram is obviously a huge deal.
> 
> Just a newbie question: any chance to not do any freezing at all on 
> modern laptops when doing s2ram, or at least only warn if it fails and 
> try to suspend?

Not really.

> Because in practice losing power due to failed freezing *will* result in 
> data loss, in about 90% of the time ...

Ugh. What are you talking about?

I don't know how you use your machines, but 95% of s2ram's I do,
machines are running on AC power, and I'll notice that freezer
failure, because the machine keeps making noise when it should be
sleeping.

There's big difference between "sync; forced_poweroff" and
"forced_poweroff", which are annoying but quite harmless (editors keep
backups, web browser stores session periodically, and filesystems have
journal...) and real data corruption as in "Pavel found another bug in
fsck.ext3, which is great, but his filesystem is corrupted, which is
not so great".

(BTW one of those bugs is unfixable; I managed to corrupt ext3 in such
a way that fsck is not able to automatically repair it, and likely
never will be. Fun?)

> So I don't even know what we are trying to protect against by refusing to 
> freeze - avoiding a 0.001% data loss risk against a 90% data loss risk?

Where did you get those 0.001% and 90% numbers?

I don't see freezer failures too often.

I see machine that is sleeping, but fails to resume, maybe once in
month. That's "sync; forced_poweroff" type failure. Not nice,
but... Unfortunately fairly hard to debug. And not worse than usual
"hard crashes" I see at about same frequency.

I did have real filesystem corruption, at least twice while debugging
hibernation. Trust me. You don't want to have that, and you certainly
don't want your users to have that.

Best regards,
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature


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

2018-12-04 Thread Linus Torvalds
On Tue, Dec 4, 2018 at 10:17 AM Michal Hocko  wrote:
>
> > How about something like we set PF_NOFREEZE when we set PF_EXITING? At
> > that point we've pretty much turned into a kernel thread, no?
>
> Hmm, that doesn't sound like a bad idea but I am not sure it will
> help because those threads we are waiting for might be block way before
> they reached PF_EXITING.

Yeah, looks that way. We've got the whole "zap_other_threads() ->
actually starting the exit" window, which is probably much bigger than
the "start the exit -> release_task" window.

So we'd have to mark things non-freezable at zap time, not at exit
time, and that's a lot more questionable.

Looking at this, I'm agreeing that ot would be better to just try to
narrow down the cred_guard_mutex use a lot.

Oleg, if you had patch that got push-back for that, maybe this problem
is now the impetus for people to say "yeah, that's not nice but we
clearly need to do it".

I'm not finding any old emails on this, but considering I redid my
email setup recently, that doesn't necessarily mean much.

   Linus


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

2018-12-04 Thread Pavel Machek
Hi!

> * Michal Hocko  wrote:
> 
> > > Do we actually have reports of this happening for people outside
> > > Android?
> > 
> > Not that I am aware of.
> 
> I'd say outside of Android 99% of the use of hibernation is the fail-safe 
> that distributions offer on laptops with very low battery levels: the 
> emergency hibernation when there's almost no power left anymore.

Android does not use hibernation AFAICT. Just s2ram.

> Do these hibernation failure messages typically make it to persistent 
> logs before the system uses power?

I'd say so. If you have enough energy left for hibernation, you also
have enough energy left to write the logs and sync.

> In practice if that is buggy the kernel won't hibernate and the laptop 
> will run out of power and the user will conclude "ugh, I shouldn't have 
> left my laptop turned on" - without looking into the logs and reporting, 
> as they'll perceive it as a user failure not a system failure.
> 
> I certainly saw random Linux laptops fail to hibernate over the years and 
> didn't report it, so if the distribution doesn't do the reporting 
> automatically then chances are we'll never see it.

There are many reasons while hibernation can fail. Buggy drivers,
tasks in D state... And there are some when hibernation can fail "by
design". If you swap does not have enough space to store the data, for
example.

Hibernation was designed to be non-intrusive, and reliable as in "if
it hibernates it will also resume ok", but not reliable as in "it will
always hibernate".

I see that is problematic for "hibernate on battery low".

Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature


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

2018-12-04 Thread Michal Hocko
On Tue 04-12-18 09:31:11, Linus Torvalds wrote:
> On Tue, Dec 4, 2018 at 1:58 AM Michal Hocko  wrote:
> >
> > AFAIU both suspend and hibernation require the system to enter quiescent
> > state with no task potentially interfering with suspended devices. And
> > in this particular case those de-thread-ed threads will certainly not
> > interfere so silencing the lockdep sounds like a reasonable workaround.
> 
> I still think it would be better to simply not freeze killed user processes.
> 
> We already  have things like
> 
> if (test_tsk_thread_flag(p, TIF_MEMDIE))
> return false;
> 
> exactly because we do not want to freeze processes that are about to
> die due to being killed. Very similar situation: we don't want to
> freeze those processes, because doing so would halt them from freeing
> the resources that may be needed for suspend or hibernate.

Yeah, we do not freeze oom victims but that is really far from trivial
to achieve. In order to do so we post-pone quiescent state until we know
all the TIF_MEMDIE threads are gone. See oom_killer_disable. In short it
is a painfull code.

> How about something like we set PF_NOFREEZE when we set PF_EXITING? At
> that point we've pretty much turned into a kernel thread, no?

Hmm, that doesn't sound like a bad idea but I am not sure it will
help because those threads we are waiting for might be block way before
they reached PF_EXITING. I would expect that threads that actually reach
that state usually die shortly. Unless I am missing something we are
talking about basically two cases here. Those threads are frozen while
de_thread waits for them because zap_other_threads cannot wake them from
the fridge or they are blocked in uninterruptible sleep somewhere in the
kernel. The later is the fundamental problem. The former could be hacked
around (check PF_FROZEN and use uninterruptible wake), I guess but I
haven't thought that through yet.
-- 
Michal Hocko
SUSE Labs


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

2018-12-04 Thread Linus Torvalds
On Tue, Dec 4, 2018 at 1:58 AM Michal Hocko  wrote:
>
> AFAIU both suspend and hibernation require the system to enter quiescent
> state with no task potentially interfering with suspended devices. And
> in this particular case those de-thread-ed threads will certainly not
> interfere so silencing the lockdep sounds like a reasonable workaround.

I still think it would be better to simply not freeze killed user processes.

We already  have things like

if (test_tsk_thread_flag(p, TIF_MEMDIE))
return false;

exactly because we do not want to freeze processes that are about to
die due to being killed. Very similar situation: we don't want to
freeze those processes, because doing so would halt them from freeing
the resources that may be needed for suspend or hibernate.

How about something like we set PF_NOFREEZE when we set PF_EXITING? At
that point we've pretty much turned into a kernel thread, no?

Linus


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

2018-12-04 Thread Michal Hocko
On Tue 04-12-18 10:33:10, Ingo Molnar wrote:
> 
> * Michal Hocko  wrote:
> 
> > I dunno. I do not use hibernation. I am a heavy user of the suspend 
> > though. I s2ram all the time. And I have certainly experienced cases 
> > where suspend has failed and I onlyi found out later when I've picked 
> > up my laptop from my heat up bag. Nothing fatal has resulted from that 
> > but this is certainly annoying.
> 
> Hm, so I (mistakenly) thought freezing was mostly limited to hibernation 
> and to a few weird cases when in flight DMA must not be suspended - but 
> I'm wrong and in practice we always freeze tasks during s2ram, right?

Yup.

> And indeed:
> 
>  config SUSPEND_FREEZER
> bool "Enable freezer for suspend to RAM/standby" \
> if ARCH_WANTS_FREEZER_CONTROL || BROKEN
> depends on SUSPEND
> default y
> 
> which is essentially always enabled on x86.
> 
> TIL ...
> 
> s2ram is obviously a huge deal.
> 
> Just a newbie question: any chance to not do any freezing at all on 
> modern laptops when doing s2ram, or at least only warn if it fails and 
> try to suspend?

AFAIU both suspend and hibernation require the system to enter quiescent
state with no task potentially interfering with suspended devices. And
in this particular case those de-thread-ed threads will certainly not
interfere so silencing the lockdep sounds like a reasonable workaround.
-- 
Michal Hocko
SUSE Labs


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

2018-12-04 Thread Ingo Molnar


* Michal Hocko  wrote:

> I dunno. I do not use hibernation. I am a heavy user of the suspend 
> though. I s2ram all the time. And I have certainly experienced cases 
> where suspend has failed and I onlyi found out later when I've picked 
> up my laptop from my heat up bag. Nothing fatal has resulted from that 
> but this is certainly annoying.

Hm, so I (mistakenly) thought freezing was mostly limited to hibernation 
and to a few weird cases when in flight DMA must not be suspended - but 
I'm wrong and in practice we always freeze tasks during s2ram, right?

And indeed:

 config SUSPEND_FREEZER
bool "Enable freezer for suspend to RAM/standby" \
if ARCH_WANTS_FREEZER_CONTROL || BROKEN
depends on SUSPEND
default y

which is essentially always enabled on x86.

TIL ...

s2ram is obviously a huge deal.

Just a newbie question: any chance to not do any freezing at all on 
modern laptops when doing s2ram, or at least only warn if it fails and 
try to suspend?

Because in practice losing power due to failed freezing *will* result in 
data loss, in about 90% of the time ...

So I don't even know what we are trying to protect against by refusing to 
freeze - avoiding a 0.001% data loss risk against a 90% data loss risk?

Thanks,

Ingo


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

2018-12-04 Thread Ingo Molnar


* Oleg Nesterov  wrote:

> > I reviewed the ->cred_guard_mutex code, and the mutex is held across all
> > of exec() - and we always did this.
> 
> Yes, and this was always wrong. For example, this test-case hangs:
> 
>   #include 
>   #include 
>   #include 
>   #include 
> 
>   void *thread(void *arg)
>   {
>   ptrace(PTRACE_TRACEME, 0,0,0);
>   return NULL;
>   }
> 
>   int main(void)
>   {
>   int pid = fork();
> 
>   if (!pid) {
>   pthread_t pt;
>   pthread_create(, NULL, thread, NULL);
>   pthread_join(pt, NULL);
>   execlp("echo", "echo", "passed", NULL);
>   }
> 
>   sleep(1);
>   // or anything else which needs ->cred_guard_mutex,
>   // say open(/proc/$pid/mem)
>   ptrace(PTRACE_ATTACH, pid, 0,0);
>   kill(pid, SIGCONT);
> 
>   return 0;
>   }
> 
> we really need to narrow the (huge) scope of ->cred_guard_mutex in exec paths.
> 
> my attempt to fix this was nacked, and nobody suggested a better solution so 
> far.

Any link to your patch and the NAK?

Thanks,

Ingo


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

2018-12-04 Thread Michal Hocko
On Tue 04-12-18 10:02:28, Ingo Molnar wrote:
> 
> * Michal Hocko  wrote:
> 
> > > Do we actually have reports of this happening for people outside
> > > Android?
> > 
> > Not that I am aware of.
> 
> I'd say outside of Android 99% of the use of hibernation is the fail-safe 
> that distributions offer on laptops with very low battery levels: the 
> emergency hibernation when there's almost no power left anymore.
> 
> Do these hibernation failure messages typically make it to persistent 
> logs before the system uses power?

I dunno. I do not use hibernation. I am a heavy user of the suspend
though. I s2ram all the time. And I have certainly experienced cases
where suspend has failed and I onlyi found out later when I've picked up
my laptop from my heat up bag. Nothing fatal has resulted from that but
this is certainly annoying.
-- 
Michal Hocko
SUSE Labs


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

2018-12-04 Thread Ingo Molnar


* Michal Hocko  wrote:

> > Do we actually have reports of this happening for people outside
> > Android?
> 
> Not that I am aware of.

I'd say outside of Android 99% of the use of hibernation is the fail-safe 
that distributions offer on laptops with very low battery levels: the 
emergency hibernation when there's almost no power left anymore.

Do these hibernation failure messages typically make it to persistent 
logs before the system uses power?

In practice if that is buggy the kernel won't hibernate and the laptop 
will run out of power and the user will conclude "ugh, I shouldn't have 
left my laptop turned on" - without looking into the logs and reporting, 
as they'll perceive it as a user failure not a system failure.

I certainly saw random Linux laptops fail to hibernate over the years and 
didn't report it, so if the distribution doesn't do the reporting 
automatically then chances are we'll never see it.

Thanks,

Ingo


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

2018-12-04 Thread Ingo Molnar


* Rafael J. Wysocki  wrote:

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

I have 1+ days uptime now on the system, no splat or other problems, as 
expected:

  Tested-by: Ingo Molnar 

I have the revert locally in -tip, will drop it once your commit hits 
upstream.

Ingo


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

2018-12-03 Thread Pavel Machek
On Mon 2018-12-03 09:06:18, Linus Torvalds wrote:
> On Mon, Dec 3, 2018 at 6:17 AM Michal Hocko  wrote:
> >
> > This argument just doesn't make any sense. Rare bugs are maybe even more
> > annoying because you do not expect them to happen.
> 
> Absolutely.
> 
> And valid lockdep complaints are a real issue too.
> 
> So I don't think there's any question that this should be reverted,
> the only question is whether I take the revert directly or get it from
> the PM tree.

I agree this to be reverted. (And the problem is bigger than lockdep,
and apparently not limited to suspend).

But we merged hacky solution because we wanted something simple for
stable, and I don't think that was good idea. (And I'm not sure this
is worth fixing in stable, as we don't have reports of people hitting
that in that kernels).

Best regards,

Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature


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

2018-12-03 Thread Michal Hocko
On Mon 03-12-18 09:06:18, Linus Torvalds wrote:
> On Mon, Dec 3, 2018 at 6:17 AM Michal Hocko  wrote:
> >
> > This argument just doesn't make any sense. Rare bugs are maybe even more
> > annoying because you do not expect them to happen.
> 
> Absolutely.
> 
> And valid lockdep complaints are a real issue too.

No questions about that. But in this case there is no real deadlock so
the splat is disputable. And we have a "do not use" freezable_schedule_unsafe
which shouldn't tigger the splat yet it would make the suspend issue go
away AFIU. Not a great fix and we should target for something better
long term but good enough as a poor's man stop gap fix.

> So I don't think there's any question that this should be reverted,
> the only question is whether I take the revert directly or get it from
> the PM tree.
> 
> It does sound like the de_thread() behavior needs more work. Maybe,
> for example, we need to make it clear that zapped threads are *not*
> frozen, and instead the freezer will wait for them to exit?

I was actually proposing something like that during the review
discussion. Basically set PF_NOFREEZE all the threads and wake them up.
But that is not so simple because they might be at any path when they
sleep (this is the biggest design flaw of the freezer IMHO).

-- 
Michal Hocko
SUSE Labs


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

2018-12-03 Thread Linus Torvalds
On Mon, Dec 3, 2018 at 6:17 AM Michal Hocko  wrote:
>
> This argument just doesn't make any sense. Rare bugs are maybe even more
> annoying because you do not expect them to happen.

Absolutely.

And valid lockdep complaints are a real issue too.

So I don't think there's any question that this should be reverted,
the only question is whether I take the revert directly or get it from
the PM tree.

It does sound like the de_thread() behavior needs more work. Maybe,
for example, we need to make it clear that zapped threads are *not*
frozen, and instead the freezer will wait for them to exit?

Hmm?

  Linus


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

2018-12-03 Thread Pavel Machek
On Mon 2018-12-03 15:17:37, Michal Hocko wrote:
> On Mon 03-12-18 15:14:59, Pavel Machek wrote:
> > On Mon 2018-12-03 14:53:51, Michal Hocko wrote:
> > > On Mon 03-12-18 14:10:06, Pavel Machek wrote:
> > > > On Mon 2018-12-03 13:38:57, Michal Hocko wrote:
> > > > > On Mon 03-12-18 13:31:49, Oleg Nesterov wrote:
> > > > > > On 12/03, Michal Hocko wrote:
> > > > > > >
> > > > > > > Now, I wouldn't mind to revert this because the code is really 
> > > > > > > old and
> > > > > > > we haven't seen many bug reports about failing suspend yet. But 
> > > > > > > what is
> > > > > > > the actual plan to make this work properly?
> > > > > > 
> > > > > > I don't see a simple solution...
> > > > > > 
> > > > > > But we need to fix exec/de_thread anyway, then we can probably 
> > > > > > reconsider
> > > > > > this patch.
> > > > > 
> > > > > My concern is that de_thread fix might be too disruptive for stable
> > > > > kernels while we might want to have a simple enough fix for the the
> > > > > suspend issue in the meantime. That was actually the primary reason 
> > > > > I've
> > > > > acked the hack even though I didn't like it.
> > > > 
> > > > Do we care about failing sleep in stable? Does someone hit the issue 
> > > > there?
> > > > 
> > > > This sounds like issue only Android is hitting, and they run very
> > > > heavily patched kernels, far away from mainline or stable.
> > > 
> > > But the underlying issue is the same and independent on their patches
> > > AFAIU. And is this really a common problem to care about in stable? I
> > > dunno to be honest but it sounds annoying for sure. Failing suspend is
> > > something that doesn't make your day when you are in hurry and want
> > > find out only later when your laptop heats up your bag ;)
> > 
> > In general, yes. In practice, if it happens 1 in 100 suspends, you
> > don't care that much (but Android cares).
> 
> This argument just doesn't make any sense. Rare bugs are maybe even
> more

I guess argumenting about this just does not make sense. Just bear in
mind -stable is not for theoretical problems.

> annoying because you do not expect them to happen. But I would be more
> interested to see whether they are any downside. Is there any actual
> risk to silence the lockup detector that you can see?

As someone else noticed:

a) the bug can be triggered outside suspend

b) the lockdep report is real. You'll still get suspend failure, but
you now need two processes to trigger it

> > Do we actually have reports of this happening for people outside
> > Android?
> 
> Not that I am aware of.

Good.
Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature


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

2018-12-03 Thread Michal Hocko
On Mon 03-12-18 15:14:59, Pavel Machek wrote:
> On Mon 2018-12-03 14:53:51, Michal Hocko wrote:
> > On Mon 03-12-18 14:10:06, Pavel Machek wrote:
> > > On Mon 2018-12-03 13:38:57, Michal Hocko wrote:
> > > > On Mon 03-12-18 13:31:49, Oleg Nesterov wrote:
> > > > > On 12/03, Michal Hocko wrote:
> > > > > >
> > > > > > Now, I wouldn't mind to revert this because the code is really old 
> > > > > > and
> > > > > > we haven't seen many bug reports about failing suspend yet. But 
> > > > > > what is
> > > > > > the actual plan to make this work properly?
> > > > > 
> > > > > I don't see a simple solution...
> > > > > 
> > > > > But we need to fix exec/de_thread anyway, then we can probably 
> > > > > reconsider
> > > > > this patch.
> > > > 
> > > > My concern is that de_thread fix might be too disruptive for stable
> > > > kernels while we might want to have a simple enough fix for the the
> > > > suspend issue in the meantime. That was actually the primary reason I've
> > > > acked the hack even though I didn't like it.
> > > 
> > > Do we care about failing sleep in stable? Does someone hit the issue 
> > > there?
> > > 
> > > This sounds like issue only Android is hitting, and they run very
> > > heavily patched kernels, far away from mainline or stable.
> > 
> > But the underlying issue is the same and independent on their patches
> > AFAIU. And is this really a common problem to care about in stable? I
> > dunno to be honest but it sounds annoying for sure. Failing suspend is
> > something that doesn't make your day when you are in hurry and want
> > find out only later when your laptop heats up your bag ;)
> 
> In general, yes. In practice, if it happens 1 in 100 suspends, you
> don't care that much (but Android cares).

This argument just doesn't make any sense. Rare bugs are maybe even more
annoying because you do not expect them to happen. But I would be more
interested to see whether they are any downside. Is there any actual
risk to silence the lockup detector that you can see?

> Do we actually have reports of this happening for people outside
> Android?

Not that I am aware of.
-- 
Michal Hocko
SUSE Labs


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

2018-12-03 Thread Pavel Machek
On Mon 2018-12-03 14:53:51, Michal Hocko wrote:
> On Mon 03-12-18 14:10:06, Pavel Machek wrote:
> > On Mon 2018-12-03 13:38:57, Michal Hocko wrote:
> > > On Mon 03-12-18 13:31:49, Oleg Nesterov wrote:
> > > > On 12/03, Michal Hocko wrote:
> > > > >
> > > > > Now, I wouldn't mind to revert this because the code is really old and
> > > > > we haven't seen many bug reports about failing suspend yet. But what 
> > > > > is
> > > > > the actual plan to make this work properly?
> > > > 
> > > > I don't see a simple solution...
> > > > 
> > > > But we need to fix exec/de_thread anyway, then we can probably 
> > > > reconsider
> > > > this patch.
> > > 
> > > My concern is that de_thread fix might be too disruptive for stable
> > > kernels while we might want to have a simple enough fix for the the
> > > suspend issue in the meantime. That was actually the primary reason I've
> > > acked the hack even though I didn't like it.
> > 
> > Do we care about failing sleep in stable? Does someone hit the issue there?
> > 
> > This sounds like issue only Android is hitting, and they run very
> > heavily patched kernels, far away from mainline or stable.
> 
> But the underlying issue is the same and independent on their patches
> AFAIU. And is this really a common problem to care about in stable? I
> dunno to be honest but it sounds annoying for sure. Failing suspend is
> something that doesn't make your day when you are in hurry and want
> find out only later when your laptop heats up your bag ;)

In general, yes. In practice, if it happens 1 in 100 suspends, you
don't care that much (but Android cares).

Do we actually have reports of this happening for people outside
Android?

Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature


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

2018-12-03 Thread Michal Hocko
On Mon 03-12-18 14:10:06, Pavel Machek wrote:
> On Mon 2018-12-03 13:38:57, Michal Hocko wrote:
> > On Mon 03-12-18 13:31:49, Oleg Nesterov wrote:
> > > On 12/03, Michal Hocko wrote:
> > > >
> > > > Now, I wouldn't mind to revert this because the code is really old and
> > > > we haven't seen many bug reports about failing suspend yet. But what is
> > > > the actual plan to make this work properly?
> > > 
> > > I don't see a simple solution...
> > > 
> > > But we need to fix exec/de_thread anyway, then we can probably reconsider
> > > this patch.
> > 
> > My concern is that de_thread fix might be too disruptive for stable
> > kernels while we might want to have a simple enough fix for the the
> > suspend issue in the meantime. That was actually the primary reason I've
> > acked the hack even though I didn't like it.
> 
> Do we care about failing sleep in stable? Does someone hit the issue there?
> 
> This sounds like issue only Android is hitting, and they run very
> heavily patched kernels, far away from mainline or stable.

But the underlying issue is the same and independent on their patches
AFAIU. And is this really a common problem to care about in stable? I
dunno to be honest but it sounds annoying for sure. Failing suspend is
something that doesn't make your day when you are in hurry and want
find out only later when your laptop heats up your bag ;)

-- 
Michal Hocko
SUSE Labs


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

2018-12-03 Thread Oleg Nesterov
On 12/03, Michal Hocko wrote:
>
> On Mon 03-12-18 13:31:49, Oleg Nesterov wrote:
> > On 12/03, Michal Hocko wrote:
> > >
> > > Now, I wouldn't mind to revert this because the code is really old and
> > > we haven't seen many bug reports about failing suspend yet. But what is
> > > the actual plan to make this work properly?
> >
> > I don't see a simple solution...
> >
> > But we need to fix exec/de_thread anyway, then we can probably reconsider
> > this patch.
>
> My concern is that de_thread fix might be too disruptive for stable
> kernels while we might want to have a simple enough fix for the the
> suspend issue in the meantime. That was actually the primary reason I've
> acked the hack even though I didn't like it.
>
> So can we find a way to shut the lockdep up

You have already mentioned freezable_schedule_unsafe(), but I agree with
"DO NOT ADD ANY NEW CALLERS OF THIS FUNCTION" above it ...

> when this is not really a
> deadlock?

not a deadlock, but lockdep is obviously right, suspend still can fail if
another task needs the same mutex.

Again, we have already discussed this, in my opinion we should blame exec/
de_thread for this and other problems.

> Or maybe this really is one and then we need a real fix for
> stable as well.

Well, strace -f can hang if it races with mt exec, it would be nice to have
a fix for stable.

Oleg.



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

2018-12-03 Thread Pavel Machek
On Mon 2018-12-03 13:38:57, Michal Hocko wrote:
> On Mon 03-12-18 13:31:49, Oleg Nesterov wrote:
> > On 12/03, Michal Hocko wrote:
> > >
> > > Now, I wouldn't mind to revert this because the code is really old and
> > > we haven't seen many bug reports about failing suspend yet. But what is
> > > the actual plan to make this work properly?
> > 
> > I don't see a simple solution...
> > 
> > But we need to fix exec/de_thread anyway, then we can probably reconsider
> > this patch.
> 
> My concern is that de_thread fix might be too disruptive for stable
> kernels while we might want to have a simple enough fix for the the
> suspend issue in the meantime. That was actually the primary reason I've
> acked the hack even though I didn't like it.

Do we care about failing sleep in stable? Does someone hit the issue there?

This sounds like issue only Android is hitting, and they run very
heavily patched kernels, far away from mainline or stable.

> So can we find a way to shut the lockdep up when this is not really a
> deadlock?  Or maybe this really is one and then we need a real fix for
> stable as well.

Shutting up the lockdep is of course possible...

...but lets first see what good solution looks like.
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature


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

2018-12-03 Thread Michal Hocko
On Mon 03-12-18 13:31:49, Oleg Nesterov wrote:
> On 12/03, Michal Hocko wrote:
> >
> > Now, I wouldn't mind to revert this because the code is really old and
> > we haven't seen many bug reports about failing suspend yet. But what is
> > the actual plan to make this work properly?
> 
> I don't see a simple solution...
> 
> But we need to fix exec/de_thread anyway, then we can probably reconsider
> this patch.

My concern is that de_thread fix might be too disruptive for stable
kernels while we might want to have a simple enough fix for the the
suspend issue in the meantime. That was actually the primary reason I've
acked the hack even though I didn't like it.

So can we find a way to shut the lockdep up when this is not really a
deadlock?  Or maybe this really is one and then we need a real fix for
stable as well.
-- 
Michal Hocko
SUSE Labs


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

2018-12-03 Thread Oleg Nesterov
On 12/03, Michal Hocko wrote:
>
> Now, I wouldn't mind to revert this because the code is really old and
> we haven't seen many bug reports about failing suspend yet. But what is
> the actual plan to make this work properly?

I don't see a simple solution...

But we need to fix exec/de_thread anyway, then we can probably reconsider
this patch.

Oleg.



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

2018-12-03 Thread Rafael J. Wysocki
On Monday, December 3, 2018 9:39:42 AM CET Michal Hocko wrote:
> On Mon 03-12-18 08:47:00, Ingo Molnar wrote:
> [...]
> > I reviewed the ->cred_guard_mutex code, and the mutex is held across all 
> > of exec() - and we always did this.
> 
> Yes, this is something that has been pointed out during the review. Oleg
> has argued that making this path freezable is really hard and that we
> should be changing de_thread to sleep withtou cred_guard_mutex long term
> anyway (http://lkml.kernel.org/r/20181114143705.gb13...@redhat.com).
> 
> Failing suspend seems like a real problem while the lockdep one doesn't
> really reflect any real deadlock, right? So while the patch is not
> perfect it shouldn't make the situation much worse. Lockdep splat is
> certainly annoying but is it any worse than a suspend failing?
> 
> Now, I wouldn't mind to revert this because the code is really old and
> we haven't seen many bug reports about failing suspend yet. But what is
> the actual plan to make this work properly? Use
> freezable_schedule_unsafe instead? Freezer code has some
> fundamental design issues which are quite hard to get over.

I agree and we just need to look deeper into this.

I had hoped that this would work since you and Oleg agreed with it, but since
it doesn't, let's do a revert for now and get back to this later.

Thanks,
Rafael



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

2018-12-03 Thread Rafael J. Wysocki
On Monday, December 3, 2018 8:47:00 AM CET Ingo Molnar wrote:
> 
> * Linus Torvalds  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: ed85fbf8 (>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 *0x812ece06
> 0x812ece06 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 */
> 55static inline bool try_to_freeze_unsafe(void)
> 56{
> 57might_sleep();
> 58if (likely(!freezing(current)))
> 59return false;
> 60return __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



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

2018-12-03 Thread Oleg Nesterov
On 12/03, Ingo Molnar wrote:
>
> 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: ed85fbf8 (>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

My fault.

Michal didn't like this patch, but I managed to confuse him ;)

I even mentioned that freezable_schedule() is obviously not right with
->cred_guard_mutex held, but I somehow I thought that this patch "doesn't
make things worse".

> I reviewed the ->cred_guard_mutex code, and the mutex is held across all
> of exec() - and we always did this.

Yes, and this was always wrong. For example, this test-case hangs:

#include 
#include 
#include 
#include 

void *thread(void *arg)
{
ptrace(PTRACE_TRACEME, 0,0,0);
return NULL;
}

int main(void)
{
int pid = fork();

if (!pid) {
pthread_t pt;
pthread_create(, NULL, thread, NULL);
pthread_join(pt, NULL);
execlp("echo", "echo", "passed", NULL);
}

sleep(1);
// or anything else which needs ->cred_guard_mutex,
// say open(/proc/$pid/mem)
ptrace(PTRACE_ATTACH, pid, 0,0);
kill(pid, SIGCONT);

return 0;
}

we really need to narrow the (huge) scope of ->cred_guard_mutex in exec paths.

my attempt to fix this was nacked, and nobody suggested a better solution so 
far.

> This reverts commit c22397888f1eed98cd59f0a88f2a5f6925f80e15.

Thanks.

> ---
>  fs/exec.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index acc3a5536384..fc281b738a98 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -62,7 +62,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>
>  #include 
>  #include 
> @@ -1084,7 +1083,7 @@ static int de_thread(struct task_struct *tsk)
>   while (sig->notify_count) {
>   __set_current_state(TASK_KILLABLE);
>   spin_unlock_irq(lock);
> - freezable_schedule();
> + schedule();
>   if (unlikely(__fatal_signal_pending(tsk)))
>   goto killed;
>   spin_lock_irq(lock);
> @@ -1112,7 +,7 @@ static int de_thread(struct task_struct *tsk)
>   __set_current_state(TASK_KILLABLE);
>   write_unlock_irq(_lock);
>   cgroup_threadgroup_change_end(tsk);
> - freezable_schedule();
> + schedule();
>   if (unlikely(__fatal_signal_pending(tsk)))
>   goto killed;
>   }



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

2018-12-03 Thread Michal Hocko
On Mon 03-12-18 08:47:00, Ingo Molnar wrote:
[...]
> I reviewed the ->cred_guard_mutex code, and the mutex is held across all 
> of exec() - and we always did this.

Yes, this is something that has been pointed out during the review. Oleg
has argued that making this path freezable is really hard and that we
should be changing de_thread to sleep withtou cred_guard_mutex long term
anyway (http://lkml.kernel.org/r/20181114143705.gb13...@redhat.com).

Failing suspend seems like a real problem while the lockdep one doesn't
really reflect any real deadlock, right? So while the patch is not
perfect it shouldn't make the situation much worse. Lockdep splat is
certainly annoying but is it any worse than a suspend failing?

Now, I wouldn't mind to revert this because the code is really old and
we haven't seen many bug reports about failing suspend yet. But what is
the actual plan to make this work properly? Use
freezable_schedule_unsafe instead? Freezer code has some
fundamental design issues which are quite hard to get over.
-- 
Michal Hocko
SUSE Labs