Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Register binfmt #2499

Merged
merged 11 commits into from
Nov 5, 2021
Merged

Register binfmt #2499

merged 11 commits into from
Nov 5, 2021

Conversation

jcaesar
Copy link
Contributor

@jcaesar jcaesar commented Aug 1, 2021

Description

Specialized command line parsing for when running wasmer/cli as a binfmt_misc "interpreter", and a new subcommand to manage the binfmt_misc registration, as described in #483.

Effect: e.g. this works

wget https://registry-cdn.wapm.io/contents/liftm/dust-wasi/0.5.4-3/dust.wasm -O dust
chmod a+x dust
./dust

What you may want to consider during review:

  • I have not seen that trick of copying the binary to /tmp to register it from a specially named path anywhere else. It might be a bad idea.
  • By default . and nothing else is preopened. Am I correct in the assumption that there is no way of giving WASI full filesystem access (because it has no concept of a working directory).
  • Should it also register .wat?

Review

  • Add a short description of the change to the CHANGELOG.md file

lib/cli/src/commands.rs Outdated Show resolved Hide resolved
lib/cli/src/commands.rs Outdated Show resolved Hide resolved
@syrusakbary
Copy link
Member

Replying inline:

I have not seen that trick of copying the binary to /tmp to register it from a specially named path anywhere else. It might be a bad idea.

Is there any way we can reuse the installed wasmer binary without copying?

By default . and nothing else is preopened. Am I correct in the assumption that there is no way of giving WASI full filesystem access (because it has no concept of a working directory).

I think giving access to . by default is good (rather than the whole filesystem)

Should it also register .wat?

Wat is fully a a text-format so it will hard to recognize it properly (if we take on account comments and so, the first bytes might vary).

@jcaesar
Copy link
Contributor Author

jcaesar commented Aug 3, 2021

Is there any way we can reuse the installed wasmer binary without copying?

Even when doing some very terrible things to command line parsing, I don't think so. With binfmt_misc, it doesn't seem like you get any extra environment variables, and you can only get the command line to be either of

  • path-to-registered-interpreter executable-wasm-file original-arg-1 original-arg-2 ...
  • path-to-registered-interpreter executable-wasm-file original-arg-0 original-arg-1 original-arg-2 ...

But I don't see any way of distinguishing that from a normal wasmer invocation, where you might do wasmer foo.wasm --dir . -- or similar. Consider: a wasm executable may have entirely the same command line parameter format that wasmer/cli has.

Without changing the behavior of invoking wasmer/cli normally, I can only think of

  • Add a second executable that generates a command line that wasmer can understand (but that won't work through lxc / docker / chroots / ...; Admittedly, this won't either, unless wasmer is compiled for x86_64-unknown-linux-musl.)
  • Set up the install script to create a hard link / ref link on install
  • Add a second executable that contains another wasmer, exclusively for binfmt_misc

Wat is fully a a text-format so it will hard to recognize it properly

It would be recognized by the .wat filename extension (and the +x bit). Not sure if that's "hard".
(Of course it is also weird but possible to require files to start with a ;; execute with wasmer comment and match on that.)

@syrusakbary
Copy link
Member

But I don't see any way of distinguishing that from a normal wasmer invocation

I think it should be possible to create a new subcommand: binfmt-run that does a specialized version of wasmer run.
That way, the binary will not needed to be copied. Other way is just create a separate binary that executes wasmer under the hood (basically calling Command::. Thoughts?

It would be recognized by the .wat filename extension (and the +x bit). Not sure if that's "hard".

Cool, let's roll with that for now then! (running executable .wat). Somehow I thought binfmt would only work if the header of the binary matched certain bytes.

@jcaesar
Copy link
Contributor Author

jcaesar commented Aug 3, 2021

I think it should be possible to create a new subcommand: binfmt-run that does a specialized version of wasmer run.
That way, the binary will not needed to be copied.

I don't see how you would get binfmt_misc to pass you this binfmt-run as $1, binfmt_misc doesn't support passing extra arguments.

(The format of registering with binfmt_misc is :name:type:offset:magic:mask:interpreter:flags, where interpreter is a file path and doesn't take arguments. You can't put a /usr/bin/wasmer binfmt-run into there (It will look for a file named wasmer\ binfmt-run.))

Other way is just create a separate binary that executes wasmer under the hood

Yes. This is what I meant by:

Add a second executable that generates a command line that wasmer can understand

But then you can't register wasmer on the "host" and run it in a docker container, or vice versa. Personally, I kind of want that feature. For example, try this:

docker run --rm --privileged -v /proc/sys/fs/binfmt_misc/:/binfmt_misc/ liftm/wasmer:binfmt-experiment

(backing Dockerfile, in case you'd rather build from source.)
Run only that, and you'll have the binfmt setup to run a wasm executable in any container on the machine.
(I'm only using docker because I think it's the currently most commonly used. This works over bwrap, systemd-nspawn, plain lxc, chroots, ...)

I'm curious, is there anything that really speaks against the copying (other than probably being unexplored solution space)?

@syrusakbary
Copy link
Member

I'm curious, is there anything that really speaks against the copying (other than probably being unexplored solution space)?

My main concern is that things will get outdated when you run wasmer self-update or install wasmer in other way, introducing unintended side effects.

Here's my proposal:
Create another binary file lib/cli/src/bin/wasmer_binfmt.rs that simply executes Command::... with wasmer binfmt-run ... under the hood. Thoughts?

@jcaesar
Copy link
Contributor Author

jcaesar commented Aug 4, 2021

My main concern is that things will get outdated when you run wasmer self-update or install wasmer in other way

That is a problem, indeed. The qemu interpreters I've used so far all handled this by being installed through the distro and having their registrations re-done on install/update. I could of course get the self-update command to be aware of binfmts, but there is a caveat about feature interaction and complexity. Hm.

(Note to self: Remove F flag to avoid the binary being preopened at registration time.)

@jcaesar
Copy link
Contributor Author

jcaesar commented Aug 4, 2021

Hm, if the "copy self to /tmp" thing is through, then this entire PR is unnecessary.

All you need is two files

  • /etc/binfmt.d/wasmer.conf
:wasm32:M::\x00asm\x01\x00\x00::/usr/bin/wasmer-binfmt:P
:wasm32-wat:E::wat::/usr/bin/wasmer-binfmt:P
  • /usr/bin/wasmer-binfmt
#!/usr/bin/env sh
set -eu

wasmer="$(dirname "$0")/wasmer"
if ! test -x "$wasmer"; then
	if ! wasmer="$(which wasmer 2>/dev/null)"; then
		echo Could not locate wasmer binary 1>&2
		exit 137
	fi
fi

dir="${WASMER_BINFMT_MISC_PREOPEN-.}"
wasm="$1"
shift
cm="$1"
shift

exec "$wasmer" run --dir "$dir" --command-name "$cm" "$wasm" -- "$@"
[Edit:] OpenRC support
  • /etc/init.d/wasmer-binfmt
#!/sbin/openrc-run

description="Register wasmer as wasm and wat interpreter"

depend()
{
	after clock procfs
	use modules devfs
	keyword -docker -lxc -openvz -prefix -systemd-nspawn -vserver
}

start() {
	ebegin "Registering wasmer binfmt"
	
	if [ ! -d /proc/sys/fs/binfmt_misc ] ; then
		modprobe -q binfmt_misc
	fi

	if [ ! -d /proc/sys/fs/binfmt_misc ] ; then
		eend 1 "You need support for 'misc binaries' in your kernel!"
		return
	fi

	if [ ! -f /proc/sys/fs/binfmt_misc/register ] ; then
		mount -t binfmt_misc -o nodev,noexec,nosuid \
			binfmt_misc /proc/sys/fs/binfmt_misc >/dev/null 2>&1
		eend $? || return
	fi

	echo ':wasm32:M::\x00asm\x01\x00\x00::/usr/bin/wasmer-binfmt:P' >/proc/sys/fs/binfmt_misc/register
	echo ':wasm32-wat:E::wat::/usr/bin/wasmer-binfmt:P' >/proc/sys/fs/binfmt_misc/register
	eend 0
}

stop() {
	ebegin "Unregistering wasmer binfmt"
	local f
	for f in /proc/sys/fs/binfmt_misc/wasm32{,-wat} ; do
		if test -f $f ; then
			echo -1 >$f
		fi
	done
	eend 0
}

but how to install those as part of the install script is something that the wasmer team will know better than I do.

Adding a wasmer binfmt register subcommand is unnecessary, and it would be a relatively unusual/odd thing to do.

@syrusakbary
Copy link
Member

Adding a wasmer binfmt register subcommand is unnecessary, and it would be a relatively unusual/odd thing to do.

What about calling wasmer binfmt register just automatically upon Wasmer installation in Linux, which creates the files with the contents you mentioned? I think that might be a bit better than just appending the wasmer.conf and other files in the bash install script?

That way we can also have a wasmer binfmt unregister that just uninstalls it.

Do you think that's a reasonable approach?

In case it's useful, the wasmer-install repo lives here: https://github.com/wasmerio/wasmer-install

@jcaesar
Copy link
Contributor Author

jcaesar commented Aug 6, 2021

What about calling wasmer binfmt register just automatically upon Wasmer installation in Linux,

That solves the "self-update won't update the binfmt registration" problem with this PR, but the command needs to be re-executed after every boot.
So either install a systemd service, or use systemd-binfmt by placing a file in /etc/binfmt.d/.

Systemd service installation is a bit meh..

if test $OS == "linux" && which systemctl 1>/dev/null 2>/dev/null; then
cat <<EoF >/etc/systemd/system/wasmer-binfmt.service
[Unit]
Description=Set up wasmer to handle execution of wasm binaries
DefaultDependencies=no
Conflicts=shutdown.target
After=proc-sys-fs-binfmt_misc.automount
After=proc-sys-fs-binfmt_misc.mount
Before=sysinit.target shutdown.target
ConditionPathIsReadWrite=/proc/sys/

[Service]
Type=oneshot
RemainAfterExit=yes
ExecStart=$INSTALL_DIRECTORY/bin/wasmer binfmt reregister
ExecStop=$INSTALL_DIRECTORY/bin/wasmer binfmt unregister
EoF
systemctl daemon-reload
systemctl enable wasmer-binfmt
systemctl restart wasmer-binfmt
fi

That way we can also have a wasmer binfmt unregister that just uninstalls it.

Like so?
https://github.com/wasmerio/wasmer/pull/2499/files#diff-daba8471e1ba6c8dab8454a5c1979594cfd27e8903dc8ce4f2ecede97f35e99dR16

@Hywan Hywan self-assigned this Aug 12, 2021
@Hywan Hywan added the 🎉 enhancement New feature! label Aug 12, 2021
@jcaesar
Copy link
Contributor Author

jcaesar commented Aug 17, 2021

All you need is two files

Looked into this a bit more: The C flag (use security context of interpreted binary - I think this mostly applies to SUID/SGID) requires the O flag (pre-open the interpreted binary so it can be read even if the mount namespace differs(?)). However, a script starting with #! itself needs another interpreter, and O will only allow one interpreted file. So for the wrapper/exec wasmer approach, it would be better to make the wrapper a normal ELF binary. (But I'm still not in favor of having a wrapper. ;))

@jcaesar
Copy link
Contributor Author

jcaesar commented Aug 27, 2021

Just had a look through the installer script, and… that's supposed to install something to ~/.wasmer. I'm a bit uncomfortable with installing a binfmt interpreter and a run-as-root systemd service for an executable in a user's home folder.
I can add a prompt at the beginning of the script going "Do you want to install wasmer system-wide? (Enables all users on the system to use wasmer, and directly executing wasm binaries.)". That would require renaming a few files in the release tarballs though (include/README.md, e.g.), and thus must be hidden behind a version check. All pretty nasty.
I'm tempted to say "For now, this is a feature either for individual user setup, or for distro installs." :/

@syrusakbary syrusakbary assigned Amanieu and unassigned Hywan Oct 28, 2021
@ptitSeb
Copy link
Contributor

ptitSeb commented Nov 5, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Nov 5, 2021

👎 Rejected by too few approved reviews

@ptitSeb ptitSeb requested review from ptitSeb and removed request for MarkMcCaskey November 5, 2021 10:29
@ptitSeb
Copy link
Contributor

ptitSeb commented Nov 5, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Nov 5, 2021

Build succeeded:

@bors bors bot merged commit d41a0c7 into wasmerio:master Nov 5, 2021
@syrusakbary syrusakbary added this to the v2.1 milestone Nov 5, 2021
@jcaesar jcaesar deleted the register-binfmt branch November 6, 2021 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎉 enhancement New feature!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants