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);

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).


> +
> +#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) != 0) {
> +        virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("pcap_compile: %s"), pcap_geterr(handle));
> +        return 0;
> +    }
> +    if (pcap_setfilter(handle, &fp) != 0) {
> +        virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("pcap_setfilter: %s"), 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