6:26 a.m.
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(a)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