ipvs: don't call protocol_to_index() unless using auto fwmarks

protocol_to_index() must only be called when there is an index. This is when the virtual server uses a virtual server group that is using auto fwmarks. Signed-off-by: Quentin Armitage <quentin@armitage.org.uk>
pqarmitage · Jan 28, 2024 · cabe011 · cabe011
1 parent d9cce81
commit cabe011
Show file tree

Hide file tree

Showing 5 changed files with 40 additions and 43 deletions.
diff --git a/keepalived/check/check_data.c b/keepalived/check/check_data.c
@@ -715,7 +715,7 @@ dump_vs(FILE *fp, const virtual_server_t *vs)
 		conf_write(fp, "   protocol = UDP");
 	else if (vs->service_type == IPPROTO_SCTP)
 		conf_write(fp, "   protocol = SCTP");
-	else if (vs->service_type == 0)
+	else if (vs->service_type == IPPROTO_IP)
 		conf_write(fp, "   protocol = none");
 	else
 		conf_write(fp, "   protocol = %d", vs->service_type);

diff --git a/keepalived/check/ipvswrapper.c b/keepalived/check/ipvswrapper.c
@@ -42,6 +42,7 @@
 #include "namespaces.h"
 #ifdef _WITH_NFTABLES_
 #include "check_nftables.h"
+#include "check_data.h"
 #endif
 
 static bool no_ipvs = false;
@@ -580,32 +581,30 @@ ipvs_cmd(int cmd, virtual_server_t *vs, real_server_t *rs)
 
 	/* Set vs rule and send to kernel */
 #ifdef _WITH_NFTABLES_
-	if (vs->service_type)
+	if (VS_USES_VSG_AUTO_FWMARK(vs))
 		proto_index = protocol_to_index(vs->service_type);
 	else
-		proto_index = PROTO_INDEX_MAX;
+		proto_index = PROTO_INDEX_NONE;
 #endif
 
 	if (vs->vsg) {
 #ifdef _WITH_NFTABLES_
 		if (cmd == IP_VS_SO_SET_ADD &&
-		    global_data->ipvs_nf_table_name &&
-		    proto_index < PROTO_INDEX_MAX &&
-		    list_empty(&vs->vsg->vfwmark) &&
+		    VS_USES_VSG_AUTO_FWMARK(vs) &&
 		    !vs->vsg->auto_fwmark[proto_index]) {
 			vs->vsg->auto_fwmark[proto_index] = set_vs_fwmark(vs);
-		} else if (proto_index == PROTO_INDEX_MAX || !vs->vsg->auto_fwmark[proto_index])
+		} else if (proto_index == PROTO_INDEX_NONE || !vs->vsg->auto_fwmark[proto_index])
 #endif
 			return ipvs_group_cmd(cmd, &srule, &drule, vs, rs);
 	}
 
 	if (vs->vfwmark
 #ifdef _WITH_NFTABLES_
-			|| (vs->vsg && proto_index < PROTO_INDEX_MAX && vs->vsg->auto_fwmark[proto_index])
+			|| VS_USES_VSG_AUTO_FWMARK(vs)
 #endif
 								) {
 #ifdef _WITH_NFTABLES_
-		srule.user.fwmark = vs->vsg ? vs->vsg->auto_fwmark[proto_index] : vs->vfwmark;
+		srule.user.fwmark = VS_USES_VSG_AUTO_FWMARK(vs) ? vs->vsg->auto_fwmark[proto_index] : vs->vfwmark;
 #else
 		srule.user.fwmark = vs->vfwmark;
 #endif
@@ -630,7 +629,7 @@ ipvs_cmd(int cmd, virtual_server_t *vs, real_server_t *rs)
 	    vs->af == AF_UNSPEC &&
 	    vs->vsg->have_ipv4 &&
 	    vs->vsg->have_ipv6 &&
-	    proto_index < PROTO_INDEX_MAX &&
+	    proto_index != PROTO_INDEX_NONE &&
 	    vs->vsg->auto_fwmark[proto_index]) {
 		srule.af = AF_INET6;
 		srule.user.netmask = 128;
@@ -648,14 +647,11 @@ ipvs_group_sync_entry(virtual_server_t *vs, virtual_server_group_entry_t *vsge)
 	real_server_t *rs;
 	ipvs_service_t srule;
 	ipvs_dest_t drule;
-#ifdef _WITH_NFTABLES_
-	proto_index_t proto_index = protocol_to_index(vs->service_type);
-#endif
 
 	ipvs_set_srule(IP_VS_SO_SET_ADDDEST, &srule, vs);
 #ifdef _WITH_NFTABLES_
-	if (vs->vsg->auto_fwmark[proto_index])
-		srule.user.fwmark = vs->vsg->auto_fwmark[proto_index];
+	if (VS_USES_VSG_AUTO_FWMARK(vs))
+		srule.user.fwmark = vs->vsg->auto_fwmark[protocol_to_index(vs->service_type)];
 	else
 #endif
 	if (vsge->is_fwmark)
@@ -696,13 +692,10 @@ ipvs_group_remove_entry(virtual_server_t *vs, virtual_server_group_entry_t *vsge
 	real_server_t *rs;
 	ipvs_service_t srule;
 	ipvs_dest_t drule;
-#ifdef _WITH_NFTABLES_
-	proto_index_t proto_index = protocol_to_index(vs->service_type);
-#endif
 
 #ifdef _WITH_NFTABLES_
 	/* Prepare target rules */
-	if (vs->vsg->auto_fwmark[proto_index]) {
+	if (VS_USES_VSG_AUTO_FWMARK(vs)) {
 		/* Remove the fwmark entry(s) */
 		remove_vs_fwmark_entry(vs, vsge);
 
@@ -716,8 +709,8 @@ ipvs_group_remove_entry(virtual_server_t *vs, virtual_server_group_entry_t *vsge
 
 	ipvs_set_srule(IP_VS_SO_SET_DELDEST, &srule, vs);
 #ifdef _WITH_NFTABLES_
-	if (vs->vsg->auto_fwmark[proto_index])
-		srule.user.fwmark = vs->vsg->auto_fwmark[proto_index];
+	if (VS_USES_VSG_AUTO_FWMARK(vs))
+		srule.user.fwmark = vs->vsg->auto_fwmark[protocol_to_index(vs->service_type)];
 	else
 #endif
 	if (vsge->is_fwmark)
@@ -892,9 +885,6 @@ ipvs_update_stats(virtual_server_t *vs)
 	real_server_t *rs;
 	time_t cur_time = time(NULL);
 	uint16_t af;
-#ifdef _WITH_NFTABLES_
-	proto_index_t proto_index = protocol_to_index(vs->service_type);
-#endif
 
 	if (cur_time - vs->lastupdated < STATS_REFRESH)
 		return;
@@ -916,8 +906,8 @@ ipvs_update_stats(virtual_server_t *vs)
 	if (vs->vsg) {
 		for (af = (vs->vsg->have_ipv4) ? AF_INET : AF_INET6; af != AF_UNSPEC; af = af == AF_INET && vs->vsg->have_ipv6 ? AF_INET6 : AF_UNSPEC) {
 #ifdef _WITH_NFTABLES_
-			if (global_data->ipvs_nf_table_name && vs->vsg->auto_fwmark[proto_index])
-				ipvs_update_vs_stats(vs, af, vs->vsg->auto_fwmark[proto_index], &nfaddr, 0);
+			if (VS_USES_VSG_AUTO_FWMARK(vs))
+				ipvs_update_vs_stats(vs, af, vs->vsg->auto_fwmark[protocol_to_index(vs->service_type)], &nfaddr, 0);
 			else
 #endif
 			{

diff --git a/keepalived/check/ipwrapper.c b/keepalived/check/ipwrapper.c
@@ -39,6 +39,7 @@
 #include "track_file.h"
 #ifdef _WITH_NFTABLES_
 #include "check_nftables.h"
+#include "check_data.h"
 #endif
 
 static bool __attribute((pure))
@@ -358,7 +359,7 @@ clear_service_vs(virtual_server_t * vs, bool stopping)
 	/* The above will handle Omega case for VS as well. */
 
 #ifdef _WITH_NFTABLES_
-	if (vs->vsg && vs->vsg->auto_fwmark[protocol_to_index(vs->service_type)])
+	if (VS_USES_VSG_AUTO_FWMARK(vs))
 		clear_vs_fwmark(vs);
 #endif
 
@@ -627,17 +628,11 @@ perform_svr_state(bool alive, checker_t *checker)
 static bool
 init_service_vs(virtual_server_t * vs)
 {
-#ifdef _WITH_NFTABLES_
-	proto_index_t proto_index = 0;
-
-	if (vs->service_type != AF_UNSPEC)
-		proto_index = protocol_to_index(vs->service_type);
-#endif
-
 	/* Init the VS root */
 	if (!ISALIVE(vs) || vs->vsg) {
 #ifdef _WITH_NFTABLES_
-		if (ISALIVE(vs) && vs->vsg && (vs->service_type == AF_UNSPEC || vs->vsg->auto_fwmark[proto_index]))
+		if (ISALIVE(vs) &&
+		    VS_USES_VSG_AUTO_FWMARK(vs))
 			set_vs_fwmark(vs);
 		else
 #endif
@@ -650,9 +645,9 @@ init_service_vs(virtual_server_t * vs)
 	/* Processing real server queue */
 	init_service_rs(vs);
 
-	if (vs->reloaded && vs->vsgname
+	if (vs->reloaded && vs->vsg
 #ifdef _WITH_NFTABLES_
-	    && !vs->vsg->auto_fwmark[proto_index]
+	    && !VS_USES_VSG_AUTO_FWMARK(vs)
 #endif
 				    ) {
 		/* add reloaded dests into new vsg entries */
@@ -893,9 +888,10 @@ clear_diff_vsg(virtual_server_t *old_vs, virtual_server_t *new_vs)
 	virtual_server_group_t *new = new_vs->vsg;
 #ifdef _WITH_NFTABLES_
 	bool vsg_already_done;
-	proto_index_t proto_index = protocol_to_index(new_vs->service_type);
+	proto_index_t proto_index;
 
-	if (old_vs->vsg->auto_fwmark[proto_index]) {
+	if (VS_USES_VSG_AUTO_FWMARK(old_vs)) {
+		proto_index = protocol_to_index(new_vs->service_type);
 		vsg_already_done = !!new_vs->vsg->auto_fwmark[proto_index];
 
 		new_vs->vsg->auto_fwmark[proto_index] = old_vs->vsg->auto_fwmark[proto_index];
@@ -1117,7 +1113,7 @@ clear_diff_services(void)
 	/* Remove diff entries from previous IPVS rules */
 	list_for_each_entry(vs, &old_check_data->vs, e_list) {
 		/*
-		 * Try to find this vs into the new conf data
+		 * Try to find this vs in the new conf data
 		 * reloaded.
 		 */
 		new_vs = vs_exist(vs);

diff --git a/keepalived/include/check_api.h b/keepalived/include/check_api.h
@@ -101,6 +101,10 @@ extern checker_t *current_checker;
 #define CHECKER_ARG(X) ((X)->data)
 #define CHECKER_NEW_CO() ((conn_opts_t *) MALLOC(sizeof (conn_opts_t)))
 #define FMT_CHK(C) FMT_RS((C)->rs, (C)->vs)
+#ifdef _WITH_NFTABLES_
+#define VSG_USES_AUTO_FWMARK(vsg)	(global_data->ipvs_nf_table_name && list_empty(&vsg->vfwmark))
+#define VS_USES_VSG_AUTO_FWMARK(vs)	(vs->vsg && VSG_USES_AUTO_FWMARK(vs->vsg))
+#endif
 
 #ifdef _CHECKER_DEBUG_
 extern bool do_checker_debug;

diff --git a/keepalived/include/check_data.h b/keepalived/include/check_data.h
@@ -58,7 +58,8 @@ typedef enum {
 	TCP_INDEX,
 	UDP_INDEX,
 	SCTP_INDEX,
-	PROTO_INDEX_MAX
+	PROTO_INDEX_NONE,
+	PROTO_INDEX_MAX = PROTO_INDEX_NONE
 } proto_index_t;
 #endif
 
@@ -274,8 +275,14 @@ real_weight(int64_t effective_weight)
 }
 
 #ifdef _WITH_NFTABLES_
+
+/* We need to ensure that the file/function/line logged relates to where
+ * protocol_to_index() is called from.
+ */
+#define protocol_to_index(proto)	protocol_to_index_flf(proto, __func__, __LINE__, __FILE__)
+
 static inline proto_index_t
-protocol_to_index(int proto)
+protocol_to_index_flf(int proto, const char *func, int line, const char *file)
 {
 	if (proto == IPPROTO_TCP)
 		return TCP_INDEX;
@@ -284,7 +291,7 @@ protocol_to_index(int proto)
 	if (proto == IPPROTO_SCTP)
 		return SCTP_INDEX;
 
-	log_message(LOG_INFO, "Unknown protocol %d at %s:%d in %s", proto, __func__, __LINE__, __FILE__);
+	log_message(LOG_INFO, "Unknown protocol %d in protocol_to_index() called from %s at %s:%d", proto, func, file, line);
 
 	return UDP_INDEX;
 }