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 <[email protected]>
Signed-off-by: Sumit Garg <[email protected]>
---
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
Hi,
On Wed, Feb 24, 2021 at 12:17 AM Sumit Garg <[email protected]> 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.
It might be worth it to mention that HW breakpoints aren't handled by
this patch but it's probably not such a big deal.
> Suggested-by: Doug Anderson <[email protected]>
> Signed-off-by: Sumit Garg <[email protected]>
> ---
> 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))
A nit, but instead of 0 should this be passing "BREAK_INSTR_SIZE" ?
Also: even if memory is about to get freed it still seems like it'd be
wise to call this:
kgdb_arch_remove_breakpoint(&kgdb_break[i]);
It looks like it shouldn't matter today but just in case an
architecture decides to do something fancy in the future it might not
hurt to tell it that the breakpoint is going away.
Everything here is pretty nitty, though. This looks good to me now.
Reviewed-by: Douglas Anderson <[email protected]>
On Wed, 24 Feb 2021 10:09:25 -0800 Doug Anderson <[email protected]> wrote:
> On Wed, Feb 24, 2021 at 12:17 AM Sumit Garg <[email protected]> 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.
>
> It might be worth it to mention that HW breakpoints aren't handled by
> this patch but it's probably not such a big deal.
I added that to the changelog, thanks.
I'll take your response to be the coveted acked-by :)
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 <[email protected]>
> Signed-off-by: Sumit Garg <[email protected]>
I saw Andrew has picked this one up. That's ok for me:
Acked-by: Daniel Thompson <[email protected]>
I already enriched kgdbtest to cover this (and they pass) so I guess
this is also:
Tested-by: Daniel Thompson <[email protected]>
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
On Wed, 24 Feb 2021 at 23:39, Doug Anderson <[email protected]> wrote:
>
> Hi,
>
> On Wed, Feb 24, 2021 at 12:17 AM Sumit Garg <[email protected]> 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.
>
> It might be worth it to mention that HW breakpoints aren't handled by
> this patch but it's probably not such a big deal.
>
>
> > Suggested-by: Doug Anderson <[email protected]>
> > Signed-off-by: Sumit Garg <[email protected]>
> > ---
> > 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))
>
> A nit, but instead of 0 should this be passing "BREAK_INSTR_SIZE" ?
>
> Also: even if memory is about to get freed it still seems like it'd be
> wise to call this:
>
> kgdb_arch_remove_breakpoint(&kgdb_break[i]);
>
> It looks like it shouldn't matter today but just in case an
> architecture decides to do something fancy in the future it might not
> hurt to tell it that the breakpoint is going away.
>
>
> Everything here is pretty nitty, though. This looks good to me now.
>
> Reviewed-by: Douglas Anderson <[email protected]>
Thanks Doug for your review.
-Sumit
On Wed, 24 Feb 2021 at 23:50, Andrew Morton <[email protected]> wrote:
>
> On Wed, 24 Feb 2021 10:09:25 -0800 Doug Anderson <[email protected]> wrote:
>
> > On Wed, Feb 24, 2021 at 12:17 AM Sumit Garg <[email protected]> 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.
> >
> > It might be worth it to mention that HW breakpoints aren't handled by
> > this patch but it's probably not such a big deal.
>
> I added that to the changelog, thanks.
>
Thanks Andrew for picking this up.
-Sumit
> I'll take your response to be the coveted acked-by :)
+ stable ML
On Thu, 25 Feb 2021 at 21:26, Daniel Thompson
<[email protected]> wrote:
>
> 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 <[email protected]>
> > Signed-off-by: Sumit Garg <[email protected]>
>
> I saw Andrew has picked this one up. That's ok for me:
> Acked-by: Daniel Thompson <[email protected]>
>
> I already enriched kgdbtest to cover this (and they pass) so I guess
> this is also:
> Tested-by: Daniel Thompson <[email protected]>
>
Thanks Daniel.
> 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.
>
Makes sense, Cc:ed stable.
-Sumit
>
> 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
On Fri, Feb 26, 2021 at 12:32:07PM +0530, Sumit Garg wrote:
> + stable ML
>
> On Thu, 25 Feb 2021 at 21:26, Daniel Thompson
> <[email protected]> wrote:
> >
> > 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 <[email protected]>
> > > Signed-off-by: Sumit Garg <[email protected]>
> >
> > I saw Andrew has picked this one up. That's ok for me:
> > Acked-by: Daniel Thompson <[email protected]>
> >
> > I already enriched kgdbtest to cover this (and they pass) so I guess
> > this is also:
> > Tested-by: Daniel Thompson <[email protected]>
> >
>
> Thanks Daniel.
>
> > 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.
> >
>
> Makes sense, Cc:ed stable.
<formletter>
This is not the correct way to submit patches for inclusion in the
stable kernel tree. Please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.
</formletter>
On Fri, 26 Feb 2021 at 13:01, Greg KH <[email protected]> wrote:
>
> On Fri, Feb 26, 2021 at 12:32:07PM +0530, Sumit Garg wrote:
> > + stable ML
> >
> > On Thu, 25 Feb 2021 at 21:26, Daniel Thompson
> > <[email protected]> wrote:
> > >
> > > 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 <[email protected]>
> > > > Signed-off-by: Sumit Garg <[email protected]>
> > >
> > > I saw Andrew has picked this one up. That's ok for me:
> > > Acked-by: Daniel Thompson <[email protected]>
> > >
> > > I already enriched kgdbtest to cover this (and they pass) so I guess
> > > this is also:
> > > Tested-by: Daniel Thompson <[email protected]>
> > >
> >
> > Thanks Daniel.
> >
> > > 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.
> > >
> >
> > Makes sense, Cc:ed stable.
>
>
> <formletter>
>
> This is not the correct way to submit patches for inclusion in the
> stable kernel tree. Please read:
> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> for how to do this properly.
>
> </formletter>
Thanks for the pointer, let me wait for this patch to land in Linus’
tree and then will drop a mail to [email protected].
-Sumit