Re: [libvirt] [libvirt PATCHv7 1/1] add DHCP snooping

27 Mar 2012

      David Stevens/Beaverton/IBM@IBMUS wrote on 03/26/2012 04:25:48 PM:
...
This patch adds DHCP snooping support to libvirt. The learning method
for
...
IP addresses is specified by setting the "ip_learning" variable to one 
of
"any" [default] (existing IP learning code), "none" (static only 
addresses)
or "dhcp" (DHCP snooping).
Active leases are saved in a lease file and reloaded on restart or HUP.
Changes since v6:
- replace pthread_cancel() with synchronous cancelation method
Signed-off-by: David L Stevens <dlstevens@us.ibm.com>
---
[...]
+
+static char *
+SnoopActivate(pthread_t thread)
+{
+    int len;
+    char *key;
+
+    len = sizeof(thread)*2 + 3; /* "0x"+'\0' */
+    if (VIR_ALLOC_N(key, len)) {
+        virReportOOMError();
+        return NULL;
+    }
+    (void) snprintf(key, len, "0x%0*lX", sizeof(thread)*2,
+                    (unsigned long int)thread);
...
+    key[len-1] = '\0';
Why does this string have to be closed here? You should use virAsprintf() 
rather than the malloc+snprintf.
...
+
+    active_lock();
+    if (virHashAddEntry(Active, key, strdup("1")))
+        VIR_FREE(key);
Check strdup() for NULL.
...
+}
+ 
+/*
+ * ipl_install - install rule for a lease
+ */
+static int
+ipl_install(struct iplease *ipl)
+{
+    char                     ipbuf[20];    /* dotted decimal IP 
addr string */
Use INET_ADDRSTRLEN, which is 16 and corresponds to exactly the number of 
bytes needed. Also you use 16 further below.
...
+    int                      rc;
+    virNWFilterVarValuePtr   ipVar;
+
+    if (!inet_ntop(AF_INET, &ipl->ipl_ipaddr, ipbuf, sizeof(ipbuf))) {
+        virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("ipl_install inet_ntop " "failed 
(0x%08X)"),
Why are there two separate strings?
...
+
+/*
+ * ipl_trun - run the IP lease timeout list
+ */
+static unsigned int
+ipl_trun(struct virNWFilterSnoopReq *req)
+{
+    uint32_t now;
time_t ?
...
+
+unsigned char dhcp_magic[4] = { 99, 130, 83, 99 };
static const unsigned char dhcp_magic[4] ...
...
+    pip = (struct iphdr *) pep->eh_data;
+    len -= sizeof(*pep);
...
+
+#define PBUFSIZE        576     /* >= IP/TCP/DHCP headers */
+#define TIMEOUT          30 /* secs */
+
+static pcap_t *
+dhcpopen(const char *intf)
+{
+    pcap_t             *handle = NULL;
+    struct bpf_program  fp;
+    char                filter[64];
+    char                pcap_errbuf[PCAP_ERRBUF_SIZE];
+    time_t              start;
+
+    start = time(0);
+    while (handle == NULL && time(0) - start < TIMEOUT)
+        handle = pcap_open_live(intf, PBUFSIZE, 0, POLL_INTERVAL, 
pcap_errbuf);
+
+    if (handle == NULL) {
+        virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("pcap_open_live: %s"), pcap_errbuf);
+        return 0;
+    }
+
+    sprintf(filter, "port 67 or dst port 68");
+    if (pcap_compile(handle, &fp, filter, 1, PCAP_NETMASK_UNKNOWN) !=
...
+        virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("pcap_compile: %s"),
...
+        return 0;
+    }
+    if (pcap_setfilter(handle, &fp) != 0) {
+        virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("pcap_setfilter: %s"),
before you used sizeof xyz and here sizeof(xyz). Can you converge to one 
style? Preferably the latter.

Check len for < 0 to not step into memory that may not have been allocated 
(for example pip->ihl).

0) {
pcap_geterr(handle));
pcap_geterr(handle));

pcap_freecode()...
...
+
+static void *
+virNWFilterDHCPSnoop(void *req0)
+{
+    struct virNWFilterSnoopReq *req = req0;
+    pcap_t                     *handle;
+    struct pcap_pkthdr         *hdr;
+    struct eth                 *packet;
+    int                         ifindex;
+    int                         errcount;
+    char                       *threadkey;
+
+    handle = dhcpopen(req->ifname);
+    if (!handle)
+        return 0;
+
+    req->threadkey = SnoopActivate(pthread_self());
+    threadkey = strdup(req->threadkey);
Check for NULL.
...
+
+int
+virNWFilterDHCPSnoopReq(virNWFilterTechDriverPtr techdriver,
+                        const char *ifname,
+                        const char *linkdev,
+                        enum virDomainNetType nettype,
+                        const unsigned char *vmuuid,
+                        const unsigned char *macaddr,
+                        const char *filtername,
+                        virNWFilterHashTablePtr filterparams,
+                        virNWFilterDriverStatePtr driver)
+{
+    struct virNWFilterSnoopReq *req;
+    bool                        isnewreq;
+    char                        ifkey[VIR_IFKEY_LEN];
+    pthread_t                   thread;
+
+    ifkeyFormat(ifkey, vmuuid, macaddr);
+    snoop_lock();
+    req = virHashLookup(SnoopReqs, ifkey);
+    isnewreq = req == NULL;
+    if (!isnewreq) {
+        if (req->threadkey) {
+            snoop_unlock();
+            return 0;
+        }
+    } else {
+        req = newreq(ifkey);
+        if (!req) {
+            snoop_unlock();
+            return 1;
All functions now return -1 on error.
...
+        }
+    }
+
+    req->techdriver = techdriver;
+    req->ifindex = if_nametoindex(ifname);
+    req->linkdev = linkdev ? strdup(linkdev) : NULL;
+    req->nettype = nettype;
+    req->ifname = strdup(ifname);
+    memcpy(req->macaddr, macaddr, sizeof(req->macaddr));
+    req->filtername = strdup(filtername);
+    if (req->filtername == NULL) {
Also check req->ifname for NULL.
...
+        snoop_unlock();
+        snoopReqFree(req);
+        virReportOOMError();
+        return 1;
+    }
return -1;
...
+
+static void
+lease_close(void)
+{
+    VIR_FORCE_CLOSE(lease_fd);
+}
+
+static void
+lease_open(void)
+{
+    lease_close();
+
+    lease_fd = open(LEASEFILE, O_CREAT|O_RDWR|O_APPEND, 0644);
Does this have to be a global variable?
...
+}
+
+int
+virNWFilterDHCPSnoopInit(void)
+{
+    if (SnoopReqs)
+        return 0;
+
+    snoop_lock();
+    IfnameToKey = virHashCreate(0, NULL);
+    SnoopReqs = virHashCreate(0, freeReq);
+    if (!SnoopReqs) {
+        snoop_unlock();
free IfnameToKey? Have a common exit for error cases .
...
+        virReportOOMError();
+        return -1;
+    }
+    lease_load();
+    lease_open();
+
+    snoop_unlock();
+
+    active_lock();
+    Active = virHashCreate(0, 0);
+    if (!Active) {
+        active_unlock();
+        virReportOOMError();
free hash tables above?
...
+        return -1;
+    }
+    active_unlock();
+    return 0;
+}
+
+static int
+lease_write(int lfd, const char *ifkey, struct iplease *ipl)
+{
+    char lbuf[256],ipstr[16],dhcpstr[16];
+
See above regarding the '16'. Space after comma.
...
+
+static struct virNWFilterSnoopReq *
+newreq(const char *ifkey)
+{
+    struct virNWFilterSnoopReq *req;
+
+    if (VIR_ALLOC(req) < 0) {
+        virReportOOMError();
+        return NULL;
+    }
+    strncpy(req->ifkey, ifkey, sizeof req->ifkey);
?? req->ifkey is NULL.

Also see Hacking for usage of strncpy. Do not use it.
...
+
+    return req;
+}
+
+static void
+SaveSnoopReqIter(void *payload,
+                 const void *name ATTRIBUTE_UNUSED,
+                 void *data)
+{
+    struct virNWFilterSnoopReq *req = payload;
+    int tfd = (int)data;
This gives the case of different size error (on 64bit) as I had said in 
previous patch reviews already.

Use *(int *)data.
...
+    struct iplease *ipl;
+
+    /* clean up orphaned, expired leases */
+    if (!req->threadkey) {
+        uint32_t now;
time_t
...
+
+        now = time(0);
+        for (ipl = req->start; ipl; ipl = ipl->ipl_next)
+            if (ipl->ipl_timeout < now)
+                ipl_del(req, ipl->ipl_ipaddr , 0);
+        if (!req->start) {
+            snoopReqFree(req);
+            return;
+        }
+    }
+    for (ipl = req->start; ipl; ipl = ipl->ipl_next)
+        (void) lease_write(tfd, req->ifkey, ipl);
+}
+
+static void
+lease_refresh(void)
+{
+    int tfd;
+
+    (void) unlink(TMPLEASEFILE);
+    /* lease file loaded, delete old one */
+    tfd = open(TMPLEASEFILE, O_CREAT|O_RDWR|O_TRUNC|O_EXCL, 0644);
+    if (tfd < 0) {
+        virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("open(\"%s\"): %s"),
+                               TMPLEASEFILE, strerror(errno));
+        return;
+    }
+    if (SnoopReqs)
+        virHashForEach(SnoopReqs, SaveSnoopReqIter, (void *)tfd);
+    (void) close(tfd);
Use VIR_FORCE_CLOSE as already shown in previous reviews.
...
+    if (rename(TMPLEASEFILE, LEASEFILE) < 0) {
+        virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("rename(\"%s\", \"%s\"): %s"),
+                               TMPLEASEFILE, LEASEFILE, 
strerror(errno));
+        (void) unlink(TMPLEASEFILE);
+    }
+    wleases = 0;
+    lease_open();
+}
+
+
+static void
+lease_load(void)
+{
+    char line[256], ifkey[VIR_IFKEY_LEN], ipstr[16], srvstr[16];
Same comment as above.
...
+    struct iplease ipl;
+    struct virNWFilterSnoopReq *req;
+    time_t now;
+    FILE *fp;
+    int ln = 0;
+
+    fp = fopen(LEASEFILE, "r");
+    time(&now);
+    while (fp && fgets(line, sizeof(line), fp)) {
+        if (line[strlen(line)-1] != '\n') {
+            virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
+                                   _("lease_load lease file line 
%dcorrupt"),
+                                   ln);
+            break;
+        }
+        ln++;
+        /* key len 55 = "VMUUID"+'-'+"MAC" */
+        if (sscanf(line, "%u %55s %16s %16s", &ipl.ipl_timeout,
+                   ifkey, ipstr, srvstr) < 4) {
+            virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
+                                   _("lease_load lease file line 
%dcorrupt"),
+                                   ln);
+            break;;
Remove ';'
...
+        }
+        if (ipl.ipl_timeout && ipl.ipl_timeout < now)
+            continue;
+        req = virHashLookup(SnoopReqs, ifkey);
+        if (!req) {
+            req = newreq(ifkey);
+            if (!req)
+               break;
+            if (virHashAddEntry(SnoopReqs, ifkey, req)) {
+                snoopReqFree(req);
+                virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
+                                       _("lease_load req add failed on 
"
+                                       "interface \"%s\""), ifkey);
+                continue;
+            }
+        }
+
+        if (inet_pton(AF_INET, ipstr, &ipl.ipl_ipaddr) <= 0) {
+            virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
+                                  _("line %d corrupt ipaddr \"%s\""),
+                                  ln, ipstr);
+            continue;
+        }
+        (void) inet_pton(AF_INET, srvstr, &ipl.ipl_server);
+        ipl.ipl_req = req;
+
+        if (ipl.ipl_timeout)
+            ipl_add(&ipl, 0);
+        else
+            ipl_del(req, ipl.ipl_ipaddr, 0);
+    }
+    if (fp != NULL)
+        (void) fclose(fp);
+    lease_refresh();
+}
+
Stefan

Re: [libvirt] [libvirt PATCHv7 1/1] add DHCP snooping

Stefan Berger