Re: [PATCH] Networking: send-to-self

Re: [PATCH] Networking: send-to-self

[David S. Miller]

We saw it the first two times.

[PATCH] Networking: send-to-self

[Ben Greear]

[-- Attachment #1: Type: text/plain, Size: 676 bytes --]

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 <greearb@candelatech.com>       <Ben_Greear AT excite.com>
President of Candela Technologies Inc      http://www.candelatech.com
ScryMUD:  http://scry.wanfear.com     http://scry.wanfear.com/~greear


[-- Attachment #2: sts_2.4.19.patch --]
[-- Type: text/plain, Size: 9264 bytes --]

--- linux-2.4.19/include/linux/sockios.h	Wed Nov  7 15:39:36 2001
+++ linux-2.4.19.dev/include/linux/sockios.h	Sun Sep 15 21:10:00 2002
@@ -114,6 +114,16 @@
 #define SIOCBONDINFOQUERY      0x8994	/* rtn info about bond state    */
 #define SIOCBONDCHANGEACTIVE   0x8995   /* update to a new active slave */
 			
+
+/* Ben's little hack land */
+#define SIOCSACCEPTLOCALADDRS  0x89a0   /*  Allow interfaces to accept pkts from
+                                         * local interfaces...use with SO_BINDTODEVICE
+                                         */
+#define SIOCGACCEPTLOCALADDRS  0x89a1   /*  Allow interfaces to accept pkts from
+                                         * local interfaces...use with SO_BINDTODEVICE
+                                         */
+
+
 /* Device private ioctl calls */
 
 /*
--- linux-2.4.19/net/Config.in	Fri Aug  2 17:39:46 2002
+++ linux-2.4.19.dev/net/Config.in	Sun Sep 15 21:27:08 2002
@@ -97,6 +97,7 @@
 mainmenu_option next_comment
 comment 'Network testing'
 tristate 'Packet Generator (USE WITH CAUTION)' CONFIG_NET_PKTGEN
+bool 'Send-to-Self' CONFIG_NET_SENDTOSELF
 endmenu
 
 endmenu
--- linux-2.4.19/include/net/tcp.h	Fri Aug  2 17:39:46 2002
+++ linux-2.4.19.dev/include/net/tcp.h	Sun Sep 15 21:57:07 2002
@@ -27,6 +27,7 @@
 #include <linux/config.h>
 #include <linux/tcp.h>
 #include <linux/slab.h>
+#include <linux/cache.h>
 #include <net/checksum.h>
 #include <net/sock.h>
 
@@ -117,8 +118,7 @@
 	 * Now align to a new cache line as all the following members
 	 * are often dirty.
 	 */
-	rwlock_t __tcp_lhash_lock
-		__attribute__((__aligned__(SMP_CACHE_BYTES)));
+	rwlock_t __tcp_lhash_lock ____cacheline_aligned;
 	atomic_t __tcp_lhash_users;
 	wait_queue_head_t __tcp_lhash_wait;
 	spinlock_t __tcp_portalloc_lock;
@@ -447,7 +447,6 @@
 extern int sysctl_tcp_retrans_collapse;
 extern int sysctl_tcp_stdurg;
 extern int sysctl_tcp_rfc1337;
-extern int sysctl_tcp_tw_recycle;
 extern int sysctl_tcp_abort_on_overflow;
 extern int sysctl_tcp_max_orphans;
 extern int sysctl_tcp_max_tw_buckets;
@@ -520,6 +519,14 @@
 		struct tcp_v6_open_req v6_req;
 #endif
 	} af;
+#ifdef CONFIG_NET_SENDTOSELF
+        int bound_dev_if; /* This is so we can connect to ourselves and not collide
+                           * in the open-request hash (the addresses and ports are the
+                           * same, but we need two ends, so use the interface to determine
+                           * one from the other.  Active when SO_BINDTODEVICE is used.
+                           * --Ben
+                           */
+#endif
 };
 
 /* SLAB cache for open requests. */
--- linux-2.4.19/net/ipv4/arp.c	Fri Aug  2 17:39:46 2002
+++ linux-2.4.19.dev/net/ipv4/arp.c	Sun Sep 15 21:14:43 2002
@@ -1,4 +1,4 @@
-/* linux/net/inet/arp.c
+/* linux/net/inet/arp.c  -*-linux-c-*-
  *
  * Version:	$Id: sts_2.4.19.patch,v 1.3 2002/09/17 07:01:55 greear Exp $
  *
@@ -351,12 +351,26 @@
 	int flag = 0; 
 	/*unsigned long now; */
 
-	if (ip_route_output(&rt, sip, tip, 0, 0) < 0) 
+	if (ip_route_output(&rt, sip, tip, 0, 0) < 0)
 		return 1;
-	if (rt->u.dst.dev != dev) { 
-		NET_INC_STATS_BH(ArpFilter);
-		flag = 1;
-	} 
+        
+	if (rt->u.dst.dev != dev) {
+#ifdef CONFIG_NET_SENDTOSELF
+                if ((dev->priv_flags & IFF_ACCEPT_LOCAL_ADDRS) &&
+                    (rt->u.dst.dev == &loopback_dev))  {
+                        /* OK, we'll let this special case slide, so that we can arp from one
+                         * local interface to another.  This seems to work, but could use some
+                         * review. --Ben
+                         */
+                }
+                else {
+#endif
+                        NET_INC_STATS_BH(ArpFilter);
+                        flag = 1;
+#ifdef CONFIG_NET_SENDTOSELF
+                }
+#endif
+        }
 	ip_rt_put(rt); 
 	return flag; 
 } 
--- linux-2.4.19/net/ipv4/fib_frontend.c	Fri Aug  2 17:39:46 2002
+++ linux-2.4.19.dev/net/ipv4/fib_frontend.c	Sun Sep 15 21:16:44 2002
@@ -233,8 +233,19 @@
 
 	if (fib_lookup(&key, &res))
 		goto last_resort;
-	if (res.type != RTN_UNICAST)
-		goto e_inval_res;
+        
+	if (res.type != RTN_UNICAST) {
+#ifdef CONFIG_NET_SENDTOSELF
+           if ((res.type == RTN_LOCAL) &&
+                    (dev->priv_flags & IFF_ACCEPT_LOCAL_ADDRS)) {
+                     /* All is OK */
+           }
+           else
+#endif
+              goto e_inval_res;
+                
+        }
+        
 	*spec_dst = FIB_RES_PREFSRC(res);
 	fib_combine_itag(itag, &res);
 #ifdef CONFIG_IP_ROUTE_MULTIPATH
--- linux-2.4.19/net/ipv4/ip_output.c	Fri Aug  2 17:39:46 2002
+++ linux-2.4.19.dev/net/ipv4/ip_output.c	Sun Sep 15 21:19:45 2002
@@ -520,8 +520,18 @@
 		/*
 		 *	Get the memory we require with some space left for alignment.
 		 */
-
-		skb = sock_alloc_send_skb(sk, fraglen+hh_len+15, flags&MSG_DONTWAIT, &err);
+		if (!(flags & MSG_DONTWAIT) || nfrags == 0) {
+			skb = sock_alloc_send_skb(sk, fraglen + hh_len + 15,
+						  (flags & MSG_DONTWAIT), &err);
+		} else {
+			/* On a non-blocking write, we check for send buffer
+			 * usage on the first fragment only.
+			 */
+			skb = sock_wmalloc(sk, fraglen + hh_len + 15, 1,
+					   sk->allocation);
+			if (!skb)
+				err = -ENOBUFS;
+		}
 		if (skb == NULL)
 			goto error;
 
@@ -965,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))
+#else
 	if (ip_route_output(&rt, daddr, rt->rt_spec_dst, RT_TOS(skb->nh.iph->tos), 0))
+#endif
 		return;
 
 	/* And let IP do all the hard work.
--- linux-2.4.19/net/ipv4/tcp_ipv4.c	Fri Aug  2 17:39:46 2002
+++ linux-2.4.19.dev/net/ipv4/tcp_ipv4.c	Sun Sep 15 21:25:42 2002
@@ -865,21 +865,36 @@
 	return h&(TCP_SYNQ_HSIZE-1);
 }
 
+/** Netdevice ID can be wild-carded by making it zero.
+ */
 static struct open_request *tcp_v4_search_req(struct tcp_opt *tp, 
 					      struct open_request ***prevp,
 					      __u16 rport,
-					      __u32 raddr, __u32 laddr)
+					      __u32 raddr, __u32 laddr,
+                                              int netdevice_id)
 {
 	struct tcp_listen_opt *lopt = tp->listen_opt;
 	struct open_request *req, **prev;  
 
+        /* 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
 			BUG_TRAP(req->sk == NULL);
 			*prevp = prev;
 			return req; 
@@ -899,7 +914,10 @@
 	req->retrans = 0;
 	req->sk = NULL;
 	req->dl_next = lopt->syn_table[h];
-
+#ifdef CONFIG_NET_SENDTOSELF
+        req->bound_dev_if = sk->bound_dev_if;
+#endif
+        
 	write_lock(&tp->syn_wait_lock);
 	lopt->syn_table[h] = req;
 	write_unlock(&tp->syn_wait_lock);
@@ -979,7 +997,8 @@
 	struct sock *sk;
 	__u32 seq;
 	int err;
-
+        int netdevice_id = 0;
+        
 	if (skb->len < (iph->ihl << 2) + 8) {
 		ICMP_INC_STATS_BH(IcmpInErrors); 
 		return;
@@ -1048,9 +1067,13 @@
 		if (sk->lock.users != 0)
 			goto out;
 
+                if (skb->dev) {
+                        netdevice_id = skb->dev->ifindex;
+                }
+                
 		req = tcp_v4_search_req(tp, &prev,
 					th->dest,
-					iph->daddr, iph->saddr); 
+					iph->daddr, iph->saddr, netdevice_id);
 		if (!req)
 			goto out;
 
@@ -1394,7 +1417,7 @@
 #define want_cookie 0 /* Argh, why doesn't gcc optimize this :( */
 #endif
 
-	/* Never answer to SYNs send to broadcast or multicast */
+	/* Never answer to SYNs sent to broadcast or multicast */
 	if (((struct rtable *)skb->dst)->rt_flags & 
 	    (RTCF_BROADCAST|RTCF_MULTICAST))
 		goto drop; 
@@ -1452,6 +1475,10 @@
 	req->af.v4_req.rmt_addr = saddr;
 	req->af.v4_req.opt = tcp_v4_save_options(sk, skb);
 	req->class = &or_ipv4;
+#ifdef CONFIG_NET_SENDTOSELF
+        req->bound_dev_if = sk->bound_dev_if;
+#endif
+        
 	if (!want_cookie)
 		TCP_ECN_create_request(req, skb->h.th);
 
@@ -1587,11 +1614,12 @@
 	struct iphdr *iph = skb->nh.iph;
 	struct tcp_opt *tp = &(sk->tp_pinfo.af_tcp);
 	struct sock *nsk;
-
+        int device_id = tcp_v4_iif(skb);
+        
 	/* Find possible connection requests. */
 	req = tcp_v4_search_req(tp, &prev,
 				th->source,
-				iph->saddr, iph->daddr);
+				iph->saddr, iph->daddr, device_id);
 	if (req)
 		return tcp_check_req(sk, skb, req, prev);
 
@@ -1599,7 +1627,7 @@
 					  th->source,
 					  skb->nh.iph->daddr,
 					  ntohs(th->dest),
-					  tcp_v4_iif(skb));
+					  device_id);
 
 	if (nsk) {
 		if (nsk->state != TCP_TIME_WAIT) {

Re: [PATCH] Networking: send-to-self

[Ben Greear]

Ben Greear wrote:
> 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

Gack, sorry for the last patch..it seems I screwed up the
patch process somehow.  Plz don't apply it as is!

-- 
Ben Greear <greearb@candelatech.com>       <Ben_Greear AT excite.com>
President of Candela Technologies Inc      http://www.candelatech.com
ScryMUD:  http://scry.wanfear.com     http://scry.wanfear.com/~greear

Re: [PATCH] Networking: send-to-self [link to non-broken patch this time]

[Ben Greear]

So, I feel bad about wasting bandwidth and spamming folks with busted patches.
The corrected patch(es) can be found here for those who are interested:

http://www.candelatech.com/sts_2.4.19.patch
http://www.candelatech.com/sts2_hack.patch

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>       <Ben_Greear AT excite.com>
President of Candela Technologies Inc      http://www.candelatech.com
ScryMUD:  http://scry.wanfear.com     http://scry.wanfear.com/~greear

Re: [PATCH] Networking: send-to-self [link to non-broken patch this time]

[David S. Miller]

   From: Ben Greear <greearb@candelatech.com>
   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.

Re: [PATCH] Networking: send-to-self [link to non-broken patch this time]

[Ben Greear]

David S. Miller wrote:
>    From: Ben Greear <greearb@candelatech.com>
>    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

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 <greearb@candelatech.com>       <Ben_Greear AT excite.com>
President of Candela Technologies Inc      http://www.candelatech.com
ScryMUD:  http://scry.wanfear.com     http://scry.wanfear.com/~greear

Re: [PATCH] Networking: send-to-self [link to non-broken patch this time]

[David S. Miller]

   From: Ben Greear <greearb@candelatech.com>
   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.

Re: [PATCH] Networking: send-to-self [link to non-broken patch this time]

[David S. Miller]

   From: Ben Greear <greearb@candelatech.com>
   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?

Re: [PATCH] Networking: send-to-self [link to non-broken patch this time]

[Ben Greear]

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


> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 


-- 
Ben Greear <greearb@candelatech.com>       <Ben_Greear AT excite.com>
President of Candela Technologies Inc      http://www.candelatech.com
ScryMUD:  http://scry.wanfear.com     http://scry.wanfear.com/~greear

Re: [PATCH] Networking: send-to-self [link to non-broken patch this time]

[Ben Greear]

David S. Miller wrote:
>    From: Ben Greear <greearb@candelatech.com>
>    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.

--- linux-2.4.19/net/ipv4/ip_output.c	Tue Sep 17 23:55:48 2002
+++ linux-2.4.19.dev/net/ipv4/ip_output.c	Sun Sep 15 21:19:45 2002
@@ -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))
+#else
  	if (ip_route_output(&rt, daddr, rt->rt_spec_dst, RT_TOS(skb->nh.iph->tos), 0))
+#endif
  		return;

  	/* 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 <greearb@candelatech.com>       <Ben_Greear AT excite.com>
President of Candela Technologies Inc      http://www.candelatech.com
ScryMUD:  http://scry.wanfear.com     http://scry.wanfear.com/~greear

Re: [PATCH] Networking: send-to-self [link to non-broken patch this

[kuznet]

Hello!

> 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

Re: [PATCH] Networking: send-to-self [link to non-broken patch this

[Ben Greear]

kuznet@ms2.inr.ac.ru wrote:
> Hello!
> 
> 
>>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.

> 
> 
> 
>>+#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

> 
> Alexey
> 


-- 
Ben Greear <greearb@candelatech.com>       <Ben_Greear AT excite.com>
President of Candela Technologies Inc      http://www.candelatech.com
ScryMUD:  http://scry.wanfear.com     http://scry.wanfear.com/~greear

Re: [PATCH] Networking: send-to-self [link to non-broken patch this

[kuznet]

Hello!

> 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

Re: [PATCH] Networking: send-to-self [link to non-broken patch this

[Ben Greear]

kuznet@ms2.inr.ac.ru wrote:
> Hello!

>>+#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 <greearb@candelatech.com>       <Ben_Greear AT excite.com>
President of Candela Technologies Inc      http://www.candelatech.com
ScryMUD:  http://scry.wanfear.com     http://scry.wanfear.com/~greear

Re: [PATCH] Networking: send-to-self [link to non-broken patch this

[kuznet]

Hello!

> 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

Re: [PATCH] Networking: send-to-self [link to non-broken patch this

[Ben Greear]

kuznet@ms2.inr.ac.ru wrote:
> Hello!
> 
> 
>>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.

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

> 
> Alexey
> 


-- 
Ben Greear <greearb@candelatech.com>       <Ben_Greear AT excite.com>
President of Candela Technologies Inc      http://www.candelatech.com
ScryMUD:  http://scry.wanfear.com     http://scry.wanfear.com/~greear

Re: [PATCH] Networking: send-to-self [link to non-broken patch this

[kuznet]

Hello!

> 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

Re: [PATCH] Networking: send-to-self [link to non-broken patch this

[Ben Greear]

kuznet@ms2.inr.ac.ru wrote:
> Hello!
> 
> 
>>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

> 
> Alexey
> 


-- 
Ben Greear <greearb@candelatech.com>       <Ben_Greear AT excite.com>
President of Candela Technologies Inc      http://www.candelatech.com
ScryMUD:  http://scry.wanfear.com     http://scry.wanfear.com/~greear