Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 25 Feb 2021 15:56:07 +0000
From:   Daniel Thompson <daniel.thompson@linaro.org>
To:     Sumit Garg <sumit.garg@linaro.org>
Cc:     kgdb-bugreport@lists.sourceforge.net, akpm@linux-foundation.org,
        mhiramat@kernel.org, rostedt@goodmis.org,
        jason.wessel@windriver.com, dianders@chromium.org,
        peterz@infradead.org, stefan.saecherl@fau.de,
        qy15sije@cip.cs.fau.de, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] kgdb: Fix to kill breakpoints on initmem after boot
Message-ID: <20210225155607.634snzzq3w62kpkn@maple.lan>
References: <20210224081652.587785-1-sumit.garg@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210224081652.587785-1-sumit.garg@linaro.org>
Precedence: bulk

On Wed, Feb 24, 2021 at 01:46:52PM +0530, Sumit Garg wrote:
> Currently breakpoints in kernel .init.text section are not handled
> correctly while allowing to remove them even after corresponding pages
> have been freed.
> 
> Fix it via killing .init.text section breakpoints just prior to initmem
> pages being freed.
> 
> Suggested-by: Doug Anderson <dianders@chromium.org>
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>

I saw Andrew has picked this one up. That's ok for me:
Acked-by: Daniel Thompson <daniel.thompson@linaro.org>

I already enriched kgdbtest to cover this (and they pass) so I guess
this is also:
Tested-by: Daniel Thompson <daniel.thompson@linaro.org>

BTW this is not Cc:ed to stable and I do wonder if it crosses the
threshold to be considered a fix rather than a feature. Normally I
consider adding safety rails for kgdb to be a new feature but, in this
case, the problem would easily ensnare an inexperienced developer who is
doing nothing more than debugging their own driver (assuming they
correctly marked their probe function as .init) so I think this weighs
in favour of being a fix.


Daniel.


> ---
>  include/linux/kgdb.h      |  2 ++
>  init/main.c               |  1 +
>  kernel/debug/debug_core.c | 11 +++++++++++
>  3 files changed, 14 insertions(+)
> 
> diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
> index 57b8885708e5..3aa503ef06fc 100644
> --- a/include/linux/kgdb.h
> +++ b/include/linux/kgdb.h
> @@ -361,9 +361,11 @@ extern atomic_t			kgdb_active;
>  extern bool dbg_is_early;
>  extern void __init dbg_late_init(void);
>  extern void kgdb_panic(const char *msg);
> +extern void kgdb_free_init_mem(void);
>  #else /* ! CONFIG_KGDB */
>  #define in_dbg_master() (0)
>  #define dbg_late_init()
>  static inline void kgdb_panic(const char *msg) {}
> +static inline void kgdb_free_init_mem(void) { }
>  #endif /* ! CONFIG_KGDB */
>  #endif /* _KGDB_H_ */
> diff --git a/init/main.c b/init/main.c
> index c68d784376ca..a446ca3d334e 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -1417,6 +1417,7 @@ static int __ref kernel_init(void *unused)
>  	async_synchronize_full();
>  	kprobe_free_init_mem();
>  	ftrace_free_init_mem();
> +	kgdb_free_init_mem();
>  	free_initmem();
>  	mark_readonly();
>  
> diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
> index 229dd119f430..319381e95d1d 100644
> --- a/kernel/debug/debug_core.c
> +++ b/kernel/debug/debug_core.c
> @@ -465,6 +465,17 @@ int dbg_remove_all_break(void)
>  	return 0;
>  }
>  
> +void kgdb_free_init_mem(void)
> +{
> +	int i;
> +
> +	/* Clear init memory breakpoints. */
> +	for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) {
> +		if (init_section_contains((void *)kgdb_break[i].bpt_addr, 0))
> +			kgdb_break[i].state = BP_UNDEFINED;
> +	}
> +}
> +
>  #ifdef CONFIG_KGDB_KDB
>  void kdb_dump_stack_on_cpu(int cpu)
>  {
> -- 
> 2.25.1