From 011cc775f87eb82167db2afb62b8e1b544d24c6a Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Mon, 22 Jun 2020 13:44:12 -0700 Subject: [PATCH 1/3] s3: selftest: Add samba3.blackbox.aio-outstanding test. Shows smbd panics if connection is terminated (torn down) by killing the client with outstanding aio requests in the queue. As we're closing smbd we should cope with this. Followup-bugfix for: BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison --- selftest/knownfail.d/aio_outstanding | 2 + source3/script/tests/test_aio_outstanding.sh | 94 ++++++++++++++++++++ source3/selftest/tests.py | 8 ++ 3 files changed, 104 insertions(+) create mode 100644 selftest/knownfail.d/aio_outstanding create mode 100755 source3/script/tests/test_aio_outstanding.sh diff --git a/selftest/knownfail.d/aio_outstanding b/selftest/knownfail.d/aio_outstanding new file mode 100644 index 00000000000..6426f760cb1 --- /dev/null +++ b/selftest/knownfail.d/aio_outstanding @@ -0,0 +1,2 @@ +samba3.blackbox.aio-outstanding + diff --git a/source3/script/tests/test_aio_outstanding.sh b/source3/script/tests/test_aio_outstanding.sh new file mode 100755 index 00000000000..6a947e15449 --- /dev/null +++ b/source3/script/tests/test_aio_outstanding.sh @@ -0,0 +1,94 @@ +#!/bin/bash +# +# Test terminating an smbclient connection with outstanding +# aio requests. +# +# Note this is designed to be run against +# the aio_delay_inject share which is preconfigured +# with 2 second delays on pread/pwrite. + +if [ $# -lt 4 ]; then + echo Usage: test_aio_outstanding.sh \ + SERVERCONFFILE SMBCLIENT IP aio_delay_inject_sharename +exit 1 +fi + +CONF=$1 +SMBCLIENT=$2 +SERVER=$3 +SHARE=$4 + +incdir=$(dirname $0)/../../../testprogs/blackbox +. $incdir/subunit.sh + +failed=0 +# +# Note if we already have any panics in the smbd log. +# +panic_count_0=$(grep -c PANIC $SMBD_TEST_LOG) + +# Create the smbclient communication pipes. +rm -f smbclient-stdin smbclient-stdout smbclient-stderr +mkfifo smbclient-stdin smbclient-stdout smbclient-stderr + +# Create a large-ish testfile +rm testfile +head -c 20MB /dev/zero >testfile + +CLI_FORCE_INTERACTIVE=1; export CLI_FORCE_INTERACTIVE + +${SMBCLIENT} //${SERVER}/${SHARE} ${CONF} -U${USER}%${PASSWORD} \ + < smbclient-stdin > smbclient-stdout 2>smbclient-stderr & +CLIENT_PID=$! + +sleep 1 + +exec 100>smbclient-stdin 101&100 +echo "put testfile" >&100 + +sleep 2 + +# Terminate the smbclient write to the aio_delay_inject share whilst +# we have outstanding writes. +kill $CLIENT_PID + +sleep 1 + +# Ensure the panic count didn't change. +# +# BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 +# + +panic_count_1=$(grep -c PANIC $SMBD_TEST_LOG) + +# Rerun smbclient to remove the testfile on the server. +rm -f smbclient-stdin smbclient-stdout smbclient-stderr testfile +mkfifo smbclient-stdin smbclient-stdout + +${SMBCLIENT} //${SERVER}/${SHARE} ${CONF} -U${USER}%${PASSWORD} \ + < smbclient-stdin > smbclient-stdout & +CLIENT_PID=$! + +sleep 1 + +exec 100>smbclient-stdin 101&100 + +sleep 1 + +kill ${CLIENT_PID} + +rm -f smbclient-stdin smbclient-stdout testfile + +testit "check_panic" test $panic_count_0 -eq $panic_count_1 || + failed=$(expr $failed + 1) + +testok $0 $failed diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py index cfca26359c3..e7794fac71c 100755 --- a/source3/selftest/tests.py +++ b/source3/selftest/tests.py @@ -943,6 +943,14 @@ plantestsuite("samba3.blackbox.force-close-share", "simpleserver:local", '$SERVER_IP', "aio_delay_inject"]) +plantestsuite("samba3.blackbox.aio-outstanding", "simpleserver:local", + [os.path.join(samba3srcdir, + "script/tests/test_aio_outstanding.sh"), + configuration, + os.path.join(bindir(), "smbclient"), + '$SERVER_IP', + "aio_delay_inject"]) + plantestsuite("samba3.blackbox.open-eintr", "simpleserver:local", [os.path.join(samba3srcdir, "script/tests/test_open_eintr.sh"), -- 2.20.1 From 97df2cabef71266e2dea90d71110b3f01bdb84dd Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 16 Jun 2020 14:58:54 -0700 Subject: [PATCH 2/3] s3: smbd: Make smbXsrv_client_valid_connections() external. We will need to this ensure our client connections are terminated in close_file before exiting with outstanding aio. Followup-bugfix for: BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison --- source3/smbd/globals.h | 1 + source3/smbd/smb2_server.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/source3/smbd/globals.h b/source3/smbd/globals.h index d3b6ac2ffe6..2a963439bef 100644 --- a/source3/smbd/globals.h +++ b/source3/smbd/globals.h @@ -222,6 +222,7 @@ void smbd_notify_cancel_by_smbreq(const struct smb_request *smbreq); void smbXsrv_connection_disconnect_transport(struct smbXsrv_connection *xconn, NTSTATUS status); +size_t smbXsrv_client_valid_connections(struct smbXsrv_client *client); void smbd_server_connection_terminate_ex(struct smbXsrv_connection *xconn, const char *reason, const char *location); diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c index 0a5083b5b8f..b58d3fbf097 100644 --- a/source3/smbd/smb2_server.c +++ b/source3/smbd/smb2_server.c @@ -1121,7 +1121,7 @@ void smbXsrv_connection_disconnect_transport(struct smbXsrv_connection *xconn, DO_PROFILE_INC(disconnect); } -static size_t smbXsrv_client_valid_connections(struct smbXsrv_client *client) +size_t smbXsrv_client_valid_connections(struct smbXsrv_client *client) { struct smbXsrv_connection *xconn = NULL; size_t num_ok = 0; -- 2.20.1 From 4a1bf9ed57e9231a03d56f4ef15d6b12c497eda4 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 16 Jun 2020 15:01:49 -0700 Subject: [PATCH 3/3] s3: smbd: Allow a SHUTDOWN_CLOSE on a file with outstanding aio if there are no client connections alive. The process is exiting now so pthreads will never complete to cause problems. Remove the knownfail.d/aio_outstanding entry. Followup-bugfix for: BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison --- selftest/knownfail.d/aio_outstanding | 2 -- source3/smbd/close.c | 17 +++++++++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) delete mode 100644 selftest/knownfail.d/aio_outstanding diff --git a/selftest/knownfail.d/aio_outstanding b/selftest/knownfail.d/aio_outstanding deleted file mode 100644 index 6426f760cb1..00000000000 --- a/selftest/knownfail.d/aio_outstanding +++ /dev/null @@ -1,2 +0,0 @@ -samba3.blackbox.aio-outstanding - diff --git a/source3/smbd/close.c b/source3/smbd/close.c index 3169c8d5487..e1491f8e6d0 100644 --- a/source3/smbd/close.c +++ b/source3/smbd/close.c @@ -639,12 +639,29 @@ static NTSTATUS ntstatus_keeperror(NTSTATUS s1, NTSTATUS s2) static void assert_no_pending_aio(struct files_struct *fsp, enum file_close_type close_type) { + struct smbXsrv_client *client = global_smbXsrv_client; + size_t num_connections_alive; unsigned num_requests = fsp->num_aio_requests; if (num_requests == 0) { return; } + num_connections_alive = smbXsrv_client_valid_connections(client); + + if (close_type == SHUTDOWN_CLOSE && num_connections_alive == 0) { + /* + * Ensure we deallocate here, as fsp->aio_requests[0] + * is owned by fsp, not fsp->aio_requests. + * fsp_aio_requests[x] gets orphaned if we don't + * delete here. + */ + while (fsp->num_aio_requests != 0) { + TALLOC_FREE(fsp->aio_requests[0]); + } + return; + } + DBG_ERR("fsp->num_aio_requests=%u\n", num_requests); smb_panic("can not close with outstanding aio requests"); return; -- 2.20.1