libct/dmz: Reduce the binary size using nolibc #4024

rata · 2023-09-25T10:44:57Z

Linux repo has under tools/include/nolibc very simple include files that we can use to generate very small binaries that don't depend on libc.

To make things even better, since Linux 6.6 it supports all the architectures we support in runc, which is just beautiful.

The runc-dmz binary on x86_64 before this patch (on my debian host) was taking 636K, with this patch it takes only 8K.

Please review this table of arches with special attention.

Here is a list of arches we currently produce a binary for (took the list from here https://github.com/opencontainers/runc/releases/tag/v1.1.9), I tried to map them to the equivalent arch in nolibc, to make sure we still support all arches. Please review this

amd64: yes, nolibc's arch-x86_64.h
armel (aarch64): yes, nolibc's arch-arm.h/arch-aarch64.h
armf: yes, nolibc's arch-arm.h
ppc64le: yes, nolibc's arch-powerpc.h
riscv64: yes, nolibc's arch-riscv.h
s390x: yes, nolibc's arch-s390.h

If some arch is not supported, we should fallback to use stdlib for that arch. But I think all of them are supported.

~~What golang seems to support and nolibc doesn't is MIPS (mips, mipsle, mipsle64, mips64). So we might want to do the fallback anyways? :-(~~ ~~Yes, arch-mips.h is supported, but it seems not all those variants.~~ It seems runc doesn't compile for mips, as shown here

cc @cyphar @lifubang

cyphar · 2023-09-25T10:48:18Z

Oops, misclicked. I'll review this tomorrow.

fuweid · 2023-09-25T12:36:03Z

libcontainer/dmz/nolibc/arch-aarch64.h

+/* SPDX-License-Identifier: LGPL-2.1 OR MIT */
+/*
+ * AARCH64 specific definitions for NOLIBC
+ * Copyright (C) 2017-2022 Willy Tarreau <w@1wt.eu>


Does it have license issue?
Not expert . Just thinking about how it lives with Apache 2.0.

It says it can be MIT, which should be compatible with Apache-2.

See here: https://law.stackexchange.com/questions/6081/can-i-bundle-mit-licensed-components-in-a-apache-2-0-licensed-project

Am I missing something?

Yes. I think we can just keep MIT and remove the LGPL-2.1. Just remove the confusion

Idem as the other comment. I think I prefer to keep it if we can keep it. Otherwise, updating to a newer version of nolibc in the future would be more painful (or we need to make a script that nolibc, removes this, maybe something else, etc.).

I think there is value in the simplicity of just a simple c&p, no patches added.

We cannot remove license headers. The code is dual-licensed but you need to keep the original copyright headers verbatim.

@cyphar thanks for the comment!

rata · 2023-09-25T14:12:20Z

libcontainer/dmz/nolibc/Makefile

@@ -0,0 +1,91 @@
+# SPDX-License-Identifier: GPL-2.0


I think it is okay to include this file is GPLv2, but if someone can confirm it will be great.

Otherwise, we can remove it? We are not using this makefile anyways.

Should be removed to avoid confusion (If we are going to merge this PR)

I think I prefer to keep it if we can keep it. Otherwise, updating to a newer version of nolibc in the future would be more painful (or we need to make a script that takes that, removes this, maybe something else, etc.).

I think there is value in the simplicity of just a simple c&p.

I think there is value in the simplicity of just a simple c&p.

The problem is that this is legally less simple.
It is not obvious (for non-maintainers) that the GPL-2.0 code is actually not linked with runc.

Fixed, then! :)

libcontainer/dmz/_dmz.c

AkihiroSuda · 2023-09-25T20:03:26Z

Alternative:

libct/dmz: Reduce the binary size by removing libc dependency #4026

eiffel-fl

This nolibc is really interesting :D!
I have a comment on the xstat.h file:

eiffel-fl · 2023-09-26T09:18:12Z

libcontainer/dmz/xstat.h

+#ifndef XSTAT_H
+#define XSTAT_H
+
+// Some old-kernels (like centos-7) don't have statx() defined in linux/stat.h. We can't include
+// sys/stat.h because it creates conflicts, so let's just define what we need here and be done with
+// this.
+// TODO (rata): I'll probably submit a patch to nolibc upstream so we can remove this hack in the
+// future.
+#include <linux/stat.h>  /* for statx() */
+
+#ifndef STATX_BASIC_STATS
+#include "linux/stat.h"
+#endif // STATX_BASIC_STATS
+
+#endif // XSTAT_H


I get your explanation regarding the problem.
But why do you have to include this file for dmz?
I mean, dmz only calls execve() which directly calls the syscall.
Moreover, statx() is defned in the nolibc.

Because on centos-7, the <linux/stat.h> header doesn't include the struct statx that nolibc uses in sys.h, where execve() is defined.

I got it! Thank you!
So, the upstream patch would be about using the nolibc stat.h in sys.h rather than linux/stat.h?
In any case, please send it!

Yes, that is the idea. But keeping nolibc unmodified here is way simpler that carrying out our patches to it. So, the xstat.h in runc seems way better than patching nolibc IMHO :)

Sure!
Once your patch would be accepted upstream, you could update the code here but at the moment this is fine.

AkihiroSuda

Please remove GPL-2.0 code
https://github.com/opencontainers/runc/pull/4024/files#r1337136436

rata · 2023-09-26T12:43:40Z

@AkihiroSuda sure, changed it. Removed that and put a note on the README about it. PTAL

AkihiroSuda · 2023-09-26T12:50:15Z

libcontainer/dmz/Makefile

@@ -2,5 +2,5 @@
 include ../../cc_platform.mk

 runc-dmz: _dmz.c
-	$(CC) $(CFLAGS) -static -o $@ $^
+	$(CC) $(CFLAGS) -fno-asynchronous-unwind-tables -fno-ident -s -Os -nostdlib -lgcc -static -o $@ $^


Could you add comments to explain these flags?
Why do we still need lgcc?

I've added a comment to explain that, it is what it is suggested in nolibc/nolibc.h: https://github.com/opencontainers/runc/pull/4024/files#diff-bc183fb7b7c25ee341d9ef03b3bcb444301270a7ef72291bc025d96f1d7046f1R66-R68

It works without -lgcc, but in nolib.h they put it and the gcc manpage says that you need to add that if you use notstdlib to make sure the linking won't fail.

Linux repo has under `tools/include/nolibc` very simple include files that we can use to generate very small binaries that don't depend on libc. To make things even better, since Linux 6.6 it supports all the architectures we support in runc, which is just beautiful. The runc-dmz binary on x86_64 before this patch (on my debian host) was taking 636K, with this patch it takes only 8K. Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>

lifubang · 2023-09-26T13:36:58Z

@rata I think the source code of nolibc should not be part of runc, because we don't know nolibc's version, and very difficult to update its version. Maybe we can use git submodule or add a configure script to copy it from torvalds/linux?

rata · 2023-09-26T13:40:01Z

@lifubang I don't know a decent way to do that. Submodules seems painful, as we can't add a sub-folder (and adding the whole linux repo seems crazy), the configure script you mention seems complex too... I don't think we will need to update this often, if at all.

Would it be okay to add the script if we happen to update this often?

lifubang · 2023-09-26T14:04:55Z

the configure script you mention seems complex too

How about https://git-scm.com/docs/git-archive ?

<path>
Without an optional path parameter, all files and subdirectories of the current working directory are included in the archive. If one or more paths are specified, only these are included.

lifubang · 2023-09-26T14:19:24Z

Or https://www.git-scm.com/docs/git-sparse-checkout ?
If these are very complex, I think we can hear other people's opinion.

rata · 2023-09-26T14:48:09Z

@lifubang hmmm, git-archive is to create an archive like tar/zip/gzip. Am I missing something? We don't want that here.

IMHO trying to use submodules with sparse checkout is not worth it at this point. Not sure if it is possible (I'm not familiar with sparse checkout), but IMHO it doesn't seem worth the effort now. We can definitely change it if we do need to update this more than once per year.

cyphar · 2023-09-26T16:13:30Z

It seems very unlikely we will need to update this at all, let alone frequently -- given how minimal runc-dmz is and the fact that syscall numbers are fixed per-architecture. Once it works, it should just continue to work without any changes needed.

The alternatives (submodules and subtree merges) have their own issues. If you use submodules, nolibc will not be included in git archive which will mean our release tarballs won't compile and we will need to separately ship nolibc.tar (or do some hacks to add it to the release tarball). subtrees mean our git history will be needlessly polluted with stuff from Linux upstream that we don't care about. AFAIK git sparse-checkout doesn't help here either. The simplest solution is to just copy-paste the code. If we end up needing to update this often, we can reconsider our options (as @rata suggested).

cyphar

LGTM.

AkihiroSuda · 2023-09-26T16:25:19Z

libcontainer/dmz/nolibc/arch.h

+#elif defined(__aarch64__)
+#include "arch-aarch64.h"
+#elif defined(__mips__) && defined(_ABIO32)
+#include "arch-mips.h"


Any reason for not supporting MIPS64? (Is it still alive?)

Runc currently doesn't support mips:

runc/cc_platform.mk

Lines 17 to 53 in 1a8b558

else ifeq ($(GOARCH),386)

# Always use the 64-bit compiler to build the 386 binary, which works for

# the more common cross-build method for x86 (namely, the equivalent of

# dpkg --add-architecture).

ifdef IS_SUSE

# There is no x86_64-suse-linux-gcc, so use the native one.

HOST :=

CPU_TYPE := i586

else

HOST := x86_64-$(PLATFORM)-

CPU_TYPE := i686

endif

CFLAGS := -m32 -march=$(CPU_TYPE) $(CFLAGS)

else ifeq ($(GOARCH),amd64)

ifdef IS_SUSE

# There is no x86_64-suse-linux-gcc, so use the native one.

HOST :=

else

HOST := x86_64-$(PLATFORM)-

endif

else ifeq ($(GOARCH),arm64)

HOST := aarch64-$(PLATFORM)-

else ifeq ($(GOARCH),arm)

# HOST already configured by release_build.sh in this case.

else ifeq ($(GOARCH),armel)

HOST := arm-$(PLATFORM)eabi-

else ifeq ($(GOARCH),armhf)

HOST := arm-$(PLATFORM)eabihf-

else ifeq ($(GOARCH),ppc64le)

HOST := powerpc64le-$(PLATFORM)-

else ifeq ($(GOARCH),riscv64)

HOST := riscv64-$(PLATFORM)-

else ifeq ($(GOARCH),s390x)

HOST := s390x-$(PLATFORM)-

else

$(error Unsupported GOARCH $(GOARCH))

endif

In a quick git blame on the linux kernel, I didn't find why

rata · 2023-09-26T16:32:28Z

@cyphar cool! I guess you checked the arches (what I said in the PR description) and looks good? :)

cyphar · 2023-09-26T16:38:29Z

Yeah, it supports all of the architectures we do releases for. If we want to add MIPS64 releases in the future we can cross that bridge when we get to it (or someone complains).

rata · 2023-09-26T16:40:21Z

Thanks, SGTM

eiffel-fl · 2023-09-27T05:56:10Z

Awesome contribution 🎉 🎉 🎉!

lifubang · 2023-09-27T06:02:40Z

Awesome contribution

Yes, exactly, I get a 4.8K runc-dmz in my machine.

cyphar enabled auto-merge September 25, 2023 10:47

cyphar disabled auto-merge September 25, 2023 10:47

rata force-pushed the rata/smaller-runc-dmz branch 4 times, most recently from 16b4e76 to 88f9c0d Compare September 25, 2023 12:22

fuweid reviewed Sep 25, 2023

View reviewed changes

rata force-pushed the rata/smaller-runc-dmz branch 2 times, most recently from e30851c to 5578e95 Compare September 25, 2023 14:08

rata commented Sep 25, 2023

View reviewed changes

rata force-pushed the rata/smaller-runc-dmz branch 2 times, most recently from 01cbee2 to 3ff27ee Compare September 25, 2023 15:26

AkihiroSuda reviewed Sep 25, 2023

View reviewed changes

libcontainer/dmz/_dmz.c Outdated Show resolved Hide resolved

AkihiroSuda mentioned this pull request Sep 25, 2023

libct/dmz: Reduce the binary size by removing libc dependency #4026

Closed

9 tasks

AkihiroSuda mentioned this pull request Sep 25, 2023

[Refactor] Reduce runc-dmz binary size #4027

Closed

eiffel-fl reviewed Sep 26, 2023

View reviewed changes

rata force-pushed the rata/smaller-runc-dmz branch from 875a48e to 8220ad2 Compare September 26, 2023 12:02

AkihiroSuda requested changes Sep 26, 2023

View reviewed changes

rata force-pushed the rata/smaller-runc-dmz branch from 8220ad2 to f6a930a Compare September 26, 2023 12:42

AkihiroSuda reviewed Sep 26, 2023

View reviewed changes

rata force-pushed the rata/smaller-runc-dmz branch from f6a930a to 90f5da6 Compare September 26, 2023 12:55

AkihiroSuda approved these changes Sep 26, 2023

View reviewed changes

cyphar approved these changes Sep 26, 2023

View reviewed changes

cyphar merged commit 1a8b558 into opencontainers:main Sep 26, 2023
45 checks passed

AkihiroSuda reviewed Sep 26, 2023

View reviewed changes

rata deleted the rata/smaller-runc-dmz branch September 26, 2023 16:27

rata mentioned this pull request Sep 26, 2023

libct/dmz: Move comment out of the Makefile rule #4030

Merged

AkihiroSuda mentioned this pull request Sep 26, 2023

docs: clarify the supported architectures (No MIPS) #4031

Merged

kolyshkin added this to the 1.2.0 milestone Sep 26, 2023

	else ifeq ($(GOARCH),386)
	# Always use the 64-bit compiler to build the 386 binary, which works for
	# the more common cross-build method for x86 (namely, the equivalent of
	# dpkg --add-architecture).
	ifdef IS_SUSE
	# There is no x86_64-suse-linux-gcc, so use the native one.
	HOST :=
	CPU_TYPE := i586
	else
	HOST := x86_64-$(PLATFORM)-
	CPU_TYPE := i686
	endif
	CFLAGS := -m32 -march=$(CPU_TYPE) $(CFLAGS)
	else ifeq ($(GOARCH),amd64)
	ifdef IS_SUSE
	# There is no x86_64-suse-linux-gcc, so use the native one.
	HOST :=
	else
	HOST := x86_64-$(PLATFORM)-
	endif
	else ifeq ($(GOARCH),arm64)
	HOST := aarch64-$(PLATFORM)-
	else ifeq ($(GOARCH),arm)
	# HOST already configured by release_build.sh in this case.
	else ifeq ($(GOARCH),armel)
	HOST := arm-$(PLATFORM)eabi-
	else ifeq ($(GOARCH),armhf)
	HOST := arm-$(PLATFORM)eabihf-
	else ifeq ($(GOARCH),ppc64le)
	HOST := powerpc64le-$(PLATFORM)-
	else ifeq ($(GOARCH),riscv64)
	HOST := riscv64-$(PLATFORM)-
	else ifeq ($(GOARCH),s390x)
	HOST := s390x-$(PLATFORM)-
	else
	$(error Unsupported GOARCH $(GOARCH))
	endif

libct/dmz: Reduce the binary size using nolibc #4024

libct/dmz: Reduce the binary size using nolibc #4024

Conversation

rata commented Sep 25, 2023 • edited Loading

cyphar commented Sep 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda commented Sep 25, 2023

eiffel-fl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rata Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda left a comment

Choose a reason for hiding this comment

rata commented Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lifubang commented Sep 26, 2023

rata commented Sep 26, 2023 • edited Loading

lifubang commented Sep 26, 2023 • edited Loading

lifubang commented Sep 26, 2023

rata commented Sep 26, 2023

cyphar commented Sep 26, 2023

cyphar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rata commented Sep 26, 2023 • edited Loading

cyphar commented Sep 26, 2023

rata commented Sep 26, 2023

eiffel-fl commented Sep 27, 2023

lifubang commented Sep 27, 2023

rata commented Sep 25, 2023 •

edited

Loading

rata Sep 26, 2023 •

edited

Loading

rata commented Sep 26, 2023 •

edited

Loading

rata commented Sep 26, 2023 •

edited

Loading

lifubang commented Sep 26, 2023 •

edited

Loading

rata commented Sep 26, 2023 •

edited

Loading