[libvirt] [libvirt PATCHv7 1/1] add DHCP snooping
Stefan Berger
stefanb at us.ibm.com
Tue Mar 27 11:26:47 UTC 2012
David Stevens/Beaverton/IBM at 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 at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120327/d786e002/attachment-0001.htm>
More information about the libvir-list
mailing list