Skip to contentSkip to conversation

Netdevice reference leak in af_ax25.c ??

DISCUSSION Ben Greear 3 participants 5 messages Sep 1, 2005

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Ben Greear

author
#0 lore raw

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

Signature · 2 lines
Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com
#1 lore raw
Quoted 7 lines · 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.

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.

author
#2 lore raw
Quoted 7 lines · Patrick McHardy
Quoted 5 lines · 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.

Quoted 6 lines
Quoted 1 line

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;
Signature · 2 lines
Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com
#3 lore raw
In reply to Patrick McHardy
Quoted 10 lines · Patrick McHardy
Quoted 6 lines

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

author
#4 lore raw
Quoted 15 lines · Ralf Baechle
Quoted 10 lines · Patrick McHardy
Quoted 6 lines

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.

Quoted 4 lines

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

Quoted 4 lines

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

Signature · 2 lines
Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com