On Wed, Jul 08, 2026 at 09:18:36AM +0100, Lorenzo Stoakes wrote: > Having had a look through, thoughts: > > I'm very concerned that this (whether intended or not) is a work around for > drivers to get access to symbols that are not exported. > > In many cases you are adding meminspect instrumentation for stuff that's > already > exported (fine). In some cases it's stuff that's only GPL-exported (not fine), > and in some cases you are exporting totally unexpected symbols (very very very > not fine). > > A driver that hooks on to meminspect then gets the PA and VA for these and can > access these symbols as far as I can tell. > > On that basis I really question the whole approach. It feels like you want to > have essentially richer symbol information here, so the approach should > perhaps > be built on the existing mechanism for symbols rather than arbitrarily + > invasively stuck into core kernel logic?
Lorenzo, thank you for the thorough review. You raise a number of valid points and I do not disagree. Quoting the lines from our LPC 2023 presentation of Minidump which is exact problem we are trying to address. https://lpc.events/event/17/contributions/1468/ "Devices in engineering mode frequently provide a mechanism for generating full system DDR dump. However, for end user devices, it is not feasible to capture the entire DDR content and transfer them electronically. Now a days, typical size of DDR on a premium tier phones is 12 GB and growing, so the problem is going to be worse." The reason for using a physical address is to get the content directly from where it is present, and not rely on a pstore kind of mechanism where data gets streamed or written to a RAM backend, which may not even get a chance to run for non-kernel crashes, or in some hung state of the device where a watchdog bite triggers crash dump collection. So, if we tell the firmware upfront where some good debug regions are located — like printk data and its metadata, or even ftrace and its content — we could retrieve dmesg and ftrace even for non-kernel crashes or hung states. This could be more helpful for production devices where existing mechanisms for various reasons are not feasible. > > I am also concerned that you're not accounting for memory lifecycle or things > like memory hotplug - you seem to just store PAs/VAs with assumption that the > ranges remain eternally mapped at the same ranges for both. > > This is a concern because you also track dynamic ranges, and drivers might > register stuff they allocate this way, and it all seems like an easy way to > get > UAF's. I agree, it may not be considering memory hotplug scenario. > > I may have missed something here and happy to be corrected, though. > > In general the series is far from being in upstreamable shape - you send a > giant > 1st patch which lumps together a whole bunch of stuff, then you send dozens of > 2-5 line patches, and finally include some qualcomm driver specific stuff for > good measure. > > The commit messages are generally awful and give little to no information, the > cover letter is deeply uncompelling and doesn't make the case for the feature > at > all. I am very sorry for this. I can really improve the commit messages and the cover letter. It should discuss the problem first, why the existing solution does not address it, and why this approach was chosen, but until now nobody has complained about it. This series has a very long history from minidump->kmemdump->meminspect and it may have lost the core detail along the way, but we can really improve this. > > You make zero comments about key concerns with regards to stability, > robustness, > security. There's no testing logic anywhere, and you even include a plea for > testing in the cover letter. We did not see the driver getting PA/VA as a concern, but I see your point. The plea for testing was only for the debug_kinfo driver, which we don't usually own in its downstream. Eugene, in discussion with others, found there could be another potential users of meminspect, and hence he converted it to use meminspect and hence, the plea for testing only the debug_kinfo driver but Qualcomm minidump driver tested with meminspect. > This is very much RFC stuff and I continue to not understand why you've taken > the RFC label off here. You are right to question it given the volume of your open concerns; removing the RFC label was premature. Eugene has even presented this to the MM maintainer at some meeting, which I did not attend, where he got a nod from David that this should be taken out of RFC. > > And it all very much smacks of trying to solve an internal problem you've > encountered downstream by changing the core of the kernel, rather than you > sending a generally useful feature. It may look like a downstream problem, but it is a problem to debug an end user's phone, and it may need more but minimal data for debugging an end user phone crash. Hence, we needed something in the core which could be used by others as well. > > You need to address all of this and re-RFC the series. It instinctively feels > like trying to solve the problem in the wrong place, but in any case it would > be > better to start with a SMALLER version of this first then look to extend it > only > if the community supports it. Surely, it should be better to start with smaller but useful information which could be capturing dmesg in any kind of crash, and for that I need at least the PA/VA for printk data and its metadata, if it is allowed. Hope this gives an idea of what we are solving. -Mukesh > > Thanks, Lorenzo > > On Wed, Jul 08, 2026 at 08:11:52AM +0100, Lorenzo Stoakes wrote: > > +cc Jason as maybe he has thoughts on this... > > > > Overall - for this to be sensibly mainlined, we need to see a. a COMPELLING > > case > > made for this feature, b. some buy-in from others, and c. have confidence > > this > > will be properly maintained. > > > > I don't feel you've demonstrated any of these. > > > > And looking through the history of this (you've made it really hard with > > your > > changelog): > > > > v2 - > > https://lore.kernel.org/lkml/[email protected]/ > > v1 - > > https://lore.kernel.org/lkml/[email protected]/ > > RFC v3 - (I literally can't find it...?) > > RFC v2 - > > https://lore.kernel.org/all/[email protected]/ > > RFC v1 - > > https://lore.kernel.org/lkml/[email protected]/ > > > > I'm struggling to understand why on earth it came out of RFC? > > > > You really shouldn't drop the RFC tag unless you've got CLEAR signal that > > the > > community wants the feature and that there's buy-in. > > > > Instead you've got some specific technical feedback, some trivia, etc. which > > warrants RFC respins, but nothing to suggest buy-in. > > > > So please attach the RFC tag to any future respins. And I suggest you make > > the > > case for this a LOT clearer and seek to engage with others to confirm that > > there > > is anybody else who wants this. > > > > On Wed, Jul 08, 2026 at 11:01:39AM +0530, Mukesh Ojha wrote: > > > First of all, I want to thank Eugene for his excellent work on this > > > series. What began as the Qualcomm Minidump driver from me has now > > > evolved into meminspect. He also presented meminspect at Linux > > > Plumbers 2025. > > > > > > Video of the recording is available here for anyone interested: > > > https://www.youtube.com/watch?v=aDZv4-kOLSc > > > > It's iffy putting links like this in a commit message that'll end up as a > > commit > > (in mm we put the cover letter in). Links can die. Companies can change > > their > > minds about videos being available. > > > > Maybe better as something explicitly after the commit message. > > > > > > > > Introduction: Memory inspection mechanism > > > > > > meminspect is a mechanism which allows the kernel to mark specific > > > memory areas for memory dumping or specific inspection, statistics, > > > usage. Once regions are marked, meminspect keeps an internal list with > > > the regions in a dedicated table. Further, these regions can be > > > accessed using specific API by any interested driver. Regions being > > > marked beforehand, when the system is up and running, there is no need > > > nor dependency on a panic handler, or a working kernel that can dump the > > > debug information. meminspect can be primarily used for debugging. The > > > approach is feasible to work when pstore, kdump, or another mechanism do > > > not. Pstore relies on persistent storage, a dedicated RAM area or > > > flash, which has the disadvantage of having the memory reserved all the > > > time, or another specific non volatile memory. Some devices cannot keep > > > the RAM contents on reboot so ramoops does not work. Some devices do not > > > allow kexec to run another kernel to debug the crashed one. For such > > > devices, that have another mechanism to help debugging, like firmware, > > > meminspect is a viable solution. > > > > Please use paragraphs, this is an unreadable wall of text and it just seems > > to be noise. > > > > Why would we want to mark specific areas a special way? What are the > > trade-offs, what does that actually do? How do you interact with it? Why > > can't other approaches be used? > > > > You should make it CLEAR what exactly the benefits are and why this is > > better than alternatives. > > > > > > > > meminspect can create a core image, similar with /proc/vmcore, with only > > > the registered regions included. This can be loaded into crash tool/gdb > > > and analyzed. This happens if CRASH_DUMP=y. To have this working, > > > > What is CRASH_DUMP? Where is it defined? You mean CONFIG_CRASH_DUMP? > > > > > specific information from the kernel is registered, and this is done at > > > meminspect init time, no need for the meminspect users to do anything. > > > > Again this just doesn't really mean anything to me? > > > > > > > > This version of the meminspect patch series includes two drivers that > > > make use of it: one is the Qualcomm Minidump, and the other one is the > > > Debug Kinfo backend for Android devices, reworked from this source here: > > > https://android.googlesource.com/kernel/common/+/refs/heads/android-mainline/drivers/android/debug_kinfo.c > > > written originally by Jone Chou <[email protected]> > > > > This feels rather like you've taken a specific use case you have, and > > worked backwards from it to changes to core, honestly. > > > > > > > > Tested on Qualcomm SA8775P (Lemans EVK) on next-20260707 with the > > > minidump backend: kernel crash dump regions correctly captured > > > via firmware. > > > > What is a minidump backend? There's no detail about the testing here > > either. Does anybody outside of Qualcomm care about this? > > > > > > > > This series introduces a new cross-subsystem framework and, to > > > avoid merge ordering issues, I suggest the entire series be taken > > > via the mm tree. Subsystem maintainers for touched files have > > > been CC'd; I request an Acked-by from them if the respective > > > patches look fine from their side. > > > > Don't put stuff about mechanics of merge in the middle of a cover letter > > that will go into the git log, put this and the stuff about the video in a > > block of text below the main text, you can tag it 'REVIEWERS NOTES' or > > similar. > > > > Not only A-b for touched files, but as this is ostensibly an mm feature > > you'll need tags from mm maintainers and acceptance of the feature overall > > for it to be accepted. > > > > > > > > Alternatively, the qcom backend patches (22-24) can be deferred > > > to Bjorn's qcom tree in a follow-on series once the core > > > meminspect framework lands. > > > > I think that would be better yes, you shouldn't be adding new driver > > changes in a core series like this, only touching stuff you have to touch > > as a result of the core change. > > > > Anything else should be saved for later series. You can express ordering > > requirements in the cover letter... > > > > > > > > *** How to use meminspect with minidump backend on Qualcomm platform > > > guide *** > > > > Odd to put a specific qualcomm TODO in a cover letter for a mainline > > feature... > > > > Again this makes it feels like this is something you want for your driver, > > and the core changes are just a means of getting there, which is not an > > acceptable justification for core changes. > > > > You have to rework this entire cover letter to sell it as a GENERAL > > feature. > > > > I'm really not convinced we want to give drivers (which are great at doing > > exactly the wrong thing with mm APIs) the ability to do invasive things in > > mm. > > > > > > > > Prerequisites: > > > Crash tool compiled with target=ARM64. > > > > > > Target kernel must be built with: CONFIG_DEBUG_INFO_REDUCED=n; > > > this will have vmlinux include all the debugging information > > > needed for the crash tool and CONFIG_MEMINSPECT, > > > CONFIG_CRASH_DUMP, and the driver CONFIG_QCOM_MINIDUMP. > > > Kernel arguments: Kernel firmware must be set to mode 'mini' by kernel > > > module parameter like this : qcom_scm.download_mode=mini > > > > > > After the kernel boots and the minidump module is loaded, > > > everything is ready for a possible crash. Upon triggering a > > > forced kernel crash, the target board will wait in download mode, > > > where QDL (https://github.com/linux-msm/qdl) running on the host > > > connected to the target (here, Lemans EVK) can be used to collect > > > minidump.elf from the target device onto the host. > > > > > > Currently, --minimal mode is being used to get dmesg log from > > > crashed device. > > > > > > Without --minimal mode, the crash tool needs to be patched to > > > not crash on missing symbols and needs to be tuned, which I am > > > currently working on in parallel. > > > > > > crash> log > > > [ 0.000000] Booting Linux on physical CPU 0x0000000000 [0x514f0014] > > > [ 0.000000] Linux version 7.0.0-rc3-next-20260309-00028-g528b3c656121 > > > (@21e3bca4168f) (aarch64-linux-gnu-gcc (Ubuntu 13.3.0-6ubuntu2~24.04) > > > 13.3.0, GNU ld (GNU Binutils for Ubuntu) 2.42) #5 SMP PREEMPT Tue Mar 10 > > > 18:18:41 UTC 2026 > > > > > > *** Debug Kinfo backend driver *** > > > I need help with the testing of this driver, Anyone who actually wants > > > > Err you're putting forward a non-RFC series to mainline and also saying > > essentially it's untested at least in one respect? > > > > This is signal that this should _remain_ RFC. > > > > > to test this, feel free to reply to the patch. we have also written a > > > simple DT binding for the driver. > > > > > > Thanks in advance for the review, and apologies if I missed addressing > > > any comment. > > > > > > -Mukesh > > > > > > Changes in v3: > > > https://lore.kernel.org/lkml/[email protected]/ > > > > And again, ->v2 but you label it v3...! > > > > > - Addressed most of doc. comments and binding and 0-day kernel > > > test robot warning. > > > - Add timekeeping: Register tk_data into meminspect > > > - fixed some of the bugs identified during testing. > > > - Fix missing return before dev_err_probe() in qcom_md_probe(); all > > > three error paths (SMEM lookup, table validation, table init) now > > > correctly abort probe on failure > > > - Fix meminspect_traverse() kernel-doc: @priv description corrected > > > - Fix meminspect.rst: replace stale MEMINSPECT_ITERATOR_CB type name > > > with the actual meminspect_iter_cb_t typedef > > > - Add kernel-doc for meminspect_iter_cb_t typedef > > > - Lot of code formating and style change as per coding standard. > > > > > > Changes in v2: > > > https://lore.kernel.org/lkml/[email protected]/ > > > > The link is to v1... what a mess! > > > > > - Fixed doc warnings > > > - Fixed kernel-test robot warnings. > > > - Took Mike suggestion to remove mark inspect flag for dynamic memory. > > > - Added R-b for printk patch. > > > - Modified some commit messages for clarity. > > > - corrected binding change for debug-kinfo as per Rob suggestion. > > > > > > Changelog for meminspect v1: > > > > You don't include a link to RFC v3 at all? > > > > > - rename to meminspect > > > - start on top of v2 actually, with the section and all. > > > - remove the backend thing, change the API to access the table > > > - move everything to kernel/ > > > - add dependency to CRASH_DUMP instead of a separate knob > > > - move the minidump driver to soc/qcom > > > - integrate the meminspect better into memblock by using a new memblock > > > flag > > > - minor fixes : use dev_err_probe everywhere, rearrange variable > > > declarations, > > > remove some useless code, etc. > > > > > > Changelog for RFC v3: > > > - V2 available here : > > > https://lore.kernel.org/all/[email protected]/ > > > > Same comment as below. Put links for changelogs with the version you're > > changing > > it's a total nightmare trying to follow this. > > > > And I literally cannot find RFC v3 on list, did you mislabel it? > > > > > - Removed the .section as requested by David Hildenbrand. > > > - Moved all kmemdump registration(when possible) to vmcoreinfo. > > > - Because of this, some of the variables that I was registering had to be > > > non-static > > > so I had to modify this as per David Hildenbrand suggestion. > > > - Fixed minor things in the Kinfo driver: one field was broken, fixed some > > > compiler warnings, fixed the copyright and remove some useless includes. > > > - Moved the whole kmemdump from drivers/debug into mm/ and Kconfigs into > > > mm/Kconfig.debug > > > and it's now available in kernel hacking, as per Randy Dunlap review > > > - Reworked some of the Documentation as per review from Jon Corbet > > > > > > Changelog for RFC v2: > > > - V1 available here: > > > https://lore.kernel.org/lkml/[email protected]/ > > > > Why are you linking the previous version here, it's strange. > > > > Just put RFC v1: with the link below, same goes for other cases. > > > > > - Reworked the whole minidump implementation based on suggestions from > > > Thomas Gleixner. > > > This means new API, macros, new way to store the regions inside kmemdump > > > (ditched the IDR, moved to static allocation, have a static default > > > backend, etc) > > > - Reworked qcom_minidump driver based on review from Bjorn Andersson > > > - Reworked printk log buffer registration based on review from Petr > > > Mladek. > > > > > > --- > > > Eugen Hristev (21): > > > kernel: Introduce meminspect > > > init/version: Annotate static information into meminspect > > > mm/percpu: Annotate static information into meminspect > > > cpu: Annotate static information into meminspect > > > genirq/irqdesc: Annotate static information into meminspect > > > timers: Annotate static information into meminspect > > > kernel/fork: Annotate static information into meminspect > > > mm/page_alloc: Annotate static information into meminspect > > > mm/show_mem: Annotate static information into meminspect > > > mm/swapfile: Annotate static information into meminspect > > > kernel/vmcore_info: Register dynamic information into meminspect > > > kernel/configs: Register dynamic information into meminspect > > > mm/init-mm: Annotate static information into meminspect > > > panic: Annotate static information into meminspect > > > kallsyms: Annotate static information into meminspect > > > mm/mm_init: Annotate static information into meminspect > > > sched/core: Annotate runqueues into meminspect > > > remoteproc: qcom: Move minidump data structures into its own header > > > soc: qcom: Add minidump backend driver > > > soc: qcom: smem: Add minidump platform device > > > meminspect: Add debug kinfo compatible driver > > > > > > Mukesh Ojha (5): > > > timekeeping: Register tk_data into meminspect > > > mm/numa: Register node data information into meminspect > > > mm/sparse: Register information into meminspect > > > printk: Register information into meminspect > > > dt-bindings: reserved-memory: Add Google Kinfo Pixel reserved memory > > > > > > Documentation/dev-tools/index.rst | 1 + > > > Documentation/dev-tools/meminspect.rst | 144 +++++++ > > > .../reserved-memory/google,debug-kinfo.yaml | 48 +++ > > > MAINTAINERS | 16 + > > > drivers/of/platform.c | 1 + > > > drivers/remoteproc/qcom_common.c | 55 +-- > > > drivers/soc/qcom/Kconfig | 13 + > > > drivers/soc/qcom/Makefile | 1 + > > > drivers/soc/qcom/minidump.c | 271 ++++++++++++ > > > drivers/soc/qcom/smem.c | 10 + > > > > Yeah I hate that you're adding a wholy unrelated driver feature as part of > > this. > > > > Let's not. > > > > > include/asm-generic/vmlinux.lds.h | 13 + > > > include/linux/meminspect.h | 271 ++++++++++++ > > > include/linux/soc/qcom/minidump.h | 70 +++ > > > init/Kconfig | 1 + > > > init/version-timestamp.c | 3 + > > > init/version.c | 3 + > > > kernel/Makefile | 1 + > > > kernel/configs.c | 6 + > > > kernel/cpu.c | 5 + > > > kernel/fork.c | 3 + > > > kernel/irq/irqdesc.c | 2 + > > > kernel/kallsyms.c | 9 + > > > kernel/meminspect/Kconfig | 29 ++ > > > kernel/meminspect/Makefile | 4 + > > > kernel/meminspect/kinfo.c | 257 +++++++++++ > > > kernel/meminspect/meminspect.c | 474 > > > +++++++++++++++++++++ > > > kernel/panic.c | 4 + > > > kernel/printk/printk.c | 12 + > > > kernel/sched/core.c | 2 + > > > kernel/time/timekeeping.c | 2 + > > > kernel/time/timer.c | 2 + > > > kernel/vmcore_info.c | 4 + > > > mm/init-mm.c | 12 + > > > mm/mm_init.c | 2 + > > > mm/numa.c | 2 + > > > mm/page_alloc.c | 2 + > > > mm/percpu.c | 5 + > > > mm/show_mem.c | 2 + > > > mm/sparse.c | 3 + > > > mm/swapfile.c | 2 + > > > 40 files changed, 1713 insertions(+), 54 deletions(-) > > > --- > > > base-commit: 5c73cd9f0819c1c44e373e3dabb68318b1de1a12 > > > change-id: 20260708-meminspect-v3-76dd8166c4f1 > > > > > > Best regards, > > > -- > > > -Mukesh Ojha > > > > > > > Thanks, Lorenzo -- -Mukesh Ojha
