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 via email to