On Wed, Feb 18, 2015 at 2:07 AM, Rusty Russell [off-list ref] wrote:

Lucas De Marchi [off-list ref] writes:

quoted

On Tue, Feb 17, 2015 at 10:56 AM, Harish Jenny K N
[off-list ref] wrote:

quoted

usecase: two sd cards are being mounted in parallel at same time on
dual core. example modules which are getting loaded is nls_cp437.
While one module is being loaded , it starts creating sysfs files.
meanwhile on other core, modprobe might return saying the module
is KMOD_MODULE_BUILTIN, which might result in not mounting sd card.

an {f,}init_module() call should not return until the sysfs files are
created and if there is /sys/module/<module>/ there should be
/sys/module/<module>/initstate unless the module is builtin.

Rusty, was there any changes in this area in the kernel recently?

Not deliberately.

quoted

Or is the creation of /sys/module/<module> and
/sys/module/<module>/initstate not atomic?

No, it's not atomic :(  That makes it unreliable (it seemed like a good
idea in 2006 I guess).

Here's what happens from a kernel side:

1) Module is marked UNFORMED.
2) Check there's no module by same name already.  If there is, and it's
   UNFORMED or COMING, we wait.
3) module is marked COMING
4) module parses arguments.
5) sysfs directory and attributes are created.
6) module's init is called.
7) module is marked LIVE.

Yeah, I just thought (an wanted that) the attributes were being
created first and then hooked up in the sysfs tree under
/sys/module/<modulename>. I.e. if the directory exists and there's no
initstate this is because it's a builtin module. I don't want to
wait/sleep on the file to appear because users of
kmod_module_get_initstate() may not tolerate this behavior.

Looking up at the old module-init-tools, it used an ugly loop with
usleep() before trying to read the file again :-/

Can we change kernel side guaranteeing the initstate file appears
together with the directory?

So, the second init_module should be waiting.

I tested this by putting a sleep in the nls_cp437 init, and watching
what modprobe did.  It works correctly.

You are checking initstate, and getting caught in the race:

783   14:33:14.170205 open("/sys/module/nls_cp437/initstate", O_RDONLY|O_LARGEFILE|O_CLOEXEC)

You should probably check initstate *after* loading fails.  It's
possible that it's unloaded in the meantime, but the race is pretty
narrow and unloading is unusual.

This call may be called in other paths, not while loading a module.
Otherwise just removing the check like what this patch does would be
sufficient.

We don't check the initstate after loading fails because we rely on
the return code of init_module, i.e. errno==EEXISTS if the previous
call succeeded.


-- 
Lucas De Marchi