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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170706142427.GA32086@zx2c4.com>
Date:   Thu, 6 Jul 2017 16:24:29 +0200
From:   "Jason A. Donenfeld" <Jason@...c4.com>
To:     Netdev <netdev@...r.kernel.org>
Subject: Re: net: Fix inconsistent teardown and release of private netdev
 state.

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