Do not fence connections without valid greeting

There is no reason to fence any connection that hasn't sent a valid
greeting, since they haven't progressed far enough for it to make
sense. Skip the fence call for these connections and let the existing
destroy path tear them down. Any real client will reconnect.

server_notify_down() previously treated any zero rid teardown as
the listening socket going down and called stop_server(). That's
correct for the listener legitimately, but not for any conn that
never completed a greeting.

Adds a test that sends a short garbage payload to each quorum port and
verifies that no greeting-less connection remains past the reconnect
timeout (20s) and that the filesystem still works afterwards.

Signed-off-by: Auke Kok <auke.kok@versity.com>

kmod/src/lock_server.c

@@ -626,14 +626,9 @@ out:
 int scoutfs_lock_server_greeting(struct super_block *sb, u64 rid)
 {
 	struct scoutfs_key key;
-	bool pending;
 	int ret;
 
-	pending = scoutfs_recov_is_pending(sb, rid, SCOUTFS_RECOV_LOCKS);
-
-	trace_scoutfs_lock_server_greeting(sb, rid, pending);
-
-	if (pending) {
+	if (scoutfs_recov_is_pending(sb, rid, SCOUTFS_RECOV_LOCKS)) {
 		scoutfs_key_set_zeros(&key);
 		ret = scoutfs_server_lock_recover_request(sb, rid, &key);
 	} else {

kmod/src/net.c

@@ -1452,15 +1452,23 @@ restart:
 			set_conn_fl(acc, reconn_freeing);
 			spin_unlock(&conn->lock);
 			if (!test_conn_fl(conn, shutting_down)) {
-				scoutfs_info(sb, "client "SIN_FMT" reconnect timed out, fencing",
-					     SIN_ARG(&acc->last_peername));
-				ret = scoutfs_fence_start(sb, acc->rid,
-						acc->last_peername.sin_addr.s_addr,
-						SCOUTFS_FENCE_CLIENT_RECONNECT);
-				if (ret) {
-					scoutfs_err(sb, "client fence returned err %d, shutting down server",
-						    ret);
-					scoutfs_server_stop(sb);
+				/*
+				 * Connections that never completed a valid greeting
+				 * (port scans, malformed traffic, half-open peers)
+				 * haven't progressed far enough to warrant fencing.
+				 * Drop them. Any real client will reconnect.
+				 */
+				if (test_conn_fl(acc, valid_greeting)) {
+					scoutfs_info(sb, "client "SIN_FMT" reconnect timed out, fencing",
+						     SIN_ARG(&acc->last_peername));
+					ret = scoutfs_fence_start(sb, acc->rid,
+							acc->last_peername.sin_addr.s_addr,
+							SCOUTFS_FENCE_CLIENT_RECONNECT);
+					if (ret) {
+						scoutfs_err(sb, "client fence returned err %d, shutting down server",
+							    ret);
+						scoutfs_server_stop(sb);
+					}
 				}
 			}
 			destroy_conn(acc);
@@ -1730,30 +1738,12 @@ int scoutfs_net_connect(struct super_block *sb,
 
 static void set_valid_greeting(struct scoutfs_net_connection *conn)
 {
-	struct net_info *ninf = SCOUTFS_SB(conn->sb)->net_info;
-	struct message_send *msend;
-	struct message_send *tmp;
-
 	assert_spin_locked(&conn->lock);
 
 	/* recv should have dropped invalid duplicate greeting messages */
 	BUG_ON(test_conn_fl(conn, valid_greeting));
 
 	set_conn_fl(conn, valid_greeting);
-
-	/*
-	 * Drop greetings from the resend_queue before splicing it into
-	 * the send_queue. We might have a greeting left in the resend
-	 * queue at the moment that we reach this point. A duplicate
-	 * greeting is treated as fatal and causes a stall and fence.
-	 */
-	list_for_each_entry_safe(msend, tmp, &conn->resend_queue, head) {
-		if (msend->nh.cmd == SCOUTFS_NET_CMD_GREETING) {
-			msend->dead = 1;
-			free_msend(ninf, conn, msend);
-		}
-	}
-
 	list_splice_tail_init(&conn->resend_queue, &conn->send_queue);
 	queue_work(conn->workq, &conn->send_work);
 }

kmod/src/recov.c

@@ -21,7 +21,6 @@
 #include "super.h"
 #include "recov.h"
 #include "cmp.h"
-#include "scoutfs_trace.h"
 
 /*
  * There are a few server messages which can't be processed until they
@@ -120,9 +119,6 @@ int scoutfs_recov_prepare(struct super_block *sb, u64 rid, int which)
 	spin_unlock(&recinf->lock);
 
 	kfree(alloc);
-
-	trace_scoutfs_recov_prepare(sb, rid, which);
-
 	return 0;
 }
 
@@ -139,15 +135,6 @@ static int recov_finished(struct recov_info *recinf)
 static void timer_callback(struct timer_list *timer)
 {
 	struct recov_info *recinf = from_timer(recinf, timer, timer);
-	struct recov_pending *pend;
-	int nr = 0;
-
-	spin_lock(&recinf->lock);
-	list_for_each_entry(pend, &recinf->pending, head)
-		nr++;
-	spin_unlock(&recinf->lock);
-
-	trace_scoutfs_recov_timeout_fire(recinf->sb, nr);
 
 	recinf->timeout_fn(recinf->sb);
 }
@@ -194,8 +181,6 @@ int scoutfs_recov_finish(struct super_block *sb, u64 rid, int which)
 {
 	DECLARE_RECOV_INFO(sb, recinf);
 	struct recov_pending *pend;
-	struct recov_pending *iter;
-	int remaining = 0;
 	int ret = 0;
 
 	spin_lock(&recinf->lock);
@@ -211,13 +196,8 @@ int scoutfs_recov_finish(struct super_block *sb, u64 rid, int which)
 		}
 	}
 
-	list_for_each_entry(iter, &recinf->pending, head)
-		remaining++;
-
 	spin_unlock(&recinf->lock);
 
-	trace_scoutfs_recov_finish(sb, rid, which, remaining);
-
 	if (ret > 0)
 		del_timer_sync(&recinf->timer);
 

kmod/src/scoutfs_trace.h

@@ -2121,110 +2121,6 @@ DEFINE_EVENT(scoutfs_server_client_count_class, scoutfs_server_client_down,
 	TP_ARGS(sb, rid, nr_clients)
 );
 
-TRACE_EVENT(scoutfs_recov_prepare,
-	TP_PROTO(struct super_block *sb, u64 rid, int which),
-
-	TP_ARGS(sb, rid, which),
-
-	TP_STRUCT__entry(
-		SCSB_TRACE_FIELDS
-		__field(__u64, c_rid)
-		__field(int, which)
-	),
-
-	TP_fast_assign(
-		SCSB_TRACE_ASSIGN(sb);
-		__entry->c_rid = rid;
-		__entry->which = which;
-	),
-
-	TP_printk(SCSBF" rid %016llx which 0x%x",
-		  SCSB_TRACE_ARGS, __entry->c_rid, __entry->which)
-);
-
-TRACE_EVENT(scoutfs_recov_finish,
-	TP_PROTO(struct super_block *sb, u64 rid, int which, int remaining),
-
-	TP_ARGS(sb, rid, which, remaining),
-
-	TP_STRUCT__entry(
-		SCSB_TRACE_FIELDS
-		__field(__u64, c_rid)
-		__field(int, which)
-		__field(int, remaining)
-	),
-
-	TP_fast_assign(
-		SCSB_TRACE_ASSIGN(sb);
-		__entry->c_rid = rid;
-		__entry->which = which;
-		__entry->remaining = remaining;
-	),
-
-	TP_printk(SCSBF" rid %016llx which 0x%x remaining %d",
-		  SCSB_TRACE_ARGS, __entry->c_rid, __entry->which,
-		  __entry->remaining)
-);
-
-TRACE_EVENT(scoutfs_recov_timeout_fire,
-	TP_PROTO(struct super_block *sb, int nr_pending),
-
-	TP_ARGS(sb, nr_pending),
-
-	TP_STRUCT__entry(
-		SCSB_TRACE_FIELDS
-		__field(int, nr_pending)
-	),
-
-	TP_fast_assign(
-		SCSB_TRACE_ASSIGN(sb);
-		__entry->nr_pending = nr_pending;
-	),
-
-	TP_printk(SCSBF" nr_pending %d",
-		  SCSB_TRACE_ARGS, __entry->nr_pending)
-);
-
-TRACE_EVENT(scoutfs_recov_fence_rid,
-	TP_PROTO(struct super_block *sb, u64 rid),
-
-	TP_ARGS(sb, rid),
-
-	TP_STRUCT__entry(
-		SCSB_TRACE_FIELDS
-		__field(__u64, c_rid)
-	),
-
-	TP_fast_assign(
-		SCSB_TRACE_ASSIGN(sb);
-		__entry->c_rid = rid;
-	),
-
-	TP_printk(SCSBF" rid %016llx",
-		  SCSB_TRACE_ARGS, __entry->c_rid)
-);
-
-TRACE_EVENT(scoutfs_lock_server_greeting,
-	TP_PROTO(struct super_block *sb, u64 rid, bool recov_pending),
-
-	TP_ARGS(sb, rid, recov_pending),
-
-	TP_STRUCT__entry(
-		SCSB_TRACE_FIELDS
-		__field(__u64, c_rid)
-		__field(bool, recov_pending)
-	),
-
-	TP_fast_assign(
-		SCSB_TRACE_ASSIGN(sb);
-		__entry->c_rid = rid;
-		__entry->recov_pending = recov_pending;
-	),
-
-	TP_printk(SCSBF" rid %016llx recov_pending %d",
-		  SCSB_TRACE_ARGS, __entry->c_rid, __entry->recov_pending)
-);
-
 DECLARE_EVENT_CLASS(scoutfs_server_commit_users_class,
         TP_PROTO(struct super_block *sb, int holding, int applying,
 		 int nr_holders, u32 budget,

kmod/src/server.c

@@ -4315,7 +4315,8 @@ static void server_notify_down(struct super_block *sb,
 		spin_unlock(&server->lock);
 
 		free_farewell_requests(sb, rid);
-	} else {
+	} else if (!conn->listening_conn) {
+		/* only the listener going down should stop the server */
 		stop_server(server);
 	}
 }
@@ -4386,8 +4387,6 @@ static void fence_pending_recov_worker(struct work_struct *work)
 		scoutfs_err(sb, "%lu ms recovery timeout expired for client rid %016llx, fencing",
 			    SERVER_RECOV_TIMEOUT_MS, rid);
 
-		trace_scoutfs_recov_fence_rid(sb, rid);
-
 		ret = lookup_mounted_client_addr(sb, rid, &addr);
 		if (ret < 0) {
 			scoutfs_err(sb, "client rid addr lookup err %d, shutting down server", ret);

tests/golden/portscan

@@ -0,0 +1,2 @@
+== send empty payload to a quorum port
+== greeting-less connections still in reconn_wait

tests/sequence

@@ -64,4 +64,5 @@ archive-light-cycle.sh
 block-stale-reads.sh
 inode-deletion.sh
 renameat2-noreplace.sh
+portscan.sh
 xfstests.sh

tests/tests/portscan.sh

@@ -0,0 +1,46 @@
+#
+# portscan tests - assure malformed packets do not cause issues
+#
+# Send a short garbage payload to a scoutfs server quorum port. The
+# accepted connection never completes a valid greeting, so after the
+# reconnect timeout the kernel must drop it silently rather than
+# fence it (which would restart the server).
+#
+
+t_require_commands scoutfs grep wc seq
+
+send_garbage()
+{
+	local port="$1"
+
+	(
+		exec 3<>"/dev/tcp/127.0.0.1/$port" || exit 1
+		printf '    ' >&3
+		exec 3>&-
+	) 2>/dev/null
+}
+
+echo "== send empty payload to a quorum port"
+slot=-1
+for i in $(seq 0 $((T_QUORUM - 1))); do
+	if send_garbage "$((T_TEST_PORT + i))"; then
+		slot=$i
+		break
+	fi
+done
+test "$slot" -ge 0 || t_fail "no quorum port accepted"
+
+# CLIENT_RECONNECT_TIMEOUT_MS is 20s - wait until that happens.
+echo "== greeting-less connections still in reconn_wait"
+for _ in $(seq 1 25); do
+	n=$(grep -h 'vg 0 .* rw 1' /sys/kernel/debug/scoutfs/*/connections | wc -l)
+	[ "$n" = 0 ] && break
+	sleep 1
+done
+test "$n" -eq 0 || t_fail "$n greeting-less conns remain in reconn_wait"
+
+# the mount whose port we hit should be up and not disconnected now.
+eval dir=\$T_D$slot
+touch "$dir/portscan-after" 2>/dev/null || t_fail "fs on $dir not responsive after portscan"
+
+t_pass