Re: Read before you deploy btrfs + zstd

[David Sterba <dsterba@suse.cz>]

On Wed, Nov 29, 2017 at 12:44:32AM +0000, Nick Terrell wrote:
> >> It looks like XZ had the same issue, and they make the decompression
> >> context a static object (grep for GRUB_EMBED_DECOMPRESSOR). We could
> >> potentially do the same and statically allocate the workspace:
> >> 
> >> ```
> >> /* Could also be size_t */
> >> #define BTRFS_ZSTD_WORKSPACE_SIZE_U64 (155984 / sizeof(uint64_t))
> >> static uint64_t workspace[BTRFS_ZSTD_WORKSPACE_SIZE_U64];
> >> 
> >> /* ... */
> >> 
> >> assert(sizeof(workspace) >= ZSTD_DCtxWorkspaceBound());
> >> ```
> > 
> > Interesting, thanks for the tip, I'll try it next.

And it worked.

> > I've meanwhile tried to tweak the numbers, the maximum block for zstd,
> > that squeezed the DCtx somewhere under 48k, with block size 8k. Still
> > enomem.
> 
> Sadly the block size has to stay 128 KiB, since that is the block size that
> is used for compression.
> 
> > I've tried to add some debugging prints to see what numbers get actually
> > passed to the allocator, but did not see anything printed.  I'm sure
> > there is a more intelligent way to test the grub changes.  So far each
> > test loop takes quite some time, as I build the rpm package, test it in
> > a VM and have to recreate the environmet each time.
> 
> Is the code in github.com/kdave/grub in the btrfs-zstd branch up to date?
> btrfs.c:1230 looks like it will error for zstd compression, but not with
> ENOMEM. btrfs.c:1272 [2] looks like a typo. Maybe the second is causing
> the compressed block to be interpreted as uncompressed, and causes a
> too-large allocation?

I had a different branch with patches from openSUSE, so the diffs apply with
minimal efforts to the package. The branch btrfs-zstd has been synced up. The
ENOMEM error was not from the file decompression but from the zstdio.c module,
that does something different, now disabled.

I got a bit further, with a few debugging messages, this check fails:

 957   total_size = grub_le_to_cpu32 (grub_get_unaligned32 (ibuf));
 958   ibuf += sizeof (total_size);
 959
 960   if (isize < total_size) {
 961     grub_error (GRUB_ERR_BAD_FS, "ASSERTION: isize < total_size: %ld < %ld",
 962                     isize, total_size);
 963     return -1;
 964   }

with: "isize < total_size: 61440 < -47205080"

total_size is u32, but wrngly printed as signed long so it's negative, but
would be wrong anyway. This looks like the compressed format is not read
correctly.