| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Z9L5HsVzQ0bVZtjp@pathway.suse.cz>
Date: Thu, 13 Mar 2025 16:26:22 +0100
From: Petr Mladek <pmladek@...e.com>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: LKML <linux-kernel@...r.kernel.org>,
Linux Trace Kernel <linux-trace-kernel@...r.kernel.org>,
linux-mm@...ck.org, Masami Hiramatsu <mhiramat@...nel.org>,
Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Michael Petlan <mpetlan@...hat.com>,
Veronika Molnarova <vmolnaro@...hat.com>,
Suren Baghdasaryan <surenb@...gle.com>,
Rasmus Villemoes <linux@...musvillemoes.dk>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Tamir Duberstein <tamird@...il.com>,
Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [RESEND][PATCH] tracing: gfp: Remove duplication of recording
GFP flags
On Tue 2025-02-25 13:56:11, Steven Rostedt wrote:
> From: Steven Rostedt <rostedt@...dmis.org>
>
> The gfp_flags when recorded in the trace require being converted from
> their numbers to values. Various macros are used to help facilitate this,
> but there's two sets of macros that need to keep track of the same GFP
> flags to stay in sync.
>
> Commit 60295b944ff68 ("tracing: gfp: Fix the GFP enum values shown for
> user space tracing tools") added a TRACE_GFP_FLAGS macro that holds the
> enum ___GFP_*_BIT defined bits, and creates the TRACE_DEFINE_ENUM()
> wrapper around them.
>
> The __def_gfpflag_names() macro creates the mapping of various flags or
> multiple flags to give them human readable names via the __print_flags()
> tracing helper macro.
>
> As the TRACE_GFP_FLAGS is a subset of the __def_gfpflags_names(), it can
> be used to cover the individual bit names, by redefining the internal
> macro TRACE_GFP_EM():
>
> #undef TRACE_GFP_EM
> #define TRACE_GFP_EM(a) gfpflag_string(__GFP_##a),
>
> This will remove the bits that are duplicate between the two macros. If a
> new bit is created, only the TRACE_GFP_FLAGS needs to be updated and that
> will also update the __def_gfpflags_names() macro.
>
> Signed-off-by: Steven Rostedt (Google) <rostedt@...dmis.org>
> ---
> Last version: https://lore.kernel.org/20250116214439.046082618@goodmis.org
>
> This was originally sent with a patch that fixed the output of gfp flags
> in trace events to show human readable flags and not hex numbers.
>
> This patch on the other hand is a clean up as the there's now two macros
> that define the bits to print. This makes the one macro use the other
> macro that is a subset of the first.
>
> Can someone in the memory management subsystem either give me an acked-by
> and I can take this through my tree, or you can just take this through
> the memory management tree. Either way works for me.
>
> include/trace/events/mmflags.h | 41 +++++++++-------------------------
> 1 file changed, 10 insertions(+), 31 deletions(-)
>
> diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h
> index 72fbfe3caeaf..82371177ef79 100644
> --- a/include/trace/events/mmflags.h
> +++ b/include/trace/events/mmflags.h
> @@ -78,6 +78,13 @@ TRACE_DEFINE_ENUM(___GFP_LAST_BIT);
>
> #define gfpflag_string(flag) {(__force unsigned long)flag, #flag}
>
> +/*
> + * For the values that match the bits, use the TRACE_GFP_FLAGS
> + * which will allow any updates to be included automatically.
> + */
> +#undef TRACE_GFP_EM
> +#define TRACE_GFP_EM(a) gfpflag_string(__GFP_##a),
> +
> #define __def_gfpflag_names \
> gfpflag_string(GFP_TRANSHUGE), \
> gfpflag_string(GFP_TRANSHUGE_LIGHT), \
> @@ -91,41 +98,13 @@ TRACE_DEFINE_ENUM(___GFP_LAST_BIT);
> gfpflag_string(GFP_NOIO), \
> gfpflag_string(GFP_NOWAIT), \
> gfpflag_string(GFP_DMA), \
> - gfpflag_string(__GFP_HIGHMEM), \
> gfpflag_string(GFP_DMA32), \
> - gfpflag_string(__GFP_HIGH), \
> - gfpflag_string(__GFP_IO), \
> - gfpflag_string(__GFP_FS), \
> - gfpflag_string(__GFP_NOWARN), \
> - gfpflag_string(__GFP_RETRY_MAYFAIL), \
> - gfpflag_string(__GFP_NOFAIL), \
> - gfpflag_string(__GFP_NORETRY), \
> - gfpflag_string(__GFP_COMP), \
> - gfpflag_string(__GFP_ZERO), \
> - gfpflag_string(__GFP_NOMEMALLOC), \
> - gfpflag_string(__GFP_MEMALLOC), \
> - gfpflag_string(__GFP_HARDWALL), \
> - gfpflag_string(__GFP_THISNODE), \
> - gfpflag_string(__GFP_RECLAIMABLE), \
> - gfpflag_string(__GFP_MOVABLE), \
> - gfpflag_string(__GFP_ACCOUNT), \
> - gfpflag_string(__GFP_WRITE), \
> gfpflag_string(__GFP_RECLAIM), \
> - gfpflag_string(__GFP_DIRECT_RECLAIM), \
> - gfpflag_string(__GFP_KSWAPD_RECLAIM), \
> - gfpflag_string(__GFP_ZEROTAGS)
> -
> -#ifdef CONFIG_KASAN_HW_TAGS
> -#define __def_gfpflag_names_kasan , \
> - gfpflag_string(__GFP_SKIP_ZERO), \
> - gfpflag_string(__GFP_SKIP_KASAN)
> -#else
> -#define __def_gfpflag_names_kasan
> -#endif
> + TRACE_GFP_FLAGS \
> + { 0, "none" }
This causes regression in the printf selftest:
# modprobe test_printf
modprobe: ERROR: could not insert 'test_printf': Invalid argument
# dmesg | tail
[ 46.206779] test_printf: vsnprintf(buf, 256, "%pGg", ...) returned 15, expected 10
[ 46.208192] test_printf: vsnprintf(buf, 3, "%pGg", ...) returned 15, expected 10
[ 46.208196] test_printf: vsnprintf(buf, 0, "%pGg", ...) returned 15, expected 10
[ 46.208199] test_printf: kvasprintf(..., "%pGg", ...) returned 'none|0xfc000000', expected '0xfc000000'
[ 46.208202] test_printf: vsnprintf(buf, 256, "%pGg", ...) returned 26, expected 21
[ 46.208204] test_printf: vsnprintf(buf, 17, "%pGg", ...) returned 26, expected 21
[ 46.208206] test_printf: vsnprintf(buf, 0, "%pGg", ...) returned 26, expected 21
[ 46.208209] test_printf: kvasprintf(..., "%pGg", ...) returned '__GFP_HIGH|none|0xfc000000', expected '__GFP_HIGH|0xfc000000'
[ 46.208865] test_printf: failed 8 out of 448 tests
=> vprintf() started printing the "none|" string.
It seems to me that "{ 0, "none" }" was added as an "innocent" entry
to avoid the trailing "," generated by TRACE_GFP_FLAGS. So, it is
not really needed.
In fact, I think that it probably causes similar regression in the
trace output because the logic in trace_print_flags_seq()
seems to be the same as in format_flags() in lib/vsprintf.c.
The following worked for me:
diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h
index 82371177ef79..15aae955a10b 100644
--- a/include/trace/events/mmflags.h
+++ b/include/trace/events/mmflags.h
@@ -101,7 +101,7 @@ TRACE_DEFINE_ENUM(___GFP_LAST_BIT);
gfpflag_string(GFP_DMA32), \
gfpflag_string(__GFP_RECLAIM), \
TRACE_GFP_FLAGS \
- { 0, "none" }
+ { 0, NULL }
#define show_gfp_flags(flags) \
(flags) ? __print_flags(flags, "|", __def_gfpflag_names \
It seems to be safe because the callers end up the cycle when .name == NULL.
I think that it actually allows to remove similar trailing {} but I am not sure
if we want it.
>
> #define show_gfp_flags(flags) \
> - (flags) ? __print_flags(flags, "|", \
> - __def_gfpflag_names __def_gfpflag_names_kasan \
> + (flags) ? __print_flags(flags, "|", __def_gfpflag_names \
> ) : "none"
>
> #ifdef CONFIG_MMU
Best Regards,
Petr
Powered by blists - more mailing lists