Changeset View
Standalone View
sys/netinet6/icmp6.c
Show First 20 Lines • Show All 2,058 Lines • ▼ Show 20 Lines | |||||
{ | { | ||||
struct in6_addr src6, *srcp; | struct in6_addr src6, *srcp; | ||||
struct ip6_hdr *ip6; | struct ip6_hdr *ip6; | ||||
struct icmp6_hdr *icmp6; | struct icmp6_hdr *icmp6; | ||||
struct in6_ifaddr *ia = NULL; | struct in6_ifaddr *ia = NULL; | ||||
struct ifnet *outif = NULL; | struct ifnet *outif = NULL; | ||||
int plen; | int plen; | ||||
int type, code, hlim; | int type, code, hlim; | ||||
int fibnum; | |||||
/* too short to reflect */ | /* too short to reflect */ | ||||
if (off < sizeof(struct ip6_hdr)) { | if (off < sizeof(struct ip6_hdr)) { | ||||
nd6log((LOG_DEBUG, | nd6log((LOG_DEBUG, | ||||
"sanity fail: off=%lx, sizeof(ip6)=%lx in %s:%d\n", | "sanity fail: off=%lx, sizeof(ip6)=%lx in %s:%d\n", | ||||
(u_long)off, (u_long)sizeof(struct ip6_hdr), | (u_long)off, (u_long)sizeof(struct ip6_hdr), | ||||
__FILE__, __LINE__)); | __FILE__, __LINE__)); | ||||
goto bad; | goto bad; | ||||
▲ Show 20 Lines • Show All 56 Lines • ▼ Show 20 Lines | if (ia != NULL && !(ia->ia6_flags & | ||||
hlim = ND_IFINFO(m->m_pkthdr.rcvif)->chlim; | hlim = ND_IFINFO(m->m_pkthdr.rcvif)->chlim; | ||||
} else | } else | ||||
hlim = V_ip6_defhlim; | hlim = V_ip6_defhlim; | ||||
} | } | ||||
if (ia != NULL) | if (ia != NULL) | ||||
ifa_free(&ia->ia_ifa); | ifa_free(&ia->ia_ifa); | ||||
} | } | ||||
ia = in6ifa_ifwithaddr(&ip6->ip6_dst, 0 /* XXX */); | |||||
if (ia != NULL) { | |||||
fibnum = ia->ia_ifp->if_fib; | |||||
ifa_free(&ia->ia_ifa); | |||||
} else | |||||
fibnum = RT_DEFAULT_FIB; | |||||
M_SETFIB(m, fibnum); | |||||
asomers: It seems like this code selects the incoming interface's FIB for routing an ICMPv6 response. | |||||
jhujhiti_adjectivism.orgAuthorUnsubmitted Done Inline ActionsYou do understand correctly, and trying to answer why equivalent code is not needed in icmp_reflect() caused me to realize that this logic is mostly redundant. icmp6_reflect() callers copy the FIB when creating the mbuf of the packet to reflect, and RT_DEFAULT_FIB will get passed to in6_selectsrc_addr() below in *almost* all of the same cases as it did before. The one case that seems worth thinking about more is the case where the packet is destined to an anycast address. The logic on lines 2125-2126 avoid selecting that address as a source. Since srcp will not be set in this case, we will end up selecting a source address for the response from the default FIB on line 2159 (assuming my change had been reverted). This seems like a case where we should attempt to reply from some other address in the same FIB. Does this logic make sense? If so, I'll rework the FIB selection logic above to be less redundant. Alternatively, I notice that a note in RFC 6724, Appendix B suggests that anycast source addresses may be valid (https://tools.ietf.org/html/rfc6724 page 30, section 3), so perhaps avoiding selecting the anycast address as a source for the reply is no longer necessary (although reworking source address selection in general is definitely not in scope for my change). jhujhiti_adjectivism.org: You do understand correctly, and trying to answer why equivalent code is not needed in… | |||||
asomersUnsubmitted Done Inline ActionsAt line 2159, do we know the fib of the receiving interface? If so, we should probably use it. The interface fib is the fib to use when forwarding packets, so it makes sense to use it for ICMP replies, too. Your observation about RFC 6724 is interesting, but I don't think we should act on it as part of this commit. It should go in separately. asomers: At line 2159, do we know the fib of the receiving interface? If so, we should probably use it. | |||||
jhujhiti_adjectivism.orgAuthorUnsubmitted Done Inline Actions
It looks like the mbuf passed to icmp6_reflect() will always have the FIB set based on the receiving interface, so we can just use that! Can a more experienced set of eyes confirm? jhujhiti_adjectivism.org: > At line 2159, do we know the fib of the receiving interface?
It looks like the mbuf passed… | |||||
jhujhiti_adjectivism.orgAuthorUnsubmitted Done Inline Actions@asomers, can you confirm that M_GETFIB(m) is always correctly set to the FIB of the receiving interface? jhujhiti_adjectivism.org: @asomers, can you confirm that M_GETFIB(m) is always correctly set to the FIB of the receiving… | |||||
asomersUnsubmitted Not Done Inline ActionsNo. According to the comment at the bottom of icmp6_error, it isn't, because icmp6_reflect can sometimes be called from the output path. In that case, there wouldn't be a receiving interface. However, M_GETFIB(m) will definitely return _a_ fib. It will either have the fib of the receiving interface if this is on the receive path, or the fib of the socket if this is on the output path. Socket fibs default to the process fib, which cannot be RT_ALL_FIBS, but they can also be set by setsockopt(..., SO_SETFIB, ...), which also doesn't allow RT_ALL_FIBS. So I think your code is ok here. asomers: No. According to the comment at the bottom of icmp6_error, it isn't, because icmp6_reflect can… | |||||
jhujhiti_adjectivism.orgAuthorUnsubmitted Not Done Inline ActionsAh yes, that's how I should have phrased it - valid FIB even if called in the output path. jhujhiti_adjectivism.org: Ah yes, that's how I should have phrased it - valid FIB even if called in the output path. | |||||
if (srcp == NULL) { | if (srcp == NULL) { | ||||
int error; | int error; | ||||
struct in6_addr dst6; | struct in6_addr dst6; | ||||
uint32_t scopeid; | uint32_t scopeid; | ||||
/* | /* | ||||
* This case matches to multicasts, our anycast, or unicasts | * This case matches to multicasts, our anycast, or unicasts | ||||
* that we do not own. Select a source address based on the | * that we do not own. Select a source address based on the | ||||
* source address of the erroneous packet. | * source address of the erroneous packet. | ||||
*/ | */ | ||||
in6_splitscope(&ip6->ip6_src, &dst6, &scopeid); | in6_splitscope(&ip6->ip6_src, &dst6, &scopeid); | ||||
error = in6_selectsrc_addr(RT_DEFAULT_FIB, &dst6, | error = in6_selectsrc_addr(fibnum, &dst6, | ||||
scopeid, NULL, &src6, &hlim); | scopeid, NULL, &src6, &hlim); | ||||
if (error) { | if (error) { | ||||
char ip6buf[INET6_ADDRSTRLEN]; | char ip6buf[INET6_ADDRSTRLEN]; | ||||
nd6log((LOG_DEBUG, | nd6log((LOG_DEBUG, | ||||
"icmp6_reflect: source can't be determined: " | "icmp6_reflect: source can't be determined: " | ||||
"dst=%s, error=%d\n", | "dst=%s, error=%d\n", | ||||
ip6_sprintf(ip6buf, &ip6->ip6_dst), error)); | ip6_sprintf(ip6buf, &ip6->ip6_dst), error)); | ||||
▲ Show 20 Lines • Show All 125 Lines • ▼ Show 20 Lines | #endif | ||||
} | } | ||||
{ | { | ||||
/* ip6->ip6_src must be equal to gw for icmp6->icmp6_reddst */ | /* ip6->ip6_src must be equal to gw for icmp6->icmp6_reddst */ | ||||
struct nhop6_basic nh6; | struct nhop6_basic nh6; | ||||
struct in6_addr kdst; | struct in6_addr kdst; | ||||
uint32_t scopeid; | uint32_t scopeid; | ||||
in6_splitscope(&reddst6, &kdst, &scopeid); | in6_splitscope(&reddst6, &kdst, &scopeid); | ||||
if (fib6_lookup_nh_basic(RT_DEFAULT_FIB, &kdst, scopeid, 0, 0,&nh6)==0){ | if (fib6_lookup_nh_basic(ifp->if_fib, &kdst, scopeid, 0, 0,&nh6)==0){ | ||||
if ((nh6.nh_flags & NHF_GATEWAY) == 0) { | if ((nh6.nh_flags & NHF_GATEWAY) == 0) { | ||||
nd6log((LOG_ERR, | nd6log((LOG_ERR, | ||||
"ICMP6 redirect rejected; no route " | "ICMP6 redirect rejected; no route " | ||||
"with inet6 gateway found for redirect dst: %s\n", | "with inet6 gateway found for redirect dst: %s\n", | ||||
icmp6_redirect_diag(&src6, &reddst6, &redtgt6))); | icmp6_redirect_diag(&src6, &reddst6, &redtgt6))); | ||||
goto bad; | goto bad; | ||||
} | } | ||||
▲ Show 20 Lines • Show All 513 Lines • Show Last 20 Lines |
It seems like this code selects the incoming interface's FIB for routing an ICMPv6 response. Do I understand that correctly? If so, why is similar code not needed in icmp_reflect?