Re: [RFC PATCH 4/4] bonding: Implement ndo_xmit_slave_get

[Jay Vosburgh <jay.vosburgh@canonical.com>]

Maor Gottlieb <maorg@mellanox.com> wrote:

>Add implementation of ndo_xmit_slave_get.
>When user set the LAG_FLAGS_HASH_ALL_SLAVES bit and the xmit slave
>result is based on the hash, then the slave will be selected from the
>array of all the slaves.
>
>Signed-off-by: Maor Gottlieb <maorg@mellanox.com>
>---
> drivers/net/bonding/bond_main.c | 63 ++++++++++++++++++++++++++++++---
> include/net/bonding.h           |  1 +
> 2 files changed, 60 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index adab1e3549ff..c8f440d1b624 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -4098,7 +4098,8 @@ static void bond_skip_slave(struct bond_up_slave *slaves,
>  */
> int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
> {
>-	struct bond_up_slave *active_slaves, *old_active_slaves;
>+	struct bond_up_slave *active_slaves = NULL, *all_slaves = NULL;
>+	struct bond_up_slave *old_active_slaves, *old_all_slaves;
> 	struct slave *slave;
> 	struct list_head *iter;
> 	int agg_id = 0;
>@@ -4110,7 +4111,9 @@ int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
> 
> 	active_slaves = kzalloc(struct_size(active_slaves, arr,
> 					    bond->slave_cnt), GFP_KERNEL);
>-	if (!active_slaves) {
>+	all_slaves = kzalloc(struct_size(all_slaves, arr,
>+					 bond->slave_cnt), GFP_KERNEL);
>+	if (!active_slaves || !all_slaves) {
> 		ret = -ENOMEM;
> 		pr_err("Failed to build slave-array.\n");
> 		goto out;
>@@ -4141,14 +4144,17 @@ int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
> 			if (!agg || agg->aggregator_identifier != agg_id)
> 				continue;
> 		}
>-		if (!bond_slave_can_tx(slave))
>+		if (!bond_slave_can_tx(slave)) {
>+			all_slaves->arr[all_slaves->count++] = slave;
> 			continue;
>+		}
> 		if (skipslave == slave)
> 			continue;
> 
> 		slave_dbg(bond->dev, slave->dev, "Adding slave to tx hash array[%d]\n",
> 			  active_slaves->count);
> 
>+		all_slaves->arr[all_slaves->count++] = slave;
> 		active_slaves->arr[active_slaves->count++] = slave;
> 	}
> 
>@@ -4156,10 +4162,18 @@ int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
> 	rcu_assign_pointer(bond->active_slaves, active_slaves);
> 	if (old_active_slaves)
> 		kfree_rcu(old_active_slaves, rcu);
>+
>+	old_all_slaves = rtnl_dereference(bond->all_slaves);
>+	rcu_assign_pointer(bond->all_slaves, all_slaves);
>+	if (old_all_slaves)
>+		kfree_rcu(old_all_slaves, rcu);
> out:
>-	if (ret != 0 && skipslave)
>+	if (ret != 0 && skipslave) {
> 		bond_skip_slave(rtnl_dereference(bond->active_slaves),
> 				skipslave);
>+		kfree(all_slaves);
>+		kfree(active_slaves);
>+	}

	I'm still going through the patch set, but noticed this right
away: the above will leak memory if !skipslave and the allocation for
active_slaves succeeds, but the allocation for all_slaves fails.
> 
> 	return ret;
> }
>@@ -4265,6 +4279,46 @@ static u16 bond_select_queue(struct net_device *dev, struct sk_buff *skb,
> 	return txq;
> }
> 
>+static struct net_device *bond_xmit_slave_get(struct net_device *master_dev,
>+					      struct sk_buff *skb,
>+					      int flags)
>+{
>+	struct bonding *bond = netdev_priv(master_dev);
>+	struct bond_up_slave *slaves;
>+	struct slave *slave;
>+
>+	switch (BOND_MODE(bond)) {
>+	case BOND_MODE_ROUNDROBIN:
>+		slave = bond_xmit_roundrobin_slave_get(bond, skb);
>+		break;
>+	case BOND_MODE_ACTIVEBACKUP:
>+		slave = bond_xmit_activebackup_slave_get(bond, skb);
>+		break;
>+	case BOND_MODE_8023AD:
>+	case BOND_MODE_XOR:
>+		if (flags & LAG_FLAGS_HASH_ALL_SLAVES)
>+			slaves = rcu_dereference(bond->all_slaves);
>+		else
>+			slaves = rcu_dereference(bond->active_slaves);
>+		slave = bond_xmit_3ad_xor_slave_get(bond, skb, slaves);
>+		break;
>+	case BOND_MODE_BROADCAST:
>+		return ERR_PTR(-EOPNOTSUPP);
>+	case BOND_MODE_ALB:
>+		slave = bond_xmit_alb_slave_get(bond, skb);
>+		break;
>+	case BOND_MODE_TLB:
>+		slave = bond_xmit_tlb_slave_get(bond, skb);
>+		break;
>+	default:
>+		return NULL;

	I would argue this should (a) return an error (not NULL), and,
(b) ideally issue a netdev_err for this impossible situation, similar to
the other switch statements in bonding.

	-J
	
>+	}
>+
>+	if (slave)
>+		return slave->dev;
>+	return NULL;
>+}
>+
> static netdev_tx_t __bond_start_xmit(struct sk_buff *skb, struct net_device *dev)
> {
> 	struct bonding *bond = netdev_priv(dev);
>@@ -4387,6 +4441,7 @@ static const struct net_device_ops bond_netdev_ops = {
> 	.ndo_del_slave		= bond_release,
> 	.ndo_fix_features	= bond_fix_features,
> 	.ndo_features_check	= passthru_features_check,
>+	.ndo_xmit_slave_get	= bond_xmit_slave_get,
> };
> 
> static const struct device_type bond_type = {
>diff --git a/include/net/bonding.h b/include/net/bonding.h
>index b77daffc1b52..6dd970eb9d3f 100644
>--- a/include/net/bonding.h
>+++ b/include/net/bonding.h
>@@ -201,6 +201,7 @@ struct bonding {
> 	struct   slave __rcu *current_arp_slave;
> 	struct   slave __rcu *primary_slave;
> 	struct   bond_up_slave __rcu *active_slaves; /* Array of usable slaves */
>+	struct   bond_up_slave __rcu *all_slaves; /* Array of all slaves */
> 	bool     force_primary;
> 	s32      slave_cnt; /* never change this value outside the attach/detach wrappers */
> 	int     (*recv_probe)(const struct sk_buff *, struct bonding *,
>-- 
>2.17.2
>

---
	-Jay Vosburgh, jay.vosburgh@canonical.com