All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net 0/4 v2] ipvlan: list corruption and rcu fixes
@ 2015-03-28 18:13 Jiri Benc
  2015-03-28 18:13 ` [PATCH net 1/4 v3] ipvlan: fix addr hash list corruption Jiri Benc
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Jiri Benc @ 2015-03-28 18:13 UTC (permalink / raw)
  To: netdev; +Cc: Mahesh Bandewar, Dan Williams

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

Only patch 1/4 changed since the last submission.

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 | 28 ++++++++++++++++++++--------
 drivers/net/ipvlan/ipvlan_main.c | 30 +++++++++++++++++++-----------
 3 files changed, 42 insertions(+), 20 deletions(-)

-- 
1.8.3.1

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH net 1/4 v3] ipvlan: fix addr hash list corruption
  2015-03-28 18:13 [PATCH net 0/4 v2] ipvlan: list corruption and rcu fixes Jiri Benc
@ 2015-03-28 18:13 ` Jiri Benc
  2015-03-30 20:24   ` Mahesh Bandewar
  2015-03-28 18:13 ` [PATCH net 2/4 v2] ipvlan: protect against concurrent link removal Jiri Benc
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 9+ messages in thread
From: Jiri Benc @ 2015-03-28 18:13 UTC (permalink / raw)
  To: netdev; +Cc: Mahesh Bandewar, Dan Williams

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

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

diff --git a/drivers/net/ipvlan/ipvlan_core.c b/drivers/net/ipvlan/ipvlan_core.c
index 2a175006028b..8a542b9340c4 100644
--- a/drivers/net/ipvlan/ipvlan_core.c
+++ b/drivers/net/ipvlan/ipvlan_core.c
@@ -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();
 }
diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
index 4f4099d5603d..1eb3f33e11cc 100644
--- a/drivers/net/ipvlan/ipvlan_main.c
+++ b/drivers/net/ipvlan/ipvlan_main.c
@@ -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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH net 2/4 v2] ipvlan: protect against concurrent link removal
  2015-03-28 18:13 [PATCH net 0/4 v2] ipvlan: list corruption and rcu fixes Jiri Benc
  2015-03-28 18:13 ` [PATCH net 1/4 v3] ipvlan: fix addr hash list corruption Jiri Benc
@ 2015-03-28 18:13 ` Jiri Benc
  2015-03-28 18:13 ` [PATCH net 3/4 v2] ipvlan: do not use rcu operations for address list Jiri Benc
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Jiri Benc @ 2015-03-28 18:13 UTC (permalink / raw)
  To: netdev; +Cc: Mahesh Bandewar, Dan Williams

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 8a542b9340c4..568628f95aa2 100644
--- a/drivers/net/ipvlan/ipvlan_core.c
+++ b/drivers/net/ipvlan/ipvlan_core.c
@@ -193,7 +193,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;
 
@@ -220,6 +221,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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH net 3/4 v2] ipvlan: do not use rcu operations for address list
  2015-03-28 18:13 [PATCH net 0/4 v2] ipvlan: list corruption and rcu fixes Jiri Benc
  2015-03-28 18:13 ` [PATCH net 1/4 v3] ipvlan: fix addr hash list corruption Jiri Benc
  2015-03-28 18:13 ` [PATCH net 2/4 v2] ipvlan: protect against concurrent link removal Jiri Benc
@ 2015-03-28 18:13 ` Jiri Benc
  2015-03-31 18:35   ` Cong Wang
  2015-03-28 18:13 ` [PATCH net 4/4 v2] ipvlan: fix check for IP addresses in control path Jiri Benc
  2015-03-31 17:29 ` [PATCH net 0/4 v2] ipvlan: list corruption and rcu fixes David Miller
  4 siblings, 1 reply; 9+ messages in thread
From: Jiri Benc @ 2015-03-28 18:13 UTC (permalink / raw)
  To: netdev; +Cc: Mahesh Bandewar, Dan Williams

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 1eb3f33e11cc..aaa005bd21ce 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.
@@ -640,7 +640,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);
@@ -692,7 +692,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.
@@ -713,7 +713,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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH net 4/4 v2] ipvlan: fix check for IP addresses in control path
  2015-03-28 18:13 [PATCH net 0/4 v2] ipvlan: list corruption and rcu fixes Jiri Benc
                   ` (2 preceding siblings ...)
  2015-03-28 18:13 ` [PATCH net 3/4 v2] ipvlan: do not use rcu operations for address list Jiri Benc
@ 2015-03-28 18:13 ` Jiri Benc
  2015-03-30 20:27   ` Mahesh Bandewar
  2015-03-31 17:29 ` [PATCH net 0/4 v2] ipvlan: list corruption and rcu fixes David Miller
  4 siblings, 1 reply; 9+ messages in thread
From: Jiri Benc @ 2015-03-28 18:13 UTC (permalink / raw)
  To: netdev; +Cc: Mahesh Bandewar, Dan Williams

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 568628f95aa2..b7877a194cfe 100644
--- a/drivers/net/ipvlan/ipvlan_core.c
+++ b/drivers/net/ipvlan/ipvlan_core.c
@@ -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;
 }
 
diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
index aaa005bd21ce..4fa14208d799 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);
@@ -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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH net 1/4 v3] ipvlan: fix addr hash list corruption
  2015-03-28 18:13 ` [PATCH net 1/4 v3] ipvlan: fix addr hash list corruption Jiri Benc
@ 2015-03-30 20:24   ` Mahesh Bandewar
  0 siblings, 0 replies; 9+ messages in thread
From: Mahesh Bandewar @ 2015-03-30 20:24 UTC (permalink / raw)
  To: Jiri Benc; +Cc: linux-netdev, Dan Williams

On Sat, Mar 28, 2015 at 11:13 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>
Signed-off-by: Mahesh Bandewar <maheshb@google.com>
> ---
>
> 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(-)
>
> diff --git a/drivers/net/ipvlan/ipvlan_core.c b/drivers/net/ipvlan/ipvlan_core.c
> index 2a175006028b..8a542b9340c4 100644
> --- a/drivers/net/ipvlan/ipvlan_core.c
> +++ b/drivers/net/ipvlan/ipvlan_core.c
> @@ -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();
>  }
> diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
> index 4f4099d5603d..1eb3f33e11cc 100644
> --- a/drivers/net/ipvlan/ipvlan_main.c
> +++ b/drivers/net/ipvlan/ipvlan_main.c
> @@ -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
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net 4/4 v2] ipvlan: fix check for IP addresses in control path
  2015-03-28 18:13 ` [PATCH net 4/4 v2] ipvlan: fix check for IP addresses in control path Jiri Benc
@ 2015-03-30 20:27   ` Mahesh Bandewar
  0 siblings, 0 replies; 9+ messages in thread
From: Mahesh Bandewar @ 2015-03-30 20:27 UTC (permalink / raw)
  To: Jiri Benc; +Cc: linux-netdev, Dan Williams

On Sat, Mar 28, 2015 at 11:13 AM, Jiri Benc <jbenc@redhat.com> wrote:
> 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>
Acked-by: Mahesh Bandewar <maheshb@google.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 568628f95aa2..b7877a194cfe 100644
> --- a/drivers/net/ipvlan/ipvlan_core.c
> +++ b/drivers/net/ipvlan/ipvlan_core.c
> @@ -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;
>  }
>
> diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
> index aaa005bd21ce..4fa14208d799 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);
> @@ -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
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net 0/4 v2] ipvlan: list corruption and rcu fixes
  2015-03-28 18:13 [PATCH net 0/4 v2] ipvlan: list corruption and rcu fixes Jiri Benc
                   ` (3 preceding siblings ...)
  2015-03-28 18:13 ` [PATCH net 4/4 v2] ipvlan: fix check for IP addresses in control path Jiri Benc
@ 2015-03-31 17:29 ` David Miller
  4 siblings, 0 replies; 9+ messages in thread
From: David Miller @ 2015-03-31 17:29 UTC (permalink / raw)
  To: jbenc; +Cc: netdev, maheshb, dcbw

From: Jiri Benc <jbenc@redhat.com>
Date: Sat, 28 Mar 2015 19:13:21 +0100

> This patch set fixes different issues leading to corrupted lists and
> incorrect rcu usage.
> 
> Only patch 1/4 changed since the last submission.

Series applied, thanks alot.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net 3/4 v2] ipvlan: do not use rcu operations for address list
  2015-03-28 18:13 ` [PATCH net 3/4 v2] ipvlan: do not use rcu operations for address list Jiri Benc
@ 2015-03-31 18:35   ` Cong Wang
  0 siblings, 0 replies; 9+ messages in thread
From: Cong Wang @ 2015-03-31 18:35 UTC (permalink / raw)
  To: Jiri Benc; +Cc: netdev, Mahesh Bandewar, Dan Williams

(Sorry for being late)

On Sat, Mar 28, 2015 at 11:13 AM, Jiri Benc <jbenc@redhat.com> wrote:
> @@ -640,7 +640,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);

But doesn't this mean kfree_rcu() can be replaced by kfree() too?
I noticed ipvlan_ht_addr_del() waits for RCU readers too.

Thanks.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2015-03-31 18:35 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-28 18:13 [PATCH net 0/4 v2] ipvlan: list corruption and rcu fixes Jiri Benc
2015-03-28 18:13 ` [PATCH net 1/4 v3] ipvlan: fix addr hash list corruption Jiri Benc
2015-03-30 20:24   ` Mahesh Bandewar
2015-03-28 18:13 ` [PATCH net 2/4 v2] ipvlan: protect against concurrent link removal Jiri Benc
2015-03-28 18:13 ` [PATCH net 3/4 v2] ipvlan: do not use rcu operations for address list Jiri Benc
2015-03-31 18:35   ` Cong Wang
2015-03-28 18:13 ` [PATCH net 4/4 v2] ipvlan: fix check for IP addresses in control path Jiri Benc
2015-03-30 20:27   ` Mahesh Bandewar
2015-03-31 17:29 ` [PATCH net 0/4 v2] ipvlan: list corruption and rcu fixes David Miller

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.