traceroute6 and ospf6d (icmp6 source addresses and link-locals)

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

traceroute6 and ospf6d (icmp6 source addresses and link-locals)

Stuart Henderson
With ospf6d, routes are added using link-local addresses. ICMP6 messages
are generated with a source of, I think, the local address associated with
the route to the recipient, so with a couple of hops in the internal network
it results in traceroutes looking like

$ traceroute6 -n www.google.com
traceroute6 to www.google.com (2a00:1450:4009:80f::2004), 64 hops max, 60 byte packets
 1  fe80::5606:33d8:d784:cd2f%vlan701 (fe80::5606:33d8:d784:cd2f%vlan701)  0.519 ms  1.547 ms  0.417 ms
 2  * * *
 3  * * *
 4  2001:728:0:5000::55  7.843 ms  8.236 ms  7.391 ms
[...]

The first hop works (ugliness aside) because the link-local is directly
connected, the next ones don't because the ICMP response are sourced from
a link-local addresses (to match the route to the destination) but these
aren't allowed to be forwarded by routers. (And once packets have left
the AS, things work as expected on global addresses).

What's anyone else doing? Just living with it or has anyone figured a way
to make it nicer? I'd like to reply with either a global scope address for
the interface, or a loopback address, I didn't get anywhere with PF
translation though.

Reply | Threaded
Open this post in threaded view
|

Re: traceroute6 and ospf6d (icmp6 source addresses and link-locals)

Alexander Bluhm
IPv6 Source selection is a mess!

> ICMP6 messages
> are generated with a source of, I think, the local address associated with
> the route to the recipient,

It is not that simple.  Look at in6_ifawithscope() in sys/netinet6/in6.c.

                        /*
                         * At this point, we have two cases:
                         * 1. we are looking at a non-deprecated address,
                         *    and ia6_best is also non-deprecated.
                         * 2. we are looking at a deprecated address,
                         *    and ia6_best is also deprecated.
                         * Also, we do not have to consider a case where
                         * the scope of if_best is larger(smaller) than dst and
                         * the scope of the current address is smaller(larger)
                         * than dst. Such a case has already been covered.
                         * Tiebreaking is done according to the following
                         * items:
                         * - the scope comparison between the address and
                         *   dst (dscopecmp)
                         * - the scope comparison between the address and
                         *   ia6_best (bscopecmp)
                         * - if the address match dst longer than ia6_best
                         *   (matchcmp)
                         * - if the address is on the outgoing I/F (outI/F)
                         *
                         * Roughly speaking, the selection policy is
                         * - the most important item is scope. The same scope
                         *   is best. Then search for a larger scope.
                         *   Smaller scopes are the last resort.
                         * - A deprecated address is chosen only when we have
                         *   no address that has an enough scope, but is
                         *   prefered to any addresses of smaller scopes.
                         * - Longest address match against dst is considered
                         *   only for addresses that has the same scope of dst.
                         * - If there is no other reasons to choose one,
                         *   addresses on the outgoing I/F are preferred.
                         *
                         * The precise decision table is as follows:
                         * dscopecmp bscopecmp matchcmp outI/F | replace?
                         *    !equal     equal      N/A    Yes |      Yes (1)
                         *    !equal     equal      N/A     No |       No (2)
                         *    larger    larger      N/A    N/A |       No (3)
                         *    larger   smaller      N/A    N/A |      Yes (4)
                         *   smaller    larger      N/A    N/A |      Yes (5)
                         *   smaller   smaller      N/A    N/A |       No (6)
                         *     equal   smaller      N/A    N/A |      Yes (7)
                         *     equal    larger       (already done)
                         *     equal     equal   larger    N/A |      Yes (8)
                         *     equal     equal  smaller    N/A |       No (9)
                         *     equal     equal    equal    Yes |      Yes (a)
                         *     equal     equal    equal     No |       No (b)
                         */

Could you provide your ifconfig and route output?  So we could
figure out into which path of this algorith you are running.

Once I have have added the following rule from a newer RFC.  It
makes things better for many caes, but not with OSPF6.  There you
may have an interface with only link-local addresses.  Then this
link-local is used instead of another better scoped one.

                        /* RFC 3484 5. Rule 5: Prefer outgoing interface */

>  4  2001:728:0:5000::55  7.843 ms  8.236 ms  7.391 ms

How can this work?  Does your AS-Boundary Router do NAT?

> What's anyone else doing? Just living with it or has anyone figured a way
> to make it nicer? I'd like to reply with either a global scope address for
> the interface, or a loopback address,

We have implemented more or less a very old RFC.  There are two
newer RFCs with different algorithms.  There is recommendation to
store policies from user-land into the kernel for address selection.

I have just looked at FreeBSD in6_ifawithifp(), it is quite simple.
Perhaps this is a way to go.

> I didn't get anywhere with PF
> translation though.

pf with IPv6 link-local addresses does not work properly.  I think
it cannot parse the %if suffixes.  The KAME hack scope id is not
handled.

bluhm

Reply | Threaded
Open this post in threaded view
|

Re: traceroute6 and ospf6d (icmp6 source addresses and link-locals)

Stuart Henderson
On 2018/10/05 18:38, Alexander Bluhm wrote:
> IPv6 Source selection is a mess!
>
> > ICMP6 messages
> > are generated with a source of, I think, the local address associated with
> > the route to the recipient,
>
> It is not that simple.  Look at in6_ifawithscope() in sys/netinet6/in6.c.

I know that's used for newly generated packets, but I'm not sure it's the
case for icmp6, I haven't tried modifying the kernel to confirm for sure
that this is the code generating the address in this case, but it seems
likely, in icmp6.c:

1111         /*
1112          * If the incoming packet was addressed directly to us (i.e. unicast),
1113          * use dst as the src for the reply.
1114          * The IN6_IFF_TENTATIVE|IN6_IFF_DUPLICATED case would be VERY rare,
1115          * but is possible (for example) when we encounter an error while
1116          * forwarding procedure destined to a duplicated address of ours.
1117          */
1118         rt = rtalloc(sin6tosa(&sa6_dst), 0, rtableid);
1119         if (rtisvalid(rt) && ISSET(rt->rt_flags, RTF_LOCAL) &&
1120             !ISSET(ifatoia6(rt->rt_ifa)->ia6_flags,
1121             IN6_IFF_ANYCAST|IN6_IFF_TENTATIVE|IN6_IFF_DUPLICATED)) {
1122                 src = &t;
1123         }

> Could you provide your ifconfig and route output?  So we could
> figure out into which path of this algorith you are running.

The host running traceroute has a handful of global scope addresses on
loopback interfaces, plus a global scope address on a vlan interface
facing the next router, all advertised into ospf.

The default source address is one of the loopbacks,
2001:67c:15f4:a423::26, so the only route back from the rest of the
network to this address is via link-locals all the way.

BGP routes changed so I'll include a traceroute using the default source
address again so all the new copied output is consistent:

$ traceroute6 -n www.google.com
traceroute6 to www.google.com (2a00:1450:4009:80b::2004), 64 hops max, 60 byte packets
 1  fe80::5606:33d8:d784:cd2f%vlan701  0.494 ms  0.362 ms  0.373 ms
 2  * * *
 3  * * *
 4  2001:7f8:17::1b1b:1  7.272 ms  7.332 ms  6.938 ms
 5  2001:7f8:17::3b41:1  6.699 ms  6.342 ms  6.453 ms
[...]

From the first hop router,

gr1$ route -n get -inet6 2001:67c:15f4:a423::26
   route to: 2001:67c:15f4:a423::26
destination: 2001:67c:15f4:a423::26
       mask: ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff
    gateway: fe80::e648:4970:e85f:5e13%vlan701
  interface: vlan701
 if address: fe80::5606:33d8:d784:cd2f%vlan701
   priority: 32 (ospf)
      flags: <UP,GATEWAY,HOST,DONE>
     use       mtu    expire
29464060         0         0

From the second hop router,

gr5$ route -n get -inet6 2001:67c:15f4:a423::26
   route to: 2001:67c:15f4:a423::26
destination: 2001:67c:15f4:a423::26
       mask: ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff
    gateway: fe80::d05a:f0e8:e5e:a30a%vlan740
  interface: vlan740
 if address: fe80::b769:5751:d87b:44b7%vlan740
   priority: 32 (ospf)
      flags: <UP,GATEWAY,HOST,DONE>
     use       mtu    expire
20369017         0         0

If instead I source packets from the vlan interface (directly connected
to the next router), I instead get this:

$ traceroute6 -n -s 2a03:8920:1:52bd::184 www.google.com
traceroute6 to www.google.com (2a00:1450:4009:80b::2004) from 2a03:8920:1:52bd::184, 64 hops max, 60 byte packets
 1  2a03:8920:1:52bd::181  1.769 ms  0.382 ms  0.377 ms
 2  * * *
 3  * * *
 4  2001:7f8:17::1b1b:1  6.931 ms  6.999 ms  7.115 ms
 5  2001:7f8:17::3b41:1  6.466 ms  6.568 ms  6.416 ms

The routes in this case from the hop 1 and 2 routers are

gr1$ route -n get -inet6 2a03:8920:1:52bd::184
   route to: 2a03:8920:1:52bd::184
destination: 2a03:8920:1:52bd::184
       mask: ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff
  interface: vlan701
 if address: 2a03:8920:1:52bd::181
   priority: 3 ()
      flags: <UP,HOST,DONE,LLINFO,CLONED>
     use       mtu    expire
   22811         0         3

gr5$ route -n get -inet6 2a03:8920:1:52bd::184
   route to: 2a03:8920:1:52bd::184
destination: 2a03:8920:1:52bd::
       mask: ffff:ffff:ffff:ffff::
    gateway: fe80::d05a:f0e8:e5e:a30a%vlan740
  interface: vlan740
 if address: fe80::b769:5751:d87b:44b7%vlan740
   priority: 32 (ospf)
      flags: <UP,GATEWAY,DONE>
     use       mtu    expire
     234         0         0

So the first hop packet is returned from a "normal" address, and the
second (from tcpdump) is returned from fe80::b769:5751:d87b:44b7 and of
course doesn't make it all the way back to the host running traceroute.

> Once I have have added the following rule from a newer RFC.  It
> makes things better for many caes, but not with OSPF6.  There you
> may have an interface with only link-local addresses.  Then this
> link-local is used instead of another better scoped one.

I have global addresses on all interfaces involved, none of the involved
interfaces just have link-local.

>                         /* RFC 3484 5. Rule 5: Prefer outgoing interface */
>
> >  4  2001:728:0:5000::55  7.843 ms  8.236 ms  7.391 ms
>
> How can this work?  Does your AS-Boundary Router do NAT?

The source address on my traceroutes is a global scope address in all these
cases, so my upstream or peer knows how to route back to that.

> > What's anyone else doing? Just living with it or has anyone figured a way
> > to make it nicer? I'd like to reply with either a global scope address for
> > the interface, or a loopback address,
>
> We have implemented more or less a very old RFC.  There are two
> newer RFCs with different algorithms.  There is recommendation to
> store policies from user-land into the kernel for address selection.
>
> I have just looked at FreeBSD in6_ifawithifp(), it is quite simple.
> Perhaps this is a way to go.

The code in FreeBSD's icmp6.c matching the above calls in6ifa_ifwithaddr
https://svnweb.freebsd.org/base/head/sys/netinet6/icmp6.c?revision=338831&view=markup#l2113

> > I didn't get anywhere with PF
> > translation though.
>
> pf with IPv6 link-local addresses does not work properly.  I think
> it cannot parse the %if suffixes.  The KAME hack scope id is not
> handled.

Thank you.

Reply | Threaded
Open this post in threaded view
|

Re: traceroute6 and ospf6d (icmp6 source addresses and link-locals)

Arnaud BRAND
Le 2018-10-05 23:42, Stuart Henderson a écrit :

> On 2018/10/05 18:38, Alexander Bluhm wrote:
>> IPv6 Source selection is a mess!
>>
>> > ICMP6 messages
>> > are generated with a source of, I think, the local address associated with
>> > the route to the recipient,
>>
>> It is not that simple.  Look at in6_ifawithscope() in
>> sys/netinet6/in6.c.
>
> I know that's used for newly generated packets, but I'm not sure it's
> the
> case for icmp6, I haven't tried modifying the kernel to confirm for
> sure
> that this is the code generating the address in this case, but it seems
> likely, in icmp6.c:
>
> 1111         /*
> 1112          * If the incoming packet was addressed directly to us
> (i.e. unicast),
> 1113          * use dst as the src for the reply.
> 1114          * The IN6_IFF_TENTATIVE|IN6_IFF_DUPLICATED case would be
> VERY rare,
> 1115          * but is possible (for example) when we encounter an
> error while
> 1116          * forwarding procedure destined to a duplicated address
> of ours.
> 1117          */
> 1118         rt = rtalloc(sin6tosa(&sa6_dst), 0, rtableid);
> 1119         if (rtisvalid(rt) && ISSET(rt->rt_flags, RTF_LOCAL) &&
> 1120             !ISSET(ifatoia6(rt->rt_ifa)->ia6_flags,
> 1121             IN6_IFF_ANYCAST|IN6_IFF_TENTATIVE|IN6_IFF_DUPLICATED))
> {
> 1122                 src = &t;
> 1123         }
>

I don't think this is the proper code extract because the traceroute6
packet
is not addressed directly to us.
It's addressed to another global unicast address and appears to time out
because the TTL gets decremented.

So I think the code that gets executed in this case is :
1127         if (src == NULL) {
1128                 /*
1129                  * This case matches to multicasts, our anycast, or
unicasts
1130                  * that we do not own.  Select a source address
based on the
1131                  * source address of the erroneous packet.
1132                  */
1133                 rt = rtalloc(sin6tosa(&sa6_src), RT_RESOLVE,
rtableid);
1134                 if (!rtisvalid(rt)) {
1135                         char addr[INET6_ADDRSTRLEN];
1136
1137                         nd6log((LOG_DEBUG,
1138                             "%s: source can't be determined:
dst=%s\n",
1139                             __func__, inet_ntop(AF_INET6,
&sa6_src.sin6_addr,
1140                                 addr, sizeof(addr))));
1141                         rtfree(rt);
1142                         goto bad;
1143                 }
1144                 src = &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
1145
1146         }

I was thinking of replacing lines 1144 and 1145 with something along the
lines of
src = in6_ifawithscope(rt->rt_ifa->ifa_ifp, &t, rtableid);
if (src=NULL)  src = &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;

Sorry if style is wrong, I'm not used to it, but you get the idea.

>> I have just looked at FreeBSD in6_ifawithifp(), it is quite simple.
>> Perhaps this is a way to go.
>
> The code in FreeBSD's icmp6.c matching the above calls
> in6ifa_ifwithaddr
> https://svnweb.freebsd.org/base/head/sys/netinet6/icmp6.c?revision=338831&view=markup#l2113
>

Again, I think the branch hat gets executed is at
https://svnweb.freebsd.org/base/head/sys/netinet6/icmp6.c?revision=338831&view=markup#l2137
which calls
2147                in6_splitscope(&ip6->ip6_src, &dst6, &scopeid);
2148                error = in6_selectsrc_addr(M_GETFIB(m), &dst6,
2149                    scopeid, NULL, &src6, &hlim);

I can't find the in6_splitscope function in OpenBSD, so I guess there's
more to it than just copy-paste...



Reply | Threaded
Open this post in threaded view
|

Re: traceroute6 and ospf6d (icmp6 source addresses and link-locals)

Arnaud BRAND
Le 2018-11-28 22:46, Arnaud BRAND a écrit :

> Le 2018-10-05 23:42, Stuart Henderson a écrit :
>> On 2018/10/05 18:38, Alexander Bluhm wrote:
>>> IPv6 Source selection is a mess!
>>>
>>> > ICMP6 messages
>>> > are generated with a source of, I think, the local address associated with
>>> > the route to the recipient,
>>>
>>> It is not that simple.  Look at in6_ifawithscope() in
>>> sys/netinet6/in6.c.
>>
>> I know that's used for newly generated packets, but I'm not sure it's
>> the
>> case for icmp6, I haven't tried modifying the kernel to confirm for
>> sure
>> that this is the code generating the address in this case, but it
>> seems
>> likely, in icmp6.c:
>>
>> 1111         /*
>> 1112          * If the incoming packet was addressed directly to us
>> (i.e. unicast),
>> 1113          * use dst as the src for the reply.
>> 1114          * The IN6_IFF_TENTATIVE|IN6_IFF_DUPLICATED case would be
>> VERY rare,
>> 1115          * but is possible (for example) when we encounter an
>> error while
>> 1116          * forwarding procedure destined to a duplicated address
>> of ours.
>> 1117          */
>> 1118         rt = rtalloc(sin6tosa(&sa6_dst), 0, rtableid);
>> 1119         if (rtisvalid(rt) && ISSET(rt->rt_flags, RTF_LOCAL) &&
>> 1120             !ISSET(ifatoia6(rt->rt_ifa)->ia6_flags,
>> 1121            
>> IN6_IFF_ANYCAST|IN6_IFF_TENTATIVE|IN6_IFF_DUPLICATED)) {
>> 1122                 src = &t;
>> 1123         }
>>
>
> I don't think this is the proper code extract because the traceroute6
> packet
> is not addressed directly to us.
> It's addressed to another global unicast address and appears to time
> out
> because the TTL gets decremented.
>
> So I think the code that gets executed in this case is :
> 1127         if (src == NULL) {
> 1128                 /*
> 1129                  * This case matches to multicasts, our anycast,
> or unicasts
> 1130                  * that we do not own.  Select a source address
> based on the
> 1131                  * source address of the erroneous packet.
> 1132                  */
> 1133                 rt = rtalloc(sin6tosa(&sa6_src), RT_RESOLVE,
> rtableid);
> 1134                 if (!rtisvalid(rt)) {
> 1135                         char addr[INET6_ADDRSTRLEN];
> 1136
> 1137                         nd6log((LOG_DEBUG,
> 1138                             "%s: source can't be determined:
> dst=%s\n",
> 1139                             __func__, inet_ntop(AF_INET6,
> &sa6_src.sin6_addr,
> 1140                                 addr, sizeof(addr))));
> 1141                         rtfree(rt);
> 1142                         goto bad;
> 1143                 }
> 1144                 src = &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
> 1145
> 1146         }
>
> I was thinking of replacing lines 1144 and 1145 with something along
> the lines of
> src = in6_ifawithscope(rt->rt_ifa->ifa_ifp, &t, rtableid);
> if (src=NULL)  src = &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
>
> Sorry if style is wrong, I'm not used to it, but you get the idea.
>

Currently building a test kernel with this diff.
Index: netinet6/icmp6.c
===================================================================
RCS file: /cvs/src/sys/netinet6/icmp6.c,v
retrieving revision 1.227
diff -u -p -u -p -r1.227 icmp6.c
--- netinet6/icmp6.c    9 Nov 2018 14:14:32 -0000       1.227
+++ netinet6/icmp6.c    28 Nov 2018 22:55:04 -0000
@@ -1038,6 +1038,7 @@ icmp6_reflect(struct mbuf *m, size_t off
         struct icmp6_hdr *icmp6;
         struct in6_addr t, *src = NULL;
         struct sockaddr_in6 sa6_src, sa6_dst;
+       struct in6_ifaddr *ifa;
         u_int rtableid;

         CTASSERT(sizeof(struct ip6_hdr) + sizeof(struct icmp6_hdr) <=
MHLEN);
@@ -1141,7 +1142,9 @@ icmp6_reflect(struct mbuf *m, size_t off
                         rtfree(rt);
                         goto bad;
                 }
-               src = &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
+               ifa = in6_ifawithscope(rt->rt_ifa->ifa_ifp, &t,
rtableid);
+               if (ifa != NULL) src = &(ifa->ia_addr.sin6_addr);
+               if (src == NULL) src =
&ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
         }

         ip6->ip6_src = *src;

But I have no -current machine in IPv6 paths to test it.
I'll have to backport on -stable next week if I get a chance.


>>> I have just looked at FreeBSD in6_ifawithifp(), it is quite simple.
>>> Perhaps this is a way to go.
>>
>> The code in FreeBSD's icmp6.c matching the above calls
>> in6ifa_ifwithaddr
>> https://svnweb.freebsd.org/base/head/sys/netinet6/icmp6.c?revision=338831&view=markup#l2113
>>
>
> Again, I think the branch hat gets executed is at
> https://svnweb.freebsd.org/base/head/sys/netinet6/icmp6.c?revision=338831&view=markup#l2137
> which calls
> 2147                in6_splitscope(&ip6->ip6_src, &dst6, &scopeid);
> 2148                error = in6_selectsrc_addr(M_GETFIB(m), &dst6,
> 2149                    scopeid, NULL, &src6, &hlim);
>
> I can't find the in6_splitscope function in OpenBSD, so I guess
> there's more to it than just copy-paste...

Actually in6_selectsrc_addr seems to do something similar to what
in6_ifawithscope does.
So this part seems correct.
What seems a little bit complicated is the call to rt_alloc just to get
the *oifp for in6_ifawithscope.
I though of it as a way to direct in6_ifawithscope to choose the
interface's global unicast address.
Perhaps there is a simpler way to obtain the pointer and/or the supposed
effect, but I'm not familiar enough with the code yet...

Thoughts ?

Reply | Threaded
Open this post in threaded view
|

Re: traceroute6 and ospf6d (icmp6 source addresses and link-locals)

Arnaud BRAND
I have setup a test environment where I can reproduce the issue with
GENERIC#481.
I can confirm that the issue is fixed by the proposed patch.
traceroute6 to/from/through the patched machine got expected result and
did not crash the machine.

Anyone else would like to try ?
Or propose improvements ?

Le 2018-11-29 00:02, Arnaud BRAND a écrit :

> Le 2018-11-28 22:46, Arnaud BRAND a écrit :
>> Le 2018-10-05 23:42, Stuart Henderson a écrit :
>>> On 2018/10/05 18:38, Alexander Bluhm wrote:
>>>> IPv6 Source selection is a mess!
>>>>
>>>> > ICMP6 messages
>>>> > are generated with a source of, I think, the local address associated with
>>>> > the route to the recipient,
>>>>
>>>> It is not that simple.  Look at in6_ifawithscope() in
>>>> sys/netinet6/in6.c.
>>>
>>> I know that's used for newly generated packets, but I'm not sure it's
>>> the
>>> case for icmp6, I haven't tried modifying the kernel to confirm for
>>> sure
>>> that this is the code generating the address in this case, but it
>>> seems
>>> likely, in icmp6.c:
>>>
>>> 1111         /*
>>> 1112          * If the incoming packet was addressed directly to us
>>> (i.e. unicast),
>>> 1113          * use dst as the src for the reply.
>>> 1114          * The IN6_IFF_TENTATIVE|IN6_IFF_DUPLICATED case would
>>> be
>>> VERY rare,
>>> 1115          * but is possible (for example) when we encounter an
>>> error while
>>> 1116          * forwarding procedure destined to a duplicated address
>>> of ours.
>>> 1117          */
>>> 1118         rt = rtalloc(sin6tosa(&sa6_dst), 0, rtableid);
>>> 1119         if (rtisvalid(rt) && ISSET(rt->rt_flags, RTF_LOCAL) &&
>>> 1120             !ISSET(ifatoia6(rt->rt_ifa)->ia6_flags,
>>> 1121            
>>> IN6_IFF_ANYCAST|IN6_IFF_TENTATIVE|IN6_IFF_DUPLICATED)) {
>>> 1122                 src = &t;
>>> 1123         }
>>>
>>
>> I don't think this is the proper code extract because the traceroute6
>> packet
>> is not addressed directly to us.
>> It's addressed to another global unicast address and appears to time
>> out
>> because the TTL gets decremented.
>>
>> So I think the code that gets executed in this case is :
>> 1127         if (src == NULL) {
>> 1128                 /*
>> 1129                  * This case matches to multicasts, our anycast,
>> or unicasts
>> 1130                  * that we do not own.  Select a source address
>> based on the
>> 1131                  * source address of the erroneous packet.
>> 1132                  */
>> 1133                 rt = rtalloc(sin6tosa(&sa6_src), RT_RESOLVE,
>> rtableid);
>> 1134                 if (!rtisvalid(rt)) {
>> 1135                         char addr[INET6_ADDRSTRLEN];
>> 1136
>> 1137                         nd6log((LOG_DEBUG,
>> 1138                             "%s: source can't be determined:
>> dst=%s\n",
>> 1139                             __func__, inet_ntop(AF_INET6,
>> &sa6_src.sin6_addr,
>> 1140                                 addr, sizeof(addr))));
>> 1141                         rtfree(rt);
>> 1142                         goto bad;
>> 1143                 }
>> 1144                 src = &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
>> 1145
>> 1146         }
>>
>> I was thinking of replacing lines 1144 and 1145 with something along
>> the lines of
>> src = in6_ifawithscope(rt->rt_ifa->ifa_ifp, &t, rtableid);
>> if (src=NULL)  src = &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
>>
>> Sorry if style is wrong, I'm not used to it, but you get the idea.
>>
>
> Currently building a test kernel with this diff.
> Index: netinet6/icmp6.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/icmp6.c,v
> retrieving revision 1.227
> diff -u -p -u -p -r1.227 icmp6.c
> --- netinet6/icmp6.c    9 Nov 2018 14:14:32 -0000       1.227
> +++ netinet6/icmp6.c    28 Nov 2018 22:55:04 -0000
> @@ -1038,6 +1038,7 @@ icmp6_reflect(struct mbuf *m, size_t off
>         struct icmp6_hdr *icmp6;
>         struct in6_addr t, *src = NULL;
>         struct sockaddr_in6 sa6_src, sa6_dst;
> +       struct in6_ifaddr *ifa;
>         u_int rtableid;
>
>         CTASSERT(sizeof(struct ip6_hdr) + sizeof(struct icmp6_hdr) <=
> MHLEN);
> @@ -1141,7 +1142,9 @@ icmp6_reflect(struct mbuf *m, size_t off
>                         rtfree(rt);
>                         goto bad;
>                 }
> -               src = &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
> +               ifa = in6_ifawithscope(rt->rt_ifa->ifa_ifp, &t,
> rtableid);
> +               if (ifa != NULL) src = &(ifa->ia_addr.sin6_addr);
> +               if (src == NULL) src =
> &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
>         }
>
>         ip6->ip6_src = *src;
>
> But I have no -current machine in IPv6 paths to test it.
> I'll have to backport on -stable next week if I get a chance.
>
>
>>>> I have just looked at FreeBSD in6_ifawithifp(), it is quite simple.
>>>> Perhaps this is a way to go.
>>>
>>> The code in FreeBSD's icmp6.c matching the above calls
>>> in6ifa_ifwithaddr
>>> https://svnweb.freebsd.org/base/head/sys/netinet6/icmp6.c?revision=338831&view=markup#l2113
>>>
>>
>> Again, I think the branch hat gets executed is at
>> https://svnweb.freebsd.org/base/head/sys/netinet6/icmp6.c?revision=338831&view=markup#l2137
>> which calls
>> 2147                in6_splitscope(&ip6->ip6_src, &dst6, &scopeid);
>> 2148                error = in6_selectsrc_addr(M_GETFIB(m), &dst6,
>> 2149                    scopeid, NULL, &src6, &hlim);
>>
>> I can't find the in6_splitscope function in OpenBSD, so I guess
>> there's more to it than just copy-paste...
>
> Actually in6_selectsrc_addr seems to do something similar to what
> in6_ifawithscope does.
> So this part seems correct.
> What seems a little bit complicated is the call to rt_alloc just to
> get the *oifp for in6_ifawithscope.
> I though of it as a way to direct in6_ifawithscope to choose the
> interface's global unicast address.
> Perhaps there is a simpler way to obtain the pointer and/or the
> supposed effect, but I'm not familiar enough with the code yet...
>
> Thoughts ?

Reply | Threaded
Open this post in threaded view
|

Re: traceroute6 and ospf6d (icmp6 source addresses and link-locals)

Arnaud BRAND
Any feedback on this patch ?
I'm running it without problems since the 30th November.


Index: netinet6/icmp6.c
===================================================================
RCS file: /cvs/src/sys/netinet6/icmp6.c,v
retrieving revision 1.227
diff -u -p -u -p -r1.227 icmp6.c
--- netinet6/icmp6.c    9 Nov 2018 14:14:32 -0000       1.227
+++ netinet6/icmp6.c    28 Nov 2018 22:55:04 -0000
@@ -1038,6 +1038,7 @@ icmp6_reflect(struct mbuf *m, size_t off
         struct icmp6_hdr *icmp6;
         struct in6_addr t, *src = NULL;
         struct sockaddr_in6 sa6_src, sa6_dst;
+       struct in6_ifaddr *ifa;
         u_int rtableid;

         CTASSERT(sizeof(struct ip6_hdr) + sizeof(struct icmp6_hdr) <=
MHLEN);
@@ -1141,7 +1142,9 @@ icmp6_reflect(struct mbuf *m, size_t off
                         rtfree(rt);
                         goto bad;
                 }
-               src = &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
+               ifa = in6_ifawithscope(rt->rt_ifa->ifa_ifp, &t,
rtableid);
+               if (ifa != NULL) src = &(ifa->ia_addr.sin6_addr);
+               if (src == NULL) src =
&ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
         }

         ip6->ip6_src = *src;



Le 2018-11-29 00:50, Arnaud BRAND a écrit :

> I have setup a test environment where I can reproduce the issue with
> GENERIC#481.
> I can confirm that the issue is fixed by the proposed patch.
> traceroute6 to/from/through the patched machine got expected result
> and did not crash the machine.
>
> Anyone else would like to try ?
> Or propose improvements ?
>
> Le 2018-11-29 00:02, Arnaud BRAND a écrit :
>> Le 2018-11-28 22:46, Arnaud BRAND a écrit :
>>> Le 2018-10-05 23:42, Stuart Henderson a écrit :
>>>> On 2018/10/05 18:38, Alexander Bluhm wrote:
>>>>> IPv6 Source selection is a mess!
>>>>>
>>>>> > ICMP6 messages
>>>>> > are generated with a source of, I think, the local address associated with
>>>>> > the route to the recipient,
>>>>>
>>>>> It is not that simple.  Look at in6_ifawithscope() in
>>>>> sys/netinet6/in6.c.
>>>>
>>>> I know that's used for newly generated packets, but I'm not sure
>>>> it's the
>>>> case for icmp6, I haven't tried modifying the kernel to confirm for
>>>> sure
>>>> that this is the code generating the address in this case, but it
>>>> seems
>>>> likely, in icmp6.c:
>>>>
>>>> 1111         /*
>>>> 1112          * If the incoming packet was addressed directly to us
>>>> (i.e. unicast),
>>>> 1113          * use dst as the src for the reply.
>>>> 1114          * The IN6_IFF_TENTATIVE|IN6_IFF_DUPLICATED case would
>>>> be
>>>> VERY rare,
>>>> 1115          * but is possible (for example) when we encounter an
>>>> error while
>>>> 1116          * forwarding procedure destined to a duplicated
>>>> address of ours.
>>>> 1117          */
>>>> 1118         rt = rtalloc(sin6tosa(&sa6_dst), 0, rtableid);
>>>> 1119         if (rtisvalid(rt) && ISSET(rt->rt_flags, RTF_LOCAL) &&
>>>> 1120             !ISSET(ifatoia6(rt->rt_ifa)->ia6_flags,
>>>> 1121            
>>>> IN6_IFF_ANYCAST|IN6_IFF_TENTATIVE|IN6_IFF_DUPLICATED)) {
>>>> 1122                 src = &t;
>>>> 1123         }
>>>>
>>>
>>> I don't think this is the proper code extract because the traceroute6
>>> packet
>>> is not addressed directly to us.
>>> It's addressed to another global unicast address and appears to time
>>> out
>>> because the TTL gets decremented.
>>>
>>> So I think the code that gets executed in this case is :
>>> 1127         if (src == NULL) {
>>> 1128                 /*
>>> 1129                  * This case matches to multicasts, our anycast,
>>> or unicasts
>>> 1130                  * that we do not own.  Select a source address
>>> based on the
>>> 1131                  * source address of the erroneous packet.
>>> 1132                  */
>>> 1133                 rt = rtalloc(sin6tosa(&sa6_src), RT_RESOLVE,
>>> rtableid);
>>> 1134                 if (!rtisvalid(rt)) {
>>> 1135                         char addr[INET6_ADDRSTRLEN];
>>> 1136
>>> 1137                         nd6log((LOG_DEBUG,
>>> 1138                             "%s: source can't be determined:
>>> dst=%s\n",
>>> 1139                             __func__, inet_ntop(AF_INET6,
>>> &sa6_src.sin6_addr,
>>> 1140                                 addr, sizeof(addr))));
>>> 1141                         rtfree(rt);
>>> 1142                         goto bad;
>>> 1143                 }
>>> 1144                 src = &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
>>> 1145
>>> 1146         }
>>>
>>> I was thinking of replacing lines 1144 and 1145 with something along
>>> the lines of
>>> src = in6_ifawithscope(rt->rt_ifa->ifa_ifp, &t, rtableid);
>>> if (src=NULL)  src = &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
>>>
>>> Sorry if style is wrong, I'm not used to it, but you get the idea.
>>>
>>
>> Currently building a test kernel with this diff.
>> Index: netinet6/icmp6.c
>> ===================================================================
>> RCS file: /cvs/src/sys/netinet6/icmp6.c,v
>> retrieving revision 1.227
>> diff -u -p -u -p -r1.227 icmp6.c
>> --- netinet6/icmp6.c    9 Nov 2018 14:14:32 -0000       1.227
>> +++ netinet6/icmp6.c    28 Nov 2018 22:55:04 -0000
>> @@ -1038,6 +1038,7 @@ icmp6_reflect(struct mbuf *m, size_t off
>>         struct icmp6_hdr *icmp6;
>>         struct in6_addr t, *src = NULL;
>>         struct sockaddr_in6 sa6_src, sa6_dst;
>> +       struct in6_ifaddr *ifa;
>>         u_int rtableid;
>>
>>         CTASSERT(sizeof(struct ip6_hdr) + sizeof(struct icmp6_hdr) <=
>> MHLEN);
>> @@ -1141,7 +1142,9 @@ icmp6_reflect(struct mbuf *m, size_t off
>>                         rtfree(rt);
>>                         goto bad;
>>                 }
>> -               src = &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
>> +               ifa = in6_ifawithscope(rt->rt_ifa->ifa_ifp, &t,
>> rtableid);
>> +               if (ifa != NULL) src = &(ifa->ia_addr.sin6_addr);
>> +               if (src == NULL) src =
>> &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
>>         }
>>
>>         ip6->ip6_src = *src;
>>
>> But I have no -current machine in IPv6 paths to test it.
>> I'll have to backport on -stable next week if I get a chance.
>>
>>
>>>>> I have just looked at FreeBSD in6_ifawithifp(), it is quite simple.
>>>>> Perhaps this is a way to go.
>>>>
>>>> The code in FreeBSD's icmp6.c matching the above calls
>>>> in6ifa_ifwithaddr
>>>> https://svnweb.freebsd.org/base/head/sys/netinet6/icmp6.c?revision=338831&view=markup#l2113
>>>>
>>>
>>> Again, I think the branch hat gets executed is at
>>> https://svnweb.freebsd.org/base/head/sys/netinet6/icmp6.c?revision=338831&view=markup#l2137
>>> which calls
>>> 2147                in6_splitscope(&ip6->ip6_src, &dst6, &scopeid);
>>> 2148                error = in6_selectsrc_addr(M_GETFIB(m), &dst6,
>>> 2149                    scopeid, NULL, &src6, &hlim);
>>>
>>> I can't find the in6_splitscope function in OpenBSD, so I guess
>>> there's more to it than just copy-paste...
>>
>> Actually in6_selectsrc_addr seems to do something similar to what
>> in6_ifawithscope does.
>> So this part seems correct.
>> What seems a little bit complicated is the call to rt_alloc just to
>> get the *oifp for in6_ifawithscope.
>> I though of it as a way to direct in6_ifawithscope to choose the
>> interface's global unicast address.
>> Perhaps there is a simpler way to obtain the pointer and/or the
>> supposed effect, but I'm not familiar enough with the code yet...
>>
>> Thoughts ?

Reply | Threaded
Open this post in threaded view
|

Re: traceroute6 and ospf6d (icmp6 source addresses and link-locals)

Denis Fondras
On Wed, Dec 05, 2018 at 11:36:14AM +0100, Arnaud BRAND wrote:

> Any feedback on this patch ?
> I'm running it without problems since the 30th November.
>
>
> Index: netinet6/icmp6.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/icmp6.c,v
> retrieving revision 1.227
> diff -u -p -u -p -r1.227 icmp6.c
> --- netinet6/icmp6.c    9 Nov 2018 14:14:32 -0000       1.227
> +++ netinet6/icmp6.c    28 Nov 2018 22:55:04 -0000
> @@ -1038,6 +1038,7 @@ icmp6_reflect(struct mbuf *m, size_t off
>         struct icmp6_hdr *icmp6;
>         struct in6_addr t, *src = NULL;
>         struct sockaddr_in6 sa6_src, sa6_dst;
> +       struct in6_ifaddr *ifa;
>         u_int rtableid;
>
>         CTASSERT(sizeof(struct ip6_hdr) + sizeof(struct icmp6_hdr) <=
> MHLEN);
> @@ -1141,7 +1142,9 @@ icmp6_reflect(struct mbuf *m, size_t off
>                         rtfree(rt);
>                         goto bad;
>                 }
> -               src = &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
> +               ifa = in6_ifawithscope(rt->rt_ifa->ifa_ifp, &t, rtableid);
> +               if (ifa != NULL) src = &(ifa->ia_addr.sin6_addr);
> +               if (src == NULL) src =
> &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
>         }
>
>         ip6->ip6_src = *src;
>

style(9) :)

>
>
> Le 2018-11-29 00:50, Arnaud BRAND a écrit :
> > I have setup a test environment where I can reproduce the issue with
> > GENERIC#481.
> > I can confirm that the issue is fixed by the proposed patch.
> > traceroute6 to/from/through the patched machine got expected result
> > and did not crash the machine.
> >
> > Anyone else would like to try ?
> > Or propose improvements ?
> >
> > Le 2018-11-29 00:02, Arnaud BRAND a écrit :
> > > Le 2018-11-28 22:46, Arnaud BRAND a écrit :
> > > > Le 2018-10-05 23:42, Stuart Henderson a écrit :
> > > > > On 2018/10/05 18:38, Alexander Bluhm wrote:
> > > > > > IPv6 Source selection is a mess!
> > > > > >
> > > > > > > ICMP6 messages
> > > > > > > are generated with a source of, I think, the local address associated with
> > > > > > > the route to the recipient,
> > > > > >
> > > > > > It is not that simple.  Look at in6_ifawithscope() in
> > > > > > sys/netinet6/in6.c.
> > > > >
> > > > > I know that's used for newly generated packets, but I'm not
> > > > > sure it's the
> > > > > case for icmp6, I haven't tried modifying the kernel to
> > > > > confirm for sure
> > > > > that this is the code generating the address in this case,
> > > > > but it seems
> > > > > likely, in icmp6.c:
> > > > >
> > > > > 1111         /*
> > > > > 1112          * If the incoming packet was addressed directly to us
> > > > > (i.e. unicast),
> > > > > 1113          * use dst as the src for the reply.
> > > > > 1114          * The IN6_IFF_TENTATIVE|IN6_IFF_DUPLICATED
> > > > > case would be
> > > > > VERY rare,
> > > > > 1115          * but is possible (for example) when we
> > > > > encounter an error while
> > > > > 1116          * forwarding procedure destined to a
> > > > > duplicated address of ours.
> > > > > 1117          */
> > > > > 1118         rt = rtalloc(sin6tosa(&sa6_dst), 0, rtableid);
> > > > > 1119         if (rtisvalid(rt) && ISSET(rt->rt_flags, RTF_LOCAL) &&
> > > > > 1120             !ISSET(ifatoia6(rt->rt_ifa)->ia6_flags,
> > > > > 1121
> > > > > IN6_IFF_ANYCAST|IN6_IFF_TENTATIVE|IN6_IFF_DUPLICATED)) {
> > > > > 1122                 src = &t;
> > > > > 1123         }
> > > > >
> > > >
> > > > I don't think this is the proper code extract because the
> > > > traceroute6 packet
> > > > is not addressed directly to us.
> > > > It's addressed to another global unicast address and appears to
> > > > time out
> > > > because the TTL gets decremented.
> > > >
> > > > So I think the code that gets executed in this case is :
> > > > 1127         if (src == NULL) {
> > > > 1128                 /*
> > > > 1129                  * This case matches to multicasts, our anycast,
> > > > or unicasts
> > > > 1130                  * that we do not own.  Select a source address
> > > > based on the
> > > > 1131                  * source address of the erroneous packet.
> > > > 1132                  */
> > > > 1133                 rt = rtalloc(sin6tosa(&sa6_src),
> > > > RT_RESOLVE, rtableid);
> > > > 1134                 if (!rtisvalid(rt)) {
> > > > 1135                         char addr[INET6_ADDRSTRLEN];
> > > > 1136
> > > > 1137                         nd6log((LOG_DEBUG,
> > > > 1138                             "%s: source can't be
> > > > determined: dst=%s\n",
> > > > 1139                             __func__, inet_ntop(AF_INET6,
> > > > &sa6_src.sin6_addr,
> > > > 1140                                 addr, sizeof(addr))));
> > > > 1141                         rtfree(rt);
> > > > 1142                         goto bad;
> > > > 1143                 }
> > > > 1144                 src = &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
> > > > 1145
> > > > 1146         }
> > > >
> > > > I was thinking of replacing lines 1144 and 1145 with something along
> > > > the lines of
> > > > src = in6_ifawithscope(rt->rt_ifa->ifa_ifp, &t, rtableid);
> > > > if (src=NULL)  src = &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
> > > >
> > > > Sorry if style is wrong, I'm not used to it, but you get the idea.
> > > >
> > >
> > > Currently building a test kernel with this diff.
> > > Index: netinet6/icmp6.c
> > > ===================================================================
> > > RCS file: /cvs/src/sys/netinet6/icmp6.c,v
> > > retrieving revision 1.227
> > > diff -u -p -u -p -r1.227 icmp6.c
> > > --- netinet6/icmp6.c    9 Nov 2018 14:14:32 -0000       1.227
> > > +++ netinet6/icmp6.c    28 Nov 2018 22:55:04 -0000
> > > @@ -1038,6 +1038,7 @@ icmp6_reflect(struct mbuf *m, size_t off
> > >         struct icmp6_hdr *icmp6;
> > >         struct in6_addr t, *src = NULL;
> > >         struct sockaddr_in6 sa6_src, sa6_dst;
> > > +       struct in6_ifaddr *ifa;
> > >         u_int rtableid;
> > >
> > >         CTASSERT(sizeof(struct ip6_hdr) + sizeof(struct icmp6_hdr)
> > > <= MHLEN);
> > > @@ -1141,7 +1142,9 @@ icmp6_reflect(struct mbuf *m, size_t off
> > >                         rtfree(rt);
> > >                         goto bad;
> > >                 }
> > > -               src = &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
> > > +               ifa = in6_ifawithscope(rt->rt_ifa->ifa_ifp, &t,
> > > rtableid);
> > > +               if (ifa != NULL) src = &(ifa->ia_addr.sin6_addr);
> > > +               if (src == NULL) src =
> > > &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
> > >         }
> > >
> > >         ip6->ip6_src = *src;
> > >
> > > But I have no -current machine in IPv6 paths to test it.
> > > I'll have to backport on -stable next week if I get a chance.
> > >
> > >
> > > > > > I have just looked at FreeBSD in6_ifawithifp(), it is quite simple.
> > > > > > Perhaps this is a way to go.
> > > > >
> > > > > The code in FreeBSD's icmp6.c matching the above calls
> > > > > in6ifa_ifwithaddr
> > > > > https://svnweb.freebsd.org/base/head/sys/netinet6/icmp6.c?revision=338831&view=markup#l2113
> > > > >
> > > >
> > > > Again, I think the branch hat gets executed is at
> > > > https://svnweb.freebsd.org/base/head/sys/netinet6/icmp6.c?revision=338831&view=markup#l2137
> > > > which calls
> > > > 2147                in6_splitscope(&ip6->ip6_src, &dst6, &scopeid);
> > > > 2148                error = in6_selectsrc_addr(M_GETFIB(m), &dst6,
> > > > 2149                    scopeid, NULL, &src6, &hlim);
> > > >
> > > > I can't find the in6_splitscope function in OpenBSD, so I guess
> > > > there's more to it than just copy-paste...
> > >
> > > Actually in6_selectsrc_addr seems to do something similar to what
> > > in6_ifawithscope does.
> > > So this part seems correct.
> > > What seems a little bit complicated is the call to rt_alloc just to
> > > get the *oifp for in6_ifawithscope.
> > > I though of it as a way to direct in6_ifawithscope to choose the
> > > interface's global unicast address.
> > > Perhaps there is a simpler way to obtain the pointer and/or the
> > > supposed effect, but I'm not familiar enough with the code yet...
> > >
> > > Thoughts ?
>

Reply | Threaded
Open this post in threaded view
|

Re: traceroute6 and ospf6d (icmp6 source addresses and link-locals)

Claudio Jeker
On Wed, Dec 05, 2018 at 12:02:25PM +0100, Denis Fondras wrote:

> On Wed, Dec 05, 2018 at 11:36:14AM +0100, Arnaud BRAND wrote:
> > Any feedback on this patch ?
> > I'm running it without problems since the 30th November.
> >
> >
> > Index: netinet6/icmp6.c
> > ===================================================================
> > RCS file: /cvs/src/sys/netinet6/icmp6.c,v
> > retrieving revision 1.227
> > diff -u -p -u -p -r1.227 icmp6.c
> > --- netinet6/icmp6.c    9 Nov 2018 14:14:32 -0000       1.227
> > +++ netinet6/icmp6.c    28 Nov 2018 22:55:04 -0000
> > @@ -1038,6 +1038,7 @@ icmp6_reflect(struct mbuf *m, size_t off
> >         struct icmp6_hdr *icmp6;
> >         struct in6_addr t, *src = NULL;
> >         struct sockaddr_in6 sa6_src, sa6_dst;
> > +       struct in6_ifaddr *ifa;
> >         u_int rtableid;
> >
> >         CTASSERT(sizeof(struct ip6_hdr) + sizeof(struct icmp6_hdr) <=
> > MHLEN);
> > @@ -1141,7 +1142,9 @@ icmp6_reflect(struct mbuf *m, size_t off
> >                         rtfree(rt);
> >                         goto bad;
> >                 }
> > -               src = &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
> > +               ifa = in6_ifawithscope(rt->rt_ifa->ifa_ifp, &t, rtableid);
> > +               if (ifa != NULL) src = &(ifa->ia_addr.sin6_addr);
> > +               if (src == NULL) src =
> > &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
> >         }
> >
> >         ip6->ip6_src = *src;
> >
>
> style(9) :)

But apart from that I think this is OK claudio@.
 

> >
> >
> > Le 2018-11-29 00:50, Arnaud BRAND a écrit :
> > > I have setup a test environment where I can reproduce the issue with
> > > GENERIC#481.
> > > I can confirm that the issue is fixed by the proposed patch.
> > > traceroute6 to/from/through the patched machine got expected result
> > > and did not crash the machine.
> > >
> > > Anyone else would like to try ?
> > > Or propose improvements ?
> > >
> > > Le 2018-11-29 00:02, Arnaud BRAND a écrit :
> > > > Le 2018-11-28 22:46, Arnaud BRAND a écrit :
> > > > > Le 2018-10-05 23:42, Stuart Henderson a écrit :
> > > > > > On 2018/10/05 18:38, Alexander Bluhm wrote:
> > > > > > > IPv6 Source selection is a mess!
> > > > > > >
> > > > > > > > ICMP6 messages
> > > > > > > > are generated with a source of, I think, the local address associated with
> > > > > > > > the route to the recipient,
> > > > > > >
> > > > > > > It is not that simple.  Look at in6_ifawithscope() in
> > > > > > > sys/netinet6/in6.c.
> > > > > >
> > > > > > I know that's used for newly generated packets, but I'm not
> > > > > > sure it's the
> > > > > > case for icmp6, I haven't tried modifying the kernel to
> > > > > > confirm for sure
> > > > > > that this is the code generating the address in this case,
> > > > > > but it seems
> > > > > > likely, in icmp6.c:
> > > > > >
> > > > > > 1111         /*
> > > > > > 1112          * If the incoming packet was addressed directly to us
> > > > > > (i.e. unicast),
> > > > > > 1113          * use dst as the src for the reply.
> > > > > > 1114          * The IN6_IFF_TENTATIVE|IN6_IFF_DUPLICATED
> > > > > > case would be
> > > > > > VERY rare,
> > > > > > 1115          * but is possible (for example) when we
> > > > > > encounter an error while
> > > > > > 1116          * forwarding procedure destined to a
> > > > > > duplicated address of ours.
> > > > > > 1117          */
> > > > > > 1118         rt = rtalloc(sin6tosa(&sa6_dst), 0, rtableid);
> > > > > > 1119         if (rtisvalid(rt) && ISSET(rt->rt_flags, RTF_LOCAL) &&
> > > > > > 1120             !ISSET(ifatoia6(rt->rt_ifa)->ia6_flags,
> > > > > > 1121
> > > > > > IN6_IFF_ANYCAST|IN6_IFF_TENTATIVE|IN6_IFF_DUPLICATED)) {
> > > > > > 1122                 src = &t;
> > > > > > 1123         }
> > > > > >
> > > > >
> > > > > I don't think this is the proper code extract because the
> > > > > traceroute6 packet
> > > > > is not addressed directly to us.
> > > > > It's addressed to another global unicast address and appears to
> > > > > time out
> > > > > because the TTL gets decremented.
> > > > >
> > > > > So I think the code that gets executed in this case is :
> > > > > 1127         if (src == NULL) {
> > > > > 1128                 /*
> > > > > 1129                  * This case matches to multicasts, our anycast,
> > > > > or unicasts
> > > > > 1130                  * that we do not own.  Select a source address
> > > > > based on the
> > > > > 1131                  * source address of the erroneous packet.
> > > > > 1132                  */
> > > > > 1133                 rt = rtalloc(sin6tosa(&sa6_src),
> > > > > RT_RESOLVE, rtableid);
> > > > > 1134                 if (!rtisvalid(rt)) {
> > > > > 1135                         char addr[INET6_ADDRSTRLEN];
> > > > > 1136
> > > > > 1137                         nd6log((LOG_DEBUG,
> > > > > 1138                             "%s: source can't be
> > > > > determined: dst=%s\n",
> > > > > 1139                             __func__, inet_ntop(AF_INET6,
> > > > > &sa6_src.sin6_addr,
> > > > > 1140                                 addr, sizeof(addr))));
> > > > > 1141                         rtfree(rt);
> > > > > 1142                         goto bad;
> > > > > 1143                 }
> > > > > 1144                 src = &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
> > > > > 1145
> > > > > 1146         }
> > > > >
> > > > > I was thinking of replacing lines 1144 and 1145 with something along
> > > > > the lines of
> > > > > src = in6_ifawithscope(rt->rt_ifa->ifa_ifp, &t, rtableid);
> > > > > if (src=NULL)  src = &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
> > > > >
> > > > > Sorry if style is wrong, I'm not used to it, but you get the idea.
> > > > >
> > > >
> > > > Currently building a test kernel with this diff.
> > > > Index: netinet6/icmp6.c
> > > > ===================================================================
> > > > RCS file: /cvs/src/sys/netinet6/icmp6.c,v
> > > > retrieving revision 1.227
> > > > diff -u -p -u -p -r1.227 icmp6.c
> > > > --- netinet6/icmp6.c    9 Nov 2018 14:14:32 -0000       1.227
> > > > +++ netinet6/icmp6.c    28 Nov 2018 22:55:04 -0000
> > > > @@ -1038,6 +1038,7 @@ icmp6_reflect(struct mbuf *m, size_t off
> > > >         struct icmp6_hdr *icmp6;
> > > >         struct in6_addr t, *src = NULL;
> > > >         struct sockaddr_in6 sa6_src, sa6_dst;
> > > > +       struct in6_ifaddr *ifa;
> > > >         u_int rtableid;
> > > >
> > > >         CTASSERT(sizeof(struct ip6_hdr) + sizeof(struct icmp6_hdr)
> > > > <= MHLEN);
> > > > @@ -1141,7 +1142,9 @@ icmp6_reflect(struct mbuf *m, size_t off
> > > >                         rtfree(rt);
> > > >                         goto bad;
> > > >                 }
> > > > -               src = &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
> > > > +               ifa = in6_ifawithscope(rt->rt_ifa->ifa_ifp, &t,
> > > > rtableid);
> > > > +               if (ifa != NULL) src = &(ifa->ia_addr.sin6_addr);
> > > > +               if (src == NULL) src =
> > > > &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
> > > >         }
> > > >
> > > >         ip6->ip6_src = *src;
> > > >
> > > > But I have no -current machine in IPv6 paths to test it.
> > > > I'll have to backport on -stable next week if I get a chance.
> > > >
> > > >
> > > > > > > I have just looked at FreeBSD in6_ifawithifp(), it is quite simple.
> > > > > > > Perhaps this is a way to go.
> > > > > >
> > > > > > The code in FreeBSD's icmp6.c matching the above calls
> > > > > > in6ifa_ifwithaddr
> > > > > > https://svnweb.freebsd.org/base/head/sys/netinet6/icmp6.c?revision=338831&view=markup#l2113
> > > > > >
> > > > >
> > > > > Again, I think the branch hat gets executed is at
> > > > > https://svnweb.freebsd.org/base/head/sys/netinet6/icmp6.c?revision=338831&view=markup#l2137
> > > > > which calls
> > > > > 2147                in6_splitscope(&ip6->ip6_src, &dst6, &scopeid);
> > > > > 2148                error = in6_selectsrc_addr(M_GETFIB(m), &dst6,
> > > > > 2149                    scopeid, NULL, &src6, &hlim);
> > > > >
> > > > > I can't find the in6_splitscope function in OpenBSD, so I guess
> > > > > there's more to it than just copy-paste...
> > > >
> > > > Actually in6_selectsrc_addr seems to do something similar to what
> > > > in6_ifawithscope does.
> > > > So this part seems correct.
> > > > What seems a little bit complicated is the call to rt_alloc just to
> > > > get the *oifp for in6_ifawithscope.
> > > > I though of it as a way to direct in6_ifawithscope to choose the
> > > > interface's global unicast address.
> > > > Perhaps there is a simpler way to obtain the pointer and/or the
> > > > supposed effect, but I'm not familiar enough with the code yet...
> > > >
> > > > Thoughts ?
> >
>

--
:wq Claudio

Reply | Threaded
Open this post in threaded view
|

Re: traceroute6 and ospf6d (icmp6 source addresses and link-locals)

Arnaud BRAND
In reply to this post by Denis Fondras
Le 2018-12-05 12:02, Denis Fondras a écrit :

> On Wed, Dec 05, 2018 at 11:36:14AM +0100, Arnaud BRAND wrote:
>> Any feedback on this patch ?
>> I'm running it without problems since the 30th November.
>>
>>
>> Index: netinet6/icmp6.c
>> ===================================================================
>> RCS file: /cvs/src/sys/netinet6/icmp6.c,v
>> retrieving revision 1.227
>> diff -u -p -u -p -r1.227 icmp6.c
>> --- netinet6/icmp6.c    9 Nov 2018 14:14:32 -0000       1.227
>> +++ netinet6/icmp6.c    28 Nov 2018 22:55:04 -0000
>> @@ -1038,6 +1038,7 @@ icmp6_reflect(struct mbuf *m, size_t off
>>         struct icmp6_hdr *icmp6;
>>         struct in6_addr t, *src = NULL;
>>         struct sockaddr_in6 sa6_src, sa6_dst;
>> +       struct in6_ifaddr *ifa;
>>         u_int rtableid;
>>
>>         CTASSERT(sizeof(struct ip6_hdr) + sizeof(struct icmp6_hdr) <=
>> MHLEN);
>> @@ -1141,7 +1142,9 @@ icmp6_reflect(struct mbuf *m, size_t off
>>                         rtfree(rt);
>>                         goto bad;
>>                 }
>> -               src = &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
>> +               ifa = in6_ifawithscope(rt->rt_ifa->ifa_ifp, &t,
>> rtableid);
>> +               if (ifa != NULL) src = &(ifa->ia_addr.sin6_addr);
>> +               if (src == NULL) src =
>> &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
>>         }
>>
>>         ip6->ip6_src = *src;
>>
>
> style(9) :)
>

OK, I thought I had this read this one carefully enough but it seems not
to be the case.

Are you referring to the new variable position ?
Rereading the examples it looks like I had not understood this rule
properly
(When declaring variables in functions, declare them sorted by size
(largest to smallest), then in alphabetical order;)

Or is there something else I missed ?

>>
>>
>> Le 2018-11-29 00:50, Arnaud BRAND a écrit :
>> > I have setup a test environment where I can reproduce the issue with
>> > GENERIC#481.
>> > I can confirm that the issue is fixed by the proposed patch.
>> > traceroute6 to/from/through the patched machine got expected result
>> > and did not crash the machine.
>> >
>> > Anyone else would like to try ?
>> > Or propose improvements ?
>> >
>> > Le 2018-11-29 00:02, Arnaud BRAND a écrit :
>> > > Le 2018-11-28 22:46, Arnaud BRAND a écrit :
>> > > > Le 2018-10-05 23:42, Stuart Henderson a écrit :
>> > > > > On 2018/10/05 18:38, Alexander Bluhm wrote:
>> > > > > > IPv6 Source selection is a mess!
>> > > > > >
>> > > > > > > ICMP6 messages
>> > > > > > > are generated with a source of, I think, the local address associated with
>> > > > > > > the route to the recipient,
>> > > > > >
>> > > > > > It is not that simple.  Look at in6_ifawithscope() in
>> > > > > > sys/netinet6/in6.c.
>> > > > >
>> > > > > I know that's used for newly generated packets, but I'm not
>> > > > > sure it's the
>> > > > > case for icmp6, I haven't tried modifying the kernel to
>> > > > > confirm for sure
>> > > > > that this is the code generating the address in this case,
>> > > > > but it seems
>> > > > > likely, in icmp6.c:
>> > > > >
>> > > > > 1111         /*
>> > > > > 1112          * If the incoming packet was addressed directly to us
>> > > > > (i.e. unicast),
>> > > > > 1113          * use dst as the src for the reply.
>> > > > > 1114          * The IN6_IFF_TENTATIVE|IN6_IFF_DUPLICATED
>> > > > > case would be
>> > > > > VERY rare,
>> > > > > 1115          * but is possible (for example) when we
>> > > > > encounter an error while
>> > > > > 1116          * forwarding procedure destined to a
>> > > > > duplicated address of ours.
>> > > > > 1117          */
>> > > > > 1118         rt = rtalloc(sin6tosa(&sa6_dst), 0, rtableid);
>> > > > > 1119         if (rtisvalid(rt) && ISSET(rt->rt_flags, RTF_LOCAL) &&
>> > > > > 1120             !ISSET(ifatoia6(rt->rt_ifa)->ia6_flags,
>> > > > > 1121
>> > > > > IN6_IFF_ANYCAST|IN6_IFF_TENTATIVE|IN6_IFF_DUPLICATED)) {
>> > > > > 1122                 src = &t;
>> > > > > 1123         }
>> > > > >
>> > > >
>> > > > I don't think this is the proper code extract because the
>> > > > traceroute6 packet
>> > > > is not addressed directly to us.
>> > > > It's addressed to another global unicast address and appears to
>> > > > time out
>> > > > because the TTL gets decremented.
>> > > >
>> > > > So I think the code that gets executed in this case is :
>> > > > 1127         if (src == NULL) {
>> > > > 1128                 /*
>> > > > 1129                  * This case matches to multicasts, our anycast,
>> > > > or unicasts
>> > > > 1130                  * that we do not own.  Select a source address
>> > > > based on the
>> > > > 1131                  * source address of the erroneous packet.
>> > > > 1132                  */
>> > > > 1133                 rt = rtalloc(sin6tosa(&sa6_src),
>> > > > RT_RESOLVE, rtableid);
>> > > > 1134                 if (!rtisvalid(rt)) {
>> > > > 1135                         char addr[INET6_ADDRSTRLEN];
>> > > > 1136
>> > > > 1137                         nd6log((LOG_DEBUG,
>> > > > 1138                             "%s: source can't be
>> > > > determined: dst=%s\n",
>> > > > 1139                             __func__, inet_ntop(AF_INET6,
>> > > > &sa6_src.sin6_addr,
>> > > > 1140                                 addr, sizeof(addr))));
>> > > > 1141                         rtfree(rt);
>> > > > 1142                         goto bad;
>> > > > 1143                 }
>> > > > 1144                 src = &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
>> > > > 1145
>> > > > 1146         }
>> > > >
>> > > > I was thinking of replacing lines 1144 and 1145 with something along
>> > > > the lines of
>> > > > src = in6_ifawithscope(rt->rt_ifa->ifa_ifp, &t, rtableid);
>> > > > if (src=NULL)  src = &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
>> > > >
>> > > > Sorry if style is wrong, I'm not used to it, but you get the idea.
>> > > >
>> > >
>> > > Currently building a test kernel with this diff.
>> > > Index: netinet6/icmp6.c
>> > > ===================================================================
>> > > RCS file: /cvs/src/sys/netinet6/icmp6.c,v
>> > > retrieving revision 1.227
>> > > diff -u -p -u -p -r1.227 icmp6.c
>> > > --- netinet6/icmp6.c    9 Nov 2018 14:14:32 -0000       1.227
>> > > +++ netinet6/icmp6.c    28 Nov 2018 22:55:04 -0000
>> > > @@ -1038,6 +1038,7 @@ icmp6_reflect(struct mbuf *m, size_t off
>> > >         struct icmp6_hdr *icmp6;
>> > >         struct in6_addr t, *src = NULL;
>> > >         struct sockaddr_in6 sa6_src, sa6_dst;
>> > > +       struct in6_ifaddr *ifa;
>> > >         u_int rtableid;
>> > >
>> > >         CTASSERT(sizeof(struct ip6_hdr) + sizeof(struct icmp6_hdr)
>> > > <= MHLEN);
>> > > @@ -1141,7 +1142,9 @@ icmp6_reflect(struct mbuf *m, size_t off
>> > >                         rtfree(rt);
>> > >                         goto bad;
>> > >                 }
>> > > -               src = &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
>> > > +               ifa = in6_ifawithscope(rt->rt_ifa->ifa_ifp, &t,
>> > > rtableid);
>> > > +               if (ifa != NULL) src = &(ifa->ia_addr.sin6_addr);
>> > > +               if (src == NULL) src =
>> > > &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
>> > >         }
>> > >
>> > >         ip6->ip6_src = *src;
>> > >
>> > > But I have no -current machine in IPv6 paths to test it.
>> > > I'll have to backport on -stable next week if I get a chance.
>> > >
>> > >
>> > > > > > I have just looked at FreeBSD in6_ifawithifp(), it is quite simple.
>> > > > > > Perhaps this is a way to go.
>> > > > >
>> > > > > The code in FreeBSD's icmp6.c matching the above calls
>> > > > > in6ifa_ifwithaddr
>> > > > > https://svnweb.freebsd.org/base/head/sys/netinet6/icmp6.c?revision=338831&view=markup#l2113
>> > > > >
>> > > >
>> > > > Again, I think the branch hat gets executed is at
>> > > > https://svnweb.freebsd.org/base/head/sys/netinet6/icmp6.c?revision=338831&view=markup#l2137
>> > > > which calls
>> > > > 2147                in6_splitscope(&ip6->ip6_src, &dst6, &scopeid);
>> > > > 2148                error = in6_selectsrc_addr(M_GETFIB(m), &dst6,
>> > > > 2149                    scopeid, NULL, &src6, &hlim);
>> > > >
>> > > > I can't find the in6_splitscope function in OpenBSD, so I guess
>> > > > there's more to it than just copy-paste...
>> > >
>> > > Actually in6_selectsrc_addr seems to do something similar to what
>> > > in6_ifawithscope does.
>> > > So this part seems correct.
>> > > What seems a little bit complicated is the call to rt_alloc just to
>> > > get the *oifp for in6_ifawithscope.
>> > > I though of it as a way to direct in6_ifawithscope to choose the
>> > > interface's global unicast address.
>> > > Perhaps there is a simpler way to obtain the pointer and/or the
>> > > supposed effect, but I'm not familiar enough with the code yet...
>> > >
>> > > Thoughts ?
>>

Reply | Threaded
Open this post in threaded view
|

Re: traceroute6 and ospf6d (icmp6 source addresses and link-locals)

Stuart Henderson
On 2018/12/05 12:40, Arnaud BRAND wrote:
> Or is there something else I missed ?

> > > +               if (ifa != NULL) src = &(ifa->ia_addr.sin6_addr);
> > > +               if (src == NULL) src = &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;

                if (ifa != NULL)
                        src = &(ifa->ia_addr.sin6_addr);
                if (src == NULL)
                        src = &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;

Reply | Threaded
Open this post in threaded view
|

Re: traceroute6 and ospf6d (icmp6 source addresses and link-locals)

Arnaud BRAND
Le 2018-12-05 13:03, Stuart Henderson a écrit :

> On 2018/12/05 12:40, Arnaud BRAND wrote:
>> Or is there something else I missed ?
>
>> > > +               if (ifa != NULL) src = &(ifa->ia_addr.sin6_addr);
>> > > +               if (src == NULL) src = &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
>
> if (ifa != NULL)
> src = &(ifa->ia_addr.sin6_addr);
> if (src == NULL)
> src = &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;

Of course, that's quite me. The more obvious, the less I spot it.
I could have spent hours reading style(9) and writing code to compare
the size of structs.

Thanks a lot Stuart !

Here is an updated diff :

Index: sys/netinet6/icmp6.c
===================================================================
RCS file: /cvs/src/sys/netinet6/icmp6.c,v
retrieving revision 1.227
diff -u -p -u -p -r1.227 icmp6.c
--- sys/netinet6/icmp6.c        9 Nov 2018 14:14:32 -0000       1.227
+++ sys/netinet6/icmp6.c        5 Dec 2018 13:31:02 -0000
@@ -1038,6 +1038,7 @@ icmp6_reflect(struct mbuf *m, size_t off
         struct icmp6_hdr *icmp6;
         struct in6_addr t, *src = NULL;
         struct sockaddr_in6 sa6_src, sa6_dst;
+       struct in6_ifaddr *ifa;
         u_int rtableid;

         CTASSERT(sizeof(struct ip6_hdr) + sizeof(struct icmp6_hdr) <=
MHLEN);
@@ -1141,7 +1142,11 @@ icmp6_reflect(struct mbuf *m, size_t off
                         rtfree(rt);
                         goto bad;
                 }
-               src = &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
+               ifa = in6_ifawithscope(rt->rt_ifa->ifa_ifp, &t,
rtableid);
+               if (ifa != NULL)
+                       src = &(ifa->ia_addr.sin6_addr);
+               if (src == NULL)
+                       src = &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
         }

         ip6->ip6_src = *src;