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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4F708A9A.8040705@dti2.net>
Date:	Mon, 26 Mar 2012 17:26:18 +0200
From:	"Jorge Boncompte [DTI2]" <jorge@...2.net>
To:	david.ward@...mit.edu
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH] net/garp: avoid infinite loop if attribute already exists

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@...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);
> 


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html