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