linux-kernel - Re: [patch net-next V5 01/15] 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] [day] [month] [year] [list]

Message-ID: <1357153046.3652.11.camel@bwh-desktop.uk.solarflarecom.com>
Date:	Wed, 2 Jan 2013 18:57:26 +0000
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 V5 01/15] net: introduce upper device lists

On Wed, 2013-01-02 at 13:28 +0100, 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 upper present. In case the upper link is
> replacement for dev->master, it is marked with "master" flag.
> 
> New upper device list resolves this limitation. Also, the information
> stored in lists is used for preventing looping setups like
> "bond->somethingelse->samebond"

Thanks for continuing with this, Jiri.  I just see some
cosmetic/documentation issues:

[...]
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
[...]
> +static bool __netdev_has_upper_dev(struct net_device *dev,
> +				   struct net_device *upper_dev)
> +{
> +	LIST_HEAD(search_list);
> +	struct netdev_upper *upper;
> +	struct netdev_upper *tmp;
> +	bool ret = false;
> +
> +	__append_search_uppers(&search_list, dev);
> +	list_for_each_entry(upper, &search_list, search_list) {
> +		if (upper->dev == upper_dev) {
> +			ret = true;
> +			break;
> +		}
> +		__append_search_uppers(&search_list, upper->dev);
> +	}
> +	list_for_each_entry_safe(upper, tmp, &search_list, search_list)
> +		INIT_LIST_HEAD(&upper->search_list);
> +	return ret;
> +}
> +
> +static struct netdev_upper *__netdev_find_upper(struct net_device *dev,
> +						struct net_device *upper_dev)
> +{
> +	struct netdev_upper *upper;
> +
> +	list_for_each_entry(upper, &dev->upper_dev_list, list) {
> +		if (upper->dev == upper_dev)
> +			return upper;
> +	}
> +	return NULL;
> +}
> +
> +/**
> + * netdev_has_upper_dev - Check if device is linked to an upper device

Please clarify that this (and other functions) only checks for an
immediate upper device and not through a complete stack of devices.

> + * @dev: device
> + * @upper_dev: upper device to check
> + *
> + * Find out if a device is linked to specified upper device and return true
> + * in case it is. The caller must hold the RTNL semaphore.

It is no longer a semaphore, even if some kernel-doc in this file calls
it that.  'RTNL lock' would be better as it matches the function naming:
rtnl_lock() etc.

> + */
> +bool netdev_has_upper_dev(struct net_device *dev,
> +			  struct net_device *upper_dev)

The '__' prefix normally implies doing less work than the un-prefixed
function but __netdev_has_upper_dev() checks all devices stacked above
dev whereas this only checks a single level.  Therefore I think one or
both should be renamed.

> +{
> +	ASSERT_RTNL();
> +
> +	return __netdev_find_upper(dev, upper_dev);
> +}
> +EXPORT_SYMBOL(netdev_has_upper_dev);
[...]
> +static int __netdev_upper_dev_link(struct net_device *dev,
> +				   struct net_device *upper_dev, bool master)
> +{
[...]
> +}
> +/**
> + * netdev_upper_dev_link - Add a link to the upper device
[...] 

There should be a blank line between the closing brace and the comment
for the next function.

Ben.

-- 
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 linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/