[REGRESSION,BISECTED] MIPv6 support broken by f4f914b58019f0

[REGRESSION,BISECTED] MIPv6 support broken by f4f914b58019f0

[Arnaud Ebalard]

Hi,

I just updated my laptop's kernel to 2.6.34 (previously running .33 and
configured to act as an IPsec/IKE-protected MIPv6 Mobile Node using
racoon and umip): after rebooting on the new kernel, the transport mode
SA protecting MIPv6 signaling traffic are missing.

I bisected the issue down to f4f914b58019f0e50d521bbbadfaee260d766f95
(net: ipv6 bind to device issue) which was added after 2.6.34-rc5: 

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index c2438e8..05ebd78 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -815,7 +815,7 @@ struct dst_entry * ip6_route_output(struct net *net, struct sock *sk,
 {
        int flags = 0;
 
-       if (rt6_need_strict(&fl->fl6_dst))
+       if (fl->oif || rt6_need_strict(&fl->fl6_dst))
                flags |= RT6_LOOKUP_F_IFACE;
 
        if (!ipv6_addr_any(&fl->fl6_src))

Reverting the patch on a 2.6.34 gives me a working kernel.

With MIPv6, the Home Address is bound to a tunnel interface but the
routing/XFRM code will not always send packet via this virtual device
(in fact, I would say never when IPsec is used for protecting signaling
and data traffic):

 - Signaling traffic will be sent using a Care-of Address from another
   interface (with the addition of a Home Address Option in a
   Destination Option Header)
 - Data traffic (when protected by tunnel mode IPsec) will also be sent
   via another interface.

I *suspect* that previous commit somehow changes the lose coupling
between the address and the device to enforce a strict routing via
associated interface.

I will try and take a look at the code tomorrow to understand what
really happens but if someone has ideas, I am interested.

Cheers,

a+

ps: I use the same working setup for all kernels since 2.6.28

Re: [REGRESSION,BISECTED] MIPv6 support broken by f4f914b58019f0

[Brian Haley]

On 05/26/2010 01:01 PM, Arnaud Ebalard wrote:
> Hi,
> 
> I just updated my laptop's kernel to 2.6.34 (previously running .33 and
> configured to act as an IPsec/IKE-protected MIPv6 Mobile Node using
> racoon and umip): after rebooting on the new kernel, the transport mode
> SA protecting MIPv6 signaling traffic are missing.
> 
> I bisected the issue down to f4f914b58019f0e50d521bbbadfaee260d766f95
> (net: ipv6 bind to device issue) which was added after 2.6.34-rc5: 
> 
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index c2438e8..05ebd78 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -815,7 +815,7 @@ struct dst_entry * ip6_route_output(struct net *net, struct sock *sk,
>  {
>         int flags = 0;
>  
> -       if (rt6_need_strict(&fl->fl6_dst))
> +       if (fl->oif || rt6_need_strict(&fl->fl6_dst))
>                 flags |= RT6_LOOKUP_F_IFACE;

Can you see if fl->oif is at least a sane value here?  Maybe there's some
partially un-initialized flowi getting passed-in, a quick source code check
didn't find anything obvious.

The other thought is that it's the tunnel code calling it, as it's going
to set 'oif' (actually it caches a whole flowi) from the tunnel parms ifindex/link
value.  It could have been setting it forever, but ip6_route_output() just
never enforced it until now.

My $.02.

-Brian

Re: [REGRESSION,BISECTED] MIPv6 support broken by f4f914b58019f0

[Arnaud Ebalard]

Hi,

Thanks for your reply Brian and sorry for the length of this response. If
Hideaki and David can comment on the IPv6/XFRM and SO_BINDTODEVICE
aspects discussed below that would be helpful, IMHO.

Brian Haley <brian.haley@hp.com> writes:

> On 05/26/2010 01:01 PM, Arnaud Ebalard wrote:
>> Hi,
>> 
>> I just updated my laptop's kernel to 2.6.34 (previously running .33 and
>> configured to act as an IPsec/IKE-protected MIPv6 Mobile Node using
>> racoon and umip): after rebooting on the new kernel, the transport mode
>> SA protecting MIPv6 signaling traffic are missing.
>> 
>> I bisected the issue down to f4f914b58019f0e50d521bbbadfaee260d766f95
>> (net: ipv6 bind to device issue) which was added after 2.6.34-rc5: 
>> 
>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>> index c2438e8..05ebd78 100644
>> --- a/net/ipv6/route.c
>> +++ b/net/ipv6/route.c
>> @@ -815,7 +815,7 @@ struct dst_entry * ip6_route_output(struct net *net, struct sock *sk,
>>  {
>>         int flags = 0;
>>  
>> -       if (rt6_need_strict(&fl->fl6_dst))
>> +       if (fl->oif || rt6_need_strict(&fl->fl6_dst))
>>                 flags |= RT6_LOOKUP_F_IFACE;
>
> Can you see if fl->oif is at least a sane value here?  Maybe there's some
> partially un-initialized flowi getting passed-in, a quick source code check
> didn't find anything obvious.

When it's not 0, fl->oif is a sane value: it is set to the index of the
interface on which the current *Care-of Address* is configured. All the
traffic is expected to leave the host via this interface. 

> The other thought is that it's the tunnel code calling it, as it's going
> to set 'oif' (actually it caches a whole flowi) from the tunnel parms ifindex/link
> value.  It could have been setting it forever, but ip6_route_output() just
> never enforced it until now.

I added some printk in the code of ip6_route_output(), rt6_score_route()
and find_rr_leaf(). Below are respectivevly what I get for a 2.6.34 with
and without f4f914b58019f0e50d521bbbadfaee260d766f95. I removed the
beginning as it is the same and only started when it starts diverging.:

...
ip6_route_output() called from ip6_dst_lookup_tail() 1
ip6_route_output: fl->oif is wlan0
2001:XXXX:XXXX:0002:020d:93ff:fe55:f897 (HoA) => 2001:XXXX:XXXX:f002:021e:0bff:fe4e:04b5 (HA@) proto 135
rt6_score_route: oif is wlan0. rt->rt6i_dev->ifindex: lo. Leaving due to strict.
rt6_score_route: oif is wlan0. rt->rt6i_dev->ifindex: lo. Leaving due to strict.
rt6_score_route: oif is wlan0. rt->rt6i_dev->ifindex: ip6tnl1. Leaving due to strict.
rt6_score_route: oif is wlan0. rt->rt6i_dev->ifindex: ip6tnl1. Leaving due to strict.
...

On a working kernel:

...
ip6_route_output() called from ip6_dst_lookup_tail() 1
ip6_route_output: fl->oif is wlan0
2001:XXXX:XXXX:0002:020d:93ff:fe55:f897 (HoA) => 2001:XXXX:XXXX:f002:021e:0bff:fe4e:04b5 (HA@) proto 135
find_rr_leaf: match is 1. oif is wlan0
find_rr_leaf: match is 1. oif is wlan0
find_rr_leaf: match is 8. oif is wlan0
ip6_route_output() called from ip6_dst_lookup_tail() 1
ip6_route_output: fl->oif is 0
...

Above, a Binding Update message (a Mobility Header (proto 135) type 5)
has to be sent to the Home Agent. It is expected to leave the system via
the wlan0 interface, which is the interface on which the Care-of Address
of the packet is configured. The *wire* format of the packet is the
following:   

 IPv6(src=CoA, dst=HA@)/DestOpt(HoA)/ESP()/MH(type=5)

The addition of Destination Option header (containing a Home Address
Option) and ESP extension header is performed via XFRM. Initially, the
packet created by userland looks like this:

 IPv6(src=HoA, dst=HA@)/MH(type=5)

In previous debug outputs, the content of the fl->oif is ok, i.e. it is
set to the interface on which the CoA is configured, i.e. the output
interface. But the commit results in flags |= RT6_LOOKUP_F_IFACE.
Later, in rt6_score_route(), the call to rt6_check_dev() returns 0
(dev->ifindex is ip6tnl1 but oif is wlan0). Because of the change to flags 
flags, we quickly return -1 in rt6_score_route():

static int rt6_score_route(struct rt6_info *rt, int oif,
			   int strict)
{
	int m, n;

	m = rt6_check_dev(rt, oif);
	if (!m && (strict & RT6_LOOKUP_F_IFACE))
                return -1;
        ...

Now, I wonder if the following is correct. Don't hesitate to correct me
if I am wrong:

Initially (before f4f914b58019f0), the purpose of the test using
rt6_need_strict() in ip6_route_output() (introduced by c71099ac) was to
allow the multiple routing table logic to be applied to all global
addresses but to preserve the addresses for which it would not make
sense (link-local, multicast, ). The change introduced by f4f914b58019f0
basically reduces the ability to route traffic as you want and forces
the traffic to leave the device by the interface on which it is
configured (if fl->oif is set). 

>From my (very limited and possibly wrong) understanding, the change
introduced by f4f914b58019f0 looks like a workaround for the 
SO_BINDTODEVICE issue. Looking at the code, there is something I don't
understand: if SO_BINDTODEVICE has been used on a socket, the socket
should have its sk_bound_dev_if attribute set to the correct ifindex
value. Hence the following (naive) question: why is that information not
used to inflect the selection of the route cached for the socket? And
why would the fix be at the adress level instead of being at the
interface level (ifindex)?

Cheers,

a+

Re: [REGRESSION,BISECTED] MIPv6 support broken by f4f914b58019f0

[YOSHIFUJI Hideaki]

Hi,

Brian Haley wrote:
> On 05/26/2010 01:01 PM, Arnaud Ebalard wrote:
>> Hi,
>>
>> I just updated my laptop's kernel to 2.6.34 (previously running .33 and
>> configured to act as an IPsec/IKE-protected MIPv6 Mobile Node using
>> racoon and umip): after rebooting on the new kernel, the transport mode
>> SA protecting MIPv6 signaling traffic are missing.
>>
>> I bisected the issue down to f4f914b58019f0e50d521bbbadfaee260d766f95
>> (net: ipv6 bind to device issue) which was added after 2.6.34-rc5: 
>>
>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>> index c2438e8..05ebd78 100644
>> --- a/net/ipv6/route.c
>> +++ b/net/ipv6/route.c
>> @@ -815,7 +815,7 @@ struct dst_entry * ip6_route_output(struct net *net, struct sock *sk,
>>  {
>>         int flags = 0;
>>  
>> -       if (rt6_need_strict(&fl->fl6_dst))
>> +       if (fl->oif || rt6_need_strict(&fl->fl6_dst))
>>                 flags |= RT6_LOOKUP_F_IFACE;
> 
> Can you see if fl->oif is at least a sane value here?  Maybe there's some
> partially un-initialized flowi getting passed-in, a quick source code check
> didn't find anything obvious.
> 
> The other thought is that it's the tunnel code calling it, as it's going
> to set 'oif' (actually it caches a whole flowi) from the tunnel parms ifindex/link
> value.  It could have been setting it forever, but ip6_route_output() just
> never enforced it until now.

Well, I'd like to rethink the original bug report / fix.
There are several factors:

1) CONFIG_IPV6_ROUTER_PREF?
2) Is it host, or router?
3) next-hop reachability

If CONFIG_IPV6_ROUTER_PREF is enabled and the node is host,
and one nexthop has better reachability, the route is always
preferred even if upper layer specified specific interface.
If we do not like this behavior, we should change
rt6_score_route() not to return -1 something like this:

         n = rt6_check_neigh(rt);
         if (!n && (strict & RT6_LOOKUP_F_REACHABLE) && !oif)
                 return -1;

instead of ip6_route_output().

--yoshfuji

Re: [REGRESSION,BISECTED] MIPv6 support broken by f4f914b58019f0

[Brian Haley]

Hi Arnoud,

On 05/27/2010 11:14 AM, Arnaud Ebalard wrote:
> Hi,
> 
> Thanks for your reply Brian and sorry for the length of this response. If
> Hideaki and David can comment on the IPv6/XFRM and SO_BINDTODEVICE
> aspects discussed below that would be helpful, IMHO.

Thanks for all the analysis, comments below.

>> On 05/26/2010 01:01 PM, Arnaud Ebalard wrote:
>>> Hi,
>>>
>>> I just updated my laptop's kernel to 2.6.34 (previously running .33 and
>>> configured to act as an IPsec/IKE-protected MIPv6 Mobile Node using
>>> racoon and umip): after rebooting on the new kernel, the transport mode
>>> SA protecting MIPv6 signaling traffic are missing.
>>>
>>> I bisected the issue down to f4f914b58019f0e50d521bbbadfaee260d766f95
>>> (net: ipv6 bind to device issue) which was added after 2.6.34-rc5: 
>>>
>>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>>> index c2438e8..05ebd78 100644
>>> --- a/net/ipv6/route.c
>>> +++ b/net/ipv6/route.c
>>> @@ -815,7 +815,7 @@ struct dst_entry * ip6_route_output(struct net *net, struct sock *sk,
>>>  {
>>>         int flags = 0;
>>>  
>>> -       if (rt6_need_strict(&fl->fl6_dst))
>>> +       if (fl->oif || rt6_need_strict(&fl->fl6_dst))
>>>                 flags |= RT6_LOOKUP_F_IFACE;
>>
>> Can you see if fl->oif is at least a sane value here?  Maybe there's some
>> partially un-initialized flowi getting passed-in, a quick source code check
>> didn't find anything obvious.
> 
> When it's not 0, fl->oif is a sane value: it is set to the index of the
> interface on which the current *Care-of Address* is configured. All the
> traffic is expected to leave the host via this interface. 

Ok, so it's not un-initialized data causing this.

> In previous debug outputs, the content of the fl->oif is ok, i.e. it is
> set to the interface on which the CoA is configured, i.e. the output
> interface. But the commit results in flags |= RT6_LOOKUP_F_IFACE.
> Later, in rt6_score_route(), the call to rt6_check_dev() returns 0
> (dev->ifindex is ip6tnl1 but oif is wlan0). Because of the change to flags 
> flags, we quickly return -1 in rt6_score_route():

Ok, so the call to ip6_route_output() was from the tunnel code, which is
using it's cached flowi, which has oif set to the tunnel.  The XFRM code
swaps the addresses, which should invalidate the oif, but it doesn't.

> static int rt6_score_route(struct rt6_info *rt, int oif,
> 			   int strict)
> {
> 	int m, n;
> 
> 	m = rt6_check_dev(rt, oif);
> 	if (!m && (strict & RT6_LOOKUP_F_IFACE))
>                 return -1;
>         ...
> 
> Now, I wonder if the following is correct. Don't hesitate to correct me
> if I am wrong:
> 
> Initially (before f4f914b58019f0), the purpose of the test using
> rt6_need_strict() in ip6_route_output() (introduced by c71099ac) was to
> allow the multiple routing table logic to be applied to all global
> addresses but to preserve the addresses for which it would not make
> sense (link-local, multicast, ). The change introduced by f4f914b58019f0
> basically reduces the ability to route traffic as you want and forces
> the traffic to leave the device by the interface on which it is
> configured (if fl->oif is set).

The problem is we assumed the caller's would only set fl->oif if they
wanted it enforced (multicast, link-local, SO_BINDTODEVICE), but it
didn't take into account the tunnel code.  I guess the easy answer
would be to revert this until we can fix it correctly.

> From my (very limited and possibly wrong) understanding, the change
> introduced by f4f914b58019f0 looks like a workaround for the 
> SO_BINDTODEVICE issue. Looking at the code, there is something I don't
> understand: if SO_BINDTODEVICE has been used on a socket, the socket
> should have its sk_bound_dev_if attribute set to the correct ifindex
> value. Hence the following (naive) question: why is that information not
> used to inflect the selection of the route cached for the socket? And
> why would the fix be at the adress level instead of being at the
> interface level (ifindex)?

I guess I always believed setting SO_BINDTODEVICE should always force
traffic out that interface, but from Yoshifuji's email it seems that
maybe wasn't the intention, at least for things that don't meet
the rt_need_strict() criteria like globals.  I don't know the history
behind the setsockopt.

The below might actually be what was actually intended, triggering
on what the user forced, rather than assuming all callers require
strict behavior.

-Brian


diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 294cbe8..252d761 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -814,7 +814,7 @@ struct dst_entry * ip6_route_output(struct net *net, struct sock *sk,
 {
 	int flags = 0;
 
-	if (fl->oif || rt6_need_strict(&fl->fl6_dst))
+	if ((sk && sk->sk_bound_dev_if) || rt6_need_strict(&fl->fl6_dst))
 		flags |= RT6_LOOKUP_F_IFACE;
 
 	if (!ipv6_addr_any(&fl->fl6_src))

Re: [REGRESSION,BISECTED] MIPv6 support broken by f4f914b58019f0

[Arnaud Ebalard]

Hi,

Brian Haley <brian.haley@hp.com> writes:

>> In previous debug outputs, the content of the fl->oif is ok, i.e. it is
>> set to the interface on which the CoA is configured, i.e. the output
>> interface. But the commit results in flags |= RT6_LOOKUP_F_IFACE.
>> Later, in rt6_score_route(), the call to rt6_check_dev() returns 0
>> (dev->ifindex is ip6tnl1 but oif is wlan0). Because of the change to flags 
>> flags, we quickly return -1 in rt6_score_route():
>
> Ok, so the call to ip6_route_output() was from the tunnel code, which is
> using it's cached flowi, which has oif set to the tunnel.  The XFRM code
> swaps the addresses, which should invalidate the oif, but it doesn't.
>
>> static int rt6_score_route(struct rt6_info *rt, int oif,
>> 			   int strict)
>> {
>> 	int m, n;
>> 
>> 	m = rt6_check_dev(rt, oif);
>> 	if (!m && (strict & RT6_LOOKUP_F_IFACE))
>>                 return -1;
>>         ...
>> 
>> Now, I wonder if the following is correct. Don't hesitate to correct me
>> if I am wrong:
>> 
>> Initially (before f4f914b58019f0), the purpose of the test using
>> rt6_need_strict() in ip6_route_output() (introduced by c71099ac) was to
>> allow the multiple routing table logic to be applied to all global
>> addresses but to preserve the addresses for which it would not make
>> sense (link-local, multicast, ). The change introduced by f4f914b58019f0
>> basically reduces the ability to route traffic as you want and forces
>> the traffic to leave the device by the interface on which it is
>> configured (if fl->oif is set).
>
> The problem is we assumed the caller's would only set fl->oif if they
> wanted it enforced (multicast, link-local, SO_BINDTODEVICE), but it
> didn't take into account the tunnel code.  I guess the easy answer
> would be to revert this until we can fix it correctly.

Nothing against it but maybe Jiri or David have other ideas.


>> From my (very limited and possibly wrong) understanding, the change
>> introduced by f4f914b58019f0 looks like a workaround for the 
>> SO_BINDTODEVICE issue. Looking at the code, there is something I don't
>> understand: if SO_BINDTODEVICE has been used on a socket, the socket
>> should have its sk_bound_dev_if attribute set to the correct ifindex
>> value. Hence the following (naive) question: why is that information not
>> used to inflect the selection of the route cached for the socket? And
>> why would the fix be at the adress level instead of being at the
>> interface level (ifindex)?
>
> I guess I always believed setting SO_BINDTODEVICE should always force
> traffic out that interface, but from Yoshifuji's email it seems that
> maybe wasn't the intention, at least for things that don't meet
> the rt_need_strict() criteria like globals.  I don't know the history
> behind the setsockopt.

The behavior I would expect from a combination of RFC 4191 and
SO_BINDTODEVICE sockopt would be the use of the interface as outgoing
interface and then the use of the best router (using router preference
info, reachability, ...) available on the subnet. IIRC, the router
preference info is per default router list in the RFC, i.e. per
interface. 


> The below might actually be what was actually intended, triggering
> on what the user forced, rather than assuming all callers require
> strict behavior.
>
> -Brian
>
>
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 294cbe8..252d761 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -814,7 +814,7 @@ struct dst_entry * ip6_route_output(struct net *net, struct sock *sk,
>  {
>  	int flags = 0;
>  
> -	if (fl->oif || rt6_need_strict(&fl->fl6_dst))
> +	if ((sk && sk->sk_bound_dev_if) || rt6_need_strict(&fl->fl6_dst))
>  		flags |= RT6_LOOKUP_F_IFACE;
>  
>  	if (!ipv6_addr_any(&fl->fl6_src))

Just for fun, I'll test it tomorrow. I think this would be a better
alternative to a simple revert.

Cheers,

a+

Re: [REGRESSION,BISECTED] MIPv6 support broken by f4f914b58019f0

[Scott C Otto]

All,
Thanks for looking into this.

The behavior of SO_BINDTODEVICE, certainly with IPV4, is to identify a specific
interface to use for sending/receiving AF_INET packets upon.   And that's how it
has behaved with IPV4.   Tools like ping, traceroute (and ping6, traceroute6)
make use of it with the -i (interface) options.

We ran into this issue when updating an IPV4 application to IPV6.   The report
provided one example of the issue.

The original change was based on the semantics of flowi.oif used in IPV4.  There
flowi.oif (as it is with IPV6 as well) is also set to sk->sk_bound_dev_if when
using ip_route_connect() and routing simply took a non-zero oif to enforce an
interface.  Looking at IPV4 tunneling, flowi.oif is set the same way from
tunnel's parms.link as with IPV6 so would follow those semantics as well.

Obviously, with IPV6, the semantics of flowi.oif are different and perhaps even
vary with the users of IPV6.

If that variability is desired, the proposed change from Brian (using
sk->sk_bound_dev_if) would be an equivalent means of supporting SO_BINDTODEVICE
while limiting the impact.

Scott

On 5/27/2010 2:39 PM, Brian Haley wrote:
> Hi Arnoud,
> 
> On 05/27/2010 11:14 AM, Arnaud Ebalard wrote:
> 
>>Hi,
>>
>>Thanks for your reply Brian and sorry for the length of this response. If
>>Hideaki and David can comment on the IPv6/XFRM and SO_BINDTODEVICE
>>aspects discussed below that would be helpful, IMHO.
> 
> 
> Thanks for all the analysis, comments below.
> 
> 
>>>On 05/26/2010 01:01 PM, Arnaud Ebalard wrote:
>>>
>>>>Hi,
>>>>
>>>>I just updated my laptop's kernel to 2.6.34 (previously running .33 and
>>>>configured to act as an IPsec/IKE-protected MIPv6 Mobile Node using
>>>>racoon and umip): after rebooting on the new kernel, the transport mode
>>>>SA protecting MIPv6 signaling traffic are missing.
>>>>
>>>>I bisected the issue down to f4f914b58019f0e50d521bbbadfaee260d766f95
>>>>(net: ipv6 bind to device issue) which was added after 2.6.34-rc5: 
>>>>
>>>>diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>>>>index c2438e8..05ebd78 100644
>>>>--- a/net/ipv6/route.c
>>>>+++ b/net/ipv6/route.c
>>>>@@ -815,7 +815,7 @@ struct dst_entry * ip6_route_output(struct net *net, struct sock *sk,
>>>> {
>>>>        int flags = 0;
>>>> 
>>>>-       if (rt6_need_strict(&fl->fl6_dst))
>>>>+       if (fl->oif || rt6_need_strict(&fl->fl6_dst))
>>>>                flags |= RT6_LOOKUP_F_IFACE;
>>>
>>>Can you see if fl->oif is at least a sane value here?  Maybe there's some
>>>partially un-initialized flowi getting passed-in, a quick source code check
>>>didn't find anything obvious.
>>
>>When it's not 0, fl->oif is a sane value: it is set to the index of the
>>interface on which the current *Care-of Address* is configured. All the
>>traffic is expected to leave the host via this interface. 
> 
> 
> Ok, so it's not un-initialized data causing this.
> 
> 
>>In previous debug outputs, the content of the fl->oif is ok, i.e. it is
>>set to the interface on which the CoA is configured, i.e. the output
>>interface. But the commit results in flags |= RT6_LOOKUP_F_IFACE.
>>Later, in rt6_score_route(), the call to rt6_check_dev() returns 0
>>(dev->ifindex is ip6tnl1 but oif is wlan0). Because of the change to flags 
>>flags, we quickly return -1 in rt6_score_route():
> 
> 
> Ok, so the call to ip6_route_output() was from the tunnel code, which is
> using it's cached flowi, which has oif set to the tunnel.  The XFRM code
> swaps the addresses, which should invalidate the oif, but it doesn't.
> 
> 
>>static int rt6_score_route(struct rt6_info *rt, int oif,
>>			   int strict)
>>{
>>	int m, n;
>>
>>	m = rt6_check_dev(rt, oif);
>>	if (!m && (strict & RT6_LOOKUP_F_IFACE))
>>                return -1;
>>        ...
>>
>>Now, I wonder if the following is correct. Don't hesitate to correct me
>>if I am wrong:
>>
>>Initially (before f4f914b58019f0), the purpose of the test using
>>rt6_need_strict() in ip6_route_output() (introduced by c71099ac) was to
>>allow the multiple routing table logic to be applied to all global
>>addresses but to preserve the addresses for which it would not make
>>sense (link-local, multicast, ). The change introduced by f4f914b58019f0
>>basically reduces the ability to route traffic as you want and forces
>>the traffic to leave the device by the interface on which it is
>>configured (if fl->oif is set).
> 
> 
> The problem is we assumed the caller's would only set fl->oif if they
> wanted it enforced (multicast, link-local, SO_BINDTODEVICE), but it
> didn't take into account the tunnel code.  I guess the easy answer
> would be to revert this until we can fix it correctly.
> 
> 
>>From my (very limited and possibly wrong) understanding, the change
>>introduced by f4f914b58019f0 looks like a workaround for the 
>>SO_BINDTODEVICE issue. Looking at the code, there is something I don't
>>understand: if SO_BINDTODEVICE has been used on a socket, the socket
>>should have its sk_bound_dev_if attribute set to the correct ifindex
>>value. Hence the following (naive) question: why is that information not
>>used to inflect the selection of the route cached for the socket? And
>>why would the fix be at the adress level instead of being at the
>>interface level (ifindex)?
> 
> 
> I guess I always believed setting SO_BINDTODEVICE should always force
> traffic out that interface, but from Yoshifuji's email it seems that
> maybe wasn't the intention, at least for things that don't meet
> the rt_need_strict() criteria like globals.  I don't know the history
> behind the setsockopt.
> 
> The below might actually be what was actually intended, triggering
> on what the user forced, rather than assuming all callers require
> strict behavior.
> 
> -Brian
> 
> 
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 294cbe8..252d761 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -814,7 +814,7 @@ struct dst_entry * ip6_route_output(struct net *net, struct sock *sk,
>  {
>  	int flags = 0;
>  
> -	if (fl->oif || rt6_need_strict(&fl->fl6_dst))
> +	if ((sk && sk->sk_bound_dev_if) || rt6_need_strict(&fl->fl6_dst))
>  		flags |= RT6_LOOKUP_F_IFACE;
>  
>  	if (!ipv6_addr_any(&fl->fl6_src))

Re: [REGRESSION,BISECTED] MIPv6 support broken by f4f914b58019f0

[Arnaud Ebalard]

Hi,

Scott C Otto <otts@alcatel-lucent.com> writes:

> All,
> Thanks for looking into this.
>
> The behavior of SO_BINDTODEVICE, certainly with IPV4, is to identify a specific
> interface to use for sending/receiving AF_INET packets upon.   And that's how it
> has behaved with IPV4.   Tools like ping, traceroute (and ping6, traceroute6)
> make use of it with the -i (interface) options.
>
> We ran into this issue when updating an IPV4 application to IPV6.   The report
> provided one example of the issue.
>
> The original change was based on the semantics of flowi.oif used in IPV4.  There
> flowi.oif (as it is with IPV6 as well) is also set to sk->sk_bound_dev_if when
> using ip_route_connect() and routing simply took a non-zero oif to enforce an
> interface.  Looking at IPV4 tunneling, flowi.oif is set the same way from
> tunnel's parms.link as with IPV6 so would follow those semantics as well.
>
> Obviously, with IPV6, the semantics of flowi.oif are different and perhaps even
> vary with the users of IPV6.
>
> If that variability is desired, the proposed change from Brian (using
> sk->sk_bound_dev_if) would be an equivalent means of supporting SO_BINDTODEVICE
> while limiting the impact.

ok. Thanks for the feedback. Can you comment on what is below?

>> The below might actually be what was actually intended, triggering
>> on what the user forced, rather than assuming all callers require
>> strict behavior.
>> 
>> -Brian
>> 
>> 
>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>> index 294cbe8..252d761 100644
>> --- a/net/ipv6/route.c
>> +++ b/net/ipv6/route.c
>> @@ -814,7 +814,7 @@ struct dst_entry * ip6_route_output(struct net *net, struct sock *sk,
>>  {
>>  	int flags = 0;
>>  
>> -	if (fl->oif || rt6_need_strict(&fl->fl6_dst))
>> +	if ((sk && sk->sk_bound_dev_if) || rt6_need_strict(&fl->fl6_dst))
>>  		flags |= RT6_LOOKUP_F_IFACE;
>>  
>>  	if (!ipv6_addr_any(&fl->fl6_src))

Brian, I tested the patch on my Mobile Node: it fixes the regression. I
also updated the kernel on my Home Agent to a 2.6.34 with that fix and
everything works as expected. *For that aspect* and fwiw, you get my

Tested-by: Arnaud Ebalard <arno@natisbad.org>

For the SO_BINDTODEVICE aspect, I don't have code at hand to test if the
fix works as expected. We should also double check that this will not
break other paths which use the sk->sk_bound_dev_if with a different
semantic:

$ grep -R sk_bound_dev_if net/ | wc -l
125
$ grep -R 'sk_bound_dev_if = ' net/
net/ieee802154/raw.c:   sk->sk_bound_dev_if = dev->ifindex;
net/core/sock.c:        sk->sk_bound_dev_if = index;
net/ipv6/datagram.c:    sk->sk_bound_dev_if = usin->sin6_scope_id;
net/ipv6/datagram.c:    sk->sk_bound_dev_if = np->mcast_oif;
net/ipv6/af_inet6.c:    sk->sk_bound_dev_if = addr->sin6_scope_id;
net/ipv6/tcp_ipv6.c:    sk->sk_bound_dev_if = usin->sin6_scope_id;
net/ipv6/tcp_ipv6.c:    newsk->sk_bound_dev_if = treq->iif;
net/ipv6/raw.c:         sk->sk_bound_dev_if = addr->sin6_scope_id;
net/sctp/socket.c:      newsk->sk_bound_dev_if = sk->sk_bound_dev_if;
net/ipv4/ip_output.c:   sk->sk_bound_dev_if = arg->bound_dev_if;
net/ipv4/udp.c:         sk->sk_bound_dev_if = 0;
net/dccp/ipv6.c:        newsk->sk_bound_dev_if = ireq6->iif;
net/dccp/ipv6.c:        sk->sk_bound_dev_if = usin->sin6_scope_id;

Cheers,

a+

Re: [REGRESSION,BISECTED] MIPv6 support broken by f4f914b58019f0

[Brian Haley]

On 05/28/2010 04:51 AM, Arnaud Ebalard wrote:
>>> The below might actually be what was actually intended, triggering
>>> on what the user forced, rather than assuming all callers require
>>> strict behavior.
>>>
>>> -Brian
>>>
>>>
>>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>>> index 294cbe8..252d761 100644
>>> --- a/net/ipv6/route.c
>>> +++ b/net/ipv6/route.c
>>> @@ -814,7 +814,7 @@ struct dst_entry * ip6_route_output(struct net *net, struct sock *sk,
>>>  {
>>>  	int flags = 0;
>>>  
>>> -	if (fl->oif || rt6_need_strict(&fl->fl6_dst))
>>> +	if ((sk && sk->sk_bound_dev_if) || rt6_need_strict(&fl->fl6_dst))
>>>  		flags |= RT6_LOOKUP_F_IFACE;
>>>  
>>>  	if (!ipv6_addr_any(&fl->fl6_src))
> 
> Brian, I tested the patch on my Mobile Node: it fixes the regression. I
> also updated the kernel on my Home Agent to a 2.6.34 with that fix and
> everything works as expected. *For that aspect* and fwiw, you get my
> 
> Tested-by: Arnaud Ebalard <arno@natisbad.org>

Ok, thanks for testing, I'll send out an updated patch, with the caveat
there still might be a DCCP regression, see below.

> For the SO_BINDTODEVICE aspect, I don't have code at hand to test if the
> fix works as expected. We should also double check that this will not
> break other paths which use the sk->sk_bound_dev_if with a different
> semantic:

> net/ieee802154/raw.c:   sk->sk_bound_dev_if = dev->ifindex;

Isn't AF_INET6, OK.

> net/core/sock.c:        sk->sk_bound_dev_if = index;

The actual setsockopt() call, OK.

> net/ipv6/datagram.c:    sk->sk_bound_dev_if = usin->sin6_scope_id;
> net/ipv6/datagram.c:    sk->sk_bound_dev_if = np->mcast_oif;
> net/ipv6/af_inet6.c:    sk->sk_bound_dev_if = addr->sin6_scope_id;
> net/ipv6/tcp_ipv6.c:    sk->sk_bound_dev_if = usin->sin6_scope_id;
> net/ipv6/tcp_ipv6.c:    newsk->sk_bound_dev_if = treq->iif;
> net/ipv6/raw.c:         sk->sk_bound_dev_if = addr->sin6_scope_id;

This are all in link-local or multicast code, caller passing-in scopeid,
would trigger rt6_need_strict() regardless, so are OK.

> net/sctp/socket.c:      newsk->sk_bound_dev_if = sk->sk_bound_dev_if;

SCTP peeling-off a socket, cloning parent, OK.

> net/ipv4/ip_output.c:   sk->sk_bound_dev_if = arg->bound_dev_if;
> net/ipv4/udp.c:         sk->sk_bound_dev_if = 0;

IPv4, shouldn't be affected by an IPv6 route change, OK.

> net/dccp/ipv6.c:        newsk->sk_bound_dev_if = ireq6->iif;

This is the only mystery in this list.  Looks like the DCCP accept()
codepath, and it's getting bound to the interface the request was
received on.  I'm not sure if the intention here was to force this
strict behavior or not.

> net/dccp/ipv6.c:        sk->sk_bound_dev_if = usin->sin6_scope_id;

In link-local code, caller passing-in scopeid, would trigger
rt6_need_strict() regardless, so OK.

-Brian

[PATCH] IPv6: fix Mobile IPv6 regression

[Brian Haley]

Commit f4f914b5 (net: ipv6 bind to device issue) caused
a regression with Mobile IPv6 when it changed the meaning
of fl->oif to become a strict requirement of the route
lookup.  Instead, only force strict mode when
sk->sk_bound_dev_if is set on the calling socket, getting
the intended behavior and fixing the regression.

Tested-by: Arnaud Ebalard <arno@natisbad.org>
Signed-off-by: Brian Haley <brian.haley@hp.com>

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 294cbe8..252d761 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -814,7 +814,7 @@ struct dst_entry * ip6_route_output(struct net *net, struct sock *sk,
 {
 	int flags = 0;
 
-	if (fl->oif || rt6_need_strict(&fl->fl6_dst))
+	if ((sk && sk->sk_bound_dev_if) || rt6_need_strict(&fl->fl6_dst))
 		flags |= RT6_LOOKUP_F_IFACE;
 
 	if (!ipv6_addr_any(&fl->fl6_src))

Re: [REGRESSION,BISECTED] MIPv6 support broken by f4f914b58019f0

[YOSHIFUJI Hideaki]

Hello.

(2010/05/28 6:01), Arnaud Ebalard wrote:
> Hi,
>
> Brian Haley<brian.haley@hp.com>  writes:
>
>>> In previous debug outputs, the content of the fl->oif is ok, i.e. it is
>>> set to the interface on which the CoA is configured, i.e. the output
>>> interface. But the commit results in flags |= RT6_LOOKUP_F_IFACE.
>>> Later, in rt6_score_route(), the call to rt6_check_dev() returns 0
>>> (dev->ifindex is ip6tnl1 but oif is wlan0). Because of the change to flags
>>> flags, we quickly return -1 in rt6_score_route():
>>
>> Ok, so the call to ip6_route_output() was from the tunnel code, which is
>> using it's cached flowi, which has oif set to the tunnel.  The XFRM code
>> swaps the addresses, which should invalidate the oif, but it doesn't.
>>
>>> static int rt6_score_route(struct rt6_info *rt, int oif,
>>> 			   int strict)
>>> {
>>> 	int m, n;
>>>
>>> 	m = rt6_check_dev(rt, oif);
>>> 	if (!m&&  (strict&  RT6_LOOKUP_F_IFACE))
>>>                  return -1;
>>>          ...
>>>
>>> Now, I wonder if the following is correct. Don't hesitate to correct me
>>> if I am wrong:
>>>
>>> Initially (before f4f914b58019f0), the purpose of the test using
>>> rt6_need_strict() in ip6_route_output() (introduced by c71099ac) was to
>>> allow the multiple routing table logic to be applied to all global
>>> addresses but to preserve the addresses for which it would not make
>>> sense (link-local, multicast, ). The change introduced by f4f914b58019f0
>>> basically reduces the ability to route traffic as you want and forces
>>> the traffic to leave the device by the interface on which it is
>>> configured (if fl->oif is set).
>>
>> The problem is we assumed the caller's would only set fl->oif if they
>> wanted it enforced (multicast, link-local, SO_BINDTODEVICE), but it
>> didn't take into account the tunnel code.  I guess the easy answer
>> would be to revert this until we can fix it correctly.
>
> Nothing against it but maybe Jiri or David have other ideas.
>
>
>>>  From my (very limited and possibly wrong) understanding, the change
>>> introduced by f4f914b58019f0 looks like a workaround for the
>>> SO_BINDTODEVICE issue. Looking at the code, there is something I don't
>>> understand: if SO_BINDTODEVICE has been used on a socket, the socket
>>> should have its sk_bound_dev_if attribute set to the correct ifindex
>>> value. Hence the following (naive) question: why is that information not
>>> used to inflect the selection of the route cached for the socket? And
>>> why would the fix be at the adress level instead of being at the
>>> interface level (ifindex)?
>>
>> I guess I always believed setting SO_BINDTODEVICE should always force
>> traffic out that interface, but from Yoshifuji's email it seems that
>> maybe wasn't the intention, at least for things that don't meet
>> the rt_need_strict() criteria like globals.  I don't know the history
>> behind the setsockopt.
>
> The behavior I would expect from a combination of RFC 4191 and
> SO_BINDTODEVICE sockopt would be the use of the interface as outgoing
> interface and then the use of the best router (using router preference
> info, reachability, ...) available on the subnet. IIRC, the router
> preference info is per default router list in the RFC, i.e. per
> interface.

Good point.

Whatever our original intention/thought was,
current RFC says that we should honor outgoing interface
specified by user (by IPV6_PKTINFO etc.), as we do for
SO_BINDTODEVICE in IPv4 as well.

In this sense, checking sk->sk_bound_dev_if in
ip6_route_output() is not enough because we need to
take outgoing interface specified in ancillary data
into account, which is set to fl->oif.

How about adding additional "flags" parameter
for ip6_route_output()?

Thoughts?

--yoshfuji

Re: [REGRESSION,BISECTED] MIPv6 support broken by f4f914b58019f0

[Arnaud Ebalard]

Hi,

YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org> writes:

>>> I guess I always believed setting SO_BINDTODEVICE should always force
>>> traffic out that interface, but from Yoshifuji's email it seems that
>>> maybe wasn't the intention, at least for things that don't meet
>>> the rt_need_strict() criteria like globals.  I don't know the history
>>> behind the setsockopt.
>>
>> The behavior I would expect from a combination of RFC 4191 and
>> SO_BINDTODEVICE sockopt would be the use of the interface as outgoing
>> interface and then the use of the best router (using router preference
>> info, reachability, ...) available on the subnet. IIRC, the router
>> preference info is per default router list in the RFC, i.e. per
>> interface.
>
> Good point.
>
> Whatever our original intention/thought was,
> current RFC says that we should honor outgoing interface
> specified by user (by IPV6_PKTINFO etc.), as we do for
> SO_BINDTODEVICE in IPv4 as well.
>
> In this sense, checking sk->sk_bound_dev_if in
> ip6_route_output() is not enough because we need to
> take outgoing interface specified in ancillary data
> into account, which is set to fl->oif.
>
> How about adding additional "flags" parameter
> for ip6_route_output()?

I think this may provide a better long term solution but getting all
combinations of cases (SO_BINDTODEVICE and other IPv6 sockopts) work
together (possibly with external info like RFC 4191 ones gathered from
RA or specific local routing config) will be a bit tricky.

Meanwhile, regarding the regression, as Brian's fix handles most
cases, I think it would be useful to apply it and push it to the
stable team.

Cheers,

a+

Re: [PATCH] IPv6: fix Mobile IPv6 regression

[David Miller]

From: Brian Haley <brian.haley@hp.com>
Date: Fri, 28 May 2010 14:17:43 -0400

> Commit f4f914b5 (net: ipv6 bind to device issue) caused
> a regression with Mobile IPv6 when it changed the meaning
> of fl->oif to become a strict requirement of the route
> lookup.  Instead, only force strict mode when
> sk->sk_bound_dev_if is set on the calling socket, getting
> the intended behavior and fixing the regression.
> 
> Tested-by: Arnaud Ebalard <arno@natisbad.org>
> Signed-off-by: Brian Haley <brian.haley@hp.com>

Applied, and queued for -stable, thanks for fixing this.

Re: [PATCH] IPv6: fix Mobile IPv6 regression

[Jiri Olsa]

On Fri, May 28, 2010 at 02:17:43PM -0400, Brian Haley wrote:
> Commit f4f914b5 (net: ipv6 bind to device issue) caused
> a regression with Mobile IPv6 when it changed the meaning
> of fl->oif to become a strict requirement of the route
> lookup.  Instead, only force strict mode when
> sk->sk_bound_dev_if is set on the calling socket, getting
> the intended behavior and fixing the regression.
> 
> Tested-by: Arnaud Ebalard <arno@natisbad.org>
> Signed-off-by: Brian Haley <brian.haley@hp.com>
> 
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 294cbe8..252d761 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -814,7 +814,7 @@ struct dst_entry * ip6_route_output(struct net *net, struct sock *sk,
>  {
>  	int flags = 0;
>  
> -	if (fl->oif || rt6_need_strict(&fl->fl6_dst))
> +	if ((sk && sk->sk_bound_dev_if) || rt6_need_strict(&fl->fl6_dst))
>  		flags |= RT6_LOOKUP_F_IFACE;
>  
>  	if (!ipv6_addr_any(&fl->fl6_src))
hi,

sorry for the late reply, I was out last week..

the change looks ok, I'll verify it with the reproducer
I used for the last fix

thanks,
jirka

Re: [PATCH] IPv6: fix Mobile IPv6 regression

[Jiri Olsa]

On Mon, May 31, 2010 at 10:46:20AM +0200, Jiri Olsa wrote:
> On Fri, May 28, 2010 at 02:17:43PM -0400, Brian Haley wrote:
> > Commit f4f914b5 (net: ipv6 bind to device issue) caused
> > a regression with Mobile IPv6 when it changed the meaning
> > of fl->oif to become a strict requirement of the route
> > lookup.  Instead, only force strict mode when
> > sk->sk_bound_dev_if is set on the calling socket, getting
> > the intended behavior and fixing the regression.
> > 
> > Tested-by: Arnaud Ebalard <arno@natisbad.org>
> > Signed-off-by: Brian Haley <brian.haley@hp.com>
> > 
> > diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> > index 294cbe8..252d761 100644
> > --- a/net/ipv6/route.c
> > +++ b/net/ipv6/route.c
> > @@ -814,7 +814,7 @@ struct dst_entry * ip6_route_output(struct net *net, struct sock *sk,
> >  {
> >  	int flags = 0;
> >  
> > -	if (fl->oif || rt6_need_strict(&fl->fl6_dst))
> > +	if ((sk && sk->sk_bound_dev_if) || rt6_need_strict(&fl->fl6_dst))
> >  		flags |= RT6_LOOKUP_F_IFACE;
> >  
> >  	if (!ipv6_addr_any(&fl->fl6_src))
> hi,
> 
> sorry for the late reply, I was out last week..
> 
> the change looks ok, I'll verify it with the reproducer
> I used for the last fix
> 
> thanks,
> jirka
it passed the test for me, so it looks fine

jirka