[ovs-dev] [PATCH 2/2] veth: Do a better job cleaning up on rmmod

Justin Pettit jpettit at nicira.com
Thu Apr 22 00:26:24 UTC 2010


On Apr 21, 2010, at 7:46 AM, Jesse Gross wrote:

> On Wed, Apr 21, 2010 at 1:50 AM, Justin Pettit <jpettit at nicira.com> wrote:
>  static __exit void veth_exit(void)
>  {
> +       struct veth_priv *p, *n;
> +
> +       rtnl_lock();
> +       list_for_each_entry_safe(p, n, &veth_list, list) {
> +               veth_dellink(p->dev);
> +       }
> 
> I don't think this is safe.  With the *_safe variants of functions it is OK to remove the current element but not necessarily arbitrary elements since they store the next list entry in n before returning p so you can delete p.  However, veth_dellink deletes both the link and its peer (which is probably the next entry).

Please double-check me, but I don't think there's a problem.  While confusing, only the first device is added to that list:

    for (i = 0; i < 2; i++) {
        struct veth_priv *priv = netdev_priv(devs[i]);
        priv->dev = devs[i];
        priv->peer = devs[!i];
        if (!i)
            list_add(&priv->list, &veth_list);
        else
            INIT_LIST_HEAD(&priv->list);
    }

> Also, kernel style is to drop the braces.

Ah right, I'll clean that up if you agree that the rest of the code looks okay.  (I'm guessing this buggy implementation of veth is not going to replace the existing one upstream.  ;-)  )

--Justin






More information about the dev mailing list