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: <4F7051B6.1030205@dti2.net>
Date:	Mon, 26 Mar 2012 13:23:34 +0200
From:	"Jorge Boncompte [DTI2]" <jorge@...2.net>
To:	david.ward@...mit.edu
CC:	netdev@...r.kernel.org
Subject: Re: [PATCH] net/garp: avoid infinite loop if attribute already exists

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


View attachment "0001-PATCH-garp-fix-livelock-on-link-bouncing.patch" of type "text/x-patch" (5777 bytes)