kmodloader: add general support for built-in modules and modinfo

[guidosarducci]

Maintainer: @blogic

Description

This series fixes some long-standing pain/annoyance points with OpenWrt around inconsistent handling between loadable and built-in modules. This is in contrast to mainstream distros, where modprobe, modinfo, rmmod, etc, are aware of built-in module metadata e.g. modprobe -q <module> can be used to test the presence of a built-in or loadable module equally. Current kmodloader also ignores parameter details already encoded in OpenWrt kernel modules.

The main changes made in the series are:

1. Teach print_modinfo() to understand and show parameters included in current modules' .modinfo sections.
2. Add scan_builtin_modules() to parse modules.builtin file if present, and use it for consistent presence-testing with modprobe or improved logging e.g. rmmod <module-builtin>.
3. Update code to parse modules.builtin.modinfo file in get_module_info(), scan_builtin_modules() and print_modinfo(). This allows usage of aliases for builtin modules, and enables modinfo to print basic details of built-in modules.
4. Clean up and factor out some duplicated mmap() related code.
5. Fix a small memory leak introduced in 4c7b720.

Related

A separate OpenWrt PR includes files modules.builtin and modules.builtin.modinfo into the kernel package for use by kmodloader (openwrt/openwrt#14218) and has since been merged. These PRs are complementary but independent since kmodloader will work with/without the files above.

Testing

• Compile and run-tested on both Ubuntu (Ubuntu 22.04 LTS) and Openwrt (master, malta/mipseb32)
• Verified behaviour of modprobe, modinfo, rmmod against built-in and loadable modules, with and without varying types of parameters.
• Compared behaviour, output and error messages between distros above.
• Confirmed kmodloader continues to function without modules.builtin and modules.builtin.modinfo.
• Examples of usage and changed behaviour are included in commit messages.
• Checked size impact of changes: updates to kmodloader, with inclusion of modules.builtin and modules.builtin.modinfo, increased OpenWrt compressed filesystem image size by ~2.5 KB.

Review

CC previous contributors: @nbd168 @hauke @Ansuel @neheb @yousong
Thanks in advance for your feedback...

[guidosarducci]

@Ansuel @nbd168 @hauke For clarity, and because this is PR #1, could someone confirm this repo is valid for PR submissions? Thanks!

[Ansuel]

Yes it's ok we will merge on the git server anyway just as we do for openwrt main tepo

[guidosarducci]

Simplified handling state of built-in modules.

[guidosarducci]

@Ansuel @nbd168 @hauke or @blogic If you have some cycles I'd kindly request any review or comments you might have on this PR as well as the related one. All the best in the new year... 🎉

[guidosarducci]

Pinging @Ansuel @nbd168 @hauke or @blogic again to request review or comment. The related openwrt/openwrt#14218 has already been merged, and I regularly test this successfully against the latest openwrt master. Thanks in advance.

[guidosarducci]

Fixed a small, existing memory leak whose code I had copied.

[guidosarducci]

@Ansuel Many thanks for taking the time to review!

[ynezz]

Hi,

Please find the latest report on new defect(s) introduced to OpenWrt found with Coverity Scan.

5 new defect(s) introduced to OpenWrt found with Coverity Scan.

New defect(s) Reported-by: Coverity Scan
Showing 5 of 5 defect(s)

---

CID 1586645: Security best practices violations (TOCTOU)

/build_dir/target-x86_64-openwrt-linux-musl_musl/ubox-2024-01-15-c006dcce/kmodloader.c: 505 in scan_builtin_modules()

499     	int rv = -1;
500     
501     	if (!module_folders && init_module_folders())
502     		goto err;
503     	for (p = module_folders; *p; p++) {
504     		snprintf(path, sizeof(path), "%s%s", *p, MOD_BUILTIN);
>>>     CID 1586645:  Security best practices violations  (TOCTOU)
>>>     Calling function "stat" to perform check on "path".
505     		if (!stat(path, &st) && S_ISREG(st.st_mode)) {
506     			fp = fopen(path, "r");
507     			if (fp)
508     				break;
509     		}
510     	}

---

CID 1586644: Resource leaks (RESOURCE_LEAK)

/build_dir/target-x86_64-openwrt-linux-musl_musl/ubox-2024-01-15-c006dcce/kmodloader.c: 653 in print_modinfo()

647     				printf("%s:\t\t%s\n",  dup, sep);
648     			else
649     				printf("%s:\t%s\n",  dup, sep);
650     		} else {
651     			sep2 = strstr(sep, ":");
652     			if (!sep2)
>>>     CID 1586644:  Resource leaks  (RESOURCE_LEAK)
>>>     Variable "dup" going out of scope leaks the storage it points to.
653     				break;
654     			pname = strndup(sep, sep2 - sep);
655     			sep2++;
656     			pdata = strdup(sep2);
657     
658     			list_for_each_entry(p, &params, list)

---

CID 1586643: Security best practices violations (STRING_OVERFLOW)

/build_dir/target-x86_64-openwrt-linux-musl_musl/fstools-2024-01-15-325d63d6/libfstools/fit.c: 82 in fit_volume_find()

76     		return NULL;
77     
78     	p = calloc(1, sizeof(struct fit_volume));
79     	if (!p)
80     		return NULL;
81     
>>>     CID 1586643:  Security best practices violations  (STRING_OVERFLOW)
>>>     You might overrun the 16-character fixed-size string "p->dev.devpathstr" by copying "fname" without checking the length.
82     	strcpy(p->dev.devpathstr, fname);
83     	p->v.drv = &fit_driver;
84     	p->v.blk = p->dev.devpathstr;
85     	p->v.name = name;
86     
87     	return &p->v;

---

CID 1586642: Null pointer dereferences (FORWARD_NULL)

/build_dir/target-x86_64-openwrt-linux-musl_musl/ubox-2024-01-15-c006dcce/kmodloader.c: 362 in scan_loaded_modules()

356     		n->usage = m.usage;
357     		n->state = LOADED;
358     	}
359     	rv = 0;
360     out:
361     	free(buf);
>>>     CID 1586642:  Null pointer dereferences  (FORWARD_NULL)
>>>     Passing null pointer "fp" to "fclose", which dereferences it.
362     	fclose(fp);
363     
364     	return rv;
365     }
366     
367     static char *mmap_modinfo(const char *module, const char *name, struct stat *s, unsigned int *offset, unsigned int *size)

---

CID 1586641: (FORWARD_NULL)

/build_dir/target-x86_64-openwrt-linux-musl_musl/ubox-2024-01-15-c006dcce/kmodloader.c: 536 in scan_builtin_modules()

530     		}
531     	}
532     out:
533     	rv = 0;
534     err:
535     	free(buf);
>>>     CID 1586641:    (FORWARD_NULL)
>>>     Passing null pointer "fp" to "fclose", which dereferences it.
536     	fclose(fp);
537     
538     	return rv;
539     }
540     
541     static int scan_module_folder(const char *dir)
/build_dir/target-x86_64-openwrt-linux-musl_musl/ubox-2024-01-15-c006dcce/kmodloader.c: 536 in scan_builtin_modules()
530     		}
531     	}
532     out:
533     	rv = 0;
534     err:
535     	free(buf);
>>>     CID 1586641:    (FORWARD_NULL)
>>>     Passing null pointer "fp" to "fclose", which dereferences it.
536     	fclose(fp);
537     
538     	return rv;
539     }
540     
541     static int scan_module_folder(const char *dir)

[Ansuel]

@ynezz took care of fix these new reported bug some of them were false positive but still a chance for better quality code in general.

[guidosarducci]

@Ansuel These look good to me. I made some patches but then needed to deal with a small HVAC emergency. Thanks for tackling this!
@ynezz BTW, is there an easy way to re-run a Coverity scan on PRs before merging?

[ynezz]

BTW, is there an easy way to re-run a Coverity scan on PRs before merging?

From https://scan.coverity.com/faq#frequency

What is the frequency for build submissions to Coverity Scan?

The number of weekly builds per project are as follows:

• Up to 28 builds per week, with a maximum of 4 builds per day, for projects with fewer than 100K lines of code
• Up to 21 builds per week, with a maximum of 3 builds per day, for projects with 100K to 500K lines of code
• Up to 14 builds per week, with a maximum of 2 build per day, for projects with 500K to 1 million lines of code
• Up to 7 builds per week, with a maximum of 1 build per day, for projects with more than 1 million lines of code

IMO it would be possible, but we would need to rework the current setup:

* create separate Coverity project for every (or selected?) such C core OpenWrt component
* make a CI action triggered on label coverity-check (optional)
* make a CI action triggered on main branch push

Brain fart, it would be possible, but quite a lot of work, because the feedback from the Coverity build is provided via email, thus quite cumbersome to automate.

We could maybe consider increasing the build frequency to two or three times a week, to have faster feedback about the changes?

[guidosarducci]

We could maybe consider increasing the build frequency to two or three times a week, to have faster feedback about the changes?

Yes, thanks, I think that would be a reasonable improvement.

[ynezz]

Yes, thanks, I think that would be a reasonable improvement.

Relevant code https://github.com/openwrt/openwrt/blob/main/.github/workflows/coverity.yml#L5 and docs

[Ansuel]

@ynezz in theory we can keep coverity and try to use other tool for the single project for more precise testing?

[guidosarducci]

@Ansuel FWIW, I've seen multiple build failures of ubox over the last few months related to some some of its build dependencies (e.g. libubox, udebug, etc.) At the same time, I know these all build fine on Ubuntu LTS from my own build/testing.

I believe it would very useful to have a separate CI for "out-of-openwrt-tree repos" that builds various top-level tools (like ubox) and their dependencies in an Ubuntu container when PRs are posted. Is that something you think feasible?

Custom run-testing post-build could also be an option (e.g. valgrind for ubox), but just a basic build would be a good start.

[Ansuel]

I started doing something like that for libubox... Main problem is adding or creating some kind of infra to simulate the data from a running system. Maybe qemu can be used but I would love to skip using real world image in virtual env.

[guidosarducci]

I started doing something like that for libubox...

Ah, good to know. How is that working so far? One advantage of building a "top-level" utility like ubox is it can pull in several build dependencies and broaden coverage. Perhaps only a couple of top-level tool builds might pull in everything?

Main problem is adding or creating some kind of infra to simulate the data from a running system.

What sort of data exactly? Things like replay traces of network traffic or remote user input? I see too many things get broken often because most testing is limited to "it compiled" at best, rather than exercising a system from OS boot-up to application testing/sanity-check.

Maybe qemu can be used but I would love to skip using real world image in virtual env.

I had suggested doing QEMU-based testing a few years ago but had zero response/interest from members. People seemed more focused assembling small router-farms to verify the hardware specifics.

I much prefer using QEMU to run pure virtual targets with minimal HW dependencies (i.e. stable QEMU machine type) but full coverage of basic OS and userspace: malta and armvirt have been perfect for me in this way. For repeatable testing, I've looked mostly at upstream kernel selftests and tried to package what I needed: e.g. for BPF I made kmod-bpf-test and kselftests-bpf. But kselftests have a ton of test coverage applicable to OpenWrt: netfilter, general network, tc filtering, etc.

Would be great if this sort of thing was something you're interesting in exploring.

(Note: armvirt was butchered/replaced by the armsr monster but I would love to see it restored for its simplicity and usefulness, from easy userspace testing to straightforward kernel major updates. I basically used OpenWrt malta/armvirt for a bunch of upstream BPF development, including a BPF JIT I submitted.)

[Ansuel]

Ah, good to know. How is that working so far? One advantage of building a "top-level" utility like ubox is it can pull in several build dependencies and broaden coverage. Perhaps only a couple of top-level tool builds might pull in everything?

As you can see https://github.com/openwrt/libubox/actions currently building everything on alpine to reduce the dependency to the min possible.

What sort of data exactly? Things like replay traces of network traffic or remote user input? I see too many things get broken often because most testing is limited to "it compiled" at best, rather than exercising a system from OS boot-up to application testing/sanity-check.

I would love to first have something that can be quickly tested and then later implement full test on a vm image with some scripts. But that was an idea. The ideal test would be fuzz test to catch all kind of strange case instead of creating simple test to make sure basic usage works... (we would catch that kind of regression already as people would scream that everything is bricked)

I had suggested doing QEMU-based testing a few years ago but had zero response/interest from members. People seemed more focused assembling small router-farms to verify the hardware specifics.

We have something like that for feeds repository but we didn't introduce that on main repository as we really need to compose a list of what to test...

What would be some test you would do with this package?

[ynezz]

I would love to first have something that can be quickly tested

Yes, we all would like to have that, but for that you would need to rewrite/refactor quite a lot of code, because its not written with unit testing/mocking/fuzzing in mind. So ideally first have a complete runtime testing pipeline using full images on QEMU/real devices, to capture/QA the current state and then you can actually start doing refactoring so you can be sure, that nothing broke during that refactoring. Yes, a lot of work ahead.

The ideal test would be fuzz test to catch all kind of strange case instead of creating simple test to make sure basic usage works... (we would catch that kind of regression already as people would scream that everything is bricked)

Indeed https://github.com/openwrt/libubox/commits/master/tests/fuzz

You can run those:

• locally, on developer's computer by following https://gitlab.com/ynezz/openwrt-ci/#usage-example
• on the GitLab CI using https://github.com/openwrt/libubox/blob/master/.gitlab-ci.yml
  • native testing/fuzzing job result https://gitlab.com/openwrt/project/libubox/-/jobs/6229027655
  • complete pipeline including SDK/native checks https://gitlab.com/openwrt/project/libubox/-/pipelines/1185790957
• on the GitHub CI
  • native testing/fuzzing https://github.com/ynezz/gh-actions-openwrt-ci-native
  • SDK based compile checks only https://github.com/ynezz/gh-actions-openwrt-ci-sdk

This is ~5 years old approach and needs to be revisited/refactored for the state of technology today, but it still works quite well https://github.com/jow-/ucode/actions/runs/7986801849/job/21807930294#step:3:1975

I had suggested doing QEMU-based testing a few years ago but had zero response/interest from members.

There is certainly an interest openwrt/openwrt#9723, but since QEMU is basically almost a real hardware target and thus once you move testing to real hardware targets, you (should) ideally share the underlying testing framework and tests. Most of the tests could be generic and can be run on QEMU and real targets and provide the same results.

What would be some test you would do with this package?

Every pull request

Native build and runtime testing

1. Using x86/64 host, build the package
2. Run unit and fuzzing tests

SDK build and QEMU runtime testing

for each target in x86/64, arm32, arm64, mips, riscv:

1. compile the package using SDK (ideally with sanitize=fuzzer,address,leak,undefined)
2. using IB container include this built package into image
3. boot image in QEMU
4. run set of shared runtime test suite

• check if the module can be loaded/unloaded etc.
• check if the logd process is running
• check if syslog message from logger and other user space utils reaches the log destination(s)
  • you want to make sure, that remote logging over network works
• craft some unit test for validate and run those under valgrind/sanitizers

Once merged in target branch main/openwrt-23.05

for each target in hardware target:

1. compile the package using SDK (ideally with sanitize=fuzzer,address,leak,undefined)
2. using IB container include this built package into initramfs image for that target
3. boot initramfs image on the hardware target
4. run the same set of shared runtime tests as in QEMU on the real target HW

[ynezz]

Yes, a lot of work ahead.

Just for example the amount of changes needed to fuzz dhcpv4 in odhcpd ynezz/openwrt-odhcpd@940054c, and you would actually cover just a small amount of possible inputs/paths. Which seems like a small reward, so I'm actually considering to start diving into snapshot based fuzzing (video).