From: Ben Greear <hidden> Date: 2002-09-18 06:42:28
This patch allows one to use the SO_BINDTODEVICE and a new ioctl against
net_device objects to send and receive regular routed traffic between two
interfaces on the same machine. It also fixes a problem when using BINDTODEVICE:
the old code does not set the bound_if correctly when sending back the
syn-ack during TCP connection initialization.
I'm not sure how useful others will find it, but it amuses me ;)
Comments and suggestions welcome.
Thanks,
Ben
--
Ben Greear [off-list ref] <Ben_Greear AT excite.com>
President of Candela Technologies Inc http://www.candelatech.com
ScryMUD: http://scry.wanfear.comhttp://scry.wanfear.com/~greear
From: David S. Miller <hidden> Date: 2002-09-18 22:59:45
From: Ben Greear [off-list ref]
Date: Wed, 18 Sep 2002 00:09:50 -0700
http://www.candelatech.com/sts_2.4.19.patch
I don't think I'll be applying this:
1) No tcp ipv6 bits
2) SIOC{S,G}ACCEPTLOCALADDRS added, but no 32-bit translation
code added to varions 64-bit/32-bit biarch port ioctl handling.
Also, no code added to the ioctl dispatch in the networking
so that devices could actually receive these requests.
3) Finally, it's just too damn ugly. If you have to ifdef it then
it really doesn't belong in the tree. Maybe if the device number
comparison logic changes existed via macros in tcp.h and thus
removing all the CONFIG_NET_SENDTOSELF ifdefs from tcp*.c code
it might be more palatable.
4) I haven't reviewed the ramifications of the route lookup changes,
that is Alexey's territory.
Sorry, these changes are pretty ugly right now.
I know squat about this, so am reluctant to hack code there.
2) SIOC{S,G}ACCEPTLOCALADDRS added, but no 32-bit translation
code added to varions 64-bit/32-bit biarch port ioctl handling.
Also, no code added to the ioctl dispatch in the networking
so that devices could actually receive these requests.
See http://www.candelatech.com/sts2_hack.patch (32-bit only), it contains the missing
bits, I'm not good at generating two patch sets (ie pktgen and send-to-self)
when they touch the same file...
3) Finally, it's just too damn ugly. If you have to ifdef it then
it really doesn't belong in the tree. Maybe if the device number
comparison logic changes existed via macros in tcp.h and thus
removing all the CONFIG_NET_SENDTOSELF ifdefs from tcp*.c code
it might be more palatable.
The #ifdefs were per request, I personally would like them not to be there
either. As far as I can tell, the changes are backwards compatible, so there
should be no need for ifdefs.
4) I haven't reviewed the ramifications of the route lookup changes,
that is Alexey's territory.
Sorry, these changes are pretty ugly right now.
Thanks for looking at them. I can fix the #ifdef cruft, but adding 64bit
support or hacking ipv6 is beyond my means of testing at this point, so
I cannot make those changes.
Ben
--
Ben Greear [off-list ref] <Ben_Greear AT excite.com>
President of Candela Technologies Inc http://www.candelatech.com
ScryMUD: http://scry.wanfear.comhttp://scry.wanfear.com/~greear
From: David S. Miller <hidden> Date: 2002-09-19 01:33:06
From: Ben Greear [off-list ref]
Date: Wed, 18 Sep 2002 16:20:49 -0700
David S. Miller wrote:
> I don't think I'll be applying this:
>
> 1) No tcp ipv6 bits
I know squat about this, so am reluctant to hack code there.
It's hash lookup code, nearly identical to ipv4 version except
it's dealing with 128-bit IP addresses instead of 32-bit.
You give up way too easily, which leads me to belive you'll disappear
just as easily if complicated bugs stop popping up as a result of your
changes.
This is one of the most important issues I consider when I get a
delicate patch to the networking for someone, how fast they throw
their arms up in the air.
For example, someone like Arnaldo, when he sends me a patch and the
whole kernel explodes as a result I know he'll stick around for
however long it takes to fix the problems and he won't go "ipv6 looks
too complicated" when I ask him to submit a complete version of his
changes.
You patch should not be a maintainence burden to me. Your attitude
tells me it is going to become one.
See http://www.candelatech.com/sts2_hack.patch (32-bit only), it contains the missing
bits, I'm not good at generating two patch sets (ie pktgen and send-to-self)
when they touch the same file...
Don't include stuff in the patch that doesn't belong there, this isn't
so difficult.
The #ifdefs were per request, I personally would like them not to be there
either. As far as I can tell, the changes are backwards compatible, so there
should be no need for ifdefs.
I mean put the ifdefs in a header file such as tcp.h, not in the *.c
code.
Thanks for looking at them. I can fix the #ifdef cruft, but adding 64bit
support or hacking ipv6 is beyond my means of testing at this point, so
I cannot make those changes.
I don't require you to test the ipv6 portions, I will be able to
eyeball them and know if they are right or not, this is how simple
the ipv6 version of the tcp bits will be.
From: Ben Greear <hidden> Date: 2002-09-19 02:03:03
David S. Miller wrote:
It's hash lookup code, nearly identical to ipv4 version except
it's dealing with 128-bit IP addresses instead of 32-bit.
You give up way too easily, which leads me to belive you'll disappear
just as easily if complicated bugs stop popping up as a result of your
changes.
I'll maintain this patch for myself if no one else, so I will not
go away. But, since I am new to this code, and do not have a test
setup to test the ipv6 changes, I was hesitant. I can at the least
patch it how I think it should be and test that ipv4 still works and
it compiles. If you or someone else can do more profound testing on
it, then that would be great.
The #ifdefs were per request, I personally would like them not to be there
either. As far as I can tell, the changes are backwards compatible, so there
should be no need for ifdefs.
I mean put the ifdefs in a header file such as tcp.h, not in the *.c
code.
Would you object to me just removing all of them and having the patch
unconditionally compiled in?
I don't require you to test the ipv6 portions, I will be able to
eyeball them and know if they are right or not, this is how simple
the ipv6 version of the tcp bits will be.
Ok, I'll work up a patch with the ipv6 support and try to get that
out sometime next week.
Thanks,
Ben
From: David S. Miller <hidden> Date: 2002-09-19 02:05:27
From: Ben Greear [off-list ref]
Date: Wed, 18 Sep 2002 19:07:33 -0700
David S. Miller wrote:
> I mean put the ifdefs in a header file such as tcp.h, not in the *.c
> code.
Would you object to me just removing all of them and having the patch
unconditionally compiled in?
Your comments say that SIOCBINDTODEVICE behavior is changed, how can
we legitimately do that all the time without breaking apps?
From: Ben Greear <hidden> Date: 2002-09-19 03:00:06
David S. Miller wrote:
From: Ben Greear [off-list ref]
Date: Wed, 18 Sep 2002 19:07:33 -0700
David S. Miller wrote:
> I mean put the ifdefs in a header file such as tcp.h, not in the *.c
> code.
Would you object to me just removing all of them and having the patch
unconditionally compiled in?
Your comments say that SIOCBINDTODEVICE behavior is changed, how can
we legitimately do that all the time without breaking apps?
The old way is broken, it sets the bound-device to 0 when sending
the syn-ack. I am not sure exactly how this worked before, or
if it even worked at all. I changed it to use the bound_dev_if of the
parent socket, which I believe is more correct.
@@ -975,7 +975,11 @@daddr=replyopts.opt.faddr;}+#ifdef CONFIG_NET_SENDTOSELF+if(ip_route_output(&rt,daddr,rt->rt_spec_dst,RT_TOS(skb->nh.iph->tos),sk->bound_dev_if))+#elseif(ip_route_output(&rt,daddr,rt->rt_spec_dst,RT_TOS(skb->nh.iph->tos),0))+#endifreturn;/* And let IP do all the hard work.
It also failed due to the hashing issue, but with the current code,
packets to self from self will be dropped before reaching the IP
code, so that is not really a bug in the current code.
Ben
--
Ben Greear [off-list ref] <Ben_Greear AT excite.com>
President of Candela Technologies Inc http://www.candelatech.com
ScryMUD: http://scry.wanfear.comhttp://scry.wanfear.com/~greear
The old way is broken, it sets the bound-device to 0 when sending
the syn-ack.
Ben, this function is _not_ used to send syn-acks...
+#ifdef CONFIG_NET_SENDTOSELF
+ if (ip_route_output(&rt, daddr, rt->rt_spec_dst, RT_TOS(skb->nh.iph->tos), sk->bound_dev_if))
+#else
if (ip_route_output(&rt, daddr, rt->rt_spec_dst, RT_TOS(skb->nh.iph->tos), 0))
+#endif
This chunk is noop, sk here is a dummy socket internal to kernel,
where sk->bound_dev_if is identical zero. Grep code to see
what it is used for.
The same ("noopness") is true about 90% of the patch. F.e. all the messing
inside tcp with openreqs is noop.
Essentially, the only chunk which has a real meaning is that one
for fib_frontend.c. And it is simpler to do this with sysctl, compare
to rp_filter at al.
Alexey
From: Ben Greear <hidden> Date: 2002-09-27 01:30:51
kuznet@ms2.inr.ac.ru wrote:
Hello!
quoted
The old way is broken, it sets the bound-device to 0 when sending
the syn-ack.
Ben, this function is _not_ used to send syn-acks...
I am not so sure, untill I changed that method, it most certainly
did not work. It could have been something other than the syn-ack
that failed though...
Unfortunately, I ripped out all of the printks, so I cannot easily back
up my claims w/out poluting the code again.
quoted
+#ifdef CONFIG_NET_SENDTOSELF
+ if (ip_route_output(&rt, daddr, rt->rt_spec_dst, RT_TOS(skb->nh.iph->tos), sk->bound_dev_if))
+#else
if (ip_route_output(&rt, daddr, rt->rt_spec_dst, RT_TOS(skb->nh.iph->tos), 0))
+#endif
This chunk is noop, sk here is a dummy socket internal to kernel,
where sk->bound_dev_if is identical zero. Grep code to see
what it is used for.
Think about this: Suppose you are connecting to a listening socket that has been
bound to a device. That creates the the temporary socket structure
on the receive side, which
is used to send the syn-ack. That temp socket structure must also be
bound to the same device, or the ack will not get routed correctly
back out of the right interface.
As far as I can tell, the code must be patched as above or the temp socket
will not use the correct bound device. Please explain how the syn-ack
can get routed based on the parent's bound_dev_if if my assumption here is
not correct.
The same ("noopness") is true about 90% of the patch. F.e. all the messing
inside tcp with openreqs is noop.
Essentially, the only chunk which has a real meaning is that one
for fib_frontend.c. And it is simpler to do this with sysctl, compare
to rp_filter at al.
I will investigate that code...I haven't used sysctl on purpose before :)
Thanks for the review, and I look forward to your response to my
assertions!
Ben
Think about this: Suppose you are connecting to a listening socket that has been
bound to a device. That creates the the temporary socket structure
on the receive side,
That does _not_. All transmits use listening socket structure.
can get routed based on the parent's bound_dev_if if my assumption here is
not correct.
Find function tcp_v4_send_synack(), then tcp_v4_route_req() and derive
that your modifications have exactly zero impact on it,
sk->bound_dev_if is setup and used exactly like it was used before.
Alexey
From: Ben Greear <hidden> Date: 2002-09-27 06:33:10
kuznet@ms2.inr.ac.ru wrote:
Hello!
quoted
+#ifdef CONFIG_NET_SENDTOSELF
+ if (ip_route_output(&rt, daddr, rt->rt_spec_dst, RT_TOS(skb->nh.iph->tos), sk->bound_dev_if))
+#else
if (ip_route_output(&rt, daddr, rt->rt_spec_dst, RT_TOS(skb->nh.iph->tos), 0))
+#endif
This chunk is noop, sk here is a dummy socket internal to kernel,
where sk->bound_dev_if is identical zero. Grep code to see
what it is used for.
Ok, I took out my changes above and sure enough, it still seems to work.
I have a question though: If this method is ever called to send an RST,
will it work? It seems to me that it may not be routed correctly, but
I don't understand all the circumstances that would cause this method to
be called either...
The same ("noopness") is true about 90% of the patch. F.e. all the messing
inside tcp with openreqs is noop.
What about this part, do you think it is not needed either? It appeared
to me that w/out this, the sending socket and the receiving socket could hash
to the same thing, and so greatly confused themselves. For instance,
src=1.2.3.4, port 9999, dst=1.2.3.5 port 9999
I believe I was seeing the first packet (SYN) from the originator being
delivered back to the socket that sent the SYN because no new socket on
the receiving side was created because the hash found one (it found the
sending one, unfortunately).
+ /* Will only take netdevice_id into the equation if neither are
+ * 0. This should be backwards compatible with older code, and also
+ * let us connect to ourselves over external ports. Otherwise, we
+ * get confused about which connection is the originator v/s the
+ * receiver of the open request. --Ben
+ */
for (prev = &lopt->syn_table[tcp_v4_synq_hash(raddr, rport)];
(req = *prev) != NULL;
prev = &req->dl_next) {
if (req->rmt_port == rport &&
req->af.v4_req.rmt_addr == raddr &&
req->af.v4_req.loc_addr == laddr &&
- TCP_INET_FAMILY(req->class->family)) {
+ TCP_INET_FAMILY(req->class->family)
+#ifdef CONFIG_NET_SENDTOSELF
+ && ((!netdevice_id) || (!req->bound_dev_if) ||
+ (req->bound_dev_if == netdevice_id))) {
+#else
+ ) {
+#endif
Thanks again,
Ben
--
Ben Greear [off-list ref] <Ben_Greear AT excite.com>
President of Candela Technologies Inc http://www.candelatech.com
ScryMUD: http://scry.wanfear.comhttp://scry.wanfear.com/~greear
I have a question though: If this method is ever called to send an RST,
will it work?
Work for what? This method is used when you have no socket in the system
matching to this skb. :-)
What about this part, do you think it is not needed either? It appeared
to me that w/out this, the sending socket and the receiving socket
...
sending one, unfortunately).
No, this is impossible. addrs/ports identity identifies socket unambiguously.
But even this does not matter, your patch is noop by much simpler pure
"syntactical" reason, it copies req->bound_dev_if from listening socket.
And after this the check in lookup req degenerates to TRUE __identitically__.
:-)
Alexey
From: Ben Greear <hidden> Date: 2002-09-27 15:40:11
kuznet@ms2.inr.ac.ru wrote:
Hello!
quoted
I have a question though: If this method is ever called to send an RST,
will it work?
Work for what? This method is used when you have no socket in the system
matching to this skb. :-)
quoted
What about this part, do you think it is not needed either? It appeared
to me that w/out this, the sending socket and the receiving socket
...
quoted
sending one, unfortunately).
No, this is impossible. addrs/ports identity identifies socket unambiguously.
Assume we are sending to ourself, and we bind to the local ip/port before we connect:
we have source IP = 1.2.3.4, source-port = 7
Assume we are connecting to a socket on a second interface, with dest ip = 1.2.3.5,
dest-port = 7
When the SYN is received on the second port after connect() is called, the code
looks for a socket based on source ip 1.2.3.4 and source port 7. In this case, it should
not find any socket because this SYN is the first communication....
However, because the sender is on the same machine, using the same kernel
structures, the socket **DOES** exist, and so the hash lookup finds it. At
this point, I believe it sends the RST because a socket in the SYN-sent state
just received a SYN packet, which is not how things should work.
But even this does not matter, your patch is noop by much simpler pure
"syntactical" reason, it copies req->bound_dev_if from listening socket.
And after this the check in lookup req degenerates to TRUE __identitically__.
:-)
It will not match the case where the sending socket is bound to interface 3
and the listening socket is bound on interface 4, but both sockets have the
same source IP and port. That was what I was trying
to do...but I may have not done it quite right :)
Thanks,
Ben
It will not match the case where the sending socket is bound to interface 3
and the listening socket is bound on interface 4, but both sockets have the
same source IP and port. That was what I was trying
to do...but I may have not done it quite right :)
I would say "quite not right". :-)
Anyway, this is supposed to work without any modifications.
Alexey
From: Ben Greear <hidden> Date: 2002-09-27 15:53:27
kuznet@ms2.inr.ac.ru wrote:
Hello!
quoted
It will not match the case where the sending socket is bound to interface 3
and the listening socket is bound on interface 4, but both sockets have the
same source IP and port. That was what I was trying
to do...but I may have not done it quite right :)
I would say "quite not right". :-)
Anyway, this is supposed to work without any modifications.
I will run some tests with that part of the code commented out.
Thanks,
Ben