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
