modprobe: don't attempt to remove an already removed module #393

jstancek · 2025-09-30T09:07:33Z

In a scenario like following:

lsmod | grep -e bnx2i -e cnic

bnx2i 94208 0
libiscsi 94208 1 bnx2i
cnic 90112 1 bnx2i
uio 32768 1 cnic
scsi_transport_iscsi 196608 2 bnx2i,libiscsi

modprobe -v --remove --remove-holders cnic

rmmod bnx2i
rmmod cnic
rmmod libiscsi
rmmod cnic
modprobe: ERROR: libkmod/libkmod-module.c:856 kmod_module_remove_module()
could not remove 'cnic': No such file or directory

modprobe attempts to remove cnic module twice and propagates that error to the user with a message as well as an exit code.

Add a check to avoid attempts to remove modules that are already gone.

Signed-off-by: Jan Stancek jstancek@redhat.com

evelikov · 2025-10-05T11:10:27Z

Thanks for the patch Jan, at a glance the patch looks great.

Some tests would be appreciated - sadly we don't have any modprobe --remove for reference, but the infra is there. See modules_loaded and modules_not_loaded.

@lucasdemarchi the help/manual for remove-holders does not imply a recursive operation, as seen in the above log. Namely: I don't think modprobe --remove-holders cnic should be removing libiscsi. It makes sense to have the recursion, but it should be a separate --recursive toggle IMHO.

jstancek · 2025-10-05T16:51:28Z

Thanks for pointer, I'll have a look if I can add some test.

jstancek · 2025-10-10T15:56:12Z

Just a small update: I haven't found a good way so far to make the test work (at least not with modules_not_loaded. If I use rootfs with sys prepopulated to show modules as loaded and TC_DELETE_MODULE_RETCODES, the bug here can't be reproduced - rmmod shows up in logs twice, but errors are ignored. If I use a rootfs with sys linked to real /sys, the error is reproduced but modules_not_loaded reports dozens of errors because it sees system modules.

jstancek · 2025-10-11T12:24:49Z

@evelikov I've added a test that goes the way of using real /sys, with a custom check for loaded modules. What do you think about this approach?

evelikov

Left a handful of comments - would be great to have them addressed, but are non blocking.

Overall I love the idea of test_spawn_and_wait4_prog even if the name is a bit long-winded.

Off the top of my head, we could convert all test_spawn users to it, merge the two while keeping the shorter name... Although that's completely orthogonal and should be done as follow-up.

Thanks again for the fix

P.S. While I have commit access to the repo, Lucas does the merge/push to both github and kernel.org. He's busy recently so your patience is appreciated 🙇

evelikov · 2025-10-19T20:23:10Z

testsuite/meson.build

  'test-loaded',
  'test-modinfo',
  'test-modprobe',
+  'test-modprobe-remove-holders',


Nit: lets move the extra test to the existing test-modprobe.c

evelikov · 2025-10-19T20:25:16Z

testsuite/test-modprobe-remove-holders.c

@@ -0,0 +1,88 @@
+// SPDX-License-Identifier: LGPL-2.1-or-later


Missing relevant copyright blurb - either personal or employer.

evelikov · 2025-10-19T20:29:26Z

testsuite/test-modprobe-remove-holders.c

+		ERR("failed trying to remove module and holders: %d\n", ret);
+		goto cleanup;
+	}
+


AFAICT modprobe will error out if the module(s) are already loaded and similarly, if remove fails we'll also get an error.

Thus we don't need to create kmod context and/or check if the modules are loaded afterwards, right? Or am I missing something.

evelikov · 2025-10-19T20:30:01Z

testsuite/testsuite.c

+	const char *msg;
+
+	pid = fork();
+	if (pid < 0) { \


evelikov · 2025-10-19T20:32:23Z

testsuite/testsuite.c

+	}
+
+	if (pid == 0)
+		test_spawn_prog(prog, args);


Nit: return test_spawn_prog...

evelikov · 2025-10-19T20:35:10Z

testsuite/testsuite.c

+			msg = "exited with";
+			goto print_error;
+		}
+		return WEXITSTATUS(err);


Nit: here and below return err;

evelikov · 2025-10-19T20:37:19Z

testsuite/testsuite.c

+			goto print_error;
+		}
+		return WEXITSTATUS(err);
+	} else if (WIFSIGNALED(err)) {


Nit: no need for an else after return. Namely:

} if (WIFSIGNALED(err)) {

evelikov · 2025-10-19T20:39:35Z

testsuite/testsuite.c

+		return WTERMSIG(err);
+	}
+
+	return 0;


The return 0 is unreachable right? Let's remove it unless the compiler complains.

evelikov · 2025-10-19T20:40:48Z

testsuite/testsuite.c

+print_error:
+	ERR("%s %s %d\n", prog, msg, ret);
+	while (*args) {
+		LOG("  args: %s\n", *args);


Nit: let's make this a small helper function. I think we want ERR for both program and args.

evelikov · 2025-10-19T20:59:52Z

@evelikov I've added a test that goes the way of using real /sys, with a custom check for loaded modules. What do you think about this approach?

About to thinker with it myself, but for the sake of posterity - what happens if we don't have the real /sys?

Note that it's breaking the tests/CI which expect self-contained environment.

Aside: there are some trivial clang-format errors.

evelikov · 2025-10-19T21:25:59Z

Adding a dummy [TC_INIT_MODULE_RETCODES] = "", is enough to make the test pass although I'm not 100% sure it's doing the right thing.

Namely: the ld preload shims are not properly managing the relevant sysfs files:

init_module.c creates /sys/module/xxx/initstate yet lacks .../refcnt and .../holders
delete_module.c returns success if /sys/module/xxx is missing aka module is not loaded (already unloaded), completely ignores ../refcnt and .../holders.

Would be awesome to have a proper test and it depends if/how much time you have to shave this yak.

Personally, I would be fine with landing the fix alone and worrying about the rest/test at a later stage.

In a scenario like following: # lsmod | grep -e bnx2i -e cnic bnx2i 94208 0 libiscsi 94208 1 bnx2i cnic 90112 1 bnx2i uio 32768 1 cnic scsi_transport_iscsi 196608 2 bnx2i,libiscsi # modprobe -v --remove --remove-holders cnic rmmod bnx2i rmmod cnic rmmod libiscsi rmmod cnic modprobe: ERROR: libkmod/libkmod-module.c:856 kmod_module_remove_module() could not remove 'cnic': No such file or directory modprobe attempts to remove cnic module twice and propagates that error to the user with a message as well as an exit code. Add a check to avoid attempts to remove modules that are already gone. Signed-off-by: Jan Stancek <jstancek@redhat.com>

jstancek · 2025-10-20T06:49:49Z

Note that it's breaking the tests/CI which expect self-contained environment.

OK, so that approach is a no-go for CI.

Personally, I would be fine with landing the fix alone and worrying about the rest/test at a later stage.

I've gone back to just modprobe fix here, and leave tests for a separate PR. Thanks for code comments, and scheding some light on how tests work. From what I've gathered I can't load module "for real" and need test infrastructure enhanced to work with refcnt, holders, etc.

codecov · 2025-10-26T11:29:18Z

Codecov Report

❌ Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
tools/modprobe.c	0.00%	3 Missing ⚠️

Files with missing lines	Coverage Δ
tools/modprobe.c	`28.24% <0.00%> (ø)`

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

evelikov · 2025-10-26T14:41:09Z

FTR the CI failure is unrelated (Debian bullseye repos have moved) and will be resolved with #395.

jstancek force-pushed the fix_double_remove_attempt branch from d2dfc1a to c2ecd12 Compare October 11, 2025 12:19

evelikov approved these changes Oct 19, 2025

View reviewed changes

jstancek force-pushed the fix_double_remove_attempt branch from c2ecd12 to fd68288 Compare October 20, 2025 06:46

evelikov mentioned this pull request Oct 26, 2025

Emulate module loading/unloading in LD_PRELOAD libraries #394

Open

		@@ -0,0 +1,88 @@
		// SPDX-License-Identifier: LGPL-2.1-or-later

modprobe: don't attempt to remove an already removed module #393

Are you sure you want to change the base?

modprobe: don't attempt to remove an already removed module #393

Uh oh!

Conversation

jstancek commented Sep 30, 2025

lsmod | grep -e bnx2i -e cnic

modprobe -v --remove --remove-holders cnic

Uh oh!

evelikov commented Oct 5, 2025

Uh oh!

jstancek commented Oct 5, 2025

Uh oh!

jstancek commented Oct 10, 2025

Uh oh!

jstancek commented Oct 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

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!

evelikov commented Oct 19, 2025

Uh oh!

evelikov commented Oct 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jstancek commented Oct 20, 2025

Uh oh!

codecov bot commented Oct 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

evelikov commented Oct 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

evelikov commented Oct 19, 2025 •

edited

Loading

codecov bot commented Oct 26, 2025 •

edited

Loading