Add mask command and modprobe test with blacklist #312

howardthuang · 2025-03-08T01:58:48Z

Closes: #40

libkmod: add mask command

Co-authored-by: Jakub Ślepecki jakub.slepecki@intel.com
Signed-off-by: Dawid Osuchowski dawid.osuchowski@intel.com
Signed-off-by: Jakub Ślepecki jakub.slepecki@intel.com

testsuite: test-modprobe: modprobe with blacklist

Co-authored-by: Dawid Osuchowski dawid.osuchowski@intel.com
Signed-off-by: Jakub Ślepecki jakub.slepecki@intel.com
Signed-off-by: Dawid Osuchowski dawid.osuchowski@intel.com

whythefyouflying · 2025-03-08T07:03:45Z

PR Title:
Introduce mask command to prevent modules from loading

PR Description:
The behavior of the new mask command could easily be replaced by modprobe -b in downstream, hence the first commit. Otherwise this allows user to prevent loading selected modules without changing any already established behaviour.

jspcki · 2025-03-11T09:05:12Z

To add more context to our initial description. In PR there are three options to prevent module from loading:

install (used with modprobe alias_or_module)
blacklist (used with modprobe alias and modprobe -b module)
mask (used with modprobe alias_or_module)

Install still provides the best flexibility: user may specify either /bin/false or /bin/true (and a lot more complex options) to get different resulting behaviour out of modprobe.

Blacklist puts the most burden on the user. If they choose to disable module with it (in contrast to just limiting the alias resolution), they need to make sure their scripts use either aliases or -b.

Mask intends to provide a strict (in contrast to install) and clear intent-based (in contrast to both blacklist and install) option that
does not change any currently established behaviour. This way we also avoid the problem of deprecation of install.

whythefyouflying · 2025-03-13T09:54:44Z

The latest force-push was to address the formatting issue reported by CI: https://github.com/kmod-project/kmod/actions/runs/13732774455/job/38592878799?pr=312

evelikov

Overall I'm loving how you went above and beyond on the prep (blacklist tests) patch. There's one serious note, but the rest are cosmetic suggestions.

Thanks team o/

libkmod/docs/libkmod-docs.xml

evelikov · 2025-03-17T17:55:16Z

libkmod/libkmod-config.c

-	if (streq(modname, "modprobe") && !strncmp(param, "blacklist=", 10)) {
+	if (streq(modname, "modprobe")) {
+		bool is_mask = !strncmp(param, "mask=", 5);
+		bool is_blacklist = !strncmp(param, "blacklist=", 10);


Note to self: replace strncmp with strstartswith tree-wide.

evelikov · 2025-03-17T17:57:43Z

libkmod/libkmod-config.c

@@ -615,13 +641,18 @@ static void kcmdline_parse_result(struct kmod_config *config, char *modname, cha

 	DBG(config->ctx, "%s %s\n", modname, param);

-	if (streq(modname, "modprobe") && !strncmp(param, "blacklist=", 10)) {
+	if (streq(modname, "modprobe")) {


We seems to be changing the logic here, instead of extending it. We should be using something like below, right?

if (streq(modname, "modprobe") && (!strncmp(param, "blacklist=", 10) || !strncmp(param, "mask=", 5)) {

Agreed, will change

evelikov · 2025-03-17T18:03:25Z

libkmod/libkmod-config.c

+			if (underscores(modname) < 0)
+				goto syntax_error;
+
+			kmod_config_add_mask(config, modname);


Nit: let's move the mask hunks just above the remove, across the patch.

Optional bonus points - alpha sort the rest (across the tree) with a follow-up commit.

Will do across the patch

Jakub is willing to add to that and sort across the tree

This one is done. I don't think we adjusted it across the tree, @whythefyouflying?

This one is done. I don't think we adjusted it across the tree, @whythefyouflying?

It was not. I believe that would best be suited in a separate series/PR.

evelikov · 2025-03-17T18:17:57Z

testsuite/test-modprobe.c

+	},
+	.modules_loaded = "",
+	.modules_not_loaded = "mod-simple",
+	);


Should we test that mask=dependency with modprobe dependee fails?

Hmm I guess the behaviour will vary based on the dependency type AFAICT:

(hard) dependency - failure

softdep - success

weakdep - not sure, need to check

Currently we always return success, which I'm not sure is the best. Need to do some testing over the weekend...

Agreed, I'd say:

dependency - module does not load, modprobe status is non-zero

softdep - target module loads without softdep(s), status is zero

weakdep - target module loads without weakdep(s), status is zero

I added such tests in local branch and they will be included in v3. I settled on weakdep behaviour because of its description in manual pages "Those are similar to pre softdep" and "userspace doesn't attempt to load that dependency before the specified module."

This is not what's happening right now, so we'll fix it.

Changed in a3780cc: hard and soft dependencies now behave like above. Weakdep has no effect at all on modprobe so it's ignored, but since it would fall into not required category it would behave like softdep.

whythefyouflying · 2025-03-18T09:29:07Z

tools/modprobe.c

@@ -163,6 +163,7 @@ static int show_config(struct kmod_ctx *ctx)
 		const char *name;
 		struct kmod_config_iter *(*get_iter)(const struct kmod_ctx *ctx);
 	} ci[] = {
+		{ "mask", kmod_config_get_masks },


@evelikov What would be your guidance on the placement of this line? I agree with alpha sorting in the rest of the patch, but in here I feel like mask list should be printed first, given it is more restrictive than blacklist and will most likely not be a huge list as well.

I'm not sure I follow how that makes a difference. People/tools already need to parse the whole output, regardless of how long/short respective sections are.

Mind you, the suggestion is a bit bike-sheddy so don't take it too seriously.

This one is not done.

I'm not sure I follow how that makes a difference. People/tools already need to parse the whole output, regardless of how long/short respective sections are.

Mind you, the suggestion is a bit bike-sheddy so don't take it too seriously.

Alpha sorted everything across the patch except this line.

I believe from the human user perspective, given mask is the most restrictive option, it would be good UI to have it displayed first. If we want to alpha sort everything in show_config() then we should also print alias first and that is not the case.

danielzgtg · 2025-03-25T03:32:12Z

I was initially confused at kmod_config_get_masks until I saw the naming was based off of kmod_config_get_blacklists. Usually "masks" means "image/bit- masks" not "masked module". It is too bad that the symbol is publicly exported from libkmod.so, otherwise I'd suggest renaming both.

user may specify either /bin/false or /bin/true

@jspcki Putting /bin/true is the same as putting nothing.

danielzgtg · 2025-03-25T03:38:54Z

libkmod/libkmod-module.c

@@ -1020,6 +1037,9 @@ KMOD_EXPORT int kmod_module_probe_insert_module(
 			return 0;
 	}

+	if (module_is_masked(mod))
+		return 0;


Loading a masked module should not return success.

Compare systemd:

root@daniel-desktop3:~# systemctl start badservice Failed to start badservice.service: Unit badservice.service is masked. root@daniel-desktop3:~# echo $? 1 root@daniel-desktop3:~#

This was mentioned just above, with some potential caveats wrt the different dependency types.

If you can share your expectations for each use-case, that would be appreciated.

Changed in a3780cc, see full description above.

jspcki · 2025-04-27T10:48:32Z

Putting /bin/true is the same as putting nothing.

Incorrect. install will use provided executable "in place of insmod". Both true and false do not have any side-effects of installing any modules in kernel, the only difference is that true will return a zero error code and false will not, causing it being treated as an error.

jspcki · 2025-04-27T10:52:08Z

libkmod/libkmod-module.c

+			if (mod->required) {
+				ERR(mod->ctx, "module is masked: %s\n",
+				    kmod_module_get_name(mod));
+				err = -EINVAL;


I'd like to hear opinions/suggestions on EINVAL in this case and in 1080. I thought about using custom error, but I disliked the part that it "only so happened to" not collide with errno. Otherwise we could return positive error (even though it's not a direct result of flags per kmod_module_probe_insert_module description) and handle it in modprobe.c.

I'd prefer standard errno codes - don't have a strong opinion either way though.

jspcki · 2025-04-27T10:53:23Z

libkmod/libkmod-module.c

@@ -759,9 +777,21 @@ KMOD_EXPORT int kmod_module_apply_filter(const struct kmod_ctx *ctx,
 		if ((filter_type & KMOD_FILTER_BUILTIN) && kmod_module_is_builtin(mod))
 			continue;

+		if ((filter_type & KMOD_FILTER_MASK) && module_is_masked(mod)) {
+			if (mod->required) {
+				ERR(mod->ctx, "module is masked: %s\n",


We could s/module/dependency/ if it feels more right. It will never be the module itself because of earlier check in 1074 unless used in different conditions.

Module is fine IMHO. It's A module, maybe not THE module the user (API or otherwise) is directly loading, but that's fine.

evelikov · 2025-05-11T18:40:33Z

Thanks for the updates team. Been quite busy with IRL, but I'll try to find some time this week to review the new version.

evelikov

Left a few minor comments - with or without them the PR looks good. Thumbs up from me.

Would be interesting to hear Lucas' POV wrt the EINVAL et al.

evelikov · 2025-05-18T22:18:27Z

libkmod/libkmod-module.c

@@ -759,9 +777,21 @@ KMOD_EXPORT int kmod_module_apply_filter(const struct kmod_ctx *ctx,
 		if ((filter_type & KMOD_FILTER_BUILTIN) && kmod_module_is_builtin(mod))
 			continue;

+		if ((filter_type & KMOD_FILTER_MASK) && module_is_masked(mod)) {
+			if (mod->required) {
+				ERR(mod->ctx, "module is masked: %s\n",


Module is fine IMHO. It's A module, maybe not THE module the user (API or otherwise) is directly loading, but that's fine.

evelikov · 2025-05-18T22:18:53Z

libkmod/libkmod-module.c

+			if (mod->required) {
+				ERR(mod->ctx, "module is masked: %s\n",
+				    kmod_module_get_name(mod));
+				err = -EINVAL;


I'd prefer standard errno codes - don't have a strong opinion either way though.

evelikov · 2025-05-18T22:20:12Z

libkmod/libkmod-module.c

+				err = -EINVAL;
+				goto fail;
+			} else
+				continue;


Style nit:

if !required continue ERR(...) err = ...; goto fail;

evelikov · 2025-05-18T22:27:11Z

libkmod/libkmod-module.c

 		if (err < 0)
 			return err;

-		kmod_module_unref_list(list);


Nicely spotted. Let's split this one-line corner-case memory leak fix in its* own commit.

Out of curiosity (aka non-blocking question): What is the command which produces the Sanitizer leak report? Does one need to modify code or it happens out of the box?

I had ASAN complaining primarily at 1072 when we had a similar sequence of statements: filter -> return -> unref. You'd need to hit any of the failing conditions of the filter: (ctx == NULL || output == NULL), mod->required, or kmod_list_append(*output, mod) == NULL. Other than required they are unlikely. Right now easiest way would be to inject error there manually and it's the way I generated sanitizer output for the commit.

Until now blacklist command was only covered by test-blacklist at libkmod interface level. A regular user will usually interact with it via modprobe(8). This commit adds tests for modprobe, alias filtering, and -b option. It is also intended as an example of -b usage. Co-authored-by: Dawid Osuchowski <dawid.osuchowski@linux.intel.com> Signed-off-by: Dawid Osuchowski <dawid.osuchowski@linux.intel.com> Signed-off-by: Jakub Ślepecki <jakub.slepecki@intel.com>

In case of error path this might have led to, for example: ==86629==ERROR: LeakSanitizer: detected memory leaks Direct leak of 24 byte(s) in 1 object(s) allocated from: #0 0x7fa904b20e15 (/usr/lib/gcc/x86_64-pc-linux-gnu/15.1.1/../../../../lib/libasan.so+0x120e15) (BuildId: 9dccfc42ea2d9790ff7cf76cc8a3210650e604e0) #1 0x55d74820fc8a ($kmod/build/kmod+0xefc8a) (BuildId: e7c136c95271cf020398e33fd5577d82adc7a47c) kmod-project#2 0x55d7482173a6 ($kmod/build/kmod+0xf73a6) (BuildId: e7c136c95271cf020398e33fd5577d82adc7a47c) kmod-project#3 0x55d74821a617 ($kmod/build/kmod+0xfa617) (BuildId: e7c136c95271cf020398e33fd5577d82adc7a47c) kmod-project#4 0x55d7481e2d80 ($kmod/build/kmod+0xc2d80) (BuildId: e7c136c95271cf020398e33fd5577d82adc7a47c) kmod-project#5 0x55d7481e314f ($kmod/build/kmod+0xc314f) (BuildId: e7c136c95271cf020398e33fd5577d82adc7a47c) kmod-project#6 0x55d7481e658c ($kmod/build/kmod+0xc658c) (BuildId: e7c136c95271cf020398e33fd5577d82adc7a47c) kmod-project#7 0x7fa9038376b4 (/usr/lib/libc.so.6+0x276b4) (BuildId: 468e3585c794491a48ea75fceb9e4d6b1464fc35) kmod-project#8 0x7fa903837768 (/usr/lib/libc.so.6+0x27768) (BuildId: 468e3585c794491a48ea75fceb9e4d6b1464fc35) kmod-project#9 0x55d7481c3334 ($kmod/build/kmod+0xa3334) (BuildId: e7c136c95271cf020398e33fd5577d82adc7a47c) Indirect leak of 4120 byte(s) in 1 object(s) allocated from: #0 0x7fa904b2073d (/usr/lib/gcc/x86_64-pc-linux-gnu/15.1.1/../../../../lib/libasan.so+0x12073d) (BuildId: 9dccfc42ea2d9790ff7cf76cc8a3210650e604e0) #1 0x55d7481eb8eb ($kmod/build/kmod+0xcb8eb) (BuildId: e7c136c95271cf020398e33fd5577d82adc7a47c) kmod-project#2 0x55d7481f6fee ($kmod/build/kmod+0xd6fee) (BuildId: e7c136c95271cf020398e33fd5577d82adc7a47c) kmod-project#3 0x55d7481e52c4 ($kmod/build/kmod+0xc52c4) (BuildId: e7c136c95271cf020398e33fd5577d82adc7a47c) kmod-project#4 0x7fa9038376b4 (/usr/lib/libc.so.6+0x276b4) (BuildId: 468e3585c794491a48ea75fceb9e4d6b1464fc35) kmod-project#5 0x7fa903837768 (/usr/lib/libc.so.6+0x27768) (BuildId: 468e3585c794491a48ea75fceb9e4d6b1464fc35) kmod-project#6 0x55d7481c3334 ($kmod/build/kmod+0xa3334) (BuildId: e7c136c95271cf020398e33fd5577d82adc7a47c) [...] SUMMARY: AddressSanitizer: 5701 byte(s) leaked in 27 allocation(s). Co-authored-by: Dawid Osuchowski <dawid.osuchowski@linux.intel.com> Signed-off-by: Dawid Osuchowski <dawid.osuchowski@linux.intel.com> Signed-off-by: Jakub Ślepecki <jakub.slepecki@intel.com>

Mask command prevents modules from loading. It intends to be a strict and intent-based replacement for variations of install and blacklist commands, for example: * install module /bin/false * install module /bin/true * blacklist module, with modprobe -b This commit is a pure extension and the already established behaviour is not expected to change as a result. Co-authored-by: Jakub Ślepecki <jakub.slepecki@intel.com> Signed-off-by: Jakub Ślepecki <jakub.slepecki@intel.com> Signed-off-by: Dawid Osuchowski <dawid.osuchowski@linux.intel.com>

whythefyouflying · 2025-06-27T14:35:51Z

We have rebased the code to latest master and addressed review concerns

whythefyouflying · 2025-07-14T11:44:55Z

@lucasdemarchi Can you please take a look?

whythefyouflying force-pushed the issue-40-mask branch from 5b5226c to abb3e2a Compare March 12, 2025 13:01

evelikov requested changes Mar 17, 2025

View reviewed changes

whythefyouflying reviewed Mar 18, 2025

View reviewed changes

danielzgtg reviewed Mar 25, 2025

View reviewed changes

whythefyouflying force-pushed the issue-40-mask branch from abb3e2a to a3780cc Compare April 26, 2025 19:29

jspcki reviewed Apr 27, 2025

View reviewed changes

howardthuang requested a review from evelikov May 8, 2025 22:36

evelikov approved these changes May 18, 2025

View reviewed changes

jspcki and others added 3 commits June 27, 2025 16:27

whythefyouflying force-pushed the issue-40-mask branch from a3780cc to d8d5d51 Compare June 27, 2025 14:32

howardthuang requested a review from evelikov June 30, 2025 06:23

evelikov approved these changes Jul 5, 2025

View reviewed changes

Add mask command and modprobe test with blacklist #312

Are you sure you want to change the base?

Add mask command and modprobe test with blacklist #312

Uh oh!

Conversation

howardthuang commented Mar 8, 2025

Uh oh!

whythefyouflying commented Mar 8, 2025

Uh oh!

jspcki commented Mar 11, 2025

Uh oh!

whythefyouflying commented Mar 13, 2025

Uh oh!

evelikov 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!

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!

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!

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!

danielzgtg commented Mar 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielzgtg Mar 25, 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!

jspcki commented Apr 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

evelikov commented May 11, 2025

Uh oh!

evelikov 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

danielzgtg Mar 25, 2025 •

edited

Loading

jspcki commented Apr 27, 2025 •

edited

Loading

evelikov May 18, 2025 •

edited

Loading

jspcki Jul 7, 2025 •

edited

Loading