* [PATCH net-next 1/8] af_packet: Check return of dev_set_promiscuity/allmulti @ 2008-06-20 0:54 Wang Chen 2008-06-20 2:05 ` David Miller 0 siblings, 1 reply; 4+ messages in thread From: Wang Chen @ 2008-06-20 0:54 UTC (permalink / raw) To: David S. Miller; +Cc: NETDEV, Patrick McHardy 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net-next 1/8] af_packet: Check return of dev_set_promiscuity/allmulti 2008-06-20 0:54 [PATCH net-next 1/8] af_packet: Check return of dev_set_promiscuity/allmulti Wang Chen @ 2008-06-20 2:05 ` David Miller 2008-06-20 2:13 ` Wang Chen 0 siblings, 1 reply; 4+ messages in thread From: David Miller @ 2008-06-20 2:05 UTC (permalink / raw) To: wangchen; +Cc: netdev, kaber 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. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net-next 1/8] af_packet: Check return of dev_set_promiscuity/allmulti 2008-06-20 2:05 ` David Miller @ 2008-06-20 2:13 ` Wang Chen 2008-06-20 2:41 ` Wang Chen 0 siblings, 1 reply; 4+ messages in thread From: Wang Chen @ 2008-06-20 2:13 UTC (permalink / raw) To: David Miller; +Cc: netdev, kaber 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 :) ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net-next 1/8] af_packet: Check return of dev_set_promiscuity/allmulti 2008-06-20 2:13 ` Wang Chen @ 2008-06-20 2:41 ` Wang Chen 0 siblings, 0 replies; 4+ messages in thread From: Wang Chen @ 2008-06-20 2:41 UTC (permalink / raw) To: David Miller; +Cc: netdev, kaber 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2008-06-20 2:45 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-06-20 0:54 [PATCH net-next 1/8] af_packet: Check return of dev_set_promiscuity/allmulti Wang Chen 2008-06-20 2:05 ` David Miller 2008-06-20 2:13 ` Wang Chen 2008-06-20 2:41 ` Wang Chen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).