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

["Ward, David - 0663 - MITLL" <david.ward@ll.mit.edu>]

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