af_packet: Check return of dev_set_promiscuity/allmulti

PATCH In review Wang Chen 2 participants 4 messages Jun 20, 2008

Subsystems (3)

Networking Network drivers Packet sockets

lore

Share & tools

lore

Wang Chen

author

2008-06-20 00:58 UTC #0 lore raw

PATCH 1/8 af_packet: Check return of dev_set_promiscuity/allmulti

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

---
 net/packet/af_packet.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

net/packet/af_packet.c +7 -4

index 2cee87d..c6a1e36 100644
@@ -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();

Signature · 1 line

1.5.3.4

David Miller

maintainer

2008-06-20 02:05 UTC #1 lore raw

From: Wang Chen <[email protected]>
Date: Fri, 20 Jun 2008 08:54:32 +0800

net/packet/af_packet.c static int packet_mc_add(struct sock *sk, struct packet_mreq_max *mreq) · line 1252

Quoted 8 lines

net/packet/af_packet.c +2 -1

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

Wang Chen

author

2008-06-20 02:19 UTC #2 lore raw

David Miller said the following on 2008-6-20 10:05:

Quoted 23 lines

From: Wang Chen <[email protected]>
Date: Fri, 20 Jun 2008 08:54:32 +0800
net/packet/af_packet.c static int packet_mc_add(struct sock *sk, struct packet_mreq_max *mreq) · line 1252
Quoted 8 lines
net/packet/af_packet.c +2 -1
@@ -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.

Quoted 4 lines

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

Wang Chen

author

2008-06-20 02:45 UTC #3 lore raw

Wang Chen said the following on 2008-6-20 10:13:

Quoted 27 lines

David Miller said the following on 2008-6-20 10:05:
Quoted 26 lines
From: Wang Chen <[email protected]>
Date: Fri, 20 Jun 2008 08:54:32 +0800
Review static int packet_mc_add(struct sock *sk, struct packet_mreq_max *mreq) · line 1252
Quoted 8 lines
(quoted diff) +2 -1
@@ -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 <[email protected]>

---
 net/packet/af_packet.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

net/packet/af_packet.c +6 -4

index 2cee87d..6370b9d 100644
@@ -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();

Signature · 1 line

1.5.3.4