From: Valentin Schneider <vschneid@redhat.com> To: "Eric W. Biederman" <ebiederm@xmission.com> Cc: linux-kernel@vger.kernel.org, kexec@lists.infradead.org, linux-rt-users@vger.kernel.org, Arnd Bergmann <arnd@arndb.de>, Petr Mladek <pmladek@suse.com>, Thomas Gleixner <tglx@linutronix.de>, Sebastian Andrzej Siewior <bigeasy@linutronix.de>, Juri Lelli <jlelli@redhat.com>, "Luis Claudio R. Goncalves" <lgoncalv@redhat.com>, Andrew Morton <akpm@linux-foundation.org>, Vivek Goyal <vgoyal@redhat.com> Subject: Re: [PATCH v2] panic, kexec: Make __crash_kexec() NMI safe Date: Mon, 27 Jun 2022 13:42:22 +0100 [thread overview] Message-ID: <xhsmhpmiu5lch.mognet@vschneid.remote.csb> (raw) In-Reply-To: <87r13c7jyp.fsf@email.froward.int.ebiederm.org> On 25/06/22 12:04, Eric W. Biederman wrote: > I am not particularly fond of this patch as it adds more complexity than > is necessary to solve the problem. > > Calling a spade a spade PREEMPT_RT's mutex_trylock implementation is > broken as it can not support the use cases of an ordinary mutex_trylock. > I have not seen (possibly I skimmed too quickly) anywhere in the > discussion why PREEMPT_RT is not being fixed. Looking at the code > there is enough going on in try_to_take_rt_mutex that I can imagine > that some part of that code is not nmi safe. So I can believe > PREEMPT_RT may be unfix-ably broken. > AFAICT same goes for !PREEMPT_RT given the mutex_unlock(); it's a bit convoluted but you can craft scenarios where the NMI ends up spinning on mutex->wait_lock that is owned by the interrupted task, e.g. CPU0 CPU1 crash_shrink_memory() mutex_lock(); crash_get_memory_size() mutex_lock() raw_spin_lock(&lock->wait_lock); // Lock acquired <NMI> mutex_unlock() <Release lock->owner>; // Owner is free at this point so this succeeds mutex_trylock(); // No kexec_crash_image mutex_unlock() raw_spin_lock(&lock->wait_lock); > > At this point I recommend going back to being ``unconventional'' with > the kexec locking and effectively reverting commit 8c5a1cf0ad3a ("kexec: > use a mutex for locking rather than xchg()"). > > That would also mean that we don't have to worry about the lockdep code > doing something weird in the future and breaking kexec. > > Your change starting to is atomic_cmpxchng is most halfway to a revert > of commit 8c5a1cf0ad3a ("kexec: use a mutex for locking rather than > xchg()"). So we might as well go the whole way and just document that > the kexec on panic code can not use conventional kernel locking > primitives and has to dig deep and build it's own. At which point it > makes no sense for the rest of the kexec code to use anything different. > Hm, I'm a bit torn about that one, ideally I'd prefer to keep "homegrown" locking primitives to just where they are needed (loading & kexec'ing), but I'm also not immensely fond of the "hybrid" mutex+cmpxchg approach. > Eric
WARNING: multiple messages have this Message-ID (diff)
From: Valentin Schneider <vschneid@redhat.com> To: "Eric W. Biederman" <ebiederm@xmission.com> Cc: linux-kernel@vger.kernel.org, kexec@lists.infradead.org, linux-rt-users@vger.kernel.org, Arnd Bergmann <arnd@arndb.de>, Petr Mladek <pmladek@suse.com>, Thomas Gleixner <tglx@linutronix.de>, Sebastian Andrzej Siewior <bigeasy@linutronix.de>, Juri Lelli <jlelli@redhat.com>, "Luis Claudio R. Goncalves" <lgoncalv@redhat.com>, Andrew Morton <akpm@linux-foundation.org>, Vivek Goyal <vgoyal@redhat.com> Subject: Re: [PATCH v2] panic, kexec: Make __crash_kexec() NMI safe Date: Mon, 27 Jun 2022 13:42:22 +0100 [thread overview] Message-ID: <xhsmhpmiu5lch.mognet@vschneid.remote.csb> (raw) In-Reply-To: <87r13c7jyp.fsf@email.froward.int.ebiederm.org> On 25/06/22 12:04, Eric W. Biederman wrote: > I am not particularly fond of this patch as it adds more complexity than > is necessary to solve the problem. > > Calling a spade a spade PREEMPT_RT's mutex_trylock implementation is > broken as it can not support the use cases of an ordinary mutex_trylock. > I have not seen (possibly I skimmed too quickly) anywhere in the > discussion why PREEMPT_RT is not being fixed. Looking at the code > there is enough going on in try_to_take_rt_mutex that I can imagine > that some part of that code is not nmi safe. So I can believe > PREEMPT_RT may be unfix-ably broken. > AFAICT same goes for !PREEMPT_RT given the mutex_unlock(); it's a bit convoluted but you can craft scenarios where the NMI ends up spinning on mutex->wait_lock that is owned by the interrupted task, e.g. CPU0 CPU1 crash_shrink_memory() mutex_lock(); crash_get_memory_size() mutex_lock() raw_spin_lock(&lock->wait_lock); // Lock acquired <NMI> mutex_unlock() <Release lock->owner>; // Owner is free at this point so this succeeds mutex_trylock(); // No kexec_crash_image mutex_unlock() raw_spin_lock(&lock->wait_lock); > > At this point I recommend going back to being ``unconventional'' with > the kexec locking and effectively reverting commit 8c5a1cf0ad3a ("kexec: > use a mutex for locking rather than xchg()"). > > That would also mean that we don't have to worry about the lockdep code > doing something weird in the future and breaking kexec. > > Your change starting to is atomic_cmpxchng is most halfway to a revert > of commit 8c5a1cf0ad3a ("kexec: use a mutex for locking rather than > xchg()"). So we might as well go the whole way and just document that > the kexec on panic code can not use conventional kernel locking > primitives and has to dig deep and build it's own. At which point it > makes no sense for the rest of the kexec code to use anything different. > Hm, I'm a bit torn about that one, ideally I'd prefer to keep "homegrown" locking primitives to just where they are needed (loading & kexec'ing), but I'm also not immensely fond of the "hybrid" mutex+cmpxchg approach. > Eric _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
next prev parent reply other threads:[~2022-06-27 12:42 UTC|newest] Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-06-20 11:15 [PATCH v2] panic, kexec: Make __crash_kexec() NMI safe Valentin Schneider 2022-06-20 11:15 ` Valentin Schneider 2022-06-23 9:31 ` Sebastian Andrzej Siewior 2022-06-23 9:31 ` Sebastian Andrzej Siewior 2022-06-23 11:39 ` Valentin Schneider 2022-06-23 11:39 ` Valentin Schneider 2022-06-23 13:35 ` Sebastian Andrzej Siewior 2022-06-23 13:35 ` Sebastian Andrzej Siewior 2022-06-24 1:30 ` Baoquan He 2022-06-24 1:30 ` Baoquan He 2022-06-24 13:37 ` Valentin Schneider 2022-06-24 13:37 ` Valentin Schneider 2022-06-26 10:37 ` Baoquan He 2022-06-26 10:37 ` Baoquan He 2022-06-26 10:45 ` Baoquan He 2022-06-26 10:45 ` Baoquan He 2022-06-25 17:04 ` Eric W. Biederman 2022-06-25 17:04 ` Eric W. Biederman 2022-06-27 12:42 ` Valentin Schneider [this message] 2022-06-27 12:42 ` Valentin Schneider 2022-06-28 17:33 ` Valentin Schneider 2022-06-28 17:33 ` Valentin Schneider 2022-06-29 11:55 ` Petr Mladek 2022-06-29 11:55 ` Petr Mladek 2022-06-29 12:23 ` Valentin Schneider 2022-06-29 12:23 ` Valentin Schneider
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=xhsmhpmiu5lch.mognet@vschneid.remote.csb \ --to=vschneid@redhat.com \ --cc=akpm@linux-foundation.org \ --cc=arnd@arndb.de \ --cc=bigeasy@linutronix.de \ --cc=ebiederm@xmission.com \ --cc=jlelli@redhat.com \ --cc=kexec@lists.infradead.org \ --cc=lgoncalv@redhat.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-rt-users@vger.kernel.org \ --cc=pmladek@suse.com \ --cc=tglx@linutronix.de \ --cc=vgoyal@redhat.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.