SO_BINDTODEVICE mismatch with man page & comments.

SO_BINDTODEVICE mismatch with man page & comments.

[Ben Greear]

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 <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: SO_BINDTODEVICE mismatch with man page & comments.

[David Miller]

From: Ben Greear <greearb@candelatech.com>
Date: Tue, 04 Sep 2007 15:45:14 -0700

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

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.

Re: SO_BINDTODEVICE mismatch with man page & comments.

[David Miller]

From: Ben Greear <greearb@candelatech.com>
Date: Tue, 04 Sep 2007 15:45:14 -0700

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

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 136f55cf4ad0a3b0185bfc97c68f9e4d74ddcfe7 Mon Sep 17 00:00:00 2001
From: David S. Miller <davem@sunset.davemloft.net>
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.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/core/sock.c |   39 +++++++++++++++++----------------------
 1 files changed, 17 insertions(+), 22 deletions(-)

diff --git a/net/core/sock.c b/net/core/sock.c
index cfed7d4..96be0ed 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -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

Re: SO_BINDTODEVICE mismatch with man page & comments.

[Ben Greear]

David Miller wrote:
> From: Ben Greear <greearb@candelatech.com>
> Date: Tue, 04 Sep 2007 15:45:14 -0700
> 
>> 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:
>  ...
>> 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);
>>
> 
> 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 136f55cf4ad0a3b0185bfc97c68f9e4d74ddcfe7 Mon Sep 17 00:00:00 2001
> From: David S. Miller <davem@sunset.davemloft.net>
> 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.

 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 <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: SO_BINDTODEVICE mismatch with man page & comments.

[David Miller]

From: Ben Greear <greearb@candelatech.com>
Date: Fri, 14 Sep 2007 15:11:39 -0700

>  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'."

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.

> 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.

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.

Re: SO_BINDTODEVICE mismatch with man page & comments.

[Ben Greear]

David Miller wrote:

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

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 <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: SO_BINDTODEVICE mismatch with man page & comments.

[David Miller]

From: Ben Greear <greearb@candelatech.com>
Date: Fri, 14 Sep 2007 15:37:45 -0700

> 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.

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. :-/

Re: SO_BINDTODEVICE mismatch with man page & comments.

[Ben Greear]

David Miller wrote:
> From: Ben Greear <greearb@candelatech.com>
> Date: Fri, 14 Sep 2007 15:37:45 -0700
> 
>> 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.
> 
> 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 <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com