Re: [PATCH] xz: make XZ_DEC_BCJ filters non-optional

[Lasse Collin <lasse.collin@tukaani.org>]

On 2014-03-05 Phillip Lougher wrote:
> (BTW Kyle you should have CC'd me on the patch as a courtesy).

I could have done that too but somehow I didn't, sorry.

> But speaking as the Squashfs author, the lack of BCJ support for
> an architecture creates a subtle failure mode in Squashfs, this is
> because not all blocks in a Squashfs filesystem get compressed
> with a BCJ filter.  At compression time each block is compressed
> without any BCJ filter, and then with the BCJ filter(s) selected on
> the command line, and the best compression for *that* block is
> chosen.  What this means is kernels without a particular
> BCJ filter can still read the Squashfs metadata (mount, ls etc.) and
> read many of the files, it is only some files that mysteriously
> fail with decompression error.  As such this will be (and has been)
> invariably treated as a bug in Squashfs.

There is an easy way to make Squashfs give an error message in the
kernel log. xz_dec_run() gives XZ_OPTIONS_ERROR when valid-looking but
unsupported input is detected. Currently Squashfs treats all error
codes from xz_dec_run() the same way so the reason for the
decompression error is silently lost.

Below is an *untested* fix. I'm not sure about the exact wording of the
error message, so feel free to improve it.

diff -Narup linux-3.14-rc5.orig/fs/squashfs/xz_wrapper.c linux-3.14-rc5/fs/squashfs/xz_wrapper.c
--- linux-3.14-rc5.orig/fs/squashfs/xz_wrapper.c	2014-03-03 04:56:16.000000000 +0200
+++ linux-3.14-rc5/fs/squashfs/xz_wrapper.c	2014-03-05 18:08:58.729643127 +0200
@@ -170,8 +170,13 @@ static int squashfs_xz_uncompress(struct
 
 	squashfs_finish_page(output);
 
-	if (xz_err != XZ_STREAM_END || k < b)
+	if (xz_err != XZ_STREAM_END || k < b) {
+		if (xz_err == XZ_OPTIONS_ERROR)
+			ERROR("Unsupported XZ-compressed data; check the XZ "
+					"options in the kernel config\n");
+
 		goto out;
+	}
 
 	return total + stream->buf.out_pos;
 

> Moreover, without expert knowledge of Squashfs, and the config
> options, most people will not have a clue how to fix the issue.
> 
> This is why I prefer the first option, which is to reinstate
> the enabling of all filters by default, and then to allow people
> to remove the filters they don't want.

I will submit the first option. In the other email Florian Fainelli
seemed to be OK with that too.

> BTW there is a potential additional fix for Squashfs that will
> make its handling of (lack of) BCJ filters more intelligent
> at mount time, but this of course only addresses Squashfs,
> and it relies on an additional call into XZ being added.  The
> BCJ filters specified at filesystem creation are stored in the
> compression options part of the superblock, and are known at
> mount time.  Squashfs should check that these filters are
> supported by the kernel and refuse to mount it otherwise.  This
> has not been done because AFAIK there is no way to query XZ to
> determine which BCJ filters are supported (beyond passing it a
> test stream which is too messy).

You can use #ifdef CONFIG_XZ_DEC_X86 and such, although maybe that's
not convenient enough.

Adding a function to xz_dec module could be sort of OK. It's important
to keep in mind that the ability to disable filters is there to reduce
code size *slightly*. If you or we add something extra to figure out
what is supported at runtime, the size benefit may get lost.

If all filters are enabled by default, a clear error message on
XZ_OPTIONS_ERROR should be enough, I think.

-- 
Lasse Collin  |  IRC: Larhzu @ IRCnet & Freenode