[patch net-next 0/3] team: bug fixes

[patch net-next 0/3] team: bug fixes

[Jiri Pirko]

Jiri Pirko (3):
  team: Do not hold rcu_read_lock when running netlink cmds
  team: convert overall spinlock to mutex
  team: replicate options on register

 drivers/net/team/team.c                   |  120 +++++++++++++++++++++--------
 drivers/net/team/team_mode_activebackup.c |    5 +-
 include/linux/if_team.h                   |   10 +-
 3 files changed, 93 insertions(+), 42 deletions(-)

-- 
1.7.6

[patch net-next 1/3] team: Do not hold rcu_read_lock when running netlink cmds

[Jiri Pirko]

Signed-off-by: Jiri Pirko <jpirko@redhat.com>
---
 drivers/net/team/team.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index 60672bb..e0cf11c 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -1043,8 +1043,7 @@ err_msg_put:
 
 /*
  * Netlink cmd functions should be locked by following two functions.
- * To ensure team_uninit would not be called in between, hold rcu_read_lock
- * all the time.
+ * Since dev gets held here, that ensures dev won't disappear in between.
  */
 static struct team *team_nl_team_get(struct genl_info *info)
 {
@@ -1057,12 +1056,9 @@ static struct team *team_nl_team_get(struct genl_info *info)
 		return NULL;
 
 	ifindex = nla_get_u32(info->attrs[TEAM_ATTR_TEAM_IFINDEX]);
-	rcu_read_lock();
-	dev = dev_get_by_index_rcu(net, ifindex);
-	if (!dev || dev->netdev_ops != &team_netdev_ops) {
-		rcu_read_unlock();
+	dev = dev_get_by_index(net, ifindex);
+	if (!dev || dev->netdev_ops != &team_netdev_ops)
 		return NULL;
-	}
 
 	team = netdev_priv(dev);
 	spin_lock(&team->lock);
@@ -1072,7 +1068,7 @@ static struct team *team_nl_team_get(struct genl_info *info)
 static void team_nl_team_put(struct team *team)
 {
 	spin_unlock(&team->lock);
-	rcu_read_unlock();
+	dev_put(team->dev);
 }
 
 static int team_nl_send_generic(struct genl_info *info, struct team *team,
-- 
1.7.6

Re: [patch net-next 1/3] team: Do not hold rcu_read_lock when running netlink cmds

[Eric Dumazet]

Le mercredi 16 novembre 2011 à 22:09 +0100, Jiri Pirko a écrit :
> Signed-off-by: Jiri Pirko <jpirko@redhat.com>
> ---
>  drivers/net/team/team.c |   12 ++++--------
>  1 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
> index 60672bb..e0cf11c 100644
> --- a/drivers/net/team/team.c
> +++ b/drivers/net/team/team.c
> @@ -1043,8 +1043,7 @@ err_msg_put:
>  
>  /*
>   * Netlink cmd functions should be locked by following two functions.
> - * To ensure team_uninit would not be called in between, hold rcu_read_lock
> - * all the time.
> + * Since dev gets held here, that ensures dev won't disappear in between.
>   */
>  static struct team *team_nl_team_get(struct genl_info *info)
>  {
> @@ -1057,12 +1056,9 @@ static struct team *team_nl_team_get(struct genl_info *info)
>  		return NULL;
>  
>  	ifindex = nla_get_u32(info->attrs[TEAM_ATTR_TEAM_IFINDEX]);
> -	rcu_read_lock();
> -	dev = dev_get_by_index_rcu(net, ifindex);
> -	if (!dev || dev->netdev_ops != &team_netdev_ops) {
> -		rcu_read_unlock();
> +	dev = dev_get_by_index(net, ifindex);
> +	if (!dev || dev->netdev_ops != &team_netdev_ops)

Hmmm, you return NULL but dev refcnt was increased...

>  		return NULL;
> -	}
>  
>  	team = netdev_priv(dev);
>  	spin_lock(&team->lock);
> @@ -1072,7 +1068,7 @@ static struct team *team_nl_team_get(struct genl_info *info)
>  static void team_nl_team_put(struct team *team)
>  {
>  	spin_unlock(&team->lock);
> -	rcu_read_unlock();
> +	dev_put(team->dev);
>  }
>  
>  static int team_nl_send_generic(struct genl_info *info, struct team *team,

[patch net-next 1/3 v2] team: Do not hold rcu_read_lock when running netlink cmds

[Jiri Pirko]

Signed-off-by: Jiri Pirko <jpirko@redhat.com>
---
 drivers/net/team/team.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index 60672bb..e5390c7 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -1043,8 +1043,7 @@ err_msg_put:
 
 /*
  * Netlink cmd functions should be locked by following two functions.
- * To ensure team_uninit would not be called in between, hold rcu_read_lock
- * all the time.
+ * Since dev gets held here, that ensures dev won't disappear in between.
  */
 static struct team *team_nl_team_get(struct genl_info *info)
 {
@@ -1057,10 +1056,10 @@ static struct team *team_nl_team_get(struct genl_info *info)
 		return NULL;
 
 	ifindex = nla_get_u32(info->attrs[TEAM_ATTR_TEAM_IFINDEX]);
-	rcu_read_lock();
-	dev = dev_get_by_index_rcu(net, ifindex);
+	dev = dev_get_by_index(net, ifindex);
 	if (!dev || dev->netdev_ops != &team_netdev_ops) {
-		rcu_read_unlock();
+		if (dev)
+			dev_put(dev);
 		return NULL;
 	}
 
@@ -1072,7 +1071,7 @@ static struct team *team_nl_team_get(struct genl_info *info)
 static void team_nl_team_put(struct team *team)
 {
 	spin_unlock(&team->lock);
-	rcu_read_unlock();
+	dev_put(team->dev);
 }
 
 static int team_nl_send_generic(struct genl_info *info, struct team *team,
-- 
1.7.6

Re: [patch net-next 1/3 v2] team: Do not hold rcu_read_lock when running netlink cmds

[David Miller]

From: Jiri Pirko <jpirko@redhat.com>
Date: Wed, 16 Nov 2011 22:55:54 +0100

> Signed-off-by: Jiri Pirko <jpirko@redhat.com>

Applied.

[patch net-next 2/3] team: convert overall spinlock to mutex

[Jiri Pirko]

No need to have spinlock for this purpose. So convert this to mutex and
avoid current schedule while atomic problems in netlink code.

Signed-off-by: Jiri Pirko <jpirko@redhat.com>
---
 drivers/net/team/team.c |   32 ++++++++++++++++----------------
 include/linux/if_team.h |    2 +-
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index e0cf11c..04f9302 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -443,9 +443,9 @@ static void __team_compute_features(struct team *team)
 
 static void team_compute_features(struct team *team)
 {
-	spin_lock(&team->lock);
+	mutex_lock(&team->lock);
 	__team_compute_features(team);
-	spin_unlock(&team->lock);
+	mutex_unlock(&team->lock);
 }
 
 static int team_port_enter(struct team *team, struct team_port *port)
@@ -647,7 +647,7 @@ static int team_init(struct net_device *dev)
 	int i;
 
 	team->dev = dev;
-	spin_lock_init(&team->lock);
+	mutex_init(&team->lock);
 
 	team->pcpu_stats = alloc_percpu(struct team_pcpu_stats);
 	if (!team->pcpu_stats)
@@ -672,13 +672,13 @@ static void team_uninit(struct net_device *dev)
 	struct team_port *port;
 	struct team_port *tmp;
 
-	spin_lock(&team->lock);
+	mutex_lock(&team->lock);
 	list_for_each_entry_safe(port, tmp, &team->port_list, list)
 		team_port_del(team, port->dev);
 
 	__team_change_mode(team, NULL); /* cleanup */
 	__team_options_unregister(team, team_options, ARRAY_SIZE(team_options));
-	spin_unlock(&team->lock);
+	mutex_unlock(&team->lock);
 }
 
 static void team_destructor(struct net_device *dev)
@@ -784,7 +784,7 @@ static int team_change_mtu(struct net_device *dev, int new_mtu)
 	 * Alhough this is reader, it's guarded by team lock. It's not possible
 	 * to traverse list in reverse under rcu_read_lock
 	 */
-	spin_lock(&team->lock);
+	mutex_lock(&team->lock);
 	list_for_each_entry(port, &team->port_list, list) {
 		err = dev_set_mtu(port->dev, new_mtu);
 		if (err) {
@@ -793,7 +793,7 @@ static int team_change_mtu(struct net_device *dev, int new_mtu)
 			goto unwind;
 		}
 	}
-	spin_unlock(&team->lock);
+	mutex_unlock(&team->lock);
 
 	dev->mtu = new_mtu;
 
@@ -802,7 +802,7 @@ static int team_change_mtu(struct net_device *dev, int new_mtu)
 unwind:
 	list_for_each_entry_continue_reverse(port, &team->port_list, list)
 		dev_set_mtu(port->dev, dev->mtu);
-	spin_unlock(&team->lock);
+	mutex_unlock(&team->lock);
 
 	return err;
 }
@@ -880,9 +880,9 @@ static int team_add_slave(struct net_device *dev, struct net_device *port_dev)
 	struct team *team = netdev_priv(dev);
 	int err;
 
-	spin_lock(&team->lock);
+	mutex_lock(&team->lock);
 	err = team_port_add(team, port_dev);
-	spin_unlock(&team->lock);
+	mutex_unlock(&team->lock);
 	return err;
 }
 
@@ -891,9 +891,9 @@ static int team_del_slave(struct net_device *dev, struct net_device *port_dev)
 	struct team *team = netdev_priv(dev);
 	int err;
 
-	spin_lock(&team->lock);
+	mutex_lock(&team->lock);
 	err = team_port_del(team, port_dev);
-	spin_unlock(&team->lock);
+	mutex_unlock(&team->lock);
 	return err;
 }
 
@@ -1061,13 +1061,13 @@ static struct team *team_nl_team_get(struct genl_info *info)
 		return NULL;
 
 	team = netdev_priv(dev);
-	spin_lock(&team->lock);
+	mutex_lock(&team->lock);
 	return team;
 }
 
 static void team_nl_team_put(struct team *team)
 {
-	spin_unlock(&team->lock);
+	mutex_unlock(&team->lock);
 	dev_put(team->dev);
 }
 
@@ -1483,9 +1483,9 @@ static void team_port_change_check(struct team_port *port, bool linkup)
 {
 	struct team *team = port->team;
 
-	spin_lock(&team->lock);
+	mutex_lock(&team->lock);
 	__team_port_change_check(port, linkup);
-	spin_unlock(&team->lock);
+	mutex_unlock(&team->lock);
 }
 
 /************************************
diff --git a/include/linux/if_team.h b/include/linux/if_team.h
index 14f6388..a6eac12 100644
--- a/include/linux/if_team.h
+++ b/include/linux/if_team.h
@@ -92,7 +92,7 @@ struct team {
 	struct net_device *dev; /* associated netdevice */
 	struct team_pcpu_stats __percpu *pcpu_stats;
 
-	spinlock_t lock; /* used for overall locking, e.g. port lists write */
+	struct mutex lock; /* used for overall locking, e.g. port lists write */
 
 	/*
 	 * port lists with port count
-- 
1.7.6

Re: [patch net-next 2/3] team: convert overall spinlock to mutex

[David Miller]

From: Jiri Pirko <jpirko@redhat.com>
Date: Wed, 16 Nov 2011 22:09:08 +0100

> No need to have spinlock for this purpose. So convert this to mutex and
> avoid current schedule while atomic problems in netlink code.
> 
> Signed-off-by: Jiri Pirko <jpirko@redhat.com>

Applied.

[patch net-next 3/3] team: replicate options on register

[Jiri Pirko]

Since multiple team instances are putting defined options into their
option list, during register each option must be cloned before added
into list. This resolves uncool memory corruptions when using multiple
teams.

Signed-off-by: Jiri Pirko <jpirko@redhat.com>
---
 drivers/net/team/team.c                   |   76 +++++++++++++++++++++++++----
 drivers/net/team/team_mode_activebackup.c |    5 +-
 include/linux/if_team.h                   |    8 ++--
 3 files changed, 72 insertions(+), 17 deletions(-)

diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index 04f9302..b47f43e 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -80,30 +80,78 @@ EXPORT_SYMBOL(team_port_set_team_mac);
  * Options handling
  *******************/
 
-void team_options_register(struct team *team, struct team_option *option,
-			   size_t option_count)
+struct team_option *__team_find_option(struct team *team, const char *opt_name)
+{
+	struct team_option *option;
+
+	list_for_each_entry(option, &team->option_list, list) {
+		if (strcmp(option->name, opt_name) == 0)
+			return option;
+	}
+	return NULL;
+}
+
+int team_options_register(struct team *team,
+			  const struct team_option *option,
+			  size_t option_count)
 {
 	int i;
+	struct team_option *dst_opts[option_count];
+	int err;
+
+	memset(dst_opts, 0, sizeof(dst_opts));
+	for (i = 0; i < option_count; i++, option++) {
+		struct team_option *dst_opt;
+
+		if (__team_find_option(team, option->name)) {
+			err = -EEXIST;
+			goto rollback;
+		}
+		dst_opt = kmalloc(sizeof(*option), GFP_KERNEL);
+		if (!dst_opt) {
+			err = -ENOMEM;
+			goto rollback;
+		}
+		memcpy(dst_opt, option, sizeof(*option));
+		dst_opts[i] = dst_opt;
+	}
+
+	for (i = 0; i < option_count; i++)
+		list_add_tail(&dst_opts[i]->list, &team->option_list);
 
-	for (i = 0; i < option_count; i++, option++)
-		list_add_tail(&option->list, &team->option_list);
+	return 0;
+
+rollback:
+	for (i = 0; i < option_count; i++)
+		kfree(dst_opts[i]);
+
+	return err;
 }
+
 EXPORT_SYMBOL(team_options_register);
 
 static void __team_options_change_check(struct team *team,
 					struct team_option *changed_option);
 
 static void __team_options_unregister(struct team *team,
-				      struct team_option *option,
+				      const struct team_option *option,
 				      size_t option_count)
 {
 	int i;
 
-	for (i = 0; i < option_count; i++, option++)
-		list_del(&option->list);
+	for (i = 0; i < option_count; i++, option++) {
+		struct team_option *del_opt;
+
+		del_opt = __team_find_option(team, option->name);
+		if (del_opt) {
+			list_del(&del_opt->list);
+			kfree(del_opt);
+		}
+	}
 }
 
-void team_options_unregister(struct team *team, struct team_option *option,
+void team_options_unregister(struct team *team,
+			     const struct team_option *option,
 			     size_t option_count)
 {
 	__team_options_unregister(team, option, option_count);
@@ -632,7 +680,7 @@ static int team_mode_option_set(struct team *team, void *arg)
 	return team_change_mode(team, *str);
 }
 
-static struct team_option team_options[] = {
+static const struct team_option team_options[] = {
 	{
 		.name = "mode",
 		.type = TEAM_OPTION_TYPE_STRING,
@@ -645,6 +693,7 @@ static int team_init(struct net_device *dev)
 {
 	struct team *team = netdev_priv(dev);
 	int i;
+	int err;
 
 	team->dev = dev;
 	mutex_init(&team->lock);
@@ -660,10 +709,17 @@ static int team_init(struct net_device *dev)
 	team_adjust_ops(team);
 
 	INIT_LIST_HEAD(&team->option_list);
-	team_options_register(team, team_options, ARRAY_SIZE(team_options));
+	err = team_options_register(team, team_options, ARRAY_SIZE(team_options));
+	if (err)
+		goto err_options_register;
 	netif_carrier_off(dev);
 
 	return 0;
+
+err_options_register:
+	free_percpu(team->pcpu_stats);
+
+	return err;
 }
 
 static void team_uninit(struct net_device *dev)
diff --git a/drivers/net/team/team_mode_activebackup.c b/drivers/net/team/team_mode_activebackup.c
index 6fe920c..b344275 100644
--- a/drivers/net/team/team_mode_activebackup.c
+++ b/drivers/net/team/team_mode_activebackup.c
@@ -83,7 +83,7 @@ static int ab_active_port_set(struct team *team, void *arg)
 	return -ENOENT;
 }
 
-static struct team_option ab_options[] = {
+static const struct team_option ab_options[] = {
 	{
 		.name = "activeport",
 		.type = TEAM_OPTION_TYPE_U32,
@@ -94,8 +94,7 @@ static struct team_option ab_options[] = {
 
 int ab_init(struct team *team)
 {
-	team_options_register(team, ab_options, ARRAY_SIZE(ab_options));
-	return 0;
+	return team_options_register(team, ab_options, ARRAY_SIZE(ab_options));
 }
 
 void ab_exit(struct team *team)
diff --git a/include/linux/if_team.h b/include/linux/if_team.h
index a6eac12..828181f 100644
--- a/include/linux/if_team.h
+++ b/include/linux/if_team.h
@@ -140,11 +140,11 @@ static inline struct team_port *team_get_port_by_index_rcu(struct team *team,
 }
 
 extern int team_port_set_team_mac(struct team_port *port);
-extern void team_options_register(struct team *team,
-				  struct team_option *option,
-				  size_t option_count);
+extern int team_options_register(struct team *team,
+				 const struct team_option *option,
+				 size_t option_count);
 extern void team_options_unregister(struct team *team,
-				    struct team_option *option,
+				    const struct team_option *option,
 				    size_t option_count);
 extern int team_mode_register(struct team_mode *mode);
 extern int team_mode_unregister(struct team_mode *mode);
-- 
1.7.6

Re: [patch net-next 3/3] team: replicate options on register

[David Miller]

From: Jiri Pirko <jpirko@redhat.com>
Date: Wed, 16 Nov 2011 22:09:09 +0100

> Since multiple team instances are putting defined options into their
> option list, during register each option must be cloned before added
> into list. This resolves uncool memory corruptions when using multiple
> teams.
> 
> Signed-off-by: Jiri Pirko <jpirko@redhat.com>

Applied.

Re: [patch net-next 3/3] team: replicate options on register

[Eric Dumazet]

Le mercredi 16 novembre 2011 à 22:09 +0100, Jiri Pirko a écrit :

> +
> +int team_options_register(struct team *team,
> +			  const struct team_option *option,
> +			  size_t option_count)
>  {
>  	int i;
> +	struct team_option *dst_opts[option_count];
> +	int err;

This kind of construct will trigger static analyzer alerts...

Re: [patch net-next 3/3] team: replicate options on register

[Jiri Pirko]

Thu, Nov 17, 2011 at 09:32:16AM CET, eric.dumazet@gmail.com wrote:
>Le mercredi 16 novembre 2011 à 22:09 +0100, Jiri Pirko a écrit :
>
>> +
>> +int team_options_register(struct team *team,
>> +			  const struct team_option *option,
>> +			  size_t option_count)
>>  {
>>  	int i;
>> +	struct team_option *dst_opts[option_count];
>> +	int err;
>
>This kind of construct will trigger static analyzer alerts...


I thought this is ok to do in kernel. Should I replace that with
kmalloc/kfree?

>
>
>
>

Re: [patch net-next 3/3] team: replicate options on register

[Eric Dumazet]

Le jeudi 17 novembre 2011 à 10:57 +0100, Jiri Pirko a écrit :
> Thu, Nov 17, 2011 at 09:32:16AM CET, eric.dumazet@gmail.com wrote:
> >Le mercredi 16 novembre 2011 à 22:09 +0100, Jiri Pirko a écrit :
> >
> >> +
> >> +int team_options_register(struct team *team,
> >> +			  const struct team_option *option,
> >> +			  size_t option_count)
> >>  {
> >>  	int i;
> >> +	struct team_option *dst_opts[option_count];
> >> +	int err;
> >
> >This kind of construct will trigger static analyzer alerts...
> 
> 
> I thought this is ok to do in kernel. Should I replace that with
> kmalloc/kfree?

If you know the absolute limit of option_count, you could use

struct team_option *dst_opts[OPTION_COUNT_MAX];

If not, then a kmalloc()/kfree() is probably needed ;)

Re: [patch net-next 3/3] team: replicate options on register

[Eric Dumazet]

Le jeudi 17 novembre 2011 à 11:00 +0100, Eric Dumazet a écrit :

> If you know the absolute limit of option_count, you could use
> 
> struct team_option *dst_opts[OPTION_COUNT_MAX];
> 
> If not, then a kmalloc()/kfree() is probably needed ;)
> 
> 

Sorry, forgot to include the link explaining Linus opinion on
variable-length arrays

https://lkml.org/lkml/2011/10/23/25