Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 22 May 2023 11:39:27 -0700 (PDT)
From:   David Rientjes <rientjes@google.com>
To:     David Hildenbrand <david@redhat.com>
cc:     Andrew Morton <akpm@linux-foundation.org>,
        Michal Hocko <mhocko@kernel.org>, Alex Shi <alexs@kernel.org>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Matthew Wilcox <willy@infradead.org>,
        Alexander Duyck <alexanderduyck@fb.com>,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [patch] mm, debug: allow suppressing panic on CONFIG_DEBUG_VM
 checks
In-Reply-To: <b4dce681-e53c-a6fd-2dab-62a82ebc6dff@redhat.com>
Message-ID: <53dd9df8-e88f-f466-89f9-3fa141a10267@google.com>
References: <c9abf109-80f2-88f5-4aae-d6fd4a30bcd3@google.com> <b4dce681-e53c-a6fd-2dab-62a82ebc6dff@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Mon, 22 May 2023, David Hildenbrand wrote:

> Let me CC Linus, he might have an opinion on this.
> 
> On 22.05.23 01:07, David Rientjes wrote:
> > CONFIG_DEBUG_VM is used to enable additional MM debug checks at runtime.
> > This can be used to catch latent kernel bugs.
> > 
> > Because this is mainly used for debugging, it is seldom enabled in
> > production environments, including due to the added performance overhead.
> > Thus, the choice between VM_BUG_ON() and VM_WARN_ON() is somewhat loosely
> > defined.
> > 
> > VM_BUG_ON() is often used because debuggers would like to collect crash
> > dumps when unexpected conditions occur.
> > 
> > When CONFIG_DEBUG_VM is enabled on a very small set of production
> > deployments to catch any unexpected condition, however, VM_WARN_ON()
> > could be used as a substitute.  In these configurations, it would be
> > useful to surface the unexpected condition in the kernel log but not
> > panic the system.
> > 
> > In other words, it would be useful if checks done by CONFIG_DEBUG_ON
> > could both generate crash dumps for kernel developers *and* surface
> > issues but not crash depending on how it's configured.
> > 
> >   [ If it is really unsafe to continue operation, then BUG_ON() would be
> >     used instead so that the kernel panics regardless of whether
> >     CONFIG_DEBUG_VM is enabled or not. ]
> > 
> > Introduce the ability to suppress kernel panics when VM_BUG_ON*() variants
> > are used.  This leverages the existing vm_debug= kernel command line
> > option.
> > 
> > Additionally, this can reduce the risk of systems boot looping if
> > VM_BUG_ON() conditions are encountered during bootstrap.
> > 
> > Signed-off-by: David Rientjes <rientjes@google.com>
> > ---
> > Note: the vm_debug= kernel parameter is only extensible for new debug
> > options, not for disabling existing debug options.
> > 
> > When adding the ability to selectively disable existing debug options,
> > such as in this patch, admins would need to know this future set of debug
> > options in advance.  In other words, if admins would like to preserve the
> > existing behavior of BUG() when VM_BUG_ON() is used after this patch, they
> > would have had to have the foresight to use vm_debug=B.
> > 
> > It would be useful to rewrite the vm_debug= interface to select the
> > specific options to disable rather than "disable all, and enable those
> > that are specified."  This could be done by making vm_debug only disable
> > the listed debug options rather than enabling them.
> > 
> > This change could be done before this patch is merged if that's the agreed
> > path forward.
> 
> 
> In general, I am not a fan of this. Someone told the system to VM_BUG_ON, but
> we ignore that and default to a warning. Yes, VM_BUG on get compiled out
> without CONFIG_DEBUG_VM, but we detected something (with more checks enabled!)
> that doesn't want the system to continue (could be an unrecoverable situation
> leading to data loss, for example).
> 

I think VM_BUG_ON*() and friends are used to crash the kernel for 
debugging so that we get a crash dump and because some variants don't 
exist for VM_WARN_ON().  There's no VM_WARN_ON_PAGE(), for example, unless 
implicitly converted with this patch.

I'm having a hard time finding a case where VM_BUG_ON() should *require* a 
kernel crash.  I'd be interested to know of these if they exist, though, 
because we have had good success discovering latent kernel bugs that have 
been reported to upstream with the exact approach being proposed here on a 
small set of production hosts.  To safely do that, we audited existing 
VM_BUG_ON()s in the code to make sure there was nothing that absolutely 
required a kernel crash.  That may have changed in more recent kernels, so 
any examples would be very useful.

> Yes, we want to convert more VM_BUG to VM_WARN (or rather WARN+recovery code
> as documented in coding-style.rst ), or even simply remove some of the old
> VM_BUG leftovers that might no longer be required. But then I'd much invest
> more time doing that step by step (keeping the VM_BUG + BUG that are
> absolutely reasonable) instead of adding such a config options.
> 

I'm not sure we actually want to do that, though, since VM_BUG_ON() does 
have a lot of benefit when debugging something: it can generate a crash 
dump that is tremendously useful to the kernel debugger who is iterating 
on their patch set.

The goal of this patch is to provide an additional use case for 
CONFIG_DEBUG_VM: we want to preserve the ability for kernel developers to 
quickly crash their debug kernel during development and add the additional 
use case of surfacing WARN_ON()s to the kernel log for unexpected issues 
on a small set of production hosts without crashing them.  This second use 
case has been very helpful in finding latent kernel bugs at runtime that 
we didn't even know existed in the kernel and could only be found while 
running on a limited production deployment.  If we had to crash the kernel 
to find those, and terminate all the associated guests/workloads, that 
would be a non-starter for us.

> 
> > ---
> >   .../admin-guide/kernel-parameters.txt         |  1 +
> >   include/linux/mmdebug.h                       | 20 ++++++++++++++-----
> >   mm/debug.c                                    | 14 ++++++++++++-
> >   3 files changed, 29 insertions(+), 6 deletions(-)
> > 
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt
> > b/Documentation/admin-guide/kernel-parameters.txt
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -6818,6 +6818,7 @@
> >   			debugging features.
> >     			Available options are:
> > +			  B	Enable panic on MM debug checks
> >   			  P	Enable page structure init time poisoning
> >   			  -	Disable all of the above options
> >   diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
> > --- a/include/linux/mmdebug.h
> > +++ b/include/linux/mmdebug.h
> > @@ -13,34 +13,44 @@ void dump_page(struct page *page, const char *reason);
> >   void dump_vma(const struct vm_area_struct *vma);
> >   void dump_mm(const struct mm_struct *mm);
> >   +extern bool debug_vm_bug_enabled;
> > +
> >   #ifdef CONFIG_DEBUG_VM
> > -#define VM_BUG_ON(cond) BUG_ON(cond)
> > +#define VM_BUG_ON(cond)
> > \
> > +	do {								\
> > +		if (unlikely(cond)) {					\
> > +			if (likely(debug_vm_bug_enabled))		\
> > +				BUG();					\
> > +			else						\
> > +				WARN_ON(1);				\
> > +		}							\
> > +	} while (0)
> >   #define VM_BUG_ON_PAGE(cond, page)					\
> >   	do {								\
> >   		if (unlikely(cond)) {					\
> >   			dump_page(page, "VM_BUG_ON_PAGE("
> > __stringify(cond)")");\
> > -			BUG();						\
> > +			VM_BUG_ON(1);					\
> >   		}							\
> >   	} while (0)
> >   #define VM_BUG_ON_FOLIO(cond, folio)
> > \
> >   	do {								\
> >   		if (unlikely(cond)) {					\
> >   			dump_page(&folio->page, "VM_BUG_ON_FOLIO("
> > __stringify(cond)")");\
> > -			BUG();						\
> > +			VM_BUG_ON(1);					\
> >   		}							\
> >   	} while (0)
> >   #define VM_BUG_ON_VMA(cond, vma)					\
> >   	do {								\
> >   		if (unlikely(cond)) {					\
> >   			dump_vma(vma);					\
> > -			BUG();						\
> > +			VM_BUG_ON(1);					\
> >   		}							\
> >   	} while (0)
> >   #define VM_BUG_ON_MM(cond, mm)
> > \
> >   	do {								\
> >   		if (unlikely(cond)) {					\
> >   			dump_mm(mm);					\
> > -			BUG();						\
> > +			VM_BUG_ON(1);					\
> >   		}							\
> >   	} while (0)
> >   #define VM_WARN_ON_ONCE_PAGE(cond, page)	({			\
> > diff --git a/mm/debug.c b/mm/debug.c
> > --- a/mm/debug.c
> > +++ b/mm/debug.c
> > @@ -224,10 +224,15 @@ void dump_mm(const struct mm_struct *mm)
> >   }
> >   EXPORT_SYMBOL(dump_mm);
> >   +/* If disabled, warns but does not panic on added CONFIG_DEBUG_VM checks
> > */
> > +bool debug_vm_bug_enabled = true;
> > +EXPORT_SYMBOL(debug_vm_bug_enabled);
> > +
> >   static bool page_init_poisoning __read_mostly = true;
> >     static int __init setup_vm_debug(char *str)
> >   {
> > +	bool __debug_vm_bug_enabled = true;
> >   	bool __page_init_poisoning = true;
> >     	/*
> > @@ -237,13 +242,17 @@ static int __init setup_vm_debug(char *str)
> >   	if (*str++ != '=' || !*str)
> >   		goto out;
> >   +	__debug_vm_bug_enabled = false;
> >   	__page_init_poisoning = false;
> >   	if (*str == '-')
> >   		goto out;
> >     	while (*str) {
> >   		switch (tolower(*str)) {
> > -		case'p':
> > +		case 'b':
> > +			__debug_vm_bug_enabled = true;
> > +			break;
> > +		case 'p':
> >   			__page_init_poisoning = true;
> >   			break;
> >   		default:
> > @@ -254,9 +263,12 @@ static int __init setup_vm_debug(char *str)
> >   		str++;
> >   	}
> >   out:
> > +	if (debug_vm_bug_enabled && !__debug_vm_bug_enabled)
> > +		pr_warn("Panic on MM debug checks disabled by kernel command
> > line option 'vm_debug'\n");
> >   	if (page_init_poisoning && !__page_init_poisoning)
> >   		pr_warn("Page struct poisoning disabled by kernel command line
> > option 'vm_debug'\n");
> >   +	debug_vm_bug_enabled = __debug_vm_bug_enabled;
> >   	page_init_poisoning = __page_init_poisoning;
> >     	return 1;
> > 
> 
> -- 
> Thanks,
> 
> David / dhildenb
> 
>