From 68d711325894a5cb58a5aabe27f351eafff04672 Mon Sep 17 00:00:00 2001 From: David Hedberg Date: Sat, 7 Jan 2023 10:45:44 +0100 Subject: [PATCH] Wait for txg sync if the last DRR_FREEOBJECTS might result in a hole If we receive a DRR_FREEOBJECTS as the first entry in an object range, this might end up producing a hole if the freed objects were the only existing objects in the block. If the txg starts syncing before we've processed any following DRR_OBJECT records, this leads to a possible race where the backing arc_buf_t gets its psize set to 0 in the arc_write_ready() callback while still being referenced from a dirty record in the open txg. To prevent this, we insert a txg_wait_synced call if the first record in the range was a DRR_FREEOBJECTS that actually resulted in one or more freed objects. Signed-off-by: David Hedberg Sponsored by: Findity AB Closes: #11893 --- module/zfs/dmu_recv.c | 26 ++++++ tests/zfs-tests/tests/Makefile.am | 1 + .../rsend/send_encrypted_freeobjects.ksh | 87 +++++++++++++++++++ 3 files changed, 114 insertions(+) create mode 100755 tests/zfs-tests/tests/functional/rsend/send_encrypted_freeobjects.ksh diff --git a/module/zfs/dmu_recv.c b/module/zfs/dmu_recv.c index 9461643ceef8..7bfc7b7ae24e 100644 --- a/module/zfs/dmu_recv.c +++ b/module/zfs/dmu_recv.c @@ -75,6 +75,12 @@ static int zfs_recv_best_effort_corrective = 0; static const void *const dmu_recv_tag = "dmu_recv_tag"; const char *const recv_clone_name = "%recv"; +typedef enum { + ORNS_NO, + ORNS_YES, + ORNS_MAYBE +} or_need_sync_t; + static int receive_read_payload_and_next_header(dmu_recv_cookie_t *ra, int len, void *buf); @@ -128,6 +134,9 @@ struct receive_writer_arg { uint8_t or_mac[ZIO_DATA_MAC_LEN]; boolean_t or_byteorder; zio_t *heal_pio; + + /* Keep track of DRR_FREEOBJECTS right after DRR_OBJECT_RANGE */ + or_need_sync_t or_need_sync; }; typedef struct dmu_recv_begin_arg { @@ -1903,10 +1912,22 @@ receive_object(struct receive_writer_arg *rwa, struct drr_object *drro, /* object was freed and we are about to allocate a new one */ object_to_hold = DMU_NEW_OBJECT; } else { + /* + * If the only record in this range so far was DRR_FREEOBJECTS + * with at least one actually freed object, it's possible that + * the block will now be converted to a hole. We need to wait + * for the txg to sync to prevent races. + */ + if (rwa->or_need_sync == ORNS_YES) + txg_wait_synced(dmu_objset_pool(rwa->os), 0); + /* object is free and we are about to allocate a new one */ object_to_hold = DMU_NEW_OBJECT; } + /* Only relevant for the first object in the range */ + rwa->or_need_sync = ORNS_NO; + /* * If this is a multi-slot dnode there is a chance that this * object will expand into a slot that is already used by @@ -2100,6 +2121,9 @@ receive_freeobjects(struct receive_writer_arg *rwa, if (err != 0) return (err); + + if (rwa->or_need_sync == ORNS_MAYBE) + rwa->or_need_sync = ORNS_YES; } if (next_err != ESRCH) return (next_err); @@ -2593,6 +2617,8 @@ receive_object_range(struct receive_writer_arg *rwa, memcpy(rwa->or_mac, drror->drr_mac, ZIO_DATA_MAC_LEN); rwa->or_byteorder = byteorder; + rwa->or_need_sync = ORNS_MAYBE; + return (0); } diff --git a/tests/zfs-tests/tests/Makefile.am b/tests/zfs-tests/tests/Makefile.am index 33f78e590876..40a172d8d87f 100644 --- a/tests/zfs-tests/tests/Makefile.am +++ b/tests/zfs-tests/tests/Makefile.am @@ -1808,6 +1808,7 @@ nobase_dist_datadir_zfs_tests_tests_SCRIPTS += \ functional/rsend/send_doall.ksh \ functional/rsend/send_encrypted_incremental.ksh \ functional/rsend/send_encrypted_files.ksh \ + functional/rsend/send_encrypted_freeobjects.ksh \ functional/rsend/send_encrypted_hierarchy.ksh \ functional/rsend/send_encrypted_props.ksh \ functional/rsend/send_encrypted_truncated_files.ksh \ diff --git a/tests/zfs-tests/tests/functional/rsend/send_encrypted_freeobjects.ksh b/tests/zfs-tests/tests/functional/rsend/send_encrypted_freeobjects.ksh new file mode 100755 index 000000000000..92451bd1ab6f --- /dev/null +++ b/tests/zfs-tests/tests/functional/rsend/send_encrypted_freeobjects.ksh @@ -0,0 +1,87 @@ +#!/bin/ksh + +# +# 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) 2017 by Lawrence Livermore National Security, LLC. +# Copyright (c) 2023 by Findity AB +# + +. $STF_SUITE/tests/functional/rsend/rsend.kshlib + +# +# Description: +# Verify that receiving a raw encrypted stream, with a FREEOBJECTS +# removing all existing objects in a block followed by an OBJECT write +# to the same block, does not result in a panic. +# +# Strategy: +# 1. Create a new encrypted filesystem +# 2. Create file f1 as the first object in some block (here object 128) +# 3. Take snapshot A +# 4. Create file f2 as the second object in the same block (here object 129) +# 5. Delete f1 +# 6. Take snapshot B +# 7. Receive a full raw encrypted send of A +# 8. Receive an incremental raw send of B +# +verify_runnable "both" + +function create_object_with_num +{ + file=$1 + num=$2 + + tries=100 + for ((i=0; i<$tries; i++)); do + touch $file + onum=$(ls -li $file | awk '{print $1}') + + if [[ $onum -ne $num ]] ; then + rm -f $file + else + break + fi + done + if [[ $i -eq $tries ]]; then + log_fail "Failed to create object with number $num" + fi +} + +log_assert "FREEOBJECTS followed by OBJECT in encrypted stream does not crash" + +sendds=sendencfods +recvds=recvencfods +keyfile=/$POOL/keyencfods +f1=/$POOL/$sendds/f1 +f2=/$POOL/$sendds/f2 + +log_must eval "echo 'password' > $keyfile" + +# +# xattr=sa and dnodesize=legacy for sequential object numbers, see +# note in send_freeobjects.ksh. +# +log_must zfs create -o xattr=sa -o dnodesize=legacy -o encryption=on \ + -o keyformat=passphrase -o keylocation=file://$keyfile $POOL/$sendds + +create_object_with_num $f1 128 +log_must zfs snap $POOL/$sendds@A +create_object_with_num $f2 129 +log_must rm $f1 +log_must zfs snap $POOL/$sendds@B + +log_must eval "zfs send -w $POOL/$sendds@A | zfs recv $POOL/$recvds" +log_must eval "zfs send -w -i $POOL/$sendds@A $POOL/$sendds@B |" \ + "zfs recv $POOL/$recvds" + +log_pass "FREEOBJECTS followed by OBJECT in encrypted stream did not crash"