problem of "ipv4: revert Set rt->rt_iif more sanely on output routes."

problem of "ipv4: revert Set rt->rt_iif more sanely on output routes."

[OGAWA Hirofumi]

Hi,

ipv4: Set rt->rt_iif more sanely on output routes.
(1018b5c01636c7c6bda31a719bda34fc631db29a)

The above patch seems to be caused of avahi breakage.

I'm not debugging fully though, avahi is using IP_PKTINFO and checking
in_pktinfo->ipi_ifindex > 0.

And if I reverted above patch, it seems to fix avahi's IP_PKTINFO problem.

Ideas?

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: problem of "ipv4: revert Set rt->rt_iif more sanely on output routes."

[OGAWA Hirofumi]

OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes:

> Hi,
>
> ipv4: Set rt->rt_iif more sanely on output routes.
> (1018b5c01636c7c6bda31a719bda34fc631db29a)
>
> The above patch seems to be caused of avahi breakage.
>
> I'm not debugging fully though, avahi is using IP_PKTINFO and checking
> in_pktinfo->ipi_ifindex > 0.
>
> And if I reverted above patch, it seems to fix avahi's IP_PKTINFO problem.
>
> Ideas?

BTW, simple revert seems to break routing or something instead.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: problem of "ipv4: revert Set rt->rt_iif more sanely on output routes."

[OGAWA Hirofumi]

OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes:

> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes:
>
>> Hi,
>>
>> ipv4: Set rt->rt_iif more sanely on output routes.
>> (1018b5c01636c7c6bda31a719bda34fc631db29a)
>>
>> The above patch seems to be caused of avahi breakage.
>>
>> I'm not debugging fully though, avahi is using IP_PKTINFO and checking
>> in_pktinfo->ipi_ifindex > 0.
>>
>> And if I reverted above patch, it seems to fix avahi's IP_PKTINFO problem.
>>
>> Ideas?
>
> BTW, simple revert seems to break routing or something instead.

Just as FYI, this patch seems to fix the problem. (Only slightly tested
and reviewed. Please don't apply this until someone review. )

 include/net/route.h     |    1 +
 net/ipv4/ip_sockglue.c  |    2 +-
 net/ipv4/route.c        |    6 +++++-
 net/ipv4/xfrm4_policy.c |    1 +
 4 files changed, 8 insertions(+), 2 deletions(-)

diff -puN net/ipv4/route.c~revert-avahi-breaker net/ipv4/route.c
--- linux-2.6/net/ipv4/route.c~revert-avahi-breaker	2011-04-05 22:59:32.000000000 +0900
+++ linux-2.6-hirofumi/net/ipv4/route.c	2011-04-05 23:06:12.000000000 +0900
@@ -1891,6 +1891,7 @@ static int ip_route_input_mc(struct sk_b
 #ifdef CONFIG_IP_ROUTE_CLASSID
 	rth->dst.tclassid = itag;
 #endif
+	rth->rt_user_if	= dev->ifindex;
 	rth->rt_iif	= dev->ifindex;
 	rth->dst.dev	= init_net.loopback_dev;
 	dev_hold(rth->dst.dev);
@@ -2026,6 +2027,7 @@ static int __mkroute_input(struct sk_buf
 	rth->rt_key_src	= saddr;
 	rth->rt_src	= saddr;
 	rth->rt_gateway	= daddr;
+	rth->rt_user_if	= in_dev->dev->ifindex;
 	rth->rt_iif 	= in_dev->dev->ifindex;
 	rth->dst.dev	= (out_dev)->dev;
 	dev_hold(rth->dst.dev);
@@ -2202,6 +2204,7 @@ local_input:
 #ifdef CONFIG_IP_ROUTE_CLASSID
 	rth->dst.tclassid = itag;
 #endif
+	rth->rt_user_if	= dev->ifindex;
 	rth->rt_iif	= dev->ifindex;
 	rth->dst.dev	= net->loopback_dev;
 	dev_hold(rth->dst.dev);
@@ -2401,6 +2404,7 @@ static struct rtable *__mkroute_output(c
 	rth->rt_mark    = oldflp4->flowi4_mark;
 	rth->rt_dst	= fl4->daddr;
 	rth->rt_src	= fl4->saddr;
+	rth->rt_user_if	= oldflp4->flowi4_oif ? : dev_out->ifindex;
 	rth->rt_iif	= 0;
 	/* get references to the devices that are to be hold by the routing
 	   cache entry */
@@ -2716,6 +2720,7 @@ struct dst_entry *ipv4_blackhole_route(s
 		rt->rt_key_dst = ort->rt_key_dst;
 		rt->rt_key_src = ort->rt_key_src;
 		rt->rt_tos = ort->rt_tos;
+		rt->rt_user_if = ort->rt_user_if;
 		rt->rt_iif = ort->rt_iif;
 		rt->rt_oif = ort->rt_oif;
 		rt->rt_mark = ort->rt_mark;
@@ -2725,7 +2730,6 @@ struct dst_entry *ipv4_blackhole_route(s
 		rt->rt_type = ort->rt_type;
 		rt->rt_dst = ort->rt_dst;
 		rt->rt_src = ort->rt_src;
-		rt->rt_iif = ort->rt_iif;
 		rt->rt_gateway = ort->rt_gateway;
 		rt->rt_spec_dst = ort->rt_spec_dst;
 		rt->peer = ort->peer;
diff -puN include/net/route.h~revert-avahi-breaker include/net/route.h
--- linux-2.6/include/net/route.h~revert-avahi-breaker	2011-04-05 23:00:34.000000000 +0900
+++ linux-2.6-hirofumi/include/net/route.h	2011-04-05 23:00:24.000000000 +0900
@@ -64,6 +64,7 @@ struct rtable {
 
 	__be32			rt_dst;	/* Path destination	*/
 	__be32			rt_src;	/* Path source		*/
+	int			rt_user_if;
 	int			rt_iif;
 	int			rt_oif;
 	__u32			rt_mark;
diff -puN net/ipv4/xfrm4_policy.c~revert-avahi-breaker net/ipv4/xfrm4_policy.c
--- linux-2.6/net/ipv4/xfrm4_policy.c~revert-avahi-breaker	2011-04-05 23:03:56.000000000 +0900
+++ linux-2.6-hirofumi/net/ipv4/xfrm4_policy.c	2011-04-05 23:04:02.000000000 +0900
@@ -74,6 +74,7 @@ static int xfrm4_fill_dst(struct xfrm_ds
 	rt->rt_key_dst = fl4->daddr;
 	rt->rt_key_src = fl4->saddr;
 	rt->rt_tos = fl4->flowi4_tos;
+	rt->rt_user_if = fl4->flowi4_iif;
 	rt->rt_iif = fl4->flowi4_iif;
 	rt->rt_oif = fl4->flowi4_oif;
 	rt->rt_mark = fl4->flowi4_mark;
diff -puN net/ipv4/ip_sockglue.c~revert-avahi-breaker net/ipv4/ip_sockglue.c
--- linux-2.6/net/ipv4/ip_sockglue.c~revert-avahi-breaker	2011-04-05 23:04:52.000000000 +0900
+++ linux-2.6-hirofumi/net/ipv4/ip_sockglue.c	2011-04-05 23:04:56.000000000 +0900
@@ -62,7 +62,7 @@ static void ip_cmsg_recv_pktinfo(struct
 
 	info.ipi_addr.s_addr = ip_hdr(skb)->daddr;
 	if (rt) {
-		info.ipi_ifindex = rt->rt_iif;
+		info.ipi_ifindex = rt->rt_user_if;
 		info.ipi_spec_dst.s_addr = rt->rt_spec_dst;
 	} else {
 		info.ipi_ifindex = 0;
_

-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: problem of "ipv4: revert Set rt->rt_iif more sanely on output routes."

[David Miller]

From: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
Date: Tue, 05 Apr 2011 22:05:10 +0900

> ipv4: Set rt->rt_iif more sanely on output routes.
> (1018b5c01636c7c6bda31a719bda34fc631db29a)
> 
> The above patch seems to be caused of avahi breakage.
> 
> I'm not debugging fully though, avahi is using IP_PKTINFO and checking
> in_pktinfo->ipi_ifindex > 0.
> 
> And if I reverted above patch, it seems to fix avahi's IP_PKTINFO problem.

in_pktinfo is given to the application only during recvmsg() calls, the
call chain is (for example):

udp_recvmsg()
	--> ip_cmsg_recv()
		--> ip_cmsg_recv_pktinfo()

Therefore we will only be working with receive packets, whose routes are
computed using ip_route_input*() which will fill in the rt_iif field
appropriately.

The only exception to this would be packets which are looped back, in
which case the cached output route attached to the packet will be used.

Your RFC patch should work, but we're trying to make "struct rtable"
smaller rather than larger.

In what way does routing break if you simply restore the original
rt_iif assignment in output route creation?  That's the most preferred
fix for this.

Re: problem of "ipv4: revert Set rt->rt_iif more sanely on output routes."

[OGAWA Hirofumi]

David Miller <davem@davemloft.net> writes:

> From: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
> Date: Tue, 05 Apr 2011 22:05:10 +0900
>
>> ipv4: Set rt->rt_iif more sanely on output routes.
>> (1018b5c01636c7c6bda31a719bda34fc631db29a)
>> 
>> The above patch seems to be caused of avahi breakage.
>> 
>> I'm not debugging fully though, avahi is using IP_PKTINFO and checking
>> in_pktinfo->ipi_ifindex > 0.
>> 
>> And if I reverted above patch, it seems to fix avahi's IP_PKTINFO problem.
>
> in_pktinfo is given to the application only during recvmsg() calls, the
> call chain is (for example):
>
> udp_recvmsg()
> 	--> ip_cmsg_recv()
> 		--> ip_cmsg_recv_pktinfo()
>
> Therefore we will only be working with receive packets, whose routes are
> computed using ip_route_input*() which will fill in the rt_iif field
> appropriately.
>
> The only exception to this would be packets which are looped back, in
> which case the cached output route attached to the packet will be used.

I see.

> Your RFC patch should work, but we're trying to make "struct rtable"
> smaller rather than larger.

I felt it from git hisotry.

> In what way does routing break if you simply restore the original
> rt_iif assignment in output route creation?  That's the most preferred
> fix for this.

I'm not pretty sure though, output message is

	ip_finish_output2: No header cache and no neighbour!

I'm not debugging this though,

static inline bool rt_is_output_route(struct rtable *rt)
{
	return rt->rt_iif == 0;
}

from review I guess the above is one of cause.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: problem of "ipv4: revert Set rt->rt_iif more sanely on output routes."

[David Miller]

From: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
Date: Thu, 07 Apr 2011 13:31:06 +0900

> I'm not pretty sure though, output message is
> 
> 	ip_finish_output2: No header cache and no neighbour!
> 
> I'm not debugging this though,
> 
> static inline bool rt_is_output_route(struct rtable *rt)
> {
> 	return rt->rt_iif == 0;
> }
> 
> from review I guess the above is one of cause.

arp_bind_neighbour() is only called if rt_is_output_route() is true
or route is unicast.

If packet is sent using a route for which arp_bind_neighbour() has not
been called, you will see that warning message.

Re: problem of "ipv4: revert Set rt->rt_iif more sanely on output routes."

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Wed, 06 Apr 2011 22:34:00 -0700 (PDT)

> From: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
> Date: Thu, 07 Apr 2011 13:31:06 +0900
> 
>> I'm not pretty sure though, output message is
>> 
>> 	ip_finish_output2: No header cache and no neighbour!
>> 
>> I'm not debugging this though,
>> 
>> static inline bool rt_is_output_route(struct rtable *rt)
>> {
>> 	return rt->rt_iif == 0;
>> }
>> 
>> from review I guess the above is one of cause.
> 
> arp_bind_neighbour() is only called if rt_is_output_route() is true
> or route is unicast.
> 
> If packet is sent using a route for which arp_bind_neighbour() has not
> been called, you will see that warning message.

Ok, the problem is that, for output routes in original code:

1) user's flow device index is stored in rt->rt_iif

2) arp_bind_neighbour() tests meanwhile used rt->fl.iif

So we do need, for now, to add a new member.  But I think for
correct semantics it needs to have inverse meaning to the one
you added in your RFC patch.

So fix is something like:

1) Add "int rt_route_iif;" to struct rtable

2) For input routes, always set rt_route_iif to same value as rt_iif

3) For output routes, always set rt_route_iif to zero.  Set rt_iif
   as it is done currently.

4) Change rt_is_{output,input}_route() to test rt_route_iif

This should fix the bug and not introduce new regressions.

Can you write and test such a patch with your test case?

Thank you!

Re: problem of "ipv4: revert Set rt->rt_iif more sanely on output routes."

[OGAWA Hirofumi]

David Miller <davem@davemloft.net> writes:

> So fix is something like:
>
> 1) Add "int rt_route_iif;" to struct rtable
>
> 2) For input routes, always set rt_route_iif to same value as rt_iif
>
> 3) For output routes, always set rt_route_iif to zero.  Set rt_iif
>    as it is done currently.
>
> 4) Change rt_is_{output,input}_route() to test rt_route_iif
>
> This should fix the bug and not introduce new regressions.
>
> Can you write and test such a patch with your test case?

Ok. I'll try, but I'm not sure I understand the above correctly. Well,
I'll send the patch after testing.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: problem of "ipv4: revert Set rt->rt_iif more sanely on output routes."

[OGAWA Hirofumi]

OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes:

> David Miller <davem@davemloft.net> writes:
>
>> So fix is something like:
>>
>> 1) Add "int rt_route_iif;" to struct rtable
>>
>> 2) For input routes, always set rt_route_iif to same value as rt_iif
>>
>> 3) For output routes, always set rt_route_iif to zero.  Set rt_iif
>>    as it is done currently.
>>
>> 4) Change rt_is_{output,input}_route() to test rt_route_iif
>>
>> This should fix the bug and not introduce new regressions.
>>
>> Can you write and test such a patch with your test case?
>
> Ok. I'll try, but I'm not sure I understand the above correctly. Well,
> I'll send the patch after testing.

This patch seems to work for avahi-daemon without any warning.

BTW, the above meant change from (there was before) "fl.iif" to
"rt_route_iif"? If so, this patch is not enough. I'm not sure

+	rth->rt_route_iif = 0;
+	rth->rt_iif	= oldflp4->flowi4_oif ? : dev_out->ifindex;

is correct one or not. Please review.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: problem of "ipv4: revert Set rt->rt_iif more sanely on output routes."

[OGAWA Hirofumi]

OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes:

> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes:
>
>> David Miller <davem@davemloft.net> writes:
>>
>>> So fix is something like:
>>>
>>> 1) Add "int rt_route_iif;" to struct rtable
>>>
>>> 2) For input routes, always set rt_route_iif to same value as rt_iif
>>>
>>> 3) For output routes, always set rt_route_iif to zero.  Set rt_iif
>>>    as it is done currently.
>>>
>>> 4) Change rt_is_{output,input}_route() to test rt_route_iif
>>>
>>> This should fix the bug and not introduce new regressions.
>>>
>>> Can you write and test such a patch with your test case?
>>
>> Ok. I'll try, but I'm not sure I understand the above correctly. Well,
>> I'll send the patch after testing.
>
> This patch seems to work for avahi-daemon without any warning.
>
> BTW, the above meant change from (there was before) "fl.iif" to
> "rt_route_iif"? If so, this patch is not enough. I'm not sure
>
> +	rth->rt_route_iif = 0;
> +	rth->rt_iif	= oldflp4->flowi4_oif ? : dev_out->ifindex;
>
> is correct one or not. Please review.

Forgot the patch.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>


[PATCH] Fix  "Set rt->rt_iif more sanely on output routes."


1018b5c01636c7c6bda31a719bda34fc631db29a breaks rt_is_{output,input}_route.

This became the cause to return "IP_PKTINFO's ->ipi_ifindex == 0".

To fix it, this does

1) Add "int rt_route_iif;" to struct rtable

2) For input routes, always set rt_route_iif to same value as rt_iif

3) For output routes, always set rt_route_iif to zero.  Set rt_iif
   as it is done currently.

4) Change rt_is_{output,input}_route() to test rt_route_iif

Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 include/net/route.h     |    5 +++--
 net/ipv4/route.c        |    8 ++++++--
 net/ipv4/xfrm4_policy.c |    1 +
 3 files changed, 10 insertions(+), 4 deletions(-)

diff -puN include/net/route.h~revert-avahi-breaker include/net/route.h
--- linux-2.6/include/net/route.h~revert-avahi-breaker	2011-04-07 17:12:05.000000000 +0900
+++ linux-2.6-hirofumi/include/net/route.h	2011-04-07 17:12:05.000000000 +0900
@@ -64,6 +64,7 @@ struct rtable {
 
 	__be32			rt_dst;	/* Path destination	*/
 	__be32			rt_src;	/* Path source		*/
+	int			rt_route_iif;
 	int			rt_iif;
 	int			rt_oif;
 	__u32			rt_mark;
@@ -80,12 +81,12 @@ struct rtable {
 
 static inline bool rt_is_input_route(struct rtable *rt)
 {
-	return rt->rt_iif != 0;
+	return rt->rt_route_iif != 0;
 }
 
 static inline bool rt_is_output_route(struct rtable *rt)
 {
-	return rt->rt_iif == 0;
+	return rt->rt_route_iif == 0;
 }
 
 struct ip_rt_acct {
diff -puN net/ipv4/route.c~revert-avahi-breaker net/ipv4/route.c
--- linux-2.6/net/ipv4/route.c~revert-avahi-breaker	2011-04-07 17:12:05.000000000 +0900
+++ linux-2.6-hirofumi/net/ipv4/route.c	2011-04-07 17:12:05.000000000 +0900
@@ -1891,6 +1891,7 @@ static int ip_route_input_mc(struct sk_b
 #ifdef CONFIG_IP_ROUTE_CLASSID
 	rth->dst.tclassid = itag;
 #endif
+	rth->rt_route_iif = dev->ifindex;
 	rth->rt_iif	= dev->ifindex;
 	rth->dst.dev	= init_net.loopback_dev;
 	dev_hold(rth->dst.dev);
@@ -2026,6 +2027,7 @@ static int __mkroute_input(struct sk_buf
 	rth->rt_key_src	= saddr;
 	rth->rt_src	= saddr;
 	rth->rt_gateway	= daddr;
+	rth->rt_route_iif = in_dev->dev->ifindex;
 	rth->rt_iif 	= in_dev->dev->ifindex;
 	rth->dst.dev	= (out_dev)->dev;
 	dev_hold(rth->dst.dev);
@@ -2202,6 +2204,7 @@ local_input:
 #ifdef CONFIG_IP_ROUTE_CLASSID
 	rth->dst.tclassid = itag;
 #endif
+	rth->rt_route_iif = dev->ifindex;
 	rth->rt_iif	= dev->ifindex;
 	rth->dst.dev	= net->loopback_dev;
 	dev_hold(rth->dst.dev);
@@ -2401,7 +2404,8 @@ static struct rtable *__mkroute_output(c
 	rth->rt_mark    = oldflp4->flowi4_mark;
 	rth->rt_dst	= fl4->daddr;
 	rth->rt_src	= fl4->saddr;
-	rth->rt_iif	= 0;
+	rth->rt_route_iif = 0;
+	rth->rt_iif	= oldflp4->flowi4_oif ? : dev_out->ifindex;
 	/* get references to the devices that are to be hold by the routing
 	   cache entry */
 	rth->dst.dev	= dev_out;
@@ -2716,6 +2720,7 @@ struct dst_entry *ipv4_blackhole_route(s
 		rt->rt_key_dst = ort->rt_key_dst;
 		rt->rt_key_src = ort->rt_key_src;
 		rt->rt_tos = ort->rt_tos;
+		rt->rt_route_iif = ort->rt_route_iif;
 		rt->rt_iif = ort->rt_iif;
 		rt->rt_oif = ort->rt_oif;
 		rt->rt_mark = ort->rt_mark;
@@ -2725,7 +2730,6 @@ struct dst_entry *ipv4_blackhole_route(s
 		rt->rt_type = ort->rt_type;
 		rt->rt_dst = ort->rt_dst;
 		rt->rt_src = ort->rt_src;
-		rt->rt_iif = ort->rt_iif;
 		rt->rt_gateway = ort->rt_gateway;
 		rt->rt_spec_dst = ort->rt_spec_dst;
 		rt->peer = ort->peer;
diff -puN net/ipv4/xfrm4_policy.c~revert-avahi-breaker net/ipv4/xfrm4_policy.c
--- linux-2.6/net/ipv4/xfrm4_policy.c~revert-avahi-breaker	2011-04-07 17:12:05.000000000 +0900
+++ linux-2.6-hirofumi/net/ipv4/xfrm4_policy.c	2011-04-07 17:12:05.000000000 +0900
@@ -74,6 +74,7 @@ static int xfrm4_fill_dst(struct xfrm_ds
 	rt->rt_key_dst = fl4->daddr;
 	rt->rt_key_src = fl4->saddr;
 	rt->rt_tos = fl4->flowi4_tos;
+	rt->rt_route_iif = fl4->flowi4_iif;
 	rt->rt_iif = fl4->flowi4_iif;
 	rt->rt_oif = fl4->flowi4_oif;
 	rt->rt_mark = fl4->flowi4_mark;
_

Re: problem of "ipv4: revert Set rt->rt_iif more sanely on output routes."

[David Miller]

From: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
Date: Thu, 07 Apr 2011 17:32:49 +0900

> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes:
> 
>> This patch seems to work for avahi-daemon without any warning.
>>
>> BTW, the above meant change from (there was before) "fl.iif" to
>> "rt_route_iif"? If so, this patch is not enough. I'm not sure
>>
>> +	rth->rt_route_iif = 0;
>> +	rth->rt_iif	= oldflp4->flowi4_oif ? : dev_out->ifindex;
>>
>> is correct one or not. Please review.
> 
> Forgot the patch.

I am reviewing your patch right now, thank you so much for working
on this bug.

Re: problem of "ipv4: revert Set rt->rt_iif more sanely on output routes."

[David Miller]

From: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
Date: Thu, 07 Apr 2011 17:29:04 +0900

> I'm not sure
> 
> +	rth->rt_route_iif = 0;
> +	rth->rt_iif	= oldflp4->flowi4_oif ? : dev_out->ifindex;
> 
> is correct one or not. Please review.

I am pretty sure this is indeed correct.

In old code this read:

	rth->rt_iif	= oldflp->oif ? : dev_out->ifindex;

And rt->fl.iif was left without explicit setting, and thus left
as zero (dst_alloc() use to bzero entire rtable object).

So new code is equivalent and provides the necessary semantics.

I think your patch is correct and I will add it to net-2.6, thanks
again!