| Message ID | CAPgLHd9KNN81cPYv3bGzoBwUtnJSAoX4NN0Fh+wNgoHE_t7d=w@mail.gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Wei Yongjun <weiyj.lk@gmail.com> Date: Mon, 26 Dec 2011 17:02:59 +0800 > From: Wei Yongjun <yongjun_wei@trendmicro.com.cn> > > If bind is fail such as bind is called after set PACKET_FANOUT > sock option, the dev refcnt will leak. > > Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn> The device pointer is stored in ->prot_hook.dev Therefore, it will be released at the latest in packet_release() or earlier if another bind() attempt is done on the socket. There is no leak here as far as I can see. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
cc netdev@vger.kernel.org > From: Wei Yongjun <weiyj.lk@gmail.com> > Date: Mon, 26 Dec 2011 17:02:59 +0800 > >> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn> >> >> If bind is fail such as bind is called after set PACKET_FANOUT >> sock option, the dev refcnt will leak. >> >> Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn> > > The device pointer is stored in ->prot_hook.dev > > Therefore, it will be released at the latest in packet_release() or > earlier if another bind() attempt is done on the socket. > > There is no leak here as far as I can see. Hi David, In packet_do_bind(), if po->fanout is set, the device pointer may not be stored in ->prot_hook.dev, see the following code: static int packet_do_bind(struct sock *sk, struct net_device *dev, ...) { struct packet_sock *po = pkt_sk(sk); if (po->fanout) return -EINVAL; ... po->prot_hook.dev = dev; ... } The leak is exists only if po->fanout is set. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Wei Yongjun <weiyj.lk@gmail.com> Date: Tue, 27 Dec 2011 12:28:31 +0800 > cc netdev@vger.kernel.org > >> From: Wei Yongjun <weiyj.lk@gmail.com> >> Date: Mon, 26 Dec 2011 17:02:59 +0800 >> >>> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn> >>> >>> If bind is fail such as bind is called after set PACKET_FANOUT >>> sock option, the dev refcnt will leak. >>> >>> Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn> >> >> The device pointer is stored in ->prot_hook.dev >> >> Therefore, it will be released at the latest in packet_release() or >> earlier if another bind() attempt is done on the socket. >> >> There is no leak here as far as I can see. > > Hi David, > > In packet_do_bind(), if po->fanout is set, the device pointer may not be > stored in ->prot_hook.dev, see the following code: > > static int packet_do_bind(struct sock *sk, struct net_device *dev, ...) > { > struct packet_sock *po = pkt_sk(sk); > > if (po->fanout) > return -EINVAL; > ... > po->prot_hook.dev = dev; > ... > } > > The leak is exists only if po->fanout is set. Then it sounds a lot simpler for packet_do_bind() (one location) to manage this reference drop, rather than every single call site of packet_do_bind(). Look how complicated you had to make the tests in those call sites to get the logic right, it's must easier to just add the dev_put() right at that po->fanout test. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index 3891702..b6ac234 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -2501,8 +2501,11 @@ static int packet_bind_spkt(struct socket *sock, struct sockaddr *uaddr, strlcpy(name, uaddr->sa_data, sizeof(name)); dev = dev_get_by_name(sock_net(sk), name); - if (dev) + if (dev) { err = packet_do_bind(sk, dev, pkt_sk(sk)->num); + if (err) + dev_put(dev); + } return err; } @@ -2530,6 +2533,8 @@ static int packet_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len goto out; } err = packet_do_bind(sk, dev, sll->sll_protocol ? : pkt_sk(sk)->num); + if (err && dev) + dev_put(dev); out: return err;