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

Support multiple initrd compression formats (non-gzip compression formats currently cause kernel crashes in worst case scenario) #4683

Closed
ericcurtin opened this issue Nov 2, 2023 · 13 comments · Fixed by #4705

Comments

@ericcurtin
Copy link
Contributor

ericcurtin commented Nov 2, 2023

Host system details

rpm-ostree status
State: idle
Deployments:
● auto-sig:cs9/aarch64/qemu-developer
                  Version: 9 (2023-11-02T10:41:30Z)
                   Commit: ad000b8c336295604353805222eee32a8cbd9dd6123807f490dea4114458e339

Expected

# rpm-ostree compose postprocess

with lz4 configured via:

/usr/lib/dracut/dracut.conf.d

of passed via parameter:

--lz4

--compress -l -2

System boots.

** Actual

# rpm-ostree compose postprocess

with lz4 configured via:

/usr/lib/dracut/dracut.conf.d

of passed via parameter:

--lz4

--compress -l -2

Kernel either displays decoding error on decompression or panics.

You can also see the decoding error via userspace lz4 tool.

Steps forward

I scratched my head for a day or two on this issue. My interpretation is that it boils down to the appending of the:

dracut-random.cpio.gz

data towards the end of rpmostree_run_dracut.

We could do this in dracut, or compress all the cpio data ourselves in rpm-ostree (if we compress ourselves we don't have to dump dracut-random.cpio.gz, dracut-random.cpio.zstd, dracut-random.cpio.lz4, etc. just shell out and pipe to compression tool maybe?).

Encountered in Red Hat/CentOS Stream Automotive distributions as we changed the initramfs compression algorithm to lz4 (it boots faster from our testing) and were encountering non-booting systems, etc.

Not really related to the issue at all, but we started the effort to support more compression formats here:

#3745

@ericcurtin ericcurtin changed the title Support multiple initrd compression formats (alternate compression formats currently cause kernel crashes in worst case scenario) Support multiple initrd compression formats (non-gzip compression formats currently cause kernel crashes in worst case scenario) Nov 2, 2023
@ericcurtin
Copy link
Contributor Author

ericcurtin commented Nov 2, 2023

The most appropriate fix would seem to be to have dracut handle compression of initrd and the additional random data.

Did we ever teach dracut how to do this as suggested in #1950 @jlebon @cgwalters ? Or is there a dracut ticket opened?

@ericcurtin
Copy link
Contributor Author

ericcurtin commented Nov 2, 2023

Issue as a one-liner:

$ sudo /bin/bash -c "dracut -f $PWD/initramfs.img --lz4; cat ./src/libpriv/dracut-random.cpio.gz >> $PWD/initramfs.img; lz4 --test $PWD/initramfs.img;"
Stream followed by undecodable data at position 21716923
/root/initramfs.img  : decoded 42743296 bytes

Working version (with gzip):

$ sudo /bin/bash -c "dracut -f $PWD/initramfs.img --gzip; cat ./src/libpriv/dracut-random.cpio.gz >> $PWD/initramfs.img; gzip --test $PWD/initramfs.img;"

@cgwalters
Copy link
Member

dracut-random.cpio.gz should go away, see dracutdevs/dracut#2331

@ericcurtin
Copy link
Contributor Author

ericcurtin commented Nov 27, 2023

I have a question @cgwalters , does the dracut-random.cpio.gz file provide value in it's current form? Could we just remove that from being appended to the initrd for now? It's not random anyway in it's current form...

@cgwalters
Copy link
Member

I have a question @cgwalters , does the dracut-random.cpio.gz file provide value in it's current form? Could we just remove that from being appended to the initrd for now?

We added it for a reason:

It's not random anyway in it's current form...

The goal isn't that the data there is random...the goal is that it provides the /dev/{u,}random devices in the generated initramfs reliably, even if we're building the initramfs in a container that doesn't have permission to create those devices at build time.

@cgwalters
Copy link
Member

In the short term, we can do two things here:

  • Detect the case where we do have the capability to make the devices and don't append the extra cpio blob
  • Detect the compression format used by dracut and match it

@ericcurtin
Copy link
Contributor Author

In the short term, we can do two things here:

  • Detect the case where we do have the capability to make the devices and don't append the extra cpio blob
  • Detect the compression format used by dracut and match it

What we could do is:

  1. In the rust code:

Read the first 6 bytes of the initrd, if it is not "gzip" (like in the example shell script from lsinitrd below), set a compressor variable as "lz4 -c", "zstd -c", etc.

  1. Shell out:

gunzip -c dracut-random.cpio.gz | whatever_is_in_compressor_variable

Example of detection from lsinitrd:

if [[ $SKIP ]]; then
    bin="$($SKIP "$image" | { read -r -N 6 bin && echo "$bin"; })"
else
    read -r -N 6 bin < "$image"
fi
case $bin in
    $'\x1f\x8b'*)
        CAT="zcat --"
        ;;
    BZh*)
        CAT="bzcat --"
        ;;
    $'\x71\xc7'* | 070701)
        CAT="cat --"
        ;;
    $'\x02\x21'*)
        CAT="lz4 -d -c"
        ;;
    $'\x89'LZO$'\0'*)
        CAT="lzop -d -c"
        ;;
    $'\x28\xB5\x2F\xFD'*)
        CAT="zstd -d -c"
        ;;
    *)
        if echo "test" | xz | xzcat --single-stream > /dev/null 2>&1; then
            CAT="xzcat --single-stream --"
        else
            CAT="xzcat --"
        fi
        ;;
esac

What do you think? I think we might as well match it, if we have to detect the compression format, the easier part is matching it.

@jlebon
Copy link
Member

jlebon commented Nov 27, 2023

Can you provide the exact error message from the kernel?

@ericcurtin
Copy link
Contributor Author

ericcurtin commented Nov 27, 2023

I'll paste it here tomorrow, the exact one, off the top of my head it might have been "decoding failed"... That was the good case, getting the kernel log, in that case it would just ignore the unrecognisable gzip data and boot anyway.... In the worse case, you wouldn't even see that log, the kernel would freak out and panic tagging @masneyb for awareness... This is easy to reproduce by the way, just use a lz4 compressed initrd with ostree... This code assumes all initrd's are compressed with gzip, which is actually quite a dated compression algorithm, not sure why we are using that in RHEL or Fedora as default for initrd, seems like we are just using it because it's what's always been used.

@ericcurtin
Copy link
Contributor Author

In the healthier case, this is the kernel log:

Initramfs unpacking failed: Decoding failed

@ericcurtin
Copy link
Contributor Author

ericcurtin commented Nov 28, 2023

This is not working, tested or complete:

https://github.com/coreos/rpm-ostree/pull/4702/files

but this is what I'm thinking... It makes sense to shell out here as it's too much maintenance to integrate all the compression/decompression formats in rpm-ostree directly... Easier to just let the command line compressors/decompressors do their job.

cgwalters added a commit to cgwalters/rpm-ostree that referenced this issue Nov 28, 2023
A hidden environment variable to allow callers to work around
coreos#4683
@cgwalters
Copy link
Member

cgwalters commented Nov 28, 2023

I did #4704 which will allow you to opt-out of this for now.

Now,

Detect the case where we do have the capability to make the devices and don't append the extra cpio blob

would be better but needs some grunt work to ensure we're detecting things correctly.

jlebon added a commit to jlebon/rpm-ostree that referenced this issue Nov 28, 2023
Any arbitrary amount of padding between concatenated CPIOs is allowed:

https://www.kernel.org/doc/Documentation/early-userspace/buffer-format.txt

However, some initramfs decompressors in the kernel expect a minimal
amount of padding to detect EOF and allow the kernel outer loop to try
out other decompressors for the next payload. This is the case for lz4.
This upstream patch allows the lz4 decompressor to treat padding as EOF:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2c484419efc09e7234c667aa72698cb79ba8d8ed

But it assumes that the padding is at least 4 bytes.

Since padding doesn't hurt regardless of the compression used, let's just
unconditionally add it.

Fixes coreos#4683.
@jlebon
Copy link
Member

jlebon commented Nov 28, 2023

The kernel should already support a series of CPIOs in different formats. The reproducer in #4683 (comment) only says that the user-space lz4 tool complains, but that's different code from what's in the kernel. For example, the latter needs to support arbitrary padding after the end of a compressed stream. (See the grammar at https://www.kernel.org/doc/Documentation/early-userspace/buffer-format.txt).

I thought at first this was a misalignment issue (see https://github.com/coreos/coreos-assembler/blob/eaaa1d7c5f235d365017c4b1d76a98833f9f411a/src/cmd-buildextend-live#L120-L133), though I don't think that applies here since we're appending a compressed CPIO.

Searching for that error message lead me to https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2c484419efc09e7234c667aa72698cb79ba8d8ed which is quite informative. The patch fixes trailing padding handling for the kernel lz4 decompressor by checking if the next lz4 frame where the magic number is expected is all zeros. But for that logic to work, it needs at least 4 bytes of padding.

So... I think all we need is to add some padding between the two?

...time passes...

OK, this should work: #4705

jlebon added a commit to jlebon/rpm-ostree that referenced this issue Nov 29, 2023
Any arbitrary amount of padding between concatenated CPIOs is allowed:

https://www.kernel.org/doc/Documentation/early-userspace/buffer-format.txt

However, some initramfs decompressors in the kernel expect a minimal
amount of padding to detect EOF and allow the kernel outer loop to try
out other decompressors for the next payload. This is the case for lz4.
This upstream patch allows the lz4 decompressor to treat padding as EOF:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2c484419efc09e7234c667aa72698cb79ba8d8ed

But it assumes that the padding is at least 4 bytes.

Since padding doesn't hurt regardless of the compression used, let's just
unconditionally add it.

Fixes coreos#4683.

Co-authored-by: Colin Walters <walters@verbum.org>
jlebon added a commit to jlebon/rpm-ostree that referenced this issue Nov 29, 2023
Any arbitrary amount of padding between concatenated CPIOs is allowed:

https://www.kernel.org/doc/Documentation/early-userspace/buffer-format.txt

However, some initramfs decompressors in the kernel expect a minimal
amount of padding to detect EOF and allow the kernel outer loop to try
out other decompressors for the next payload. This is the case for lz4.
This upstream patch allows the lz4 decompressor to treat padding as EOF:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2c484419efc09e7234c667aa72698cb79ba8d8ed

But it assumes that the padding is at least 4 bytes.

Since padding doesn't hurt regardless of the compression used, let's just
unconditionally add it.

Fixes coreos#4683.

Co-authored-by: Colin Walters <walters@verbum.org>
cgwalters added a commit that referenced this issue Nov 29, 2023
Any arbitrary amount of padding between concatenated CPIOs is allowed:

https://www.kernel.org/doc/Documentation/early-userspace/buffer-format.txt

However, some initramfs decompressors in the kernel expect a minimal
amount of padding to detect EOF and allow the kernel outer loop to try
out other decompressors for the next payload. This is the case for lz4.
This upstream patch allows the lz4 decompressor to treat padding as EOF:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2c484419efc09e7234c667aa72698cb79ba8d8ed

But it assumes that the padding is at least 4 bytes.

Since padding doesn't hurt regardless of the compression used, let's just
unconditionally add it.

Fixes #4683.

Co-authored-by: Colin Walters <walters@verbum.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants