Improvements in search of kernel modules directory

These series of patches add more control over how kernel modules directory is
found:

* Add an environment variable which allows to override kernel modules
  directory;
* Allow to hardcode several paths which are searched in order for `uname -r`
  subdirectory;
* Add a configure option to set those paths, instead of hardcoding
  /lib/modules.

We have used the first patch in NixOS[1] for a long time to point kmod to
kernel modules. While an environment variable is handy and has been solving our
problems, it doesn't cover all our cases. We have two directories:

* /run/current-system/kernel-modules/lib/modules
* /run/booted-system/kernel-modules/lib/modules

, which are symlinked to modules for current system configuration (i.e. after
an update) and the one which the system was booted with. It allows us to both
give users ability to install new modules and have their old kernel modules
accessible until a reboot (which is useful in case of kernel upgrade).

Before those patches the necessary logic (see if kernel modules for current
kernel version are available in current-system, if not then fall back to
booted-system) was implemented as a shell wrapper around kmod, which was
unwieldy and didn't work for applications that use kmod directly. It was
considered better to move this logic to kmod itself. Also, NixOS uses nixpkgs,
a set of distribution-agnostic packages (which run on e.g. Ubuntu and even Mac
OS X where applicable), so it was necessary to preserve /lib/modules as a
fallback directory in kmod for it to work on FHS distributions.

As a result a set of generic patches was written that implement necessary
behaviour while being potentially useful for upstream. Environment variable is
still used in several places (e.g. in automatic generation and running of
virtual machines) and is useful for us even with the rest of those patches.

A home of those patches can be found on GitHub[2] along with some
discussion, as can be related NixOS patches and discussion[3].

Big thanks to Shea Levy for the original patch, extensive code review and
useful advices.

Additionally, while working on those it was discovered that kmod makes use of
PATH_MAX. This constant is considered harmful[4] because it doesn't correspond
to real possible length of filesystem paths. This can be considered a bug, but
in those patches it was decided to follow upstream design decisions wherever
possible and so we also use it here.

[1]: http://nixos.org/
[2]: https://github.com/abbradar/kmod/
[3]: https://github.com/NixOS/nixpkgs/pull/17738/
[4]: http://insanecoding.blogspot.ru/2007/11/pathmax-simply-isnt.html

Let the user override default /lib/modules path. One can also
define several directories to be looked in order by specifying them
separated with a colon, like this:

./configure --with-modulesdirs=/lib/modules:/usr/local/lib/modules
---
 Makefile.am       | 1 +
 configure.ac      | 6 ++++++
 libkmod/libkmod.c | 2 +-
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/Makefile.am b/Makefile.am
index d4eeb7e..5c9f603 100644
--- a/Makefile.am
+++ b/Makefile.am

@@ -19,6 +19,7 @@ AM_CPPFLAGS = \
 	-include $(top_builddir)/config.h \
 	-I$(top_srcdir) \
 	-DSYSCONFDIR=\""$(sysconfdir)"\" \
+	-DMODULESDIRS=\""$(shell echo $(modulesdirs) | $(SED) 's|:|\\",\\"|g')"\" \
 	${zlib_CFLAGS}
 
 AM_CFLAGS = $(OUR_CFLAGS)

diff --git a/configure.ac b/configure.ac
index 23510c8..66490cf 100644
--- a/configure.ac
+++ b/configure.ac

@@ -202,6 +202,12 @@ GTK_DOC_CHECK([1.14],[--flavour no-tmpl-flat])
 ], [
 AM_CONDITIONAL([ENABLE_GTK_DOC], false)])
 
+AC_ARG_WITH([modulesdirs],
+	AS_HELP_STRING([--with-modulesdirs=DIRS], [Kernel modules directories, separated by :]),
+	[],
+	[with_modulesdirs=/lib/modules])
+AC_SUBST([modulesdirs], [$with_modulesdirs])
+
 
 #####################################################################
 # Default CFLAGS and LDFLAGS

diff --git a/libkmod/libkmod.c b/libkmod/libkmod.c
index be9358d..291c08d 100644
--- a/libkmod/libkmod.c
+++ b/libkmod/libkmod.c

@@ -207,7 +207,7 @@ static int log_priority(const char *priority)
 }
 
 static const char *dirname_default_prefixes[] = {
-	"/lib/modules",
+	MODULESDIRS,
 	NULL
 };
 

-- 
2.9.2

Directories in the array are searched until the first directory with `uname -r`
subdirectory is found. As a fallback last item in the array is used
unconditionally.
---
 libkmod/libkmod.c | 45 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 40 insertions(+), 5 deletions(-)

diff --git a/libkmod/libkmod.c b/libkmod/libkmod.c
index 7b0247f..be9358d 100644
--- a/libkmod/libkmod.c
+++ b/libkmod/libkmod.c

@@ -206,7 +206,10 @@ static int log_priority(const char *priority)
 	return 0;
 }
 
-static const char *dirname_default_prefix = "/lib/modules";
+static const char *dirname_default_prefixes[] = {
+	"/lib/modules",
+	NULL
+};
 
 static char *get_kernel_release(const char *dirname)
 {

@@ -219,11 +222,43 @@ static char *get_kernel_release(const char *dirname)
 	if (uname(&u) < 0)
 		return NULL;
 
-	if ((dirname_prefix = getenv("MODULE_DIR")) == NULL)
-		dirname_prefix = dirname_default_prefix;
+	if ((dirname_prefix = getenv("MODULE_DIR")) != NULL) {
+		if(asprintf(&p, "%s/%s", dirname_prefix, u.release) < 0)
+			return NULL;
+	} else {
+		size_t i;
+		char buf[PATH_MAX];
+
+		for (i = 0; dirname_default_prefixes[i] != NULL; i++) {
+			int plen;
+
+			plen = snprintf(buf, sizeof(buf), "%s/%s", dirname_default_prefixes[i], u.release);
+			if (plen < 0)
+				return NULL;
+			else if (plen >= PATH_MAX)
+				continue;
+
+			if (dirname_default_prefixes[i + 1] != NULL) {
+				struct stat dirstat;
+
+				if (stat(buf, &dirstat) < 0) {
+					if (errno == ENOENT)
+						continue;
+					else
+						return NULL;
+				}
+
+				if (!S_ISDIR(dirstat.st_mode))
+					continue;
+			}
 
-	if (asprintf(&p, "%s/%s", dirname_prefix, u.release) < 0)
-		return NULL;
+			p = malloc(plen + 1);
+			if (p == NULL)
+				return NULL;
+			memcpy(p, buf, plen + 1);
+			break;
+		}
+	}
 
 	return p;
 }

-- 
2.9.2

From: Shea Levy <redacted>

It is used as a path if this environment variable is set, and `uname -r` is
appended to the end. Otherwise, default /lib/modules/`uname -r` is used as
usual.
---
 libkmod/libkmod.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/libkmod/libkmod.c b/libkmod/libkmod.c
index 69fe431..7b0247f 100644
--- a/libkmod/libkmod.c
+++ b/libkmod/libkmod.c

@@ -211,7 +211,7 @@ static const char *dirname_default_prefix = "/lib/modules";
 static char *get_kernel_release(const char *dirname)
 {
 	struct utsname u;
-	char *p;
+	char *p, *dirname_prefix;
 
 	if (dirname != NULL)
 		return path_make_absolute_cwd(dirname);

@@ -219,7 +219,10 @@ static char *get_kernel_release(const char *dirname)
 	if (uname(&u) < 0)
 		return NULL;
 
-	if (asprintf(&p, "%s/%s", dirname_default_prefix, u.release) < 0)
+	if ((dirname_prefix = getenv("MODULE_DIR")) == NULL)
+		dirname_prefix = dirname_default_prefix;
+
+	if (asprintf(&p, "%s/%s", dirname_prefix, u.release) < 0)
 		return NULL;
 
 	return p;

-- 
2.9.2

static-nodes has just used /lib/modules/`uname -r` before with no way to
specify another directory. Instead, make it get the path via kmod, which has a
more sophisticated algorithm for searching the modules directory.

As a side effect, cleanup error messages printing a little.
---
 tools/static-nodes.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/tools/static-nodes.c b/tools/static-nodes.c
index 8d2356d..2ed306d 100644
--- a/tools/static-nodes.c
+++ b/tools/static-nodes.c

@@ -29,10 +29,11 @@
 #include <unistd.h>
 #include <sys/stat.h>
 #include <sys/types.h>
-#include <sys/utsname.h>
 
 #include <shared/util.h>
 
+#include <libkmod/libkmod.h>
+
 #include "kmod.h"
 
 struct static_nodes_format {

@@ -154,8 +155,8 @@ static void help(void)
 
 static int do_static_nodes(int argc, char *argv[])
 {
-	struct utsname kernel;
 	char modules[PATH_MAX], buf[4096];
+	struct kmod_ctx *ctx;
 	const char *output = "/dev/stdout";
 	FILE *in = NULL, *out = NULL;
 	const struct static_nodes_format *format = &static_nodes_format_human;

@@ -206,22 +207,25 @@ static int do_static_nodes(int argc, char *argv[])
 		}
 	}
 
-	if (uname(&kernel) < 0) {
-		fputs("Error: uname failed!\n", stderr);
+	ctx = kmod_new(NULL, NULL);
+	if (ctx == NULL) {
+		fprintf(stderr, "Error: failed to create kmod context\n");
 		ret = EXIT_FAILURE;
 		goto finish;
 	}
-
-	snprintf(modules, sizeof(modules), "/lib/modules/%s/modules.devname", kernel.release);
+	if (snprintf(modules, sizeof(modules), "%s/modules.devname", kmod_get_dirname(ctx)) < 0) {
+		fprintf(stderr, "Error: path to modules.devname is too long\n");
+		ret = EXIT_FAILURE;
+		goto finish;
+	}
+	kmod_unref(ctx);
 	in = fopen(modules, "re");
 	if (in == NULL) {
 		if (errno == ENOENT) {
-			fprintf(stderr, "Warning: /lib/modules/%s/modules.devname not found - ignoring\n",
-				kernel.release);
+			fprintf(stderr, "Warning: %s not found - ignoring\n", modules);
 			ret = EXIT_SUCCESS;
 		} else {
-			fprintf(stderr, "Error: could not open /lib/modules/%s/modules.devname - %m\n",
-				kernel.release);
+			fprintf(stderr, "Error: could not open %s - %m\n", modules);
 			ret = EXIT_FAILURE;
 		}
 		goto finish;

-- 
2.9.2

Hi Nikolay,

I'm very sorry to miss these patches and not give an answer soon...
I totally forgot about them. I'll take a look this week.

Lucas De Marchi

On Mon, Aug 15, 2016 at 9:50 PM, Nikolay Amiantov [off-list ref] wrote:

These series of patches add more control over how kernel modules directory
is
found:

* Add an environment variable which allows to override kernel modules
  directory;
* Allow to hardcode several paths which are searched in order for `uname
-r`
  subdirectory;
* Add a configure option to set those paths, instead of hardcoding
  /lib/modules.

We have used the first patch in NixOS[1] for a long time to point kmod to
kernel modules. While an environment variable is handy and has been
solving our
problems, it doesn't cover all our cases. We have two directories:

* /run/current-system/kernel-modules/lib/modules
* /run/booted-system/kernel-modules/lib/modules

, which are symlinked to modules for current system configuration (i.e.
after
an update) and the one which the system was booted with. It allows us to
both
give users ability to install new modules and have their old kernel modules
accessible until a reboot (which is useful in case of kernel upgrade).

Before those patches the necessary logic (see if kernel modules for current
kernel version are available in current-system, if not then fall back to
booted-system) was implemented as a shell wrapper around kmod, which was
unwieldy and didn't work for applications that use kmod directly. It was
considered better to move this logic to kmod itself. Also, NixOS uses
nixpkgs,
a set of distribution-agnostic packages (which run on e.g. Ubuntu and even
Mac
OS X where applicable), so it was necessary to preserve /lib/modules as a
fallback directory in kmod for it to work on FHS distributions.

As a result a set of generic patches was written that implement necessary
behaviour while being potentially useful for upstream. Environment
variable is
still used in several places (e.g. in automatic generation and running of
virtual machines) and is useful for us even with the rest of those patches.

A home of those patches can be found on GitHub[2] along with some
discussion, as can be related NixOS patches and discussion[3].

Big thanks to Shea Levy for the original patch, extensive code review and
useful advices.

Additionally, while working on those it was discovered that kmod makes use
of
PATH_MAX. This constant is considered harmful[4] because it doesn't
correspond
to real possible length of filesystem paths. This can be considered a bug,
but
in those patches it was decided to follow upstream design decisions
wherever
possible and so we also use it here.

[1]: http://nixos.org/
[2]: https://github.com/abbradar/kmod/
[3]: https://github.com/NixOS/nixpkgs/pull/17738/
[4]: http://insanecoding.blogspot.ru/2007/11/pathmax-simply-isnt.html

--
To unsubscribe from this list: send the line "unsubscribe linux-modules" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Lucas De Marchi

Hi,

On Mon, Aug 15, 2016 at 5:50 PM, Nikolay Amiantov [off-list ref] wrote:

These series of patches add more control over how kernel modules directory is
found:

* Add an environment variable which allows to override kernel modules
  directory;
* Allow to hardcode several paths which are searched in order for `uname -r`
  subdirectory;
* Add a configure option to set those paths, instead of hardcoding
  /lib/modules.

We have used the first patch in NixOS[1] for a long time to point kmod to
kernel modules. While an environment variable is handy and has been solving our
problems, it doesn't cover all our cases. We have two directories:

An environment variable for the library IMO is not a good option. If
you take a look on how we separate the responsibility of each
component you will see that we parse environment vars on the tools
(e.g. modprobe) not on the library.

Additionally, while working on those it was discovered that kmod makes use of
PATH_MAX. This constant is considered harmful[4] because it doesn't correspond
to real possible length of filesystem paths. This can be considered a bug, but
in those patches it was decided to follow upstream design decisions wherever
possible and so we also use it here.

PATH_MAX is just used as a large enough buffer to hold some filenames,
module paths, etc.  Recently we introduced a new helper APIs
(scratchbuffer) to cover the cases in which we don't know the maximum
size of the buffer... Some places were converted to this new API. Few
free to submit patches using it in places it makes sense (i.e. it's
buggy to use PATH_MAX).

Lucas De Marchi

Hi Lucas,

Lucas De Marchi [off-list ref] writes:

An environment variable for the library IMO is not a good option. If
you take a look on how we separate the responsibility of each
component you will see that we parse environment vars on the tools
(e.g. modprobe) not on the library.

Can you explain a bit why an environment variable is not OK here? The
code path using get_kernel_release is shared by many tools, and as
things currently stand there is already a global setting fixed in this
file. If we were to move this out to the tools, we would have to
duplicate this code in several places and any external tools linked
against kmod (e.g. systemd) will need to be updated as well. By default
users of this interface will not know that they need to check the env
var, because as things stand the code will just work, so it's likely
that new uses, in and out of kmod, will start out not respecting
MODULE_DIR and only after someone notices things aren't working for a
case relying on it will they be changed.

Thanks,
Shea

Hi, Shea!

quoted

quoted

quoted

quoted

On Mon, 05 Dec 2016 07:35:51 -0500, Shea Levy  wrote:

 > Lucas De Marchi [off-list ref] writes:
 >> 
 >> An environment variable for the library IMO is not a good option. If
 >> you take a look on how we separate the responsibility of each
 >> component you will see that we parse environment vars on the tools
 >> (e.g. modprobe) not on the library.
 >> 

 > Can you explain a bit why an environment variable is not OK here? The
 > code path using get_kernel_release is shared by many tools, and as
 > things currently stand there is already a global setting fixed in this
 > file. If we were to move this out to the tools, we would have to
 > duplicate this code in several places and any external tools linked
 > against kmod (e.g. systemd) will need to be updated as well. By default
 > users of this interface will not know that they need to check the env
 > var, because as things stand the code will just work, so it's likely
 > that new uses, in and out of kmod, will start out not respecting
 > MODULE_DIR and only after someone notices things aren't working for a
 > case relying on it will they be changed.

What worries me with the approach is increasing ambiguity and number of
points of syncronization.

I see kernel modules directory location as a part of shared state of
several parties, at least "make modules_install" of kbuild, module builders
like dkms, binary module packages, the kmod tools themself (this is
significant different to variables like LS_COLORS, which affect only the
current program run). Since the possible modules directories are mutually
exclusive (only one is taken in use), they must be somehow in sync, or I
expect some surprises for users, a-la:

- you install a module package, it has it's own idea about the directory
  (most probably hardcoded), but depmod from postinstall script doesn't
  generate working configuration (but doesn't fail as well most probably,
  since it's just working on a different directory);
- in general, result of an operation like "dpkg -i module.deb" depends of
  which user (with what environment) performs it;
...

Something similar about several directories. If we install some package
which installs modules (and creates the dir) into a directory which did not
exist and is checked earlier, then the old modules stop working without
very obvious (as for me) connection with the installed one.

The modules installation tools should be aware somehow about directory,
which is going to be used.

Since, as I mentioned, they are exclusive, there is no analogy with
variables like PATH, where all of the dirs are part of the configuration.

I'm not sure if the concerns are important in the real life.

-- 
WBR,
Yauheni Kaliuta