[PATCH net-next 1/8] af_packet: Check return of dev_set_promiscuity/allmulti

[PATCH net-next 1/8] af_packet: Check return of dev_set_promiscuity/allmulti

[Wang Chen]

dev_set_promiscuity/allmulti might overflow.
Commit: "netdevice: Fix promiscuity and allmulti overflow" in net-next makes
dev_set_promiscuity/allmulti return error number if overflow happened.

In af_packet, we check all positive increment for promiscuity and allmulti
to get error return.

Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
---
 net/packet/af_packet.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 2cee87d..c6a1e36 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1175,7 +1175,8 @@ static int packet_getname(struct socket *sock, struct sockaddr *uaddr,
 	return 0;
 }
 
-static void packet_dev_mc(struct net_device *dev, struct packet_mclist *i, int what)
+static int packet_dev_mc(struct net_device *dev, struct packet_mclist *i,
+			 int what)
 {
 	switch (i->type) {
 	case PACKET_MR_MULTICAST:
@@ -1185,13 +1186,14 @@ static void packet_dev_mc(struct net_device *dev, struct packet_mclist *i, int w
 			dev_mc_delete(dev, i->addr, i->alen, 0);
 		break;
 	case PACKET_MR_PROMISC:
-		dev_set_promiscuity(dev, what);
+		return dev_set_promiscuity(dev, what);
 		break;
 	case PACKET_MR_ALLMULTI:
-		dev_set_allmulti(dev, what);
+		return dev_set_allmulti(dev, what);
 		break;
 	default:;
 	}
+	return 0;
 }
 
 static void packet_dev_mclist(struct net_device *dev, struct packet_mclist *i, int what)
@@ -1245,7 +1247,8 @@ static int packet_mc_add(struct sock *sk, struct packet_mreq_max *mreq)
 	i->count = 1;
 	i->next = po->mclist;
 	po->mclist = i;
-	packet_dev_mc(dev, i, +1);
+	/* Positive increment should be checked for overflow --WCN */
+	err = packet_dev_mc(dev, i, 1);
 
 done:
 	rtnl_unlock();
-- 
1.5.3.4

Re: [PATCH net-next 1/8] af_packet: Check return of dev_set_promiscuity/allmulti

[David Miller]

From: Wang Chen <wangchen@cn.fujitsu.com>
Date: Fri, 20 Jun 2008 08:54:32 +0800

> @@ -1245,7 +1247,8 @@ static int packet_mc_add(struct sock *sk, struct packet_mreq_max *mreq)
>  	i->count = 1;
>  	i->next = po->mclist;
>  	po->mclist = i;
> -	packet_dev_mc(dev, i, +1);
> +	/* Positive increment should be checked for overflow --WCN */
> +	err = packet_dev_mc(dev, i, 1);
>  

Please don't add these little signatures to comments.  That might have
been useful to do 10 years ago when we didn't use proper source
control, but now we do and anyone interested can do a "git blame"
to see who added that comment and why.

Also, this comment doesn't really add any information.  We check
error return values simply because errors can happen, that's just
a straight fact.  If packet_dev_mc() and it's sub calls can error
for other reasons this comment is only telling part of the story
and as a result becomes inaccurate.

Therefore, I'd like to ask that you not add this comment, it doesn't
really help anything.  This kind of information can go into the
commit log message.  That's where "why" information tends to belong.

Re: [PATCH net-next 1/8] af_packet: Check return of dev_set_promiscuity/allmulti

[Wang Chen]

David Miller said the following on 2008-6-20 10:05:
> From: Wang Chen <wangchen@cn.fujitsu.com>
> Date: Fri, 20 Jun 2008 08:54:32 +0800
> 
>> @@ -1245,7 +1247,8 @@ static int packet_mc_add(struct sock *sk, struct packet_mreq_max *mreq)
>>  	i->count = 1;
>>  	i->next = po->mclist;
>>  	po->mclist = i;
>> -	packet_dev_mc(dev, i, +1);
>> +	/* Positive increment should be checked for overflow --WCN */
>> +	err = packet_dev_mc(dev, i, 1);
>>  
> 
> Please don't add these little signatures to comments.  That might have
> been useful to do 10 years ago when we didn't use proper source
> control, but now we do and anyone interested can do a "git blame"
> to see who added that comment and why.
> 
> Also, this comment doesn't really add any information.  We check
> error return values simply because errors can happen, that's just
> a straight fact.  If packet_dev_mc() and it's sub calls can error
> for other reasons this comment is only telling part of the story
> and as a result becomes inaccurate.
> 
> Therefore, I'd like to ask that you not add this comment, it doesn't
> really help anything.  This kind of information can go into the
> commit log message.  That's where "why" information tends to belong.
> 

Roger.
I will remove all useless comment in my patch series.
Thanks David.
Don't blame me in every patch :)

Re: [PATCH net-next 1/8] af_packet: Check return of dev_set_promiscuity/allmulti

[Wang Chen]

Wang Chen said the following on 2008-6-20 10:13:
> David Miller said the following on 2008-6-20 10:05:
>> From: Wang Chen <wangchen@cn.fujitsu.com>
>> Date: Fri, 20 Jun 2008 08:54:32 +0800
>>
>>> @@ -1245,7 +1247,8 @@ static int packet_mc_add(struct sock *sk, struct packet_mreq_max *mreq)
>>>  	i->count = 1;
>>>  	i->next = po->mclist;
>>>  	po->mclist = i;
>>> -	packet_dev_mc(dev, i, +1);
>>> +	/* Positive increment should be checked for overflow --WCN */
>>> +	err = packet_dev_mc(dev, i, 1);
>>>  
>> Please don't add these little signatures to comments.  That might have
>> been useful to do 10 years ago when we didn't use proper source
>> control, but now we do and anyone interested can do a "git blame"
>> to see who added that comment and why.
>>
>> Also, this comment doesn't really add any information.  We check
>> error return values simply because errors can happen, that's just
>> a straight fact.  If packet_dev_mc() and it's sub calls can error
>> for other reasons this comment is only telling part of the story
>> and as a result becomes inaccurate.
>>
>> Therefore, I'd like to ask that you not add this comment, it doesn't
>> really help anything.  This kind of information can go into the
>> commit log message.  That's where "why" information tends to belong.
>>

No useless comment :)

---
dev_set_promiscuity/allmulti might overflow.
Commit: "netdevice: Fix promiscuity and allmulti overflow" in net-next makes
dev_set_promiscuity/allmulti return error number if overflow happened.

In af_packet, we check all positive increment for promiscuity and allmulti
to get error return.

Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
---
 net/packet/af_packet.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 2cee87d..6370b9d 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1175,7 +1175,8 @@ static int packet_getname(struct socket *sock, struct sockaddr *uaddr,
 	return 0;
 }
 
-static void packet_dev_mc(struct net_device *dev, struct packet_mclist *i, int what)
+static int packet_dev_mc(struct net_device *dev, struct packet_mclist *i,
+			 int what)
 {
 	switch (i->type) {
 	case PACKET_MR_MULTICAST:
@@ -1185,13 +1186,14 @@ static void packet_dev_mc(struct net_device *dev, struct packet_mclist *i, int w
 			dev_mc_delete(dev, i->addr, i->alen, 0);
 		break;
 	case PACKET_MR_PROMISC:
-		dev_set_promiscuity(dev, what);
+		return dev_set_promiscuity(dev, what);
 		break;
 	case PACKET_MR_ALLMULTI:
-		dev_set_allmulti(dev, what);
+		return dev_set_allmulti(dev, what);
 		break;
 	default:;
 	}
+	return 0;
 }
 
 static void packet_dev_mclist(struct net_device *dev, struct packet_mclist *i, int what)
@@ -1245,7 +1247,7 @@ static int packet_mc_add(struct sock *sk, struct packet_mreq_max *mreq)
 	i->count = 1;
 	i->next = po->mclist;
 	po->mclist = i;
-	packet_dev_mc(dev, i, +1);
+	err = packet_dev_mc(dev, i, 1);
 
 done:
 	rtnl_unlock();
-- 
1.5.3.4