On Mon, Jul 31, 2017 at 05:59:46PM -0400, Rob Pierce wrote:
> Good evening all,
>
> Currently, ifstated does not detect the removal of an IFT_CARP pseudo device.
> As such, you can delete a carp interface and have ifstated happily remain in
> the current state without detecting any interface change.
>
> The reasons are two fold:
>
> 1. The routing socket is only monitored for RTM_IFINFO messages and therefore
> misses interface departure and arrival events. On an interface departure,
> for example, there is only one RTM_IFINFO message, and that particular
> message identifies the departed (carp) interface as up and master.
>
> 2. As I have recently discovered, LINK_STATE_UNKNOWN is equivalent to
> LINK_STATE_UP, so even if a removal was detected, the state change would go
> unnoticed (i.e. there would be no state change). I ran into this while
> trying to address (1).
>
> The following diff resolves this problem by adding RTM_IFANNOUNCE to the
> routing socket filter, and the route message handler now handles interface
> departures and arrivals. Upon departure, a rescan is performed with a (forced)
> state of LINK_STATE_DOWN, and upon the arrival of a previously configured
> pseudo device the interface is re-indexed and a rescan is performed with the
> actual interface state.
>
> I have also updated the regression test to cover carp interface departure and
> arrival (diff included for testing convenience).
>
> At this stage I am only looking for general comments on the approach and/or
> some testing if you are an ifstated user.
>
> For example, maybe it is unnecessary to save and check the interface type,
> assuming that interface departure and arrival will always be on a (supported)
> pseudo device (e.g. IFT_CARP), and actual physical interfaces will not behave
> in this manner. Also, ifi_type is not available via an ioctl under pledge, so
> obtaining this information dynamically through routing messages may be a bit
> of a hack.
>
> Thanks and regards,
>
> Rob
I need to rethink this approach. A departing interface must be handled
appropriately, but the arrival of a new interface is problematic as we really
need to confirm that the interface is configured exactly as expected (i.e. as
it was prior to departure).
Pledge currently prevents us from obtaining some of that information upon
arrival for comparison purposes (e.g. SIOCGVH). This will also be an issue on
reload under the current design, as interface parameters are not inherently
addressed - only their state is considered.
Maybe a simpler diff that only handles interface departure would be better,
with the assumption that the interface will not come back, and indeed is not
permitted to return after departure. This would at least ensure that a
fail-over was handled appropriately within a pair of CARPed firewealls on
interface departure. The departing interface would need to be re-configured
and ifstated restarted in order for that host to return to a normal operating
state.
Rob
>
> Index: src/usr.sbin/ifstated/ifstated.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/ifstated/ifstated.c,v
> retrieving revision 1.56
> diff -u -p -r1.56 ifstated.c
> --- src/usr.sbin/ifstated/ifstated.c 24 Jul 2017 12:33:59 -0000 1.56
> +++ src/usr.sbin/ifstated/ifstated.c 31 Jul 2017 20:48:42 -0000
> @@ -28,6 +28,7 @@
> #include <sys/wait.h>
>
> #include <net/if.h>
> +#include <net/if_types.h>
> #include <net/route.h>
> #include <netinet/in.h>
>
> @@ -73,6 +74,8 @@ void do_action(struct ifsd_action *);
> void remove_action(struct ifsd_action *, struct ifsd_state *);
> void remove_expression(struct ifsd_expression *,
> struct ifsd_state *);
> +void set_iftype(int, u_char);
> +void reindex_if(int, char *);
>
> __dead void
> usage(void)
> @@ -149,7 +152,7 @@ main(int argc, char *argv[])
> if ((rt_fd = socket(PF_ROUTE, SOCK_RAW, 0)) < 0)
> err(1, "no routing socket");
>
> - rtfilter = ROUTE_FILTER(RTM_IFINFO);
> + rtfilter = ROUTE_FILTER(RTM_IFINFO) | ROUTE_FILTER(RTM_IFANNOUNCE);
> if (setsockopt(rt_fd, PF_ROUTE, ROUTE_MSGFILTER,
> &rtfilter, sizeof(rtfilter)) == -1) /* not fatal */
> log_warn("%s: setsockopt msgfilter", __func__);
> @@ -231,24 +234,79 @@ void
> rt_msg_handler(int fd, short event, void *arg)
> {
> char msg[2048];
> + char ifname[IFNAMSIZ];
> struct rt_msghdr *rtm = (struct rt_msghdr *)&msg;
> struct if_msghdr ifm;
> + struct if_announcemsghdr ifan;
> + struct ifaddrs *ifap, *ifa;
> ssize_t len;
>
> len = read(fd, msg, sizeof(msg));
>
> /* XXX ignore errors? */
> - if (len < sizeof(struct rt_msghdr))
> + if ((size_t)len < sizeof(rtm->rtm_msglen) || len < rtm->rtm_msglen)
> return;
>
> if (rtm->rtm_version != RTM_VERSION)
> return;
>
> - if (rtm->rtm_type != RTM_IFINFO)
> - return;
> -
> - memcpy(&ifm, rtm, sizeof(ifm));
> - scan_ifstate(ifm.ifm_index, ifm.ifm_data.ifi_link_state, 1);
> + switch (rtm->rtm_type) {
> + /*
> + * We can't access ifi_type via an ioctl under pledge, but we can get
> + * it for free in an RTM_IFINFO and update ifstate accordingly. This is
> + * (may be?) required for reindexing, and reindexing isn't necessary
> + * until we have received an RTM_IFINFO message.
> + */
> + case RTM_IFINFO:
> + memcpy(&ifm, rtm, sizeof(ifm));
> + set_iftype(ifm.ifm_index, ifm.ifm_data.ifi_type);
> + scan_ifstate(ifm.ifm_index, ifm.ifm_data.ifi_link_state, 1);
> + break;
> + case RTM_IFANNOUNCE:
> + memcpy(&ifan, rtm, sizeof(ifan));
> + switch (ifan.ifan_what) {
> + case IFAN_ARRIVAL:
> + /*
> + * We only reindex IFT_CARP pseudo devices that have
> + * already been configured, and then rescan with the
> + * actual interface state.
> + */
> + log_warnx("interface #%d arrived", ifan.ifan_index);
> + if_indextoname(ifan.ifan_index, ifname);
> + reindex_if(ifan.ifan_index, ifname);
> + if (getifaddrs(&ifap) != 0)
> + err(1, "getifaddrs");
> + for (ifa = ifap; ifa; ifa = ifa->ifa_next) {
> + if (ifa->ifa_addr->sa_family == AF_LINK) {
> + struct if_data *ifdata = ifa->ifa_data;
> + if (strcmp(ifa->ifa_name, ifname) == 0)
> + {
> + scan_ifstate(ifan.ifan_index,
> + ifdata->ifi_link_state, 1);
> + break;
> + }
> + }
> + }
> + freeifaddrs(ifap);
> + break;
> + case IFAN_DEPARTURE:
> + /*
> + * Pseudo devices configured in ifstated that have
> + * departed need to be explicitly scanned as
> + * LINK_STATE_DOWN since LINK_STATE_UNKNOWN is
> + * equivalent to LINK_STATE_UP (which results in no
> + * detected interface stata change).
> + */
> + log_warnx("interface #%d departed", ifan.ifan_index);
> + scan_ifstate(ifan.ifan_index, LINK_STATE_DOWN, 1);
> + break;
> + default:
> + break;
> + }
> + default:
> + break;
> + }
> + return;
> }
>
> void
> @@ -414,6 +472,42 @@ external_evtimer_setup(struct ifsd_state
> evtimer_del(&external->ev);
> }
> break;
> + }
> + }
> +}
> +
> +void
> +set_iftype(int ifindex, u_char iftype)
> +{
> + struct ifsd_state *state;
> + struct ifsd_ifstate *ifstate;
> +
> + TAILQ_FOREACH(state, &conf->states, entries) {
> + TAILQ_FOREACH(ifstate, &state->interface_states, entries) {
> + if (ifstate->ifindex == ifindex)
> + if (ifstate->iftype == 0)
> + ifstate->iftype = iftype;
> + }
> + }
> +}
> +
> +void
> +reindex_if(int ifindex, char *ifname)
> +{
> + struct ifsd_state *state;
> + struct ifsd_ifstate *ifstate;
> +
> + TAILQ_FOREACH(state, &conf->states, entries) {
> + TAILQ_FOREACH(ifstate, &state->interface_states, entries) {
> + if (strcmp(ifstate->ifname, ifname) == 0) {
> + if (ifindex > ifstate->ifindex) {
> + if (ifstate->iftype == IFT_CARP)
> + log_warnx("%s: reindexing %s",
> + state->name,
> + ifstate->ifname);
> + ifstate->ifindex = ifindex;
> + }
> + }
> }
> }
> }
> Index: src/usr.sbin/ifstated/ifstated.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/ifstated/ifstated.h,v
> retrieving revision 1.17
> diff -u -p -r1.17 ifstated.h
> --- src/usr.sbin/ifstated/ifstated.h 23 Jul 2017 13:48:18 -0000 1.17
> +++ src/usr.sbin/ifstated/ifstated.h 31 Jul 2017 20:48:42 -0000
> @@ -42,6 +42,8 @@ struct ifsd_ifstate {
> int prevstate;
> u_int32_t refcount;
> u_short ifindex;
> + char ifname[IFNAMSIZ];
> + u_char iftype;
> };
>
> struct ifsd_external {
> Index: src/usr.sbin/ifstated/parse.y
> ===================================================================
> RCS file: /cvs/src/usr.sbin/ifstated/parse.y,v
> retrieving revision 1.45
> diff -u -p -r1.45 parse.y
> --- src/usr.sbin/ifstated/parse.y 23 Jul 2017 13:53:54 -0000 1.45
> +++ src/usr.sbin/ifstated/parse.y 31 Jul 2017 20:48:43 -0000
> @@ -950,6 +950,7 @@ new_ifstate(u_short ifindex, int s)
> if ((ifstate = calloc(1, sizeof(*ifstate))) == NULL)
> err(1, NULL);
> ifstate->ifindex = ifindex;
> + if_indextoname(ifstate->ifindex, ifstate->ifname);
> ifstate->ifstate = s;
> TAILQ_INIT(&ifstate->expressions);
> TAILQ_INSERT_TAIL(&state->interface_states, ifstate, entries);
> Index: src/regress/usr.sbin/ifstated/ifstated
> ===================================================================
> RCS file: /cvs/src/regress/usr.sbin/ifstated/ifstated,v
> retrieving revision 1.3
> diff -u -p -r1.3 ifstated
> --- src/regress/usr.sbin/ifstated/ifstated 31 Jul 2017 18:41:21 -0000
> 1.3
> +++ src/regress/usr.sbin/ifstated/ifstated 31 Jul 2017 20:48:43 -0000
> @@ -123,12 +123,21 @@ changing state to demoted
> changing state to primary
> changing state to demoted
> changing state to primary
> +changing state to demoted
> +changing state to primary
> +changing state to primary
> +changing state to demoted
> changing state to primary
> EOF
>
> (cd working && nohup ifstated -dvf ./ifstated.conf > ifstated.log 2>&1) &
>
> sleep ${SLEEP}
> +ifconfig carp${VHIDA} destroy
> +sleep ${SLEEP}
> +ifconfig carp${VHIDA} inet ${PREFIX}.${VHIDA} netmask 255.255.255.0
> broadcast \
> + ${PREFIX}.255 vhid ${VHIDA} carpdev ${NIC}
> +sleep ${SLEEP}
> ifconfig carp${VHIDA} down
> sleep ${SLEEP}
> ifconfig carp${VHIDA} up
> @@ -142,12 +151,21 @@ ifconfig carp${VHIDA} down
> sleep ${SLEEP}
> ifconfig carp${VHIDB} destroy
> sleep ${SLEEP}
> +ifconfig carp${VHIDA} destroy
> +sleep ${SLEEP}
> +ifconfig carp${VHIDA} inet ${PREFIX}.${VHIDA} netmask 255.255.255.0
> broadcast \
> + ${PREFIX}.255 vhid ${VHIDA} carpdev ${NIC}
> ifconfig carp${VHIDA} up
> sleep ${SLEEP}
> ifconfig carp${VHIDB} inet ${PREFIX}.${VHIDB} netmask 255.255.255.0
> broadcast \
> ${PREFIX}.255 vhid ${VHIDB} carpdev ${NIC}
> sleep ${SLEEP}
> kill -HUP $(pgrep ifstated) >/dev/null 2>&1
> +sleep ${SLEEP}
> +ifconfig carp${VHIDA} destroy
> +sleep ${SLEEP}
> +ifconfig carp${VHIDA} inet ${PREFIX}.${VHIDA} netmask 255.255.255.0
> broadcast \
> + ${PREFIX}.255 vhid ${VHIDA} carpdev ${NIC}
> sleep ${SLEEP}
>
> grep ^changing working/ifstated.log > working/output.new
>
