[PATCH] packet: fix dev refcnt leak when bind fail

[PATCH] packet: fix dev refcnt leak when bind fail

[Wei Yongjun]

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>
---
 net/packet/af_packet.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

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;
-- 
1.7.7

Re: [PATCH] packet: fix dev refcnt leak when bind fail

[David Miller]

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.

Fwd: [PATCH] packet: fix dev refcnt leak when bind fail

[Wei Yongjun]

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.

Re: [PATCH] packet: fix dev refcnt leak when bind fail

[David Miller]

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.