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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 2017-03-23 at 12:09 -0400, John Ferlan wrote:
> 
> 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?

IMHO getting more than one RTA_OIF per message would be a bug from the
kernel: a route is associated to one device. Think of these messages like
the lines printed by 'ip r'

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

I can remove the break without that for sure.

> John (who doesn't know the answer!)
> 
> BTW: The issue from patch4 is resolved by my 22 patch bomb from yesterday

Yep, seen that one: thanks a lot
--
Cedric

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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux