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 ?
