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

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

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

[Jason A. Donenfeld]

On Thu, Jul 6, 2017 at 4:24 PM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
>         if (!ret)
>                 pr_info("Yay it worked!\n");
>
>         return 0;

This is supposed to be `return ret;`. See, even in psuedocode it's
hard to get right.

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

[Cong Wang]

On Thu, Jul 6, 2017 at 7:24 AM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>         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");

I fail to understand what you mean by RACE here.

Here you should already have RTNL lock, so it can't race with any other
newlink() calls. In fact you can't acquire RTNL lock in your destructor
since register_netdevice() already gets it. Perhaps you mean
netdev_run_todo() calls it without RTNL lock?

I don't know why you reorder the above list_add(), you can order it
as it was before, aka, call it after register_netdevice(), but you have to
init the priv->list now for the list_del() on error path.

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

[Jason A. Donenfeld]

On Sat, Jul 8, 2017 at 12:39 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Thu, Jul 6, 2017 at 7:24 AM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>>         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");
>
> I fail to understand what you mean by RACE here.
>
> Here you should already have RTNL lock, so it can't race with any other
> newlink() calls. In fact you can't acquire RTNL lock in your destructor
> since register_netdevice() already gets it. Perhaps you mean
> netdev_run_todo() calls it without RTNL lock?
>
> I don't know why you reorder the above list_add(), you can order it
> as it was before, aka, call it after register_netdevice(), but you have to
> init the priv->list now for the list_del() on error path.

The race is that there's a state in which priv->list is part of
list_of_things before the interface is actually successfully setup and
ready to go.

And no, it's not possible to order it _after_ register_netdevice,
since register_netdevice might call priv_destructor, and
priv_destructor calls list_del, so if it's not already on the list,
we'll OOPS. In otherwords, API problem.

To work around this shortcoming, I'm actually just assigning
dev->priv_device *after* a successful call to register_netdevice. This
seems to be working well and allows me to retain ± the old behavior.
Hopefully this is an okay way to go about things?

Jason

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

[Cong Wang]

On Sun, Jul 9, 2017 at 7:07 PM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> On Sat, Jul 8, 2017 at 12:39 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> On Thu, Jul 6, 2017 at 7:24 AM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>>>         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");
>>
>> I fail to understand what you mean by RACE here.
>>
>> Here you should already have RTNL lock, so it can't race with any other
>> newlink() calls. In fact you can't acquire RTNL lock in your destructor
>> since register_netdevice() already gets it. Perhaps you mean
>> netdev_run_todo() calls it without RTNL lock?
>>
>> I don't know why you reorder the above list_add(), you can order it
>> as it was before, aka, call it after register_netdevice(), but you have to
>> init the priv->list now for the list_del() on error path.
>
> The race is that there's a state in which priv->list is part of
> list_of_things before the interface is actually successfully setup and
> ready to go.
>
> And no, it's not possible to order it _after_ register_netdevice,
> since register_netdevice might call priv_destructor, and
> priv_destructor calls list_del, so if it's not already on the list,
> we'll OOPS. In otherwords, API problem.


As I said, you have to initialize it, list_del() on an empty head
is literally a nop, why oops?