linux-kernel - Re: [PATCH v6 1/3] lib: zstd: Add kernel-specific API

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 3 Dec 2020 01:42:03 +0000
From:   Nick Terrell <terrelln@...com>
To:     Michał Mirosław <mirq-linux@...e.qmqm.pl>
CC:     Nick Terrell <nickrterrell@...il.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
        Btrfs BTRFS <linux-btrfs@...r.kernel.org>,
        "squashfs-devel@...ts.sourceforge.net" 
        <squashfs-devel@...ts.sourceforge.net>,
        "linux-f2fs-devel@...ts.sourceforge.net" 
        <linux-f2fs-devel@...ts.sourceforge.net>,
        LKML <linux-kernel@...r.kernel.org>,
        Kernel Team <Kernel-team@...com>, Chris Mason <clm@...com>,
        Petr Malat <oss@...at.biz>, Johannes Weiner <jweiner@...com>,
        Niket Agarwal <niketa@...com>, Yann Collet <cyan@...com>,
        Christoph Hellwig <hch@...radead.org>
Subject: Re: [PATCH v6 1/3] lib: zstd: Add kernel-specific API



> On Dec 2, 2020, at 5:16 PM, Michał Mirosław <mirq-linux@...e.qmqm.pl> wrote:
> 
> On Wed, Dec 02, 2020 at 12:32:40PM -0800, Nick Terrell wrote:
>> From: Nick Terrell <terrelln@...com>
>> 
>> This patch:
>> - Moves `include/linux/zstd.h` -> `lib/zstd/zstd.h`
>> - Adds a new API in `include/linux/zstd.h` that is functionally
>>  equivalent to the in-use subset of the current API. Functions are
>>  renamed to avoid symbol collisions with zstd, to make it clear it is
>>  not the upstream zstd API, and to follow the kernel style guide.
>> - Updates all callers to use the new API.
>> 
>> There are no functional changes in this patch. Since there are no
>> functional change, I felt it was okay to update all the callers in a
>> single patch, since once the API is approved, the callers are
>> mechanically changed.
> [...]
>> --- a/lib/decompress_unzstd.c
>> +++ b/lib/decompress_unzstd.c
> [...]
>> static int INIT handle_zstd_error(size_t ret, void (*error)(char *x))
>> {
>> -	const int err = ZSTD_getErrorCode(ret);
>> -
>> -	if (!ZSTD_isError(ret))
>> +	if (!zstd_is_error(ret))
>> 		return 0;
>> 
>> -	switch (err) {
>> -	case ZSTD_error_memory_allocation:
>> -		error("ZSTD decompressor ran out of memory");
>> -		break;
>> -	case ZSTD_error_prefix_unknown:
>> -		error("Input is not in the ZSTD format (wrong magic bytes)");
>> -		break;
>> -	case ZSTD_error_dstSize_tooSmall:
>> -	case ZSTD_error_corruption_detected:
>> -	case ZSTD_error_checksum_wrong:
>> -		error("ZSTD-compressed data is corrupt");
>> -		break;
>> -	default:
>> -		error("ZSTD-compressed data is probably corrupt");
>> -		break;
>> -	}
>> +	error("ZSTD decompression failed");
>> 	return -1;
>> }
> 
> This looses diagnostics specificity - is this intended? At least the
> out-of-memory condition seems useful to distinguish.

Good point. The zstd API no longer exposes the error code enum,
but it does expose zstd_get_error_name() which can be used here.
I was thinking that the string needed to be static for some reason, but
that is not the case. I will make that change.

>> +size_t zstd_compress_stream(zstd_cstream *cstream,
>> +	struct zstd_out_buffer *output, struct zstd_in_buffer *input)
>> +{
>> +	ZSTD_outBuffer o;
>> +	ZSTD_inBuffer i;
>> +	size_t ret;
>> +
>> +	memcpy(&o, output, sizeof(o));
>> +	memcpy(&i, input, sizeof(i));
>> +	ret = ZSTD_compressStream(cstream, &o, &i);
>> +	memcpy(output, &o, sizeof(o));
>> +	memcpy(input, &i, sizeof(i));
>> +	return ret;
>> +}
> 
> Is all this copying necessary? How is it different from type-punning by
> direct pointer cast?

If breaking strict aliasing and type-punning by pointer casing is okay, then
we can do that here. These memcpys will be negligible for performance, but
type-punning would be more succinct if allowed.

Best,
Nick