[PATCH 0/4] Replace sysctl setting invocations triggered from udev rule instead of modprobe configuration

[PATCH 0/4] Replace sysctl setting invocations triggered from udev rule instead of modprobe configuration

[Salvatore Bonaccorso]

Hi Neil, hi Steve

In Debian for the update including the systemd/50-nfs.conf there was a
report that sunrpc is not included anymore in the initrd through the
initramfs-tools hooks. 

The report is at https://bugs.debian.org/1022172

Marco d'Intri suggested there three possible solutions, of which one
could be done in nfs-utils (whereas the other two are either in kmod
upstream or initramfs-tools upstream). The nfs-utils one would be to
replace the modprobe configuration with a set of udev rules instead.

This series reverts the commit wich intorduce the use of the modprobe
configuration and instead replaces it with an udev rule which triggers
setting the sysctl settings when the respective modules are loaded

Regards,
Salvatore

Salvatore Bonaccorso (4):
  Revert "configure: make modprobe.d directory configurable."
  Revert "modprobe: protect against sysctl errors"
  Revert "systemd: Apply all sysctl settings when NFS-related modules
    are loaded"
  systemd: Apply all sysctl settings through udev rule  when NFS-related
    modules are loaded

 configure.ac         | 12 ------------
 systemd/50-nfs.conf  | 16 ----------------
 systemd/60-nfs.rules | 21 +++++++++++++++++++++
 systemd/Makefile.am  | 15 ++++++---------
 4 files changed, 27 insertions(+), 37 deletions(-)
 delete mode 100644 systemd/50-nfs.conf
 create mode 100644 systemd/60-nfs.rules

-- 
2.38.1

[PATCH 1/4] Revert "configure: make modprobe.d directory configurable."

[Salvatore Bonaccorso]

This reverts commit 7d76dd2e6f09a141eb6303b7343baa5c4f9c85ad.

As part of the full revert of adding support via modprobe.d
configuration to set sysctl settings of NFS-related modules when loading
the modules.

The approach caused problems with sysctl from busybox and with kmod as
reported in Debian (https://bugs.debian.org/1024082)

Signed-off-by: Salvatore Bonaccorso <carnil@debian.org>
---
 configure.ac        | 12 ------------
 systemd/Makefile.am |  6 ++----
 2 files changed, 2 insertions(+), 16 deletions(-)

diff --git a/configure.ac b/configure.ac
index 5d9cbf317453..4280cc770a45 100644
--- a/configure.ac
+++ b/configure.ac
@@ -71,18 +71,6 @@ AC_ARG_WITH(systemd,
 	AM_CONDITIONAL(INSTALL_SYSTEMD, [test "$use_systemd" = 1])
 	AC_SUBST(unitdir)
 
-modprobedir=/usr/lib/modprobe.d
-AC_ARG_WITH(modprobedir,
-	[AS_HELP_STRING([--with-modprobedir@<:@=modprobe-dir-path@:>@],[install modprobe config files @<:@Default: /usr/lib/modprobe.d@:>@])],
-	if test "$withval" != "no" ; then
-		modprobedir=$withval
-	else
-		modprobedir=
-	fi
-	)
-	AM_CONDITIONAL(INSTALL_MODPROBEDIR, [test -n "$modprobedir"])
-	AC_SUBST(modprobedir)
-
 AC_ARG_ENABLE(nfsv4,
 	[AS_HELP_STRING([--disable-nfsv4],[disable support for NFSv4 @<:@default=no@:>@])],
 	enable_nfsv4=$enableval,
diff --git a/systemd/Makefile.am b/systemd/Makefile.am
index 7b5ab84bd793..63a50bf2c07e 100644
--- a/systemd/Makefile.am
+++ b/systemd/Makefile.am
@@ -82,7 +82,5 @@ install-data-hook: $(unit_files) $(modprobe_files)
 else
 install-data-hook: $(modprobe_files)
 endif
-if INSTALL_MODPROBEDIR
-	mkdir -p $(DESTDIR)$(modprobedir)
-	cp $(modprobe_files) $(DESTDIR)$(modprobedir)
-endif
+	mkdir -p $(DESTDIR)/usr/lib/modprobe.d
+	cp $(modprobe_files) $(DESTDIR)/usr/lib/modprobe.d/
-- 
2.38.1

[PATCH 2/4] Revert "modprobe: protect against sysctl errors"

[Salvatore Bonaccorso]

This reverts commit 5e60e38aa4ba251ef66610514be5f45c41519e0f.

As part of the full revert of adding support via modprobe.d
configuration to set sysctl settings of NFS-related modules when loading
the modules.

The approach caused problems with sysctl from busybox and with kmod as
reported in Debian (https://bugs.debian.org/1024082)

Signed-off-by: Salvatore Bonaccorso <carnil@debian.org>
---
 systemd/50-nfs.conf | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/systemd/50-nfs.conf b/systemd/50-nfs.conf
index 19e8ee734c8e..b56b2d765969 100644
--- a/systemd/50-nfs.conf
+++ b/systemd/50-nfs.conf
@@ -1,16 +1,16 @@
 # Ensure all NFS systctl settings get applied when modules load
 
 # sunrpc module supports "sunrpc.*" sysctls
-install sunrpc /sbin/modprobe --ignore-install sunrpc $CMDLINE_OPTS && { /sbin/sysctl -q --pattern sunrpc --system; exit 0; }
+install sunrpc /sbin/modprobe --ignore-install sunrpc $CMDLINE_OPTS && /sbin/sysctl -q --pattern sunrpc --system
 
 # rpcrdma module supports sunrpc.svc_rdma.*
-install rpcrdma /sbin/modprobe --ignore-install rpcrdma $CMDLINE_OPTS && { /sbin/sysctl -q --pattern sunrpc.svc_rdma --system; exit 0; }
+install rpcrdma /sbin/modprobe --ignore-install rpcrdma $CMDLINE_OPTS && /sbin/sysctl -q --pattern sunrpc.svc_rdma --system
 
 # lockd module supports "fs.nfs.nlm*" and "fs.nfs.nsm*" sysctls
-install lockd /sbin/modprobe --ignore-install lockd $CMDLINE_OPTS && { /sbin/sysctl -q --pattern fs.nfs.n[sl]m --system; exit 0; }
+install lockd /sbin/modprobe --ignore-install lockd $CMDLINE_OPTS && /sbin/sysctl -q --pattern fs.nfs.n[sl]m --system
 
 # nfsv4 module supports "fs.nfs.*" sysctls (nfs_callback_tcpport and idmap_cache_timeout)
-install nfsv4 /sbin/modprobe --ignore-install nfsv4 $CMDLINE_OPTS && { /sbin/sysctl -q --pattern 'fs.nfs.(nfs_callback_tcpport|idmap_cache_timeout)' --system; exit 0; }
+install nfsv4 /sbin/modprobe --ignore-install nfsv4 $CMDLINE_OPTS && /sbin/sysctl -q --pattern 'fs.nfs.(nfs_callback_tcpport|idmap_cache_timeout)' --system
 
 # nfs module supports "fs.nfs.*" sysctls
-install nfs /sbin/modprobe --ignore-install nfs $CMDLINE_OPTS && { /sbin/sysctl -q --pattern fs.nfs --system; exit 0; }
+install nfs /sbin/modprobe --ignore-install nfs $CMDLINE_OPTS && /sbin/sysctl -q --pattern fs.nfs --system
-- 
2.38.1

[PATCH 3/4] Revert "systemd: Apply all sysctl settings when NFS-related modules are loaded"

[Salvatore Bonaccorso]

This reverts commit afc7132dfb212ac1f676a5ac36d29a9e06325645.

The approach caused problems with sysctl from busybox and with kmod as
reported in Debian (https://bugs.debian.org/1024082).

Signed-off-by: Salvatore Bonaccorso <carnil@debian.org>
---
 systemd/50-nfs.conf | 16 ----------------
 systemd/Makefile.am | 10 ++--------
 2 files changed, 2 insertions(+), 24 deletions(-)
 delete mode 100644 systemd/50-nfs.conf

diff --git a/systemd/50-nfs.conf b/systemd/50-nfs.conf
deleted file mode 100644
index b56b2d765969..000000000000
--- a/systemd/50-nfs.conf
+++ /dev/null
@@ -1,16 +0,0 @@
-# Ensure all NFS systctl settings get applied when modules load
-
-# sunrpc module supports "sunrpc.*" sysctls
-install sunrpc /sbin/modprobe --ignore-install sunrpc $CMDLINE_OPTS && /sbin/sysctl -q --pattern sunrpc --system
-
-# rpcrdma module supports sunrpc.svc_rdma.*
-install rpcrdma /sbin/modprobe --ignore-install rpcrdma $CMDLINE_OPTS && /sbin/sysctl -q --pattern sunrpc.svc_rdma --system
-
-# lockd module supports "fs.nfs.nlm*" and "fs.nfs.nsm*" sysctls
-install lockd /sbin/modprobe --ignore-install lockd $CMDLINE_OPTS && /sbin/sysctl -q --pattern fs.nfs.n[sl]m --system
-
-# nfsv4 module supports "fs.nfs.*" sysctls (nfs_callback_tcpport and idmap_cache_timeout)
-install nfsv4 /sbin/modprobe --ignore-install nfsv4 $CMDLINE_OPTS && /sbin/sysctl -q --pattern 'fs.nfs.(nfs_callback_tcpport|idmap_cache_timeout)' --system
-
-# nfs module supports "fs.nfs.*" sysctls
-install nfs /sbin/modprobe --ignore-install nfs $CMDLINE_OPTS && /sbin/sysctl -q --pattern fs.nfs --system
diff --git a/systemd/Makefile.am b/systemd/Makefile.am
index 63a50bf2c07e..e7f5d818a913 100644
--- a/systemd/Makefile.am
+++ b/systemd/Makefile.am
@@ -2,8 +2,6 @@
 
 MAINTAINERCLEANFILES = Makefile.in
 
-modprobe_files = 50-nfs.conf
-
 unit_files =  \
     nfs-client.target \
     rpc_pipefs.target \
@@ -53,7 +51,7 @@ endif
 
 man5_MANS	= nfs.conf.man
 man7_MANS	= nfs.systemd.man
-EXTRA_DIST = $(unit_files) $(modprobe_files) $(man5_MANS) $(man7_MANS)
+EXTRA_DIST = $(unit_files) $(man5_MANS) $(man7_MANS)
 
 generator_dir = $(unitdir)/../system-generators
 
@@ -75,12 +73,8 @@ rpc_pipefs_generator_LDADD = ../support/nfs/libnfs.la
 
 if INSTALL_SYSTEMD
 genexec_PROGRAMS = nfs-server-generator rpc-pipefs-generator
-install-data-hook: $(unit_files) $(modprobe_files)
+install-data-hook: $(unit_files)
 	mkdir -p $(DESTDIR)/$(unitdir)
 	cp $(unit_files) $(DESTDIR)/$(unitdir)
 	cp $(rpc_pipefs_mount_file) $(DESTDIR)/$(unitdir)/$(rpc_pipefsmount)
-else
-install-data-hook: $(modprobe_files)
 endif
-	mkdir -p $(DESTDIR)/usr/lib/modprobe.d
-	cp $(modprobe_files) $(DESTDIR)/usr/lib/modprobe.d/
-- 
2.38.1

[PATCH 4/4] systemd: Apply all sysctl settings through udev rule when NFS-related modules are loaded

[Salvatore Bonaccorso]

sysctl settings (e.g.  /etc/sysctl.conf and others) are normally loaded
once at boot.  If the module that implements some settings is no yet
loaded, those settings don't get applied.

Various NFS modules support various sysctl settings.  If they are loaded
after boot, they miss out.

Add a new udev rule configuration to udev/rules.d/60-nfs.rules to apply
the relevant settings when the module is loaded.

Placing it in the systemd directory similarly as the coice for the
original commit afc7132dfb21 ("systemd: Apply all sysctl settings when
NFS-related modules are loaded").

Link: https://lore.kernel.org/linux-nfs/Y1KoKwu88PulcokW@eldamar.lan/
Link: https://bugs.debian.org/1022172
Link: https://bugs.debian.org/1024082
Suggested-by: Marco d'Itri <md@linux.it>
Signed-off-by: Salvatore Bonaccorso <carnil@debian.org>
---
 systemd/60-nfs.rules | 21 +++++++++++++++++++++
 systemd/Makefile.am  |  9 +++++++--
 2 files changed, 28 insertions(+), 2 deletions(-)
 create mode 100644 systemd/60-nfs.rules

diff --git a/systemd/60-nfs.rules b/systemd/60-nfs.rules
new file mode 100644
index 000000000000..188423c1d2e3
--- /dev/null
+++ b/systemd/60-nfs.rules
@@ -0,0 +1,21 @@
+# Ensure all NFS systctl settings get applied when modules load
+
+# sunrpc module supports "sunrpc.*" sysctls
+ACTION=="add", SUBSYSTEM=="module", KERNEL=="sunrpc", \
+  RUN+="/sbin/sysctl -q --pattern ^sunrpc --system"
+
+# rpcrdma module supports sunrpc.svc_rdma.*
+ACTION=="add", SUBSYSTEM=="module", KERNEL=="rpcrdma", \
+  RUN+="/sbin/sysctl -q --pattern ^sunrpc.svc_rdma --system"
+
+# lockd module supports "fs.nfs.nlm*" and "fs.nfs.nsm*" sysctls
+ACTION=="add", SUBSYSTEM=="module", KERNEL=="lockd", \
+  RUN+="/sbin/sysctl -q --pattern ^fs.nfs.n[sl]m --system"
+
+# nfsv4 module supports "fs.nfs.*" sysctls (nfs_callback_tcpport and idmap_cache_timeout)
+ACTION=="add", SUBSYSTEM=="module", KERNEL=="nfsv4", \
+  RUN+="/sbin/sysctl -q --pattern ^fs.nfs.(nfs_callback_tcpport|idmap_cache_timeout) --system"
+
+# nfs module supports "fs.nfs.*" sysctls
+ACTION=="add", SUBSYSTEM=="module", KERNEL=="nfs", \
+  RUN+="/sbin/sysctl -q --pattern ^fs.nfs --system"
diff --git a/systemd/Makefile.am b/systemd/Makefile.am
index e7f5d818a913..577c6a2286c0 100644
--- a/systemd/Makefile.am
+++ b/systemd/Makefile.am
@@ -2,6 +2,9 @@
 
 MAINTAINERCLEANFILES = Makefile.in
 
+udev_rulesdir = /usr/lib/udev/rules.d/
+udev_files = 60-nfs.rules
+
 unit_files =  \
     nfs-client.target \
     rpc_pipefs.target \
@@ -51,7 +54,7 @@ endif
 
 man5_MANS	= nfs.conf.man
 man7_MANS	= nfs.systemd.man
-EXTRA_DIST = $(unit_files) $(man5_MANS) $(man7_MANS)
+EXTRA_DIST = $(unit_files) $(udev_files) $(man5_MANS) $(man7_MANS)
 
 generator_dir = $(unitdir)/../system-generators
 
@@ -73,8 +76,10 @@ rpc_pipefs_generator_LDADD = ../support/nfs/libnfs.la
 
 if INSTALL_SYSTEMD
 genexec_PROGRAMS = nfs-server-generator rpc-pipefs-generator
-install-data-hook: $(unit_files)
+install-data-hook: $(unit_files) $(udev_files)
 	mkdir -p $(DESTDIR)/$(unitdir)
 	cp $(unit_files) $(DESTDIR)/$(unitdir)
 	cp $(rpc_pipefs_mount_file) $(DESTDIR)/$(unitdir)/$(rpc_pipefsmount)
+	mkdir -p $(DESTDIR)/$(udev_rulesdir)
+	cp $(udev_files) $(DESTDIR)/$(udev_rulesdir)
 endif
-- 
2.38.1

Re: [PATCH 4/4] systemd: Apply all sysctl settings through udev rule when NFS-related modules are loaded

[Michael Prokop]

[-- Attachment #1: Type: text/plain, Size: 1402 bytes --]

Hi,

* Salvatore Bonaccorso [Fri Nov 25, 2022 at 02:07:25PM +0100]:

> sysctl settings (e.g.  /etc/sysctl.conf and others) are normally loaded
> once at boot.  If the module that implements some settings is no yet
> loaded, those settings don't get applied.
> 
> Various NFS modules support various sysctl settings.  If they are loaded
> after boot, they miss out.
> 
> Add a new udev rule configuration to udev/rules.d/60-nfs.rules to apply
> the relevant settings when the module is loaded.
> 
> Placing it in the systemd directory similarly as the coice for the
> original commit afc7132dfb21 ("systemd: Apply all sysctl settings when
> NFS-related modules are loaded").
[...]

> --- /dev/null
> +++ b/systemd/60-nfs.rules
> @@ -0,0 +1,21 @@
> +# Ensure all NFS systctl settings get applied when modules load
> +
> +# sunrpc module supports "sunrpc.*" sysctls
> +ACTION=="add", SUBSYSTEM=="module", KERNEL=="sunrpc", \
> +  RUN+="/sbin/sysctl -q --pattern ^sunrpc --system"
[...]

Thanks for taking care of this problem, Salvatore!

AFAICT even latest busybox's sysctl does not support the `--pattern`
option yet:

| sysctl: unrecognized option '--pattern'
| BusyBox v1.35.0 (Debian 1:1.35.0-4) multi-call binary.
| [....]

So any initramfs that uses busybox and its sysctl (like in Debian)
and trying to apply above udev rules might fail?

regards
-mika-

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/4] systemd: Apply all sysctl settings through udev rule when NFS-related modules are loaded

[Salvatore Bonaccorso]

Hi Michael,

On Fri, Nov 25, 2022 at 02:29:35PM +0100, Michael Prokop wrote:
> Hi,
> 
> * Salvatore Bonaccorso [Fri Nov 25, 2022 at 02:07:25PM +0100]:
> 
> > sysctl settings (e.g.  /etc/sysctl.conf and others) are normally loaded
> > once at boot.  If the module that implements some settings is no yet
> > loaded, those settings don't get applied.
> > 
> > Various NFS modules support various sysctl settings.  If they are loaded
> > after boot, they miss out.
> > 
> > Add a new udev rule configuration to udev/rules.d/60-nfs.rules to apply
> > the relevant settings when the module is loaded.
> > 
> > Placing it in the systemd directory similarly as the coice for the
> > original commit afc7132dfb21 ("systemd: Apply all sysctl settings when
> > NFS-related modules are loaded").
> [...]
> 
> > --- /dev/null
> > +++ b/systemd/60-nfs.rules
> > @@ -0,0 +1,21 @@
> > +# Ensure all NFS systctl settings get applied when modules load
> > +
> > +# sunrpc module supports "sunrpc.*" sysctls
> > +ACTION=="add", SUBSYSTEM=="module", KERNEL=="sunrpc", \
> > +  RUN+="/sbin/sysctl -q --pattern ^sunrpc --system"
> [...]
> 
> Thanks for taking care of this problem, Salvatore!

Thanks to you for prodding about it, hope to bring the issue bit
forward with the series proposal.

> AFAICT even latest busybox's sysctl does not support the `--pattern`
> option yet:
> 
> | sysctl: unrecognized option '--pattern'
> | BusyBox v1.35.0 (Debian 1:1.35.0-4) multi-call binary.
> | [....]
> 
> So any initramfs that uses busybox and its sysctl (like in Debian)
> and trying to apply above udev rules might fail?

But would this actually be a problem for us here? There is no hook
script which would copy the 60-nfs.rules (not relevant in initrd) to
the initrd. The rule only would apply on module load outside the
initrd.

There is only a subset of rules which would be copied into initrd,
like the ones in hook/udev. But 60-nfs.rules would be specific to
nfs-utils, which does not provide a initramfs-tools hook to include
the rules into initrd.

Now the question you raise, is, do they need to be handled actually
already as well in initrd? You are correct, when handled through the
previous mechanism with modrobe.d configuration, 50-nfs.conf was added
to initramfs:

usr/lib/modprobe.d/50-nfs.conf 

(and causing the issues seen).

Please correct me if I missed something from the picture.

Regards,
Salvatore

Re: [PATCH 4/4] systemd: Apply all sysctl settings through udev rule when NFS-related modules are loaded

[Michael Prokop]

[-- Attachment #1: Type: text/plain, Size: 2279 bytes --]

Hi!

* Salvatore Bonaccorso [Fri Nov 25, 2022 at 05:21:41PM +0100]:
> On Fri, Nov 25, 2022 at 02:29:35PM +0100, Michael Prokop wrote:
> > * Salvatore Bonaccorso [Fri Nov 25, 2022 at 02:07:25PM +0100]:

> > > --- /dev/null
> > > +++ b/systemd/60-nfs.rules
> > > @@ -0,0 +1,21 @@
> > > +# Ensure all NFS systctl settings get applied when modules load
> > > +
> > > +# sunrpc module supports "sunrpc.*" sysctls
> > > +ACTION=="add", SUBSYSTEM=="module", KERNEL=="sunrpc", \
> > > +  RUN+="/sbin/sysctl -q --pattern ^sunrpc --system"
> > [...]
> > 
> > Thanks for taking care of this problem, Salvatore!
> 
> Thanks to you for prodding about it, hope to bring the issue bit
> forward with the series proposal.

ACK, thanks, I highly appreciate your efforts!

> > AFAICT even latest busybox's sysctl does not support the `--pattern`
> > option yet:
> > 
> > | sysctl: unrecognized option '--pattern'
> > | BusyBox v1.35.0 (Debian 1:1.35.0-4) multi-call binary.
> > | [....]
> > 
> > So any initramfs that uses busybox and its sysctl (like in Debian)
> > and trying to apply above udev rules might fail?
> 
> But would this actually be a problem for us here? There is no hook
> script which would copy the 60-nfs.rules (not relevant in initrd) to
> the initrd. The rule only would apply on module load outside the
> initrd.

Indeed, I also think that as long as this udev rule doesn't end up
in the initrd there shouldn't be any problem with it.

> There is only a subset of rules which would be copied into initrd,
> like the ones in hook/udev. But 60-nfs.rules would be specific to
> nfs-utils, which does not provide a initramfs-tools hook to include
> the rules into initrd.

Good point, thanks for checking and clarifying.

> Now the question you raise, is, do they need to be handled actually
> already as well in initrd? You are correct, when handled through the
> previous mechanism with modrobe.d configuration, 50-nfs.conf was added
> to initramfs:
> 
> usr/lib/modprobe.d/50-nfs.conf 
> 
> (and causing the issues seen).
> 
> Please correct me if I missed something from the picture.

No, I think you're right and AFAICS we shouldn't see the issues we
originally noticed any longer. Thanks! :)

regards
-mika-

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]