netdev - Re: [patch net-next 01/16] net: introduce upper device lists

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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1344877451.2733.26.camel@bwh-desktop.uk.solarflarecom.com>
Date:	Mon, 13 Aug 2012 18:04:11 +0100
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	Jiri Pirko <jiri@...nulli.us>
CC:	<netdev@...r.kernel.org>, <davem@...emloft.net>,
	<edumazet@...gle.com>, <faisal.latif@...el.com>,
	<roland@...nel.org>, <sean.hefty@...el.com>,
	<hal.rosenstock@...il.com>, <fubar@...ibm.com>,
	<andy@...yhouse.net>, <divy@...lsio.com>,
	<jitendra.kalsaria@...gic.com>, <sony.chacko@...gic.com>,
	<linux-driver@...gic.com>, <kaber@...sh.net>,
	<ursula.braun@...ibm.com>, <blaschka@...ux.vnet.ibm.com>,
	<linux390@...ibm.com>, <shemminger@...tta.com>,
	<therbert@...gle.com>, <xiyou.wangcong@...il.com>,
	<joe@...ches.com>, <gregory.v.rose@...el.com>,
	<john.r.fastabend@...el.com>, <linux-rdma@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <linux-s390@...r.kernel.org>,
	<bridge@...ts.linux-foundation.org>, <fbl@...hat.com>
Subject: Re: [patch net-next 01/16] net: introduce upper device lists

On Mon, 2012-08-13 at 17:27 +0200, Jiri Pirko wrote:
> This lists are supposed to serve for storing pointers to all upper devices.
> Eventually it will replace dev->master pointer which is used for
> bonding, bridge, team but it cannot be used for vlan, macvlan where
> there might be multiple "masters" present.
> 
> New upper device list resolves this limitation. Also, the information
> stored in lists is used for preventing looping setups like
> "bond->somethingelse->samebond"
> 
> Signed-off-by: Jiri Pirko <jiri@...nulli.us>
[...]
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4425,6 +4425,229 @@ static int __init dev_proc_init(void)
>  #endif	/* CONFIG_PROC_FS */
>  
> 
> +struct netdev_upper {
> +	struct net_device *dev;
> +	bool unique;

This needs a better name.  It doesn't really have anything to do with
uniqueness and doesn't ensure exclusivity.  I think that it would be
fine to keep the 'master' term.

> +	struct list_head list;
> +	struct rcu_head rcu;
> +};
[...]
> +static int __netdev_upper_dev_link(struct net_device *dev,
> +				   struct net_device *upper_dev, bool unique)
> +{
> +	struct netdev_upper *upper;
> +
> +	ASSERT_RTNL();
> +
> +	if (dev == upper_dev)
> +		return -EBUSY;
> +	/*
> +	 * To prevent loops, check if dev is not upper device to upper_dev.
> +	 */
> +	if (__netdev_has_upper_dev(upper_dev, dev, true))
> +		return -EBUSY;
> +
> +	if (__netdev_find_upper(dev, upper_dev))
> +		return -EEXIST;
> +
> +	if (unique && netdev_unique_upper_dev_get(dev))
> +		return -EBUSY;
> +
> +	upper = kmalloc(sizeof(*upper), GFP_KERNEL);
> +	if (!upper)
> +		return -ENOMEM;
> +
> +	upper->dev = upper_dev;
> +	upper->unique = unique;
> +
> +	/*
> +	 * Ensure that unique upper link is always the first item in the list.
> +	 */
> +	if (unique)
> +		list_add_rcu(&upper->list, &dev->upper_dev_list);
> +	else
> +		list_add_tail_rcu(&upper->list, &dev->upper_dev_list);
> +	dev_hold(upper_dev);

This behaviour (calling dev_hold()) matches netdev_set_master().  But
it's oddly asymmetric: generally the administrator can remove either the
upper device or the lower device (rtnl_link_ops or unbinding a physical
device) and the upper device driver must then unlink itself from the
lower device (using a notifier to catch lower device removal).

If the upper device driver fails to unlink when the upper device is
unregistered, then this extra reference causes netdev_wait_allrefs() to
hang... is that the intent?  Or should there be a more explicit counter
and check on unregistration, e.g. WARN_ON(dev->num_lower_devs != 0)?

If it fails to unlink when the lower device is removed, this warning in
rollback_registered_many() may be triggered:

		/* Notifier chain MUST detach us from master device. */
		WARN_ON(dev->master);

I think that needs to become WARN_ON(netdev_has_upper_dev(dev)).

> +	return 0;
> +}
[...] 

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html