MODPROBE: next generation

Denys Vlasenko vda.linux at googlemail.com
Tue Jun 24 16:00:35 UTC 2008


On Monday 23 June 2008 19:02, dronnikov at gmail.com wrote:
> Hello, Denys!
> 
> Made a patch:
> 1. lsmod, modprobe, rmmod, depmod retain compatible interface: 1717 octets for all.

Wow.

> 2. insmod: do we really need to "publish" it as applet?

Ja ja. We love our users, we don't want to upset them by having
insmod disappear (or change interface).

> 3. modules.dep move to /tmp/modules.dep.bb: that way we ensure we can [re]create it.

I propose that an attempt should be made to copy it to
/lib/modules/$KERNELVERSION. If that fails, maybe issue a diagnostic
("cannot copy, will regenerate $FILE on every run [which is slow]").
Come to think about it, instead of "/tmp/modules.dep.bb" you may
call fd = mkstemp(DEFAULT_DEPMOD_FILE ".bb.XXXXXX") - makes you
safe versus modprobe races...

> Please, do try and test.

Testing.

modprobe with no parameters did generate a /tmp/modules.dep.bb,
which on the first glance seems to be ok. Strace:

execve("./busybox", ["./busybox", "modprobe"], [/* 32 vars */]) = 0
ioctl(0, SNDCTL_TMR_TIMEBASE or TCGETS, {B38400 opost isig icanon echo ...}) = 0
ioctl(1, SNDCTL_TMR_TIMEBASE or TCGETS, {B38400 opost isig icanon echo ...}) = 0
getuid32()                              = 0
chdir("/lib/modules")                   = 0
uname({sys="Linux", node="shadow", ...}) = 0
chdir("2.6.25-rc6")                     = 0
open("/tmp/modules.dep.bb", O_RDONLY|O_LARGEFILE) = -1 ENOENT (No such file or directory)
open("/tmp/modules.dep.bb", O_WRONLY|O_CREAT|O_TRUNC|O_LARGEFILE, 0644) = 3
...

But _depmod_ apparently failed to cd to $KERNELVERSION and as a result
it generated /tmp/modules.dep.bb for ALL kernels I have.
This is wrong. Strace:

execve("./busybox", ["./busybox", "depmod"], [/* 32 vars */]) = 0
ioctl(0, SNDCTL_TMR_TIMEBASE or TCGETS, {B38400 opost isig icanon echo ...}) = 0
ioctl(1, SNDCTL_TMR_TIMEBASE or TCGETS, {B38400 opost isig icanon echo ...}) = 0
getuid32()                              = 0
chdir("/lib/modules")                   = 0
uname({sys="Linux", node="shadow", ...}) = 0
unlink("/tmp/modules.dep.bb")           = -1 ENOENT (No such file or directory)
open("/tmp/modules.dep.bb", O_RDONLY|O_LARGEFILE) = -1 ENOENT (No such file or directory)
open("/tmp/modules.dep.bb", O_WRONLY|O_CREAT|O_TRUNC|O_LARGEFILE, 0644) = 3
...

Will try to reboot with bb's modprobe_ng later. Now, comments on code,
some of them just nitpicking...

#include <libbb.h>

Most other applets use #include "libbb.h"


#undef  CONFIG_DEFAULT_DEPMOD_FILE
#define CONFIG_DEFAULT_DEPMOD_FILE "/tmp/modules.dep.bb"

Shouldn't it be "/tmp/" DEFAULT_DEPMOD_FILE ".bb"?


static char* find_keyword(void *the_module, size_t len, const char * const word)

Well, const parameters buy you nothing, can use "const char *word" as well,
more readable (does not distract into "why is it * const? Is it something subtle?").


fd = (int)data;
...
fdprintf(fd, "%s ", name);

Well, can you use FILE-based io here? fdprintf is unbuffered, as a result,
modules.dep.bb generation is slow because of tons of small writes:
# cat modprobe.strace | wc -l
8128
# grep write modprobe.strace | wc -l
5124


static int inline already_loaded_issue(const char *name)
{

Don't use inline unless you must. gcc inlines single-callsite static funcs itself.


static int inline already_loaded_issue(const char *name)
{

// FIXME: overflow?
#define modules bb_common_bufsiz1

        int ret = 0;
        // N.B. can't use mmap or xmalloc_open_read_close()! They report wrong file length on mem-backed /proc/modules!!!
        if (open_read_close("/proc/modules", modules, sizeof(modules)) >= 0) {
                for (char *s = modules; (s = strstr(s, name)); s += strlen(name)) {
                        if (' ' == s[strlen(name)] && (s == s || '\n' == s[-1])) {
                                ++ret;
                                break;
                        }
                }
        }

#undef modules

        return ret;
}

* Can't you use local buffer? Or fix xmalloc_open_read_close to grow allocation
and continue reading if (unexpectedly) read() reads farther than stat.st_size.
* do strlen just once.


static int inline already_loaded_ugly(const char *name)
{
        int ret = 0;

        int fd = open("/proc/modules", O_RDONLY);
        if (fd >= 0) {
#define modules bb_common_bufsiz1
//              char *s;
                while (reads(fd, modules, sizeof(modules))) {
                        if (' ' == modules[strlen(name)] && 0 == strncmp(modules, name, strlen(name))) {
                                ++ret;
                                break;
                        }
                }
#undef modules
        }

        return ret;
}

FILE-based io will look better here. reads() _seeks_ IIRC, that can be problematic
on /proc files.


                        // load it
                        extern int init_module(void *module, unsigned long len, const char *options);
                        size_t len = MAXINT(ssize_t);

Place externs at the top (below #includes).


                        char *options = strchrnul(ptr, ' '), *module_image;

Not readable. Is it "char *options = (strchrnul(ptr, ' '), *module_image);" ?
(Yes I know that it is not, but it looks confusing).


                        if (module_image)
                                free(module_image);

just free(module_image), free(NULL) is safe.


        // lsmod called? ->
        if ('l' == argv[0][0]) {

Will fail with "/bin/lsmod". Use applet_name[0].

        // goto modules directory
        if (option_mask32 & OPT_b)
                xchdir(base_dir);
        xchdir(CONFIG_DEFAULT_MODULES_DIR);

Will first xchdir be basically always ignored (CONFIG_DEFAULT_MODULES_DIR
usually starts with "/")?


int modprobe_main(int ATTRIBUTE_UNUSED argc, char **argv)
{
        int ret = EXIT_FAILURE;
        struct utsname uts;
        char *modules_dep;
#define base_dir modules_dep

        // lsmod called? ->
        if ('l' == argv[0][0]) {
                // just dump /proc/modules
                bb_copyfd_eof(xopen("/proc/modules", O_RDONLY), STDOUT_FILENO);
                goto done;
        }
...
 done:
                if (ENABLE_FEATURE_CLEAN_UP)
                        free(modules_dep);
                ret = EXIT_SUCCESS;
        }

        // TODO: more elaborate exitcode!
        return ret;
}

free(modules_dep) would try to free garbage pointer.
--
vda



More information about the busybox mailing list