New issue
Advanced search Search tips

Issue 765844 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: Oct 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug

Blocking:
issue 772450



Sign in to add a comment

futility on the recovery image sucks in libcrypto, which is too big

Project Member Reported by diand...@chromium.org, Sep 15 2017

Issue description

Much discussion in  bug #764753 , but in summary:

---

The only user of libcrypto in the recovery image is futility.  ...futility only uses a tiny bit of libcrypto and libcrypto is big.  So it turns out that if we just statically link futility we get a big savings.  Taking all the above and statically linking futility:

-rw-r--r-- 1 root root 3321248 Sep 15 09:43 /build/peppy/var/lib/initramfs/recovery_ramfs.cpio.xz

...that's compared to dynamically linking futility:

-rw-r--r-- 1 root root 4053512 Sep 15 09:31 /build/peppy/var/lib/initramfs/recovery_ramfs.cpio.xz

...so we save 732264 bytes (!).


Yeah we pay a little bit because we've statically linked other things to futility, but wow the savings.  We already build both futility and futility_s.

---

Change is at <https://chromium-review.googlesource.com/c/chromiumos/platform/initramfs/+/668689>
 
Cc: diand...@chromium.org
Owner: vapier@chromium.org
Status: Assigned (was: Started)
In Mike's court to decide between giving a +2 to the change or making this WontFix.
Blocking: 772450
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/initramfs/+/436366ccc92184b7015483137d2ac46a7618ac5f

commit 436366ccc92184b7015483137d2ac46a7618ac5f
Author: Douglas Anderson <dianders@chromium.org>
Date: Tue Oct 10 19:19:08 2017

recovery: Use the static linked version of futility

In the recovery initramfs the only user of libcrypto is futility.
futility uses a tiny tiny amount of the functionality of libcrypto and
libcrypto is huge (780K after xz compression).

It turns out that we get a big savings (715K compressed) if we use the
statically linked version of futility for the initramfs.  This makes
futility bigger (because it now includes functions that are already
present in the filesystem) but avoiding the inclusion of the giant
libcrypto is such a win that it's worth it.

BUG= chromium:765844 
TEST=Recovery kernel is much smaller, still works

Change-Id: Ic57e9d5569c0b629140c2d962ee187d492dc82f5
Reviewed-on: https://chromium-review.googlesource.com/668689
Commit-Ready: Douglas Anderson <dianders@chromium.org>
Tested-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Hung-Te Lin <hungte@chromium.org>
Reviewed-by: Sameer Nanda <snanda@chromium.org>

[modify] https://crrev.com/436366ccc92184b7015483137d2ac46a7618ac5f/recovery/Makefile

Status: Fixed (was: Assigned)

Comment 5 by hungte@chromium.org, Oct 11 2017

crypto lib seems to be entirely not included in the static version and was putted back ( https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/708236 ).

You probably want to check again that the built binary is still smaller than dynamic version?
Weird. I'll dig tomorrow.
OK, so "10021.0.0" has the fix.  Getting chromeos_10021.0.0_daisy_recovery_dev-channel_snow-mp-v4.bin

---

Using the same steps for getting initramfs from an x86 kernel that I cobbled together in  bug #764753 :

IMAGE='dev-channel%2Fpeppy%2F10021.0.0%2Fchromeos_10021.0.0_peppy_recovery_dev-channel_mp-v3.bin'
START_BLK=$(cgpt show "${IMAGE}" | grep KERN-A | awk '{print $1};')
SIZE_BLK=$(cgpt show "${IMAGE}" | grep KERN-A | awk '{print $2};')
dd if="${IMAGE}" of=kern.bin bs=512 skip=${START_BLK} count=${SIZE_BLK}
vbutil_kernel --get-vmlinuz kern.bin --vmlinuz-out vmlinuz
~/trunk/src/third_party/kernel/v3.8/scripts/extract-vmlinux vmlinuz > vmlinux
grep -abo 7zXZ vmlinux

# Look at the output of the grep.  I see:
# 9784290:7zXZ
# 14079921:7zXZ

# This doesn't work:
tail -c +9784290 vmlinux | xz -d --stdout > b
cpio -t < b

# This does (ignore the errors)
tail -c +14079921 vmlinux | xz -d --stdout > b
mkdir out
cd out
cpio -i < ../b

---

Looking at bin:

cd bin
for f in *; do tar cvfJ ${f}.tar.xz ${f}; done

-rw-r--r-- 1 dianders eng 878404 Oct 11 08:04 futility.tar.xz

Futility zipped up is 878K.  Sigh.  So I guess that's a fail, then.  :(

I'm a bit amazed that it really needs the whole libcrypto--I would have imagined that more code would have been dead and been stripped when statically linked.  Apparently I was wrong.

I'll post a revert now.  Sigh.

Owner: diand...@chromium.org
Status: Started (was: Fixed)
Moving this till Started till the revert, then I'll make this as WontFix.  We'll need to find some space somewhere else to give us a little more margin.

Comment 9 by vapier@chromium.org, Oct 11 2017

i'm not even sure it was working, ignoring the size issue.  via the other bug, we found futility_s only had the subcommands dump_fmap & gbb_utility.  but the recovery image wants vbutil_kernel.  so if we switched to futility_s, the recovery image breaks.

i think we need to go back to building a simple set of programs for recovery so we can pull in the specific commands we need and no more.
 https://crbug.com/772862 #34 is an example of the recovery image failing due to futility_s missing vbutil_kernel:

+ echo Rollback version stored in the TPM: 0x00030001
+ tee -a /run/frecon/vt1 /run/frecon/vt2
Rollback version stored in the TPM: 0x00030001
+ vbutil_kernel --verify /tmp/kern.bin --minversion 0x00030001
Unrecognized option: --verify

Usage: futility [options] COMMAND [args...]
Argh.  Sorry.  :(  I originally confirmed that the size was sensible.  ...actually, though, I swear that I even ran a recovery image with this change long ago when I actually wrote it.  Totally confused what happened, but that was quite a while ago now...
don't sweat it.  everyone breaks something at some point.  i think this just reinforces our recovery code flows don't have great test coverage :/.

a quick test over here shows that vbutil_kernel also needs openssl.  i think if we want to elide openssl, we're going to have to figure out something more radical :(.
Labels: -Pri-2 ReleaseBlock-Dev M-63 Pri-1
Project Member

Comment 14 by bugdroid1@chromium.org, Oct 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/initramfs/+/9bd6f53bb1f3771a79873df62f48ffc001d6a91c

commit 9bd6f53bb1f3771a79873df62f48ffc001d6a91c
Author: Douglas Anderson <dianders@chromium.org>
Date: Wed Oct 11 23:11:10 2017

Revert "recovery: Use the static linked version of futility"

This reverts commit 436366ccc92184b7015483137d2ac46a7618ac5f.

Reason for revert: No longer a space savings, and futility_s
doesn't provide vbutil_kernel which recovery needs.

Original change's description:
> recovery: Use the static linked version of futility
>
> In the recovery initramfs the only user of libcrypto is futility.
> futility uses a tiny tiny amount of the functionality of libcrypto and
> libcrypto is huge (780K after xz compression).
>
> It turns out that we get a big savings (715K compressed) if we use the
> statically linked version of futility for the initramfs.  This makes
> futility bigger (because it now includes functions that are already
> present in the filesystem) but avoiding the inclusion of the giant
> libcrypto is such a win that it's worth it.
>
> BUG= chromium:765844 
> TEST=Recovery kernel is much smaller, still works
>
> Change-Id: Ic57e9d5569c0b629140c2d962ee187d492dc82f5
> Reviewed-on: https://chromium-review.googlesource.com/668689
> Commit-Ready: Douglas Anderson <dianders@chromium.org>
> Tested-by: Douglas Anderson <dianders@chromium.org>
> Reviewed-by: Hung-Te Lin <hungte@chromium.org>
> Reviewed-by: Sameer Nanda <snanda@chromium.org>

Bug:  chromium:765844 
Change-Id: I88c9f6b728c82ef3d11770a8acb2a16374613150
Reviewed-on: https://chromium-review.googlesource.com/712855
Commit-Ready: Douglas Anderson <dianders@chromium.org>
Tested-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/9bd6f53bb1f3771a79873df62f48ffc001d6a91c/recovery/Makefile

Status: WontFix (was: Started)
OK, so giving up on this bug then.  Revert has landed and we won't try that again.

If Recovery images are blowing up then we can file separate bugs for that...

Sign in to add a comment