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