ipvlan: list corruption and rcu fixes

When ipvlan interface with IP addresses attached is brought down and then deleted, the assigned addresses are deleted twice from the address hash list, first on the interface down and second on the link deletion. Similarly, when an address is added while the interface is down, it is added second time once the interface is brought up.

When the interface is down, the addresses should be kept off the hash list for performance reasons. Ensure this is true, which also fixes the double add problem. To fix the double free, check whether the address is hashed before removing it.

Reported-by Dan Williams <[email protected]>

Signed-off-by Jiri Benc <[email protected]>

---

v2 -> v3: check hlist_unhashed also on addition; fixed comment coding style

v1 -> v2: check hlist_unhashed on deletion instead of depending on netif_running

---
 drivers/net/ipvlan/ipvlan_core.c |  5 +++--
 drivers/net/ipvlan/ipvlan_main.c | 12 ++++++++++--
 2 files changed, 13 insertions(+), 4 deletions(-)

drivers/net/ipvlan/ipvlan_core.c +3 -2

index 2a175006028b..8a542b9340c4 100644
@@ -81,12 +81,13 @@ void ipvlan_ht_addr_add(struct ipvl_dev *ipvlan, struct ipvl_addr *addr)
        hash = (addr->atype == IPVL_IPV6) ?
               ipvlan_get_v6_hash(&addr->ip6addr) :
               ipvlan_get_v4_hash(&addr->ip4addr);
-       hlist_add_head_rcu(&addr->hlnode, &port->hlhead[hash]);
+       if (hlist_unhashed(&addr->hlnode))
+               hlist_add_head_rcu(&addr->hlnode, &port->hlhead[hash]);
 }
 
 void ipvlan_ht_addr_del(struct ipvl_addr *addr, bool sync)
 {
-       hlist_del_rcu(&addr->hlnode);
+       hlist_del_init_rcu(&addr->hlnode);
        if (sync)
                synchronize_rcu();
 }

drivers/net/ipvlan/ipvlan_main.c +10 -3

index 4f4099d5603d..1eb3f33e11cc 100644
@@ -622,7 +622,11 @@ static int ipvlan_add_addr6(struct ipvl_dev *ipvlan, struct in6_addr *ip6_addr)
        addr->atype = IPVL_IPV6;
        list_add_tail_rcu(&addr->anode, &ipvlan->addrs);
        ipvlan->ipv6cnt++;
-       ipvlan_ht_addr_add(ipvlan, addr);
+       /* If the interface is not up, the address will be added to the hash
+        * list by ipvlan_open.
+        */
+       if (netif_running(ipvlan->dev))
+               ipvlan_ht_addr_add(ipvlan, addr);
 
        return 0;
 }

@@ -690,7 +694,11 @@ static int ipvlan_add_addr4(struct ipvl_dev *ipvlan, struct in_addr *ip4_addr)
        addr->atype = IPVL_IPV4;
        list_add_tail_rcu(&addr->anode, &ipvlan->addrs);
        ipvlan->ipv4cnt++;
-       ipvlan_ht_addr_add(ipvlan, addr);
+       /* If the interface is not up, the address will be added to the hash
+        * list by ipvlan_open.
+        */
+       if (netif_running(ipvlan->dev))
+               ipvlan_ht_addr_add(ipvlan, addr);
        ipvlan_set_broadcast_mac_filter(ipvlan, true);
 
        return 0;
--

1.8.3.1

When an ipvlan interface is down, its addresses are not on the hash list. Fix checks for existence of addresses not to depend on the hash list, walk through all interface addresses instead.

Signed-off-by Jiri Benc <[email protected]>

---

Note that while this patch is needed and fixes problems like ipv4cnt underflow and trigerring WARN_ON in ipvlan_del_addr4, it does not fix the more substantial problem: although the current code suggests that it prevents assignment of the same IP address to multiple ipvlan interfaces, it does not really do that. The address will be assigned to both interfaces, ipvlan just silently considers such address to belong to the first interface only.

Seems the original intention was to prevent address assignment by returning NOTIFY_BAD but inet_insert_ifa does not really care about notifier results. Till such feature is implemented, this patch at least makes sure we don't have corrupted counters and don't leave kernel traces in the log.

---
 drivers/net/ipvlan/ipvlan.h      |  4 +++-
 drivers/net/ipvlan/ipvlan_core.c | 19 ++++++++++++++-----
 drivers/net/ipvlan/ipvlan_main.c |  8 ++++----
 3 files changed, 21 insertions(+), 10 deletions(-)

drivers/net/ipvlan/ipvlan.h +3 -1

index 924ea98bd531..54549a6223dd 100644
@@ -114,7 +114,9 @@ unsigned int ipvlan_mac_hash(const unsigned char *addr);
 rx_handler_result_t ipvlan_handle_frame(struct sk_buff **pskb);
 int ipvlan_queue_xmit(struct sk_buff *skb, struct net_device *dev);
 void ipvlan_ht_addr_add(struct ipvl_dev *ipvlan, struct ipvl_addr *addr);
-bool ipvlan_addr_busy(struct ipvl_dev *ipvlan, void *iaddr, bool is_v6);
+struct ipvl_addr *ipvlan_find_addr(const struct ipvl_dev *ipvlan,
+                                  const void *iaddr, bool is_v6);
+bool ipvlan_addr_busy(struct ipvl_port *port, void *iaddr, bool is_v6);
 struct ipvl_addr *ipvlan_ht_addr_lookup(const struct ipvl_port *port,
                                        const void *iaddr, bool is_v6);
 void ipvlan_ht_addr_del(struct ipvl_addr *addr, bool sync);

drivers/net/ipvlan/ipvlan_core.c +14 -5

index 568628f95aa2..b7877a194cfe 100644
@@ -92,9 +92,9 @@ void ipvlan_ht_addr_del(struct ipvl_addr *addr, bool sync)
                synchronize_rcu();
 }
 
-bool ipvlan_addr_busy(struct ipvl_dev *ipvlan, void *iaddr, bool is_v6)
+struct ipvl_addr *ipvlan_find_addr(const struct ipvl_dev *ipvlan,
+                                  const void *iaddr, bool is_v6)
 {
-       struct ipvl_port *port = ipvlan->port;
        struct ipvl_addr *addr;
 
        list_for_each_entry(addr, &ipvlan->addrs, anode) {

@@ -102,12 +102,21 @@ bool ipvlan_addr_busy(struct ipvl_dev *ipvlan, void *iaddr, bool is_v6)
                    ipv6_addr_equal(&addr->ip6addr, iaddr)) ||
                    (!is_v6 && addr->atype == IPVL_IPV4 &&
                    addr->ip4addr.s_addr == ((struct in_addr *)iaddr)->s_addr))
-                       return true;
+                       return addr;
        }
+       return NULL;
+}
 
-       if (ipvlan_ht_addr_lookup(port, iaddr, is_v6))
-               return true;
+bool ipvlan_addr_busy(struct ipvl_port *port, void *iaddr, bool is_v6)
+{
+       struct ipvl_dev *ipvlan;
+
+       ASSERT_RTNL();
 
+       list_for_each_entry(ipvlan, &port->ipvlans, pnode) {
+               if (ipvlan_find_addr(ipvlan, iaddr, is_v6))
+                       return true;
+       }
        return false;
 }
 

drivers/net/ipvlan/ipvlan_main.c +4 -5

index aaa005bd21ce..4fa14208d799 100644
@@ -607,7 +607,7 @@ static int ipvlan_add_addr6(struct ipvl_dev *ipvlan, struct in6_addr *ip6_addr)
 {
        struct ipvl_addr *addr;
 
-       if (ipvlan_addr_busy(ipvlan, ip6_addr, true)) {
+       if (ipvlan_addr_busy(ipvlan->port, ip6_addr, true)) {
                netif_err(ipvlan, ifup, ipvlan->dev,
                          "Failed to add IPv6=%pI6c addr for %s intf\n",
                          ip6_addr, ipvlan->dev->name);

@@ -635,7 +635,7 @@ static void ipvlan_del_addr6(struct ipvl_dev *ipvlan, struct in6_addr *ip6_addr)
 {
        struct ipvl_addr *addr;
 
-       addr = ipvlan_ht_addr_lookup(ipvlan->port, ip6_addr, true);
+       addr = ipvlan_find_addr(ipvlan, ip6_addr, true);
        if (!addr)
                return;
 

@@ -679,7 +679,7 @@ static int ipvlan_add_addr4(struct ipvl_dev *ipvlan, struct in_addr *ip4_addr)
 {
        struct ipvl_addr *addr;
 
-       if (ipvlan_addr_busy(ipvlan, ip4_addr, false)) {
+       if (ipvlan_addr_busy(ipvlan->port, ip4_addr, false)) {
                netif_err(ipvlan, ifup, ipvlan->dev,
                          "Failed to add IPv4=%pI4 on %s intf.\n",
                          ip4_addr, ipvlan->dev->name);

@@ -708,7 +708,7 @@ static void ipvlan_del_addr4(struct ipvl_dev *ipvlan, struct in_addr *ip4_addr)
 {
        struct ipvl_addr *addr;
 
-       addr = ipvlan_ht_addr_lookup(ipvlan->port, ip4_addr, false);
+       addr = ipvlan_find_addr(ipvlan, ip4_addr, false);
        if (!addr)
                return;
 
--

1.8.3.1

This patch set fixes different issues leading to corrupted lists and incorrect rcu usage.

Only patch 1/4 changed since the last submission.