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

Attachments:

(No filename) (1.82 kB)
signature.asc (188.00 B)
Digital signature Download all attachments

2018-12-03 14:18:52

by Michal Hocko

[permalink] [raw]

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

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

2018-12-03 14:46:31

Hi!

> * Michal Hocko <[email protected]> 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

Attachments:

(No filename) (1.76 kB)
signature.asc (188.00 B)
Digital signature Download all attachments

2018-12-04 19:35:23

by Linus Torvalds

[permalink] [raw]

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

On Tue, Dec 4, 2018 at 10:17 AM Michal Hocko <[email protected]> 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

2018-12-04 19:44:41

On Tue 2018-12-04 09:31:11, Linus Torvalds wrote:
> On Tue, Dec 4, 2018 at 1:58 AM Michal Hocko <[email protected]> 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

Attachments:

(No filename) (1.49 kB)
signature.asc (188.00 B)
Digital signature Download all attachments

2018-12-04 20:07:07

by Linus Torvalds

[permalink] [raw]

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

On Tue, Dec 4, 2018 at 11:49 AM Linus Torvalds
<[email protected]> 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

2018-12-05 14:37:33

by Oleg Nesterov

[permalink] [raw]

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 <[email protected]> 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/[email protected]/

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

Oleg.

2018-12-06 08:56:19

by Chanho Min

[permalink] [raw]

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

> From: Oleg Nesterov [mailto:[email protected]]
> 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 <[email protected]> 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/[email protected]/
>
> 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

2018-12-06 08:58:48

by Pavel Machek

[permalink] [raw]

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

Hi!

> > On 12/04, Ingo Molnar wrote:
> > >
> > > * Oleg Nesterov <[email protected]> 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/[email protected]/
> >
> > 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

Attachments:

(No filename) (1.10 kB)
signature.asc (188.00 B)
Digital signature Download all attachments

2018-12-06 09:09:39

by Chanho Min

[permalink] [raw]

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

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