From cabe011118a58d08b0e4cdb89a9380f7c127a5e0 Mon Sep 17 00:00:00 2001 From: Quentin Armitage Date: Sun, 28 Jan 2024 12:21:49 +0000 Subject: [PATCH] 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 --- keepalived/check/check_data.c | 2 +- keepalived/check/ipvswrapper.c | 40 +++++++++++++-------------------- keepalived/check/ipwrapper.c | 24 +++++++++----------- keepalived/include/check_api.h | 4 ++++ keepalived/include/check_data.h | 13 ++++++++--- 5 files changed, 40 insertions(+), 43 deletions(-) diff --git a/keepalived/check/check_data.c b/keepalived/check/check_data.c index 9081719c47..1a17cbcdde 100644 --- 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 index d528ac1822..779ca7a0d4 100644 --- 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 index 31a2a42bf7..7e54a27b05 100644 --- 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 index 5ccfe5ea4e..f13be751f8 100644 --- 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 index a0ca4c0435..9b9d828559 100644 --- 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; }