Re: [PATCH] Add Zstandard support

[Dave Reisner <d@falconindy.com>]

On Tue, Aug 25, 2020 at 11:29:43AM -0700, Lucas De Marchi wrote:
> On Tue, Aug 25, 2020 at 10:49 AM Dave Reisner <d@falconindy.com> wrote:
> >
> > On Tue, Aug 25, 2020 at 10:12:57AM -0700, Lucas De Marchi wrote:
> > > Hi,
> > >
> > > On Fri, Aug 21, 2020 at 3:46 PM Torge Matthies
> > > <openglfreak@googlemail.com> wrote:
> > > >
> > > > I was not able to find any existing zstd patch for kmod, so I wrote my own.
> > >
> > > thanks a lot for adding this.
> > >
> > > >
> > > > I was struggling to get the C code below the 80 character line length limit,
> > > > that's why I used gotos. I used descriptive label names, and it still looks
> > > > clean enough in my opinion.
> > >
> > > Humn.. I'd rather prefer not to be on 80 chars than using some of the gotos,
> > > but take a look below
> > >
> > > >
> > > > I changed the style of the hackargs variable in autogen.sh to multiline
> > > > because said line was becoming a bit long with the new --with-zstd arg
> > > > added.
> > >
> > > ok
> > >
> > > >
> > > > This patch has been running on my two Arch Linux installations (with an
> > > > accompanying mkinitcpio patch) for several months over many kernel updates
> > > > without any issues.
> > >
> > > great. Is Arch already using zstd for the kernel? Once this lands, is
> > > it going to
> > > zstd for modules?
> > >
> > > +Dave
> > >
> >
> > No plans that I'm aware of. We currently use CONFIG_MODULE_COMPRESS_XZ=y
> > to have Kbuild do the module compression for us at build time. Is Kbuild
> > being updated to support zstd if/when this patch lands in a kmod
> > release?
> >
> > Personally, I'd rather see less module compression, not more. This
> > "flavor of the week" compression stuff makes me tired, and the way
> > compression is implemented in userspace inhibits more important features
> > like secure module loading via finit_module.
> 
> zstd is being added to the kernel as well:
> http://lkml.iu.edu/hypermail/linux/kernel/1710.1/02783.html

Yes, there's upstream support for compressing the kernel images (and
soon initramfs) with zstd, but the Kbuild process allows for compressing
*modules* as part of modules_install:

https://github.com/torvalds/linux/blob/master/Makefile#L1053

Granted, distros could just do this after the fact in their packaging
process, but it seems weird to make zstd an outlier and not have an
associated CONFIG_MODULE_COMPRESS_ZSTD Kbuild option.

dR

> I don't mind having the userspace implementation in kmod - adding any
> compression
> to the kernel has stalled over the years and IMO shouldn't prevent
> this going in.
> Most of distros are already using compressed modules, so it's not that
> adding another
> one would do any harm. It may even help to allow them to switch between formats
> without worrying about supporting older kernels since there would be a
> fallback in
> userspace if they are not willing to back port the kernel changs when
> that happens
> 
> Lucas De Marchi
> 
> >
> > dR
> >
> > > > Any additional testing and/or patch review would of course be appreciated.
> > > >
> > > > Signed-off-by: Torge Matthies <openglfreak@googlemail.com>
> > > > ---
> > > >  .semaphore/semaphore.yml     |   4 +-
> > > >  .travis.yml                  |   1 +
> > > >  Makefile.am                  |   4 +-
> > > >  autogen.sh                   |   9 ++-
> > > >  configure.ac                 |  13 +++-
> > > >  libkmod/libkmod-file.c       | 119 +++++++++++++++++++++++++++++++++++
> > > >  libkmod/libkmod.pc.in        |   2 +-
> > > >  shared/util.c                |   3 +
> > > >  testsuite/mkosi/mkosi.fedora |   1 +
> > > >  testsuite/test-util.c        |   3 +
> > > >  10 files changed, 153 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/.semaphore/semaphore.yml b/.semaphore/semaphore.yml
> > > > index db47ca1..fe1d78a 100644
> > > > --- a/.semaphore/semaphore.yml
> > > > +++ b/.semaphore/semaphore.yml
> > > > @@ -22,7 +22,7 @@ blocks:
> > > >        prologue:
> > > >          commands:
> > > >            - sudo apt update
> > > > -          - sudo apt --yes install docbook-xsl liblzma-dev zlib1g-dev cython linux-headers-generic libssl-dev
> > > > +          - sudo apt --yes install docbook-xsl libzstd-dev liblzma-dev zlib1g-dev cython linux-headers-generic libssl-dev
> > > >            - checkout
> > > >
> > > >        epilogue:
> > > > @@ -42,5 +42,5 @@ blocks:
> > > >        prologue:
> > > >          commands:
> > > >            - sudo apt update
> > > > -          - sudo apt --yes install docbook-xsl liblzma-dev zlib1g-dev cython linux-headers-generic libssl-dev
> > > > +          - sudo apt --yes install docbook-xsl libzstd-dev liblzma-dev zlib1g-dev cython linux-headers-generic libssl-dev
> > > >            - checkout
> > > > diff --git a/.travis.yml b/.travis.yml
> > > > index 02c753e..0fcce14 100644
> > > > --- a/.travis.yml
> > > > +++ b/.travis.yml
> > > > @@ -14,6 +14,7 @@ matrix:
> > > >  before_install:
> > > >    - sudo add-apt-repository ppa:ubuntu-toolchain-r/test -y
> > > >    - sudo apt-get update -qq
> > > > +  - sudo apt-get install -qq libzstd-dev
> > > >    - sudo apt-get install -qq liblzma-dev
> > > >    - sudo apt-get install -qq zlib1g-dev
> > > >    - sudo apt-get install -qq xsltproc docbook-xsl
> > > > diff --git a/Makefile.am b/Makefile.am
> > > > index 8eadb99..37d840b 100644
> > > > --- a/Makefile.am
> > > > +++ b/Makefile.am
> > > > @@ -31,6 +31,8 @@ SED_PROCESS = \
> > > >         -e 's,@exec_prefix\@,$(exec_prefix),g' \
> > > >         -e 's,@libdir\@,$(libdir),g' \
> > > >         -e 's,@includedir\@,$(includedir),g' \
> > > > +       -e 's,@libzstd_CFLAGS\@,${libzstd_CFLAGS},g' \
> > > > +       -e 's,@libzstd_LIBS\@,${libzstd_LIBS},g' \
> > > >         -e 's,@liblzma_CFLAGS\@,${liblzma_CFLAGS},g' \
> > > >         -e 's,@liblzma_LIBS\@,${liblzma_LIBS},g' \
> > > >         -e 's,@zlib_CFLAGS\@,${zlib_CFLAGS},g' \
> > > > @@ -90,7 +92,7 @@ libkmod_libkmod_la_DEPENDENCIES = \
> > > >         ${top_srcdir}/libkmod/libkmod.sym
> > > >  libkmod_libkmod_la_LIBADD = \
> > > >         shared/libshared.la \
> > > > -       ${liblzma_LIBS} ${zlib_LIBS} ${libcrypto_LIBS}
> > > > +       ${libzstd_LIBS} ${liblzma_LIBS} ${zlib_LIBS} ${libcrypto_LIBS}
> > > >
> > > >  noinst_LTLIBRARIES += libkmod/libkmod-internal.la
> > > >  libkmod_libkmod_internal_la_SOURCES = $(libkmod_libkmod_la_SOURCES)
> > > > diff --git a/autogen.sh b/autogen.sh
> > > > index 67b119f..e4997c4 100755
> > > > --- a/autogen.sh
> > > > +++ b/autogen.sh
> > > > @@ -32,7 +32,14 @@ fi
> > > >
> > > >  cd $oldpwd
> > > >
> > > > -hackargs="--enable-debug --enable-python --with-xz --with-zlib --with-openssl"
> > > > +hackargs="\
> > > > +--enable-debug \
> > > > +--enable-python \
> > > > +--with-zstd \
> > > > +--with-xz \
> > > > +--with-zlib \
> > > > +--with-openssl \
> > > > +"
> > > >
> > > >  if [ "x$1" = "xc" ]; then
> > > >          shift
> > > > diff --git a/configure.ac b/configure.ac
> > > > index 4a65d6b..a49f6b9 100644
> > > > --- a/configure.ac
> > > > +++ b/configure.ac
> > > > @@ -83,6 +83,17 @@ AC_ARG_WITH([rootlibdir],
> > > >          [], [with_rootlibdir=$libdir])
> > > >  AC_SUBST([rootlibdir], [$with_rootlibdir])
> > > >
> > > > +AC_ARG_WITH([zstd],
> > > > +       AS_HELP_STRING([--with-zstd], [handle Zstandard-compressed modules @<:@default=disabled@:>@]),
> > > > +       [], [with_zstd=no])
> > > > +AS_IF([test "x$with_zstd" != "xno"], [
> > > > +       PKG_CHECK_MODULES([libzstd], [libzstd >= 1.4.4])
> > > > +       AC_DEFINE([ENABLE_ZSTD], [1], [Enable Zstandard for modules.])
> > > > +], [
> > > > +       AC_MSG_NOTICE([Zstandard support not requested])
> > > > +])
> > > > +CC_FEATURE_APPEND([with_features], [with_zstd], [ZSTD])
> > > > +
> > > >  AC_ARG_WITH([xz],
> > > >         AS_HELP_STRING([--with-xz], [handle Xz-compressed modules @<:@default=disabled@:>@]),
> > > >         [], [with_xz=no])
> > > > @@ -307,7 +318,7 @@ AC_MSG_RESULT([
> > > >         tools:                  ${enable_tools}
> > > >         python bindings:        ${enable_python}
> > > >         logging:                ${enable_logging}
> > > > -       compression:            xz=${with_xz}  zlib=${with_zlib}
> > > > +       compression:            zstd=${with_zstd}  xz=${with_xz}  zlib=${with_zlib}
> > > >         debug:                  ${enable_debug}
> > > >         coverage:               ${enable_coverage}
> > > >         doc:                    ${enable_gtk_doc}
> > > > diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
> > > > index 5eeba6a..1a2e753 100644
> > > > --- a/libkmod/libkmod-file.c
> > > > +++ b/libkmod/libkmod-file.c
> > > > @@ -26,6 +26,9 @@
> > > >  #include <sys/stat.h>
> > > >  #include <sys/types.h>
> > > >  #include <unistd.h>
> > > > +#ifdef ENABLE_ZSTD
> > > > +#include <zstd.h>
> > > > +#endif
> > > >  #ifdef ENABLE_XZ
> > > >  #include <lzma.h>
> > > >  #endif
> > > > @@ -45,6 +48,9 @@ struct file_ops {
> > > >  };
> > > >
> > > >  struct kmod_file {
> > > > +#ifdef ENABLE_ZSTD
> > > > +       bool zstd_used;
> > > > +#endif
> > > >  #ifdef ENABLE_XZ
> > > >         bool xz_used;
> > > >  #endif
> > > > @@ -60,6 +66,116 @@ struct kmod_file {
> > > >         struct kmod_elf *elf;
> > > >  };
> > > >
> > > > +#ifdef ENABLE_ZSTD
> > > > +static int load_zstd(struct kmod_file *file)
> > > > +{
> > > > +       ZSTD_DStream *dstr = NULL;
> > > > +       size_t in_buf_size, out_buf_min_size, out_buf_size;
> > > > +       uint8_t *in_buf = NULL, *out_buf = NULL;
> > > > +       ZSTD_inBuffer zst_input;
> > > > +       ZSTD_outBuffer zst_output;
> > > > +       int ret = -EINVAL;
> > > > +
> > > > +       dstr = ZSTD_createDStream();
> > > > +       if (dstr == NULL) {
> > > > +               ERR(file->ctx, "zstd: Failed to create decompression stream\n");
> > > > +               ret = -EINVAL;
> > > > +               goto out;
> > > > +       }
> > > > +
> > > > +       in_buf_size = ZSTD_initDStream(dstr);
> > > > +       in_buf = malloc(in_buf_size);
> > > > +       if (in_buf == NULL) {
> > > > +               ERR(file->ctx, "zstd: %s\n", strerror(ENOMEM));
> > >
> > > no big deal, but malloc() should set errno, so you could use
> > > ret = -errno before printing and then use %m in the message.
> > >
> > > Same thing in other places.
> > >
> > > > +               ret = -ENOMEM;
> > > > +               goto out;
> > > > +       }
> > > > +
> > > > +       out_buf_size = out_buf_min_size = ZSTD_DStreamOutSize();
> > > > +       out_buf = malloc(out_buf_size);
> > > > +       if (out_buf == NULL) {
> > > > +               ERR(file->ctx, "zstd: %s\n", strerror(ENOMEM));
> > > > +               ret = -ENOMEM;
> > > > +               goto out;
> > > > +       }
> > > > +
> > > > +       zst_input.src = in_buf;
> > > > +       zst_output.dst = out_buf;
> > > > +       zst_output.size = out_buf_size;
> > > > +       zst_output.pos = 0;
> > > > +       while (true) {
> > > > +               ssize_t rdret;
> > > > +               size_t dsret;
> > > > +               uint8_t *tmp;
> > > > +
> > > > +               rdret = read(file->fd, in_buf, in_buf_size);
> > > > +               if (rdret < 0) {
> > > > +                       ret = -errno;
> > > > +                       goto out;
> > > > +               }
> > > > +               if (rdret == 0)
> > > > +                       break;
> > > > +
> > > > +               zst_input.size = rdret;
> > > > +               zst_input.pos = 0;
> > > > +
> > > > +               if (zst_output.size - zst_output.pos < out_buf_min_size)
> > > > +                       goto realloc_buffer;
> > >
> > > this could be a helper function to realloc the buffer. It can even be shared
> > > for the first alloc if you initialize zst_output.size to 0.. Then you
> > > have something like:
> > >
> > > > +
> > > > +try_decompress:
> > > > +               dsret = ZSTD_decompressStream(dstr, &zst_output, &zst_input);
> > > > +               if (ZSTD_isError(dsret)) {
> > > > +                       ERR(file->ctx, "zstd: %s\n", ZSTD_getErrorName(dsret));
> > > > +                       ret = -EINVAL;
> > > > +                       goto out;
> > > > +               }
> > > > +
> > > > +               if (zst_output.pos < zst_output.size
> > > > +                       && zst_output.size - zst_output.pos
> > > > +                       >= out_buf_min_size) {
> > > > +                       if (zst_input.pos < zst_input.size)
> > > > +                               goto try_decompress;
> > >
> > > this looks like a while/for loop. Could even be in a helper function
> > >
> > >
> > > > +                       else
> > > > +                               continue;
> > > > +               }
> > > > +
> > > > +realloc_buffer:
> > > > +               tmp = realloc(out_buf, out_buf_size + out_buf_min_size);
> > > > +               if (tmp == NULL) {
> > > > +                       ret = -errno;
> > > > +                       goto out;
> > > > +               }
> > > > +               out_buf_size += out_buf_min_size;
> > > > +               out_buf = tmp;
> > > > +               zst_output.dst = out_buf;
> > > > +               zst_output.size = out_buf_size;
> > > > +               goto try_decompress;
> > > > +       }
> > > > +
> > > > +       ZSTD_freeDStream(dstr);
> > > > +       free(in_buf);
> > > > +       file->zstd_used = true;
> > > > +       file->memory = out_buf;
> > > > +       file->size = zst_output.pos;
> > > > +       return 0;
> > > > +out:
> > > > +       if (dstr != NULL)
> > > > +               ZSTD_freeDStream(dstr);
> > > > +       free(out_buf);
> > > > +       free(in_buf);
> > > > +       return ret;
> > > > +}
> > > > +
> > > > +static void unload_zstd(struct kmod_file *file)
> > > > +{
> > > > +       if (!file->zstd_used)
> > > > +               return;
> > > > +       free(file->memory);
> > > > +}
> > > > +
> > > > +static const char magic_zstd[] = {0x28, 0xB5, 0x2F, 0xFD};
> > > > +#endif
> > > > +
> > > >  #ifdef ENABLE_XZ
> > > >  static void xz_uncompress_belch(struct kmod_file *file, lzma_ret ret)
> > > >  {
> > > > @@ -238,6 +354,9 @@ static const struct comp_type {
> > > >         const char *magic_bytes;
> > > >         const struct file_ops ops;
> > > >  } comp_types[] = {
> > > > +#ifdef ENABLE_ZSTD
> > > > +       {sizeof(magic_zstd), magic_zstd, {load_zstd, unload_zstd}},
> > > > +#endif
> > > >  #ifdef ENABLE_XZ
> > > >         {sizeof(magic_xz), magic_xz, {load_xz, unload_xz}},
> > > >  #endif
> > > > diff --git a/libkmod/libkmod.pc.in b/libkmod/libkmod.pc.in
> > > > index e4fdf21..3acca6a 100644
> > > > --- a/libkmod/libkmod.pc.in
> > > > +++ b/libkmod/libkmod.pc.in
> > > > @@ -7,5 +7,5 @@ Name: libkmod
> > > >  Description: Library to deal with kernel modules
> > > >  Version: @VERSION@
> > > >  Libs: -L${libdir} -lkmod
> > > > -Libs.private: @liblzma_LIBS@ @zlib_LIBS@
> > > > +Libs.private: @libzstd_LIBS@ @liblzma_LIBS@ @zlib_LIBS@
> > > >  Cflags: -I${includedir}
> > > > diff --git a/shared/util.c b/shared/util.c
> > > > index fd2028d..b487b5f 100644
> > > > --- a/shared/util.c
> > > > +++ b/shared/util.c
> > > > @@ -45,6 +45,9 @@ static const struct kmod_ext {
> > > >  #endif
> > > >  #ifdef ENABLE_XZ
> > > >         {".ko.xz", sizeof(".ko.xz") - 1},
> > > > +#endif
> > > > +#ifdef ENABLE_ZSTD
> > > > +       {".ko.zst", sizeof(".ko.zst") - 1},
> > > >  #endif
> > > >         { }
> > > >  };
> > > > diff --git a/testsuite/mkosi/mkosi.fedora b/testsuite/mkosi/mkosi.fedora
> > > > index 5252f87..7a2ee5e 100644
> > > > --- a/testsuite/mkosi/mkosi.fedora
> > > > +++ b/testsuite/mkosi/mkosi.fedora
> > > > @@ -19,6 +19,7 @@ BuildPackages =
> > > >         make
> > > >         pkgconf-pkg-config
> > > >         xml-common
> > > > +       libzstd-devel
> > > >         xz-devel
> > > >         zlib-devel
> > > >         openssl-devel
> > > > diff --git a/testsuite/test-util.c b/testsuite/test-util.c
> > > > index 5e25e58..621446b 100644
> > > > --- a/testsuite/test-util.c
> > > > +++ b/testsuite/test-util.c
> > > > @@ -156,6 +156,9 @@ static int test_path_ends_with_kmod_ext(const struct test *t)
> > > >  #endif
> > > >  #ifdef ENABLE_XZ
> > > >                 { "/bla.ko.xz", true },
> > > > +#endif
> > > > +#ifdef ENABLE_ZSTD
> > > > +               { "/bla.ko.zst", true },
> > >
> > > I think we want a zstd-compressed module in the testsuite as well to test this.
> > >
> > > thanks
> > > Lucas De Marchi
> > >
> > > >  #endif
> > > >                 { "/bla.ko.x", false },
> > > >                 { "/bla.ko.", false },
> > > > --
> > > > 2.28.0
> > > >