Skip to content

Commit

Permalink
IBM Z DFLTCC: Buffer deflate() input data
Browse files Browse the repository at this point in the history 
When testing QEMU live migration on an IBM z15 machine, a problem with
DFLTCC was discovered: modifying dfltcc_cmpr() input data concurrently
with dfltcc_cmpr() leads to data corruption  [1] [2].

Based on zlib manual, one might argue both ways whether it is safe to
perform such concurrent modifications. At the end of the day, it's
simply undefined, and now we know that there are applications out there
that rely on this being allowed.

So tolerate this in the DFLTCC implementation by introducing a
buffering layer.

[1] https://lists.gnu.org/archive/html/qemu-devel/2022-03/msg06841.html
[2] https://lists.gnu.org/archive/html/qemu-devel/2022-04/msg00329.html
  • Loading branch information
@iii-i
iii-i committed Jun 20, 2022
1 parent eb0d181 commit efe7f3a
Show file tree
Hide file tree
Showing 3 changed files with 109 additions and 45 deletions.
144 changes: 103 additions & 41 deletions arch/s390/dfltcc_deflate.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ struct dfltcc_deflate_state {
uint32_t block_size; /* New block each X bytes */
size_t block_threshold; /* New block after total_in > X */
uint32_t dht_threshold; /* New block only if avail_in >= X */
uint8_t buf[HB_SIZE + DFLTCC_BUF_SIZE]; /* Internal buffer for history and input data */
uint32_t buf_offset; /* Offset of input data in buf */
uint32_t buf_size; /* Size of input data in buf */
};

#define GET_DFLTCC_DEFLATE_STATE(state) ((struct dfltcc_deflate_state *)GET_DFLTCC_STATE(state))
Expand All @@ -44,6 +47,10 @@ void Z_INTERNAL PREFIX(dfltcc_reset_deflate_state)(PREFIX3(streamp) strm) {
dfltcc_state->block_size = DFLTCC_BLOCK_SIZE;
dfltcc_state->block_threshold = DFLTCC_FIRST_FHT_BLOCK_SIZE;
dfltcc_state->dht_threshold = DFLTCC_DHT_MIN_SAMPLE_SIZE;

/* Initialize the internal buffer */
dfltcc_state->buf_offset = 0;
dfltcc_state->buf_size = 0;
}

void Z_INTERNAL PREFIX(dfltcc_copy_deflate_state)(void *dst, const void *src) {
Expand Down Expand Up @@ -95,9 +102,9 @@ static inline dfltcc_cc dfltcc_cmpr(PREFIX3(streamp) strm) {
size_t avail_out = strm->avail_out;
dfltcc_cc cc;

cc = dfltcc(DFLTCC_CMPR | HBT_CIRCULAR,
cc = dfltcc(DFLTCC_CMPR,
param, &strm->next_out, &avail_out,
&strm->next_in, &avail_in, state->window);
&strm->next_in, &avail_in, NULL);
strm->total_in += (strm->avail_in - avail_in);
strm->total_out += (strm->avail_out - avail_out);
strm->avail_in = avail_in;
Expand Down Expand Up @@ -125,6 +132,59 @@ static inline void send_eobs(PREFIX3(streamp) strm, const struct dfltcc_param_v0
#endif
}

/*
Modifying dfltcc_cmpr() input concurrently with dfltcc_cmpr() may desync the sliding window and the deflate stream,
causing data corruption. Therefore, in order to allow modifying deflate() input concurrently with deflate(), we
need input data buffering. zlib manual does not guarantee that such concurrent modifications are safe, but there
are applications out there that perform them anyway. Extra copying causes ~25% compression performance degradation,
but this is better than risking data corruption.
*/

struct dfltcc_buf_sync {
z_const unsigned char *next_in;
uint32_t avail_in;
};

static void dfltcc_buf_sync_start(PREFIX3(streamp) strm, struct dfltcc_buf_sync *sync) {
deflate_state *state = (deflate_state *)strm->state;
struct dfltcc_deflate_state *dfltcc_state = GET_DFLTCC_DEFLATE_STATE(state);
struct dfltcc_param_v0 *param = &dfltcc_state->common.param;
uint32_t count = sizeof(dfltcc_state->buf) - (dfltcc_state->buf_offset + dfltcc_state->buf_size);

if (strm->avail_in > count && dfltcc_state->buf_offset > param->hl && dfltcc_state->buf_size < DFLTCC_BUF_THRESHOLD) {
/* Input does not fit into the internal buffer. Make some room by sliding it to the left: it is less expensive
* than calling dfltcc_cmpr() one more time.
*/
memmove(dfltcc_state->buf, dfltcc_state->buf + dfltcc_state->buf_offset - param->hl, param->hl + dfltcc_state->buf_size);
count += dfltcc_state->buf_offset - param->hl;
dfltcc_state->buf_offset = param->hl;
}
if (strm->avail_in < count)
count = strm->avail_in;

/* Copy input data into the internal buffer */
memcpy(dfltcc_state->buf + dfltcc_state->buf_offset + dfltcc_state->buf_size, strm->next_in, count);
dfltcc_state->buf_size += count;

/* Point the stream to the internal buffer; save the original input data pointer and size */
sync->next_in = strm->next_in + count;
sync->avail_in = strm->avail_in - count;
strm->next_in = dfltcc_state->buf + dfltcc_state->buf_offset;
strm->avail_in = dfltcc_state->buf_size;
}

static void dfltcc_buf_sync_end(PREFIX3(streamp) strm, struct dfltcc_buf_sync *sync) {
deflate_state *state = (deflate_state *)strm->state;
struct dfltcc_deflate_state *dfltcc_state = GET_DFLTCC_DEFLATE_STATE(state);
uint32_t consumed = strm->next_in - (dfltcc_state->buf + dfltcc_state->buf_offset);

/* Update the internal buffer positions; restore the original input data pointer and size */
dfltcc_state->buf_offset += consumed;
dfltcc_state->buf_size -= consumed;
strm->next_in = sync->next_in;
strm->avail_in = sync->avail_in;
}

int Z_INTERNAL PREFIX(dfltcc_deflate)(PREFIX3(streamp) strm, int flush, block_state *result) {
deflate_state *state = (deflate_state *)strm->state;
struct dfltcc_deflate_state *dfltcc_state = GET_DFLTCC_DEFLATE_STATE(state);
Expand All @@ -134,6 +194,7 @@ int Z_INTERNAL PREFIX(dfltcc_deflate)(PREFIX3(streamp) strm, int flush, block_st
int need_empty_block;
int soft_bcc;
int no_flush;
struct dfltcc_buf_sync sync;

if (!PREFIX(dfltcc_can_deflate)(strm)) {
/* Clear history. */
Expand Down Expand Up @@ -242,6 +303,9 @@ int Z_INTERNAL PREFIX(dfltcc_deflate)(PREFIX3(streamp) strm, int flush, block_st
param->nt = 0;
param->cv = state->wrap == 2 ? ZSWAP32(state->crc_fold.value) : strm->adler;

/* We may access input, but must not return before calling dfltcc_buf_sync_end() */
dfltcc_buf_sync_start(strm, &sync);

/* When opening a block, choose a Huffman-Table Type */
if (!param->bcf) {
if (state->strategy == Z_FIXED || (strm->total_in == 0 && dfltcc_state->block_threshold > 0))
Expand All @@ -253,16 +317,27 @@ int Z_INTERNAL PREFIX(dfltcc_deflate)(PREFIX3(streamp) strm, int flush, block_st
}

/* Deflate */
do {
for (;;) {
cc = dfltcc_cmpr(strm);
dfltcc_buf_sync_end(strm, &sync);
if (strm->avail_in < 4096 && masked_avail_in > 0)
/* We are about to call DFLTCC with a small input buffer, which is
* inefficient. Since there is masked data, there will be at least
* one more DFLTCC call, so skip the current one and make the next
* one handle more data.
*/
break;
} while (cc == DFLTCC_CC_AGAIN);
if (cc == DFLTCC_CC_AGAIN)
/* DFLTCC told us to try again */
;
else if (cc == DFLTCC_CC_OK && strm->avail_in != 0 && strm->avail_out != 0)
/* There is more input data that we can copy into the internal buffer */
param->bcf = 1;
else
break;
dfltcc_buf_sync_start(strm, &sync);
}
/* We must no longer access input, but we may return */

/* Translate parameter block to stream */
strm->msg = oesc_msg(dfltcc_state->common.msg, param->oesc);
Expand All @@ -288,6 +363,7 @@ int Z_INTERNAL PREFIX(dfltcc_deflate)(PREFIX3(streamp) strm, int flush, block_st
*/
if (cc == DFLTCC_CC_OK) {
if (soft_bcc) {
Assert(!param->bhf || strm->avail_in == 0, "There must be no input data after the final EOBS");
send_eobs(strm, param);
param->bcf = 0;
dfltcc_state->block_threshold = strm->total_in + dfltcc_state->block_size;
Expand Down Expand Up @@ -376,64 +452,50 @@ int Z_INTERNAL PREFIX(dfltcc_can_set_reproducible)(PREFIX3(streamp) strm, int re
/*
Preloading history.
*/
static void append_history(struct dfltcc_param_v0 *param, unsigned char *history, const unsigned char *buf, uInt count) {
size_t offset;
size_t n;
static void append_history(deflate_state *state, const unsigned char *buf, uInt count) {
struct dfltcc_deflate_state *dfltcc_state = GET_DFLTCC_DEFLATE_STATE(state);
struct dfltcc_param_v0 *param = &dfltcc_state->common.param;
size_t keep;

/* Do not use more than 32K */
if (count > HB_SIZE) {
buf += count - HB_SIZE;
count = HB_SIZE;
}
offset = (param->ho + param->hl) % HB_SIZE;
if (offset + count <= HB_SIZE)
/* Circular history buffer does not wrap - copy one chunk */
memcpy(history + offset, buf, count);
else {
/* Circular history buffer wraps - copy two chunks */
n = HB_SIZE - offset;
memcpy(history + offset, buf, n);
memcpy(history, buf + n, count - n);
}
n = param->hl + count;
if (n <= HB_SIZE)
/* All history fits into buffer - no need to discard anything */
param->hl = n;
else {
/* History does not fit into buffer - discard extra bytes */
param->ho = (param->ho + (n - HB_SIZE)) % HB_SIZE;
param->hl = HB_SIZE;
}

/* Determine how much of the existing history to keep */
if (param->hl + count <= HB_SIZE)
keep = param->hl;
else
keep = HB_SIZE - count;

memmove(dfltcc_state->buf, dfltcc_state->buf + dfltcc_state->buf_offset - keep, keep);
memmove(dfltcc_state->buf + keep + count, dfltcc_state->buf + dfltcc_state->buf_offset, dfltcc_state->buf_size);
memcpy(dfltcc_state->buf + keep, buf, count);
dfltcc_state->buf_offset = keep + count;
param->hl = keep + count;
}

int Z_INTERNAL PREFIX(dfltcc_deflate_set_dictionary)(PREFIX3(streamp) strm,
const unsigned char *dictionary, uInt dict_length) {
deflate_state *state = (deflate_state *)strm->state;
struct dfltcc_state *dfltcc_state = GET_DFLTCC_STATE(state);
struct dfltcc_param_v0 *param = &dfltcc_state->param;

append_history(param, state->window, dictionary, dict_length);
append_history(state, dictionary, dict_length);
state->strstart = 1; /* Add FDICT to zlib header */
state->block_start = state->strstart; /* Make deflate_stored happy */
return Z_OK;
}

int Z_INTERNAL PREFIX(dfltcc_deflate_get_dictionary)(PREFIX3(streamp) strm, unsigned char *dictionary, uInt *dict_length) {
deflate_state *state = (deflate_state *)strm->state;
struct dfltcc_state *dfltcc_state = GET_DFLTCC_STATE(state);
struct dfltcc_param_v0 *param = &dfltcc_state->param;
struct dfltcc_deflate_state *dfltcc_state = GET_DFLTCC_DEFLATE_STATE(state);
struct dfltcc_param_v0 *param = &dfltcc_state->common.param;

if (dictionary)
memcpy(dictionary, dfltcc_state->buf + dfltcc_state->buf_offset - param->hl, param->hl);

if (dictionary) {
if (param->ho + param->hl <= HB_SIZE)
/* Circular history buffer does not wrap - copy one chunk */
memcpy(dictionary, state->window + param->ho, param->hl);
else {
/* Circular history buffer wraps - copy two chunks */
memcpy(dictionary, state->window + param->ho, HB_SIZE - param->ho);
memcpy(dictionary + HB_SIZE - param->ho, state->window, param->ho + param->hl - HB_SIZE);
}
}
if (dict_length)
*dict_length = param->hl;

return Z_OK;
}
6 changes: 6 additions & 0 deletions arch/s390/dfltcc_detail.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,12 @@
#ifndef DFLTCC_RIBM
#define DFLTCC_RIBM 0
#endif
#ifndef DFLTCC_BUF_SIZE
#define DFLTCC_BUF_SIZE 262144
#endif
#ifndef DFLTCC_BUF_THRESHOLD
#define DFLTCC_BUF_THRESHOLD 98304
#endif

/*
Parameter Block for Query Available Functions.
Expand Down
4 changes: 0 additions & 4 deletions test/test_deflate_concurrency.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,6 @@ class Mutator {
};

TEST(deflate, concurrency) {
#ifdef S390_DFLTCC_DEFLATE
GTEST_SKIP() << "XFAIL S390_DFLTCC_DEFLATE";
#endif

/* Create reusable mutator and streams. */
Mutator mutator;

Expand Down

0 comments on commit efe7f3a

Please sign in to comment.