SO_BINDTODEVICE mismatch with man page & comments.

According to the comment in the net/core/sock.c code (in 2.6.20), I should be able to pass a zero optlen to the setsockopt method for SO_BINDTODEVICE:

                case SO_BINDTODEVICE:
                {
                        char devname[IFNAMSIZ];

                        /* Sorry... */
                        if (!capable(CAP_NET_RAW)) {
                                ret = -EPERM;
                                break;
                        }

                        /* Bind this socket to a particular device like "eth0",
                         * as specified in the passed interface name. If the
                         * name is "" or the option length is zero the socket
                         * is not bound.
                         */

However, earlier in that method it returns -EINVAL if optlen is < sizeof(int).

The man page has comments similar to that in the code above.

Also, even when I get the un-bind call working with code similar to:

int z = 0;
setsockopt(s, SOL_SOCKET, SO_BINDTODEVICE, &z, sizeof(z));

The app I'm working on (Xorp) does not appear to work. Perhaps because the kernel does not clean up the cached route when you un-bind as it does in the (re)bind logic?

                                /* Remove any cached route for this socket. */
                                sk_dst_reset(sk);

Thanks,
Ben

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

From: Ben Greear <[email protected]>
Date: Tue, 04 Sep 2007 15:45:14 -0700

...

Yes, the comment definitely does not match the code and should be fixed one way or the other.

I'll do some code history research to try and figure out if this optlen==0 case ever worked properly.

Thanks.

From: Ben Greear <[email protected]>
Date: Tue, 04 Sep 2007 15:45:14 -0700

...

Ok, the patch below is how I'm dealing with this.

Let me know if things work better now, and also I would appreciate it if you could contact the man page maintainers to remove the optlen==0 language.

Thanks.

From: David S. Miller <[email protected]>
Date: Fri, 14 Sep 2007 13:10:17 -0700
Subject: [PATCH] [NET]: Fix two issues wrt. SO_BINDTODEVICE.

1) Comments suggest that setting optlen to zero will unbind the socket from whatever device it might be attached to. This hasn't been the case since at least 2.2.x because the first thing this function does is return -EINVAL if 'optlen' is less than sizeof(int).

Furthermore, there are not "optlen == 0" tests in the SO_BINDTODEVICE code either.

This also means we can toss the "!valbool" code block because if that is true we'll also see the first byte of the passed in name buffer as '\0' and this will also unbind the socket.

2) We should reset the cached route of the socket after we have made the device binding changes, not before.

Reported by Ben Greear.

---
 net/core/sock.c |   39 +++++++++++++++++----------------------
 1 files changed, 17 insertions(+), 22 deletions(-)

index cfed7d4..96be0ed 100644
@@ -591,36 +591,31 @@ set_rcvbuf:
 
 		/* Bind this socket to a particular device like "eth0",
 		 * as specified in the passed interface name. If the
-		 * name is "" or the option length is zero the socket
-		 * is not bound.
+		 * name is "", the socket is not bound.
 		 */
+		if (optlen > IFNAMSIZ - 1)
+			optlen = IFNAMSIZ - 1;
+		memset(devname, 0, sizeof(devname));
+		if (copy_from_user(devname, optval, optlen)) {
+			ret = -EFAULT;
+			break;
+		}
 
-		if (!valbool) {
+		if (devname[0] == '\0') {
 			sk->sk_bound_dev_if = 0;
 		} else {
-			if (optlen > IFNAMSIZ - 1)
-				optlen = IFNAMSIZ - 1;
-			memset(devname, 0, sizeof(devname));
-			if (copy_from_user(devname, optval, optlen)) {
-				ret = -EFAULT;
+			struct net_device *dev = dev_get_by_name(devname);
+			if (!dev) {
+				ret = -ENODEV;
 				break;
 			}
+			sk->sk_bound_dev_if = dev->ifindex;
+			dev_put(dev);
+		}
 
-			/* Remove any cached route for this socket. */
-			sk_dst_reset(sk);
+		/* Remove any cached route for this socket. */
+		sk_dst_reset(sk);
 
-			if (devname[0] == '\0') {
-				sk->sk_bound_dev_if = 0;
-			} else {
-				struct net_device *dev = dev_get_by_name(devname);
-				if (!dev) {
-					ret = -ENODEV;
-					break;
-				}
-				sk->sk_bound_dev_if = dev->ifindex;
-				dev_put(dev);
-			}
-		}
 		break;
 	}
 #endif

1.5.2.4

From user-space, does this imply that the 'empty string' we use to unbind must be at least 4 bytes long, but with the first byte /0?

If so, I think it might be confusing for the comments to say use "" to unbind, since that would not be a long enough chunk of memory.

Maybe something like "Use a character array of at least 4 bytes in length with the first byte set to '/0'."

This brings up another issue as well: What if the device name is "tr1", to bind to it we'd have to pass in "tr1/0" and optlen of 4. Not that this is difficult to do, but it does seem like a weird thing to have to do.

Thanks,
Ben

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

From: Ben Greear <[email protected]>
Date: Fri, 14 Sep 2007 15:11:39 -0700

I realize this, I'll try to come up with some clean way to deal with this.

I don't want to touch that "if (optlen < sizeof(int))" test at all because it makes all of the logic incredibly complicated because you then have to forcefully initialize the 'val' and 'valbool' if you bypass reading the int.

You always to account for the NULL termination, thus you always have to pass in "strlen(X) + 1" as optlen.

But I see your point, and my answer is the same as the first one.

Instead of nit-picking, can you instead try the patch and let me know if it makes things work for you?

Thanks.

I don't mean to complain, just trying to make sure that you had thought about the length and were happy with how it works before I try to get the man page changed.

I will try the patch, but it will probably not be until Monday. The existing code actually can be made to work by passing in a 4+ byte string and setting the first byte to zero, and as far as I can see, that remains the same with the new patch.

Thanks,
Ben

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

From: Ben Greear <[email protected]>
Date: Fri, 14 Sep 2007 15:37:45 -0700

You said the route wasn't cleared correctly in your test, and that's one part I'm concerned about making sure is fixed.

To be honest I'm slightly angry at you. I can travel for 24 hours non-stop on trains and planes and still be mostly responsive to people and apply patches. But I simply ask you to test a simple patch for a test case you said you already have and you can't do it until Monday?!?!?

This is why I typically low priority all reports from you, sorry for not doing so this time too. :-/

My previous report was because I was trying to get Xorp to work with BINDTODEVICE, and Xorp's a huge complicated beast. Due to it's actual use of the socket it would be very hard to trigger (or notice) the route bug, and since we use a 4-byte null string instead of depending on the bool value, I think it works around
it anyway.

The reason xorp wasn't working for me when I made the report was a problem with the netlink headers I was compiling against (didn't support the > 255 routing tables), but I didn't figure that out until later. Since the BINDTODEVICE route clearing code in the kernel appeared to be broken, I thought that might have been the problem, but in that I was wrong. I _do_ think the logic was/is wrong, it's just that I was hitting something else in this particular case.

So, I will have to write a whole new app to test your fix properly, and I plan to do so, but it will probably be next week.

I appreciate your responsiveness on the patch, as always, and will not be overly offended if you choose to ignore subsequent reports from me.

Thanks,
Ben

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