[PATCH] net/garp: avoid infinite loop if attribute already exists

[PATCH] net/garp: avoid infinite loop if attribute already exists

[David Ward]

An infinite loop occurred if garp_attr_create was called with the
values of an existing attribute. Return -EEXIST instead.

Signed-off-by: David Ward <david.ward@ll.mit.edu>
---
 net/802/garp.c |   18 +++++++++++++-----
 1 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/net/802/garp.c b/net/802/garp.c
index 8e21b6d..bb5015e 100644
--- a/net/802/garp.c
+++ b/net/802/garp.c
@@ -167,7 +167,7 @@ static struct garp_attr *garp_attr_lookup(const struct garp_applicant *app,
 	return NULL;
 }
 
-static void garp_attr_insert(struct garp_applicant *app, struct garp_attr *new)
+static int garp_attr_insert(struct garp_applicant *app, struct garp_attr *new)
 {
 	struct rb_node *parent = NULL, **p = &app->gid.rb_node;
 	struct garp_attr *attr;
@@ -181,24 +181,32 @@ static void garp_attr_insert(struct garp_applicant *app, struct garp_attr *new)
 			p = &parent->rb_left;
 		else if (d > 0)
 			p = &parent->rb_right;
+		else
+			return -EEXIST;
 	}
 	rb_link_node(&new->node, parent, p);
 	rb_insert_color(&new->node, &app->gid);
+	return 0;
 }
 
 static struct garp_attr *garp_attr_create(struct garp_applicant *app,
 					  const void *data, u8 len, u8 type)
 {
 	struct garp_attr *attr;
+	int err;
 
 	attr = kmalloc(sizeof(*attr) + len, GFP_ATOMIC);
 	if (!attr)
-		return attr;
+		return PTR_ERR(-ENOMEM);
 	attr->state = GARP_APPLICANT_VO;
 	attr->type  = type;
 	attr->dlen  = len;
 	memcpy(attr->data, data, len);
-	garp_attr_insert(app, attr);
+	err = garp_attr_insert(app, attr);
+	if (err < 0) {
+		kfree(attr);
+		return PTR_ERR(err);
+	}
 	return attr;
 }
 
@@ -353,9 +361,9 @@ int garp_request_join(const struct net_device *dev,
 
 	spin_lock_bh(&app->lock);
 	attr = garp_attr_create(app, data, len, type);
-	if (!attr) {
+	if (IS_ERR(attr)) {
 		spin_unlock_bh(&app->lock);
-		return -ENOMEM;
+		return ERR_PTR(attr);
 	}
 	garp_attr_event(app, attr, GARP_EVENT_REQ_JOIN);
 	spin_unlock_bh(&app->lock);
-- 
1.7.1

[PATCH] net/vlan: withdraw VLAN ID attribute from GVRP on VLAN device stop

[David Ward]

When a VLAN device is stopped which has VLAN_FLAG_GVRP set, the VLAN
ID attribute that was previously declared by GVRP must be withdrawn.

Signed-off-by: David Ward <david.ward@ll.mit.edu>
---
 net/8021q/vlan_dev.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 9988d4a..df86dd0 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -319,6 +319,9 @@ static int vlan_dev_stop(struct net_device *dev)
 	struct vlan_dev_priv *vlan = vlan_dev_priv(dev);
 	struct net_device *real_dev = vlan->real_dev;
 
+	if (vlan->flags & VLAN_FLAG_GVRP)
+		vlan_gvrp_request_leave(dev);
+
 	dev_mc_unsync(real_dev, dev);
 	dev_uc_unsync(real_dev, dev);
 	if (dev->flags & IFF_ALLMULTI)
-- 
1.7.1

Re: [PATCH] net/vlan: withdraw VLAN ID attribute from GVRP on VLAN device stop

[Jorge Boncompte [DTI2]]

El 26/03/2012 0:43, David Ward escribió:
> When a VLAN device is stopped which has VLAN_FLAG_GVRP set, the VLAN
> ID attribute that was previously declared by GVRP must be withdrawn.
> 

	Hmm, maybe I am missing something but I think it only makes sense to withdrawn
the attribute when you delete the interface, and vlan_dev_stop() it's called if
you just put the interface down. It's better for the network convergence to not
signal the switches just for this. IMHO.


> Signed-off-by: David Ward <david.ward@ll.mit.edu>
> ---
>  net/8021q/vlan_dev.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
> index 9988d4a..df86dd0 100644
> --- a/net/8021q/vlan_dev.c
> +++ b/net/8021q/vlan_dev.c
> @@ -319,6 +319,9 @@ static int vlan_dev_stop(struct net_device *dev)
>  	struct vlan_dev_priv *vlan = vlan_dev_priv(dev);
>  	struct net_device *real_dev = vlan->real_dev;
>  
> +	if (vlan->flags & VLAN_FLAG_GVRP)
> +		vlan_gvrp_request_leave(dev);
> +
>  	dev_mc_unsync(real_dev, dev);
>  	dev_uc_unsync(real_dev, dev);
>  	if (dev->flags & IFF_ALLMULTI)

Re: [PATCH] net/vlan: withdraw VLAN ID attribute from GVRP on VLAN device stop

[Ward, David - 0663 - MITLL]

On 26/03/12 07:29, Jorge Boncompte [DTI2] wrote:
> El 26/03/2012 0:43, David Ward escribió:
>> When a VLAN device is stopped which has VLAN_FLAG_GVRP set, the VLAN
>> ID attribute that was previously declared by GVRP must be withdrawn.
>>
> 	Hmm, maybe I am missing something but I think it only makes sense to withdrawn
> the attribute when you delete the interface, and vlan_dev_stop() it's called if
> you just put the interface down. It's better for the network convergence to not
> signal the switches just for this. IMHO.

If I bring a VLAN interface down, then I stop participating in the 
VLAN.  If my NIC still receives traffic for the VLAN, I drop it.  So to 
remove unnecessary load on the (potentially shared) network link and 
remove unnecessary local processing by the kernel of packets I know I am 
going to drop, I should tell the switch that I am no longer interested 
in receiving the VLAN traffic.  Which is the whole point of GVRP.  Right?

In any case, we currently register the attribute when the interface is 
brought up, not when it is created.  However we do it, the attribute 
declaration/withdrawal should be symmetric.

>
>> Signed-off-by: David Ward<david.ward@ll.mit.edu>
>> ---
>>   net/8021q/vlan_dev.c |    3 +++
>>   1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
>> index 9988d4a..df86dd0 100644
>> --- a/net/8021q/vlan_dev.c
>> +++ b/net/8021q/vlan_dev.c
>> @@ -319,6 +319,9 @@ static int vlan_dev_stop(struct net_device *dev)
>>   	struct vlan_dev_priv *vlan = vlan_dev_priv(dev);
>>   	struct net_device *real_dev = vlan->real_dev;
>>
>> +	if (vlan->flags&  VLAN_FLAG_GVRP)
>> +		vlan_gvrp_request_leave(dev);
>> +
>>   	dev_mc_unsync(real_dev, dev);
>>   	dev_uc_unsync(real_dev, dev);
>>   	if (dev->flags&  IFF_ALLMULTI)

Re: [PATCH] net/vlan: withdraw VLAN ID attribute from GVRP on VLAN device stop

[Jorge Boncompte [DTI2]]

El 26/03/2012 15:38, Ward, David - 0663 - MITLL escribió:
> On 26/03/12 07:29, Jorge Boncompte [DTI2] wrote:
>> El 26/03/2012 0:43, David Ward escribió:
>>> When a VLAN device is stopped which has VLAN_FLAG_GVRP set, the VLAN
>>> ID attribute that was previously declared by GVRP must be withdrawn.
>>>
>> 	Hmm, maybe I am missing something but I think it only makes sense to withdrawn
>> the attribute when you delete the interface, and vlan_dev_stop() it's called if
>> you just put the interface down. It's better for the network convergence to not
>> signal the switches just for this. IMHO.
> 
> If I bring a VLAN interface down, then I stop participating in the 
> VLAN.  If my NIC still receives traffic for the VLAN, I drop it.  So to 
> remove unnecessary load on the (potentially shared) network link and 
> remove unnecessary local processing by the kernel of packets I know I am 
> going to drop, I should tell the switch that I am no longer interested 
> in receiving the VLAN traffic.  Which is the whole point of GVRP.  Right?

	In the non-GVRP case, as far as i can see, you still receive the traffic for
that VLAN and the kernel drops it. Maybe is that I think that a downed interface
it's more a transient state, and administratively choosen one. If you don't want
to participate in that VLAN, you always can disable GVRP on it or delete the
interface.

> In any case, we currently register the attribute when the interface is 
> brought up, not when it is created.

	And that makes sense to me, if you have never uped the interface you have never
participated in that VLAN.

>  However we do it, the attribute
> declaration/withdrawal should be symmetric.
> 
>>
>>> Signed-off-by: David Ward<david.ward@ll.mit.edu>
>>> ---
>>>   net/8021q/vlan_dev.c |    3 +++
>>>   1 files changed, 3 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
>>> index 9988d4a..df86dd0 100644
>>> --- a/net/8021q/vlan_dev.c
>>> +++ b/net/8021q/vlan_dev.c
>>> @@ -319,6 +319,9 @@ static int vlan_dev_stop(struct net_device *dev)
>>>   	struct vlan_dev_priv *vlan = vlan_dev_priv(dev);
>>>   	struct net_device *real_dev = vlan->real_dev;
>>>
>>> +	if (vlan->flags&  VLAN_FLAG_GVRP)
>>> +		vlan_gvrp_request_leave(dev);
>>> +
>>>   	dev_mc_unsync(real_dev, dev);
>>>   	dev_uc_unsync(real_dev, dev);
>>>   	if (dev->flags&  IFF_ALLMULTI)--
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH] net/vlan: withdraw VLAN ID attribute from GVRP on VLAN device stop

[Ward, David - 0663 - MITLL]

[-- Attachment #1: Type: text/plain, Size: 4157 bytes --]

On 26/03/12 11:14, Jorge Boncompte [DTI2] wrote:
> El 26/03/2012 15:38, Ward, David - 0663 - MITLL escribió:
>> On 26/03/12 07:29, Jorge Boncompte [DTI2] wrote:
>>> El 26/03/2012 0:43, David Ward escribió:
>>>> When a VLAN device is stopped which has VLAN_FLAG_GVRP set, the VLAN
>>>> ID attribute that was previously declared by GVRP must be withdrawn.
>>>>
>>> 	Hmm, maybe I am missing something but I think it only makes sense to withdrawn
>>> the attribute when you delete the interface, and vlan_dev_stop() it's called if
>>> you just put the interface down. It's better for the network convergence to not
>>> signal the switches just for this. IMHO.
>> If I bring a VLAN interface down, then I stop participating in the
>> VLAN.  If my NIC still receives traffic for the VLAN, I drop it.  So to
>> remove unnecessary load on the (potentially shared) network link and
>> remove unnecessary local processing by the kernel of packets I know I am
>> going to drop, I should tell the switch that I am no longer interested
>> in receiving the VLAN traffic.  Which is the whole point of GVRP.  Right?
> 	In the non-GVRP case, as far as i can see, you still receive the traffic for
> that VLAN and the kernel drops it.
But this is why GVRP is advantageous.  I don't have to have a static 
configuration on the switch for the VLANs I am participating in, and so 
I only receive VLAN traffic when the VLAN interface is active and I 
would actually process it.

>   Maybe is that I think that a downed interface
> it's more a transient state, and administratively choosen one. If you don't want
> to participate in that VLAN, you always can disable GVRP on it or delete the
> interface.
>
>> In any case, we currently register the attribute when the interface is
>> brought up, not when it is created.
> 	And that makes sense to me, if you have never uped the interface you have never
> participated in that VLAN.

If I create an interface that is initially down, then bring it up, and 
finally bring it back down, I think it should return to the state it was 
in initially.  So in my opinion, either the GVRP attribute should exist 
while the interface is down, or it shouldn't.

For comparison, bringing a physical NIC interface down causes the other 
end of a point-to-point Ethernet link to sense "no carrier", which could 
trigger a topology change on the switch.  So to me, it's semantically 
the same that bringing a VLAN interface down could trigger a topology 
change on the switch.

If your concern is about the effect on the network switching topology of 
brief periods in which you bring the VLAN interface down (in order to 
move it to another namespace), keep in mind that the default LeaveTimer 
is 600 ms so the switch wouldn't propagate any topology changes unless 
you don't bring the interface back up by then.  Or you could consider 
changing this timer on your switch.

David

>>   However we do it, the attribute
>> declaration/withdrawal should be symmetric.
>>
>>>> Signed-off-by: David Ward<david.ward@ll.mit.edu>
>>>> ---
>>>>    net/8021q/vlan_dev.c |    3 +++
>>>>    1 files changed, 3 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
>>>> index 9988d4a..df86dd0 100644
>>>> --- a/net/8021q/vlan_dev.c
>>>> +++ b/net/8021q/vlan_dev.c
>>>> @@ -319,6 +319,9 @@ static int vlan_dev_stop(struct net_device *dev)
>>>>    	struct vlan_dev_priv *vlan = vlan_dev_priv(dev);
>>>>    	struct net_device *real_dev = vlan->real_dev;
>>>>
>>>> +	if (vlan->flags&   VLAN_FLAG_GVRP)
>>>> +		vlan_gvrp_request_leave(dev);
>>>> +
>>>>    	dev_mc_unsync(real_dev, dev);
>>>>    	dev_uc_unsync(real_dev, dev);
>>>>    	if (dev->flags&   IFF_ALLMULTI)--
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>


-- 
David Ward, Associate Staff
Wideband Tactical Networking Group
MIT Lincoln Laboratory
Office: 781-981-4266
Mobile: 781-999-1925
Fax: 781-981-4583



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4558 bytes --]

Re: [PATCH] net/vlan: withdraw VLAN ID attribute from GVRP on VLAN device stop

[David Miller]

From: "Ward, David - 0663 - MITLL" <david.ward@ll.mit.edu>
Date: Mon, 26 Mar 2012 11:50:16 -0400

> If I create an interface that is initially down, then bring it up, and
> finally bring it back down, I think it should return to the state it
> was in initially.  So in my opinion, either the GVRP attribute should
> exist while the interface is down, or it shouldn't.

You're whole argument breaks down because we definitely don't do this
for IPv4 addresses, we'll keep receiving traffic for IPv4 addresses
configured on that interface when received on other interfaces which
are still up, even when you bring that interface down.

Really, if you want to leave GVRP, unconfigure it.

I'm not applying this patch.

Re: [PATCH] net/vlan: withdraw VLAN ID attribute from GVRP on VLAN device stop

[Ward, David - 0663 - MITLL]

[-- Attachment #1: Type: text/plain, Size: 2194 bytes --]

On 03/26/2012 05:42 PM, David Miller wrote:
> From: "Ward, David - 0663 - MITLL"<david.ward@ll.mit.edu>
> Date: Mon, 26 Mar 2012 11:50:16 -0400
>
>> If I create an interface that is initially down, then bring it up, and
>> finally bring it back down, I think it should return to the state it
>> was in initially.  So in my opinion, either the GVRP attribute should
>> exist while the interface is down, or it shouldn't.
> You're whole argument breaks down because we definitely don't do this
> for IPv4 addresses, we'll keep receiving traffic for IPv4 addresses
> configured on that interface when received on other interfaces which
> are still up, even when you bring that interface down.

... but that also seems to work if you never bring the interface up.  I 
can create a dummy interface, set an IP address on it, and immediately 
receive traffic on that IP address from another interface, without ever 
having brought the dummy interface up.  (The behavior after setting the 
interface down is the same as before the interface was ever brought up.)

Instead with GVRP, I create an interface and set the GVRP flag on it, 
but GVRP does not declare the attribute for that VLAN until you bring 
the VLAN interface up the first time.  However it doesn't withdraw the 
attribute when you bring the VLAN interface down, only when you delete it.

So to approach this differently -- would there be anything wrong with 
GVRP declaring the attribute as soon as the GVRP flag is set, even if 
the VLAN interface has not come up yet?  The user might do this to 
deliberately give the switching infrastructure a head start, so that 
(for example) the IPv6 DAD packets that are sent as soon as the VLAN 
interface comes up make it to all participants on the LAN...right now 
you can't do that.  You could still choose not to set the GVRP flag on 
the interface until you bring it up, similar to what you were saying.

Then each attribute declaration event would have an opposite attribute 
withdrawal event.  This means if we tried to create an attribute that 
already existed and wasn't in the "leaving" state, it would alert us to 
a bug.

Thoughts?

David


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4558 bytes --]

Re: [PATCH] net/garp: avoid infinite loop if attribute already exists

[Jorge Boncompte [DTI2]]

[-- Attachment #1: Type: text/plain, Size: 2697 bytes --]

El 26/03/2012 0:43, David Ward escribió:
> An infinite loop occurred if garp_attr_create was called with the
> values of an existing attribute. Return -EEXIST instead.

	I should have sent this some months ago but others things keep me from doing it.
	Anyway, I think that the right thing to do it's reuse the attribute to not
disturb the switch/network. Also, returning an error it's pointless because
nobody checks vlan_gvrp_request_join() return and you'll end up in a state where
the VLAN device has the GVRP flag but it's not announcing the attribute.

 	Please take a look at the conversation I had with Patrick in the patch commit log.

	If you agree, I'm fine with you redoing your patch or David using mine if he
thinks it's ok.

	Regards,
		Jorge

> 
> Signed-off-by: David Ward <david.ward@ll.mit.edu>
> ---
>  net/802/garp.c |   18 +++++++++++++-----
>  1 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/net/802/garp.c b/net/802/garp.c
> index 8e21b6d..bb5015e 100644
> --- a/net/802/garp.c
> +++ b/net/802/garp.c
> @@ -167,7 +167,7 @@ static struct garp_attr *garp_attr_lookup(const struct garp_applicant *app,
>  	return NULL;
>  }
>  
> -static void garp_attr_insert(struct garp_applicant *app, struct garp_attr *new)
> +static int garp_attr_insert(struct garp_applicant *app, struct garp_attr *new)
>  {
>  	struct rb_node *parent = NULL, **p = &app->gid.rb_node;
>  	struct garp_attr *attr;
> @@ -181,24 +181,32 @@ static void garp_attr_insert(struct garp_applicant *app, struct garp_attr *new)
>  			p = &parent->rb_left;
>  		else if (d > 0)
>  			p = &parent->rb_right;
> +		else
> +			return -EEXIST;
>  	}
>  	rb_link_node(&new->node, parent, p);
>  	rb_insert_color(&new->node, &app->gid);
> +	return 0;
>  }
>  
>  static struct garp_attr *garp_attr_create(struct garp_applicant *app,
>  					  const void *data, u8 len, u8 type)
>  {
>  	struct garp_attr *attr;
> +	int err;
>  
>  	attr = kmalloc(sizeof(*attr) + len, GFP_ATOMIC);
>  	if (!attr)
> -		return attr;
> +		return PTR_ERR(-ENOMEM);
>  	attr->state = GARP_APPLICANT_VO;
>  	attr->type  = type;
>  	attr->dlen  = len;
>  	memcpy(attr->data, data, len);
> -	garp_attr_insert(app, attr);
> +	err = garp_attr_insert(app, attr);
> +	if (err < 0) {
> +		kfree(attr);
> +		return PTR_ERR(err);
> +	}
>  	return attr;
>  }
>  
> @@ -353,9 +361,9 @@ int garp_request_join(const struct net_device *dev,
>  
>  	spin_lock_bh(&app->lock);
>  	attr = garp_attr_create(app, data, len, type);
> -	if (!attr) {
> +	if (IS_ERR(attr)) {
>  		spin_unlock_bh(&app->lock);
> -		return -ENOMEM;
> +		return ERR_PTR(attr);
>  	}
>  	garp_attr_event(app, attr, GARP_EVENT_REQ_JOIN);
>  	spin_unlock_bh(&app->lock);


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-PATCH-garp-fix-livelock-on-link-bouncing.patch --]
[-- Type: text/x-patch; name="0001-PATCH-garp-fix-livelock-on-link-bouncing.patch", Size: 5776 bytes --]

>From 9570dd1bd728399bf47722a9e966ace42d6441fa Mon Sep 17 00:00:00 2001
From: "Jorge Boncompte [DTI2]" <jorge@dti2.net>
Date: Mon, 26 Mar 2012 12:52:13 +0200
Subject: [PATCH] [PATCH] garp: fix livelock on link bouncing
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

    Fixes a bug in the GARP code when a VLAN interface changes from down
to up state quickly.

    The tree insertion routine doesn't check that the attr it's still on
the tree and enters and infinite loop (livelocks).

    Instead of returning an error we just reuse the existing attr to not
disturb the switch in front of us.

    Conversation with Patrick follows...

El 30/11/2011 14:02, Patrick McHardy escribió:
> On 11/29/2011 06:01 PM, Jorge Boncompte [DTI2] wrote:
>> El 29/11/2011 14:50, Patrick McHardy escribió:
>>> On 11/28/2011 04:39 PM, Jorge Boncompte [DTI2] wrote:
>>>> El 28/11/2011 16:11, Patrick McHardy escribió:
>>>>> On 25.11.2011 18:13, Jorge Boncompte [DTI2] wrote:
>>>>>>      Patrick, could you take a look at the attached patch? It fixes
>>>>>> a bug in the GARP code when you put a VLAN interface down/up in a
>>>>>> quick sucession. I've noticed it because some of my boxes where
>>>>>> hanging when moving interfaces between netns's.
>>>>> I'm not sure I understand the condition leading to this.
>>>>> Basically the GVRP join should only happen under the
>>>>> RTNL mutex, so I don't see how we can race during that.
>>>>      It has nothing to do with the RTNL, the problem is that
>>>> garp_request_leave() arms the timer and the leave packet it's not sent
>>>> inmediately and the attribute it's still on the tree when the new
>>>> garp_request_join() happens, for example moving a VLAN interface from
>>>> netns or by a quick down/up sucession.
>>>> garp_attr_insert() doesn't handle the condition that the attr exists
>>>> already and livelocks in the while loop.
>>>>
>>>>      So, instead of sending the leave packet synchronously from the
>>>> leave path I have opted for reinitializing the already existing
>>>> attribute that should not disturb the switch where we're attached.
>>> Yes, that seems like the better option. But I don't think this is fully
>>> correct (might be missing something though, it has been quite a while
>>> since I last worked on this code). If the applicant is in
>>> GARP_APPLICANT_LA state and a GARP_EVENT_TRANSMIT_PDU event is
>>> generated, the action will be GARP_ACTION_S_LEAVE_EMPTY, after which
>>> the attribute is destroyed immediately. Since we skip insertion with
>>> your patch, the attribute will incorrectly be gone. So I think what we
>>> need is either refcounting on the attribute to handle this case or
>>> alternatively send the message immediately.
>>>
>>> Does that make sense?
>>     What you describe happens from the timer, after garp_request_leave()
>> has moved the applicant to _LA state and _REQ_LEAVE event. That's what
>> happens in the normal case when you put the interface down for example.
>>
>>     In the case I describe the garp_request_join() comes betweeen the
>> call to garp_request_leave() and before the timer fires. The attr is
>> still on the tree so without my fix you livelock. With my changes
>> garp_request_join() calls garp_attr_create() that founds and return the
>> existing attr, and then calls garp_attr_event() with the _REQ_JOIN event.
>> This moves the attr from the _LA state to the _VA one. After that the
>> timer fires and founds the attr in the _VA state instead of _LA and so
>> the attr is not destroyed (and no need to refcount it).
>>
>>     All this happens under the app->lock so the timer sees the attr on
>> the _LA state or on the VA state and in either case it does the right
>> thing from my POV. I am no expert on GARP but I've researched a bit the
>> protocol and the state transition from _LA to _VA looks ok to me.
>>
>>     Here you have some output of a my patch with some test printk()'s
>> in case you can understand it better that my non-native english :)
>
> OK, this seems fine to me. If you change the WARN_ON to a
> WARN_ON(err) with err = garp_attr_insert(...) you can add
> my Acked-by: Patrick McHardy <kaber@trash.net>.
>
> Thanks!

Signed-off-by: Jorge Boncompte [DTI2] <jorge@dti2.net>
Acked-by: Patrick McHardy <kaber@trash.net>
---
 net/802/garp.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/net/802/garp.c b/net/802/garp.c
index 8e21b6d..ad72ac4 100644
--- a/net/802/garp.c
+++ b/net/802/garp.c
@@ -167,7 +167,7 @@ static struct garp_attr *garp_attr_lookup(const struct garp_applicant *app,
 	return NULL;
 }
 
-static void garp_attr_insert(struct garp_applicant *app, struct garp_attr *new)
+static int garp_attr_insert(struct garp_applicant *app, struct garp_attr *new)
 {
 	struct rb_node *parent = NULL, **p = &app->gid.rb_node;
 	struct garp_attr *attr;
@@ -181,15 +181,24 @@ static void garp_attr_insert(struct garp_applicant *app, struct garp_attr *new)
 			p = &parent->rb_left;
 		else if (d > 0)
 			p = &parent->rb_right;
+		else
+			return -EEXIST;
 	}
 	rb_link_node(&new->node, parent, p);
 	rb_insert_color(&new->node, &app->gid);
+
+	return 0;
 }
 
 static struct garp_attr *garp_attr_create(struct garp_applicant *app,
 					  const void *data, u8 len, u8 type)
 {
 	struct garp_attr *attr;
+	int err;
+
+	attr = garp_attr_lookup(app, data, len, type);
+	if (attr)
+		return attr;
 
 	attr = kmalloc(sizeof(*attr) + len, GFP_ATOMIC);
 	if (!attr)
@@ -198,7 +207,10 @@ static struct garp_attr *garp_attr_create(struct garp_applicant *app,
 	attr->type  = type;
 	attr->dlen  = len;
 	memcpy(attr->data, data, len);
-	garp_attr_insert(app, attr);
+	err = garp_attr_insert(app, attr);
+
+	WARN_ON(err);
+
 	return attr;
 }
 
-- 
1.7.8.3

Re: [PATCH] net/garp: avoid infinite loop if attribute already exists

[Ward, David - 0663 - MITLL]

[-- Attachment #1: Type: text/plain, Size: 3746 bytes --]

On 26/03/12 07:23, Jorge Boncompte [DTI2] wrote:
> El 26/03/2012 0:43, David Ward escribió:
>> An infinite loop occurred if garp_attr_create was called with the
>> values of an existing attribute. Return -EEXIST instead.
> 	I should have sent this some months ago but others things keep me from doing it.
> 	Anyway, I think that the right thing to do it's reuse the attribute to not
> disturb the switch/network. Also, returning an error it's pointless because
> nobody checks vlan_gvrp_request_join() return and you'll end up in a state where
> the VLAN device has the GVRP flag but it's not announcing the attribute.

I think what you are saying is that if we try to create an attribute 
that already exists, we should leave the old attribute alone.  I agree 
with that, and the patch I sent does that.  I also think the fact that 
the attribute existed would likely indicate a bug in the GARP 
application, in which we are not withdrawing an existing attribute when 
we should.  Your patch warns on this condition.

Our patches are mostly the same.  One thing I notice with your patch is 
that for new attributes, it now traverses the RB tree twice.  And it 
doesn't free the memory for a new attribute if it wasn't inserted into 
the RB tree.  So, what if instead I modified my patch to add 
"WARN_ON(err)" to either garp_request_join or vlan_gvrp_request_join?  
This would also warn us on -ENOMEM.

David

>
>   	Please take a look at the conversation I had with Patrick in the patch commit log.
>
> 	If you agree, I'm fine with you redoing your patch or David using mine if he
> thinks it's ok.
>
> 	Regards,
> 		Jorge
>
>> Signed-off-by: David Ward<david.ward@ll.mit.edu>
>> ---
>>   net/802/garp.c |   18 +++++++++++++-----
>>   1 files changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/802/garp.c b/net/802/garp.c
>> index 8e21b6d..bb5015e 100644
>> --- a/net/802/garp.c
>> +++ b/net/802/garp.c
>> @@ -167,7 +167,7 @@ static struct garp_attr *garp_attr_lookup(const struct garp_applicant *app,
>>   	return NULL;
>>   }
>>
>> -static void garp_attr_insert(struct garp_applicant *app, struct garp_attr *new)
>> +static int garp_attr_insert(struct garp_applicant *app, struct garp_attr *new)
>>   {
>>   	struct rb_node *parent = NULL, **p =&app->gid.rb_node;
>>   	struct garp_attr *attr;
>> @@ -181,24 +181,32 @@ static void garp_attr_insert(struct garp_applicant *app, struct garp_attr *new)
>>   			p =&parent->rb_left;
>>   		else if (d>  0)
>>   			p =&parent->rb_right;
>> +		else
>> +			return -EEXIST;
>>   	}
>>   	rb_link_node(&new->node, parent, p);
>>   	rb_insert_color(&new->node,&app->gid);
>> +	return 0;
>>   }
>>
>>   static struct garp_attr *garp_attr_create(struct garp_applicant *app,
>>   					  const void *data, u8 len, u8 type)
>>   {
>>   	struct garp_attr *attr;
>> +	int err;
>>
>>   	attr = kmalloc(sizeof(*attr) + len, GFP_ATOMIC);
>>   	if (!attr)
>> -		return attr;
>> +		return PTR_ERR(-ENOMEM);
>>   	attr->state = GARP_APPLICANT_VO;
>>   	attr->type  = type;
>>   	attr->dlen  = len;
>>   	memcpy(attr->data, data, len);
>> -	garp_attr_insert(app, attr);
>> +	err = garp_attr_insert(app, attr);
>> +	if (err<  0) {
>> +		kfree(attr);
>> +		return PTR_ERR(err);
>> +	}
>>   	return attr;
>>   }
>>
>> @@ -353,9 +361,9 @@ int garp_request_join(const struct net_device *dev,
>>
>>   	spin_lock_bh(&app->lock);
>>   	attr = garp_attr_create(app, data, len, type);
>> -	if (!attr) {
>> +	if (IS_ERR(attr)) {
>>   		spin_unlock_bh(&app->lock);
>> -		return -ENOMEM;
>> +		return ERR_PTR(attr);
>>   	}
>>   	garp_attr_event(app, attr, GARP_EVENT_REQ_JOIN);
>>   	spin_unlock_bh(&app->lock);


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4558 bytes --]

Re: [PATCH] net/garp: avoid infinite loop if attribute already exists

[Jorge Boncompte [DTI2]]

El 26/03/2012 16:11, Ward, David - 0663 - MITLL escribió:
> On 26/03/12 07:23, Jorge Boncompte [DTI2] wrote:
>> El 26/03/2012 0:43, David Ward escribió:
>>> An infinite loop occurred if garp_attr_create was called with the
>>> values of an existing attribute. Return -EEXIST instead.
>>     I should have sent this some months ago but others things keep me from
>> doing it.
>>     Anyway, I think that the right thing to do it's reuse the attribute to not
>> disturb the switch/network. Also, returning an error it's pointless because
>> nobody checks vlan_gvrp_request_join() return and you'll end up in a state where
>> the VLAN device has the GVRP flag but it's not announcing the attribute.
> 
> I think what you are saying is that if we try to create an attribute that
> already exists, we should leave the old attribute alone.  I agree with that, and
> the patch I sent does that.  I also think the fact that the attribute existed
> would likely indicate a bug in the GARP application, in which we are not
> withdrawing an existing attribute when we should.  Your patch warns on this
> condition.

	The attribute it's still on the tree because the leave path it's called
asynchronously from a timer. As far as I could see there's no other code path
that can put an attribute on the tree twice and that's why I put the WARN_ON().

> Our patches are mostly the same.  One thing I notice with your patch is that for
> new attributes, it now traverses the RB tree twice.  And it doesn't free the
> memory for a new attribute if it wasn't inserted into the RB tree.  So, what if
> instead I modified my patch to add "WARN_ON(err)" to either garp_request_join or
> vlan_gvrp_request_join?  This would also warn us on -ENOMEM.
> 
> David
> 
>>
>>       Please take a look at the conversation I had with Patrick in the patch
>> commit log.
>>
>>     If you agree, I'm fine with you redoing your patch or David using mine if he
>> thinks it's ok.
>>
>>     Regards,
>>         Jorge
>>
>>> Signed-off-by: David Ward<david.ward@ll.mit.edu>
>>> ---
>>>   net/802/garp.c |   18 +++++++++++++-----
>>>   1 files changed, 13 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/net/802/garp.c b/net/802/garp.c
>>> index 8e21b6d..bb5015e 100644
>>> --- a/net/802/garp.c
>>> +++ b/net/802/garp.c
>>> @@ -167,7 +167,7 @@ static struct garp_attr *garp_attr_lookup(const struct
>>> garp_applicant *app,
>>>       return NULL;
>>>   }
>>>
>>> -static void garp_attr_insert(struct garp_applicant *app, struct garp_attr *new)
>>> +static int garp_attr_insert(struct garp_applicant *app, struct garp_attr *new)
>>>   {
>>>       struct rb_node *parent = NULL, **p =&app->gid.rb_node;
>>>       struct garp_attr *attr;
>>> @@ -181,24 +181,32 @@ static void garp_attr_insert(struct garp_applicant
>>> *app, struct garp_attr *new)
>>>               p =&parent->rb_left;
>>>           else if (d>  0)
>>>               p =&parent->rb_right;
>>> +        else
>>> +            return -EEXIST;
>>>       }
>>>       rb_link_node(&new->node, parent, p);
>>>       rb_insert_color(&new->node,&app->gid);
>>> +    return 0;
>>>   }
>>>
>>>   static struct garp_attr *garp_attr_create(struct garp_applicant *app,
>>>                         const void *data, u8 len, u8 type)
>>>   {
>>>       struct garp_attr *attr;
>>> +    int err;
>>>
>>>       attr = kmalloc(sizeof(*attr) + len, GFP_ATOMIC);
>>>       if (!attr)
>>> -        return attr;
>>> +        return PTR_ERR(-ENOMEM);
>>>       attr->state = GARP_APPLICANT_VO;
>>>       attr->type  = type;
>>>       attr->dlen  = len;
>>>       memcpy(attr->data, data, len);
>>> -    garp_attr_insert(app, attr);
>>> +    err = garp_attr_insert(app, attr);
>>> +    if (err<  0) {
>>> +        kfree(attr);
>>> +        return PTR_ERR(err);
>>> +    }
>>>       return attr;
>>>   }
>>>
>>> @@ -353,9 +361,9 @@ int garp_request_join(const struct net_device *dev,
>>>
>>>       spin_lock_bh(&app->lock);
>>>       attr = garp_attr_create(app, data, len, type);
>>> -    if (!attr) {
>>> +    if (IS_ERR(attr)) {
>>>           spin_unlock_bh(&app->lock);
>>> -        return -ENOMEM;
>>> +        return ERR_PTR(attr);
>>>       }
>>>       garp_attr_event(app, attr, GARP_EVENT_REQ_JOIN);
>>>       spin_unlock_bh(&app->lock);
> 

Re: [PATCH] net/garp: avoid infinite loop if attribute already exists

[Ward, David - 0663 - MITLL]

[-- Attachment #1: Type: text/plain, Size: 4958 bytes --]

On 26/03/12 11:26, Jorge Boncompte [DTI2] wrote:
> El 26/03/2012 16:11, Ward, David - 0663 - MITLL escribió:
>> On 26/03/12 07:23, Jorge Boncompte [DTI2] wrote:
>>> El 26/03/2012 0:43, David Ward escribió:
>>>> An infinite loop occurred if garp_attr_create was called with the
>>>> values of an existing attribute. Return -EEXIST instead.
>>>      I should have sent this some months ago but others things keep me from
>>> doing it.
>>>      Anyway, I think that the right thing to do it's reuse the attribute to not
>>> disturb the switch/network. Also, returning an error it's pointless because
>>> nobody checks vlan_gvrp_request_join() return and you'll end up in a state where
>>> the VLAN device has the GVRP flag but it's not announcing the attribute.
>> I think what you are saying is that if we try to create an attribute that
>> already exists, we should leave the old attribute alone.  I agree with that, and
>> the patch I sent does that.  I also think the fact that the attribute existed
>> would likely indicate a bug in the GARP application, in which we are not
>> withdrawing an existing attribute when we should.  Your patch warns on this
>> condition.
> 	The attribute it's still on the tree because the leave path it's called
> asynchronously from a timer. As far as I could see there's no other code path
> that can put an attribute on the tree twice and that's why I put the WARN_ON().

Oh right...  so in that case, garp_attr_create should change the state 
of the attribute from LA back to VA.  I think it should only trigger a 
warning if the current state of the attribute was not LA?

I'll update my patch and resend.

>
>> Our patches are mostly the same.  One thing I notice with your patch is that for
>> new attributes, it now traverses the RB tree twice.  And it doesn't free the
>> memory for a new attribute if it wasn't inserted into the RB tree.  So, what if
>> instead I modified my patch to add "WARN_ON(err)" to either garp_request_join or
>> vlan_gvrp_request_join?  This would also warn us on -ENOMEM.
>>
>> David
>>
>>>        Please take a look at the conversation I had with Patrick in the patch
>>> commit log.
>>>
>>>      If you agree, I'm fine with you redoing your patch or David using mine if he
>>> thinks it's ok.
>>>
>>>      Regards,
>>>          Jorge
>>>
>>>> Signed-off-by: David Ward<david.ward@ll.mit.edu>
>>>> ---
>>>>    net/802/garp.c |   18 +++++++++++++-----
>>>>    1 files changed, 13 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/net/802/garp.c b/net/802/garp.c
>>>> index 8e21b6d..bb5015e 100644
>>>> --- a/net/802/garp.c
>>>> +++ b/net/802/garp.c
>>>> @@ -167,7 +167,7 @@ static struct garp_attr *garp_attr_lookup(const struct
>>>> garp_applicant *app,
>>>>        return NULL;
>>>>    }
>>>>
>>>> -static void garp_attr_insert(struct garp_applicant *app, struct garp_attr *new)
>>>> +static int garp_attr_insert(struct garp_applicant *app, struct garp_attr *new)
>>>>    {
>>>>        struct rb_node *parent = NULL, **p =&app->gid.rb_node;
>>>>        struct garp_attr *attr;
>>>> @@ -181,24 +181,32 @@ static void garp_attr_insert(struct garp_applicant
>>>> *app, struct garp_attr *new)
>>>>                p =&parent->rb_left;
>>>>            else if (d>   0)
>>>>                p =&parent->rb_right;
>>>> +        else
>>>> +            return -EEXIST;
>>>>        }
>>>>        rb_link_node(&new->node, parent, p);
>>>>        rb_insert_color(&new->node,&app->gid);
>>>> +    return 0;
>>>>    }
>>>>
>>>>    static struct garp_attr *garp_attr_create(struct garp_applicant *app,
>>>>                          const void *data, u8 len, u8 type)
>>>>    {
>>>>        struct garp_attr *attr;
>>>> +    int err;
>>>>
>>>>        attr = kmalloc(sizeof(*attr) + len, GFP_ATOMIC);
>>>>        if (!attr)
>>>> -        return attr;
>>>> +        return PTR_ERR(-ENOMEM);
>>>>        attr->state = GARP_APPLICANT_VO;
>>>>        attr->type  = type;
>>>>        attr->dlen  = len;
>>>>        memcpy(attr->data, data, len);
>>>> -    garp_attr_insert(app, attr);
>>>> +    err = garp_attr_insert(app, attr);
>>>> +    if (err<   0) {
>>>> +        kfree(attr);
>>>> +        return PTR_ERR(err);
>>>> +    }
>>>>        return attr;
>>>>    }
>>>>
>>>> @@ -353,9 +361,9 @@ int garp_request_join(const struct net_device *dev,
>>>>
>>>>        spin_lock_bh(&app->lock);
>>>>        attr = garp_attr_create(app, data, len, type);
>>>> -    if (!attr) {
>>>> +    if (IS_ERR(attr)) {
>>>>            spin_unlock_bh(&app->lock);
>>>> -        return -ENOMEM;
>>>> +        return ERR_PTR(attr);
>>>>        }
>>>>        garp_attr_event(app, attr, GARP_EVENT_REQ_JOIN);
>>>>        spin_unlock_bh(&app->lock);
>


-- 
David Ward, Associate Staff
Wideband Tactical Networking Group
MIT Lincoln Laboratory
Office: 781-981-4266
Mobile: 781-999-1925
Fax: 781-981-4583



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4558 bytes --]

[PATCH v2] net/garp: avoid infinite loop if attribute already exists

[David Ward]

An infinite loop occurred if garp_attr_create was called with the values
of an existing attribute. This might happen if a previous leave request
for the attribute has not yet been followed by a PDU transmission (or,
if the application previously issued a join request for the attribute
and is now issuing another one, without having issued a leave request).

If garp_attr_create finds an existing attribute having the same values,
return the address to it. Its state will then get updated (i.e., if it
was in a leaving state, it will move into a non-leaving state and not
get deleted during the next PDU transmission).

To accomplish this fix, collapse garp_attr_insert into garp_attr_create
(which is its only caller).

Signed-off-by: David Ward <david.ward@ll.mit.edu>
---
 net/802/garp.c |   22 ++++++++++------------
 1 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/net/802/garp.c b/net/802/garp.c
index 8e21b6d..a5c2248 100644
--- a/net/802/garp.c
+++ b/net/802/garp.c
@@ -167,7 +167,8 @@ static struct garp_attr *garp_attr_lookup(const struct garp_applicant *app,
 	return NULL;
 }
 
-static void garp_attr_insert(struct garp_applicant *app, struct garp_attr *new)
+static struct garp_attr *garp_attr_create(struct garp_applicant *app,
+					  const void *data, u8 len, u8 type)
 {
 	struct rb_node *parent = NULL, **p = &app->gid.rb_node;
 	struct garp_attr *attr;
@@ -176,21 +177,16 @@ static void garp_attr_insert(struct garp_applicant *app, struct garp_attr *new)
 	while (*p) {
 		parent = *p;
 		attr = rb_entry(parent, struct garp_attr, node);
-		d = garp_attr_cmp(attr, new->data, new->dlen, new->type);
+		d = garp_attr_cmp(attr, data, len, type);
 		if (d < 0)
 			p = &parent->rb_left;
 		else if (d > 0)
 			p = &parent->rb_right;
+		else {
+			/* The attribute already exists; re-use it. */
+			return attr;
+		}
 	}
-	rb_link_node(&new->node, parent, p);
-	rb_insert_color(&new->node, &app->gid);
-}
-
-static struct garp_attr *garp_attr_create(struct garp_applicant *app,
-					  const void *data, u8 len, u8 type)
-{
-	struct garp_attr *attr;
-
 	attr = kmalloc(sizeof(*attr) + len, GFP_ATOMIC);
 	if (!attr)
 		return attr;
@@ -198,7 +194,9 @@ static struct garp_attr *garp_attr_create(struct garp_applicant *app,
 	attr->type  = type;
 	attr->dlen  = len;
 	memcpy(attr->data, data, len);
-	garp_attr_insert(app, attr);
+
+	rb_link_node(&attr->node, parent, p);
+	rb_insert_color(&attr->node, &app->gid);
 	return attr;
 }
 
-- 
1.7.4.1

Re: [PATCH v2] net/garp: avoid infinite loop if attribute already exists

[Jorge Boncompte [DTI2]]

El 27/03/2012 21:01, David Ward escribió:
> An infinite loop occurred if garp_attr_create was called with the values
> of an existing attribute. This might happen if a previous leave request
> for the attribute has not yet been followed by a PDU transmission (or,
> if the application previously issued a join request for the attribute
> and is now issuing another one, without having issued a leave request).
> 
> If garp_attr_create finds an existing attribute having the same values,
> return the address to it. Its state will then get updated (i.e., if it
> was in a leaving state, it will move into a non-leaving state and not
> get deleted during the next PDU transmission).
> 
> To accomplish this fix, collapse garp_attr_insert into garp_attr_create
> (which is its only caller).
> 

	I wouldn't mind if you credited me given that your patch now is based on my fix
and analysis. ;-)

Anyway...

Acked-by: Jorge Boncompte [DTI2] <jorge@dti2.net>

Re: [PATCH v2] net/garp: avoid infinite loop if attribute already exists

[Ward, David - 0663 - MITLL]

[-- Attachment #1: Type: text/plain, Size: 1166 bytes --]

On 28/03/12 07:28, Jorge Boncompte [DTI2] wrote:
> El 27/03/2012 21:01, David Ward escribió:
>> An infinite loop occurred if garp_attr_create was called with the values
>> of an existing attribute. This might happen if a previous leave request
>> for the attribute has not yet been followed by a PDU transmission (or,
>> if the application previously issued a join request for the attribute
>> and is now issuing another one, without having issued a leave request).
>>
>> If garp_attr_create finds an existing attribute having the same values,
>> return the address to it. Its state will then get updated (i.e., if it
>> was in a leaving state, it will move into a non-leaving state and not
>> get deleted during the next PDU transmission).
>>
>> To accomplish this fix, collapse garp_attr_insert into garp_attr_create
>> (which is its only caller).
>>
> 	I wouldn't mind if you credited me given that your patch now is based on my fix
> and analysis. ;-)

Please add to commit message:

Thanks to Jorge Boncompte [DTI2] <jorge@dti2.net> for contributing to
this fix.

> Anyway...
>
> Acked-by: Jorge Boncompte [DTI2]<jorge@dti2.net>
>


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4558 bytes --]

Re: [PATCH v2] net/garp: avoid infinite loop if attribute already exists

[David Miller]

From: "Ward, David - 0663 - MITLL" <david.ward@ll.mit.edu>
Date: Wed, 28 Mar 2012 09:28:50 -0400

> Please add to commit message:
> 
> Thanks to Jorge Boncompte [DTI2] <jorge@dti2.net> for contributing to
> this fix.

It's already committed to my tree and therefore in the permanent
record and therefore the message cannot be changed.

Be more careful in the future.

Re: [PATCH] net/garp: avoid infinite loop if attribute already exists

[David Miller]

From: David Ward <david.ward@ll.mit.edu>
Date: Sun, 25 Mar 2012 18:43:56 -0400

> @@ -353,9 +361,9 @@ int garp_request_join(const struct net_device *dev,
>  
>  	spin_lock_bh(&app->lock);
>  	attr = garp_attr_create(app, data, len, type);
> -	if (!attr) {
> +	if (IS_ERR(attr)) {
>  		spin_unlock_bh(&app->lock);
> -		return -ENOMEM;
> +		return ERR_PTR(attr);

You cannot tell me that this ERR_PTR() thing didn't emit a very loud
warning from the compiler.  You want PTR_ERR() instead.

Please don't be so careless, if I can predict compiler warnings just
by reading your patches you are so doing it wrong.