[libvirt] [PATCH V14 2/5] nwfilter: add DHCP snooping

[Stefan Berger]

On 05/25/2012 11:59 PM, Eric Blake wrote:
>> +<span class="since">Since 0.9.12</span>
> s/12/13/ (hmm, sorry for the delays)

No problem.

>> +# define virNWFilterSnoopLock() \
>> +    do { \
>> +        virMutexLock(&virNWFilterSnoopState.snoopLock); \
>> +    } while (0);
> Ouch.  Lose the trailing ';', or you will cause problems with:
>
> if (foo)
>      virNWFilterSnoopLock();
> else
>      bar;
Fixed this and 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.)

Fixed.

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

pip->ihl accesses 4 bits of a byte. So that should be ok.

The MAC header is typically (6+6+2) 14 bytes long. The IP header is a 
multiple of 4 bytes, typically 20 bytes. UDP is always 8 bytes. So we 
have 2 bytes alignment once we get to the DHCP header. Inside the DHCP 
header we are accessing the 32bit IPv4 addresses and timeouts using 
memcpy now and otherwise there are only byte accesses.

>> +    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?

Ooops. Fixed.

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

Yes, it's user-visible. Fixed.

> +
> +    /* 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.

Fixed this and converted to use virAsprintf.

>> +
>> +    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?

I change this to use ignore_value().

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

Fixed.

>> +    /* 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.

Fixed. Skipping the renaming in case of an error.

> +        }
> +        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.

Then I'll cowardly dodge this one now.


>> +
>> +        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?

The DHCP server's IP address is (currently) recorded more for debugging 
purposes but not as important for this code to work correctly.

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

I fixed a lot of the problems you mentioned.

For patch 3/5 I could use the ACK that DV gave on 4/19:

https://www.redhat.com/archives/libvir-list/2012-April/msg01009.html

I hope you'll have time to review the other 2 patches. 5/5 for example 
makes it easier to verify what is going on since it adds the convenience 
of displaying the current leases in the XML.

Thanks for the review.

I'll wait a bit until I push the patch.

      Stefan