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,
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,
Re: [PATCH] Revert "exec: make de_thread() freezable (was: Re: Linux 4.20-rc4)
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)
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)
> 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)
> 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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
* 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)
* 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)
* 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)
* 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)
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)
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)
* 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)
* 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)
* 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)
* 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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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)
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
Re: [PATCH] Revert "exec: make de_thread() freezable (was: Re: Linux 4.20-rc4)
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
