carp: add a 'SUPPRESS' state

[kp]

Summary

Introduce a new state for carp interfaces where the link or interface
are down, or net.inet.carp.allow is 0.

The intent is to make it a bit easier for users to figure out why a
given carp address is not negotiating MASTER/BACKUP.

Suggested by: melifaro@
Sponsored by: Rubicon Communications, LLC ("Netgate")

[kp]

There's a rather annoying bug left in this. If we add a carp address on an interface that's down (i.e. IFF_UP is not set) the interface will come up during this, but remain in SUPPRESS state.

That's because the SIOCSIFADDR ioctl is passed to ether_ioctl(), which sets IFF_UP in ifp->if_flags directly, without any event handling. Ordinarily we'd call if_up(), which gives carp a chance to react to the interface coming up. That's not the case here, so it stays in SUPPRESS.

Initial attempts to fix this, by replacing ifp->if_flags |= IFF_UP in ether_ioctl() with if_up(ifp) resulted in ifconfig: ioctl (SIOCAIFADDR): File exists when trying to set an address on the interface. There's clearly an order of operations problem around this, but I don't fully understand it.

In other words: this patch cannot go in as-is (or at least not until the IFF_UP issues are addressed).

[zlei]

When I was reviewing the design of CARP, I intended to introduce a new state COMPETING to resolve an annoying bug https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=269908 .

I'm not sure it is right to do so, too little resources for CARP rather than VRRP.

I'd suggest suspend this new feature, until we have a clear state machine.

The intent is to make it a bit easier for users to figure out why a given carp address is not negotiating MASTER/BACKUP.

I think we may have some debug / trace logs for that, other than a new state SUPPRESS.

[melifaro]

In D40317#917957, @kp wrote:

There's a rather annoying bug left in this. If we add a carp address on an interface that's down (i.e. IFF_UP is not set) the interface will come up during this, but remain in SUPPRESS state.

That's because the SIOCSIFADDR ioctl is passed to ether_ioctl(), which sets IFF_UP in ifp->if_flags directly, without any event handling. Ordinarily we'd call if_up(), which gives carp a chance to react to the interface coming up. That's not the case here, so it stays in SUPPRESS.

Initial attempts to fix this, by replacing ifp->if_flags |= IFF_UP in ether_ioctl() with if_up(ifp) resulted in ifconfig: ioctl (SIOCAIFADDR): File exists when trying to set an address on the interface. There's clearly an order of operations problem around this, but I don't fully understand it.

In other words: this patch cannot go in as-is (or at least not until the IFF_UP issues are addressed).

I ran into that when using netlink to configure interfaces. I guess we need to issue an eventhandler contification if some of interface flags were changed. I’ll take a look at that

[melifaro]

In other words: this patch cannot go in as-is (or at least not until the IFF_UP issues are addressed).

I ran into that when using netlink to configure interfaces. I guess we need to issue an eventhandler contification if some of interface flags were changed. I’ll take a look at that

I've raised D40332 to address lack of notifications to carp and the rest of the system.

[melifaro]

In D40317#918251, @melifaro wrote:

In other words: this patch cannot go in as-is (or at least not until the IFF_UP issues are addressed).

I ran into that when using netlink to configure interfaces. I guess we need to issue an eventhandler contification if some of interface flags were changed. I’ll take a look at that

I've raised D40332 to address lack of notifications to carp and the rest of the system.

This one got committed.

[rcm]

sys/netinet/ip_carp.h

131

Should this be bumped to 3?

[kp]

sys/netinet/ip_carp.h

131

It should, but we should probably also improve the parsing on the set state side of things, because I don't think we want users to manually set 'SUPPRESS' (or to allow users to manually leave that state, for that matter).

[melifaro]

sys/netinet/ip_carp.h

131

I don't have the domain knowledge on the carp implementation and I don't hold any opinion on whether the state machine should be extended or not.

Personally I'd consider two things here:

1. does the state machine change simplify (or help) kernel handling?
2. does the state machine change simplify (or help) user handling?

If the answer for the first question is "no" then, maybe, (2) may be achieved by the other means (e.g. just providing string hint for the userland) ?

[glebius]

This creates invariant (sc_suppress == 1 && sc_state == SUPPRESS || sc_suppress == 0 && sc_state != SUPPRESS). I'm not in favor of this change, but if it goes in, I would suggest to remove sc_suppress fields and use sc_state as a flag of suppressed state.

sys/netinet/ip_carp.h

 /*
  * Configuration structure for SIOCSVH SIOCGVH
  */
 struct carpreq {
         int             carpr_count;
         int             carpr_vhid;
 #define CARP_MAXVHID    255
         int             carpr_state;
-#define CARP_STATES     "INIT", "BACKUP", "MASTER"
+#define CARP_STATES     "INIT", "BACKUP", "MASTER", "SUPPRESS"
 #define CARP_MAXSTATE   2
         int             carpr_advskew;
 #define CARP_MAXSKEW    240
         int             carpr_advbase;
         unsigned char   carpr_key[CARP_KEY_LEN];
 };
 #define SIOCSVH _IOWR('i', 245, struct ifreq)
 #define SIOCGVH _IOWR('i', 246, struct ifreq)
 

sys/netinet/ip_carp.c

#endif
         int                     sc_advskew;
         int                     sc_advbase;
         struct in_addr          sc_carpaddr;
         struct in6_addr         sc_carpaddr6;
 
         int                     sc_naddrs;
         int                     sc_naddrs6;
         int                     sc_ifasiz;
-        enum { INIT = 0, BACKUP, MASTER }        sc_state;
+        enum { INIT = 0, BACKUP, MASTER, SUPPRESS }        sc_state;
         int                     sc_suppress;
         int                     sc_sendad_errors;
 #define CARP_SENDAD_MAX_ERRORS  3
         int                     sc_sendad_success;
 #define CARP_SENDAD_MIN_SUCCESS 3
 
         int                     sc_init_counter;
         uint64_t                sc_counter;
carp_input_c(struct mbuf *m, struct carp_header *ch, sa_family_t af, int ttl)
         sc->sc_counter = tmp_counter;
 
         sc_tv.tv_sec = sc->sc_advbase;
         sc_tv.tv_usec = DEMOTE_ADVSKEW(sc) * 1000000 / 256;
         ch_tv.tv_sec = ch->carp_advbase;
         ch_tv.tv_usec = ch->carp_advskew * 1000000 / 256;
 
         switch (sc->sc_state) {
+        case SUPPRESS:
         case INIT:
                 break;
         case MASTER:
                 /*
                  * If we receive an advertisement from a master who's going to
                  * be more frequent than us, go into BACKUP state.
                  */
                 if (timevalcmp(&sc_tv, &ch_tv, >) ||
#ifdef INET
                 carp_send_arp(sc);
 #endif
 #ifdef INET6
                 carp_send_na(sc);
 #endif
                 carp_setrun(sc, 0);
                 carp_addroute(sc);
                 break;
+        case SUPPRESS:
         case INIT:
         case MASTER:
 #ifdef INVARIANTS
                 panic("carp: VHID %u@%s: master_down event in %s state\n",
                     sc->sc_vhid,
                     if_name(sc->sc_carpdev),
                     sc->sc_state ? "MASTER" : "INIT");
 #endif
carp_setrun(struct carp_softc *sc, sa_family_t af)
 
         if ((sc->sc_carpdev->if_flags & IFF_UP) == 0 ||
             sc->sc_carpdev->if_link_state != LINK_STATE_UP ||
             (sc->sc_naddrs == 0 && sc->sc_naddrs6 == 0) ||
             !V_carp_allow)
                 return;
 
         switch (sc->sc_state) {
+        case SUPPRESS:
         case INIT:
                 carp_set_state(sc, BACKUP, "initialization complete");
                 carp_setrun(sc, 0);
                 break;
         case BACKUP:
                 callout_stop(&sc->sc_ad_tmo);
                 tv.tv_sec = 3 * sc->sc_advbase;
                 tv.tv_usec = sc->sc_advskew * 1000000 / 256;
carp_ioctl_set(if_t ifp, struct carpkreq *carpr)
         if (! IN6_IS_ADDR_UNSPECIFIED(&carpr->carpr_addr6)) {
                 memcpy(&sc->sc_carpaddr6, &carpr->carpr_addr6,
                     sizeof(sc->sc_carpaddr6));
         }
         if (carpr->carpr_key[0] != '\0') {
                 bcopy(carpr->carpr_key, sc->sc_key, sizeof(sc->sc_key));
                 carp_hmac_prepare(sc);
         }
-        if (sc->sc_state != INIT &&
+        if (sc->sc_state != INIT && sc->sc_state != SUPPRESS &&
             carpr->carpr_state != sc->sc_state) {
                 switch (carpr->carpr_state) {
                 case BACKUP:
                         callout_stop(&sc->sc_ad_tmo);
                         carp_set_state(sc, BACKUP,
                             "user requested via ifconfig");
                         carp_setrun(sc, 0);
                         carp_delroute(sc);
if (sc->sc_carpdev->if_link_state != LINK_STATE_UP ||
             !V_carp_allow) {
                 callout_stop(&sc->sc_ad_tmo);
 #ifdef INET
                 callout_stop(&sc->sc_md_tmo);
 #endif
 #ifdef INET6
                 callout_stop(&sc->sc_md6_tmo);
 #endif
-                carp_set_state(sc, INIT, "hardware interface down");
+                carp_set_state(sc, SUPPRESS, "hardware interface down");
                 carp_setrun(sc, 0);
                 if (!sc->sc_suppress)
                         carp_demote_adj(V_carp_ifdown_adj, "interface down");
                 sc->sc_suppress = 1;
         } else {
                 carp_set_state(sc, INIT, "hardware interface up");
                 carp_setrun(sc, 0);
                 if (sc->sc_suppress)