On 05/25/2012 05:56 AM, Stefan Berger wrote: > This patch adds DHCP snooping support to libvirt. The learning method for > IP addresses is specified by setting the "CTRL_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. > > The following interface XML activates and uses the DHCP snooping: > > <interface type='bridge'> > <source bridge='virbr0'/> > <filterref filter='clean-traffic'> > <parameter name='CTRL_IP_LEARNING' value='dhcp'/> > </filterref> > </interface> > > All filters containing the variable 'IP' are automatically adjusted when > the VM receives an IP address via DHCP. However, multiple IP addresses per > interface are silently ignored in this patch, thus only supporting one IP > address per interface. Multiple IP address support is added in a later > patch in this series. > > Signed-off-by: David L Stevens <dlstevens@xxxxxxxxxx> > Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxxxxxxxxxx> > > --- > > Changes since v12: > - use pcap_create to open pcap to be able to set smaller buffer > than the 2 MB default buffer used by pcap which leads to processing > of huge backlog of messages if flooding occurrs > - use virAtomicInc/Dec > - use indep. rate controllers, one per direction to tune out of > flooding in each direction individually (even if flooding was to > occur, libvirt doesn't use much CPU [0.3%]) > My last review was here: https://www.redhat.com/archives/libvir-list/2012-April/msg01542.html > > --- > docs/formatnwfilter.html.in | 143 +- > po/POTFILES.in | 1 > src/Makefile.am | 2 > src/conf/nwfilter_params.h | 5 > src/nwfilter/nwfilter_dhcpsnoop.c | 2166 +++++++++++++++++++++++++++++++++ > src/nwfilter/nwfilter_dhcpsnoop.h | 39 > src/nwfilter/nwfilter_driver.c | 6 > src/nwfilter/nwfilter_gentech_driver.c | 63 > 8 files changed, 2380 insertions(+), 45 deletions(-) > create mode 100644 src/nwfilter/nwfilter_dhcpsnoop.c > create mode 100644 src/nwfilter/nwfilter_dhcpsnoop.h > > Index: libvirt-acl/docs/formatnwfilter.html.in > =================================================================== > --- libvirt-acl.orig/docs/formatnwfilter.html.in > +++ libvirt-acl/docs/formatnwfilter.html.in > @@ -371,6 +371,118 @@ > Further, the notation of $VARIABLE is short-hand for $VARIABLE[@0]. The > former notation always assumes the iterator with Id '0'. > <p> > + > + <h3><a name="nwfelemsRulesAdvIPAddrDetection">Automatic IP address detection</a></h3> > + <p> > + The detection of IP addresses used on a virtual machine's interface > + is automatically activated if the variable <code>IP</code> is referenced > + but not value has been assigned to it. s/not/no/ > + <span class="since">Since 0.9.12</span> s/12/13/ (hmm, sorry for the delays) > +# define virNWFilterSnoopLock() \ > + do { \ > + virMutexLock(&virNWFilterSnoopState.snoopLock); \ > + } while (0); Ouch. Lose the trailing ';', or you will cause problems with: if (foo) virNWFilterSnoopLock(); else bar; > +# define virNWFilterSnoopUnlock() \ > + do { \ > + virMutexUnlock(&virNWFilterSnoopState.snoopLock); \ > + } while (0); > +# define virNWFilterSnoopActiveLock() \ > + do { \ > + virMutexLock(&virNWFilterSnoopState.activeLock); \ > + } while (0); > +# define virNWFilterSnoopActiveUnlock() \ > + do { \ > + virMutexUnlock(&virNWFilterSnoopState.activeLock); \ > + } while (0); Three more times. > +static virNWFilterSnoopReqPtr > +virNWFilterSnoopReqNew(const char *ifkey) > +{ > + virNWFilterSnoopReqPtr req; > + > + if (ifkey == NULL || strlen(ifkey) != VIR_IFKEY_LEN - 1) { > + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, > + _("virNWFilterSnoopReqNew called with invalid " > + "key \"%s\" (%u)"), > + ifkey ? ifkey : "", > + (unsigned int)strlen(ifkey)); In libvirt, it's safe and preferrable to use %zu and drop the cast when dealing with size_t. (Gnulib is awesome.) > +/* > + * Decode the DHCP options > + * > + * Returns 0 in case of full success. > + * Returns -2 in case of some error with the packet. > + * Returns -1 in case of error with the installation of rules > + */ > +static int > +virNWFilterSnoopDHCPDecode(virNWFilterSnoopReqPtr req, > + virNWFilterSnoopEthHdrPtr pep, > + int len, bool fromVM) > +{ > + > + pup = (struct udphdr *) ((char *) pip + (pip->ihl << 2)); > + len -= pip->ihl << 2; > + if (len < 0) > + return -2; > + > + pd = (virNWFilterSnoopDHCPHdrPtr) ((char *) pup + sizeof(*pup)); Two instances of type punning that you didn't convert. I hope these are packed types, so that the compiler ensures we don't get any unaligned accesses on platforms where it matters. > + len -= sizeof(*pup); > + if (len < 0) > + return -2; /* invalid packet length */ > + > + if (virNWFilterSnoopDHCPGetOpt(pd, len, &mtype, &leasetime) < 0) > + return -2; > + > + memset(&ipl, 0, sizeof(ipl)); > + > + memcpy(&nwint, &pd->d_yiaddr, sizeof(nwint)); > + virSocketAddrSetIPv4Addr(&ipl.ipAddress, ntohl(nwint)); > + > + memcpy(&nwint, &pd->d_siaddr, sizeof(nwint)); > + virSocketAddrSetIPv4Addr(&ipl.ipServer, ntohl(nwint)); But I do see that you fixed some of the other type punning instances that were in v13. > +/* > + * virNWFilterSnoopRateLimit -- limit the rate of jobs submitted to the > + * worker thread > + * > + * Help defend the worker thread from being flooded with likely bogus packets > + * sent by the VM. > + * > + * rl: The state of the rate limiter > + * > + * Returns the delta of packets compared to the rate, i.e. if the rate > + * is 4 (pkts/s) and we now have received 5 within a second, it would > + * return 1. If the number of packets is below the rate, it returns 0. > + */ > +static unsigned int > +virNWFilterSnoopRateLimit(virNWFilterSnoopRateLimitConfPtr rl) > +{ > + time_t now = time(0); > + int diff; > +# define IN_BURST(n,b) ((n)-(b) <= 1) /* bursts span 2 discreet seconds */ s/discreet/discrete/ (both valid words, but quite a different connotation :) > +static int > +virNWFilterSnoopAdjustPoll(virNWFilterSnoopPcapConfPtr pc, > + size_t nPc, struct pollfd *pfd, > + int *pollTo) > +{ > + int ret = 0; > + size_t i; > + int tmp; > + unsigned long long now = 0; > + > + *pollTo = -1; > + > + for (i = 0; i < nPc; i++) { > + if (pc[i].penaltyTimeoutAbs != 0) { > + if (now == 0) { > + if (virTimeMillisNow(&now) < 0) { > + ret = -1; > + break; > + } > + } > + > + if (now < pc[i].penaltyTimeoutAbs) { > + /* don't listen to incoming data on the fd for some time */ double spaces > +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) > +{ > + req->driver = driver; > + req->techdriver = techdriver; > + tmp = virNetDevGetIndex(ifname, &req->ifindex); > + req->linkdev = linkdev ? strdup(linkdev) : NULL; > + req->nettype = nettype; > + req->ifname = strdup(ifname); > + memcpy(req->macaddr, macaddr, sizeof(req->macaddr)); > + req->filtername = strdup(filtername); > + req->vars = virNWFilterHashTableCreate(0); > + > + if (!req->ifname || !req->filtername || !req->vars || tmp < 0) { > + virReportOOMError(); You don't detect strdup(linkdev) failure; is that intentional? > + goto exit_snoopreqput; > + } > + > + /* check that all tools are available for applying the filters (late) */ > + if ( !techdriver->canApplyBasicRules()) { > + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("IP parameter must be provided since " > + "snooping the IP address does not work " > + "possibly due to missing tools")); Is this error user-visible? If so, VIR_ERR_INTERNAL_ERROR is probably the wrong category; maybe VIR_ERR_CONFIG_UNSUPPORTED fits better (the user is using $IP without configuring it properly). > + virAtomicIntInc(&virNWFilterSnoopState.nThreads); > + > + req->threadkey = virNWFilterSnoopActivate(req); > + if (!req->threadkey) { > + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, > + _("Actication of snoop request failed on " s/Actication/Activation/ > +/* > + * Write a single lease to the given file. > + * > + */ > +static int > +virNWFilterSnoopLeaseFileWrite(int lfd, const char *ifkey, > + virNWFilterSnoopIPLeasePtr ipl) > +{ This patch is pretty large. Reviewing might have been easier if it were in pieces; I've seen at least three major pieces: lease file management, pcap interaction, throttling. While I won't insist on a split, it's food for thought if you think it is worth a v15; a series of smaller patches is less daunting than a single 2000 line email. > + char lbuf[256]; > + char *ipstr, *dhcpstr; > + size_t len; > + int ret = 0; > + > + ipstr = virSocketAddrFormat(&ipl->ipAddress); > + dhcpstr = virSocketAddrFormat(&ipl->ipServer); > + > + if (!dhcpstr || !ipstr) { > + ret = -1; > + goto cleanup; > + } > + > + /* time intf ip dhcpserver */ > + snprintf(lbuf, sizeof(lbuf), "%u %s %s %s\n", ipl->timeout, > + ifkey, ipstr, dhcpstr); You're lucky that this doesn't overflow. A virAsprintf avoids the need to audit whether 256 bytes will always be enough. > + > + len = strlen(lbuf); The return value of snprintf/virAsprintf is the length; its faster to grab the length on the first pass through the string than to go through the string twice. > + > + if (safewrite(lfd, lbuf, len) != len) { > + virReportSystemError(errno, "%s", _("lease file write failed")); > + ret = -1; > + goto cleanup; > + } > + > + (void) fsync(lfd); Is there any problem if you delete the cast to void? > +static void > +virNWFilterSnoopSaveIter(void *payload, > + const void *name ATTRIBUTE_UNUSED, > + void *data) > +{ > + virNWFilterSnoopReqPtr req = payload; > + int tfd = *(int *)data; double spaces > +/* > + * Write all valid leases into a temporary file and then > + * rename the file to the final file. > + * Call this function with the SnoopLock held. > + */ > +static void > +virNWFilterSnoopLeaseFileRefresh(void) > +{ > + int tfd; > + > + (void) unlink(TMPLEASEFILE); You should report unlink() failure rather than ignore it, unless errno is ENOENT. > + /* lease file loaded, delete old one */ > + tfd = open(TMPLEASEFILE, O_CREAT|O_RDWR|O_TRUNC|O_EXCL, 0644); > + if (tfd < 0) { > + virReportSystemError(errno, _("open(\"%s\")"), TMPLEASEFILE); > + return; > + } > + > + if (virNWFilterSnoopState.snoopReqs) { > + /* clean up the requests */ > + virHashRemoveSet(virNWFilterSnoopState.snoopReqs, > + virNWFilterSnoopPruneIter, NULL); > + /* now save them */ > + virHashForEach(virNWFilterSnoopState.snoopReqs, > + virNWFilterSnoopSaveIter, (void *)&tfd); > + } > + > + VIR_FORCE_CLOSE(tfd); I'd use VIR_CLOSE(fd) to check for and report errors. > +static void > +virNWFilterSnoopLeaseFileLoad(void) > +{ > + char line[256], ifkey[VIR_IFKEY_LEN]; > + char ipstr[INET_ADDRSTRLEN], srvstr[INET_ADDRSTRLEN]; > + virNWFilterSnoopIPLease ipl; > + virNWFilterSnoopReqPtr req; > + time_t now; > + FILE *fp; > + int ln = 0, tmp; > + > + /* protect the lease file */ > + virNWFilterSnoopLock(); > + > + fp = fopen(LEASEFILE, "r"); > + time(&now); > + while (fp && fgets(line, sizeof(line), fp)) { > + if (line[strlen(line)-1] != '\n') { > + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, > + _("virNWFilterSnoopLeaseFileLoad lease file " > + "line %d corrupt"), ln); > + break; > + } > + ln++; > + /* key len 55 = "VMUUID"+'-'+"MAC" */ > + if (sscanf(line, "%u %55s %16s %16s", &ipl.timeout, > + ifkey, ipstr, srvstr) < 4) { I'm trying to reduce the number of sscanf(%u) in our codebase. *scanf has undefined behavior on integer overflow, and if someone were to intentionally corrupt our lease file (unlikely as that is - if they have write permissions there they have write permissions to do much worse), then using virStrToLong_* rather than *scanf will let us detect it. But as you are not the lone offender, I can save it for a subsequent global cleanup. > + > + if (virSocketAddrParseIPv4(&ipl.ipAddress, ipstr) < 0) { > + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, > + _("line %d corrupt ipaddr \"%s\""), > + ln, ipstr); > + virNWFilterSnoopReqPut(req); > + continue; > + } > + (void) virSocketAddrParseIPv4(&ipl.ipServer, srvstr); Why are you reporting errors on one address and ignoring the other? > + > +/* > + * Iterator to remove a requests, repeatedly called on one s/a requests/requests/ > + * request after another. > + * The requests' ifname is freed allowing for an association > + * of the Snoop request's leases with the same VM under a > + * different interface name at a later time. > + */ > +static int > +virNWFilterSnoopRemAllReqIter(const void *payload, > + const void *name ATTRIBUTE_UNUSED, > + const void *data ATTRIBUTE_UNUSED) > +++ libvirt-acl/src/nwfilter/nwfilter_dhcpsnoop.h > @@ -0,0 +1,39 @@ > +/* > + * nwfilter_dhcpsnoop.h: support DHCP snooping for a VM on an interface > + * > + * Copyright (C) 2010 IBM Corp. > + * Copyright (C) 2010 David L Stevens Shouldn't you also include 2012? I found a few things worth fixing, but I think the overall patch is pretty sound. I'm okay if you get it into the tree now so it can get wider exposure, and leave it up to you whether to post a v15 (if so, post an incremental diff so I don't have to read the entire patch again) or just push once you've made the fixes. ACK. -- Eric Blake eblake@xxxxxxxxxx +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list