From 5ee7c719e1480f66eaa7ec0534c6e93d78bfce6f Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 23 Mar 2021 01:52:07 +0900 Subject: [PATCH] firewall-util: do not use goto for retrying --- src/shared/firewall-util-nft.c | 132 +++++++++++++++++---------------- 1 file changed, 70 insertions(+), 62 deletions(-) diff --git a/src/shared/firewall-util-nft.c b/src/shared/firewall-util-nft.c index fb5857d9ec..b661a81ff7 100644 --- a/src/shared/firewall-util-nft.c +++ b/src/shared/firewall-util-nft.c @@ -810,40 +810,6 @@ static int nft_message_add_setelem_iprange(sd_netlink_message *m, return 0; } -/* When someone runs 'nft flush ruleset' in the same net namespace - * this will also tear down the systemd nat table. - * - * Unlike iptables -t nat -F (which will remove all rules added by the - * systemd iptables backend, iptables has builtin chains that cannot be - * deleted -- the next add operation will 'just work'. - * - * In the nftables case, everything gets removed. The next add operation - * will yield -ENOENT. - * - * If we see -ENOENT on add, replay the initial table setup. - * If that works, re-do the add operation. - * - * Note that this doesn't protect against external sabotage such as a - * 'while true; nft flush ruleset;done'. There is nothing that could be - * done about that short of extending the kernel to allow tables to be - * owned by stystemd-networkd and making them non-deleteable except by - * the 'owning process'. - */ -static int fw_nftables_recreate_table(sd_netlink *nfnl, int af, sd_netlink_message **old, size_t size) { - int r = fw_nftables_init_family(nfnl, af); - - if (r != 0) - return r; - - while (size > 0) { - size_t i = --size; - - old[i] = sd_netlink_message_unref(old[i]); - } - - return 0; -} - static int nft_message_add_setelem_ip6range( sd_netlink_message *m, const union in_addr_union *source, @@ -877,14 +843,14 @@ static int nft_message_add_setelem_ip6range( #define NFT_MASQ_MSGS 3 -int fw_nftables_add_masquerade( +static int fw_nftables_add_masquerade_internal( FirewallContext *ctx, bool add, int af, const union in_addr_union *source, unsigned int source_prefixlen) { + sd_netlink_message *transaction[NFT_MASQ_MSGS] = {}; - bool retry = true; size_t tsize; int r; @@ -893,7 +859,7 @@ int fw_nftables_add_masquerade( if (af == AF_INET6 && source_prefixlen < 8) return -EINVAL; -again: + r = sd_nfnl_message_batch_begin(ctx->nfnl, &transaction[0]); if (r < 0) return r; @@ -902,7 +868,6 @@ again: r = sd_nfnl_nft_message_new_setelems_begin(ctx->nfnl, &transaction[tsize], af, NFT_SYSTEMD_TABLE_NAME, NFT_SYSTEMD_MASQ_SET_NAME); else r = sd_nfnl_nft_message_del_setelems_begin(ctx->nfnl, &transaction[tsize], af, NFT_SYSTEMD_TABLE_NAME, NFT_SYSTEMD_MASQ_SET_NAME); - if (r < 0) goto out_unref; @@ -910,7 +875,6 @@ again: r = nft_message_add_setelem_iprange(transaction[tsize], source, source_prefixlen); else r = nft_message_add_setelem_ip6range(transaction[tsize], source, source_prefixlen); - if (r < 0) goto out_unref; @@ -919,26 +883,56 @@ again: r = sd_nfnl_message_batch_end(ctx->nfnl, &transaction[tsize]); if (r < 0) return r; + ++tsize; r = nfnl_netlink_sendv(ctx->nfnl, transaction, tsize); - if (retry && r == -ENOENT) { - int tmp = fw_nftables_recreate_table(ctx->nfnl, af, transaction, tsize); - if (tmp == 0) { - retry = false; - goto again; - } - } - out_unref: while (tsize > 0) sd_netlink_message_unref(transaction[--tsize]); return r < 0 ? r : 0; } +int fw_nftables_add_masquerade( + FirewallContext *ctx, + bool add, + int af, + const union in_addr_union *source, + unsigned int source_prefixlen) { + + int r; + + r = fw_nftables_add_masquerade_internal(ctx, add, af, source, source_prefixlen); + if (r != -ENOENT) + return r; + + /* When someone runs 'nft flush ruleset' in the same net namespace this will also tear down the + * systemd nat table. + * + * Unlike iptables -t nat -F (which will remove all rules added by the systemd iptables + * backend, iptables has builtin chains that cannot be deleted -- the next add operation will + * 'just work'. + * + * In the nftables case, everything gets removed. The next add operation will yield -ENOENT. + * + * If we see -ENOENT on add, replay the initial table setup. If that works, re-do the add + * operation. + * + * Note that this doesn't protect against external sabotage such as a + * 'while true; nft flush ruleset; done'. There is nothing that could be done about that short + * of extending the kernel to allow tables to be owned by stystemd-networkd and making them + * non-deleteable except by the 'owning process'. */ + + r = fw_nftables_init_family(ctx->nfnl, af); + if (r < 0) + return r; + + return fw_nftables_add_masquerade_internal(ctx, add, af, source, source_prefixlen); +} + #define NFT_DNAT_MSGS 4 -int fw_nftables_add_local_dnat( +static int fw_nftables_add_local_dnat_internal( FirewallContext *ctx, bool add, int af, @@ -947,9 +941,9 @@ int fw_nftables_add_local_dnat( const union in_addr_union *remote, uint16_t remote_port, const union in_addr_union *previous_remote) { - uint32_t data[5], key[2], dlen; + sd_netlink_message *transaction[NFT_DNAT_MSGS] = {}; - bool retry = true; + uint32_t data[5], key[2], dlen; size_t tsize; int r; @@ -961,7 +955,6 @@ int fw_nftables_add_local_dnat( if (local_port <= 0) return -EINVAL; -again: key[0] = protocol; key[1] = htobe16(local_port); @@ -1023,19 +1016,34 @@ again: assert(tsize <= NFT_DNAT_MSGS); r = nfnl_netlink_sendv(ctx->nfnl, transaction, tsize); - if (retry && r == -ENOENT) { - int tmp = fw_nftables_recreate_table(ctx->nfnl, af, transaction, tsize); - - if (tmp == 0) { - /* table created anew; previous address already gone */ - previous_remote = NULL; - retry = false; - goto again; - } - } - out_unref: while (tsize > 0) sd_netlink_message_unref(transaction[--tsize]); + return r < 0 ? r : 0; } + +int fw_nftables_add_local_dnat( + FirewallContext *ctx, + bool add, + int af, + int protocol, + uint16_t local_port, + const union in_addr_union *remote, + uint16_t remote_port, + const union in_addr_union *previous_remote) { + + int r; + + r = fw_nftables_add_local_dnat_internal(ctx, add, af, protocol, local_port, remote, remote_port, previous_remote); + if (r != -ENOENT) + return r; + + /* See comment in fw_nftables_add_masquerade(). */ + r = fw_nftables_init_family(ctx->nfnl, af); + if (r < 0) + return r; + + /* table created anew; previous address already gone */ + return fw_nftables_add_local_dnat_internal(ctx, add, af, protocol, local_port, remote, remote_port, NULL); +}