netdev - Re: [patch v5, kernel version 3.2.1] Source mode for macvlan interface

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]

Date:	Fri, 27 Jan 2012 11:38:41 +0100
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Stefan Gula <steweg@...t.sk>
Cc:	Patrick McHardy <kaber@...sh.net>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [patch v5, kernel version 3.2.1] Source mode for macvlan
 interface

Le vendredi 27 janvier 2012 à 10:58 +0100, Stefan Gula a écrit :
> From: Stefan Gula <steweg@...il.com>
> 

...

> +static struct macvlan_source_list *macvlan_hash_lookup_sources_list(
> +	const struct macvlan_dev *vlan,
> +	const unsigned char *addr)
> +{
> +	struct macvlan_source_list *list;
> +	struct hlist_node *n;
> +	struct hlist_head *h = &vlan->port->vlan_source_hash[addr[5]];
> +
> +	hlist_for_each_entry_rcu(list, n, h, hlist) {

You dont hold rcu_read_lock() here, and dont need to, since you are in
the controler path, rtnl is locked :

	hlist_for_each_entry(...)

> +		if (!compare_ether_addr_64bits(list->addr, addr) &&
> +		    list->vlan == vlan)
> +			return list;
> +	}
> +	return NULL;
> +}
> +
> +static int macvlan_hash_add_sources(struct macvlan_dev *vlan,
> +				    const unsigned char *addr)
> +{
> +	struct macvlan_port *port = vlan->port;
> +	struct macvlan_source_list *list;
> +	struct hlist_head *h;
> +
> +	list = macvlan_hash_lookup_sources_list(vlan, addr);
> +	if (!list) {
> +		list = kmalloc(sizeof(*list), GFP_KERNEL);

By the way, use of GFP_KERNEL here is a proof you dont hold
rcu_read_lock().

(If you were, you would not be allowed to sleep in kmalloc())

> +		if (list) {
> +			memcpy(list->addr, addr, ETH_ALEN);
> +			list->vlan = vlan;
> +			h = &port->vlan_source_hash[addr[5]];
> +			hlist_add_head_rcu(&list->hlist, h);
> +		} else
> +			return -ENOMEM;

	if (cond) {
		foo;
		bar;
	} else {
		blip;
	}


> +	}
> +	return 0;
> +}
> +
> +static void macvlan_hash_del_sources(struct macvlan_source_list *list)
> +{
> +	hlist_del_rcu(&list->hlist);
> +	kfree_rcu(list, rcu);
> +}
> +

...

> +static int macvlan_changelink_sources(struct macvlan_dev *vlan, u32 mode,
> +				      unsigned char *addr)
> +{
> +	if (mode == MACVLAN_MACADDR_ADD)
> +		return macvlan_hash_add_sources(vlan, addr);
> +

Same codestyle here...

	if (cond) {
		foo;
	} else if (cond2) {
	} else {
		blip;
	}


> +	else if (mode == MACVLAN_MACADDR_DEL) {
> +		struct macvlan_source_list *list;
> +
> +		list = macvlan_hash_lookup_sources_list(vlan, addr);
> +		if (list)
> +			macvlan_hash_del_sources(list);
> +	} else
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +


--
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