Re: svn commit: r310847 - head/sys/netinet
Josh,
if you don't mind, here is some review inlined down below.
On Fri, Dec 30, 2016 at 06:46:21PM +, Josh Paetzel wrote:
J> Author: jpaetzel
J> Date: Fri Dec 30 18:46:21 2016
J> New Revision: 310847
J> URL: https://svnweb.freebsd.org/changeset/base/310847
J>
J> Log:
J> Harden CARP against network loops.
J>
J> If there is a loop in the network a CARP that is in MASTER state will see
it's
J> own broadcasts, which will then cause it to assume BACKUP state. When it
J> assumes BACKUP it will stop sending advertisements. In that state it will
no
J> longer see advertisements and will assume MASTER...
J>
J> We can't catch all the cases where we are seeing our own CARP broadcast,
but
J> we can catch the obvious case.
J>
J> Submitted by: torek
J> Obtained from: FreeNAS
J> MFC after: 2 weeks
J> Sponsored by: iXsystems
J>
J> Modified:
J> head/sys/netinet/ip_carp.c
J>
J> Modified: head/sys/netinet/ip_carp.c
J>
==
J> --- head/sys/netinet/ip_carp.c Fri Dec 30 18:23:58 2016
(r310846)
J> +++ head/sys/netinet/ip_carp.c Fri Dec 30 18:46:21 2016
(r310847)
J> @@ -581,27 +581,90 @@ carp6_input(struct mbuf **mp, int *offp,
J> }
J> #endif /* INET6 */
J>
J> +/*
J> + * This routine should not be necessary at all, but some switches
J> + * (VMWare ESX vswitches) can echo our own packets back at us,
J> + * and we must ignore them or they will cause us to drop out of
J> + * MASTER mode.
J> + *
J> + * We cannot catch all cases of network loops. Instead, what we
J> + * do here is catch any packet that arrives with a carp header
J> + * with a VHID of 0, that comes from an address that is our own.
J> + * These packets are by definition "from us" (even if they are from
J> + * a misconfigured host that is pretending to be us).
I failed to find in the patch a place where we generate packets with
VHID 0, we only check for them. Can you please explain how this loop
detection works?
J> + * The VHID test is outside this mini-function.
J> + */
J> +static int
J> +carp_source_is_self(struct mbuf *m, struct ifaddr *ifa, sa_family_t af)
J> +{
J> +struct ip *ip4;
J> +struct in_addr in4;
J> +struct ip6_hdr *ip6;
J> +struct in6_addr in6;
J> +
J> +switch (af) {
J> +case AF_INET:
J> +ip4 = mtod(m, struct ip *);
J> +in4 = ifatoia(ifa)->ia_addr.sin_addr;
J> +return (in4.s_addr == ip4->ip_src.s_addr);
J> +
J> +case AF_INET6:
J> +ip6 = mtod(m, struct ip6_hdr *);
J> +in6 = ifatoia6(ifa)->ia_addr.sin6_addr;
J> +return (memcmp(&in6, &ip6->ip6_src, sizeof(in6)) == 0);
J> +
J> +default:/* how did this happen? */
J> +break;
This must be panic().
J> +}
J> +return (0);
J> +}
J> +
J> static void
J> carp_input_c(struct mbuf *m, struct carp_header *ch, sa_family_t af)
J> {
J> struct ifnet *ifp = m->m_pkthdr.rcvif;
J> -struct ifaddr *ifa;
J> +struct ifaddr *ifa, *match;
J> struct carp_softc *sc;
J> uint64_t tmp_counter;
J> struct timeval sc_tv, ch_tv;
J> +int error;
J>
J> -/* verify that the VHID is valid on the receiving interface */
J> +/*
J> + * Verify that the VHID is valid on the receiving interface.
J> + *
J> + * There should be just one match. If there are none
J> + * the VHID is not valid and we drop the packet. If
J> + * there are multiple VHID matches, take just the first
J> + * one, for compatibility with previous code. While we're
J> + * scanning, check for obvious loops in the network topology
J> + * (these should never happen, and as noted above, we may
J> + * miss real loops; this is just a double-check).
J> + */
J> IF_ADDR_RLOCK(ifp);
J> -IFNET_FOREACH_IFA(ifp, ifa)
J> -if (ifa->ifa_addr->sa_family == af &&
J> -ifa->ifa_carp->sc_vhid == ch->carp_vhid) {
J> -ifa_ref(ifa);
J> -break;
J> -}
J> +error = 0;
J> +match = NULL;
J> +IFNET_FOREACH_IFA(ifp, ifa) {
J> +if (match == NULL && ifa->ifa_carp != NULL &&
J> +ifa->ifa_addr->sa_family == af &&
J> +ifa->ifa_carp->sc_vhid == ch->carp_vhid)
J> +match = ifa;
J> +if (ch->carp_vhid == 0 && carp_source_is_self(m, ifa, af))
J> +error = ELOOP;
J> +}
J> +ifa = error ? NULL : match;
J> +if (ifa != NULL)
J> +ifa_ref(ifa);
J> IF_ADDR_RUNLOCK(ifp);
J>
J> if (ifa == NULL) {
J> -CARPSTATS_INC(carps_badvhid);
J> +if (error == ELOOP) {
J> +CARP_DEBUG("dropping looped packet on interface %s\n",
J> +ifp->if_xname);
J> +CARPSTATS_INC(carps_badif); /* ??? */
J> +} else {
J> +CARPS
svn commit: r310847 - head/sys/netinet
Author: jpaetzel
Date: Fri Dec 30 18:46:21 2016
New Revision: 310847
URL: https://svnweb.freebsd.org/changeset/base/310847
Log:
Harden CARP against network loops.
If there is a loop in the network a CARP that is in MASTER state will see it's
own broadcasts, which will then cause it to assume BACKUP state. When it
assumes BACKUP it will stop sending advertisements. In that state it will no
longer see advertisements and will assume MASTER...
We can't catch all the cases where we are seeing our own CARP broadcast, but
we can catch the obvious case.
Submitted by: torek
Obtained from:FreeNAS
MFC after:2 weeks
Sponsored by: iXsystems
Modified:
head/sys/netinet/ip_carp.c
Modified: head/sys/netinet/ip_carp.c
==
--- head/sys/netinet/ip_carp.c Fri Dec 30 18:23:58 2016(r310846)
+++ head/sys/netinet/ip_carp.c Fri Dec 30 18:46:21 2016(r310847)
@@ -581,27 +581,90 @@ carp6_input(struct mbuf **mp, int *offp,
}
#endif /* INET6 */
+/*
+ * This routine should not be necessary at all, but some switches
+ * (VMWare ESX vswitches) can echo our own packets back at us,
+ * and we must ignore them or they will cause us to drop out of
+ * MASTER mode.
+ *
+ * We cannot catch all cases of network loops. Instead, what we
+ * do here is catch any packet that arrives with a carp header
+ * with a VHID of 0, that comes from an address that is our own.
+ * These packets are by definition "from us" (even if they are from
+ * a misconfigured host that is pretending to be us).
+ *
+ * The VHID test is outside this mini-function.
+ */
+static int
+carp_source_is_self(struct mbuf *m, struct ifaddr *ifa, sa_family_t af)
+{
+ struct ip *ip4;
+ struct in_addr in4;
+ struct ip6_hdr *ip6;
+ struct in6_addr in6;
+
+ switch (af) {
+ case AF_INET:
+ ip4 = mtod(m, struct ip *);
+ in4 = ifatoia(ifa)->ia_addr.sin_addr;
+ return (in4.s_addr == ip4->ip_src.s_addr);
+
+ case AF_INET6:
+ ip6 = mtod(m, struct ip6_hdr *);
+ in6 = ifatoia6(ifa)->ia_addr.sin6_addr;
+ return (memcmp(&in6, &ip6->ip6_src, sizeof(in6)) == 0);
+
+ default:/* how did this happen? */
+ break;
+ }
+ return (0);
+}
+
static void
carp_input_c(struct mbuf *m, struct carp_header *ch, sa_family_t af)
{
struct ifnet *ifp = m->m_pkthdr.rcvif;
- struct ifaddr *ifa;
+ struct ifaddr *ifa, *match;
struct carp_softc *sc;
uint64_t tmp_counter;
struct timeval sc_tv, ch_tv;
+ int error;
- /* verify that the VHID is valid on the receiving interface */
+ /*
+* Verify that the VHID is valid on the receiving interface.
+*
+* There should be just one match. If there are none
+* the VHID is not valid and we drop the packet. If
+* there are multiple VHID matches, take just the first
+* one, for compatibility with previous code. While we're
+* scanning, check for obvious loops in the network topology
+* (these should never happen, and as noted above, we may
+* miss real loops; this is just a double-check).
+*/
IF_ADDR_RLOCK(ifp);
- IFNET_FOREACH_IFA(ifp, ifa)
- if (ifa->ifa_addr->sa_family == af &&
- ifa->ifa_carp->sc_vhid == ch->carp_vhid) {
- ifa_ref(ifa);
- break;
- }
+ error = 0;
+ match = NULL;
+ IFNET_FOREACH_IFA(ifp, ifa) {
+ if (match == NULL && ifa->ifa_carp != NULL &&
+ ifa->ifa_addr->sa_family == af &&
+ ifa->ifa_carp->sc_vhid == ch->carp_vhid)
+ match = ifa;
+ if (ch->carp_vhid == 0 && carp_source_is_self(m, ifa, af))
+ error = ELOOP;
+ }
+ ifa = error ? NULL : match;
+ if (ifa != NULL)
+ ifa_ref(ifa);
IF_ADDR_RUNLOCK(ifp);
if (ifa == NULL) {
- CARPSTATS_INC(carps_badvhid);
+ if (error == ELOOP) {
+ CARP_DEBUG("dropping looped packet on interface %s\n",
+ ifp->if_xname);
+ CARPSTATS_INC(carps_badif); /* ??? */
+ } else {
+ CARPSTATS_INC(carps_badvhid);
+ }
m_freem(m);
return;
}
@@ -787,12 +850,41 @@ carp_send_ad_error(struct carp_softc *sc
}
}
+/*
+ * Pick the best ifaddr on the given ifp for sending CARP
+ * advertisements.
+ *
+ * "Best" here is defined by ifa_preferred(). This function is much
+ * much like ifaof_ifpforaddr() except that we just use ifa_preferred().
+ *
+ * (This could be simplified to return the actual address, except that
+ * it has a d
