Re: net: Fix inconsistent teardown and release of private netdev state.

[Jason A. Donenfeld]

Hey guys,

I see why this priv_destructor patch is an acceptable bandaid for
certain drivers, but I'm not exactly sure on the cleanest way of using
it in new drivers. Check out the following psuedocode.

Here's how error handling in newlink used to work before this patch:

static void destruct(struct net_device *dev)
{
        struct private *priv = netdev_priv(dev);

        rtnl_lock();
        list_del(&priv->list);
        rtnl_unlock();
        stop_anotherweirdthing(&priv->wow);
        put_can_in_garbage(priv->burrito);
        kfree(priv->cheese);
        stop_something(&priv->something);
        put_whatever(priv->whatever);
}

static int newlink(struct net *src_net, struct net_device *dev, ...)
{
        int ret;
        struct private *priv = netdev_priv(dev);

        priv->whatever = get_whatever(src_net); // never fails

        ret = do_something(&private);
        if (ret < 0)
                goto error_1;

        ret = -ENOMEM;
        priv->cheese = kmalloc(128, GFP_KERNEL);
        if (!priv->cheese)
                goto error_2;

        priv->burrito = allocate_bean_can(&init_net);
        if (IS_ERR(priv->burrito)) {
                ret = PTR_ERR(priv->burrito);
                goto error_3;
        }

        do_anotherweirdthing(&priv->wow); // never fails

        ret = register_netdevice();
        if (ret < 0)
                goto error_4;

        list_add(&priv->list, &list_of_things);
        pr_info("Yay it worked!\n");
        
        return ret;

error_4:
        stop_anotherweirdthing(&priv->wow);
        put_can_in_garbage(priv->burrito);
error_3:
        kfree(priv->cheese);
error_2:
        stop_something(&priv->something);
error_1:
        put_whatever(priv->whatever);

        return ret;
}

Here's what it must look like now, which introduces a weird race with
list_add, and doesn't really have a consistent clean-up section. Having
to remember that register_netdevice will call destruct() when it fails
is quite confusing. Observe:

static int newlink(struct net *src_net, struct net_device *dev, ...)
{
        int ret;
        struct private *priv = netdev_priv(dev);

        priv->whatever = get_whatever(src_net); // never fails

        ret = do_something(&private);
        if (ret < 0)
                goto error_1;

        ret = -ENOMEM;
        priv->cheese = kmalloc(128, GFP_KERNEL);
        if (!priv->cheese)
                goto error_2;

        priv->burrito = allocate_bean_can(&init_net);
        if (IS_ERR(priv->burrito)) {
                ret = PTR_ERR(priv->burrito);
                goto error_3;
        }

        do_anotherweirdthing(&priv->wow); // never fails

        list_add(&priv->list, &list_of_things);

        ret = register_netdevice(); // if ret is < 0, then destruct above is 
automatically called

        // RACE WITH LIST_ADD/LIST_DEL!! It's impossible to call list_add only 
after
        // things are brought up successfully. This is problematic.

        if (!ret)
                pr_info("Yay it worked!\n");

        return 0;

error_3:
        kfree(priv->cheese);
error_2:
        stop_something(&priv->something);
error_1:
        put_whatever(priv->whatever);

        return ret;
}

Something we just have to live with? Any cleaner way of approaching
this?

Regards,
Jason