Create versioned parent class for all plugins that enumerate Linux ke… #1719

atcuno · 2025-03-17T17:30:42Z

…rnel modules. Convert plugins to new method.

Ok @ikelos I am really happy to see how this turned out. Previously each of the enumeration plugins printed different columns and used different names. This gets us to where everything is consistent with a proper name.

Furthermore, given the maturity we now have with the versioning of modules, there is no need for a particular plugin to be the "main" one that the others inherit from (like thrdscan in Windows for threads plugins), and instead we can have a fully versioned chain of classes that plugins can use for our self.implementation style enumeration when the outputs will be the same.

You will see the in the comment I left, but this also sets us up to display Load Arguments, but jamming that into this PR would have spiraled it, so that will fill in here once merged.

atcuno · 2025-03-17T17:50:28Z

This will need to be merged after #1717 is merged and then this branch rebased, but otherwise will be good.

ikelos

This isn't passing some of the tests at the moment.

Also I really uncertain as to the effects of inheriting from a plugin that doesn't live under an os directory. I'd really strongly advise that we only proxy methods from the class, that will allow the methods to stay shared, with minimal duplicated code and ensures we don't have to deal with any shared complexity (like a plugin inheritting from itself before it knows whether its own version passes the requirements. I can't see a compelling reason to use inheritance for the sake of run = ModuleDisplayPlugin.run (although we'll need to think about binding of methods to make sure that works properly)...

ikelos · 2025-03-17T21:19:06Z

volatility3/framework/symbols/linux/utilities/modules.py

        return reqs
+
+
+class ModuleDisplayPlugin(plugins.PluginInterface):


This seems like the wrong place for it, and inheriting from the plugin interface means it may get listed as main plugin (our code to list plugins looks for anything that inherits from plugin). Are we sure that's what we want? I'd much prefer any plugins we have to live as a plugin in the right directory.

ikelos · 2025-03-17T21:22:18Z

volatility3/framework/plugins/linux/hidden_modules.py

-                name="linux_utilities_modules",
-                component=linux_utilities_modules.Modules,
-                version=(3, 0, 0),
+                name="linux_utilities_modules_module_display_plugin",


It would surely be better to define classmethod generator and run methods and then just proxy them at this point? We're really not getting anything from inheritance other than complexity?

Can __init__ of each plugin just set .run and and ._generator to the shared function? and Where would that shared functons best live now? I was trying to avoid one plugin becoming the container (say lsmod) that the other plugins then need to import and depend on.

Yeah, that should be ok, but you can just define it straight in the class I think? So

class Blah: run = linux_utilities_modules.ModuleDisplayPlugin.run

?

atcuno · 2025-03-17T21:37:48Z

Yes, the 1 test would break until I rebase with the netfilter PR you just merged. That is a really good point on inheriting to look like a plugin + re-using .run. I will adjust to that.

…rnel modules. Convert plugins to new method.

volatility3/framework/plugins/linux/check_modules.py

volatility3/framework/plugins/linux/hidden_modules.py

volatility3/framework/plugins/linux/lsmod.py

volatility3/framework/symbols/linux/utilities/modules.py

ikelos

Looking much better, but there's nothing to be gained by the multiple inheritance. Literally just reach into the classes and assign the methods to the new classes. It should all work (because self is included explicitly, so it'll refer to the instantiated class, not the ModuleDisplayPlugin class, which likely will never get instantiated ever. It's just acting as a code wrapper to keep it all nice and warm and bundled up for the plugins that actually want to use it... 5:)

volatility3/framework/plugins/linux/check_modules.py

ikelos · 2025-03-18T00:09:32Z

volatility3/framework/plugins/linux/check_modules.py

    _required_framework_version = (2, 0, 0)

+    def __init__(self, *args, **kwargs):
+        super().__init__(self.compare_kset_and_lsmod, *args, **kwargs)


I mean this is nice, but not worth it, just set a self variable in here, and pull it back in the proxied method.

ikelos · 2025-03-18T00:11:08Z

volatility3/framework/plugins/linux/hidden_modules.py

-                name="linux_utilities_modules",
-                component=linux_utilities_modules.Modules,
-                version=(3, 0, 0),
+                name="linux_utilities_modules_module_display_plugin",


Yeah, that should be ok, but you can just define it straight in the class I think? So

class Blah: run = linux_utilities_modules.ModuleDisplayPlugin.run

?

ikelos · 2025-03-18T00:13:23Z

volatility3/framework/symbols/linux/utilities/modules.py

+        self.implementation = implementation
+
+    @classmethod
+    def get_requirements(cls) -> List[interfaces.configuration.RequirementInterface]:


Add the results of this to the individual class get_requirements.

ikelos · 2025-03-18T00:13:27Z

volatility3/framework/symbols/linux/utilities/modules.py

+    The constructor of the plugin must call super() with the `implementation` set
+    """
+
+    _version = (1, 0, 0)


Don't need these local variables, since they shouldn't be being inherited anyway (you can if you really want, but the plugins should be defining their own needs

Sorry, I realized we version this directly, so it's important for this to have its own version! 5:S

ikelos · 2025-03-18T00:14:42Z

volatility3/framework/symbols/linux/utilities/modules.py

+            ),
+        ]
+
+    def _generator(self):


This might give a ruff warning, because it's a private member, but you can actually make it public and just assign it to a private member in the new class?

_generator = modules.ModuleDisplayPlugin.generator

for example?

atcuno · 2025-03-18T01:15:48Z

Nice idea! I think its sorted now. Also, if you see in the diff functions moving, its so they could be referenced from the code under them.

ikelos

Looks good, let's get it landed 5:)

ikelos · 2025-05-08T11:41:24Z

So I said:

I'd really strongly advise that we only proxy methods from the class, that will allow the methods to stay shared, with minimal duplicated code and ensures we don't have to deal with any shared complexity.

Looking much better, but there's nothing to be gained by the multiple inheritance.

To which you eventually said:

I think its sorted now.

And I then merged it in, seemingly because I was getting tired of reworking it and because it was an overly large pull request. I'm sorry, I shouldn't have accepted it, I don't think I thought this through full before landing it. You sounded as though you'd fixed the inheritance that I objected to twice, but there's still inheritance being used and is now causing some headaches in a circular dependency because this extension module is actually partial plugins rather than discrete code blocks each doing their own thing (see #1773).

Please can we can this refactored ASAP. No inheritance please. None. Only code blocks and version checks...

ikelos · 2025-05-08T11:52:58Z

Ok, having calmed a bit (but only a bit) and scoped out exactly what's going on, it seems we jury rigged inheritance. It's not as bad as I thought, but trying to pull in complex functions (like run and generator) removes the ability for plugin to easily be able to override settings (or for us to separate concerns when, for example, part of the dumping functionality needs to be refactored). I'm working up a PR that should improve this, but I suspect that the ModuleDisplayPlugin will not exist in its current form for long.

ikelos · 2025-05-08T22:07:17Z

Ok, I've worked up #1801 to get this slightly more in order.

atcuno added the parity-release label Mar 17, 2025

atcuno requested a review from ikelos March 17, 2025 17:30

ikelos reviewed Mar 17, 2025

View reviewed changes

atcuno force-pushed the linux_unifiy_module_gathering_output branch from f2e10da to 1096217 Compare March 17, 2025 23:42

atcuno added 3 commits March 17, 2025 18:56

Create versioned parent class for all plugins that enumerate Linux ke…

7146b45

…rnel modules. Convert plugins to new method.

bump version on kallsyms

971f069

change lmsod call

f906bde

atcuno force-pushed the linux_unifiy_module_gathering_output branch from 1096217 to f906bde Compare March 17, 2025 23:57

atcuno added 2 commits March 17, 2025 18:58

remove errant space

00c4a13

Change how the inheritance is performed

93be148

github-advanced-security bot found potential problems Mar 18, 2025

View reviewed changes

Removed unused import

0667a40

ikelos reviewed Mar 18, 2025

View reviewed changes

Update for new accessing method

06a4c56

ikelos approved these changes Mar 18, 2025

View reviewed changes

ikelos merged commit bd5fb7d into develop Mar 18, 2025
24 checks passed

ikelos deleted the linux_unifiy_module_gathering_output branch March 18, 2025 08:18

ikelos assigned atcuno and unassigned atcuno May 8, 2025

		return reqs


		class ModuleDisplayPlugin(plugins.PluginInterface):

Create versioned parent class for all plugins that enumerate Linux ke… #1719

Create versioned parent class for all plugins that enumerate Linux ke… #1719

Uh oh!

Conversation

atcuno commented Mar 17, 2025

Uh oh!

atcuno commented Mar 17, 2025

Uh oh!

ikelos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atcuno commented Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ikelos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ikelos Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atcuno commented Mar 18, 2025

Uh oh!

ikelos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ikelos commented May 8, 2025

Uh oh!

ikelos commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ikelos commented May 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

atcuno commented Mar 17, 2025 •

edited

Loading

ikelos Mar 18, 2025 •

edited

Loading

ikelos commented May 8, 2025 •

edited

Loading