Re: [libvirt] [PATCH v2 5/5] network: check accept_ra before enabling ipv6 forwarding
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;
>> +}
>
>