Netdevice reference leak in af_ax25.c ??

Netdevice reference leak in af_ax25.c ??

[Ben Greear]

I believe the SO_BINDTODEVICE case in net/ax25/af_x25.c  (line 613 or so)
leaks a reference to a net device.  It does a dev_get_by_name,
which holds a reference, but since it never assigns the pointer
anywhere, I do not see how it can ever free it later.

Please clue me in as to where it's released if it actually is.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Netdevice reference leak in af_ax25.c ??

[Patrick McHardy]

Ben Greear wrote:
> 
> I believe the SO_BINDTODEVICE case in net/ax25/af_x25.c  (line 613 or so)
> leaks a reference to a net device.  It does a dev_get_by_name,
> which holds a reference, but since it never assigns the pointer
> anywhere, I do not see how it can ever free it later.
> 
> Please clue me in as to where it's released if it actually is.

I can't find the code you're talking about, there's no dev_get* in my
version of af_x25.c. Please paste the code you're talking about in
your bugreports, thanks.

Re: Netdevice reference leak in af_ax25.c ??

[Ben Greear]

Patrick McHardy wrote:
> Ben Greear wrote:
> 
>>I believe the SO_BINDTODEVICE case in net/ax25/af_x25.c  (line 613 or so)
>>leaks a reference to a net device.  It does a dev_get_by_name,
>>which holds a reference, but since it never assigns the pointer
>>anywhere, I do not see how it can ever free it later.
>>
>>Please clue me in as to where it's released if it actually is.
> 
> 
> I can't find the code you're talking about, there's no dev_get* in my
> version of af_x25.c. Please paste the code you're talking about in
> your bugreports, thanks.

Please ignore the NRDK thing..I am adding reference counting debugging
to the netdevice code.  This is from the 2.6.13 kernel:

In this method:

/*
  *	Handling for system calls applied via the various interfaces to an
  *	AX25 socket object
  */

static int ax25_setsockopt(struct socket *sock, int level, int optname,
	char __user *optval, int optlen)
{

.....

	case SO_BINDTODEVICE:
		if (optlen > IFNAMSIZ)
			optlen=IFNAMSIZ;
		if (copy_from_user(devname, optval, optlen)) {
		res = -EFAULT;
			break;
		}

		dev = dev_get_by_name(devname, NDRK_GENERIC);
		if (dev == NULL) {
			res = -ENODEV;
			break;
		}

		if (sk->sk_type == SOCK_SEQPACKET &&
		   (sock->state != SS_UNCONNECTED ||
		    sk->sk_state == TCP_LISTEN)) {
			res = -EADDRNOTAVAIL;
			dev_put(dev, NDRK_GENERIC);
			break;
		}

		ax25->ax25_dev = ax25_dev_ax25dev(dev);
		ax25_fillin_cb(ax25, ax25->ax25_dev);
                 dev_put(dev, NDRK_GENERIC); /* TODO:  Verify we should put it here. */
		break;

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Netdevice reference leak in af_ax25.c ??

[Ralf Baechle]

On Thu, Sep 01, 2005 at 08:56:19PM +0200, Patrick McHardy wrote:

> > I believe the SO_BINDTODEVICE case in net/ax25/af_x25.c  (line 613 or so)
> > leaks a reference to a net device.  It does a dev_get_by_name,
> > which holds a reference, but since it never assigns the pointer
> > anywhere, I do not see how it can ever free it later.
> > 
> > Please clue me in as to where it's released if it actually is.
> 
> I can't find the code you're talking about, there's no dev_get* in my
> version of af_x25.c. Please paste the code you're talking about in
> your bugreports, thanks.

Ben meant net/ax25/af_ax25.  The dev value is stored in the ax25_cb
indirectly after converting it to an ax25dev pointer and will be freed
what that ax25_cb (which really is the protocol-specific part of the
socket) is going to be closed.

You poked my nose at a bug though - it is possible to leak references by
performing multiple SO_BINDTODEVICE operations; we should either only
permit the first one to succeed or to drop the reference of the old
device in case of a repeated SO_BINDTODEVICE.  After the weekend ...

  Ralf

Re: Netdevice reference leak in af_ax25.c ??

[Ben Greear]

Ralf Baechle wrote:
> On Thu, Sep 01, 2005 at 08:56:19PM +0200, Patrick McHardy wrote:
> 
> 
>>>I believe the SO_BINDTODEVICE case in net/ax25/af_x25.c  (line 613 or so)
>>>leaks a reference to a net device.  It does a dev_get_by_name,
>>>which holds a reference, but since it never assigns the pointer
>>>anywhere, I do not see how it can ever free it later.
>>>
>>>Please clue me in as to where it's released if it actually is.
>>
>>I can't find the code you're talking about, there's no dev_get* in my
>>version of af_x25.c. Please paste the code you're talking about in
>>your bugreports, thanks.
> 
> 
> Ben meant net/ax25/af_ax25.  The dev value is stored in the ax25_cb
> indirectly after converting it to an ax25dev pointer and will be freed
> what that ax25_cb (which really is the protocol-specific part of the
> socket) is going to be closed.

Ok, I'm getting hopelessly lost in the ax25 code trying to follow
references, so I'm just going to use the generic ref counting debugging.

That will still point to the right module, but not the line of code,
should a leak occur (and should the patch be accepted) :)

> You poked my nose at a bug though - it is possible to leak references by
> performing multiple SO_BINDTODEVICE operations; we should either only
> permit the first one to succeed or to drop the reference of the old
> device in case of a repeated SO_BINDTODEVICE.  After the weekend ...

Thanks for taking a look.

Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com