netdev - Re: [PATCH V2 mlx5-next 04/10] bonding: Implement ndo_xmit_slave

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: <d01c874f-419e-36b1-5ddd-72daabcc0b83@gmail.com>
Date:   Mon, 20 Apr 2020 09:36:43 -0600
From:   David Ahern <dsahern@...il.com>
To:     Maor Gottlieb <maorg@...lanox.com>, davem@...emloft.net,
        jgg@...lanox.com, dledford@...hat.com, j.vosburgh@...il.com,
        vfalico@...il.com, andy@...yhouse.net, kuba@...nel.org
Cc:     leonro@...lanox.com, saeedm@...lanox.com, jiri@...lanox.com,
        linux-rdma@...r.kernel.org, netdev@...r.kernel.org,
        alexr@...lanox.com
Subject: Re: [PATCH V2 mlx5-next 04/10] bonding: Implement ndo_xmit_slave_get

On 4/20/20 1:54 AM, Maor Gottlieb wrote:
> Add implementation of ndo_xmit_slave_get.
> When user sets 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@...lanox.com>
> ---
>  drivers/net/bonding/bond_main.c | 123 +++++++++++++++++++++++++++-----
>  include/net/bonding.h           |   1 +
>  2 files changed, 105 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 7e04be86fda8..320bcb1394fd 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -4137,6 +4137,40 @@ static void bond_skip_slave(struct bond_up_slave *slaves,
>  	}
>  }
>  
> +static void bond_set_slave_arr(struct bonding *bond,
> +			       struct bond_up_slave *usable_slaves,
> +			       struct bond_up_slave *all_slaves)
> +{
> +	struct bond_up_slave *usable, *all;
> +
> +	usable = rtnl_dereference(bond->usable_slaves);
> +	rcu_assign_pointer(bond->usable_slaves, usable_slaves);
> +	if (usable)
> +		kfree_rcu(usable, rcu);
> +
> +	all = rtnl_dereference(bond->all_slaves);
> +	rcu_assign_pointer(bond->all_slaves, all_slaves);
> +	if (all)
> +		kfree_rcu(all, rcu);
> +}
> +
> +static void bond_reset_slave_arr(struct bonding *bond)
> +{
> +	struct bond_up_slave *usable, *all;
> +
> +	usable = rtnl_dereference(bond->usable_slaves);
> +	if (usable) {
> +		RCU_INIT_POINTER(bond->usable_slaves, NULL);
> +		kfree_rcu(usable, rcu);
> +	}
> +
> +	all = rtnl_dereference(bond->all_slaves);
> +	if (all) {
> +		RCU_INIT_POINTER(bond->all_slaves, NULL);
> +		kfree_rcu(all, rcu);
> +	}
> +}
> +
>  /* Build the usable slaves array in control path for modes that use xmit-hash
>   * to determine the slave interface -
>   * (a) BOND_MODE_8023AD
> @@ -4147,7 +4181,7 @@ 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 *usable_slaves, *old_usable_slaves;
> +	struct bond_up_slave *usable_slaves = NULL, *all_slaves = NULL;
>  	struct slave *slave;
>  	struct list_head *iter;
>  	int agg_id = 0;
> @@ -4159,7 +4193,9 @@ int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
>  
>  	usable_slaves = kzalloc(struct_size(usable_slaves, arr,
>  					    bond->slave_cnt), GFP_KERNEL);
> -	if (!usable_slaves) {
> +	all_slaves = kzalloc(struct_size(all_slaves, arr,
> +					 bond->slave_cnt), GFP_KERNEL);
> +	if (!usable_slaves || !all_slaves) {
>  		ret = -ENOMEM;
>  		goto out;
>  	}
> @@ -4168,20 +4204,19 @@ int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
>  
>  		if (bond_3ad_get_active_agg_info(bond, &ad_info)) {
>  			pr_debug("bond_3ad_get_active_agg_info failed\n");
> -			kfree_rcu(usable_slaves, rcu);
>  			/* No active aggragator means it's not safe to use
>  			 * the previous array.
>  			 */
> -			old_usable_slaves = rtnl_dereference(bond->usable_slaves);
> -			if (old_usable_slaves) {
> -				RCU_INIT_POINTER(bond->usable_slaves, NULL);
> -				kfree_rcu(old_usable_slaves, rcu);
> -			}
> +			bond_reset_slave_arr(bond);
>  			goto out;
>  		}
>  		agg_id = ad_info.aggregator_id;
>  	}
>  	bond_for_each_slave(bond, slave, iter) {
> +		if (skipslave == slave)
> +			continue;
> +
> +		all_slaves->arr[all_slaves->count++] = slave;
>  		if (BOND_MODE(bond) == BOND_MODE_8023AD) {
>  			struct aggregator *agg;
>  
> @@ -4191,8 +4226,6 @@ int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
>  		}
>  		if (!bond_slave_can_tx(slave))
>  			continue;
> -		if (skipslave == slave)
> -			continue;
>  
>  		slave_dbg(bond->dev, slave->dev, "Adding slave to tx hash array[%d]\n",
>  			  usable_slaves->count);
> @@ -4200,14 +4233,17 @@ int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
>  		usable_slaves->arr[usable_slaves->count++] = slave;
>  	}
>  
> -	old_usable_slaves = rtnl_dereference(bond->usable_slaves);
> -	rcu_assign_pointer(bond->usable_slaves, usable_slaves);
> -	if (old_usable_slaves)
> -		kfree_rcu(old_usable_slaves, rcu);
> +	bond_set_slave_arr(bond, usable_slaves, all_slaves);
> +	return ret;
>  out:
> -	if (ret != 0 && skipslave)
> +	if (ret != 0 && skipslave) {
> +		bond_skip_slave(rtnl_dereference(bond->all_slaves),
> +				skipslave);
>  		bond_skip_slave(rtnl_dereference(bond->usable_slaves),
>  				skipslave);
> +	}
> +	kfree_rcu(all_slaves, rcu);
> +	kfree_rcu(usable_slaves, rcu);
>  
>  	return ret;
>  }

none of the above code has anything to do directly with looking up the
bond slave in bond_xmit_get_slave; all of that and the bond_uninit
changes should be done in refactoring patch(es) that prepare existing
code to be called from the new ndo.