| Message ID | 20170515224733.29586-12-bart.vanassche@sandisk.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, May 15, 2017 at 03:47:33PM -0700, Bart Van Assche wrote: > srp_daemon.service is modified such that instead of starting > /usr/sbin/srp_daemon.sh that it does not start any process. A new > template service is added, namely srp_daemon_port@.service. This > service replaces srp_daemon.sh and controls /usr/sbin/srp_daemon > for a single port. A udev rule is added that instantiates the > srp_daemon_port@.service every time an RDMA port is hot-added. > Since the per-port service depends on the srp_daemon service, > starting or stopping the srp_daemon service affects all per-port > services. This looks pretty good to me ... You are happy with how the user experience works? A few minor thoughts > +++ b/srp_daemon/90-srp-daemon.rules > @@ -0,0 +1 @@ > +ACTION=="add", SUBSYSTEM=="infiniband_mad", PROGRAM:="/usr/bin/systemctl show srp_daemon -p UnitFileState", RESULT=="UnitFileState=enabled", TAG+="systemd", ENV{SYSTEMD_WANTS}+="srp_daemon_port@$attr{ibdev}:$attr{port}.service" I think we should have a main rdma udev rules file that always creates .device targets inside systemd for umad, unconditionally. This should be useful for other things like opensm/etc. So we'd have a line like: ACTION=="add", SUBSYSTEM=="infiniband_mad", TAG+="systemd", ENV{SYSTEMD_ALIAS}="/dev/infiniband/umad/$attr{ibdev}:$attr{port}" So we get (unescaped) /dev/infiniband/umad0.device /dev/infiniband/umad/mlx4_0:0.device Then the srp-daemon.rule would just be the wants: ACTION=="add", SUBSYSTEM=="infiniband_mad", PROGRAM:="/usr/bin/systemctl show srp_daemon -p UnitFileState", RESULT=="UnitFileState=enabled", ENV{SYSTEMD_WANTS}+="srp_daemon_port@$attr{ibdev}:$attr{port}.service" > index 00000000..666400ab > +++ b/srp_daemon/srp_daemon_port@.service > @@ -0,0 +1,18 @@ > +[Unit] > +Description=SRP daemon that monitors port %i > +Documentation=man:srp_daemon file:/etc/rdma/rdma.conf file:/etc/srp_daemon.conf > +DefaultDependencies=false > +Conflicts=emergency.target emergency.service > +Requires=rdma.service > +Wants=opensm.service > +After=rdma.service opensm.service srp_daemon.service > +After=network.target > +Before=remote-fs-pre.target > +BindsTo=srp_daemon.service Using the new .device, we can add a requires: Requires: -dev-infiniband-umad0-%i.device Now, systemd will never start the port service until udev says that the umad is available for that port. It will actually wait until udev says that umad is present if something requires it. This would allow the admin to setup various things to guarentee ordering, most directly, adding a Requires: srp_daemon_port@mlx4_0:1 To a .mount unit will make everything very reliable. (Suggest adding notes about this to a man page) I also wonder if the systemd activation protocol should be used by srp_daemon, this should defer mount until srp_daemon has done the right stuff, but that might be no good if the 'right stuff' requires a link up? However, if the admin is using the above .mount unit Requires then they would want to wait until the link is up and the SM has been contacted before attempting the first mount? systemd obnoxiously does not have mount retries so everything should be perfect before mount is done. Probably something about this should be in the man page? [You could also consider using umad0 as the %I, which would avoid the alias, unclear to me if that good/bad for an admin.] With the other changes you did, this means we can drop this: > +After=rdma.service opensm.service srp_daemon.service rdma.service would now be obsolete because the Requires on the .device guarentees this will not start until something has loaded the modules. Also, when you fixed the port state issue, I think that solved the need for opensm.service. Thus, the after should be: After=srp_daemon.service Which is good because rdma.service or opensm.service should not appear outside the redhat/ directory. > +The srp_daemon_port@\&.service controls whether or not an srp_daemon process > +is monitoring the RDMA port specified as template argument. The format for the > +RDMA port name is \fIdev:port\fR where \fIdev\fR is the name of an RDMA device > +and \fIport\fR is an port number starting from one. Starting an instance of > +this template will start an srp_daemon process. Stopping an instance of this > +template will stop the srp_daemon process for the specified port. Here is an > +example of how to obtain a list of all RDMA device and port number > pairs: Does sysemctl mask srp_daemon_port@mlx4_0:1 Work to inhibit the daemon on a specific port? If yes that should be mentioned in here I think. Otherwise, we probably still need a way to do that. The final bits would be to see if any security sandboxing options are appropriate for the .unit file, since srp_daemon is network facing. Drop caps, ro filesystems, etc. It should also dump root Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2017-05-15 at 17:25 -0600, Jason Gunthorpe wrote: > On Mon, May 15, 2017 at 03:47:33PM -0700, Bart Van Assche wrote: > > srp_daemon.service is modified such that instead of starting > > /usr/sbin/srp_daemon.sh that it does not start any process. A new > > template service is added, namely srp_daemon_port@.service. This > > service replaces srp_daemon.sh and controls /usr/sbin/srp_daemon > > for a single port. A udev rule is added that instantiates the > > srp_daemon_port@.service every time an RDMA port is hot-added. > > Since the per-port service depends on the srp_daemon service, > > starting or stopping the srp_daemon service affects all per-port > > services. > > This looks pretty good to me ... You are happy with how the user experience works? Yes, except for one aspect, namely that starting srp_daemon does not start the per-port services. I have addressed that as follows in srp_daemon.service: ExecStart=.../srp_daemon_start_all.sh where srp_daemon_start_all.sh is the following script: #!/bin/sh for p in /sys/class/infiniband/*/ports/*; do [ -e "$p" ] || continue p=${p#/sys/class/infiniband/} nohup /usr/bin/systemctl start "srp_daemon_port@${p/\/ports\//:}" </dev/null >&/dev/null & done > A few minor thoughts > > > +++ b/srp_daemon/90-srp-daemon.rules > > @@ -0,0 +1 @@ > > +ACTION=="add", SUBSYSTEM=="infiniband_mad", PROGRAM:="/usr/bin/systemctl show srp_daemon -p UnitFileState", RESULT=="UnitFileState=enabled", TAG+="systemd", ENV{SYSTEMD_WANTS}+="srp_daemon_port@$attr{ibdev}:$attr{port}.service" > > I think we should have a main rdma udev rules file that always creates > .device targets inside systemd for umad, unconditionally. This should > be useful for other things like opensm/etc. > > So we'd have a line like: > > ACTION=="add", SUBSYSTEM=="infiniband_mad", TAG+="systemd", ENV{SYSTEMD_ALIAS}="/dev/infiniband/umad/$attr{ibdev}:$attr{port}" > > So we get (unescaped) > > /dev/infiniband/umad0.device > /dev/infiniband/umad/mlx4_0:0.device > > Then the srp-daemon.rule would just be the wants: > > ACTION=="add", SUBSYSTEM=="infiniband_mad", PROGRAM:="/usr/bin/systemctl show srp_daemon -p UnitFileState", RESULT=="UnitFileState=enabled", ENV{SYSTEMD_WANTS}+="srp_daemon_port@$attr{ibdev}:$attr{port}.service" > > > index 00000000..666400ab > > +++ b/srp_daemon/srp_daemon_port@.service > > @@ -0,0 +1,18 @@ > > +[Unit] > > +Description=SRP daemon that monitors port %i > > +Documentation=man:srp_daemon file:/etc/rdma/rdma.conf file:/etc/srp_daemon.conf > > +DefaultDependencies=false > > +Conflicts=emergency.target emergency.service > > +Requires=rdma.service > > +Wants=opensm.service > > +After=rdma.service opensm.service srp_daemon.service > > +After=network.target > > +Before=remote-fs-pre.target > > +BindsTo=srp_daemon.service > > Using the new .device, we can add a requires: > > Requires: -dev-infiniband-umad0-%i.device > > Now, systemd will never start the port service until udev says that > the umad is available for that port. It will actually wait until udev > says that umad is present if something requires it. All of this has been addressed in the following updated pull request: https://github.com/linux-rdma/rdma-core/pull/135 > I also wonder if the systemd activation protocol should be used by > srp_daemon, this should defer mount until srp_daemon has done the > right stuff, but that might be no good if the 'right stuff' requires a > link up? > > However, if the admin is using the above .mount unit Requires then > they would want to wait until the link is up and the SM has been > contacted before attempting the first mount? systemd obnoxiously does > not have mount retries so everything should be perfect before mount is > done. I think that systemd mount units should have a dependency on /dev/disk/by-id or /dev/disk/by-uuid instead of srp_daemon. Even after an srp_daemon_port@ service has been started it can take some time before login occurs. Using a dependency on /dev/disk/by-*id avoids that the mount service gets started before the path it needs is available. > Does > systemctl mask srp_daemon_port@mlx4_0:1 > > Work to inhibit the daemon on a specific port? If yes that should be > mentioned in here I think. Otherwise, we probably still need a way to > do that. Information about masking has been added to the srp_daemon_port@ man page. > The final bits would be to see if any security sandboxing options are > appropriate for the .unit file, since srp_daemon is network > facing. Drop caps, ro filesystems, etc. It should also dump root. Sorry but since srp_daemon writes into /sys/class/infiniband_srp/*/add_target I don't think it's possible to drop root privileges for the srp_daemon_port@ service. Bart.-- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 24, 2017 at 08:33:32PM +0000, Bart Van Assche wrote: > On Mon, 2017-05-15 at 17:25 -0600, Jason Gunthorpe wrote: > > On Mon, May 15, 2017 at 03:47:33PM -0700, Bart Van Assche wrote: > > > srp_daemon.service is modified such that instead of starting > > > /usr/sbin/srp_daemon.sh that it does not start any process. A new > > > template service is added, namely srp_daemon_port@.service. This > > > service replaces srp_daemon.sh and controls /usr/sbin/srp_daemon > > > for a single port. A udev rule is added that instantiates the > > > srp_daemon_port@.service every time an RDMA port is hot-added. > > > Since the per-port service depends on the srp_daemon service, > > > starting or stopping the srp_daemon service affects all per-port > > > services. > > > > This looks pretty good to me ... You are happy with how the user experience works? > > Yes, except for one aspect, namely that starting srp_daemon does not > start the per-port services. I have addressed that as follows in > srp_daemon.service: Interesting.. What use case do have in mind for start on all ports? > nohup /usr/bin/systemctl start "srp_daemon_port@${p/\/ports\//:}" </dev/null >&/dev/null & I wonder, does this defeat the 'systemctl mask srp_daemon_port@mlx4_0:1' ? Anyhow, I think it looks pretty good, the 'BindsTo' is a nice touch to support hot-removal as well. > > However, if the admin is using the above .mount unit Requires then > > they would want to wait until the link is up and the SM has been > > contacted before attempting the first mount? systemd obnoxiously does > > not have mount retries so everything should be perfect before mount is > > done. > > I think that systemd mount units should have a dependency on > /dev/disk/by-id or /dev/disk/by-uuid instead of srp_daemon. Even > after an srp_daemon_port@ service has been started it can take some > time before login occurs. Using a dependency on /dev/disk/by-*id > avoids that the mount service gets started before the path it needs > is available. That makes some sense. > > The final bits would be to see if any security sandboxing options are > > appropriate for the .unit file, since srp_daemon is network > > facing. Drop caps, ro filesystems, etc. It should also dump root. > > Sorry but since srp_daemon writes into /sys/class/infiniband_srp/*/add_target > I don't think it's possible to drop root privileges for the srp_daemon_port@ > service. Dropping root is something that has to be done inside the daemon.. eg it would open the required files as root and never close them, then drop root. So eg it would lseek,write to add_target instead of open,write. Similar with /dev/umad. The sandboxing options are different, these do not have to impact DAC_OVERRIDE for root, but permanently limit other things the daemon can do. Eg for reference here is what timesyncd.service sets: CapabilityBoundingSet=CAP_SYS_TIME CAP_SETUID CAP_SETGID CAP_SETPCAP CAP_CHOWN CAP_DAC_OVERRIDE CAP_FOWNER PrivateTmp=yes PrivateDevices=yes ProtectSystem=full ProtectHome=yes ProtectControlGroups=yes ProtectKernelTunables=yes MemoryDenyWriteExecute=yes RestrictRealtime=yes RestrictAddressFamilies=AF_UNIX AF_INET AF_INET6 SystemCallFilter=~@cpu-emulation @debug @keyring @module @mount @obsolete @raw-io Some subset of those ideas may ultimately apply to srp-daemon. Anyhow, I think the series is basically fine by me.. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/debian/srptools.install b/debian/srptools.install index ecec5d9a..81dbd4cd 100644 --- a/debian/srptools.install +++ b/debian/srptools.install @@ -1,6 +1,11 @@ etc/srp_daemon.conf +lib/systemd/system/srp_daemon.service +lib/systemd/system/srp_daemon_port@.service +lib/udev/rules.d/90-srp-daemon.rules usr/sbin/ibsrpdm usr/sbin/srp_daemon usr/share/doc/rdma-core/ibsrpdm.md usr/share/doc/srptools/ usr/share/man/man1/ibsrpdm.1 usr/share/man/man1/srp_daemon.1 +usr/share/man/man5/srp_daemon.service.5 +usr/share/man/man5/srp_daemon_port@.service.5 diff --git a/redhat/rdma-core.spec b/redhat/rdma-core.spec index 993a6c80..fc9edaa1 100644 --- a/redhat/rdma-core.spec +++ b/redhat/rdma-core.spec @@ -439,10 +439,13 @@ rm -rf %{buildroot}/%{_initrddir}/ %files -n srp_daemon %config(noreplace) %{_sysconfdir}/srp_daemon.conf %{_unitdir}/srp_daemon.service +%{_unitdir}/srp_daemon_port@.service %{_sbindir}/ibsrpdm %{_sbindir}/srp_daemon -%{_sbindir}/srp_daemon.sh %{_sbindir}/run_srp_daemon +%{_udevrulesdir}/90-srp-daemon.rules %{_mandir}/man1/ibsrpdm.1* %{_mandir}/man1/srp_daemon.1* +%{_mandir}/man5/srp_daemon.service.5* +%{_mandir}/man5/srp_daemon_port@.service.5* %doc %{_docdir}/%{name}-%{version}/ibsrpdm.md diff --git a/srp_daemon/90-srp-daemon.rules b/srp_daemon/90-srp-daemon.rules new file mode 100644 index 00000000..6d6788f0 --- /dev/null +++ b/srp_daemon/90-srp-daemon.rules @@ -0,0 +1 @@ +ACTION=="add", SUBSYSTEM=="infiniband_mad", PROGRAM:="/usr/bin/systemctl show srp_daemon -p UnitFileState", RESULT=="UnitFileState=enabled", TAG+="systemd", ENV{SYSTEMD_WANTS}+="srp_daemon_port@$attr{ibdev}:$attr{port}.service" diff --git a/srp_daemon/CMakeLists.txt b/srp_daemon/CMakeLists.txt index 24e4c30d..39f4bc99 100644 --- a/srp_daemon/CMakeLists.txt +++ b/srp_daemon/CMakeLists.txt @@ -3,6 +3,8 @@ set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${NO_STRICT_ALIASING_FLAGS}") rdma_man_pages( ibsrpdm.1 srp_daemon.1.in + srp_daemon.service.5 + srp_daemon_port@.service.5 ) rdma_sbin_executable(srp_daemon @@ -28,6 +30,9 @@ rdma_subst_install(FILES "srp_daemon.sh.in" install(FILES srp_daemon.conf DESTINATION "${CMAKE_INSTALL_SYSCONFDIR}") install(FILES srp_daemon.service DESTINATION "${CMAKE_INSTALL_SYSTEMD_SERVICEDIR}") +install(FILES srp_daemon_port@.service DESTINATION "${CMAKE_INSTALL_SYSTEMD_SERVICEDIR}") + +install(FILES 90-srp-daemon.rules DESTINATION "${CMAKE_INSTALL_UDEV_RULESDIR}") # FIXME: The ib init.d file should really be included in rdma-core as well. set(RDMA_SERVICE "openibd" CACHE STRING "init.d file service name to order srpd after") diff --git a/srp_daemon/srp_daemon.service b/srp_daemon/srp_daemon.service index 9510f5fb..5bcb1d53 100644 --- a/srp_daemon/srp_daemon.service +++ b/srp_daemon/srp_daemon.service @@ -1,17 +1,15 @@ [Unit] -Description=Start or stop the daemon that attaches to SRP devices +Description=Daemon that discovers and logs in to SRP target systems Documentation=man:srp_daemon file:/etc/rdma/rdma.conf file:/etc/srp_daemon.conf DefaultDependencies=false Conflicts=emergency.target emergency.service -Requires=rdma.service -Wants=opensm.service -After=rdma.service opensm.service -After=network.target Before=remote-fs-pre.target [Service] -Type=simple -ExecStart=/usr/sbin/srp_daemon.sh +Type=oneshot +RemainAfterExit=yes +ExecStart=/bin/true +ExecStop=/bin/true [Install] WantedBy=remote-fs-pre.target diff --git a/srp_daemon/srp_daemon.service.5 b/srp_daemon/srp_daemon.service.5 new file mode 100644 index 00000000..a6b25d6a --- /dev/null +++ b/srp_daemon/srp_daemon.service.5 @@ -0,0 +1,30 @@ +'\" t +.TH "SRP_DAEMON\&.SERVICE" "5" "" "srp_daemon" "srp_daemon.service" +.\" ----------------------------------------------------------------- +.\" * set default formatting +.\" ----------------------------------------------------------------- +.\" disable hyphenation +.nh +.\" disable justification (adjust text to left margin only) +.ad l +.\" ----------------------------------------------------------------- +.\" * MAIN CONTENT STARTS HERE * +.\" ----------------------------------------------------------------- +.SH "NAME" +srp_daemon.service \- srp_daemon systemd service that controls all ports +.SH "SYNOPSIS" +.PP +srp_daemon\&.service +.SH "DESCRIPTION" +.PP +The srp_daemon\&.service controls whether or not any srp_daemon processes are +running. Although no srp_daemon processes are controlled directly by the +srp_daemon\&.service, this service controls whether or not any +srp_daemon_port@\&.service are allowed to be active. Each +srp_daemon_port@\&.service controls one srp_daemon process. + +.SH "SEE ALSO" +.PP +\fBsrp_daemon\fR(1), +\fBsrp_daemon_port@.service\fR(5), +\fBsystemctl\fR(1) diff --git a/srp_daemon/srp_daemon_port@.service b/srp_daemon/srp_daemon_port@.service new file mode 100644 index 00000000..666400ab --- /dev/null +++ b/srp_daemon/srp_daemon_port@.service @@ -0,0 +1,18 @@ +[Unit] +Description=SRP daemon that monitors port %i +Documentation=man:srp_daemon file:/etc/rdma/rdma.conf file:/etc/srp_daemon.conf +DefaultDependencies=false +Conflicts=emergency.target emergency.service +Requires=rdma.service +Wants=opensm.service +After=rdma.service opensm.service srp_daemon.service +After=network.target +Before=remote-fs-pre.target +BindsTo=srp_daemon.service + +[Service] +Type=simple +ExecStart=/usr/sbin/srp_daemon -e -c -n -j %I -R 60 + +[Install] +WantedBy=remote-fs-pre.target diff --git a/srp_daemon/srp_daemon_port@.service.5 b/srp_daemon/srp_daemon_port@.service.5 new file mode 100644 index 00000000..a755b2eb --- /dev/null +++ b/srp_daemon/srp_daemon_port@.service.5 @@ -0,0 +1,46 @@ +'\" t +.TH "SRP_DAEMON_PORT@\&.SERVICE" "5" "" "srp_daemon" "srp_daemon_port@.service" +.\" ----------------------------------------------------------------- +.\" * set default formatting +.\" ----------------------------------------------------------------- +.\" disable hyphenation +.nh +.\" disable justification (adjust text to left margin only) +.ad l +.\" ----------------------------------------------------------------- +.\" * MAIN CONTENT STARTS HERE * +.\" ----------------------------------------------------------------- +.SH "NAME" +srp_daemon_port@.service \- srp_daemon_port@ systemd service that controls a +single port +.SH "SYNOPSIS" +.PP +srp_daemon_port@\&.service +.SH "DESCRIPTION" +.PP +The srp_daemon_port@\&.service controls whether or not an srp_daemon process +is monitoring the RDMA port specified as template argument. The format for the +RDMA port name is \fIdev:port\fR where \fIdev\fR is the name of an RDMA device +and \fIport\fR is an port number starting from one. Starting an instance of +this template will start an srp_daemon process. Stopping an instance of this +template will stop the srp_daemon process for the specified port. Here is an +example of how to obtain a list of all RDMA device and port number pairs: +.PP +.nf +.RS +$ for p in /sys/class/infiniband/*/ports/*; do + echo $p | sed 's,/sys/class/infiniband/,,;s,/ports/,:,' + done +mlx4_0:1 +mlx4_0:2 +mlx4_1:1 +mlx4_1:2 +.RE +.fi +.PP + +.SH "SEE ALSO" +.PP +\fBsrp_daemon\fR(1), +\fBsrp_daemon.service\fR(5), +\fBsystemctl\fR(1)
srp_daemon.service is modified such that instead of starting /usr/sbin/srp_daemon.sh that it does not start any process. A new template service is added, namely srp_daemon_port@.service. This service replaces srp_daemon.sh and controls /usr/sbin/srp_daemon for a single port. A udev rule is added that instantiates the srp_daemon_port@.service every time an RDMA port is hot-added. Since the per-port service depends on the srp_daemon service, starting or stopping the srp_daemon service affects all per-port services. Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> --- debian/srptools.install | 5 ++++ redhat/rdma-core.spec | 5 +++- srp_daemon/90-srp-daemon.rules | 1 + srp_daemon/CMakeLists.txt | 5 ++++ srp_daemon/srp_daemon.service | 12 ++++----- srp_daemon/srp_daemon.service.5 | 30 +++++++++++++++++++++++ srp_daemon/srp_daemon_port@.service | 18 ++++++++++++++ srp_daemon/srp_daemon_port@.service.5 | 46 +++++++++++++++++++++++++++++++++++ 8 files changed, 114 insertions(+), 8 deletions(-) create mode 100644 srp_daemon/90-srp-daemon.rules create mode 100644 srp_daemon/srp_daemon.service.5 create mode 100644 srp_daemon/srp_daemon_port@.service create mode 100644 srp_daemon/srp_daemon_port@.service.5