* Re: [PATCH 00/16] MAINTAINERS: Include GDB scripts under their relevant subsystems
From: Jan Kiszka @ 2025-06-26 16:48 UTC (permalink / raw)
To: Liam R. Howlett, Florian Fainelli, linux-kernel, Kieran Bingham,
Michael Turquette, Stephen Boyd, Dennis Zhou, Tejun Heo,
Christoph Lameter, Greg Kroah-Hartman, Rafael J. Wysocki,
Danilo Krummrich, Petr Mladek, Steven Rostedt, John Ogness,
Sergey Senozhatsky, Ulf Hansson, Thomas Gleixner, Andrey Ryabinin,
Alexander Potapenko, Andrey Konovalov, Dmitry Vyukov,
Vincenzo Frascino, Andrew Morton, Luis Chamberlain, Petr Pavlu,
Sami Tolvanen, Daniel Gomez, Kent Overstreet, Anna-Maria Behnsen,
Frederic Weisbecker, Alexander Viro, Christian Brauner, Jan Kara,
Uladzislau Rezki, Matthew Wilcox, Kuan-Ying Lee, Ilya Leoshkevich,
Etienne Buira, Antonio Quartulli, Illia Ostapyshyn,
open list:COMMON CLK FRAMEWORK,
open list:PER-CPU MEMORY ALLOCATOR, open list:GENERIC PM DOMAINS,
open list:KASAN, open list:MAPLE TREE, open list:MODULE SUPPORT,
open list:PROC FILESYSTEM
In-Reply-To: <fynmrmsglw4liexcb37ykutf724lh7zbibilcjpysbmvgtkmes@mtjrfkve4av7>
On 26.06.25 18:17, Liam R. Howlett wrote:
> * Florian Fainelli <florian.fainelli@broadcom.com> [250625 19:13]:
>> Linux has a number of very useful GDB scripts under scripts/gdb/linux/*
>> that provide OS awareness for debuggers and allows for debugging of a
>> variety of data structures (lists, timers, radix tree, mapletree, etc.)
>> as well as subsystems (clocks, devices, classes, busses, etc.).
>>
>> These scripts are typically maintained in isolation from the subsystem
>> that they parse the data structures and symbols of, which can lead to
>> people playing catch up with fixing bugs or updating the script to work
>> with updates made to the internal APIs/objects etc. Here are some
>> recents examples:
>>
>> https://lore.kernel.org/all/20250601055027.3661480-1-tony.ambardar@gmail.com/
>> https://lore.kernel.org/all/20250619225105.320729-1-florian.fainelli@broadcom.com/
>> https://lore.kernel.org/all/20250625021020.1056930-1-florian.fainelli@broadcom.com/
>>
>> This patch series is intentionally split such that each subsystem
>> maintainer can decide whether to accept the extra
>> review/maintenance/guidance that can be offered when GDB scripts are
>> being updated or added.
>
> I don't see why you think it was okay to propose this in the way you
> have gone about it. Looking at the mailing list, you've been around for
> a while.
>
> The file you are telling me about seems to be extremely new and I needed
> to pull akpm/mm-new to discover where it came from.. because you never
> Cc'ed me on the file you are asking me to own.
>
> I'm actually apposed to the filename you used for the script you want me
> to own.
>
> I consider myself a low-volume email maintainer and I get enough useless
> emails about apparent trivial fixes that end up causing significant
> damage if they are not dealt with. So I take care not to sign up for
> more time erosion from meaningful forward progress on tasks I hope to
> have high impact. I suspect you know that, but I don't know you so I
> don't want to assume.
>
> Is there anything else you might want to share to entice me to maintain
> this file? Perhaps there's a documentation pointer that shows how
> useful it is and why I should use it?
>
> Right now, I have no idea what that file does or how to even check if
> that file works today, so I cannot sign on to maintain it.
>
> If you want to depend on APIs, this should probably be generated in a
> way that enables updates. And if that's the case, then why even have a
> file at all and just generate it when needed? Or, at least, half
> generated and finished by hand?
>
> Maybe this is the case but scripts/gdb doesn't have any documentation in
> there, there's no Documentation/scripts or Documentation/gdb either.
>
> Can you please include more details on the uses of these files? Failing
> that, perhaps you could point to any documentation?
FWIW, I once wrote
Documentation/process/debugging/gdb-kernel-debugging.rst. Hope it didn't
age too much.
Jan
--
Siemens AG, Foundational Technologies
Linux Expert Center
^ permalink raw reply
* Re: [PATCH 00/16] MAINTAINERS: Include GDB scripts under their relevant subsystems
From: Jan Kara @ 2025-06-27 7:55 UTC (permalink / raw)
To: Florian Fainelli
Cc: Liam R. Howlett, linux-kernel, Jan Kiszka, Kieran Bingham,
Michael Turquette, Stephen Boyd, Dennis Zhou, Tejun Heo,
Christoph Lameter, Greg Kroah-Hartman, Rafael J. Wysocki,
Danilo Krummrich, Petr Mladek, Steven Rostedt, John Ogness,
Sergey Senozhatsky, Ulf Hansson, Thomas Gleixner, Andrey Ryabinin,
Alexander Potapenko, Andrey Konovalov, Dmitry Vyukov,
Vincenzo Frascino, Andrew Morton, Luis Chamberlain, Petr Pavlu,
Sami Tolvanen, Daniel Gomez, Kent Overstreet, Anna-Maria Behnsen,
Frederic Weisbecker, Alexander Viro, Christian Brauner, Jan Kara,
Uladzislau Rezki, Matthew Wilcox, Kuan-Ying Lee, Ilya Leoshkevich,
Etienne Buira, Antonio Quartulli, Illia Ostapyshyn,
open list:COMMON CLK FRAMEWORK,
open list:PER-CPU MEMORY ALLOCATOR, open list:GENERIC PM DOMAINS,
open list:KASAN, open list:MAPLE TREE, open list:MODULE SUPPORT,
open list:PROC FILESYSTEM
In-Reply-To: <c66deb8f-774e-4981-accf-4f507943e08c@broadcom.com>
On Thu 26-06-25 09:39:36, Florian Fainelli wrote:
> On 6/26/25 09:17, Liam R. Howlett wrote:
> > * Florian Fainelli <florian.fainelli@broadcom.com> [250625 19:13]:
> > > Linux has a number of very useful GDB scripts under scripts/gdb/linux/*
> > > that provide OS awareness for debuggers and allows for debugging of a
> > > variety of data structures (lists, timers, radix tree, mapletree, etc.)
> > > as well as subsystems (clocks, devices, classes, busses, etc.).
> > >
> > > These scripts are typically maintained in isolation from the subsystem
> > > that they parse the data structures and symbols of, which can lead to
> > > people playing catch up with fixing bugs or updating the script to work
> > > with updates made to the internal APIs/objects etc. Here are some
> > > recents examples:
> > >
> > > https://lore.kernel.org/all/20250601055027.3661480-1-tony.ambardar@gmail.com/
> > > https://lore.kernel.org/all/20250619225105.320729-1-florian.fainelli@broadcom.com/
> > > https://lore.kernel.org/all/20250625021020.1056930-1-florian.fainelli@broadcom.com/
> > >
> > > This patch series is intentionally split such that each subsystem
> > > maintainer can decide whether to accept the extra
> > > review/maintenance/guidance that can be offered when GDB scripts are
> > > being updated or added.
> >
> > I don't see why you think it was okay to propose this in the way you
> > have gone about it. Looking at the mailing list, you've been around for
> > a while.
>
> This should probably have been posted as RFC rather than PATCH, but as I
> indicate in the cover letter this is broken down to allow maintainers like
> yourself to accept/reject
>
> >
> > The file you are telling me about seems to be extremely new and I needed
> > to pull akpm/mm-new to discover where it came from.. because you never
> > Cc'ed me on the file you are asking me to own.
>
> Yes, that file is very new indeed, and my bad for not copying you on it.
>
> I was not planning on burning an entire day worth of work to transition the
> GDB scripts dumping the interrupt tree away from a radix tree to a maple
> tree. All of which happens with the author of that conversion having
> absolutely no idea that broke anything in the tree because very few people
> know about the Python GDB scripts that Linux has. It is not pleasant to be
> playing catch when it would have take maybe an extra couple hours for
> someone intimately familiar with the maple tree to come up with a suitable
> implementation replacement for mtree_load().
>
> So having done it felt like there is a maintenance void that needs to be
> filled, hence this patch set.
I can see that it takes a lot of time to do a major update of a gdb
debugging script after some refactoring like this. OTOH mandating some gdb
scripts update is adding non-trivial amount of work to changes that are
already hard enough to do as is. And the obvious question is what is the
value? I've personally never used these gdb scripts and never felt a strong
need for something like that. People have various debugging aids (like BPF
scripts, gdb scripts, there's crash tool and drgn, and many more) lying
around. I'm personally of an opinion that it is not a responsibility of
the person doing refactoring to make life easier for them or even fixing
them and I don't think that the fact that some debug aid is under
scripts/gdb/ directory is making it more special. So at least as far as I'm
concerned (VFS, fsnotify and other filesystem related stuff) I don't plan
on requiring updates to gdb scripts from people doing changes or otherwise
actively maintain them.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH v13 2/6] rust: introduce module_param module
From: Andreas Hindborg @ 2025-06-27 7:57 UTC (permalink / raw)
To: Benno Lossin
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier, Trevor Gross, Adam Bratschi-Kaye, rust-for-linux,
linux-kernel, linux-kbuild, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, Simona Vetter, Greg KH, Fiona Behrens,
Daniel Almeida, linux-modules
In-Reply-To: <878qlh4aj1.fsf@kernel.org>
Andreas Hindborg <a.hindborg@kernel.org> writes:
> "Benno Lossin" <lossin@kernel.org> writes:
>
>> On Mon Jun 23, 2025 at 4:31 PM CEST, Andreas Hindborg wrote:
>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>
>>>> On Mon Jun 23, 2025 at 11:44 AM CEST, Andreas Hindborg wrote:
>>>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>>>
>>>>>> On Fri Jun 20, 2025 at 1:29 PM CEST, Andreas Hindborg wrote:
>>>>>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>>>>>> On Thu Jun 12, 2025 at 3:40 PM CEST, Andreas Hindborg wrote:
>>>>>>>>> +/// A wrapper for kernel parameters.
>>>>>>>>> +///
>>>>>>>>> +/// This type is instantiated by the [`module!`] macro when module parameters are
>>>>>>>>> +/// defined. You should never need to instantiate this type directly.
>>>>>>>>> +///
>>>>>>>>> +/// Note: This type is `pub` because it is used by module crates to access
>>>>>>>>> +/// parameter values.
>>>>>>>>> +#[repr(transparent)]
>>>>>>>>> +pub struct ModuleParamAccess<T> {
>>>>>>>>> + data: core::cell::UnsafeCell<T>,
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +// SAFETY: We only create shared references to the contents of this container,
>>>>>>>>> +// so if `T` is `Sync`, so is `ModuleParamAccess`.
>>>>>>>>> +unsafe impl<T: Sync> Sync for ModuleParamAccess<T> {}
>>>>>>>>> +
>>>>>>>>> +impl<T> ModuleParamAccess<T> {
>>>>>>>>> + #[doc(hidden)]
>>>>>>>>> + pub const fn new(value: T) -> Self {
>>>>>>>>> + Self {
>>>>>>>>> + data: core::cell::UnsafeCell::new(value),
>>>>>>>>> + }
>>>>>>>>> + }
>>>>>>>>> +
>>>>>>>>> + /// Get a shared reference to the parameter value.
>>>>>>>>> + // Note: When sysfs access to parameters are enabled, we have to pass in a
>>>>>>>>> + // held lock guard here.
>>>>>>>>> + pub fn get(&self) -> &T {
>>>>>>>>> + // SAFETY: As we only support read only parameters with no sysfs
>>>>>>>>> + // exposure, the kernel will not touch the parameter data after module
>>>>>>>>> + // initialization.
>>>>>>>>
>>>>>>>> This should be a type invariant. But I'm having difficulty defining one
>>>>>>>> that's actually correct: after parsing the parameter, this is written
>>>>>>>> to, but when is that actually?
>>>>>>>
>>>>>>> For built-in modules it is during kernel initialization. For loadable
>>>>>>> modules, it during module load. No code from the module will execute
>>>>>>> before parameters are set.
>>>>>>
>>>>>> Gotcha and there never ever will be custom code that is executed
>>>>>> before/during parameter setting (so code aside from code in `kernel`)?
>>>>>>
>>>>>>>> Would we eventually execute other Rust
>>>>>>>> code during that time? (for example when we allow custom parameter
>>>>>>>> parsing)
>>>>>>>
>>>>>>> I don't think we will need to synchronize because of custom parameter
>>>>>>> parsing. Parameters are initialized sequentially. It is not a problem if
>>>>>>> the custom parameter parsing code name other parameters, because they
>>>>>>> are all initialized to valid values (as they are statics).
>>>>>>
>>>>>> If you have `&'static i64`, then the value at that reference is never
>>>>>> allowed to change.
>>>>>>
>>>>>>>> This function also must never be `const` because of the following:
>>>>>>>>
>>>>>>>> module! {
>>>>>>>> // ...
>>>>>>>> params: {
>>>>>>>> my_param: i64 {
>>>>>>>> default: 0,
>>>>>>>> description: "",
>>>>>>>> },
>>>>>>>> },
>>>>>>>> }
>>>>>>>>
>>>>>>>> static BAD: &'static i64 = module_parameters::my_param.get();
>>>>>>>>
>>>>>>>> AFAIK, this static will be executed before loading module parameters and
>>>>>>>> thus it makes writing to the parameter UB.
>>>>>>>
>>>>>>> As I understand, the static will be initialized by a constant expression
>>>>>>> evaluated at compile time. I am not sure what happens when this is
>>>>>>> evaluated in const context:
>>>>>>>
>>>>>>> pub fn get(&self) -> &T {
>>>>>>> // SAFETY: As we only support read only parameters with no sysfs
>>>>>>> // exposure, the kernel will not touch the parameter data after module
>>>>>>> // initialization.
>>>>>>> unsafe { &*self.data.get() }
>>>>>>> }
>>>>>>>
>>>>>>> Why would that not be OK? I would assume the compiler builds a dependency graph
>>>>>>> when initializing statics?
>>>>>>
>>>>>> Yes it builds a dependency graph, but that is irrelevant? The problem is
>>>>>> that I can create a `'static` reference to the inner value *before* the
>>>>>> parameter is written-to (as the static is initialized before the
>>>>>> parameters).
>>>>>
>>>>> I see, I did not consider this situation. Thanks for pointing this out.
>>>>>
>>>>> Could we get around this without a lock maybe? If we change
>>>>> `ModuleParamAccess::get` to take a closure instead:
>>>>>
>>>>> /// Call `func` with a reference to the parameter value stored in `Self`.
>>>>> pub fn read(&self, func: impl FnOnce(&T)) {
>>>>> // SAFETY: As we only support read only parameters with no sysfs
>>>>> // exposure, the kernel will not touch the parameter data after module
>>>>> // initialization.
>>>>> let data = unsafe { &*self.data.get() };
>>>>>
>>>>> func(data)
>>>>> }
>>>>>
>>>>> I think this would bound the lifetime of the reference passed to the
>>>>> closure to the duration of the call, right?
>>>>
>>>> Yes that is correct. Now you can't assign the reference to a static.
>>>> However, this API is probably very clunky to use, since you always have
>>>> to create a closure etc.
>>>>
>>>> Since you mentioned in the other reply that one could spin up a thread
>>>> and do something simultaneously, I don't think this is enough. You could
>>>> have a loop spin over the new `read` function and read the value and
>>>> then the write happens.
>>>
>>> Yes you are right, we have to treat it as if it could be written at any
>>> point in time.
>>>
>>>> One way to fix this issue would be to use atomics to read the value and
>>>> to not create a reference to it. So essentially have
>>>>
>>>> pub fn read(&self) -> T {
>>>> unsafe { atomic_read_unsafe_cell(&self.data) }
>>>> }
>>>
>>> That could work.
>>>
>>>> Another way would be to use a `Once`-like type (does that exist on the C
>>>> side?) so a type that can be initialized once and then never changes.
>>>> While it doesn't have a value set, we return some default value for the
>>>> param and print a warning, when it's set, we just return the value. But
>>>> this probably also requires atomics...
>>>
>>> I think atomic bool is not that far away. Either that, or we can lock.
>>>
>>>> Is parameter accessing used that often in hot paths? Can't you just copy
>>>> the value into your `Module` struct?
>>>
>>> I don't imagine this being read in a hot path. If so, the user could
>>> make a copy.
>>
>> That's good to know, then let's try to go for something simple.
>>
>> I don't think that we can just use a `Mutex<T>`, because we don't have a
>> way to create it at const time... I guess we could have
>>
>> impl<T> Mutex<T>
>> /// # Safety
>> ///
>> /// The returned value needs to be pinned and then `init` needs
>> /// to be called before any other methods are called on this.
>> pub unsafe const fn const_new() -> Self;
>>
>> pub unsafe fn init(&self);
>> }
>>
>> But that seems like a bad idea, because where would we call the `init`
>> function? That also needs to be synchronized...
>
> Ah, that is unfortunate. The init function will not run before this, so
> we would need a `Once` or an atomic anyway to initialize the lock.
>
> I am not sure if we are allowed to sleep during this, I would have to
> check. But then we could use a spin lock.
>
> We will need the locking anyway, when we want to enable sysfs write
> access to the parameters.
>
>>
>> Maybe we can just like you said use an atomic bool?
>
> Sigh, I will have to check how far that series has come.
>
I think I am going to build some kind of `Once` feature on top of
Boqun's atomic series [1], so that we can initialize a lock in these
statics. We can't use `global_lock!`, because that depends on module
init to initialize the lock before first use.
As far as I can tell, atomics may not land in v6.17, so this series
will probably not be ready for merge until v6.18 at the earliest.
Thanks for the input, Benno!
Best regards,
Andreas Hindborg
[1] https://lore.kernel.org/all/20250618164934.19817-1-boqun.feng@gmail.com
^ permalink raw reply
* Re: [PATCH v13 2/6] rust: introduce module_param module
From: Benno Lossin @ 2025-06-27 8:23 UTC (permalink / raw)
To: Andreas Hindborg
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier, Trevor Gross, Adam Bratschi-Kaye, rust-for-linux,
linux-kernel, linux-kbuild, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, Simona Vetter, Greg KH, Fiona Behrens,
Daniel Almeida, linux-modules
In-Reply-To: <87plepzke5.fsf@kernel.org>
On Fri Jun 27, 2025 at 9:57 AM CEST, Andreas Hindborg wrote:
> Andreas Hindborg <a.hindborg@kernel.org> writes:
>> "Benno Lossin" <lossin@kernel.org> writes:
>>> That's good to know, then let's try to go for something simple.
>>>
>>> I don't think that we can just use a `Mutex<T>`, because we don't have a
>>> way to create it at const time... I guess we could have
>>>
>>> impl<T> Mutex<T>
>>> /// # Safety
>>> ///
>>> /// The returned value needs to be pinned and then `init` needs
>>> /// to be called before any other methods are called on this.
>>> pub unsafe const fn const_new() -> Self;
>>>
>>> pub unsafe fn init(&self);
>>> }
>>>
>>> But that seems like a bad idea, because where would we call the `init`
>>> function? That also needs to be synchronized...
>>
>> Ah, that is unfortunate. The init function will not run before this, so
>> we would need a `Once` or an atomic anyway to initialize the lock.
>>
>> I am not sure if we are allowed to sleep during this, I would have to
>> check. But then we could use a spin lock.
>>
>> We will need the locking anyway, when we want to enable sysfs write
>> access to the parameters.
>>
>>>
>>> Maybe we can just like you said use an atomic bool?
>>
>> Sigh, I will have to check how far that series has come.
>>
>
> I think I am going to build some kind of `Once` feature on top of
> Boqun's atomic series [1], so that we can initialize a lock in these
> statics. We can't use `global_lock!`, because that depends on module
> init to initialize the lock before first use.
Sounds good, though we probably don't want to name it `Once`. Since it
is something that will be populated in the future, but not by some
random accessor, but rather a specific populator.
So maybe:
pub struct Delayed<T> {
dummy: T,
real: Opaque<T>,
populated: Atomic<bool>, // or Atomic<Flag>
writing: Atomic<bool>, // or Atomic<Flag>
}
impl<T> Delayed<T> {
pub fn new(dummy: T) -> Self {
Self {
dummy,
real: Opaque::uninit(),
populated: Atomic::new(false),
writing: Atomic::new(false),
}
}
pub fn get(&self) -> &T {
if self.populated.load(Acquire) {
unsafe { &*self.real.get() }
} else {
// maybe print a warning here?
// or maybe let the user configure this in `new()`?
&self.dummy
}
}
pub fn populate(&self, value: T) {
if self.writing.cmpxchg(false, true, Release) {
unsafe { *self.real.get() = value };
self.populated.store(true, Release);
} else {
pr_warn!("`Delayed<{}>` written to twice!\n", core::any::type_name::<T>());
}
}
}
(no idea if the orderings are correct, I always have to think way to
much about that... especially since our atomics seem to only take one
ordering in compare_exchange?)
> As far as I can tell, atomics may not land in v6.17, so this series
> will probably not be ready for merge until v6.18 at the earliest.
Yeah, sorry about that :(
> Thanks for the input, Benno!
My pleasure!
---
Cheers,
Benno
^ permalink raw reply
* Re: [PATCH 00/16] MAINTAINERS: Include GDB scripts under their relevant subsystems
From: Florian Fainelli @ 2025-06-27 16:09 UTC (permalink / raw)
To: Jan Kara
Cc: Liam R. Howlett, linux-kernel, Jan Kiszka, Kieran Bingham,
Michael Turquette, Stephen Boyd, Dennis Zhou, Tejun Heo,
Christoph Lameter, Greg Kroah-Hartman, Rafael J. Wysocki,
Danilo Krummrich, Petr Mladek, Steven Rostedt, John Ogness,
Sergey Senozhatsky, Ulf Hansson, Thomas Gleixner, Andrey Ryabinin,
Alexander Potapenko, Andrey Konovalov, Dmitry Vyukov,
Vincenzo Frascino, Andrew Morton, Luis Chamberlain, Petr Pavlu,
Sami Tolvanen, Daniel Gomez, Kent Overstreet, Anna-Maria Behnsen,
Frederic Weisbecker, Alexander Viro, Christian Brauner,
Uladzislau Rezki, Matthew Wilcox, Kuan-Ying Lee, Ilya Leoshkevich,
Etienne Buira, Antonio Quartulli, Illia Ostapyshyn,
open list:COMMON CLK FRAMEWORK,
open list:PER-CPU MEMORY ALLOCATOR, open list:GENERIC PM DOMAINS,
open list:KASAN, open list:MAPLE TREE, open list:MODULE SUPPORT,
open list:PROC FILESYSTEM
In-Reply-To: <iup2plrwgkxlnywm3imd2ctkbqzkckn4t3ho56kq4y4ykgzvbk@cefy6hl7yu6c>
On 6/27/25 00:55, Jan Kara wrote:
> On Thu 26-06-25 09:39:36, Florian Fainelli wrote:
>> On 6/26/25 09:17, Liam R. Howlett wrote:
>>> * Florian Fainelli <florian.fainelli@broadcom.com> [250625 19:13]:
>>>> Linux has a number of very useful GDB scripts under scripts/gdb/linux/*
>>>> that provide OS awareness for debuggers and allows for debugging of a
>>>> variety of data structures (lists, timers, radix tree, mapletree, etc.)
>>>> as well as subsystems (clocks, devices, classes, busses, etc.).
>>>>
>>>> These scripts are typically maintained in isolation from the subsystem
>>>> that they parse the data structures and symbols of, which can lead to
>>>> people playing catch up with fixing bugs or updating the script to work
>>>> with updates made to the internal APIs/objects etc. Here are some
>>>> recents examples:
>>>>
>>>> https://lore.kernel.org/all/20250601055027.3661480-1-tony.ambardar@gmail.com/
>>>> https://lore.kernel.org/all/20250619225105.320729-1-florian.fainelli@broadcom.com/
>>>> https://lore.kernel.org/all/20250625021020.1056930-1-florian.fainelli@broadcom.com/
>>>>
>>>> This patch series is intentionally split such that each subsystem
>>>> maintainer can decide whether to accept the extra
>>>> review/maintenance/guidance that can be offered when GDB scripts are
>>>> being updated or added.
>>>
>>> I don't see why you think it was okay to propose this in the way you
>>> have gone about it. Looking at the mailing list, you've been around for
>>> a while.
>>
>> This should probably have been posted as RFC rather than PATCH, but as I
>> indicate in the cover letter this is broken down to allow maintainers like
>> yourself to accept/reject
>>
>>>
>>> The file you are telling me about seems to be extremely new and I needed
>>> to pull akpm/mm-new to discover where it came from.. because you never
>>> Cc'ed me on the file you are asking me to own.
>>
>> Yes, that file is very new indeed, and my bad for not copying you on it.
>>
>> I was not planning on burning an entire day worth of work to transition the
>> GDB scripts dumping the interrupt tree away from a radix tree to a maple
>> tree. All of which happens with the author of that conversion having
>> absolutely no idea that broke anything in the tree because very few people
>> know about the Python GDB scripts that Linux has. It is not pleasant to be
>> playing catch when it would have take maybe an extra couple hours for
>> someone intimately familiar with the maple tree to come up with a suitable
>> implementation replacement for mtree_load().
>>
>> So having done it felt like there is a maintenance void that needs to be
>> filled, hence this patch set.
>
> I can see that it takes a lot of time to do a major update of a gdb
> debugging script after some refactoring like this. OTOH mandating some gdb
> scripts update is adding non-trivial amount of work to changes that are
> already hard enough to do as is.
This really should have been posted as RFC, because I can see how
posting this as PATCH would be seen as coercing maintainers into taking
those GDB scripts under their umbrella.
> And the obvious question is what is the
> value? I've personally never used these gdb scripts and never felt a strong
> need for something like that. People have various debugging aids (like BPF
> scripts, gdb scripts, there's crash tool and drgn, and many more) lying
> around.
Those are valuable tools in the tool box, but GDB scripts can work when
your only debug tool accessible is JTAG for instance, I appreciate this
is typically miles away from what most of the kernel community does, but
this is quite typical and common in embedded systems. When you operate
in that environment, having a decent amount of debugger awareness of
what is being debugged is immensely valuable in saving time.
> I'm personally of an opinion that it is not a responsibility of
> the person doing refactoring to make life easier for them or even fixing
> them and I don't think that the fact that some debug aid is under
> scripts/gdb/ directory is making it more special.
That is really the question that I am trying to get answered with this
patch series. IMHO as a subsystem maintainer it is not fair to be
completely oblivious to scripts that live in the source tree, even if
you are not aware of those.
> So at least as far as I'm> concerned (VFS, fsnotify and other
filesystem related stuff) I don't plan
> on requiring updates to gdb scripts from people doing changes or otherwise
> actively maintain them.
vfs.py script is beyond trivial, the largest and most complicated IMHO
is mapletree.py which had to be recently developed to continue to
support parsing the interrupt descriptor tree in the kernel, I can
maintain that one now that I know a lot more than I ever wished I knew
about maple trees. So really the burden is not as big as it may seem but
it's fair not to be taking on more work as a maintainer, I get that.
Thanks for your feedback!
--
Florian
^ permalink raw reply
* Re: [PATCH 08/16] MAINTAINERS: Include GDB scripts under MEMORY MANAGEMENT entry
From: David Hildenbrand @ 2025-06-27 17:10 UTC (permalink / raw)
To: Florian Fainelli, linux-kernel
Cc: Jan Kiszka, Kieran Bingham, Michael Turquette, Stephen Boyd,
Dennis Zhou, Tejun Heo, Christoph Lameter, Greg Kroah-Hartman,
Rafael J. Wysocki, Danilo Krummrich, Petr Mladek, Steven Rostedt,
John Ogness, Sergey Senozhatsky, Ulf Hansson, Thomas Gleixner,
Andrey Ryabinin, Alexander Potapenko, Andrey Konovalov,
Dmitry Vyukov, Vincenzo Frascino, Liam R. Howlett, Andrew Morton,
Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Kent Overstreet, Anna-Maria Behnsen, Frederic Weisbecker,
Alexander Viro, Christian Brauner, Jan Kara, Uladzislau Rezki,
Matthew Wilcox, Kuan-Ying Lee, Ilya Leoshkevich, Etienne Buira,
Antonio Quartulli, Illia Ostapyshyn,
open list:COMMON CLK FRAMEWORK,
open list:PER-CPU MEMORY ALLOCATOR, open list:GENERIC PM DOMAINS,
open list:KASAN, open list:MAPLE TREE, open list:MODULE SUPPORT,
open list:PROC FILESYSTEM
In-Reply-To: <20250625231053.1134589-9-florian.fainelli@broadcom.com>
On 26.06.25 01:10, Florian Fainelli wrote:
> Include the GDB scripts file under scripts/gdb/linux/ that deal with
> memory mamagenement code under the MEMORY MANAGEMENT subsystem since
> they parses internal data structures that depend upon that subsystem.
>
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
> MAINTAINERS | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index cad5d613cab0..52b37196d024 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15812,6 +15812,10 @@ F: include/linux/mmu_notifier.h
> F: include/linux/pagewalk.h
> F: include/trace/events/ksm.h
> F: mm/
> +F: scripts/gdb/linux/mm.py
> +F: scripts/gdb/linux/page_owner.py
> +F: scripts/gdb/linux/pgtable.py
> +F: scripts/gdb/linux/slab.py
Probably they should go to the corresponding sub-sections. At least slab.py?
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH] gendwarfksyms: order -T symtypes output by name
From: Masahiro Yamada @ 2025-06-29 17:51 UTC (permalink / raw)
To: Giuliano Procida
Cc: Sami Tolvanen, Greg Kroah-Hartman, linux-modules, linux-kbuild,
linux-kernel
In-Reply-To: <20250625095215.4027938-1-gprocida@google.com>
On Wed, Jun 25, 2025 at 6:52 PM Giuliano Procida <gprocida@google.com> wrote:
>
> When writing symtypes information, we iterate through the entire hash
> table containing type expansions. The key order varies unpredictably
> as new entries are added, making it harder to compare symtypes between
> builds.
>
> Resolve this by sorting the type expansions by name before output.
>
> Signed-off-by: Giuliano Procida <gprocida@google.com>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
> ---
> scripts/gendwarfksyms/types.c | 29 ++++++++++++++++++++++++++---
> 1 file changed, 26 insertions(+), 3 deletions(-)
>
> [Adjusted the first line of the description. Added reviewer tags.
> Added missing CC to linux-modules.]
>
> diff --git a/scripts/gendwarfksyms/types.c b/scripts/gendwarfksyms/types.c
> index 7bd459ea6c59..51c1471e8684 100644
> --- a/scripts/gendwarfksyms/types.c
> +++ b/scripts/gendwarfksyms/types.c
> @@ -6,6 +6,8 @@
> #define _GNU_SOURCE
> #include <inttypes.h>
> #include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> #include <zlib.h>
>
> #include "gendwarfksyms.h"
> @@ -179,20 +181,41 @@ static int type_map_get(const char *name, struct type_expansion **res)
> return -1;
> }
>
> +static int cmp_expansion_name(const void *p1, const void *p2)
> +{
> + struct type_expansion *const *e1 = p1;
> + struct type_expansion *const *e2 = p2;
> +
> + return strcmp((*e1)->name, (*e2)->name);
> +}
> +
> static void type_map_write(FILE *file)
> {
> struct type_expansion *e;
> struct hlist_node *tmp;
> + struct type_expansion **es;
> + size_t count = 0;
> + size_t i = 0;
>
> if (!file)
> return;
>
> - hash_for_each_safe(type_map, e, tmp, hash) {
> - checkp(fputs(e->name, file));
> + hash_for_each_safe(type_map, e, tmp, hash)
> + ++count;
> + es = xmalloc(count * sizeof(struct type_expansion *));
Just a nit:
es = xmalloc(count * sizeof(*es));
is better?
> + hash_for_each_safe(type_map, e, tmp, hash)
> + es[i++] = e;
> +
> + qsort(es, count, sizeof(struct type_expansion *), cmp_expansion_name);
qsort(es, count, sizeof(*es), cmp_expansion_name);
> +
> + for (i = 0; i < count; ++i) {
> + checkp(fputs(es[i]->name, file));
> checkp(fputs(" ", file));
> - type_list_write(&e->expanded, file);
> + type_list_write(&es[i]->expanded, file);
> checkp(fputs("\n", file));
> }
> +
> + free(es);
> }
>
> static void type_map_free(void)
> --
> 2.50.0.714.g196bf9f422-goog
>
>
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* Re: [PATCH] gendwarfksyms: order -T symtypes output by name
From: Giuliano Procida @ 2025-06-30 10:05 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Sami Tolvanen, Greg Kroah-Hartman, linux-modules, linux-kbuild,
linux-kernel
In-Reply-To: <CAK7LNASNVh8fDErjSbcR1TiCfy=LM-j3iYSNpqAvp8OhGmsKjQ@mail.gmail.com>
Hi.
On Sun, 29 Jun 2025 at 18:51, Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Wed, Jun 25, 2025 at 6:52 PM Giuliano Procida <gprocida@google.com> wrote:
> >
> > When writing symtypes information, we iterate through the entire hash
> > table containing type expansions. The key order varies unpredictably
> > as new entries are added, making it harder to compare symtypes between
> > builds.
> >
> > Resolve this by sorting the type expansions by name before output.
> >
> > Signed-off-by: Giuliano Procida <gprocida@google.com>
> > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
> > ---
> > scripts/gendwarfksyms/types.c | 29 ++++++++++++++++++++++++++---
> > 1 file changed, 26 insertions(+), 3 deletions(-)
> >
> > [Adjusted the first line of the description. Added reviewer tags.
> > Added missing CC to linux-modules.]
> >
> > diff --git a/scripts/gendwarfksyms/types.c b/scripts/gendwarfksyms/types.c
> > index 7bd459ea6c59..51c1471e8684 100644
> > --- a/scripts/gendwarfksyms/types.c
> > +++ b/scripts/gendwarfksyms/types.c
> > @@ -6,6 +6,8 @@
> > #define _GNU_SOURCE
> > #include <inttypes.h>
> > #include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > #include <zlib.h>
> >
> > #include "gendwarfksyms.h"
> > @@ -179,20 +181,41 @@ static int type_map_get(const char *name, struct type_expansion **res)
> > return -1;
> > }
> >
> > +static int cmp_expansion_name(const void *p1, const void *p2)
> > +{
> > + struct type_expansion *const *e1 = p1;
> > + struct type_expansion *const *e2 = p2;
> > +
> > + return strcmp((*e1)->name, (*e2)->name);
> > +}
> > +
> > static void type_map_write(FILE *file)
> > {
> > struct type_expansion *e;
> > struct hlist_node *tmp;
> > + struct type_expansion **es;
> > + size_t count = 0;
> > + size_t i = 0;
> >
> > if (!file)
> > return;
> >
> > - hash_for_each_safe(type_map, e, tmp, hash) {
> > - checkp(fputs(e->name, file));
> > + hash_for_each_safe(type_map, e, tmp, hash)
> > + ++count;
> > + es = xmalloc(count * sizeof(struct type_expansion *));
>
> Just a nit:
>
> es = xmalloc(count * sizeof(*es));
>
> is better?
>
> > + hash_for_each_safe(type_map, e, tmp, hash)
> > + es[i++] = e;
> > +
> > + qsort(es, count, sizeof(struct type_expansion *), cmp_expansion_name);
>
> qsort(es, count, sizeof(*es), cmp_expansion_name);
>
That's a fair point.
However, in the gendwarfksyms code, all but one of the sizeofs uses an
explicit type name. The exception is sizeof(stats) where stats is an array.
I'll leave Sami's code as it is.
Giuliano.
>
> > +
> > + for (i = 0; i < count; ++i) {
> > + checkp(fputs(es[i]->name, file));
> > checkp(fputs(" ", file));
> > - type_list_write(&e->expanded, file);
> > + type_list_write(&es[i]->expanded, file);
> > checkp(fputs("\n", file));
> > }
> > +
> > + free(es);
> > }
> >
> > static void type_map_free(void)
> > --
> > 2.50.0.714.g196bf9f422-goog
> >
> >
>
>
> --
> Best Regards
> Masahiro Yamada
^ permalink raw reply
* Re: [PATCH v13 2/6] rust: introduce module_param module
From: Andreas Hindborg @ 2025-06-30 11:18 UTC (permalink / raw)
To: Benno Lossin
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier, Trevor Gross, Adam Bratschi-Kaye, rust-for-linux,
linux-kernel, linux-kbuild, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, Simona Vetter, Greg KH, Fiona Behrens,
Daniel Almeida, linux-modules
In-Reply-To: <DAX65TRN0TGP.25VZ9DYV86XWY@kernel.org>
"Benno Lossin" <lossin@kernel.org> writes:
> On Fri Jun 27, 2025 at 9:57 AM CEST, Andreas Hindborg wrote:
>> Andreas Hindborg <a.hindborg@kernel.org> writes:
>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>> That's good to know, then let's try to go for something simple.
>>>>
>>>> I don't think that we can just use a `Mutex<T>`, because we don't have a
>>>> way to create it at const time... I guess we could have
>>>>
>>>> impl<T> Mutex<T>
>>>> /// # Safety
>>>> ///
>>>> /// The returned value needs to be pinned and then `init` needs
>>>> /// to be called before any other methods are called on this.
>>>> pub unsafe const fn const_new() -> Self;
>>>>
>>>> pub unsafe fn init(&self);
>>>> }
>>>>
>>>> But that seems like a bad idea, because where would we call the `init`
>>>> function? That also needs to be synchronized...
>>>
>>> Ah, that is unfortunate. The init function will not run before this, so
>>> we would need a `Once` or an atomic anyway to initialize the lock.
>>>
>>> I am not sure if we are allowed to sleep during this, I would have to
>>> check. But then we could use a spin lock.
>>>
>>> We will need the locking anyway, when we want to enable sysfs write
>>> access to the parameters.
>>>
>>>>
>>>> Maybe we can just like you said use an atomic bool?
>>>
>>> Sigh, I will have to check how far that series has come.
>>>
>>
>> I think I am going to build some kind of `Once` feature on top of
>> Boqun's atomic series [1], so that we can initialize a lock in these
>> statics. We can't use `global_lock!`, because that depends on module
>> init to initialize the lock before first use.
>
> Sounds good, though we probably don't want to name it `Once`. Since it
> is something that will be populated in the future, but not by some
> random accessor, but rather a specific populator.
>
> So maybe:
>
> pub struct Delayed<T> {
> dummy: T,
> real: Opaque<T>,
> populated: Atomic<bool>, // or Atomic<Flag>
> writing: Atomic<bool>, // or Atomic<Flag>
> }
>
> impl<T> Delayed<T> {
> pub fn new(dummy: T) -> Self {
> Self {
> dummy,
> real: Opaque::uninit(),
> populated: Atomic::new(false),
> writing: Atomic::new(false),
> }
> }
>
> pub fn get(&self) -> &T {
> if self.populated.load(Acquire) {
> unsafe { &*self.real.get() }
> } else {
> // maybe print a warning here?
> // or maybe let the user configure this in `new()`?
> &self.dummy
> }
> }
>
> pub fn populate(&self, value: T) {
> if self.writing.cmpxchg(false, true, Release) {
> unsafe { *self.real.get() = value };
> self.populated.store(true, Release);
> } else {
> pr_warn!("`Delayed<{}>` written to twice!\n", core::any::type_name::<T>());
> }
> }
> }
>
> (no idea if the orderings are correct, I always have to think way to
> much about that... especially since our atomics seem to only take one
> ordering in compare_exchange?)
>
>> As far as I can tell, atomics may not land in v6.17, so this series
>> will probably not be ready for merge until v6.18 at the earliest.
>
> Yeah, sorry about that :(
Actually, perhaps we could aim at merging this code without this
synchronization? The lack of synchronization is only a problem if we
support custom parsing. This patch set does not allow custom parsing
code, so it does not suffer this issue.
Best regards,
Andreas Hindborg
^ permalink raw reply
* Re: [PATCH v13 5/6] rust: samples: add a module parameter to the rust_minimal sample
From: Danilo Krummrich @ 2025-06-30 11:30 UTC (permalink / raw)
To: Andreas Hindborg
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Benno Lossin, Nicolas Schier,
Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules
In-Reply-To: <20250612-module-params-v3-v13-5-bc219cd1a3f8@kernel.org>
(Sorry for being late on this one, just a minor nit below.)
On 6/12/25 3:40 PM, Andreas Hindborg wrote:
> struct RustMinimal {
> @@ -20,6 +26,10 @@ impl kernel::Module for RustMinimal {
> fn init(_module: &'static ThisModule) -> Result<Self> {
> pr_info!("Rust minimal sample (init)\n");
> pr_info!("Am I built-in? {}\n", !cfg!(MODULE));
> + pr_info!(
> + "test_parameter: {}\n",
> + *module_parameters::test_parameter.get()
Can we please call it something else than get(), maybe obtain(), access() or
just ref()?
> + );
>
> let mut numbers = KVec::new();
> numbers.push(72, GFP_KERNEL)?;
>
^ permalink raw reply
* Re: [PATCH v2 1/2] module: Fix memory deallocation on error path in move_module()
From: Daniel Gomez @ 2025-06-30 12:09 UTC (permalink / raw)
To: Petr Pavlu, Luis Chamberlain, Sami Tolvanen, Daniel Gomez
Cc: linux-modules, linux-kernel
In-Reply-To: <20250618122730.51324-2-petr.pavlu@suse.com>
On 18/06/2025 14.26, Petr Pavlu wrote:
> The function move_module() uses the variable t to track how many memory
> types it has allocated and consequently how many should be freed if an
> error occurs.
>
> The variable is initially set to 0 and is updated when a call to
> module_memory_alloc() fails. However, move_module() can fail for other
> reasons as well, in which case t remains set to 0 and no memory is freed.
>
> Fix the problem by initializing t to MOD_MEM_NUM_TYPES. Additionally, make
> the deallocation loop more robust by not relying on the mod_mem_type_t enum
> having a signed integer as its underlying type.
This is a follow up based on v1 discussion [1] regarding a way to reproduce
the leak.
[1]
https://lore.kernel.org/linux-modules/ae967353-71fa-4438-a84b-8f7e2815f485@kernel.org/
I tried using eBPF and moderr [2][3] to find a way to reproduce this. I run into
some issues but I applied some workarounds. If this is useful we can check at
how to solve this properly.
[2] moderr RFC:
https://lore.kernel.org/linux-modules/20250122-modules-error-injection-v1-0-910590a04fd5@samsung.com/
[3] updated moderr branch:
https://git.kernel.org/pub/scm/linux/kernel/git/da.gomez/linux.git/?h=b4%2Fmodules-error-injection
The logs below show an A/B test memory consumption comparison when a module
is loaded and leaked (A) because of the issue described in the commit log
vs when is correctly loaded/unloaded (B). Besides observing a huge leak,
kernel logs (dmesg) show the message "execmem: unable to allocate memory".
Note that we can't use kmemleak as we skip it with kmemleak_not_leak() in
module_memory_alloc(), except for is_rox memory.
Memory lost:
A test: 1220444 kB
B test: 892 kB
Thoughts?
Workflow to reproduce the issue (before applying this patchset):
1. Run moderr to return error in codetag_needs_module_section()
sudo moderr \
-modname=brd
--modfunc=module_codetag_needs_module_section \
--error=1
Note: We can also force codetag_alloc_module_section() with:
sudo moderr \
--modname=brd \
--modfunc=module_codetag_alloc_module_section \
--error=-22
2. Run a loop to probe a module with:
while true; do modprobe brd; done
3. Monitor memory before/after and check delta:
Note: this output it's a wrapper around the above loop.
[*] Dropping caches...
[*] Wait...
[*] Starting memory: 6758564 kB
[*] Loop 200s...
[*] Wait...
[*] Dropping caches...
[*] Wait...
[*] Ending memory: 5538120 kB
[*] Memory lost: 1220444 kB
--- meminfo_before.txt 2025-06-30 10:29:49.880661145 +0000
+++ meminfo_after.txt 2025-06-30 10:33:20.091501671 +0000
@@ -1,38 +1,38 @@
MemTotal: 8133036 kB
-MemFree: 6858820 kB
-MemAvailable: 6758652 kB
-Buffers: 2576 kB
-Cached: 73164 kB
+MemFree: 5620596 kB
+MemAvailable: 5538204 kB
+Buffers: 2552 kB
+Cached: 107256 kB
SwapCached: 0 kB
-Active: 25448 kB
-Inactive: 95272 kB
-Active(anon): 176 kB
-Inactive(anon): 45684 kB
-Active(file): 25272 kB
-Inactive(file): 49588 kB
+Active: 67688 kB
+Inactive: 95284 kB
+Active(anon): 184 kB
+Inactive(anon): 54028 kB
+Active(file): 67504 kB
+Inactive(file): 41256 kB
Unevictable: 0 kB
Mlocked: 0 kB
SwapTotal: 0 kB
SwapFree: 0 kB
-Dirty: 468 kB
+Dirty: 2804 kB
Writeback: 0 kB
-AnonPages: 45192 kB
-Mapped: 72028 kB
-Shmem: 916 kB
-KReclaimable: 5816 kB
-Slab: 42336 kB
-SReclaimable: 5816 kB
-SUnreclaim: 36520 kB
-KernelStack: 1984 kB
-PageTables: 2652 kB
+AnonPages: 53556 kB
+Mapped: 105784 kB
+Shmem: 936 kB
+KReclaimable: 7468 kB
+Slab: 176080 kB
+SReclaimable: 7468 kB
+SUnreclaim: 168612 kB
+KernelStack: 2016 kB
+PageTables: 5472 kB
SecPageTables: 0 kB
NFS_Unstable: 0 kB
Bounce: 0 kB
WritebackTmp: 0 kB
CommitLimit: 3542228 kB
-Committed_AS: 89960 kB
+Committed_AS: 92956 kB
VmallocTotal: 34359738367 kB
-VmallocUsed: 5112 kB
+VmallocUsed: 1041252 kB
VmallocChunk: 0 kB
Percpu: 1712 kB
AnonHugePages: 4096 kB
@@ -49,6 +49,6 @@
HugePages_Surp: 0
Hugepagesize: 2048 kB
Hugetlb: 1048576 kB
-DirectMap4k: 30548 kB
-DirectMap2M: 3115008 kB
-DirectMap1G: 7340032 kB
+DirectMap4k: 730964 kB
+DirectMap2M: 3463168 kB
+DirectMap1G: 6291456 kB
4. Check kernel logs
[ 30.099667] tee (618): drop_caches: 3
[ 221.695667] execmem: unable to allocate memory
{...}
[ 226.701772] execmem_vmalloc: 1110 callbacks suppressed
{...}
[ 231.705976] execmem_vmalloc: 1099 callbacks suppressed
{...}
[ 231.742226] execmem: unable to allocate memory
[ 231.747107] execmem: unable to allocate memory
[ 240.109107] tee (92935): drop_caches: 3
Now, I repeated the test without error injection:
Loop:
while true; do modprobe brd && rmmod brd; done
[*] Dropping caches...
[*] Wait...
[*] Starting memory: 6747960 kB
Loop 100s...
[*] Wait...
[*] Dropping caches...
[*] Wait...
[*] Ending memory: 6747068 kB
[*] Memory lost: 892 kB
--- meminfo_before.txt 2025-06-30 11:21:44.922597662 +0000
+++ meminfo_after.txt 2025-06-30 11:23:35.023419139 +0000
@@ -1,38 +1,38 @@
MemTotal: 8133036 kB
-MemFree: 6853796 kB
-MemAvailable: 6747960 kB
-Buffers: 2920 kB
-Cached: 61976 kB
+MemFree: 6852956 kB
+MemAvailable: 6747244 kB
+Buffers: 2540 kB
+Cached: 62124 kB
SwapCached: 0 kB
-Active: 25968 kB
-Inactive: 83616 kB
-Active(anon): 176 kB
-Inactive(anon): 45744 kB
-Active(file): 25792 kB
-Inactive(file): 37872 kB
+Active: 26744 kB
+Inactive: 83120 kB
+Active(anon): 172 kB
+Inactive(anon): 46048 kB
+Active(file): 26572 kB
+Inactive(file): 37072 kB
Unevictable: 0 kB
Mlocked: 0 kB
SwapTotal: 0 kB
SwapFree: 0 kB
-Dirty: 996 kB
+Dirty: 452 kB
Writeback: 0 kB
-AnonPages: 45116 kB
-Mapped: 54632 kB
-Shmem: 928 kB
-KReclaimable: 5680 kB
-Slab: 42392 kB
-SReclaimable: 5680 kB
-SUnreclaim: 36712 kB
-KernelStack: 1888 kB
-PageTables: 2468 kB
+AnonPages: 45372 kB
+Mapped: 59120 kB
+Shmem: 992 kB
+KReclaimable: 5940 kB
+Slab: 43056 kB
+SReclaimable: 5940 kB
+SUnreclaim: 37116 kB
+KernelStack: 1904 kB
+PageTables: 2460 kB
SecPageTables: 0 kB
NFS_Unstable: 0 kB
Bounce: 0 kB
WritebackTmp: 0 kB
CommitLimit: 3542228 kB
-Committed_AS: 81484 kB
+Committed_AS: 81680 kB
VmallocTotal: 34359738367 kB
-VmallocUsed: 5056 kB
+VmallocUsed: 7112 kB
VmallocChunk: 0 kB
Percpu: 1632 kB
AnonHugePages: 2048 kB
@@ -49,6 +49,6 @@
HugePages_Surp: 0
Hugepagesize: 2048 kB
Hugetlb: 1048576 kB
-DirectMap4k: 28500 kB
-DirectMap2M: 3117056 kB
-DirectMap1G: 7340032 kB
+DirectMap4k: 274260 kB
+DirectMap2M: 3919872 kB
+DirectMap1G: 6291456 kB
^ permalink raw reply
* Re: [PATCH v13 5/6] rust: samples: add a module parameter to the rust_minimal sample
From: Andreas Hindborg @ 2025-06-30 12:12 UTC (permalink / raw)
To: Danilo Krummrich
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
=?utf-8?Q?Bj=C3=B6rn?= Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Benno Lossin, Nicolas Schier,
Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules
In-Reply-To: <a343d3ce-a179-4c2c-a279-e2c3919a0a99@kernel.org>
"Danilo Krummrich" <dakr@kernel.org> writes:
> (Sorry for being late on this one, just a minor nit below.)
>
> On 6/12/25 3:40 PM, Andreas Hindborg wrote:
>> struct RustMinimal {
>> @@ -20,6 +26,10 @@ impl kernel::Module for RustMinimal {
>> fn init(_module: &'static ThisModule) -> Result<Self> {
>> pr_info!("Rust minimal sample (init)\n");
>> pr_info!("Am I built-in? {}\n", !cfg!(MODULE));
>> + pr_info!(
>> + "test_parameter: {}\n",
>> + *module_parameters::test_parameter.get()
>
> Can we please call it something else than get(), maybe obtain(), access() or
> just ref()?
Probably `ref` is the most precise of the options you propose. I would
go with that one. Or, should it be `as_ref`?
Best regards,
Andreas Hindborg
^ permalink raw reply
* Re: [PATCH v13 5/6] rust: samples: add a module parameter to the rust_minimal sample
From: Danilo Krummrich @ 2025-06-30 12:18 UTC (permalink / raw)
To: Andreas Hindborg
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Benno Lossin, Nicolas Schier,
Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules
In-Reply-To: <87qzz1xwdg.fsf@kernel.org>
On 6/30/25 2:12 PM, Andreas Hindborg wrote:
> "Danilo Krummrich" <dakr@kernel.org> writes:
>
>> (Sorry for being late on this one, just a minor nit below.)
>>
>> On 6/12/25 3:40 PM, Andreas Hindborg wrote:
>>> struct RustMinimal {
>>> @@ -20,6 +26,10 @@ impl kernel::Module for RustMinimal {
>>> fn init(_module: &'static ThisModule) -> Result<Self> {
>>> pr_info!("Rust minimal sample (init)\n");
>>> pr_info!("Am I built-in? {}\n", !cfg!(MODULE));
>>> + pr_info!(
>>> + "test_parameter: {}\n",
>>> + *module_parameters::test_parameter.get()
>>
>> Can we please call it something else than get(), maybe obtain(), access() or
>> just ref()?
>
> Probably `ref` is the most precise of the options you propose. I would
> go with that one. Or, should it be `as_ref`?
Guess that works as well.
One question additional question: Can't we just impl Deref for
ModuleParamAccess<T>?
^ permalink raw reply
* Re: [PATCH v13 5/6] rust: samples: add a module parameter to the rust_minimal sample
From: Danilo Krummrich @ 2025-06-30 12:23 UTC (permalink / raw)
To: Andreas Hindborg
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Benno Lossin, Nicolas Schier,
Trevor Gross, Adam Bratschi-Kaye, rust-for-linux, linux-kernel,
linux-kbuild, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Simona Vetter, Greg KH, Fiona Behrens, Daniel Almeida,
linux-modules
In-Reply-To: <211a3491-7082-488a-b744-fa7416db18c3@kernel.org>
On 6/30/25 2:18 PM, Danilo Krummrich wrote:
> On 6/30/25 2:12 PM, Andreas Hindborg wrote:
>> "Danilo Krummrich" <dakr@kernel.org> writes:
>>
>>> (Sorry for being late on this one, just a minor nit below.)
>>>
>>> On 6/12/25 3:40 PM, Andreas Hindborg wrote:
>>>> struct RustMinimal {
>>>> @@ -20,6 +26,10 @@ impl kernel::Module for RustMinimal {
>>>> fn init(_module: &'static ThisModule) -> Result<Self> {
>>>> pr_info!("Rust minimal sample (init)\n");
>>>> pr_info!("Am I built-in? {}\n", !cfg!(MODULE));
>>>> + pr_info!(
>>>> + "test_parameter: {}\n",
>>>> + *module_parameters::test_parameter.get()
>>>
>>> Can we please call it something else than get(), maybe obtain(), access() or
>>> just ref()?
>>
>> Probably `ref` is the most precise of the options you propose. I would
>> go with that one. Or, should it be `as_ref`?
>
> Guess that works as well.
>
> One question additional question: Can't we just impl Deref for
> ModuleParamAccess<T>?
Just notice that it would work for now, but not in the future, because this will
apparently require some lock guard? Then maybe access() describes it best?
^ permalink raw reply
* Re: [PATCH v13 2/6] rust: introduce module_param module
From: Benno Lossin @ 2025-06-30 12:27 UTC (permalink / raw)
To: Andreas Hindborg
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier, Trevor Gross, Adam Bratschi-Kaye, rust-for-linux,
linux-kernel, linux-kbuild, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, Simona Vetter, Greg KH, Fiona Behrens,
Daniel Almeida, linux-modules
In-Reply-To: <87wm8txysl.fsf@kernel.org>
On Mon Jun 30, 2025 at 1:18 PM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
>> On Fri Jun 27, 2025 at 9:57 AM CEST, Andreas Hindborg wrote:
>>> Andreas Hindborg <a.hindborg@kernel.org> writes:
>>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>>> That's good to know, then let's try to go for something simple.
>>>>>
>>>>> I don't think that we can just use a `Mutex<T>`, because we don't have a
>>>>> way to create it at const time... I guess we could have
>>>>>
>>>>> impl<T> Mutex<T>
>>>>> /// # Safety
>>>>> ///
>>>>> /// The returned value needs to be pinned and then `init` needs
>>>>> /// to be called before any other methods are called on this.
>>>>> pub unsafe const fn const_new() -> Self;
>>>>>
>>>>> pub unsafe fn init(&self);
>>>>> }
>>>>>
>>>>> But that seems like a bad idea, because where would we call the `init`
>>>>> function? That also needs to be synchronized...
>>>>
>>>> Ah, that is unfortunate. The init function will not run before this, so
>>>> we would need a `Once` or an atomic anyway to initialize the lock.
>>>>
>>>> I am not sure if we are allowed to sleep during this, I would have to
>>>> check. But then we could use a spin lock.
>>>>
>>>> We will need the locking anyway, when we want to enable sysfs write
>>>> access to the parameters.
>>>>
>>>>>
>>>>> Maybe we can just like you said use an atomic bool?
>>>>
>>>> Sigh, I will have to check how far that series has come.
>>>>
>>>
>>> I think I am going to build some kind of `Once` feature on top of
>>> Boqun's atomic series [1], so that we can initialize a lock in these
>>> statics. We can't use `global_lock!`, because that depends on module
>>> init to initialize the lock before first use.
>>
>> Sounds good, though we probably don't want to name it `Once`. Since it
>> is something that will be populated in the future, but not by some
>> random accessor, but rather a specific populator.
>>
>> So maybe:
>>
>> pub struct Delayed<T> {
>> dummy: T,
>> real: Opaque<T>,
>> populated: Atomic<bool>, // or Atomic<Flag>
>> writing: Atomic<bool>, // or Atomic<Flag>
>> }
>>
>> impl<T> Delayed<T> {
>> pub fn new(dummy: T) -> Self {
>> Self {
>> dummy,
>> real: Opaque::uninit(),
>> populated: Atomic::new(false),
>> writing: Atomic::new(false),
>> }
>> }
>>
>> pub fn get(&self) -> &T {
>> if self.populated.load(Acquire) {
>> unsafe { &*self.real.get() }
>> } else {
>> // maybe print a warning here?
>> // or maybe let the user configure this in `new()`?
>> &self.dummy
>> }
>> }
>>
>> pub fn populate(&self, value: T) {
>> if self.writing.cmpxchg(false, true, Release) {
>> unsafe { *self.real.get() = value };
>> self.populated.store(true, Release);
>> } else {
>> pr_warn!("`Delayed<{}>` written to twice!\n", core::any::type_name::<T>());
>> }
>> }
>> }
>>
>> (no idea if the orderings are correct, I always have to think way to
>> much about that... especially since our atomics seem to only take one
>> ordering in compare_exchange?)
>>
>>> As far as I can tell, atomics may not land in v6.17, so this series
>>> will probably not be ready for merge until v6.18 at the earliest.
>>
>> Yeah, sorry about that :(
>
> Actually, perhaps we could aim at merging this code without this
> synchronization?
I won't remember this issue in a few weeks and I fear that it will just
get buried. In fact, I already had to re-read now what the actual issue
was...
> The lack of synchronization is only a problem if we
> support custom parsing. This patch set does not allow custom parsing
> code, so it does not suffer this issue.
... In doing that, I saw my original example of UB:
module! {
// ...
params: {
my_param: i64 {
default: 0,
description: "",
},
},
}
static BAD: &'static i64 = module_parameters::my_param.get();
That can happen without custom parsing, so it's still a problem...
---
Cheers,
Benno
^ permalink raw reply
* Re: [PATCH v13 5/6] rust: samples: add a module parameter to the rust_minimal sample
From: Benno Lossin @ 2025-06-30 12:31 UTC (permalink / raw)
To: Danilo Krummrich, Andreas Hindborg
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Nicolas Schier, Trevor Gross,
Adam Bratschi-Kaye, rust-for-linux, linux-kernel, linux-kbuild,
Petr Pavlu, Sami Tolvanen, Daniel Gomez, Simona Vetter, Greg KH,
Fiona Behrens, Daniel Almeida, linux-modules
In-Reply-To: <a29fa1fe-b614-4f2f-8da7-4feb4b73953f@kernel.org>
On Mon Jun 30, 2025 at 2:23 PM CEST, Danilo Krummrich wrote:
> On 6/30/25 2:18 PM, Danilo Krummrich wrote:
>> On 6/30/25 2:12 PM, Andreas Hindborg wrote:
>>> "Danilo Krummrich" <dakr@kernel.org> writes:
>>>
>>>> (Sorry for being late on this one, just a minor nit below.)
>>>>
>>>> On 6/12/25 3:40 PM, Andreas Hindborg wrote:
>>>>> struct RustMinimal {
>>>>> @@ -20,6 +26,10 @@ impl kernel::Module for RustMinimal {
>>>>> fn init(_module: &'static ThisModule) -> Result<Self> {
>>>>> pr_info!("Rust minimal sample (init)\n");
>>>>> pr_info!("Am I built-in? {}\n", !cfg!(MODULE));
>>>>> + pr_info!(
>>>>> + "test_parameter: {}\n",
>>>>> + *module_parameters::test_parameter.get()
>>>>
>>>> Can we please call it something else than get(), maybe obtain(), access() or
>>>> just ref()?
>>>
>>> Probably `ref` is the most precise of the options you propose. I would
>>> go with that one. Or, should it be `as_ref`?
>>
>> Guess that works as well.
>>
>> One question additional question: Can't we just impl Deref for
>> ModuleParamAccess<T>?
>
> Just notice that it would work for now, but not in the future, because this will
> apparently require some lock guard? Then maybe access() describes it best?
What about `value()`?
---
Cheers,
Benno
^ permalink raw reply
* Re: [PATCH v13 2/6] rust: introduce module_param module
From: Andreas Hindborg @ 2025-06-30 13:15 UTC (permalink / raw)
To: Benno Lossin
Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Alice Ryhl, Masahiro Yamada,
Nathan Chancellor, Luis Chamberlain, Danilo Krummrich,
Nicolas Schier, Trevor Gross, Adam Bratschi-Kaye, rust-for-linux,
linux-kernel, linux-kbuild, Petr Pavlu, Sami Tolvanen,
Daniel Gomez, Simona Vetter, Greg KH, Fiona Behrens,
Daniel Almeida, linux-modules
In-Reply-To: <DAZV8OGL8BMH.11SLXBXQ17ZJ9@kernel.org>
"Benno Lossin" <lossin@kernel.org> writes:
> On Mon Jun 30, 2025 at 1:18 PM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@kernel.org> writes:
>>> On Fri Jun 27, 2025 at 9:57 AM CEST, Andreas Hindborg wrote:
>>>> Andreas Hindborg <a.hindborg@kernel.org> writes:
>>>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>>>> That's good to know, then let's try to go for something simple.
>>>>>>
>>>>>> I don't think that we can just use a `Mutex<T>`, because we don't have a
>>>>>> way to create it at const time... I guess we could have
>>>>>>
>>>>>> impl<T> Mutex<T>
>>>>>> /// # Safety
>>>>>> ///
>>>>>> /// The returned value needs to be pinned and then `init` needs
>>>>>> /// to be called before any other methods are called on this.
>>>>>> pub unsafe const fn const_new() -> Self;
>>>>>>
>>>>>> pub unsafe fn init(&self);
>>>>>> }
>>>>>>
>>>>>> But that seems like a bad idea, because where would we call the `init`
>>>>>> function? That also needs to be synchronized...
>>>>>
>>>>> Ah, that is unfortunate. The init function will not run before this, so
>>>>> we would need a `Once` or an atomic anyway to initialize the lock.
>>>>>
>>>>> I am not sure if we are allowed to sleep during this, I would have to
>>>>> check. But then we could use a spin lock.
>>>>>
>>>>> We will need the locking anyway, when we want to enable sysfs write
>>>>> access to the parameters.
>>>>>
>>>>>>
>>>>>> Maybe we can just like you said use an atomic bool?
>>>>>
>>>>> Sigh, I will have to check how far that series has come.
>>>>>
>>>>
>>>> I think I am going to build some kind of `Once` feature on top of
>>>> Boqun's atomic series [1], so that we can initialize a lock in these
>>>> statics. We can't use `global_lock!`, because that depends on module
>>>> init to initialize the lock before first use.
>>>
>>> Sounds good, though we probably don't want to name it `Once`. Since it
>>> is something that will be populated in the future, but not by some
>>> random accessor, but rather a specific populator.
>>>
>>> So maybe:
>>>
>>> pub struct Delayed<T> {
>>> dummy: T,
>>> real: Opaque<T>,
>>> populated: Atomic<bool>, // or Atomic<Flag>
>>> writing: Atomic<bool>, // or Atomic<Flag>
>>> }
>>>
>>> impl<T> Delayed<T> {
>>> pub fn new(dummy: T) -> Self {
>>> Self {
>>> dummy,
>>> real: Opaque::uninit(),
>>> populated: Atomic::new(false),
>>> writing: Atomic::new(false),
>>> }
>>> }
>>>
>>> pub fn get(&self) -> &T {
>>> if self.populated.load(Acquire) {
>>> unsafe { &*self.real.get() }
>>> } else {
>>> // maybe print a warning here?
>>> // or maybe let the user configure this in `new()`?
>>> &self.dummy
>>> }
>>> }
>>>
>>> pub fn populate(&self, value: T) {
>>> if self.writing.cmpxchg(false, true, Release) {
>>> unsafe { *self.real.get() = value };
>>> self.populated.store(true, Release);
>>> } else {
>>> pr_warn!("`Delayed<{}>` written to twice!\n", core::any::type_name::<T>());
>>> }
>>> }
>>> }
>>>
>>> (no idea if the orderings are correct, I always have to think way to
>>> much about that... especially since our atomics seem to only take one
>>> ordering in compare_exchange?)
>>>
>>>> As far as I can tell, atomics may not land in v6.17, so this series
>>>> will probably not be ready for merge until v6.18 at the earliest.
>>>
>>> Yeah, sorry about that :(
>>
>> Actually, perhaps we could aim at merging this code without this
>> synchronization?
>
> I won't remember this issue in a few weeks and I fear that it will just
> get buried. In fact, I already had to re-read now what the actual issue
> was...
>
>> The lack of synchronization is only a problem if we
>> support custom parsing. This patch set does not allow custom parsing
>> code, so it does not suffer this issue.
>
> ... In doing that, I saw my original example of UB:
>
> module! {
> // ...
> params: {
> my_param: i64 {
> default: 0,
> description: "",
> },
> },
> }
>
> static BAD: &'static i64 = module_parameters::my_param.get();
>
> That can happen without custom parsing, so it's still a problem...
Ah, got it. Thanks.
Best regards,
Andreas Hindborg
^ permalink raw reply
* Re: [PATCH] gendwarfksyms: order -T symtypes output by name
From: Masahiro Yamada @ 2025-06-30 13:24 UTC (permalink / raw)
To: Giuliano Procida
Cc: Sami Tolvanen, Greg Kroah-Hartman, linux-modules, linux-kbuild,
linux-kernel
In-Reply-To: <CAGvU0HnzfLxGhLT3Se4wNvyzEkpTKmd8ATFFgBRBVNrOKDXcgA@mail.gmail.com>
On Mon, Jun 30, 2025 at 7:05 PM Giuliano Procida <gprocida@google.com> wrote:
>
> Hi.
>
> On Sun, 29 Jun 2025 at 18:51, Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > On Wed, Jun 25, 2025 at 6:52 PM Giuliano Procida <gprocida@google.com> wrote:
> > >
> > > When writing symtypes information, we iterate through the entire hash
> > > table containing type expansions. The key order varies unpredictably
> > > as new entries are added, making it harder to compare symtypes between
> > > builds.
> > >
> > > Resolve this by sorting the type expansions by name before output.
> > >
> > > Signed-off-by: Giuliano Procida <gprocida@google.com>
> > > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
> > > ---
> > > scripts/gendwarfksyms/types.c | 29 ++++++++++++++++++++++++++---
> > > 1 file changed, 26 insertions(+), 3 deletions(-)
> > >
> > > [Adjusted the first line of the description. Added reviewer tags.
> > > Added missing CC to linux-modules.]
> > >
> > > diff --git a/scripts/gendwarfksyms/types.c b/scripts/gendwarfksyms/types.c
> > > index 7bd459ea6c59..51c1471e8684 100644
> > > --- a/scripts/gendwarfksyms/types.c
> > > +++ b/scripts/gendwarfksyms/types.c
> > > @@ -6,6 +6,8 @@
> > > #define _GNU_SOURCE
> > > #include <inttypes.h>
> > > #include <stdio.h>
> > > +#include <stdlib.h>
> > > +#include <string.h>
> > > #include <zlib.h>
> > >
> > > #include "gendwarfksyms.h"
> > > @@ -179,20 +181,41 @@ static int type_map_get(const char *name, struct type_expansion **res)
> > > return -1;
> > > }
> > >
> > > +static int cmp_expansion_name(const void *p1, const void *p2)
> > > +{
> > > + struct type_expansion *const *e1 = p1;
> > > + struct type_expansion *const *e2 = p2;
> > > +
> > > + return strcmp((*e1)->name, (*e2)->name);
> > > +}
> > > +
> > > static void type_map_write(FILE *file)
> > > {
> > > struct type_expansion *e;
> > > struct hlist_node *tmp;
> > > + struct type_expansion **es;
> > > + size_t count = 0;
> > > + size_t i = 0;
> > >
> > > if (!file)
> > > return;
> > >
> > > - hash_for_each_safe(type_map, e, tmp, hash) {
> > > - checkp(fputs(e->name, file));
> > > + hash_for_each_safe(type_map, e, tmp, hash)
> > > + ++count;
> > > + es = xmalloc(count * sizeof(struct type_expansion *));
> >
> > Just a nit:
> >
> > es = xmalloc(count * sizeof(*es));
> >
> > is better?
> >
> > > + hash_for_each_safe(type_map, e, tmp, hash)
> > > + es[i++] = e;
> > > +
> > > + qsort(es, count, sizeof(struct type_expansion *), cmp_expansion_name);
> >
> > qsort(es, count, sizeof(*es), cmp_expansion_name);
> >
>
> That's a fair point.
>
> However, in the gendwarfksyms code, all but one of the sizeofs uses an
> explicit type name. The exception is sizeof(stats) where stats is an array.
>
> I'll leave Sami's code as it is.
This rule is clearly documented with rationale.
See this:
https://github.com/torvalds/linux/blob/v6.15/Documentation/process/coding-style.rst?plain=1#L941
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* Re: [PATCH] gendwarfksyms: order -T symtypes output by name
From: Giuliano Procida @ 2025-06-30 13:45 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Sami Tolvanen, Greg Kroah-Hartman, linux-modules, linux-kbuild,
linux-kernel
In-Reply-To: <CAK7LNATp1n2c9RqNoe0oztRtLoMy8JqHF1KqSRsj5avp3vjHCQ@mail.gmail.com>
On Mon, 30 Jun 2025 at 14:24, Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Mon, Jun 30, 2025 at 7:05 PM Giuliano Procida <gprocida@google.com> wrote:
> >
> > Hi.
> >
> > On Sun, 29 Jun 2025 at 18:51, Masahiro Yamada <masahiroy@kernel.org> wrote:
> > >
> > > On Wed, Jun 25, 2025 at 6:52 PM Giuliano Procida <gprocida@google.com> wrote:
> > > >
> > > > When writing symtypes information, we iterate through the entire hash
> > > > table containing type expansions. The key order varies unpredictably
> > > > as new entries are added, making it harder to compare symtypes between
> > > > builds.
> > > >
> > > > Resolve this by sorting the type expansions by name before output.
> > > >
> > > > Signed-off-by: Giuliano Procida <gprocida@google.com>
> > > > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
> > > > ---
> > > > scripts/gendwarfksyms/types.c | 29 ++++++++++++++++++++++++++---
> > > > 1 file changed, 26 insertions(+), 3 deletions(-)
> > > >
> > > > [Adjusted the first line of the description. Added reviewer tags.
> > > > Added missing CC to linux-modules.]
> > > >
> > > > diff --git a/scripts/gendwarfksyms/types.c b/scripts/gendwarfksyms/types.c
> > > > index 7bd459ea6c59..51c1471e8684 100644
> > > > --- a/scripts/gendwarfksyms/types.c
> > > > +++ b/scripts/gendwarfksyms/types.c
> > > > @@ -6,6 +6,8 @@
> > > > #define _GNU_SOURCE
> > > > #include <inttypes.h>
> > > > #include <stdio.h>
> > > > +#include <stdlib.h>
> > > > +#include <string.h>
> > > > #include <zlib.h>
> > > >
> > > > #include "gendwarfksyms.h"
> > > > @@ -179,20 +181,41 @@ static int type_map_get(const char *name, struct type_expansion **res)
> > > > return -1;
> > > > }
> > > >
> > > > +static int cmp_expansion_name(const void *p1, const void *p2)
> > > > +{
> > > > + struct type_expansion *const *e1 = p1;
> > > > + struct type_expansion *const *e2 = p2;
> > > > +
> > > > + return strcmp((*e1)->name, (*e2)->name);
> > > > +}
> > > > +
> > > > static void type_map_write(FILE *file)
> > > > {
> > > > struct type_expansion *e;
> > > > struct hlist_node *tmp;
> > > > + struct type_expansion **es;
> > > > + size_t count = 0;
> > > > + size_t i = 0;
> > > >
> > > > if (!file)
> > > > return;
> > > >
> > > > - hash_for_each_safe(type_map, e, tmp, hash) {
> > > > - checkp(fputs(e->name, file));
> > > > + hash_for_each_safe(type_map, e, tmp, hash)
> > > > + ++count;
> > > > + es = xmalloc(count * sizeof(struct type_expansion *));
> > >
> > > Just a nit:
> > >
> > > es = xmalloc(count * sizeof(*es));
> > >
> > > is better?
> > >
> > > > + hash_for_each_safe(type_map, e, tmp, hash)
> > > > + es[i++] = e;
> > > > +
> > > > + qsort(es, count, sizeof(struct type_expansion *), cmp_expansion_name);
> > >
> > > qsort(es, count, sizeof(*es), cmp_expansion_name);
> > >
> >
> > That's a fair point.
> >
> > However, in the gendwarfksyms code, all but one of the sizeofs uses an
> > explicit type name. The exception is sizeof(stats) where stats is an array.
> >
> > I'll leave Sami's code as it is.
>
>
> This rule is clearly documented with rationale.
>
> See this:
> https://github.com/torvalds/linux/blob/v6.15/Documentation/process/coding-style.rst?plain=1#L941
>
>
I can follow up with a change that adjusts all occurrences. That
shouldn't take long at all.
>
> --
> Best Regards
> Masahiro Yamada
^ permalink raw reply
* Re: [PATCH v2 0/2] module: Fix memory deallocation on error path in move_module()
From: Daniel Gomez @ 2025-06-30 13:57 UTC (permalink / raw)
To: Luis Chamberlain, Sami Tolvanen, Petr Pavlu
Cc: Daniel Gomez, linux-modules, linux-kernel
In-Reply-To: <20250618122730.51324-1-petr.pavlu@suse.com>
From: Daniel Gomez <da.gomez@samsung.com>
On Wed, 18 Jun 2025 14:26:25 +0200, Petr Pavlu wrote:
> The first patch is an actual fix. The second patch is a minor related
> cleanup.
>
> Changes since v1 [1]:
> - Initialize t to MOD_MEM_NUM_TYPES in move_module(), instead of assigning
> the value later.
> - Merge the definitions of the variables i and ret in move_module().
>
> [...]
Applied, thanks!
[1/2] module: Fix memory deallocation on error path in move_module()
commit: caca01f251ac37807ff10380d5dafb9576cdfef0
[2/2] module: Avoid unnecessary return value initialization in move_module()
commit: f4e47f3ac79849846189058491ff885491223ab8
Best regards,
--
Daniel Gomez <da.gomez@samsung.com>
^ permalink raw reply
* [PATCH] testsuite/init_module: Handle __NR_riscv_hwprobe in syscall wrapper
From: Andreas Schwab @ 2025-06-30 14:21 UTC (permalink / raw)
To: linux-modules
The libcrypto library constructor calls the riscv_hwprobe syscall through
the syscall wrapper. Forward it to the __riscv_hwprobe function.
Signed-off-by: Andreas Schwab <schwab@suse.de>
---
testsuite/init_module.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/testsuite/init_module.c b/testsuite/init_module.c
index 8ff2699..3516c1d 100644
--- a/testsuite/init_module.c
+++ b/testsuite/init_module.c
@@ -26,6 +26,9 @@
#include <sys/syscall.h>
#include <sys/types.h>
#include <sys/utsname.h>
+#ifdef __riscv
+#include <sys/hwprobe.h>
+#endif
#include <shared/util.h>
@@ -370,6 +373,24 @@ TS_EXPORT long int syscall(long int __sysno, ...)
return nextlib_syscall(__NR_gettid);
}
+#ifdef __NR_riscv_hwprobe
+ if (__sysno == __NR_riscv_hwprobe) {
+ struct riscv_hwprobe *pairs;
+ size_t pair_count, cpusetsize;
+ unsigned long int *cpus;
+ unsigned int flags;
+
+ va_start (ap, __sysno);
+ pairs = va_arg (ap, struct riscv_hwprobe *);
+ pair_count = va_arg (ap, size_t);
+ cpusetsize = va_arg (ap, size_t);
+ cpus = va_arg (ap, unsigned long int *);
+ flags = va_arg (ap, unsigned int);
+
+ return __riscv_hwprobe (pairs, pair_count, cpusetsize, cpus, flags);
+ }
+#endif
+
/*
* FIXME: no way to call the libc function due since this is a
* variadic argument function and we don't have a vsyscall() variant
--
2.50.0
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply related
* [PATCH] gendwarfksyms: use preferred form of sizeof for allocation
From: Giuliano Procida @ 2025-06-30 14:27 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Giuliano Procida, Sami Tolvanen, Greg Kroah-Hartman,
linux-modules, linux-kbuild, linux-kernel
In-Reply-To: <CAGvU0HkKacQKB1q9NWcqChLGoMB+1vu9UdqYc+tBRbTTc3++GQ@mail.gmail.com>
The preferred form is to supply the variable being allocated to rather
than an explicit type name which might become stale.
Also do this for memset and qsort arguments.
Suggested-by: Masahiro Yamada <masahiroy@kernel.org>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
scripts/gendwarfksyms/cache.c | 2 +-
scripts/gendwarfksyms/die.c | 4 ++--
scripts/gendwarfksyms/dwarf.c | 2 +-
scripts/gendwarfksyms/kabi.c | 2 +-
scripts/gendwarfksyms/symbols.c | 2 +-
scripts/gendwarfksyms/types.c | 8 ++++----
6 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/scripts/gendwarfksyms/cache.c b/scripts/gendwarfksyms/cache.c
index c9c19b86a686..1c640db93db3 100644
--- a/scripts/gendwarfksyms/cache.c
+++ b/scripts/gendwarfksyms/cache.c
@@ -15,7 +15,7 @@ void cache_set(struct cache *cache, unsigned long key, int value)
{
struct cache_item *ci;
- ci = xmalloc(sizeof(struct cache_item));
+ ci = xmalloc(sizeof(*ci));
ci->key = key;
ci->value = value;
hash_add(cache->cache, &ci->hash, hash_32(key));
diff --git a/scripts/gendwarfksyms/die.c b/scripts/gendwarfksyms/die.c
index 6183bbbe7b54..052f7a3f975a 100644
--- a/scripts/gendwarfksyms/die.c
+++ b/scripts/gendwarfksyms/die.c
@@ -33,7 +33,7 @@ static struct die *create_die(Dwarf_Die *die, enum die_state state)
{
struct die *cd;
- cd = xmalloc(sizeof(struct die));
+ cd = xmalloc(sizeof(*cd));
init_die(cd);
cd->addr = (uintptr_t)die->addr;
@@ -123,7 +123,7 @@ static struct die_fragment *append_item(struct die *cd)
{
struct die_fragment *df;
- df = xmalloc(sizeof(struct die_fragment));
+ df = xmalloc(sizeof(*df));
df->type = FRAGMENT_EMPTY;
list_add_tail(&df->list, &cd->fragments);
return df;
diff --git a/scripts/gendwarfksyms/dwarf.c b/scripts/gendwarfksyms/dwarf.c
index 13ea7bf1ae7d..3538a7d9cb07 100644
--- a/scripts/gendwarfksyms/dwarf.c
+++ b/scripts/gendwarfksyms/dwarf.c
@@ -634,7 +634,7 @@ static int get_union_kabi_status(Dwarf_Die *die, Dwarf_Die *placeholder,
* Note that the user of this feature is responsible for ensuring
* that the structure actually remains ABI compatible.
*/
- memset(&state.kabi, 0, sizeof(struct kabi_state));
+ memset(&state.kabi, 0, sizeof(state.kabi));
res = checkp(process_die_container(&state, NULL, die,
check_union_member_kabi_status,
diff --git a/scripts/gendwarfksyms/kabi.c b/scripts/gendwarfksyms/kabi.c
index b3ade713778f..e3c2a3ccf51a 100644
--- a/scripts/gendwarfksyms/kabi.c
+++ b/scripts/gendwarfksyms/kabi.c
@@ -228,7 +228,7 @@ void kabi_read_rules(int fd)
if (type == KABI_RULE_TYPE_UNKNOWN)
error("unsupported kABI rule type: '%s'", field);
- rule = xmalloc(sizeof(struct rule));
+ rule = xmalloc(sizeof(*rule));
rule->type = type;
rule->target = xstrdup(get_rule_field(&rule_str, &left));
diff --git a/scripts/gendwarfksyms/symbols.c b/scripts/gendwarfksyms/symbols.c
index 327f87389c34..35ed594f0749 100644
--- a/scripts/gendwarfksyms/symbols.c
+++ b/scripts/gendwarfksyms/symbols.c
@@ -146,7 +146,7 @@ void symbol_read_exports(FILE *file)
continue;
}
- sym = xcalloc(1, sizeof(struct symbol));
+ sym = xcalloc(1, sizeof(*sym));
sym->name = name;
sym->addr.section = SHN_UNDEF;
sym->state = SYMBOL_UNPROCESSED;
diff --git a/scripts/gendwarfksyms/types.c b/scripts/gendwarfksyms/types.c
index 51c1471e8684..9c3b053bf061 100644
--- a/scripts/gendwarfksyms/types.c
+++ b/scripts/gendwarfksyms/types.c
@@ -45,7 +45,7 @@ static int type_list_append(struct list_head *list, const char *s, void *owned)
if (!s)
return 0;
- entry = xmalloc(sizeof(struct type_list_entry));
+ entry = xmalloc(sizeof(*entry));
entry->str = s;
entry->owned = owned;
list_add_tail(&entry->list, list);
@@ -122,7 +122,7 @@ static struct type_expansion *type_map_add(const char *name,
struct type_expansion *e;
if (__type_map_get(name, &e)) {
- e = xmalloc(sizeof(struct type_expansion));
+ e = xmalloc(sizeof(*e));
type_expansion_init(e);
e->name = xstrdup(name);
@@ -202,11 +202,11 @@ static void type_map_write(FILE *file)
hash_for_each_safe(type_map, e, tmp, hash)
++count;
- es = xmalloc(count * sizeof(struct type_expansion *));
+ es = xmalloc(count * sizeof(*es));
hash_for_each_safe(type_map, e, tmp, hash)
es[i++] = e;
- qsort(es, count, sizeof(struct type_expansion *), cmp_expansion_name);
+ qsort(es, count, sizeof(*es), cmp_expansion_name);
for (i = 0; i < count; ++i) {
checkp(fputs(es[i]->name, file));
--
2.50.0.727.gbf7dc18ff4-goog
^ permalink raw reply related
* [PATCH 0/5] module: Fix minor problems related to MODULE_NAME_LEN
From: Petr Pavlu @ 2025-06-30 14:32 UTC (permalink / raw)
To: Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Daniel Gomez
Cc: linux-modules, linux-kernel
Fix a few minor problems related to MODULE_NAME_LEN and
MAX_PARAM_PREFIX_LEN, and clean up their usage.
Petr Pavlu (5):
module: Prevent silent truncation of module name in delete_module(2)
module: Remove unnecessary +1 from last_unloaded_module::name size
module: Restore the moduleparam prefix length check
tracing: Replace MAX_PARAM_PREFIX_LEN with MODULE_NAME_LEN
module: Rename MAX_PARAM_PREFIX_LEN to __MODULE_NAME_LEN
include/linux/module.h | 2 +-
include/linux/moduleparam.h | 15 +++++++++------
kernel/module/main.c | 12 +++++++-----
kernel/trace/trace.c | 2 +-
4 files changed, 18 insertions(+), 13 deletions(-)
base-commit: d0b3b7b22dfa1f4b515fd3a295b3fd958f9e81af
--
2.49.0
^ permalink raw reply
* [PATCH 2/5] module: Remove unnecessary +1 from last_unloaded_module::name size
From: Petr Pavlu @ 2025-06-30 14:32 UTC (permalink / raw)
To: Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Daniel Gomez
Cc: linux-modules, linux-kernel
In-Reply-To: <20250630143535.267745-1-petr.pavlu@suse.com>
The variable last_unloaded_module::name tracks the name of the last
unloaded module. It is a string copy of module::name, which is
MODULE_NAME_LEN bytes in size and includes the NUL terminator. Therefore,
the size of last_unloaded_module::name can also be just MODULE_NAME_LEN,
without the need for an extra byte.
Fixes: e14af7eeb47e ("debug: track and print last unloaded module in the oops trace")
Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
kernel/module/main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 933a9854cb7d..04173543639c 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -580,7 +580,7 @@ MODINFO_ATTR(version);
MODINFO_ATTR(srcversion);
static struct {
- char name[MODULE_NAME_LEN + 1];
+ char name[MODULE_NAME_LEN];
char taints[MODULE_FLAGS_BUF_SIZE];
} last_unloaded_module;
--
2.49.0
^ permalink raw reply related
* [PATCH 1/5] module: Prevent silent truncation of module name in delete_module(2)
From: Petr Pavlu @ 2025-06-30 14:32 UTC (permalink / raw)
To: Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Daniel Gomez
Cc: linux-modules, linux-kernel
In-Reply-To: <20250630143535.267745-1-petr.pavlu@suse.com>
Passing a module name longer than MODULE_NAME_LEN to the delete_module
syscall results in its silent truncation. This really isn't much of
a problem in practice, but it could theoretically lead to the removal of an
incorrect module. It is more sensible to return ENAMETOOLONG or ENOENT in
such a case.
Update the syscall to return ENOENT, as documented in the delete_module(2)
man page to mean "No module by that name exists." This is appropriate
because a module with a name longer than MODULE_NAME_LEN cannot be loaded
in the first place.
Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
kernel/module/main.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 413ac6ea3702..933a9854cb7d 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -751,14 +751,16 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
struct module *mod;
char name[MODULE_NAME_LEN];
char buf[MODULE_FLAGS_BUF_SIZE];
- int ret, forced = 0;
+ int ret, len, forced = 0;
if (!capable(CAP_SYS_MODULE) || modules_disabled)
return -EPERM;
- if (strncpy_from_user(name, name_user, MODULE_NAME_LEN-1) < 0)
- return -EFAULT;
- name[MODULE_NAME_LEN-1] = '\0';
+ len = strncpy_from_user(name, name_user, MODULE_NAME_LEN);
+ if (len == 0 || len == MODULE_NAME_LEN)
+ return -ENOENT;
+ if (len < 0)
+ return len;
audit_log_kern_module(name);
--
2.49.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox