From c5ff3ea39882609b307c4a9925d1c17413d17dfc Mon Sep 17 00:00:00 2001
From: Yu Watanabe <watanabe.yu+github@gmail.com>
Date: Tue, 17 Aug 2021 14:03:19 +0900
Subject: [PATCH] network: do not assume the highest priority when Priority= is
 unspecified

Previously, when Priority= is unspecified, networkd configured the rule with
the highest (=0) priority. This commit makes networkd distinguish the case
the setting is unspecified and one explicitly specified as Priority=0.

Note.
1) If the priority is unspecified on configure, then kernel dynamically picks
   a priority for the rule.
2) The new behavior is consistent with 'ip rule' command.

Replaces #15606.

(cherry picked from commit c4f7a347566b8926382029593b4d9957fef2564c)

Conflict:NA
Reference:https://github.com/systemd/systemd/commit/c5ff3ea39882609b307c4a9925d1c17413d17dfc
---
 man/systemd.network.xml                     |   4 +-
 src/network/networkd-routing-policy-rule.c  | 120 +++++++++++++++++---
 src/network/networkd-routing-policy-rule.h  |   1 +
 test/test-network/systemd-networkd-tests.py |   2 +-
 4 files changed, 110 insertions(+), 17 deletions(-)

diff --git a/man/systemd.network.xml b/man/systemd.network.xml
index 3b7680eb8b..9de9816ced 100644
--- a/man/systemd.network.xml
+++ b/man/systemd.network.xml
@@ -1238,7 +1238,9 @@ IPv6Token=prefixstable:2002:da8:1::</programlisting></para>
           <term><varname>Priority=</varname></term>
           <listitem>
             <para>Specifies the priority of this rule. <varname>Priority=</varname> is an unsigned
-            integer. Higher number means lower priority, and rules get processed in order of increasing number.</para>
+            integer in the range 0…4294967295. Higher number means lower priority, and rules get
+            processed in order of increasing number. Defaults to unset, and the kernel will pick
+            a value dynamically.</para>
           </listitem>
         </varlistentry>
         <varlistentry>
diff --git a/src/network/networkd-routing-policy-rule.c b/src/network/networkd-routing-policy-rule.c
index af7e8a973c..03ccbd8e85 100644
--- a/src/network/networkd-routing-policy-rule.c
+++ b/src/network/networkd-routing-policy-rule.c
@@ -163,7 +163,9 @@ void routing_policy_rule_hash_func(const RoutingPolicyRule *rule, struct siphash
                 siphash24_compress(&rule->type, sizeof(rule->type), state);
                 siphash24_compress(&rule->fwmark, sizeof(rule->fwmark), state);
                 siphash24_compress(&rule->fwmask, sizeof(rule->fwmask), state);
-                siphash24_compress(&rule->priority, sizeof(rule->priority), state);
+                siphash24_compress_boolean(rule->priority_set, state);
+                if (rule->priority_set)
+                        siphash24_compress(&rule->priority, sizeof(rule->priority), state);
                 siphash24_compress(&rule->table, sizeof(rule->table), state);
                 siphash24_compress(&rule->suppress_prefixlen, sizeof(rule->suppress_prefixlen), state);
 
@@ -229,10 +231,16 @@ int routing_policy_rule_compare_func(const RoutingPolicyRule *a, const RoutingPo
                 if (r != 0)
                         return r;
 
-                r = CMP(a->priority, b->priority);
+                r = CMP(a->priority_set, b->priority_set);
                 if (r != 0)
                         return r;
 
+                if (a->priority_set) {
+                        r = CMP(a->priority, b->priority);
+                        if (r != 0)
+                                return r;
+                }
+
                 r = CMP(a->table, b->table);
                 if (r != 0)
                         return r;
@@ -293,8 +301,9 @@ DEFINE_PRIVATE_HASH_OPS_WITH_KEY_DESTRUCTOR(
                 routing_policy_rule_compare_func,
                 routing_policy_rule_free);
 
-static int routing_policy_rule_get(Manager *m, const RoutingPolicyRule *rule, RoutingPolicyRule **ret) {
+static int routing_policy_rule_get(Manager *m, const RoutingPolicyRule *rule, bool require_priority, RoutingPolicyRule **ret) {
         RoutingPolicyRule *existing;
+        int r;
 
         assert(m);
 
@@ -312,6 +321,23 @@ static int routing_policy_rule_get(Manager *m, const RoutingPolicyRule *rule, Ro
                 return 0;
         }
 
+        if (!require_priority && rule->priority_set) {
+                _cleanup_(routing_policy_rule_freep) RoutingPolicyRule *tmp = NULL;
+
+                r = routing_policy_rule_dup(rule, &tmp);
+                if (r < 0)
+                        return r;
+
+                tmp->priority_set = false;
+
+                existing = set_get(m->rules, tmp);
+                if (existing) {
+                        if (ret)
+                                *ret = existing;
+                        return 1;
+                }
+        }
+
         return -ENOENT;
 }
 
@@ -328,7 +354,7 @@ static int routing_policy_rule_add(Manager *m, const RoutingPolicyRule *in, Rout
         if (r < 0)
                 return r;
 
-        r = routing_policy_rule_get(m, rule, &existing);
+        r = routing_policy_rule_get(m, rule, true, &existing);
         if (r == -ENOENT) {
                 /* Rule does not exist, use a new one. */
                 r = set_ensure_put(&m->rules, &routing_policy_rule_hash_ops, rule);
@@ -371,6 +397,32 @@ static int routing_policy_rule_consume_foreign(Manager *m, RoutingPolicyRule *ru
         return 1;
 }
 
+static int routing_policy_rule_update_priority(RoutingPolicyRule *rule, uint32_t priority) {
+        int r;
+
+        assert(rule);
+        assert(rule->manager);
+
+        if (rule->priority_set)
+                return 0;
+
+        if (!set_remove(rule->manager->rules, rule))
+                return -ENOENT;
+
+        rule->priority = priority;
+        rule->priority_set = true;
+
+        r = set_put(rule->manager->rules, rule);
+        if (r <= 0) {
+                /* Undo */
+                rule->priority_set = false;
+                assert_se(set_put(rule->manager->rules, rule) > 0);
+                return r == 0 ? -EEXIST : r;
+        }
+
+        return 1;
+}
+
 static void log_routing_policy_rule_debug(const RoutingPolicyRule *rule, const char *str, const Link *link, const Manager *m) {
         _cleanup_free_ char *from = NULL, *to = NULL, *table = NULL;
 
@@ -422,9 +474,11 @@ static int routing_policy_rule_set_netlink_message(const RoutingPolicyRule *rule
                         return log_link_error_errno(link, r, "Could not set destination prefix length: %m");
         }
 
-        r = sd_netlink_message_append_u32(m, FRA_PRIORITY, rule->priority);
-        if (r < 0)
-                return log_link_error_errno(link, r, "Could not append FRA_PRIORITY attribute: %m");
+        if (rule->priority_set) {
+                r = sd_netlink_message_append_u32(m, FRA_PRIORITY, rule->priority);
+                if (r < 0)
+                        return log_link_error_errno(link, r, "Could not append FRA_PRIORITY attribute: %m");
+        }
 
         if (rule->tos > 0) {
                 r = sd_rtnl_message_routing_policy_rule_set_tos(m, rule->tos);
@@ -662,6 +716,28 @@ int manager_drop_routing_policy_rules_internal(Manager *m, bool foreign, const L
                         continue;
                 }
 
+                if (!foreign) {
+                        _cleanup_(routing_policy_rule_freep) RoutingPolicyRule *tmp = NULL;
+
+                        /* The rule may be configured without priority. Try to find without priority. */
+
+                        k = routing_policy_rule_dup(rule, &tmp);
+                        if (k < 0) {
+                                if (r >= 0)
+                                        r = k;
+                                continue;
+                        }
+
+                        tmp->priority_set = false;
+
+                        k = links_have_routing_policy_rule(m, tmp, except);
+                        if (k != 0) {
+                                if (k < 0 && r >= 0)
+                                        r = k;
+                                continue;
+                        }
+                }
+
                 k = routing_policy_rule_remove(rule, m);
                 if (k < 0 && r >= 0)
                         r = k;
@@ -821,11 +897,11 @@ int request_process_routing_policy_rule(Request *req) {
 }
 
 static const RoutingPolicyRule kernel_rules[] = {
-        { .family = AF_INET,  .priority = 0,     .table = RT_TABLE_LOCAL,   .type = FR_ACT_TO_TBL, .uid_range.start = UID_INVALID, .uid_range.end = UID_INVALID, .suppress_prefixlen = -1, },
-        { .family = AF_INET,  .priority = 32766, .table = RT_TABLE_MAIN,    .type = FR_ACT_TO_TBL, .uid_range.start = UID_INVALID, .uid_range.end = UID_INVALID, .suppress_prefixlen = -1, },
-        { .family = AF_INET,  .priority = 32767, .table = RT_TABLE_DEFAULT, .type = FR_ACT_TO_TBL, .uid_range.start = UID_INVALID, .uid_range.end = UID_INVALID, .suppress_prefixlen = -1, },
-        { .family = AF_INET6, .priority = 0,     .table = RT_TABLE_LOCAL,   .type = FR_ACT_TO_TBL, .uid_range.start = UID_INVALID, .uid_range.end = UID_INVALID, .suppress_prefixlen = -1, },
-        { .family = AF_INET6, .priority = 32766, .table = RT_TABLE_MAIN,    .type = FR_ACT_TO_TBL, .uid_range.start = UID_INVALID, .uid_range.end = UID_INVALID, .suppress_prefixlen = -1, },
+        { .family = AF_INET,  .priority_set = true, .priority = 0,     .table = RT_TABLE_LOCAL,   .type = FR_ACT_TO_TBL, .uid_range.start = UID_INVALID, .uid_range.end = UID_INVALID, .suppress_prefixlen = -1, },
+        { .family = AF_INET,  .priority_set = true, .priority = 32766, .table = RT_TABLE_MAIN,    .type = FR_ACT_TO_TBL, .uid_range.start = UID_INVALID, .uid_range.end = UID_INVALID, .suppress_prefixlen = -1, },
+        { .family = AF_INET,  .priority_set = true, .priority = 32767, .table = RT_TABLE_DEFAULT, .type = FR_ACT_TO_TBL, .uid_range.start = UID_INVALID, .uid_range.end = UID_INVALID, .suppress_prefixlen = -1, },
+        { .family = AF_INET6, .priority_set = true, .priority = 0,     .table = RT_TABLE_LOCAL,   .type = FR_ACT_TO_TBL, .uid_range.start = UID_INVALID, .uid_range.end = UID_INVALID, .suppress_prefixlen = -1, },
+        { .family = AF_INET6, .priority_set = true, .priority = 32766, .table = RT_TABLE_MAIN,    .type = FR_ACT_TO_TBL, .uid_range.start = UID_INVALID, .uid_range.end = UID_INVALID, .suppress_prefixlen = -1, },
 };
 
 static bool routing_policy_rule_is_created_by_kernel(const RoutingPolicyRule *rule) {
@@ -936,6 +1012,9 @@ int manager_rtnl_process_rule(sd_netlink *rtnl, sd_netlink_message *message, Man
                 log_warning_errno(r, "rtnl: could not get FRA_PRIORITY attribute, ignoring: %m");
                 return 0;
         }
+        /* The kernel does not send priority if priority is zero. So, the flag below must be always set
+         * even if the message does not contain FRA_PRIORITY. */
+        tmp->priority_set = true;
 
         r = sd_netlink_message_read_u32(message, FRA_TABLE, &tmp->table);
         if (r < 0 && r != -ENODATA) {
@@ -1027,13 +1106,16 @@ int manager_rtnl_process_rule(sd_netlink *rtnl, sd_netlink_message *message, Man
                  * protocol of the received rule is RTPROT_KERNEL or RTPROT_STATIC. */
                 tmp->protocol = routing_policy_rule_is_created_by_kernel(tmp) ? RTPROT_KERNEL : RTPROT_STATIC;
 
-        (void) routing_policy_rule_get(m, tmp, &rule);
+        (void) routing_policy_rule_get(m, tmp, false, &rule);
 
         switch (type) {
         case RTM_NEWRULE:
-                if (rule)
+                if (rule) {
                         log_routing_policy_rule_debug(tmp, "Received remembered", NULL, m);
-                else if (!m->manage_foreign_routes)
+                        r = routing_policy_rule_update_priority(rule, tmp->priority);
+                        if (r < 0)
+                                log_warning_errno(r, "Failed to update priority of remembered routing policy rule, ignoring: %m");
+                } else if (!m->manage_foreign_routes)
                         log_routing_policy_rule_debug(tmp, "Ignoring received foreign", NULL, m);
                 else {
                         log_routing_policy_rule_debug(tmp, "Remembering foreign", NULL, m);
@@ -1155,11 +1237,19 @@ int config_parse_routing_policy_rule_priority(
         if (r < 0)
                 return log_oom();
 
+        if (isempty(rvalue)) {
+                n->priority = 0;
+                n->priority_set = false;
+                TAKE_PTR(n);
+                return 0;
+        }
+
         r = safe_atou32(rvalue, &n->priority);
         if (r < 0) {
                 log_syntax(unit, LOG_WARNING, filename, line, r, "Failed to parse RPDB rule priority, ignoring: %s", rvalue);
                 return 0;
         }
+        n->priority_set = true;
 
         TAKE_PTR(n);
         return 0;
diff --git a/src/network/networkd-routing-policy-rule.h b/src/network/networkd-routing-policy-rule.h
index aed37b00d2..557048c3f4 100644
--- a/src/network/networkd-routing-policy-rule.h
+++ b/src/network/networkd-routing-policy-rule.h
@@ -20,6 +20,7 @@ typedef struct RoutingPolicyRule {
         NetworkConfigSection *section;
 
         bool invert_rule;
+        bool priority_set;
 
         uint8_t tos;
         uint8_t type;
diff --git a/test/test-network/systemd-networkd-tests.py b/test/test-network/systemd-networkd-tests.py
index 0eb2fdf87e..4a2af0c500 100755
--- a/test/test-network/systemd-networkd-tests.py
+++ b/test/test-network/systemd-networkd-tests.py
@@ -3644,7 +3644,7 @@ class NetworkdBridgeTests(unittest.TestCase, Utilities):
 
         output = check_output('ip rule list table 100')
         print(output)
-        self.assertIn('0:	from all to 8.8.8.8 lookup 100', output)
+        self.assertIn('from all to 8.8.8.8 lookup 100', output)
 
 class NetworkdLLDPTests(unittest.TestCase, Utilities):
     links = ['veth99']
-- 
2.33.0