Tolerate missing /usr/bin/timeout in zfskeys

[ltning-freebsd_anduin.net]

Summary

If /usr/bin is on an encrypted dataset, zfskeys will fail as it relies on timeout(1) during key loading.

Change the call to timeout(1) to a variable which is set based on the existence and execute bit of /usr/bin/timeout.

[ltning-freebsd_anduin.net]

• Fix style

[allanjude]

libexec/rc/rc.d/zfskeys

53

if $timeout is blank, the $zfskeys_timeout value will still be there, and try to run the command 10 zfs load-key which won't work

[kevans]

libexec/rc/rc.d/zfskeys

118

I reckon this should either incorporate $zfskeys_timeout or the else branch should empty that out as well, right? To avoid executing:

10 zfs load-key -L ...

[ltning-freebsd_anduin.net]

• Add $zfskeys_timeout variable to $timeout variable

[allanjude]

Reviewed by: allanjude

[imp]

This tolerates the program being missing, but doesn't provide the failsafe that timeout gives us.

libexec/rc/rc.d/zfskeys

53

so how does this actually timeout?

[imp]

A quick search suggests this

function run_cmd { 
    cmd="$1"; timeout="$2";
    ( 
        eval "$cmd" &
        child=$!
        trap -- "" SIGTERM 
        (       
                sleep $timeout
                kill $child 2> /dev/null 
        ) &     
        wait $child
    )
}

might do the trick. I don't see any bashisms in it, though I found it in answer to a how do I implement a timeout in bash web page.

[imp]

diff --git a/libexec/rc/rc.d/zfskeys b/libexec/rc/rc.d/zfskeys
index c558eb3af5d7..f063d54b18d8 100755
--- a/libexec/rc/rc.d/zfskeys
+++ b/libexec/rc/rc.d/zfskeys
@@ -21,6 +21,23 @@ required_modules="zfs"
 : ${zfskeys_timeout:=10}
 : ${zfskeys_unload_force:='NO'}

+# A failsafe version of timeout
+alt_timeout()
+{
+    local child
+    local to=$1
+    shift
+
+    eval "$*" &
+    child=$!
+    trap -- "" SIGTERM
+    (
+        sleep $to
+        kill $child 2> /dev/null
+    ) &
+    wait $child
+}
+
 encode_args()
 {
     shift && [ $# -gt 0 ] && printf "%s\0" "$@" | b64encode -r -
@@ -50,7 +67,7 @@ unlock_fs()
             echo "Key already loaded for $fs."
         elif keytest=$(zfs load-key -n -L "$kl" "$fs" 2>&1); then
             echo "Loading key for $fs from $kl.."
-            if ! keyload=$(timeout $zfskeys_timeout zfs load-key -L "$kl" "$fs" 2>&1) ; then
+            if ! keyload=$($timeout $zfskeys_timeout zfs load-key -L "$kl" "$fs" 2>&1) ; then
                 if [ $? -eq 124 ]; then
                     echo "Timed out loading key from $kl for $fs"
                 else
@@ -114,6 +131,12 @@ unload_zfs_keys()
     done
 }

+if [ -x '/usr/bin/timeout' ]; then
+    timeout="/usr/bin/timeout"
+else
+    timeout="alt_timeout"
+fi
+
 zfskeys_args=$(encode_args "$@")
 load_rc_config $name
 run_rc_command "$1"

would wrap it in. Does that work for your situation? I don't have something handy to test it with.

[imp]

Alternatively, it was suggested on IRC to just always use alt_timeout...
Or maybe I'm nuts, in which case, ignore me :)

[kevans]

@jrtc27 noted a couple issues with alt_timeout... OTOH, would there really be any objections to mv /usr/bin/timeout /bin?

kevans@ifrit:~$ ls -l /usr/bin/timeout
-r-xr-xr-x  1 root  wheel  11584 Jul 31 06:33 /usr/bin/timeout

[imp]

In D36906#838151, @kevans wrote:

@jrtc27 noted a couple issues with alt_timeout... OTOH, would there really be any objections to mv /usr/bin/timeout /bin?

kevans@ifrit:~$ ls -l /usr/bin/timeout
-r-xr-xr-x  1 root  wheel  11584 Jul 31 06:33 /usr/bin/timeout

That would be easiest... we likely need a symlink from /usr/bin/timeout to /bin/timeout though if we did that. Other things we've moved have that link because in the ancient past we moved something w/o a symlink and that caused issues...

[0mp]

I created a patch to move timeout to /bin: https://reviews.freebsd.org/D38344

[0mp]

In D36906#871633, @0mp wrote:

I created a patch to move timeout to /bin: https://reviews.freebsd.org/D38344

The other patch has been committed: https://reviews.freebsd.org/D38344

[emaste]

@0mp if you put just the short version of the review (D38344) Phab will show if its closed or still open. With that change committed there is presumably no reason for this change any longer?