[PATCH net 0/4] ipvlan: list corruption and rcu fixes

[PATCH net 0/4] ipvlan: list corruption and rcu fixes

[Jiri Benc]

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

Jiri Benc (4):
  ipvlan: fix addr hash list corruption
  ipvlan: protect against concurrent link removal
  ipvlan: do not use rcu operations for address list
  ipvlan: fix check for IP addresses in control path

 drivers/net/ipvlan/ipvlan.h      |  4 +++-
 drivers/net/ipvlan/ipvlan_core.c | 25 ++++++++++++++++++-------
 drivers/net/ipvlan/ipvlan_main.c | 28 +++++++++++++++++-----------
 3 files changed, 38 insertions(+), 19 deletions(-)

-- 
1.8.3.1

[PATCH net 1/4 v2] ipvlan: fix addr hash list corruption

[Jiri Benc]

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 <dcbw@redhat.com>
Signed-off-by: Jiri Benc <jbenc@redhat.com>
---

Changes since v1: check hlist_unhashed on deletion instead of depending on
netif_running

---
 drivers/net/ipvlan/ipvlan_core.c |  2 +-
 drivers/net/ipvlan/ipvlan_main.c | 10 ++++++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ipvlan/ipvlan_core.c b/drivers/net/ipvlan/ipvlan_core.c
index 2a175006028b..9f705fc9e0ef 100644
--- a/drivers/net/ipvlan/ipvlan_core.c
+++ b/drivers/net/ipvlan/ipvlan_core.c
@@ -86,7 +86,7 @@ void ipvlan_ht_addr_add(struct ipvl_dev *ipvlan, struct ipvl_addr *addr)
 
 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();
 }
diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
index 4f4099d5603d..dbaf67c722a0 100644
--- a/drivers/net/ipvlan/ipvlan_main.c
+++ b/drivers/net/ipvlan/ipvlan_main.c
@@ -622,7 +622,10 @@ 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 +693,10 @@ 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

Re: [PATCH net 1/4 v2] ipvlan: fix addr hash list corruption

[Sergei Shtylyov]

Hello.

On 03/27/2015 08:30 PM, Jiri Benc wrote:

> 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 <dcbw@redhat.com>
> Signed-off-by: Jiri Benc <jbenc@redhat.com>
> ---

> Changes since v1: check hlist_unhashed on deletion instead of depending on
> netif_running

[...]

> diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
> index 4f4099d5603d..dbaf67c722a0 100644
> --- a/drivers/net/ipvlan/ipvlan_main.c
> +++ b/drivers/net/ipvlan/ipvlan_main.c
> @@ -622,7 +622,10 @@ 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. */

    The preferred multi-line comment style in the networking code is:

/* Like
  * this.
  */

[...]
> @@ -690,7 +693,10 @@ 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. */

    Likewise.

[...]

WBR, Sergei

Re: [PATCH net 1/4 v2] ipvlan: fix addr hash list corruption

[Mahesh Bandewar]

On Fri, Mar 27, 2015 at 10:30 AM, Jiri Benc <jbenc@redhat.com> wrote:
> 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 <dcbw@redhat.com>
> Signed-off-by: Jiri Benc <jbenc@redhat.com>
> ---
>
> Changes since v1: check hlist_unhashed on deletion instead of depending on
> netif_running
>
> ---
>  drivers/net/ipvlan/ipvlan_core.c |  2 +-
>  drivers/net/ipvlan/ipvlan_main.c | 10 ++++++++--
>  2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ipvlan/ipvlan_core.c b/drivers/net/ipvlan/ipvlan_core.c
> index 2a175006028b..9f705fc9e0ef 100644
> --- a/drivers/net/ipvlan/ipvlan_core.c
> +++ b/drivers/net/ipvlan/ipvlan_core.c
> @@ -86,7 +86,7 @@ void ipvlan_ht_addr_add(struct ipvl_dev *ipvlan, struct ipvl_addr *addr)
>
>  void ipvlan_ht_addr_del(struct ipvl_addr *addr, bool sync)
>  {
> -       hlist_del_rcu(&addr->hlnode);
> +       hlist_del_init_rcu(&addr->hlnode);
Though this fixes the current crash problem; the duplicate addition in
the hash-table is still open. I would still prefer it to be protected
by hlist_unhashed() check.
>         if (sync)
>                 synchronize_rcu();
>  }
> diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
> index 4f4099d5603d..dbaf67c722a0 100644
> --- a/drivers/net/ipvlan/ipvlan_main.c
> +++ b/drivers/net/ipvlan/ipvlan_main.c
> @@ -622,7 +622,10 @@ 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 +693,10 @@ 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
>

Re: [PATCH net 1/4 v2] ipvlan: fix addr hash list corruption

[Jiri Benc]

On Fri, 27 Mar 2015 17:16:49 -0700, Mahesh Bandewar wrote:
> On Fri, Mar 27, 2015 at 10:30 AM, Jiri Benc <jbenc@redhat.com> wrote:
> >  void ipvlan_ht_addr_del(struct ipvl_addr *addr, bool sync)
> >  {
> > -       hlist_del_rcu(&addr->hlnode);
> > +       hlist_del_init_rcu(&addr->hlnode);
> Though this fixes the current crash problem; the duplicate addition in
> the hash-table is still open. I would still prefer it to be protected
> by hlist_unhashed() check.

It cannot happen but whatever, there's no harm in such check. Just sent
a new version.

 Jiri

-- 
Jiri Benc

Re: [PATCH net 1/4 v2] ipvlan: fix addr hash list corruption

[David Miller]

From: Jiri Benc <jbenc@redhat.com>
Date: Fri, 27 Mar 2015 18:30:23 +0100

> @@ -622,7 +622,10 @@ 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. */

Comments in the networking should be formatted:

	/* Like
	 * this.
	 */

Thanks.

[PATCH net 2/4] ipvlan: protect against concurrent link removal

[Jiri Benc]

Adding and removing to the 'ipvlans' list is already done using _rcu list
operations.

Signed-off-by: Jiri Benc <jbenc@redhat.com>
---
 drivers/net/ipvlan/ipvlan_core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ipvlan/ipvlan_core.c b/drivers/net/ipvlan/ipvlan_core.c
index 9f705fc9e0ef..77b7f56d1551 100644
--- a/drivers/net/ipvlan/ipvlan_core.c
+++ b/drivers/net/ipvlan/ipvlan_core.c
@@ -192,7 +192,8 @@ static void ipvlan_multicast_frame(struct ipvl_port *port, struct sk_buff *skb,
 	if (skb->protocol == htons(ETH_P_PAUSE))
 		return;
 
-	list_for_each_entry(ipvlan, &port->ipvlans, pnode) {
+	rcu_read_lock();
+	list_for_each_entry_rcu(ipvlan, &port->ipvlans, pnode) {
 		if (local && (ipvlan == in_dev))
 			continue;
 
@@ -219,6 +220,7 @@ static void ipvlan_multicast_frame(struct ipvl_port *port, struct sk_buff *skb,
 mcast_acct:
 		ipvlan_count_rx(ipvlan, len, ret == NET_RX_SUCCESS, true);
 	}
+	rcu_read_unlock();
 
 	/* Locally generated? ...Forward a copy to the main-device as
 	 * well. On the RX side we'll ignore it (wont give it to any
-- 
1.8.3.1

[PATCH net 3/4] ipvlan: do not use rcu operations for address list

[Jiri Benc]

All accesses to ipvlan->addrs are under rtnl.

Signed-off-by: Jiri Benc <jbenc@redhat.com>
---
 drivers/net/ipvlan/ipvlan_main.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
index dbaf67c722a0..f5b1c7b38770 100644
--- a/drivers/net/ipvlan/ipvlan_main.c
+++ b/drivers/net/ipvlan/ipvlan_main.c
@@ -505,7 +505,7 @@ static void ipvlan_link_delete(struct net_device *dev, struct list_head *head)
 	if (ipvlan->ipv6cnt > 0 || ipvlan->ipv4cnt > 0) {
 		list_for_each_entry_safe(addr, next, &ipvlan->addrs, anode) {
 			ipvlan_ht_addr_del(addr, !dev->dismantle);
-			list_del_rcu(&addr->anode);
+			list_del(&addr->anode);
 		}
 	}
 	list_del_rcu(&ipvlan->pnode);
@@ -620,7 +620,7 @@ static int ipvlan_add_addr6(struct ipvl_dev *ipvlan, struct in6_addr *ip6_addr)
 	addr->master = ipvlan;
 	memcpy(&addr->ip6addr, ip6_addr, sizeof(struct in6_addr));
 	addr->atype = IPVL_IPV6;
-	list_add_tail_rcu(&addr->anode, &ipvlan->addrs);
+	list_add_tail(&addr->anode, &ipvlan->addrs);
 	ipvlan->ipv6cnt++;
 	/* If the interface is not up, the address will be added to the hash
 	 * list by ipvlan_open. */
@@ -639,7 +639,7 @@ static void ipvlan_del_addr6(struct ipvl_dev *ipvlan, struct in6_addr *ip6_addr)
 		return;
 
 	ipvlan_ht_addr_del(addr, true);
-	list_del_rcu(&addr->anode);
+	list_del(&addr->anode);
 	ipvlan->ipv6cnt--;
 	WARN_ON(ipvlan->ipv6cnt < 0);
 	kfree_rcu(addr, rcu);
@@ -691,7 +691,7 @@ static int ipvlan_add_addr4(struct ipvl_dev *ipvlan, struct in_addr *ip4_addr)
 	addr->master = ipvlan;
 	memcpy(&addr->ip4addr, ip4_addr, sizeof(struct in_addr));
 	addr->atype = IPVL_IPV4;
-	list_add_tail_rcu(&addr->anode, &ipvlan->addrs);
+	list_add_tail(&addr->anode, &ipvlan->addrs);
 	ipvlan->ipv4cnt++;
 	/* If the interface is not up, the address will be added to the hash
 	 * list by ipvlan_open. */
@@ -711,7 +711,7 @@ static void ipvlan_del_addr4(struct ipvl_dev *ipvlan, struct in_addr *ip4_addr)
 		return;
 
 	ipvlan_ht_addr_del(addr, true);
-	list_del_rcu(&addr->anode);
+	list_del(&addr->anode);
 	ipvlan->ipv4cnt--;
 	WARN_ON(ipvlan->ipv4cnt < 0);
 	if (!ipvlan->ipv4cnt)
-- 
1.8.3.1

[PATCH net 4/4] ipvlan: fix check for IP addresses in control path

[Jiri Benc]

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 <jbenc@redhat.com>

---

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(-)

diff --git a/drivers/net/ipvlan/ipvlan.h b/drivers/net/ipvlan/ipvlan.h
index 924ea98bd531..54549a6223dd 100644
--- a/drivers/net/ipvlan/ipvlan.h
+++ b/drivers/net/ipvlan/ipvlan.h
@@ -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);
diff --git a/drivers/net/ipvlan/ipvlan_core.c b/drivers/net/ipvlan/ipvlan_core.c
index 77b7f56d1551..a8a271ecfe65 100644
--- a/drivers/net/ipvlan/ipvlan_core.c
+++ b/drivers/net/ipvlan/ipvlan_core.c
@@ -91,9 +91,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) {
@@ -101,12 +101,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;
 }
 
diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
index f5b1c7b38770..9be4cf809856 100644
--- a/drivers/net/ipvlan/ipvlan_main.c
+++ b/drivers/net/ipvlan/ipvlan_main.c
@@ -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);
@@ -634,7 +634,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;
 
@@ -678,7 +678,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);
@@ -706,7 +706,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