[PATCH net] ipvlan: fix addr hash list corruption

[PATCH net] 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.

Check the interface status before adding/removing to the hash list in the
notifiers.

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

diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
index 4f4099d5603d..04cfa7a13645 100644
--- a/drivers/net/ipvlan/ipvlan_main.c
+++ b/drivers/net/ipvlan/ipvlan_main.c
@@ -504,7 +504,8 @@ 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);
+			if (netif_running(dev))
+				ipvlan_ht_addr_del(addr, !dev->dismantle);
 			list_del_rcu(&addr->anode);
 		}
 	}
@@ -622,7 +623,8 @@ 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 (netif_running(ipvlan->dev))
+		ipvlan_ht_addr_add(ipvlan, addr);
 
 	return 0;
 }
@@ -635,7 +637,8 @@ static void ipvlan_del_addr6(struct ipvl_dev *ipvlan, struct in6_addr *ip6_addr)
 	if (!addr)
 		return;
 
-	ipvlan_ht_addr_del(addr, true);
+	if (netif_running(ipvlan->dev))
+		ipvlan_ht_addr_del(addr, true);
 	list_del_rcu(&addr->anode);
 	ipvlan->ipv6cnt--;
 	WARN_ON(ipvlan->ipv6cnt < 0);
@@ -690,7 +693,8 @@ 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 (netif_running(ipvlan->dev))
+		ipvlan_ht_addr_add(ipvlan, addr);
 	ipvlan_set_broadcast_mac_filter(ipvlan, true);
 
 	return 0;
@@ -704,7 +708,8 @@ static void ipvlan_del_addr4(struct ipvl_dev *ipvlan, struct in_addr *ip4_addr)
 	if (!addr)
 		return;
 
-	ipvlan_ht_addr_del(addr, true);
+	if (netif_running(ipvlan->dev))
+		ipvlan_ht_addr_del(addr, true);
 	list_del_rcu(&addr->anode);
 	ipvlan->ipv4cnt--;
 	WARN_ON(ipvlan->ipv4cnt < 0);
-- 
1.8.3.1

Re: [PATCH net] ipvlan: fix addr hash list corruption

[Mahesh Bandewar]

On Mon, Mar 23, 2015 at 2:10 PM, 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.
>
Presumably this is creating problems for you and I'm not sure why? Do
you have a script  / (sequence of commands) to produce this condition?

ipvlan_ht_addr_del() does use synchronize_rcu() expect when the device
in dismantle state. Could that be a reason?

> Check the interface status before adding/removing to the hash list in the
> notifiers.
>
> Reported-by: Dan Williams <dcbw@redhat.com>
> Signed-off-by: Jiri Benc <jbenc@redhat.com>
> ---
>  drivers/net/ipvlan/ipvlan_main.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
> index 4f4099d5603d..04cfa7a13645 100644
> --- a/drivers/net/ipvlan/ipvlan_main.c
> +++ b/drivers/net/ipvlan/ipvlan_main.c
> @@ -504,7 +504,8 @@ 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);
> +                       if (netif_running(dev))
> +                               ipvlan_ht_addr_del(addr, !dev->dismantle);
>                         list_del_rcu(&addr->anode);
>                 }
>         }
> @@ -622,7 +623,8 @@ 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 (netif_running(ipvlan->dev))
> +               ipvlan_ht_addr_add(ipvlan, addr);
>
>         return 0;
>  }
> @@ -635,7 +637,8 @@ static void ipvlan_del_addr6(struct ipvl_dev *ipvlan, struct in6_addr *ip6_addr)
>         if (!addr)
>                 return;
>
> -       ipvlan_ht_addr_del(addr, true);
> +       if (netif_running(ipvlan->dev))
> +               ipvlan_ht_addr_del(addr, true);
>         list_del_rcu(&addr->anode);
>         ipvlan->ipv6cnt--;
>         WARN_ON(ipvlan->ipv6cnt < 0);
> @@ -690,7 +693,8 @@ 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 (netif_running(ipvlan->dev))
> +               ipvlan_ht_addr_add(ipvlan, addr);
>         ipvlan_set_broadcast_mac_filter(ipvlan, true);
>
>         return 0;
> @@ -704,7 +708,8 @@ static void ipvlan_del_addr4(struct ipvl_dev *ipvlan, struct in_addr *ip4_addr)
>         if (!addr)
>                 return;
>
> -       ipvlan_ht_addr_del(addr, true);
> +       if (netif_running(ipvlan->dev))
> +               ipvlan_ht_addr_del(addr, true);
>         list_del_rcu(&addr->anode);
>         ipvlan->ipv4cnt--;
>         WARN_ON(ipvlan->ipv4cnt < 0);
> --
> 1.8.3.1
>

Re: [PATCH net] ipvlan: fix addr hash list corruption

[Jiri Benc]

On Mon, 23 Mar 2015 18:10:01 -0700, Mahesh Bandewar wrote:
> On Mon, Mar 23, 2015 at 2:10 PM, 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.
> >
> Presumably this is creating problems for you and I'm not sure why? Do
> you have a script  / (sequence of commands) to produce this condition?

You cannot call hlist_del_rcu on the same pointer twice. Nor
hlist_add_head_rcu, for that matter.

Try e.g.:

ip link add link eth0 name ipvl0 type ipvlan mode l2
ip link set ipvl0 up
ip addr add 1.2.3.1/24 dev ipvl0
ip link set ipvl0 down
	-> ipvlan_ht_addr_del(1.2.3.1)
ip link del ipvl0
	-> ipvlan_ht_addr_del(1.2.3.1)
-> crash

Or:

ip link add link eth0 name ipvl0 type ipvlan mode l2
ip addr add 1.2.3.1/24 dev ipvl0
	-> ipvlan_ht_addr_add(1.2.3.1)
ip link set ipvl0 up
	-> ipvlan_ht_addr_add(1.2.3.1)
-> does not crash immediately but the list is corrupted, go see how the
   pointers in port->hlhead[hash] list look now

> ipvlan_ht_addr_del() does use synchronize_rcu() expect when the device
> in dismantle state. Could that be a reason?

This has nothing to do with RCU.

 Jiri

-- 
Jiri Benc

Re: [PATCH net] ipvlan: fix addr hash list corruption

[David Miller]

From: Jiri Benc <jbenc@redhat.com>
Date: Mon, 23 Mar 2015 22:10:19 +0100

> @@ -504,7 +504,8 @@ 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);
> +			if (netif_running(dev))
> +				ipvlan_ht_addr_del(addr, !dev->dismantle);
>  			list_del_rcu(&addr->anode);
>  		}
>  	}

This is so error prone, because you are depending upon so many implementation
details to infer a boolean state "is this address hashed".

So just add the boolean state to struct ipvl_addr, and manage it in
the ipvlan_ht_addr_{add,del}() code.

Re: [PATCH net] ipvlan: fix addr hash list corruption

[Jiri Benc]

On Tue, 24 Mar 2015 13:00:37 -0400 (EDT), David Miller wrote:
> From: Jiri Benc <jbenc@redhat.com>
> Date: Mon, 23 Mar 2015 22:10:19 +0100
> 
> > @@ -504,7 +504,8 @@ 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);
> > +			if (netif_running(dev))
> > +				ipvlan_ht_addr_del(addr, !dev->dismantle);
> >  			list_del_rcu(&addr->anode);
> >  		}
> >  	}
> 
> This is so error prone, because you are depending upon so many implementation
> details to infer a boolean state "is this address hashed".
> 
> So just add the boolean state to struct ipvl_addr, and manage it in
> the ipvlan_ht_addr_{add,del}() code.

I had that originally but then decided to go with smaller memory
footprint. Which obviously doesn't matter much here. I'll send v2 with
the boolean state.

 Jiri

-- 
Jiri Benc

Re: [PATCH net] ipvlan: fix addr hash list corruption

[Mahesh Bandewar]

On Tue, Mar 24, 2015 at 10:06 AM, Jiri Benc <jbenc@redhat.com> wrote:
> On Tue, 24 Mar 2015 13:00:37 -0400 (EDT), David Miller wrote:
>> From: Jiri Benc <jbenc@redhat.com>
>> Date: Mon, 23 Mar 2015 22:10:19 +0100
>>
>> > @@ -504,7 +504,8 @@ 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);
>> > +                   if (netif_running(dev))
>> > +                           ipvlan_ht_addr_del(addr, !dev->dismantle);
>> >                     list_del_rcu(&addr->anode);
>> >             }
>> >     }
>>
>> This is so error prone, because you are depending upon so many implementation
>> details to infer a boolean state "is this address hashed".
>>
>> So just add the boolean state to struct ipvl_addr, and manage it in
>> the ipvlan_ht_addr_{add,del}() code.
>
> I had that originally but then decided to go with smaller memory
> footprint. Which obviously doesn't matter much here. I'll send v2 with
> the boolean state.
>
Hi Jiri,

Well, we already have hlist_unhashed().The following patch should fix
the duplicate addition as well as deletion. Please give it a try.

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

Thanks,
--mahesh..
>  Jiri
>
> --
> Jiri Benc

Re: [PATCH net] ipvlan: fix addr hash list corruption

[David Miller]

From: Mahesh Bandewar <maheshb@google.com>
Date: Tue, 24 Mar 2015 16:16:38 -0700

> Well, we already have hlist_unhashed().The following patch should fix
> the duplicate addition as well as deletion. Please give it a try.

Yes, this looks a _lot_ better.

Re: [PATCH net] ipvlan: fix addr hash list corruption

[Jiri Benc]

On Tue, 24 Mar 2015 16:16:38 -0700, Mahesh Bandewar wrote:
> Well, we already have hlist_unhashed().The following patch should fix
> the duplicate addition as well as deletion. Please give it a try.

Good idea, it's surely better than adding a new boolean.

However, I'm wondering that when there's apparently no problem with the
addresses being on the hash list when interface is down, what's the
point in clearing the hash list in the ndo_stop handler and
repopulating it in ndo_open?

The following patch fixes the problem, too, and as a bonus removes code:

diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
index 4f4099d5603d..03def841b1a9 100644
--- a/drivers/net/ipvlan/ipvlan_main.c
+++ b/drivers/net/ipvlan/ipvlan_main.c
@@ -149,10 +149,6 @@ static int ipvlan_open(struct net_device *dev)
 	else
 		dev->flags &= ~IFF_NOARP;
 
-	if (ipvlan->ipv6cnt > 0 || ipvlan->ipv4cnt > 0) {
-		list_for_each_entry(addr, &ipvlan->addrs, anode)
-			ipvlan_ht_addr_add(ipvlan, addr);
-	}
 	return dev_uc_add(phy_dev, phy_dev->dev_addr);
 }
 
@@ -166,11 +162,6 @@ static int ipvlan_stop(struct net_device *dev)
 	dev_mc_unsync(phy_dev, dev);
 
 	dev_uc_del(phy_dev, phy_dev->dev_addr);
-
-	if (ipvlan->ipv6cnt > 0 || ipvlan->ipv4cnt > 0) {
-		list_for_each_entry(addr, &ipvlan->addrs, anode)
-			ipvlan_ht_addr_del(addr, !dev->dismantle);
-	}
 	return 0;
 }
 

What do you think? Should I submit this formally?

 Jiri

-- 
Jiri Benc

Re: [PATCH net] ipvlan: fix addr hash list corruption

[David Miller]

From: Jiri Benc <jbenc@redhat.com>
Date: Wed, 25 Mar 2015 09:58:51 +0100

> On Tue, 24 Mar 2015 16:16:38 -0700, Mahesh Bandewar wrote:
>> Well, we already have hlist_unhashed().The following patch should fix
>> the duplicate addition as well as deletion. Please give it a try.
> 
> Good idea, it's surely better than adding a new boolean.
> 
> However, I'm wondering that when there's apparently no problem with the
> addresses being on the hash list when interface is down, what's the
> point in clearing the hash list in the ndo_stop handler and
> repopulating it in ndo_open?
> 
> The following patch fixes the problem, too, and as a bonus removes code:

I'll let Mahesh reply to this.

Re: [PATCH net] ipvlan: fix addr hash list corruption

[Mahesh Bandewar]

On Wed, Mar 25, 2015 at 8:46 AM, David Miller <davem@davemloft.net> wrote:
> From: Jiri Benc <jbenc@redhat.com>
> Date: Wed, 25 Mar 2015 09:58:51 +0100
>
>> On Tue, 24 Mar 2015 16:16:38 -0700, Mahesh Bandewar wrote:
>>> Well, we already have hlist_unhashed().The following patch should fix
>>> the duplicate addition as well as deletion. Please give it a try.
>>
>> Good idea, it's surely better than adding a new boolean.
>>
>> However, I'm wondering that when there's apparently no problem with the
>> addresses being on the hash list when interface is down, what's the
>> point in clearing the hash list in the ndo_stop handler and
>> repopulating it in ndo_open?
>>
>> The following patch fixes the problem, too, and as a bonus removes code:
>
> I'll let Mahesh reply to this.

Yes functionally you will get the same result. However during the RX
processing, that code helps ipvlan-demux machine along with
packet-dispatcher to determine it early to drop the packet rather than
later. Also note that addition / deletion of address entries in
hash-table is done in control-path while the demux / dispatcher
operate in data-path. So for this reason I would prefer to leave the
hash-table entries addition / deletion as it is.

--mahesh..

Re: [PATCH net] ipvlan: fix addr hash list corruption

[Dan Williams]

On Wed, 2015-03-25 at 11:11 -0700, Mahesh Bandewar wrote:
> On Wed, Mar 25, 2015 at 8:46 AM, David Miller <davem@davemloft.net> wrote:
> > From: Jiri Benc <jbenc@redhat.com>
> > Date: Wed, 25 Mar 2015 09:58:51 +0100
> >
> >> On Tue, 24 Mar 2015 16:16:38 -0700, Mahesh Bandewar wrote:
> >>> Well, we already have hlist_unhashed().The following patch should fix
> >>> the duplicate addition as well as deletion. Please give it a try.
> >>
> >> Good idea, it's surely better than adding a new boolean.
> >>
> >> However, I'm wondering that when there's apparently no problem with the
> >> addresses being on the hash list when interface is down, what's the
> >> point in clearing the hash list in the ndo_stop handler and
> >> repopulating it in ndo_open?
> >>
> >> The following patch fixes the problem, too, and as a bonus removes code:
> >
> > I'll let Mahesh reply to this.
> 
> Yes functionally you will get the same result. However during the RX
> processing, that code helps ipvlan-demux machine along with
> packet-dispatcher to determine it early to drop the packet rather than
> later. Also note that addition / deletion of address entries in
> hash-table is done in control-path while the demux / dispatcher
> operate in data-path. So for this reason I would prefer to leave the
> hash-table entries addition / deletion as it is.

Jiri's patch was actually prompted by my testing of ipvlan with L2 mode.
How much testing of L2 have you given ipvlan internally and what setups
have you tested?  It doesn't look like ipvlan handles ARP/ICMP very well
at all right now, and while I've got patches to fix some of that I'm
trying to characterize the rest.  Also, have you ever tested it with
DHCP?

Dan

Re: [PATCH net] ipvlan: fix addr hash list corruption

[Jiri Benc]

On Wed, 25 Mar 2015 11:11:47 -0700, Mahesh Bandewar wrote:
> On Wed, Mar 25, 2015 at 8:46 AM, David Miller <davem@davemloft.net> wrote:
> > From: Jiri Benc <jbenc@redhat.com>
> >> However, I'm wondering that when there's apparently no problem with the
> >> addresses being on the hash list when interface is down, what's the
> >> point in clearing the hash list in the ndo_stop handler and
> >> repopulating it in ndo_open?
> >>
> >> The following patch fixes the problem, too, and as a bonus removes code:
> >
> > I'll let Mahesh reply to this.
> 
> Yes functionally you will get the same result. However during the RX
> processing, that code helps ipvlan-demux machine along with
> packet-dispatcher to determine it early to drop the packet rather than
> later.

When the interface is down, this doesn't matter, does it? You don't
send/receive anything when the interface is down. But this is actually
not so important for the discussion, see below.

> Also note that addition / deletion of address entries in
> hash-table is done in control-path while the demux / dispatcher
> operate in data-path. So for this reason I would prefer to leave the
> hash-table entries addition / deletion as it is.

I don't understand how the context in which the addresses are added is
relevant to the problem at hand. The addresses are still added and
removed in the control path whichever patch is applied.

Basically, there are two (and only two) possible cases: 1. the
addresses should not be on the hash list when the interface is down,
and 2. there's no problem with the addresses being on the hash list
when the interface is down.

If 1. is true, than my first patch does exactly that. It ensures that
the addresses are on the hash list if and only if the interface is up.

If 2. is true, than my second patch does that. Addresses are added to
the hash list on their addition to the interface and removed on their
removal.

The patch you sent (and the boolean flag David suggested) is actually
kind of strange hybrid: when the interface is down, sometimes the
addresses are on the hash list and sometimes not, depending on the
order in which the user added them and brought the interface up/down.

Maybe I'm just missing some important piece of information, though, in
which case it would help me if you could explain why these two
operations are fundamentally different (assuming ipvlan0 is up at the
beginning of each):

ip a a 1.2.3.4/24 dev ipvlan0
ip l s ipvlan0 down
-> at this point, 1.2.3.4 is *not* on the hash list

and

ip l s ipvlan0 down
ip a a 1.2.3.4/24 dev ipvlan0
-> at this point, 1.2.3.4 is on the hash list

Please note that my first patch turns both consistently to the first
result, my second patch turns both consistently to the second result.

Thanks,

 Jiri

-- 
Jiri Benc

Re: [PATCH net] ipvlan: fix addr hash list corruption

[Jiri Benc]

On Thu, 26 Mar 2015 00:21:53 +0100, Jiri Benc wrote:
> On Wed, 25 Mar 2015 11:11:47 -0700, Mahesh Bandewar wrote:
> > Yes functionally you will get the same result. However during the RX
> > processing, that code helps ipvlan-demux machine along with
> > packet-dispatcher to determine it early to drop the packet rather than
> > later.
> 
> When the interface is down, this doesn't matter, does it? You don't
> send/receive anything when the interface is down.

But obviously, with more ipvlan interfaces under the same master, some
of them being up and some down, it may be beneficial to keep the
addresses of interfaces that are down off the hash list. Which means my
original patch was correct after all...

Another question is whether the logic in ipvlan_addr_busy is correct.
To my understanding, the purpose of ipvlan_ht_addr_lookup call in
ipvlan_addr_busy is to ensure two interfaces under the same master
cannot have the same IP address assigned. As assigned addresses may not
be on the hash list when an interface is down, this check does not
really work. Note that this problem is already present in the current
code, with or without any patches currently discussed.

 Jiri

-- 
Jiri Benc

Re: [PATCH net] ipvlan: fix addr hash list corruption

[Mahesh Bandewar]

On Wed, Mar 25, 2015 at 4:21 PM, Jiri Benc <jbenc@redhat.com> wrote:
>
> On Wed, 25 Mar 2015 11:11:47 -0700, Mahesh Bandewar wrote:
> > On Wed, Mar 25, 2015 at 8:46 AM, David Miller <davem@davemloft.net> wrote:
> > > From: Jiri Benc <jbenc@redhat.com>
> > >> However, I'm wondering that when there's apparently no problem with the
> > >> addresses being on the hash list when interface is down, what's the
> > >> point in clearing the hash list in the ndo_stop handler and
> > >> repopulating it in ndo_open?
> > >>
> > >> The following patch fixes the problem, too, and as a bonus removes code:
> > >
> > > I'll let Mahesh reply to this.
> >
> > Yes functionally you will get the same result. However during the RX
> > processing, that code helps ipvlan-demux machine along with
> > packet-dispatcher to determine it early to drop the packet rather than
> > later.
>
> When the interface is down, this doesn't matter, does it? You don't
> send/receive anything when the interface is down. But this is actually
> not so important for the discussion, see below.
>
When the interface is down, you wont send but you might receive.

> > Also note that addition / deletion of address entries in
> > hash-table is done in control-path while the demux / dispatcher
> > operate in data-path. So for this reason I would prefer to leave the
> > hash-table entries addition / deletion as it is.
>
> I don't understand how the context in which the addresses are added is
> relevant to the problem at hand. The addresses are still added and
> removed in the control path whichever patch is applied.
>
I think you missed the point. Addition / deletion of addresses into
the hash-table (always) happens in the control path while the cost of
those decisions will be paid per packet in data-path; was the point I
was trying to make.

> Basically, there are two (and only two) possible cases: 1. the
> addresses should not be on the hash list when the interface is down,
> and 2. there's no problem with the addresses being on the hash list
> when the interface is down.
>
As I mentioned earlier in my previous reply; it does work and
functionally same. However the decision to drop the packet will happen
later in RX path when the interface is down.

> If 1. is true, than my first patch does exactly that. It ensures that
> the addresses are on the hash list if and only if the interface is up.
>
> If 2. is true, than my second patch does that. Addresses are added to
> the hash list on their addition to the interface and removed on their
> removal.
>
> The patch you sent (and the boolean flag David suggested) is actually
> kind of strange hybrid: when the interface is down, sometimes the
> addresses are on the hash list and sometimes not, depending on the
> order in which the user added them and brought the interface up/down.
>
The root cause of the issue is addition / deletion of duplicate
entries into the hash-table and I think fixing it is better thing
which is exactly what the fix I proposed do. Your first patch though
fixes the symptoms, I believe it's not the correct fix while your
second patch does fix it but eliminates this optimization that is in
place. Hence I suggested that  "though correct, I would not *prefer*
it" for the said reasons. If the order in which user pushes config
alters the hash-table entries, then that's unintended and need to be
fixed.

> Maybe I'm just missing some important piece of information, though, in
> which case it would help me if you could explain why these two
> operations are fundamentally different (assuming ipvlan0 is up at the
> beginning of each):
>
> ip a a 1.2.3.4/24 dev ipvlan0
> ip l s ipvlan0 down
> -> at this point, 1.2.3.4 is *not* on the hash list
>
Intended behavior.

> and
>
> ip l s ipvlan0 down
> ip a a 1.2.3.4/24 dev ipvlan0
> -> at this point, 1.2.3.4 is on the hash list
>
unintended behavior and need to be fixed.

> Please note that my first patch turns both consistently to the first
> result, my second patch turns both consistently to the second result.
>
Well, no one can force you to choose which patch to use to fix the
problem you are facing. I'm merely suggesting which fix is a better
fix with reasons (of course in my opinion). I don't see a point in
this proof / debate.

> Thanks,
>
>  Jiri
>
> --
> Jiri Benc

Re: [PATCH net] ipvlan: fix addr hash list corruption

[Jiri Benc]

On Wed, 25 Mar 2015 19:15:55 -0700, Mahesh Bandewar wrote:
> I think you missed the point. Addition / deletion of addresses into
> the hash-table (always) happens in the control path while the cost of
> those decisions will be paid per packet in data-path; was the point I
> was trying to make.

I think you're trying to say "don't add addresses to the hash table
unless necessary". If so, we're not in argument here.

> > Basically, there are two (and only two) possible cases: 1. the
> > addresses should not be on the hash list when the interface is down,
> > and 2. there's no problem with the addresses being on the hash list
> > when the interface is down.
> >
> As I mentioned earlier in my previous reply; it does work and
> functionally same. However the decision to drop the packet will happen
> later in RX path when the interface is down.

Yes.

> The root cause of the issue is addition / deletion of duplicate
> entries into the hash-table and I think fixing it is better thing
> which is exactly what the fix I proposed do.

Except that it fixes only part of the bug, see below.

> Your first patch though
> fixes the symptoms, I believe it's not the correct fix

Could you please elaborate why it's not the correct fix? It ensures
that the addresses are on the hash list *iff* the interface is up. When
the interface is down, the addresses are not there. If it is up, they
are. My impression so far is this is exactly what you want. I don't see
why the patch is not correct.

> while your
> second patch does fix it but eliminates this optimization that is in
> place.

Agreed.

> Hence I suggested that  "though correct, I would not *prefer*
> it" for the said reasons. If the order in which user pushes config
> alters the hash-table entries, then that's unintended and need to be
> fixed.

Agreed.

> > ip l s ipvlan0 down
> > ip a a 1.2.3.4/24 dev ipvlan0
> > -> at this point, 1.2.3.4 is on the hash list
> >
> unintended behavior and need to be fixed.

Which is exactly what my first patch does. Could you please look at it
again, keeping also this scenario in mind?

Thanks,

 Jiri

-- 
Jiri Benc

Re: [PATCH net] ipvlan: fix addr hash list corruption

[Mahesh Bandewar]

On Thu, Mar 26, 2015 at 1:45 AM, Jiri Benc <jbenc@redhat.com> wrote:
> On Wed, 25 Mar 2015 19:15:55 -0700, Mahesh Bandewar wrote:
>> I think you missed the point. Addition / deletion of addresses into
>> the hash-table (always) happens in the control path while the cost of
>> those decisions will be paid per packet in data-path; was the point I
>> was trying to make.
>
> I think you're trying to say "don't add addresses to the hash table
> unless necessary". If so, we're not in argument here.
>
>> > Basically, there are two (and only two) possible cases: 1. the
>> > addresses should not be on the hash list when the interface is down,
>> > and 2. there's no problem with the addresses being on the hash list
>> > when the interface is down.
>> >
>> As I mentioned earlier in my previous reply; it does work and
>> functionally same. However the decision to drop the packet will happen
>> later in RX path when the interface is down.
>
> Yes.
>
>> The root cause of the issue is addition / deletion of duplicate
>> entries into the hash-table and I think fixing it is better thing
>> which is exactly what the fix I proposed do.
>
> Except that it fixes only part of the bug, see below.
>
>> Your first patch though
>> fixes the symptoms, I believe it's not the correct fix
>
> Could you please elaborate why it's not the correct fix? It ensures
> that the addresses are on the hash list *iff* the interface is up. When
> the interface is down, the addresses are not there. If it is up, they
> are. My impression so far is this is exactly what you want. I don't see
> why the patch is not correct.
>
>> while your
>> second patch does fix it but eliminates this optimization that is in
>> place.
>
> Agreed.
>
>> Hence I suggested that  "though correct, I would not *prefer*
>> it" for the said reasons. If the order in which user pushes config
>> alters the hash-table entries, then that's unintended and need to be
>> fixed.
>
> Agreed.
>
>> > ip l s ipvlan0 down
>> > ip a a 1.2.3.4/24 dev ipvlan0
>> > -> at this point, 1.2.3.4 is on the hash list
>> >
>> unintended behavior and need to be fixed.
>
> Which is exactly what my first patch does. Could you please look at it
> again, keeping also this scenario in mind?
>
OK. I'm not comfortable using the patch as it is since duplicate
entries in hash-table should be controlled by the fact that link is up
or down. So I would like to use the hybrid approach where the
hash-table entries are controlled by hlist_unhashed() while the
addr_add event should add hash-table entry should the link be up (as
it is in your first patch). addr_del event handler can stay as it is.
This should take care of the crash issue.

Now the ipvlan_addr_busy() can not use hash-table reliably and should
first iterate through all the logical links on a port and then iterate
through all addresses on logical link.

Do you want to send this / these patches or do you want me to send?

> Thanks,
>
>  Jiri
>
> --
> Jiri Benc