Page MenuHomeFreeBSD

Tolerate missing /usr/bin/timeout in zfskeys
Needs RevisionPublic

Authored by ltning-freebsd_anduin.net on Oct 7 2022, 5:28 PM.
Tags
Referenced Files
Unknown Object (File)
Mon, Jan 9, 9:15 PM
Unknown Object (File)
Mon, Jan 9, 10:28 AM
Unknown Object (File)
Dec 24 2022, 6:36 AM
Unknown Object (File)
Dec 15 2022, 8:06 PM

Details

Reviewers
allanjude
imp
Group Reviewers
Klara
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.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 47740
Build 44627: arc lint + arc unit

Event Timeline

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 added inline comments.
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 ...

  • Add $zfskeys_timeout variable to $timeout variable
This revision is now accepted and ready to land.Oct 7 2022, 6:24 PM
imp requested changes to this revision.Oct 7 2022, 6:26 PM

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?

This revision now requires changes to proceed.Oct 7 2022, 6:26 PM

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.

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.

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

@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

@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...

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