[src/trunk]: src/sys/net/npf NPF ifmap: rework and fix a few small bugs.

[rmind <rmind%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/d064be8f3aa7
branches:  trunk
changeset: 464244:d064be8f3aa7
user:      rmind <rmind%NetBSD.org@localhost>
date:      Sun Sep 29 17:00:29 2019 +0000

description:
NPF ifmap: rework and fix a few small bugs.

diffstat:

 sys/net/npf/npf_conn.c    |    5 +-
 sys/net/npf/npf_if.c      |  197 +++++++++++++++++++++++++++------------------
 sys/net/npf/npf_impl.h    |    6 +-
 sys/net/npf/npf_ruleset.c |    5 +-
 4 files changed, 128 insertions(+), 85 deletions(-)

diffs (truncated from 365 to 300 lines):

diff -r fdd43a4f5dd1 -r d064be8f3aa7 sys/net/npf/npf_conn.c
--- a/sys/net/npf/npf_conn.c    Sun Sep 29 16:58:35 2019 +0000
+++ b/sys/net/npf/npf_conn.c    Sun Sep 29 17:00:29 2019 +0000
@@ -107,7 +107,7 @@
 
 #ifdef _KERNEL
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: npf_conn.c,v 1.29 2019/08/06 11:40:15 christos Exp $");
+__KERNEL_RCSID(0, "$NetBSD: npf_conn.c,v 1.30 2019/09/29 17:00:29 rmind Exp $");
 
 #include <sys/param.h>
 #include <sys/types.h>
@@ -782,7 +782,8 @@
        nvlist_add_number(cdict, "flags", con->c_flags);
        nvlist_add_number(cdict, "proto", con->c_proto);
        if (con->c_ifid) {
-               const char *ifname = npf_ifmap_getname(npf, con->c_ifid);
+               char ifname[IFNAMSIZ];
+               npf_ifmap_copyname(npf, con->c_ifid, ifname, sizeof(ifname));
                nvlist_add_string(cdict, "ifname", ifname);
        }
        nvlist_add_binary(cdict, "state", &con->c_state, sizeof(npf_state_t));
diff -r fdd43a4f5dd1 -r d064be8f3aa7 sys/net/npf/npf_if.c
--- a/sys/net/npf/npf_if.c      Sun Sep 29 16:58:35 2019 +0000
+++ b/sys/net/npf/npf_if.c      Sun Sep 29 17:00:29 2019 +0000
@@ -1,4 +1,5 @@
 /*-
+ * Copyright (c) 2019 Mindaugas Rasiukevicius <rmind at noxt eu>
  * Copyright (c) 2013 The NetBSD Foundation, Inc.
  * All rights reserved.
  *
@@ -28,23 +29,34 @@
  */
 
 /*
- * NPF network interface handling module.
+ * NPF network interface handling.
+ *
+ * NPF uses its own interface IDs (npf-if-id).  These IDs start from 1.
+ * Zero is reserved to indicate "no interface" case or an interface of
+ * no interest (i.e. not registered).
+ *
+ * This module provides an interface to primarily handle the following:
+ *
+ * - Bind a symbolic interface name to NPF interface ID.
+ * - Associate NPF interface ID when the network interface is attached.
  *
- * NPF uses its own interface IDs (npf-if-id).  When NPF configuration is
- * (re)loaded, each required interface name is registered and a matching
- * network interface gets an ID assigned.  If an interface is not present,
- * it gets an ID on attach.
+ * When NPF configuration is (re)loaded, each referenced network interface
+ * name is registered with a unique ID.  If the network interface is already
+ * attached, then the ID is associated with it immediately; otherwise, IDs
+ * are associated/disassociated on interface events which are monitored
+ * using pfil(9) hooks.
  *
- * IDs start from 1.  Zero is reserved to indicate "no interface" case or
- * an interface of no interest (i.e. not registered).
+ * To avoid race conditions when an active NPF configuration is updated or
+ * interfaces are detached/attached, the interface names are never removed
+ * and therefore IDs are never re-assigned.  The only point when interface
+ * names and IDs are cleared is when the configuration is flushed.
  *
- * The IDs are mapped synchronously based on interface events which are
- * monitored using pfil(9) hooks.
+ * A linear counter is used for IDs.
  */
 
 #ifdef _KERNEL
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: npf_if.c,v 1.10 2019/08/11 20:26:33 rmind Exp $");
+__KERNEL_RCSID(0, "$NetBSD: npf_if.c,v 1.11 2019/09/29 17:00:29 rmind Exp $");
 
 #include <sys/param.h>
 #include <sys/types.h>
@@ -55,9 +67,13 @@
 #include "npf_impl.h"
 
 typedef struct npf_ifmap {
-       char            n_ifname[IFNAMSIZ];
+       char            ifname[IFNAMSIZ + 1];
 } npf_ifmap_t;
 
+#define        NPF_IFMAP_NOID                  (0U)
+#define        NPF_IFMAP_SLOT2ID(npf, slot)    ((npf)->ifmap_off + (slot) + 1)
+#define        NPF_IFMAP_ID2SLOT(npf, id)      ((id) - (npf)->ifmap_off - 1)
+
 void
 npf_ifmap_init(npf_t *npf, const npf_ifops_t *ifops)
 {
@@ -66,8 +82,10 @@
        KASSERT(ifops != NULL);
        ifops->flush((void *)(uintptr_t)0);
 
+       mutex_init(&npf->ifmap_lock, MUTEX_DEFAULT, IPL_SOFTNET);
        npf->ifmap = kmem_zalloc(nbytes, KM_SLEEP);
        npf->ifmap_cnt = 0;
+       npf->ifmap_off = 0;
        npf->ifops = ifops;
 }
 
@@ -75,82 +93,101 @@
 npf_ifmap_fini(npf_t *npf)
 {
        const size_t nbytes = sizeof(npf_ifmap_t) * NPF_MAX_IFMAP;
+       mutex_destroy(&npf->ifmap_lock);
        kmem_free(npf->ifmap, nbytes);
 }
 
-static u_int
-npf_ifmap_new(npf_t *npf)
-{
-       KASSERT(npf_config_locked_p(npf));
-
-       for (u_int i = 0; i < npf->ifmap_cnt; i++)
-               if (npf->ifmap[i].n_ifname[0] == '\0')
-                       return i + 1;
-
-       if (npf->ifmap_cnt == NPF_MAX_IFMAP) {
-               printf("npf_ifmap_new: out of slots; bump NPF_MAX_IFMAP\n");
-               return 0;
-       }
-       return ++npf->ifmap_cnt;
-}
-
-static u_int
+static unsigned
 npf_ifmap_lookup(npf_t *npf, const char *ifname)
 {
-       KASSERT(npf_config_locked_p(npf));
+       KASSERT(mutex_owned(&npf->ifmap_lock));
 
-       for (u_int i = 0; i < npf->ifmap_cnt; i++) {
-               npf_ifmap_t *nim = &npf->ifmap[i];
+       for (unsigned i = 0; i < npf->ifmap_cnt; i++) {
+               npf_ifmap_t *ifmap = &npf->ifmap[i];
 
-               if (nim->n_ifname[0] && strcmp(nim->n_ifname, ifname) == 0)
-                       return i + 1;
+               if (strcmp(ifmap->ifname, ifname) == 0) {
+                       return NPF_IFMAP_SLOT2ID(npf, i);
+               }
        }
-       return 0;
+       return NPF_IFMAP_NOID;
 }
 
-u_int
+/*
+ * npf_ifmap_register: register an interface name; return an assigned
+ * NPF network ID on success (non-zero).
+ *
+ * This routine is mostly called on NPF configuration (re)load for the
+ * interfaces names referenced by the rules.
+ */
+unsigned
 npf_ifmap_register(npf_t *npf, const char *ifname)
 {
-       npf_ifmap_t *nim;
+       npf_ifmap_t *ifmap;
+       unsigned id, i;
        ifnet_t *ifp;
-       u_int i;
 
-       npf_config_enter(npf);
-       if ((i = npf_ifmap_lookup(npf, ifname)) != 0) {
+       mutex_enter(&npf->ifmap_lock);
+       if ((id = npf_ifmap_lookup(npf, ifname)) != NPF_IFMAP_NOID) {
+               goto out;
+       }
+       if (npf->ifmap_cnt == NPF_MAX_IFMAP) {
+               printf("npf_ifmap_new: out of slots; bump NPF_MAX_IFMAP\n");
+               id = NPF_IFMAP_NOID;
                goto out;
        }
-       if ((i = npf_ifmap_new(npf)) == 0) {
-               goto out;
-       }
-       nim = &npf->ifmap[i - 1];
-       strlcpy(nim->n_ifname, ifname, IFNAMSIZ);
+       KASSERT(npf->ifmap_cnt < NPF_MAX_IFMAP);
+
+       /* Allocate a new slot and convert and assign an ID. */
+       i = npf->ifmap_cnt++;
+       ifmap = &npf->ifmap[i];
+       strlcpy(ifmap->ifname, ifname, IFNAMSIZ);
+       id = NPF_IFMAP_SLOT2ID(npf, i);
 
        if ((ifp = npf->ifops->lookup(ifname)) != NULL) {
-               npf->ifops->setmeta(ifp, (void *)(uintptr_t)i);
+               npf->ifops->setmeta(ifp, (void *)(uintptr_t)id);
        }
 out:
-       npf_config_exit(npf);
-       return i;
+       mutex_exit(&npf->ifmap_lock);
+       return id;
 }
 
 void
 npf_ifmap_flush(npf_t *npf)
 {
-       KASSERT(npf_config_locked_p(npf));
-
-       for (u_int i = 0; i < npf->ifmap_cnt; i++) {
-               npf->ifmap[i].n_ifname[0] = '\0';
+       mutex_enter(&npf->ifmap_lock);
+       npf->ifops->flush((void *)(uintptr_t)NPF_IFMAP_NOID);
+       for (unsigned i = 0; i < npf->ifmap_cnt; i++) {
+               npf->ifmap[i].ifname[0] = '\0';
        }
        npf->ifmap_cnt = 0;
-       npf->ifops->flush((void *)(uintptr_t)0);
+
+       /*
+        * Reset the ID counter if reaching the overflow; this is not
+        * realistic, but we maintain correctness.
+        */
+       if (npf->ifmap_off < (UINT_MAX - NPF_MAX_IFMAP)) {
+               npf->ifmap_off += NPF_MAX_IFMAP;
+       } else {
+               npf->ifmap_off = 0;
+       }
+       mutex_exit(&npf->ifmap_lock);
 }
 
-u_int
+/*
+ * npf_ifmap_getid: get the ID for the given network interface.
+ *
+ * => This routine is typically called from the packet handler when
+ *    matching whether the packet is on particular network interface.
+ *
+ * => This routine is lock-free; if the NPF configuration is flushed
+ *    while the packet is in-flight, the ID will not match because we
+ *    keep the IDs linear.
+ */
+unsigned
 npf_ifmap_getid(npf_t *npf, const ifnet_t *ifp)
 {
-       const u_int i = (uintptr_t)npf->ifops->getmeta(ifp);
-       KASSERT(i <= npf->ifmap_cnt);
-       return i;
+       const unsigned id = (uintptr_t)npf->ifops->getmeta(ifp);
+       return id;
 }
 
 /*
@@ -158,45 +195,47 @@
  * lock, but it is only used temporarily and only for logging.
  */
 void
-npf_ifmap_copyname(npf_t *npf, u_int id, char *buf, size_t len)
+npf_ifmap_copylogname(npf_t *npf, unsigned id, char *buf, size_t len)
 {
-       if (id > 0 && id < npf->ifmap_cnt)
-               strlcpy(buf, npf->ifmap[id - 1].n_ifname,
-                   MIN(len, sizeof(npf->ifmap[id - 1].n_ifname)));
-       else
+       if (id != NPF_IFMAP_NOID) {
+               const unsigned i = NPF_IFMAP_ID2SLOT(npf, id);
+               npf_ifmap_t *ifmap = &npf->ifmap[i];
+
+               /*
+                * Lock-free access is safe as there is an extra byte
+                * with a permanent NUL terminator at the end.
+                */
+               strlcpy(buf, ifmap->ifname, MIN(len, IFNAMSIZ));
+       } else {
                strlcpy(buf, "???", len);
+       }
 }
 
-const char *
-npf_ifmap_getname(npf_t *npf, const u_int id)
+void
+npf_ifmap_copyname(npf_t *npf, unsigned id, char *buf, size_t len)
 {
-       const char *ifname;
-
-       KASSERT(npf_config_locked_p(npf));
-       KASSERT(id > 0 && id <= npf->ifmap_cnt);
-
-       ifname = npf->ifmap[id - 1].n_ifname;
-       KASSERT(ifname[0] != '\0');
-       return ifname;
+       mutex_enter(&npf->ifmap_lock);
+       npf_ifmap_copylogname(npf, id, buf, len);
+       mutex_exit(&npf->ifmap_lock);
 }
 
 __dso_public void
 npfk_ifmap_attach(npf_t *npf, ifnet_t *ifp)
 {
        const npf_ifops_t *ifops = npf->ifops;
-       u_int i;
+       unsigned id;
 
-       npf_config_enter(npf);