On Wed, Jul 08, 2026 at 08:38:13AM +0100, Lorenzo Stoakes wrote:
> This is an unmergeable mess of a patch but I have broader concerns:
>
> * I'm very concerned that this is just one giant workaround to
> EXPORT_SYMBOLS() for unexported memory, or really just a way for drivers
> to stuff arbitrary VAs/PAs in some global region.
>
> * I'm also very concerned about memory lifecycle - saying a physical range
> X-Y is Z doesn't mean it's forever that? And for virtual addresses it's
> even more problematic - are you going to put vmalloc pointers there that
> can get unmapped? Or direct map ranges that might end up unmapped for
> various reasons? What about memory hotplug?...
Static entries (patches 02–21) are all kernel .data/.bss symbols
init_mm, cpu masks, kallsyms arrays, printk ring buffer descriptors,
etc. These are never freed, never remapped, and live for the full kernel
lifetime. There is no lifecycle issue for these.
The unregister API exists precisely for lifecycle management. No
vmalloc memory is registered in this series. The only dynamic
registrations today are the ELF header (CMA allocation,
intentionally never freed) and vmcoreinfo (a static kernel
buffer).
Memory hotplug is a legitimate gap for the NUMA node_data registration.
Currently, all the tagged region are required by crash-utility
to launch and get dmesg from the collected elf.
>
> * I hate the idea of exposing information about physical (and virtual!)
> memory ranges to drivers arbitrarily, drivers are notorious for doing the
> wrong thing (TM) and this is just asking for people to do 'interesting'
> things with this stuff.
flow:
kernel subsystems (owners of the data)
│ annotate their own symbols
▼
meminspect table (PA + size, kernel-internal)
│ pushed to registered backends only
▼
platform backend driver (e.g. qcom-minidump)
│ writes PA+size pairs to SMEM
▼
firmware ── actual consumer
│ reads kernel memory directly via hardware privilege
▼
crash dump
The driver should not dereference the addresses. It should not "do the wrong
thing" with them beyond writing them into a firmware-readable table —
which is its entire purpose.
The closest upstream analogy is /proc/vmcoreinfo, which already exposes
kernel symbol addresses, struct offsets, and memory layout to crash
analysis tools. meminspect is a more selective, machine-readable variant
of the same concept, scoped to platforms where kdump and pstore are not
viable (no kexec support, RAM contents lost on reboot, no persistent
storage).
I can further fine tune only restrict to specific backend or remove traverse
or notifier API that arbitrary code can call. The registration can be gated by
backend platform driver, not by an open subscription API.
If the concern is that future drivers will misuse the framework beyond
its intended purpose i.e., a valid long-term worry, and I am open
to adding documentation constraints or restricting the API further
(e.g., limiting to a single backend registration).
>
> On Wed, Jul 08, 2026 at 11:01:40AM +0530, Mukesh Ojha wrote:
> > From: Eugen Hristev <[email protected]>
> >
> > Memory inspection mechanism allows registration of a specific
> > memory area (or object) for later inspection purposes. Ranges are
> > added into an inspection table, which can be requested and
> > analyzed by specific drivers. Drivers would interface with any
> > hardware mechanism that will allow inspection of the data,
> > including but not limited to: dumping for debugging, creating a
> > core dump, analysis, or statistical information. Drivers can
> > register a notifier to know when new objects are registered, or
> > to traverse an existing inspection table. The inspection table is
> > created ahead of time such that it can be later used regardless
> > of the state of the kernel (running, frozen, crashed, or any
> > particular state).
>
> This is the complete opposite of what a commit message should be...
>
> Firstly it's an totally unacceptable, unreadable wall of text.
>
> Secondly you're not at all describing what you're doing in this commit
> here.
>
> As far as I can tell from the code here, you're explicitly only supporting
> ELF registered ranges (perhaps static also?)
>
> I mean this is clearly not the whole thing, despite what the commit msg
> suggests, as you have 25 further commits (!)
>
> The commit message should explain WHAT THE PATCH IS DOING, the trade-offs,
> the motivation, anything that needs to be called out.
>
> It does absolutely none of that and instead you get what seems like
> marketing copy in a giant unbroken, unreadable, paragraph.
Surely, I could have explained more about the problem and how it
tries to solve it, and its limitations if any, instead of just
the introduction part.
>
> >
> > Signed-off-by: Eugen Hristev <[email protected]>
> > Signed-off-by: Mukesh Ojha <[email protected]>
>
> This patch is seriously far from being even vaguely upstreamable, and for
> an effective v5 this is concerning.
>
> > ---
> > Documentation/dev-tools/index.rst | 1 +
> > Documentation/dev-tools/meminspect.rst | 144 ++++++++++
> > MAINTAINERS | 9 +
> > include/asm-generic/vmlinux.lds.h | 13 +
> > include/linux/meminspect.h | 270 +++++++++++++++++++
> > init/Kconfig | 1 +
> > kernel/Makefile | 1 +
> > kernel/meminspect/Kconfig | 18 ++
> > kernel/meminspect/Makefile | 3 +
> > kernel/meminspect/meminspect.c | 474
> > +++++++++++++++++++++++++++++++++
> > 10 files changed, 934 insertions(+)
>
> This changelog is completely ridiculous for a single patch.
>
> This is kernel 101 stuff... you don't send everything in a single commit
> including documentation + MAINTAINERS changes...!
>
> You have to split things up into logical parts.
>
I surely did a very poor job not changing too much from version
v1. It is indeed waiting for such comments to be addressed for it
to be on the right path, thank you, will split it logically.
> >
> > diff --git a/Documentation/dev-tools/index.rst
> > b/Documentation/dev-tools/index.rst
> > index 59cbb77b33ff..ea2989ca1566 100644
> > --- a/Documentation/dev-tools/index.rst
> > +++ b/Documentation/dev-tools/index.rst
> > @@ -40,3 +40,4 @@ Documentation/process/debugging/index.rst
> > autofdo
> > propeller
> > container
> > + meminspect
> > diff --git a/Documentation/dev-tools/meminspect.rst
> > b/Documentation/dev-tools/meminspect.rst
> > new file mode 100644
> > index 000000000000..4ca10e33e4fd
> > --- /dev/null
> > +++ b/Documentation/dev-tools/meminspect.rst
> > @@ -0,0 +1,144 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +==========
> > +meminspect
> > +==========
> > +
> > +This document provides information about the meminspect feature.
> > +
> > +Overview
> > +========
> > +
> > +meminspect is a mechanism that allows the kernel to register a chunk of
> > +memory into a table, to be used at a later time for a specific
> > +inspection purpose like debugging, memory dumping or statistics.
>
> What's a chunk of memory? Physical memory? MMIO ranges? Virtual ranges? Direct
> map? Vmalloc?
>
> Be more specific.
That's a valid concern to be addressed; it should be a directly
mapped region and should be documented and WARN on if it is not
directly mapped memory.
>
> > +
> > +meminspect allows drivers to traverse the inspection table on demand,
> > +or to register a notifier to be called whenever a new entry is being added
> > +or removed.
>
> I absolutely hate the idea of exposing a table of pointers to 'memory' that
> has
> lifetime information to drivers with arbitrary context.
>
> What about security?
I see a point with the current implementation: meminspect core is
more open in such a sense that anybody can register a notifier
and traverse the table, so it really need not be a device.
However, current users like qcom minidump and google debug-kinfo
are platform drivers. I can fix that by removing notifiers and
having the backend supply ops callbacks, like how the pstore
backend does it.
>
> > +
> > +The reasoning for meminspect is also to minimize the required information
> > +in case of a kernel problem. For example a traditional debug method
> > involves
> > +dumping the whole kernel memory and then inspecting it. Meminspect allows
> > the
> > +users to select which memory is of interest, in order to help this specific
> > +use case in production, where memory and connectivity are limited.
>
> Again this is so vague it's uselses...
The intention of meminspect is to get minimal information from
the kernel that gets registered with any backend firmware so that
the firmware can dump it from a field device such as a mobile
phone or any device..
>
> > +
> > +Although the kernel has multiple internal mechanisms, meminspect fits
> > +a particular model which is not covered by the others.
>
> This is nebulous nonsense you have to explain WHY and what exactly it gives
> that
> others don't.
>
> You can very well inspect specific areas of memory using
Ack, it should rightly explain why the other models do not fit.
>
> > +
> > +meminspect Internals
> > +====================
> > +
> > +API
> > +---
> > +
> > +Static memory can be registered at compile time, by instructing the
> > compiler
> > +to create a separate section with annotation info.
> > +For each such annotated memory (variables usually), a dedicated struct
> > +is created with the required information.
> > +To achieve this goal, some basic APIs are available:
> > +
> > +* MEMINSPECT_ENTRY(idx, sym, sz)
> > + is the basic macro that takes an ID, the symbol, and a size.
> > +
> > +To make it easier, some wrappers are also defined
> > +
> > +* MEMINSPECT_SIMPLE_ENTRY(sym)
> > + uses the dedicated MEMINSPECT_ID_##sym with a size equal to sizeof(sym)
> > +
> > +* MEMINSPECT_NAMED_ENTRY(name, sym)
> > + is a simple entry that has an id that cannot be derived from the sym,
> > + so a name has to be provided
> > +
> > +* MEMINSPECT_AREA_ENTRY(sym, sz)
> > + registers sym, but with the size given as sz, useful for e.g.
> > + arrays which do not have a fixed size at compile time.
> > +
> > +For dynamically allocated memory, or for other cases, the following APIs
> > +are defined::
> > +
> > + meminspect_register_id_pa(enum meminspect_uid id, phys_addr_t zone,
> > + size_t size, unsigned int type);
> > +
> > +which takes the ID and the physical address.
>
> What about lifetime?...
>
> Exposing arbitrary physical memory to drivers is just a big nope.
>
> > +
> > +Similarly there are variations:
> > +
> > + * meminspect_register_pa() omits the ID
> > + * meminspect_register_id_va() requires the ID but takes a virtual address
> > + * meminspect_register_va() omits the ID and requires a virtual address
> > +
> > +If the ID is not given, the next available dynamic ID is allocated.
> > +
> > +To unregister a dynamic entry, some APIs are defined:
> > + * meminspect_unregister_pa(phys_addr_t zone, size_t size);
> > + * meminspect_unregister_id(enum meminspect_uid id);
> > + * meminspect_unregister_va(va, size);
> > +
> > +All of the above have a lock variant that ensures the lock on the table
> > +is taken.
> > +
> > +
> > +meminspect drivers
> > +------------------
> > +
> > +Drivers are free to traverse the table by using a dedicated function::
> > +
> > + meminspect_traverse(void *priv, meminspect_iter_cb_t cb)
> > +
> > +The callback is called for each entry in the table.
> > +
> > +Drivers can also register a notifier with meminspect_notifier_register()
> > +and unregister with meminspect_notifier_unregister() to be called when a
> > new
> > +entry is added or removed.
> > +
> > +Data structures
> > +---------------
> > +
> > +The regions are stored in a simple fixed size array. It avoids
> > +memory allocation overhead. This is not performance critical nor does
> > +allocating a few hundred entries create a memory consumption problem.
> > +
> > +The static variables registered into meminspect are annotated into
> > +a dedicated .inspect_table memory section. This is then walked by
> > meminspect
> > +at a later time and each variable is then copied to the whole inspect
> > table.
> > +
> > +meminspect Initialization
> > +-------------------------
> > +
> > +At any time, meminspect is ready to accept region registration
> > +from any part of the kernel. The table does not require any initialization.
>
>
>
> > +In case CONFIG_CRASH_DUMP is enabled, meminspect creates an ELF header
> > +corresponding to a core dump image, in which each region is added as a
> > +program header. In this scenario, the first region is this ELF header, and
> > +the second region is the vmcoreinfo ELF note.
> > +By using this mechanism, all the meminspect table, if dumped, can be
> > +concatenated to obtain a core image that is loadable with the `crash` tool.
> > +
> > +meminspect example
> > +==================
> > +
> > +A simple scenario for meminspect is the following:
> > +The kernel registers the linux_banner variable into meminspect with
> > +a simple annotation like::
> > +
> > + MEMINSPECT_SIMPLE_ENTRY(linux_banner);
> > +
> > +The meminspect late initcall will parse the compile-time table
> > +and copy the entry information into the inspection table.
> > +At a later point, any interested driver can call the traverse function to
> > +find out all entries in the table.
> > +A specific driver will then note into a specific table the address of the
> > +banner and the size of it.
> > +The specific table is then written to a shared memory area that can be
> > +read by upper level firmware.
>
> This feels like you're finding a way to export things that aren't
> exported...?!
Umm... The intention was to share this information for debugging
purposes.
>
> > +When the kernel freezes (hypothetically), the kernel will no longer feed
> > +the watchdog. The watchdog will trigger a higher exception level interrupt
> > +which will be handled by the upper level firmware. This firmware will then
> > +read the shared memory table and find an entry with the start and size of
> > +the banner. It will then copy it for debugging purpose. The upper level
>
> This feels like a giant workaround to provide EXPORT_SYMBOLS()
Well, it was not an intention to share this with everybody but
with the backend device representing a platform device.
>
> > +firmware will then be able to provide useful debugging information,
> > +like in this example, the banner.
> > +
> > +As seen here, meminspect facilitates the interaction between the kernel
> > +and a specific firmware.
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index c3f72058a2f2..fdad8ef377c8 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -16892,6 +16892,15 @@ F: arch/*/include/asm/sync_core.h
> > F: include/uapi/linux/membarrier.h
> > F: kernel/sched/membarrier.c
> >
> > +MEMINSPECT
> > +M: Eugen Hristev <[email protected]>
> > +M: Mukesh Ojha <[email protected]>
> > +L: [email protected]
> > +S: Maintained
> > +F: Documentation/dev-tools/meminspect.rst
> > +F: include/linux/meminspect.h
> > +F: kernel/meminspect/*
> > +
> > MEMBLOCK AND MEMORY MANAGEMENT INITIALIZATION
> > M: Mike Rapoport <[email protected]>
> > L: [email protected]
> > diff --git a/include/asm-generic/vmlinux.lds.h
> > b/include/asm-generic/vmlinux.lds.h
> > index 5659f4b5a125..3122ddf5517a 100644
> > --- a/include/asm-generic/vmlinux.lds.h
> > +++ b/include/asm-generic/vmlinux.lds.h
> > @@ -506,6 +506,8 @@
> > FW_LOADER_BUILT_IN_DATA \
> > TRACEDATA \
> > \
> > + MEMINSPECT_TABLE \
> > + \
> > PRINTK_INDEX \
> > \
> > /* Kernel symbol table */ \
> > @@ -904,6 +906,17 @@
> > #define TRACEDATA
> > #endif
> >
> > +#ifdef CONFIG_MEMINSPECT
> > +#define MEMINSPECT_TABLE \
> > + . = ALIGN(8); \
> > + .inspect_table : AT(ADDR(.inspect_table) - LOAD_OFFSET) { \
> > + BOUNDED_SECTION_POST_LABEL(.inspect_table, \
> > + __inspect_table, , _end) \
> > + }
> > +#else
> > +#define MEMINSPECT_TABLE
> > +#endif
> > +
> > #ifdef CONFIG_PRINTK_INDEX
> > #define PRINTK_INDEX
> > \
> > .printk_index : AT(ADDR(.printk_index) - LOAD_OFFSET) { \
> > diff --git a/include/linux/meminspect.h b/include/linux/meminspect.h
> > new file mode 100644
> > index 000000000000..a9eb93c6ea2f
> > --- /dev/null
> > +++ b/include/linux/meminspect.h
> > @@ -0,0 +1,270 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef _MEMINSPECT_H
> > +#define _MEMINSPECT_H
> > +
> > +#include <asm/page.h>
> > +#include <linux/notifier.h>
> > +
> > +enum meminspect_uid {
> > + MEMINSPECT_ID_NONE = 0,
> > + MEMINSPECT_ID_ELF,
> > + MEMINSPECT_ID_VMCOREINFO,
> > + MEMINSPECT_ID_CONFIG,
> > + MEMINSPECT_ID__totalram_pages,
> > + MEMINSPECT_ID___cpu_possible_mask,
> > + MEMINSPECT_ID___cpu_present_mask,
> > + MEMINSPECT_ID___cpu_online_mask,
> > + MEMINSPECT_ID___cpu_active_mask,
> > + MEMINSPECT_ID_mem_section,
> > + MEMINSPECT_ID_jiffies_64,
> > + MEMINSPECT_ID_linux_banner,
> > + MEMINSPECT_ID_nr_threads,
> > + MEMINSPECT_ID_total_nr_irqs,
> > + MEMINSPECT_ID_tainted_mask,
> > + MEMINSPECT_ID_taint_flags,
> > + MEMINSPECT_ID_node_states,
> > + MEMINSPECT_ID___per_cpu_offset,
> > + MEMINSPECT_ID_nr_swapfiles,
> > + MEMINSPECT_ID_init_uts_ns,
> > + MEMINSPECT_ID_printk_rb_static,
> > + MEMINSPECT_ID_printk_rb_dynamic,
> > + MEMINSPECT_ID_prb,
> > + MEMINSPECT_ID_prb_descs,
> > + MEMINSPECT_ID_prb_infos,
> > + MEMINSPECT_ID_prb_data,
> > + MEMINSPECT_ID_clear_seq,
> > + MEMINSPECT_ID_high_memory,
> > + MEMINSPECT_ID_init_mm,
> > + MEMINSPECT_ID__sinittext,
> > + MEMINSPECT_ID__einittext,
> > + MEMINSPECT_ID__end,
> > + MEMINSPECT_ID__text,
> > + MEMINSPECT_ID__stext,
> > + MEMINSPECT_ID__etext,
> > + MEMINSPECT_ID_kallsyms_num_syms,
> > + MEMINSPECT_ID_kallsyms_offsets,
> > + MEMINSPECT_ID_kallsyms_names,
> > + MEMINSPECT_ID_kallsyms_token_table,
> > + MEMINSPECT_ID_kallsyms_token_index,
> > + MEMINSPECT_ID_kallsyms_markers,
> > + MEMINSPECT_ID_kallsyms_seqs_of_names,
> > + MEMINSPECT_ID_swapper_pg_dir,
> > + MEMINSPECT_ID_DYNAMIC,
> > + MEMINSPECT_ID_MAX = 201,
>
> Err why 201?
Ideally, there should not be any limit, but to add more context,
this was one of the backend limitation and should be removed from
here.
>
> > +};
>
> This feels like a bunch of stuff you didn't document?
If it looks worth documenting, I will do it..
>
> > +
> > +#define MEMINSPECT_TYPE_REGULAR 0
> > +
> > +#define MEMINSPECT_NOTIFIER_ADD 0
> > +#define MEMINSPECT_NOTIFIER_REMOVE 1
>
> Broken indentation...
Ack.
>
> > +
> > +/**
> > + * struct inspect_entry - memory inspect entry information
> > + * @id: unique id for this entry
> > + * @va: virtual address for the memory (pointer)
>
> Err what?? Virtual address as mapped how? Are you now assuming things are
> mapped?
It should be directly mapped virtual addresses and should be
documented.
>
> > + * @pa: physical address for the memory
> > + * @size: size of the memory area of this entry
> > + * @type: type of the entry (class)
>
> 'Class'?
Ack.
>
> > + */
> > +struct inspect_entry {
> > + enum meminspect_uid id;
> > + void *va;
> > + phys_addr_t pa;
> > + size_t size;
> > + unsigned int type;
> > +};
> > +
> > +/**
> > + * typedef meminspect_iter_cb_t - Iterator callback for meminspect
> > traversal
> > + * @priv: private data passed through from the caller of
> > meminspect_traverse()
> > + * @ie: pointer to the current inspect_entry; read-only, table lock held
> > + *
> > + * The table lock is held by the caller; the callback must not call any
> > + * meminspect_table_lock() or meminspect_table_unlock() variants.
> > + */
> > +typedef void (*meminspect_iter_cb_t)(void *priv, const struct
> > inspect_entry *ie);
>
> So drivers can now just access some tagged region, with a VA that err I
> guess we just assume is valid when accessed and a PA that err I guess we
> just assume is still being used for what it was tagged for?
>
Currently, all the tagged one are trusted as directly mapped
one.
> This seems insane?
>
> > +
> > +#ifdef CONFIG_MEMINSPECT
> > +/* .inspect_table section table markers*/
> > +extern const struct inspect_entry __inspect_table[];
> > +extern const struct inspect_entry __inspect_table_end[];
> > +
> > +/*
> > + * Annotate a static variable into inspection table.
> > + * Can be called multiple times for the same ID, in which case
> > + * multiple table entries will be created
> > + */
> > +#define MEMINSPECT_ENTRY(idx, sym, sz)
> > \
> > + static const struct inspect_entry __UNIQUE_ID(__inspect_entry_##idx)
> > \
> > + __used __section(".inspect_table") = {
> > \
> > + .id = idx,
> > \
> > + .va = (void *)&(sym),
> > \
> > + .size = (sz),
> > \
> > + }
> > +/*
> > + * A simple entry is just a variable, the size of the entry is the
> > variable size
> > + * The variable can also be a pointer, the pointer itself is being added
> > in this
> > + * case.
> > + */
> > +#define MEMINSPECT_SIMPLE_ENTRY(sym) \
> > + MEMINSPECT_ENTRY(MEMINSPECT_ID_##sym, sym, sizeof(sym))
> > +/*
> > + * In the case when `sym` is not a variable, but a member of a struct e.g.,
> > + * and we cannot derive a name from it, a name must be provided.
> > + */
> > +#define MEMINSPECT_NAMED_ENTRY(name, sym) \
> > + MEMINSPECT_ENTRY(MEMINSPECT_ID_##name, sym, sizeof(sym))
> > +/*
> > + * Create a more complex entry, by registering an arbitrary memory starting
> > + * at sym. The size is provided as a parameter.
> > + * This is used e.g. when the symbol is a start of an unknown sized array.
> > + */
> > +#define MEMINSPECT_AREA_ENTRY(sym, sz) \
> > + MEMINSPECT_ENTRY(MEMINSPECT_ID_##sym, sym, sz)
> > +
> > +/* Iterate through .inspect_table section entries */
> > +#define for_each_meminspect_entry(__entry) \
> > + for (__entry = __inspect_table; \
> > + __entry < __inspect_table_end; \
> > + __entry++)
> > +
> > +#else
> > +#define MEMINSPECT_ENTRY(...)
> > +#define MEMINSPECT_SIMPLE_ENTRY(...)
> > +#define MEMINSPECT_NAMED_ENTRY(...)
> > +#define MEMINSPECT_AREA_ENTRY(...)
> > +#endif
> > +
> > +#ifdef CONFIG_MEMINSPECT
> > +
> > +/*
> > + * Dynamic helpers to register entries.
> > + * These do not lock the table, so use with caution.
> > + */
> > +void meminspect_register_id_pa(enum meminspect_uid id, phys_addr_t zone,
> > + size_t size, unsigned int type);
> > +void meminspect_table_lock(void);
> > +void meminspect_table_unlock(void);
> > +
> > +#define meminspect_register_pa(...) \
> > + meminspect_register_id_pa(MEMINSPECT_ID_DYNAMIC, __VA_ARGS__,
> > MEMINSPECT_TYPE_REGULAR)
> > +
> > +#define meminspect_register_id_va(id, va, size) \
> > + meminspect_register_id_pa(id, virt_to_phys(va), size,
> > MEMINSPECT_TYPE_REGULAR)
> > +
> > +#define meminspect_register_va(...) \
> > + meminspect_register_id_va(MEMINSPECT_ID_DYNAMIC, __VA_ARGS__)
> > +
> > +void meminspect_unregister_pa(phys_addr_t zone, size_t size);
> > +void meminspect_unregister_id(enum meminspect_uid id);
> > +
> > +#define meminspect_unregister_va(va, size) \
> > + meminspect_unregister_pa(virt_to_phys(va), size)
> > +
> > +void meminspect_traverse(void *priv, meminspect_iter_cb_t cb);
> > +
> > +/*
> > + * Producers, or registrators, are advised to use the locked API below
> > + */
> > +#define meminspect_lock_register_pa(...) \
> > + do { \
> > + meminspect_table_lock(); \
> > + meminspect_register_pa(__VA_ARGS__); \
> > + meminspect_table_unlock(); \
> > + } while (0)
> > +
> > +#define meminspect_lock_register_id_va(...) \
> > + do { \
> > + meminspect_table_lock(); \
> > + meminspect_register_id_va(__VA_ARGS__); \
> > + meminspect_table_unlock(); \
> > + } while (0)
> > +
> > +#define meminspect_lock_register_va(...) \
> > + do { \
> > + meminspect_table_lock(); \
> > + meminspect_register_va(__VA_ARGS__); \
> > + meminspect_table_unlock(); \
> > + } while (0)
> > +
> > +#define meminspect_lock_unregister_pa(...) \
> > + do { \
> > + meminspect_table_lock(); \
> > + meminspect_unregister_pa(__VA_ARGS__); \
> > + meminspect_table_unlock(); \
> > + } while (0)
> > +
> > +#define meminspect_lock_unregister_va(...) \
> > + do { \
> > + meminspect_table_lock(); \
> > + meminspect_unregister_va(__VA_ARGS__); \
> > + meminspect_table_unlock(); \
> > + } while (0)
> > +
> > +#define meminspect_lock_unregister_id(...) \
> > + do { \
> > + meminspect_table_lock(); \
> > + meminspect_unregister_id(__VA_ARGS__); \
> > + meminspect_table_unlock(); \
> > + } while (0)
> > +
> > +#define meminspect_lock_traverse(...) \
> > + do { \
> > + meminspect_table_lock(); \
> > + meminspect_traverse(__VA_ARGS__); \
> > + meminspect_table_unlock(); \
> > + } while (0)
> > +
> > +int meminspect_notifier_register(struct notifier_block *n);
> > +int meminspect_notifier_unregister(struct notifier_block *n);
> > +
> > +#else
> > +static inline void meminspect_register_id_pa(enum meminspect_uid id,
> > + phys_addr_t zone,
> > + size_t size, unsigned int type)
> > +{
> > +}
> > +
> > +static inline void meminspect_table_lock(void)
> > +{
> > +}
> > +
> > +static inline void meminspect_table_unlock(void)
> > +{
> > +}
> > +
> > +static inline void meminspect_unregister_pa(phys_addr_t zone, size_t size)
> > +{
> > +}
> > +
> > +static inline void meminspect_unregister_id(enum meminspect_uid id)
> > +{
> > +}
> > +
> > +static inline void meminspect_traverse(void *priv, meminspect_iter_cb_t cb)
> > +{
> > +}
> > +
> > +static inline int meminspect_notifier_register(struct notifier_block *n)
> > +{
> > + return 0;
> > +}
> > +
> > +static inline int meminspect_notifier_unregister(struct notifier_block *n)
> > +{
> > + return 0;
> > +}
> > +
> > +#define meminspect_register_pa(...) do { } while (0)
> > +#define meminspect_register_id_va(...) do { } while (0)
> > +#define meminspect_register_va(...) do { } while (0)
> > +#define meminspect_lock_register_pa(...) do { } while (0)
> > +#define meminspect_lock_register_va(...) do { } while (0)
> > +#define meminspect_lock_register_id_va(...) do { } while (0)
> > +#define meminspect_lock_traverse(...) do { } while (0)
> > +#define meminspect_lock_unregister_va(...) do { } while (0)
> > +#define meminspect_lock_unregister_pa(...) do { } while (0)
> > +#define meminspect_lock_unregister_id(...) do { } while (0)
> > +#endif
> > +
> > +#endif
> > diff --git a/init/Kconfig b/init/Kconfig
> > index fa42fb264c9c..adbe607b7b62 100644
> > --- a/init/Kconfig
> > +++ b/init/Kconfig
> > @@ -2294,6 +2294,7 @@ config TRACEPOINTS
> > source "kernel/Kconfig.kexec"
> >
> > source "kernel/liveupdate/Kconfig"
> > +source "kernel/meminspect/Kconfig"
> >
> > endmenu # General setup
> >
> > diff --git a/kernel/Makefile b/kernel/Makefile
> > index 1e1a31673577..7c74a4f94a81 100644
> > --- a/kernel/Makefile
> > +++ b/kernel/Makefile
> > @@ -52,6 +52,7 @@ obj-y += locking/
> > obj-y += power/
> > obj-y += printk/
> > obj-y += irq/
> > +obj-y += meminspect/
> > obj-y += rcu/
> > obj-y += livepatch/
> > obj-y += liveupdate/
> > diff --git a/kernel/meminspect/Kconfig b/kernel/meminspect/Kconfig
> > new file mode 100644
> > index 000000000000..18ff511ad4cf
> > --- /dev/null
> > +++ b/kernel/meminspect/Kconfig
> > @@ -0,0 +1,18 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +config MEMINSPECT
> > + bool "Allow the kernel to register memory regions for inspection
> > purpose"
> > + depends on !CRASH_DUMP || DMA_CMA
>
> Why does it depend on CONFIG_DMA_CMA?
It uses CMA for contiguous memory to store ELF metadata for all
segments, which will be the very first entry in the table. The
dma_alloc_from_contiguous() and dma_common_contiguous_remap()
APIs are used for this purpose.
>
> > + help
> > + Inspection mechanism allows registration of a specific memory
> > + area (or object) for later inspection purposes. Ranges are added
> > + into an inspection table, which can be requested and analyzed by
> > + specific drivers. Drivers would interface any hardware mechanism
> > + that will allow inspection of the data, including but not limited
> > + to: dumping for debugging, creating a coredump, analysis, or
> > + statistical information. The inspection table is created ahead
> > + of time such that it can be later used regardless of the state of
> > + the kernel (running, frozen, crashed, or any particular state).
>
> Another wall of text and it's not at all clear what this is suppose to do,
> this needs to be completely reworked.
Ack.
>
> > +
> > + Note that modules using this feature must be rebuilt if this
> > + option changes.
> > diff --git a/kernel/meminspect/Makefile b/kernel/meminspect/Makefile
> > new file mode 100644
> > index 000000000000..09fd55e6d9cf
> > --- /dev/null
> > +++ b/kernel/meminspect/Makefile
> > @@ -0,0 +1,3 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +obj-$(CONFIG_MEMINSPECT) += meminspect.o
> > diff --git a/kernel/meminspect/meminspect.c b/kernel/meminspect/meminspect.c
> > new file mode 100644
> > index 000000000000..d9d38f484f1f
> > --- /dev/null
> > +++ b/kernel/meminspect/meminspect.c
> > @@ -0,0 +1,474 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/crash_core.h>
> > +#include <linux/dma-map-ops.h>
> > +#include <linux/errno.h>
> > +#include <linux/meminspect.h>
> > +#include <linux/notifier.h>
> > +#include <linux/vmcore_info.h>
> > +
> > +static DEFINE_MUTEX(meminspect_lock);
> > +static struct inspect_entry inspect_entries[MEMINSPECT_ID_MAX];
> > +
> > +static ATOMIC_NOTIFIER_HEAD(meminspect_notifier_list);
> > +
> > +#ifdef CONFIG_CRASH_DUMP
> > +
> > +#define CORE_STR "CORE"
> > +
> > +static struct elfhdr *ehdr;
> > +static size_t elf_offset;
> > +static bool elf_hdr_ready;
> > +
> > +static void append_kcore_note(char *notes, size_t *i, const char *name,
> > + unsigned int type, const void *desc,
> > + size_t descsz)
> > +{
> > + struct elf_note *note = (struct elf_note *)¬es[*i];
> > +
> > + note->n_namesz = strlen(name) + 1;
> > + note->n_descsz = descsz;
> > + note->n_type = type;
> > + *i += sizeof(*note);
> > + memcpy(¬es[*i], name, note->n_namesz);
> > + *i = ALIGN(*i + note->n_namesz, 4);
> > + memcpy(¬es[*i], desc, descsz);
> > + *i = ALIGN(*i + descsz, 4);
> > +}
> > +
> > +static void append_kcore_note_nodesc(char *notes, size_t *i, const char
> > *name,
> > + unsigned int type, size_t descsz)
> > +{
> > + struct elf_note *note = (struct elf_note *)¬es[*i];
> > +
> > + note->n_namesz = strlen(name) + 1;
> > + note->n_descsz = descsz;
> > + note->n_type = type;
> > + *i += sizeof(*note);
> > + memcpy(¬es[*i], name, note->n_namesz);
> > + *i = ALIGN(*i + note->n_namesz, 4);
> > +}
> > +
> > +static struct elf_phdr *elf_phdr_entry_addr(struct elfhdr *ehdr, int idx)
> > +{
> > + struct elf_phdr *ephdr = (struct elf_phdr *)((size_t)ehdr +
> > ehdr->e_phoff);
> > +
> > + return &ephdr[idx];
> > +}
> > +
> > +static int clear_elfheader(const struct inspect_entry *e)
> > +{
> > + struct elf_phdr *phdr;
> > + struct elf_phdr *tmp_phdr;
> > + unsigned int phidx;
> > + unsigned int i;
> > +
> > + for (i = 0; i < ehdr->e_phnum; i++) {
> > + phdr = elf_phdr_entry_addr(ehdr, i);
> > + if (phdr->p_paddr == e->pa &&
> > + phdr->p_memsz == ALIGN(e->size, 4))
> > + break;
> > + }
> > +
> > + if (i == ehdr->e_phnum) {
> > + pr_debug("Cannot find program header entry in elf\n");
> > + return -EINVAL;
> > + }
> > +
> > + phidx = i;
> > +
> > + /* Clear program header */
> > + tmp_phdr = elf_phdr_entry_addr(ehdr, phidx);
> > + for (i = phidx; i < ehdr->e_phnum - 1; i++) {
> > + tmp_phdr = elf_phdr_entry_addr(ehdr, i + 1);
> > + phdr = elf_phdr_entry_addr(ehdr, i);
> > + memcpy(phdr, tmp_phdr, sizeof(*phdr));
> > + phdr->p_offset = phdr->p_offset - ALIGN(e->size, 4);
> > + }
> > + memset(tmp_phdr, 0, sizeof(*tmp_phdr));
> > + ehdr->e_phnum--;
> > +
> > + elf_offset -= ALIGN(e->size, 4);
> > +
> > + return 0;
> > +}
> > +
> > +static void update_elfheader(const struct inspect_entry *e)
> > +{
> > + struct elf_phdr *phdr;
> > +
> > + phdr = elf_phdr_entry_addr(ehdr, ehdr->e_phnum++);
> > +
> > + phdr->p_type = PT_LOAD;
> > + phdr->p_offset = elf_offset;
> > + phdr->p_vaddr = (elf_addr_t)e->va;
> > + if (e->pa)
> > + phdr->p_paddr = (elf_addr_t)e->pa;
> > + else
> > + phdr->p_paddr = (elf_addr_t)virt_to_phys(e->va);
> > +
> > + phdr->p_filesz = ALIGN(e->size, 4);
> > + phdr->p_memsz = ALIGN(e->size, 4);
> > + phdr->p_flags = PF_R | PF_W;
> > + elf_offset += ALIGN(e->size, 4);
> > +}
> > +
> > +/*
> > + * This function prepares the elf header for the coredump image.
> > + * Initially there is a single program header for the elf NOTE.
> > + * The note contains the usual core dump information, and the vmcoreinfo.
> > + */
> > +static int init_elfheader(void)
> > +{
> > + struct elf_phdr *phdr;
> > + void *notes;
> > + unsigned int elfh_size, buf_sz;
> > + unsigned int phdr_off;
> > + size_t note_len, i = 0;
> > + struct page *p;
> > +
> > + struct elf_prstatus prstatus = {};
> > + struct elf_prpsinfo prpsinfo = {
> > + .pr_sname = 'R',
> > + .pr_fname = "vmlinux",
> > + };
> > +
> > + /*
> > + * Header buffer contains:
> > + * ELF header, Note entry with PR status, PR ps info, and vmcoreinfo.
> > + * Also, MEMINSPECT_ID_MAX program headers.
> > + */
> > + elfh_size = sizeof(*ehdr);
> > + elfh_size += sizeof(struct elf_prstatus);
> > + elfh_size += sizeof(struct elf_prpsinfo);
> > + elfh_size += sizeof(VMCOREINFO_NOTE_NAME);
> > + elfh_size += ALIGN(vmcoreinfo_size, 4);
> > + elfh_size += (sizeof(*phdr)) * (MEMINSPECT_ID_MAX);
> > +
> > + elfh_size = ALIGN(elfh_size, 4);
> > +
> > + /* Length of the note is made of :
> > + * 3 elf notes structs (prstatus, prpsinfo, vmcoreinfo)
> > + * 3 notes names (2 core strings, 1 vmcoreinfo name)
> > + * sizeof each note
> > + */
> > + note_len = (3 * sizeof(struct elf_note) +
> > + 2 * ALIGN(sizeof(CORE_STR), 4) +
> > + VMCOREINFO_NOTE_NAME_BYTES +
> > + ALIGN(sizeof(struct elf_prstatus), 4) +
> > + ALIGN(sizeof(struct elf_prpsinfo), 4) +
> > + ALIGN(vmcoreinfo_size, 4));
> > +
> > + buf_sz = elfh_size + note_len - ALIGN(vmcoreinfo_size, 4);
> > +
> > + /* Never freed */
> > + p = dma_alloc_from_contiguous(NULL, buf_sz >> PAGE_SHIFT,
> > + get_order(buf_sz), true);
> > + if (!p)
> > + return -ENOMEM;
> > +
> > + ehdr = dma_common_contiguous_remap(p, buf_sz,
> > +
> > pgprot_decrypted(pgprot_dmacoherent(PAGE_KERNEL)),
> > + __builtin_return_address(0));
> > + if (!ehdr) {
> > + dma_release_from_contiguous(NULL, p, buf_sz >> PAGE_SHIFT);
> > + return -ENOMEM;
> > + }
> > +
> > + memset(ehdr, 0, elfh_size);
> > +
> > + /* Assign Program headers offset, it's right after the elf header. */
> > + phdr = (struct elf_phdr *)(ehdr + 1);
> > + phdr_off = sizeof(*ehdr);
> > +
> > + memcpy(ehdr->e_ident, ELFMAG, SELFMAG);
> > + ehdr->e_ident[EI_CLASS] = ELF_CLASS;
> > + ehdr->e_ident[EI_DATA] = ELF_DATA;
> > + ehdr->e_ident[EI_VERSION] = EV_CURRENT;
> > + ehdr->e_ident[EI_OSABI] = ELF_OSABI;
> > + ehdr->e_type = ET_CORE;
> > + ehdr->e_machine = ELF_ARCH;
> > + ehdr->e_version = EV_CURRENT;
> > + ehdr->e_ehsize = sizeof(*ehdr);
> > + ehdr->e_phentsize = sizeof(*phdr);
> > +
> > + elf_offset = elfh_size;
> > +
> > + notes = (void *)(((char *)ehdr) + elf_offset);
> > +
> > + /* we have a single program header now */
> > + ehdr->e_phnum = 1;
> > +
> > + phdr->p_type = PT_NOTE;
> > + phdr->p_offset = elf_offset;
> > + phdr->p_filesz = note_len;
> > +
> > + /* advance elf offset */
> > + elf_offset += note_len;
> > +
> > + strscpy(prpsinfo.pr_psargs, saved_command_line,
> > + sizeof(prpsinfo.pr_psargs));
> > +
> > + append_kcore_note(notes, &i, CORE_STR, NT_PRSTATUS, &prstatus,
> > + sizeof(prstatus));
> > + append_kcore_note(notes, &i, CORE_STR, NT_PRPSINFO, &prpsinfo,
> > + sizeof(prpsinfo));
> > + append_kcore_note_nodesc(notes, &i, VMCOREINFO_NOTE_NAME, 0,
> > + ALIGN(vmcoreinfo_size, 4));
> > +
> > + ehdr->e_phoff = phdr_off;
> > +
> > + /* This is the first coredump region, the ELF header */
> > + meminspect_register_id_pa(MEMINSPECT_ID_ELF, page_to_phys(p),
> > + buf_sz, MEMINSPECT_TYPE_REGULAR);
> > +
> > + /*
> > + * The second region is the vmcoreinfo, which goes right after.
> > + * It's being registered through vmcoreinfo.
> > + */
> > +
> > + return 0;
> > +}
> > +#endif
>
> Put /* <condition> */ on the end of big #endif blocks please.
Ack.
>
> > +
> > +/**
> > + * meminspect_unregister_id() - Unregister region from inspection table.
> > + * @id: region's id in the table
> > + *
> > + * Return: None
> > + */
> > +void meminspect_unregister_id(enum meminspect_uid id)
> > +{
> > + struct inspect_entry *e;
> > +
> > + WARN_ON(!mutex_is_locked(&meminspect_lock));
> > +
> > + e = &inspect_entries[id];
> > + if (!e->id)
> > + return;
> > +
> > + atomic_notifier_call_chain(&meminspect_notifier_list,
> > + MEMINSPECT_NOTIFIER_REMOVE, e);
> > +#ifdef CONFIG_CRASH_DUMP
> > + if (elf_hdr_ready)
> > + clear_elfheader(e);
> > +#endif
> > + memset(e, 0, sizeof(*e));
> > +}
> > +EXPORT_SYMBOL_GPL(meminspect_unregister_id);
> > +
> > +/**
> > + * meminspect_unregister_pa() - Unregister region from inspection table.
> > + * @pa: Physical address of the memory region to remove
> > + * @size: Size of the memory region to remove
> > + *
> > + * Return: None
> > + */
> > +void meminspect_unregister_pa(phys_addr_t pa, size_t size)
> > +{
> > + struct inspect_entry *e;
> > + enum meminspect_uid i;
> > +
> > + WARN_ON(!mutex_is_locked(&meminspect_lock));
> > +
> > + for (i = MEMINSPECT_ID_ELF; i < MEMINSPECT_ID_MAX; i++) {
> > + e = &inspect_entries[i];
> > + if (e->pa != pa)
> > + continue;
> > + if (e->size != size)
> > + continue;
> > + meminspect_unregister_id(e->id);
> > + return;
> > + }
> > +}
> > +EXPORT_SYMBOL_GPL(meminspect_unregister_pa);
> > +
> > +/**
> > + * meminspect_register_id_pa() - Register region into inspection table
> > + * with given ID and physical address.
> > + * @req_id: Requested unique meminspect_uid that identifies the region
> > + * This can be MEMINSPECT_ID_DYNAMIC, in which case the function will
> > + * find an unused ID and register with it.
> > + * @pa: physical address of the memory region
> > + * @size: region size
> > + * @type: region type
> > + *
> > + * Return: None
> > + */
> > +void meminspect_register_id_pa(enum meminspect_uid req_id, phys_addr_t pa,
> > + size_t size, unsigned int type)
> > +{
> > + struct inspect_entry *e;
> > + enum meminspect_uid uid = req_id;
> > +
> > + WARN_ON(!mutex_is_locked(&meminspect_lock));
> > +
> > + if (uid <= MEMINSPECT_ID_NONE || uid >= MEMINSPECT_ID_MAX)
> > + return;
> > +
> > + if (uid == MEMINSPECT_ID_DYNAMIC)
> > + while (uid < MEMINSPECT_ID_MAX) {
> > + if (!inspect_entries[uid].id)
> > + break;
> > + uid++;
> > + }
> > +
> > + if (uid == MEMINSPECT_ID_MAX)
> > + return;
> > +
> > + e = &inspect_entries[uid];
> > +
> > + if (e->id)
> > + meminspect_unregister_id(e->id);
> > +
> > + e->pa = pa;
> > + e->va = phys_to_virt(pa);
> > + e->size = size;
> > + e->id = uid;
> > + e->type = type;
> > +#ifdef CONFIG_CRASH_DUMP
> > + if (elf_hdr_ready)
> > + update_elfheader(e);
> > +#endif
> > + atomic_notifier_call_chain(&meminspect_notifier_list,
> > + MEMINSPECT_NOTIFIER_ADD, e);
> > +}
> > +EXPORT_SYMBOL_GPL(meminspect_register_id_pa);
> > +
> > +/**
> > + * meminspect_table_lock() - Lock the mutex on the inspection table
> > + *
> > + * Return: None
> > + */
> > +void meminspect_table_lock(void)
> > +{
> > + mutex_lock(&meminspect_lock);
> > +}
> > +EXPORT_SYMBOL_GPL(meminspect_table_lock);
> > +
> > +/**
> > + * meminspect_table_unlock() - Unlock the mutex on the inspection table
> > + *
> > + * Return: None
> > + */
> > +void meminspect_table_unlock(void)
> > +{
> > + mutex_unlock(&meminspect_lock);
> > +}
> > +EXPORT_SYMBOL_GPL(meminspect_table_unlock);
> > +
> > +/**
> > + * meminspect_traverse() - Traverse the meminspect table and call the
> > + * callback function for each valid entry.
> > + * @priv: private data to be passed to the callback
> > + * @cb: meminspect iterator callback that should be called for each entry
> > + *
> > + * Return: None
> > + */
> > +void meminspect_traverse(void *priv, meminspect_iter_cb_t cb)
> > +{
> > + const struct inspect_entry *e;
> > + int i;
> > +
> > + WARN_ON(!mutex_is_locked(&meminspect_lock));
> > +
> > + for (i = MEMINSPECT_ID_ELF; i < MEMINSPECT_ID_MAX; i++) {
> > + e = &inspect_entries[i];
> > + if (e->id)
> > + cb(priv, e);
> > + }
> > +}
> > +EXPORT_SYMBOL_GPL(meminspect_traverse);
> > +
> > +/**
> > + * meminspect_notifier_register() - Register a notifier to meminspect table
> > + * @n: notifier block to register. This will be called whenever an entry
> > + * is being added or removed.
> > + *
> > + * Return: errno
> > + */
> > +int meminspect_notifier_register(struct notifier_block *n)
> > +{
> > + return atomic_notifier_chain_register(&meminspect_notifier_list, n);
> > +}
> > +EXPORT_SYMBOL_GPL(meminspect_notifier_register);
> > +
> > +/**
> > + * meminspect_notifier_unregister() - Unregister a previously registered
> > + * notifier from meminspect table.
> > + * @n: notifier block to unregister.
> > + *
> > + * Return: errno
> > + */
> > +int meminspect_notifier_unregister(struct notifier_block *n)
> > +{
> > + return atomic_notifier_chain_unregister(&meminspect_notifier_list, n);
> > +}
> > +EXPORT_SYMBOL_GPL(meminspect_notifier_unregister);
> > +
> > +#ifdef CONFIG_CRASH_DUMP
> > +static int __init meminspect_prepare_crashdump(void)
> > +{
> > + const struct inspect_entry *e;
> > + int ret;
> > + enum meminspect_uid i;
> > +
> > + ret = init_elfheader();
> > +
> > + if (ret < 0)
> > + return ret;
> > +
> > + /*
> > + * Some regions may have been registered very early.
> > + * Update the elf header for all existing regions,
> > + * except for MEMINSPECT_ID_ELF and MEMINSPECT_ID_VMCOREINFO,
> > + * those are included in the ELF header upon its creation.
> > + */
> > + for (i = MEMINSPECT_ID_VMCOREINFO + 1; i < MEMINSPECT_ID_MAX; i++) {
> > + e = &inspect_entries[i];
> > + if (e->id)
> > + update_elfheader(e);
> > + }
> > +
> > + elf_hdr_ready = true;
> > +
> > + return 0;
> > +}
> > +#endif
> > +
> > +static int __init meminspect_prepare_table(void)
> > +{
> > + const struct inspect_entry *e;
> > + enum meminspect_uid i;
> > + int ret;
> > +
> > + meminspect_table_lock();
> > + /*
> > + * First, copy all entries from the compiler built table
> > + * In case some entries are registered multiple times,
> > + * the last chronological entry will be stored.
> > + * Previously registered entries will be dropped.
> > + */
> > + for_each_meminspect_entry(e) {
> > + inspect_entries[e->id] = *e;
> > + if (!inspect_entries[e->id].pa && inspect_entries[e->id].va)
> > + inspect_entries[e->id].pa =
> > virt_to_phys(inspect_entries[e->id].va);
> > + }
> > +#ifdef CONFIG_CRASH_DUMP
> > + ret = meminspect_prepare_crashdump();
> > + if (ret)
> > + pr_warn("meminspect: failed to prepare crashdump ELF header:
> > %d\n", ret);
> > +#endif
> > + /* if we have early notifiers registered, call them now */
> > + for (i = MEMINSPECT_ID_ELF; i < MEMINSPECT_ID_MAX; i++)
> > + if (inspect_entries[i].id)
> > + atomic_notifier_call_chain(&meminspect_notifier_list,
> > + MEMINSPECT_NOTIFIER_ADD,
> > + &inspect_entries[i]);
> > + meminspect_table_unlock();
> > +
> > + pr_debug("Memory inspection table initialized\n");
> > +
> > + return 0;
> > +}
> > +late_initcall(meminspect_prepare_table);
> >
> > --
> > 2.53.0
> >
>
> Thanks, Lorenzo
--
-Mukesh Ojha
