[PATCH 0/3] Fixes and enhancements for memory debugging

[PATCH 0/3] Fixes and enhancements for memory debugging

[Glenn Washburn]

The first patch supercedes a patch sent earlier of the same subject. I've
removed the #undef if --mm-debug is not passwed to configure. The idea is
that even if --mm-debug is not passed to configure, the user should still
be able to enable memory debugging by passing -DMM_DEBUG in CFLAGS.

The second patch I found useful in dumping the memory allocation state from
a module.

And the third patch is a bug fix, without which enabling memory debugging
leads to a certain infinite recursion crash (when also enabling grub_mm_debug).
As an aside, this leads me to believe that no one has used this feature in
a very long time (and is not currently using it).

Glenn

Glenn Washburn (3):
  configure: Properly handle MM_DEBUG
  mm: Export grub_mm_dump and grub_mm_dump_free
  mm: Temporarily disable grub_mm_debug while calling grub_vprintf in
    grub_printf

 config.h.in           |  4 ++++
 configure.ac          |  6 ++++--
 grub-core/kern/misc.c | 19 +++++++++++++++++++
 include/grub/mm.h     |  4 ++--
 4 files changed, 29 insertions(+), 4 deletions(-)

-- 
2.27.0

[PATCH 1/3] configure: Properly handle MM_DEBUG

[Glenn Washburn]

Define MM_DEBUG in config.h when --enable-mm-debug is passed to configure.
It was being defined in config-util.h which only gets used when building
GRUB utilities for the host side. The enabling of debugging for memory
management in include/grub/mm.h explicitly does not happen when compiling
for the GRUB utilities. So this debugging code effectively could never be
enabled. Note, that MM_DEBUG is defined in an #if directive because the
enabling of debugging checks if MM_DEBUG is defined, not what its value is.
So even if MM_DEBUG were defined to nothing, the debugging code would
still be enabled.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 config.h.in  | 4 ++++
 configure.ac | 6 ++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/config.h.in b/config.h.in
index 9e8f9911b1..c1323a88e3 100644
--- a/config.h.in
+++ b/config.h.in
@@ -9,6 +9,10 @@
 #define GCRYPT_NO_DEPRECATED 1
 #define HAVE_MEMMOVE 1
 
+#if @MM_DEBUG@
+#define MM_DEBUG @MM_DEBUG@
+#endif
+
 /* Define to 1 to enable disk cache statistics.  */
 #define DISK_CACHE_STATS @DISK_CACHE_STATS@
 #define BOOT_TIME_STATS @BOOT_TIME_STATS@
diff --git a/configure.ac b/configure.ac
index 5c01af0fab..65d7f8a3be 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1519,9 +1519,11 @@ AC_ARG_ENABLE([mm-debug],
 	      AS_HELP_STRING([--enable-mm-debug],
                              [include memory manager debugging]))
 if test x$enable_mm_debug = xyes; then
-    AC_DEFINE([MM_DEBUG], [1],
-            [Define to 1 if you enable memory manager debugging.])
+    MM_DEBUG=1
+else
+    MM_DEBUG=0
 fi
+AC_SUBST([MM_DEBUG])
 
 AC_ARG_ENABLE([cache-stats],
 	      AS_HELP_STRING([--enable-cache-stats],
-- 
2.27.0

Re: [PATCH 1/3] configure: Properly handle MM_DEBUG

[Daniel Kiper]

On Tue, Feb 15, 2022 at 12:36:41PM -0600, Glenn Washburn wrote:
> Define MM_DEBUG in config.h when --enable-mm-debug is passed to configure.
> It was being defined in config-util.h which only gets used when building
> GRUB utilities for the host side. The enabling of debugging for memory
> management in include/grub/mm.h explicitly does not happen when compiling
> for the GRUB utilities. So this debugging code effectively could never be
> enabled. Note, that MM_DEBUG is defined in an #if directive because the
> enabling of debugging checks if MM_DEBUG is defined, not what its value is.
> So even if MM_DEBUG were defined to nothing, the debugging code would
> still be enabled.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

[PATCH 2/3] mm: Export grub_mm_dump and grub_mm_dump_free

[Glenn Washburn]

These functions may be useful within modules as well. Export them so that
modules can use them.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 include/grub/mm.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/grub/mm.h b/include/grub/mm.h
index 9c38dd3ca5..44fde7cb90 100644
--- a/include/grub/mm.h
+++ b/include/grub/mm.h
@@ -46,8 +46,8 @@ void grub_mm_check_real (const char *file, int line);
 /* Set this variable to 1 when you want to trace all memory function calls.  */
 extern int EXPORT_VAR(grub_mm_debug);
 
-void grub_mm_dump_free (void);
-void grub_mm_dump (unsigned lineno);
+void EXPORT_FUNC(grub_mm_dump_free) (void);
+void EXPORT_FUNC(grub_mm_dump) (unsigned lineno);
 
 #define grub_calloc(nmemb, size)	\
   grub_debug_calloc (GRUB_FILE, __LINE__, nmemb, size)
-- 
2.27.0

Re: [PATCH 2/3] mm: Export grub_mm_dump and grub_mm_dump_free

[Daniel Kiper]

On Tue, Feb 15, 2022 at 12:36:42PM -0600, Glenn Washburn wrote:
> These functions may be useful within modules as well. Export them so that
> modules can use them.

Though there are no users for these functions today. So, I am not
convinced we should export them.

Daniel

Re: [PATCH 2/3] mm: Export grub_mm_dump and grub_mm_dump_free

[Glenn Washburn]

On Tue, 22 Feb 2022 19:03:39 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Tue, Feb 15, 2022 at 12:36:42PM -0600, Glenn Washburn wrote:
> > These functions may be useful within modules as well. Export them so that
> > modules can use them.
> 
> Though there are no users for these functions today. So, I am not
> convinced we should export them.

How do you know there are no users for these functions today? As I see
it, the point of these functions is only for doing memory debugging.
They should not be enabled in general use. So no modules should ever
use them unconditionally. Also, here they are only export _if_
configure was passed --mm-debug or MM_DEBUG is defined, which is never
don't by default and likey only done when someone would be more likely
to use these functions. I'm not quite following what the concern is.

Glenn

Re: [PATCH 2/3] mm: Export grub_mm_dump and grub_mm_dump_free

[Daniel Kiper]

On Fri, Feb 25, 2022 at 03:04:57PM -0600, Glenn Washburn wrote:
> On Tue, 22 Feb 2022 19:03:39 +0100
> Daniel Kiper <dkiper@net-space.pl> wrote:
>
> > On Tue, Feb 15, 2022 at 12:36:42PM -0600, Glenn Washburn wrote:
> > > These functions may be useful within modules as well. Export them so that
> > > modules can use them.
> >
> > Though there are no users for these functions today. So, I am not
> > convinced we should export them.
>
> How do you know there are no users for these functions today? As I see

OK, to be precise currently there are no callers in the GRUB source code.

> it, the point of these functions is only for doing memory debugging.
> They should not be enabled in general use. So no modules should ever
> use them unconditionally. Also, here they are only export _if_
> configure was passed --mm-debug or MM_DEBUG is defined, which is never
> don't by default and likey only done when someone would be more likely
> to use these functions. I'm not quite following what the concern is.

Sorry, I missed they are exported only conditionally. So, taking into
account this Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>...

Daniel

[PATCH 3/3] mm: Temporarily disable grub_mm_debug while calling grub_vprintf in grub_printf

[Glenn Washburn]

To prevent infinite recursion when grub_mm_debug is on, disable it when
calling grub_vprintf. One such call loop is:
  grub_vprintf -> parse_printf_args -> parse_printf_arg_fmt
    -> grub_debug_calloc -> grub_printf -> grub_vprintf

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/kern/misc.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
index de40f566d6..cd6f460a21 100644
--- a/grub-core/kern/misc.c
+++ b/grub-core/kern/misc.c
@@ -113,10 +113,29 @@ grub_printf (const char *fmt, ...)
   va_list ap;
   int ret;
 
+#if defined(MM_DEBUG) && !defined(GRUB_UTIL) && !defined (GRUB_MACHINE_EMU)
+  /*
+   * To prevent infinite recursion when grub_mm_debug is on, disable it
+   * when calling grub_vprintf. One such call loop is:
+   *   grub_vprintf -> parse_printf_args -> parse_printf_arg_fmt
+   *     -> grub_debug_calloc -> grub_printf -> grub_vprintf
+   */
+  int grub_mm_debug_save = 0;
+  if (grub_mm_debug)
+    {
+      grub_mm_debug_save = grub_mm_debug;
+      grub_mm_debug = 0;
+    }
+#endif
+
   va_start (ap, fmt);
   ret = grub_vprintf (fmt, ap);
   va_end (ap);
 
+#if defined(MM_DEBUG) && !defined(GRUB_UTIL) && !defined (GRUB_MACHINE_EMU)
+  grub_mm_debug = grub_mm_debug_save;
+#endif
+
   return ret;
 }
 
-- 
2.27.0

Re: [PATCH 3/3] mm: Temporarily disable grub_mm_debug while calling grub_vprintf in grub_printf

[Daniel Kiper]

On Tue, Feb 15, 2022 at 12:36:43PM -0600, Glenn Washburn wrote:
> To prevent infinite recursion when grub_mm_debug is on, disable it when
> calling grub_vprintf. One such call loop is:
>   grub_vprintf -> parse_printf_args -> parse_printf_arg_fmt
>     -> grub_debug_calloc -> grub_printf -> grub_vprintf
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel