Re: [PATCH] ipv4: do not cache local route when using SO_BINDTODEVICE

[Kouya Shimura]

Thanks for reviewing.

David Miller <da...@davemloft.net> writes:
> From: Kouya Shimura <ko...@jp.fujitsu.com>
> Date: Tue, 23 Feb 2016 14:27:32 +0900
>
>> After v3.6, output routes are cached, however, the 'rt_iif' field of
>> struct rtable can not be shared by various traffics using SO_BINDTODEVICE.
>> It causes that traffic can not reach the local receiver which also uses 
>> SO_BINDTODEVICE after another traffic creates a local route cache. 
>> 
>>   /* sender and receiver are running on same host. */
>>   sender:
>> - socket(SOCK_DGRAM)
>> - setsockopt(SO_BINDTODEVICE, "eth0")
>> - connect(INADDR_ANY)
>> - send("message") -->[loopback]--+
>>  |
>>   receiver:  |
>> - socket(SOCK_DGRAM) |
>> - setsockopt(SO_BINDTODEVICE, "eth0")|
>> - bind(INADDR_ANY)   |
>> - recv()  <--+
>>   /* sometimes reach, sometimes not reach!!! */
>> 
>> Before v3.6, above traffics always reached. It seems to be a
>> regression. Actually the 'dhcp_release' command relies on it.
>> 
>> Commit 7bd86cc282a458b66c41e3f6 ("ipv4: Cache local output routes")
>> originated this issue. Revert it conditionally.
>> 
>> Signed-off-by: Kouya Shimura <ko...@jp.fujitsu.com>
>
> I strongly fear that this will cause a performance regression for some 
> important
> cases.  If you elide the cache, it means that every route lookup has
> to allocate,
> create, and destroy the cache object.  This is really expensive.

The performace degradation was on my mind. Thus, I intended that this
patch affects only special (abnormal?) case that destination is local
and using SO_BINDTODEVICE. Isn't it acceptable?

This patch deesn't elide the existing cache. Normal (not using
SO_BINDTODEVICE) routes are still cached (not destoyed). Outer
routes, too. I confirmed it.

If you worry about non-zeroed 'orig_oif' case, this patch certainly
implies a perfomance problem. 'orig_oif' seems to be non-zero in
cases of ICMP, GRE, MULTICAST besides SO_BINDTODEVICE. 

>
> There has to be a better way to do so, so I'm not applying this patch,
> sorry.

Yes, it has to be. The root cause is sharing the 'rt_iif' field.
I'm looking forward to the right fix.

Thanks,
Kouya

[PATCH] ipv4: do not cache local route when using SO_BINDTODEVICE

[Kouya Shimura]

After v3.6, output routes are cached, however, the 'rt_iif' field of
struct rtable can not be shared by various traffics using SO_BINDTODEVICE.
It causes that traffic can not reach the local receiver which also uses 
SO_BINDTODEVICE after another traffic creates a local route cache. 

  /* sender and receiver are running on same host. */
  sender:
- socket(SOCK_DGRAM)
- setsockopt(SO_BINDTODEVICE, "eth0")
- connect(INADDR_ANY)
- send("message") -->[loopback]--+
 |
  receiver:  |
- socket(SOCK_DGRAM) |
- setsockopt(SO_BINDTODEVICE, "eth0")|
- bind(INADDR_ANY)   |
- recv()  <--+
  /* sometimes reach, sometimes not reach!!! */

Before v3.6, above traffics always reached. It seems to be a
regression. Actually the 'dhcp_release' command relies on it.

Commit 7bd86cc282a458b66c41e3f6 ("ipv4: Cache local output routes")
originated this issue. Revert it conditionally.

Signed-off-by: Kouya Shimura <ko...@jp.fujitsu.com>
---
 net/ipv4/route.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 02c6229..4b5e024 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -2285,6 +2285,8 @@ struct rtable *__ip_route_output_key_hash(struct net 
*net, struct flowi4 *fl4,
}
dev_out = net->loopback_dev;
fl4->flowi4_oif = dev_out->ifindex;
+   if (orig_oif)
+   res.fi = NULL;
flags |= RTCF_LOCAL;
goto make_route;
}
-- 
1.9.1