[libvirt] [PATCH v2 5/5] network: check accept_ra before enabling ipv6 forwarding

[John Ferlan]

On 03/23/2017 04:44 AM, Cedric Bosdonnat wrote:
> On Wed, 2017-03-22 at 07:16 -0400, John Ferlan wrote:
>> [...]
>>
>>> +
>>> +static int
>>> +virNetDevIPCheckIPv6ForwardingCallback(const struct nlmsghdr *resp,
>>> +                                       void *opaque)
>>> +{
>>> +    struct rtmsg *rtmsg = NLMSG_DATA(resp);
>>> +    int accept_ra = -1;
>>> +    struct rtattr *rta;
>>> +    char *ifname = NULL;
>>> +    struct virNetDevIPCheckIPv6ForwardingData *data = opaque;
>>> +    int ret = 0;
>>> +    int len = RTM_PAYLOAD(resp);
>>> +    int oif = -1;
>>> +
>>> +    /* Ignore messages other than route ones */
>>> +    if (resp->nlmsg_type != RTM_NEWROUTE)
>>> +        return ret;
>>> +
>>> +    /* Extract a few attributes */
>>> +    for (rta = RTM_RTA(rtmsg); RTA_OK(rta, len); rta = RTA_NEXT(rta, len)) {
>>> +        switch (rta->rta_type) {
>>> +        case RTA_OIF:
>>> +            oif = *(int *)RTA_DATA(rta);
>>> +
>>> +            if (!(ifname = virNetDevGetName(oif)))
>>> +                goto error;
>>> +            break;
>>
>> Did you really mean to break from the for loop if ifname is set?  This
>> breaks only from the switch/case.  Of course Coverity doesn't know much
>> more than you'd be going back to the top of the for loop and could
>> overwrite ifname again. It proclaims a resource leak...
> 
> In my dev version I was also getting the RTA_DST attribute to print some
> debugging message that I removed in the submitted version. The break was
> here between the switch cases.
> 
> In the current case I don't really care if we break out of the loop or not.
> As there aren't that many attributes in an rtnetlink message to loop over,
> breaking wouldn't gain a lot of cycles.
> 
> What I don't get though is what this break is actually doing? isn't it
> for the switch case even though there is no other case after it?
> 
> --
> Cedric

So should this be changed to:

        if (rta->rta_type == RTA_OIF) {
            oif = *(int *)RTA_DATA(rta);

            if (!(ifname = virNetDevGetName(oif)))
                goto error;
            break;
        }

leaving two questions in my mind

  1. Can there be more than one RTA_OIF
  2. If we don't finish the loop (e.g. we break out), then does one of
the subsequent checks fail?

Is it more of a problem that we find *two* RTA_OIF's and thus should add a:

    if (ifname) {
        VIR_DEBUG("some sort of message");
        goto error;
    }

prior to the virNetDevGetName call and then remove the break;?

John (who doesn't know the answer!)

BTW: The issue from patch4 is resolved by my 22 patch bomb from yesterday

> 
>> John
>>> +        }
>>> +    }
>>> +
>>> +    /* No need to do anything else for non RA routes */
>>> +    if (rtmsg->rtm_protocol != RTPROT_RA)
>>> +        goto cleanup;
>>> +
>>> +    data->hasRARoutes = true;
>>> +
>>> +    /* Check the accept_ra value for the interface */
>>> +    accept_ra = virNetDevIPGetAcceptRA(ifname);
>>> +    VIR_DEBUG("Checking route for device %s, accept_ra: %d", ifname, accept_ra);
>>> +
>>> +    if (accept_ra != 2 && VIR_APPEND_ELEMENT(data->devices, data->ndevices, ifname) < 0)
>>> +        goto error;
>>> +
>>> + cleanup:
>>> +    VIR_FREE(ifname);
>>> +    return ret;
>>> +
>>> + error:
>>> +    ret = -1;
>>> +    goto cleanup;
>>> +}
>>
>>