From da19d919a853ad05ef300fe000e6c96c4db84bcf Mon Sep 17 00:00:00 2001 From: Paul Dagnelie Date: Fri, 10 Mar 2023 09:52:44 -0800 Subject: [PATCH] Fix incremental receive silently failing for recursive sends The problem occurs because dmu_recv_begin pulls in the payload and next header from the input stream in order to use the contents of the begin record's nvlist. However, the change to do that before the other checks in dmu_recv_begin occur caused a regression where an empty send stream in a recursive send could have its END record consumed by this, which broke the logic of recv_skip. A test is also included to protect against this case in the future. Reviewed-by: Brian Behlendorf Signed-off-by: Paul Dagnelie Closes #12661 Closes #14568 --- module/zfs/dmu_recv.c | 35 ++++++++--- tests/runfiles/common.run | 27 ++++---- tests/zfs-tests/cmd/libzfs_input_check.c | 2 +- tests/zfs-tests/tests/Makefile.am | 1 + .../tests/functional/rsend/rsend_031_pos.ksh | 63 +++++++++++++++++++ 5 files changed, 105 insertions(+), 23 deletions(-) create mode 100755 tests/zfs-tests/tests/functional/rsend/rsend_031_pos.ksh diff --git a/module/zfs/dmu_recv.c b/module/zfs/dmu_recv.c index 1684d9d92..3763cba02 100644 --- a/module/zfs/dmu_recv.c +++ b/module/zfs/dmu_recv.c @@ -1255,7 +1255,6 @@ dmu_recv_begin(char *tofs, char *tosnap, dmu_replay_record_t *drr_begin, DMU_GET_FEATUREFLAGS(drc->drc_drrb->drr_versioninfo); uint32_t payloadlen = drc->drc_drr_begin->drr_payloadlen; - void *payload = NULL; /* * Since OpenZFS 2.0.0, we have enforced a 64MB limit in userspace @@ -1266,16 +1265,23 @@ dmu_recv_begin(char *tofs, char *tosnap, dmu_replay_record_t *drr_begin, if (payloadlen > (MIN((1U << 28), arc_all_memory() / 4))) return (E2BIG); - if (payloadlen != 0) - payload = vmem_alloc(payloadlen, KM_SLEEP); - err = receive_read_payload_and_next_header(drc, payloadlen, - payload); - if (err != 0) { - vmem_free(payload, payloadlen); - return (err); - } if (payloadlen != 0) { + void *payload = vmem_alloc(payloadlen, KM_SLEEP); + /* + * For compatibility with recursive send streams, we don't do + * this here if the stream could be part of a package. Instead, + * we'll do it in dmu_recv_stream. If we pull the next header + * too early, and it's the END record, we break the `recv_skip` + * logic. + */ + + err = receive_read_payload_and_next_header(drc, payloadlen, + payload); + if (err != 0) { + vmem_free(payload, payloadlen); + return (err); + } err = nvlist_unpack(payload, payloadlen, &drc->drc_begin_nvl, KM_SLEEP); vmem_free(payload, payloadlen); @@ -3315,6 +3321,17 @@ dmu_recv_stream(dmu_recv_cookie_t *drc, offset_t *voffp) goto out; } + /* + * For compatibility with recursive send streams, we do this here, + * rather than in dmu_recv_begin. If we pull the next header too + * early, and it's the END record, we break the `recv_skip` logic. + */ + if (drc->drc_drr_begin->drr_payloadlen == 0) { + err = receive_read_payload_and_next_header(drc, 0, NULL); + if (err != 0) + goto out; + } + /* * If we failed before this point we will clean up any new resume * state that was created. Now that we've gotten past the initial diff --git a/tests/runfiles/common.run b/tests/runfiles/common.run index 132df4925..d30bcd804 100644 --- a/tests/runfiles/common.run +++ b/tests/runfiles/common.run @@ -837,19 +837,20 @@ tests = ['recv_dedup', 'recv_dedup_encrypted_zvol', 'rsend_001_pos', 'rsend_014_pos', 'rsend_016_neg', 'rsend_019_pos', 'rsend_020_pos', 'rsend_021_pos', 'rsend_022_pos', 'rsend_024_pos', 'rsend_025_pos', 'rsend_026_neg', 'rsend_027_pos', 'rsend_028_neg', 'rsend_029_neg', - 'rsend_030_pos', 'send-c_verify_ratio', 'send-c_verify_contents', - 'send-c_props', 'send-c_incremental', 'send-c_volume', - 'send-c_zstream_recompress', 'send-c_zstreamdump', 'send-c_lz4_disabled', - 'send-c_recv_lz4_disabled', 'send-c_mixed_compression', - 'send-c_stream_size_estimate', 'send-c_embedded_blocks', 'send-c_resume', - 'send-cpL_varied_recsize', 'send-c_recv_dedup', 'send-L_toggle', - 'send_encrypted_incremental', 'send_encrypted_freeobjects', - 'send_encrypted_hierarchy', 'send_encrypted_props', - 'send_encrypted_truncated_files', 'send_freeobjects', 'send_realloc_files', - 'send_realloc_encrypted_files', 'send_spill_block', 'send_holds', - 'send_hole_birth', 'send_mixed_raw', 'send-wR_encrypted_zvol', - 'send_partial_dataset', 'send_invalid', 'send_doall', - 'send_raw_spill_block', 'send_raw_ashift', 'send_raw_large_blocks'] + 'rsend_030_pos', 'rsend_031_pos', 'send-c_verify_ratio', + 'send-c_verify_contents', 'send-c_props', 'send-c_incremental', + 'send-c_volume', 'send-c_zstream_recompress', 'send-c_zstreamdump', + 'send-c_lz4_disabled', 'send-c_recv_lz4_disabled', + 'send-c_mixed_compression', 'send-c_stream_size_estimate', + 'send-c_embedded_blocks', 'send-c_resume', 'send-cpL_varied_recsize', + 'send-c_recv_dedup', 'send-L_toggle', 'send_encrypted_incremental', + 'send_encrypted_freeobjects', 'send_encrypted_hierarchy', + 'send_encrypted_props', 'send_encrypted_truncated_files', + 'send_freeobjects', 'send_realloc_files', 'send_realloc_encrypted_files', + 'send_spill_block', 'send_holds', 'send_hole_birth', 'send_mixed_raw', + 'send-wR_encrypted_zvol', 'send_partial_dataset', 'send_invalid', + 'send_doall', 'send_raw_spill_block', 'send_raw_ashift', + 'send_raw_large_blocks'] tags = ['functional', 'rsend'] [tests/functional/scrub_mirror] diff --git a/tests/zfs-tests/cmd/libzfs_input_check.c b/tests/zfs-tests/cmd/libzfs_input_check.c index 2e1859b1e..a1dfaefd7 100644 --- a/tests/zfs-tests/cmd/libzfs_input_check.c +++ b/tests/zfs-tests/cmd/libzfs_input_check.c @@ -563,7 +563,7 @@ test_recv_new(const char *dataset, int fd) fnvlist_add_uint64(optional, "action_handle", *action_handle); #endif IOC_INPUT_TEST(ZFS_IOC_RECV_NEW, dataset, required, optional, - ZFS_ERR_STREAM_TRUNCATED); + ENOTSUP); nvlist_free(props); nvlist_free(optional); diff --git a/tests/zfs-tests/tests/Makefile.am b/tests/zfs-tests/tests/Makefile.am index efce46123..4b0a28a6e 100644 --- a/tests/zfs-tests/tests/Makefile.am +++ b/tests/zfs-tests/tests/Makefile.am @@ -1795,6 +1795,7 @@ nobase_dist_datadir_zfs_tests_tests_SCRIPTS += \ functional/rsend/rsend_028_neg.ksh \ functional/rsend/rsend_029_neg.ksh \ functional/rsend/rsend_030_pos.ksh \ + functional/rsend/rsend_031_pos.ksh \ functional/rsend/send-c_embedded_blocks.ksh \ functional/rsend/send-c_incremental.ksh \ functional/rsend/send-c_lz4_disabled.ksh \ diff --git a/tests/zfs-tests/tests/functional/rsend/rsend_031_pos.ksh b/tests/zfs-tests/tests/functional/rsend/rsend_031_pos.ksh new file mode 100755 index 000000000..119da7150 --- /dev/null +++ b/tests/zfs-tests/tests/functional/rsend/rsend_031_pos.ksh @@ -0,0 +1,63 @@ +#!/bin/ksh -p + +# +# This file and its contents are supplied under the terms of the +# Common Development and Distribution License ("CDDL"), version 1.0. +# You may only use this file in accordance with the terms of version +# 1.0 of the CDDL. +# +# A full copy of the text of the CDDL should have accompanied this +# source. A copy of the CDDL is also available via the Internet at +# http://www.illumos.org/license/CDDL. +# + +# +# Copyright (c) 2023 by Delphix. All rights reserved. +# + +. $STF_SUITE/tests/functional/rsend/rsend.kshlib + +# +# Description: +# Verify recursive incremental sends missing snapshots behave correctly. +# +# Strategy: +# 1. Create snapshots on source filesystem. +# 2. Recursively send snapshots. +# 3. Delete snapshot on source filesystem +# 4. Perform incremental recursive send. +# 5. Verify matching snapshot lists. +# + +verify_runnable "both" + +sendfs=$POOL/sendfs +recvfs=$POOL2/recvfs + +function cleanup { + rm $BACKDIR/stream1 + rm $BACKDIR/stream2 + zfs destroy -r $sendfs + zfs destroy -r $recvfs +} + +log_assert "Verify recursive incremental sends missing snapshots behave correctly." +log_onexit cleanup + +log_must zfs create $sendfs +log_must zfs snapshot $sendfs@A +log_must zfs snapshot $sendfs@B +log_must zfs snapshot $sendfs@C +log_must eval "zfs send -Rpv $sendfs@C > $BACKDIR/stream1" +log_must eval "zfs receive -F $recvfs < $BACKDIR/stream1" +log_must zfs list $sendfs@C + +log_must zfs destroy $sendfs@C +log_must zfs snapshot $sendfs@D +log_must zfs snapshot $sendfs@E +log_must eval "zfs send -Rpv -I $sendfs@A $sendfs@E > $BACKDIR/stream2" +log_must eval "zfs receive -Fv $recvfs < $BACKDIR/stream2" +log_must zfs list $sendfs@D +log_must zfs list $sendfs@E +log_mustnot zfs list $sendfs@C +log_pass "Verify recursive incremental sends missing snapshots behave correctly."