Re: [PATCH linux v2 net-next 2/4] bonding: Allow userspace to set actors' macaddr in an AD-system.

[Nikolay Aleksandrov <razor@blackwall.org>]

On 05/06/2015 10:41 PM, Jonathan Toppins wrote:
> From: Mahesh Bandewar <maheshb@google.com>
> 
> In an AD system, the communication between actor and partner is the
> business between these two entities. In the current setup anyone on the
> same L2 can "guess" the LACPDU contents and then possibly send the
> spoofed LACPDUs and trick the partner causing connectivity issues for
> the AD system. This patch allows to use a random mac-address obscuring
> it's identity making it harder for someone in the L2 is do the same thing.
> 
> This patch allows user-space to choose the mac-address for the AD-system.
> This mac-address can not be NULL or a Multicast. If the mac-address is set
> from user-space; kernel will honor it and will not overwrite it. In the
> absence (value from user space); the logic will default to using the
> masters' mac as the mac-address for the AD-system.
> 
> It can be set using example code below -
> 
>    # modprobe bonding mode=4
>    # sys_mac_addr=$(printf '%02x:%02x:%02x:%02x:%02x:%02x' \
>                     $(( (RANDOM & 0xFE) | 0x02 )) \
>                     $(( RANDOM & 0xFF )) \
>                     $(( RANDOM & 0xFF )) \
>                     $(( RANDOM & 0xFF )) \
>                     $(( RANDOM & 0xFF )) \
>                     $(( RANDOM & 0xFF )))
>    # echo $sys_mac_addr > /sys/class/net/bond0/bonding/ad_actor_system
>    # echo +eth1 > /sys/class/net/bond0/bonding/slaves
>    ...
>    # ip link set bond0 up
> 
> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
> Reviewed-by: Nikolay Aleksandrov <nikolay@redhat.com>
> [jt: fixed up style issues reported by checkpatch, also changed
>   bond_option_ad_actor_system_set to assume a binary mac so it can
>   be reused in the netlink option set case]
> Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
> ---
> v2:
>   * rebased
> 
>  Documentation/networking/bonding.txt |   12 +++++++++++
>  drivers/net/bonding/bond_3ad.c       |    7 +++++-
>  drivers/net/bonding/bond_main.c      |    1 +
>  drivers/net/bonding/bond_options.c   |   21 ++++++++++++++++++
>  drivers/net/bonding/bond_procfs.c    |    6 ++++++
>  drivers/net/bonding/bond_sysfs.c     |   39 ++++++++++++++++++++++++++++++++++
>  include/net/bond_options.h           |    1 +
>  include/net/bonding.h                |    1 +
>  8 files changed, 87 insertions(+), 1 deletion(-)
> 
<<<snip>>>
>  /* Searches for an option by name */
> @@ -1375,3 +1384,15 @@ static int bond_option_ad_actor_sys_prio_set(struct bonding *bond,
>  	bond->params.ad_actor_sys_prio = newval->value;
>  	return 0;
>  }
> +
> +static int bond_option_ad_actor_system_set(struct bonding *bond,
> +					   const struct bond_opt_value *newval)
> +{
> +	if (!is_valid_ether_addr(newval->string)) {
> +		netdev_err(bond->dev, "Invalid MAC address.\n");
> +		return -EINVAL;
> +	}
> +
> +	ether_addr_copy(bond->params.ad_actor_system, newval->string);
> +	return 0;
> +}
> diff --git a/drivers/net/bonding/bond_procfs.c b/drivers/net/bonding/bond_procfs.c
> index 1136929..e7f3047 100644
> --- a/drivers/net/bonding/bond_procfs.c
> +++ b/drivers/net/bonding/bond_procfs.c
> @@ -137,6 +137,8 @@ static void bond_info_show_master(struct seq_file *seq)
>  			   optval->string);
>  		seq_printf(seq, "System priority: %d\n",
>  			   BOND_AD_INFO(bond).system.sys_priority);
> +		seq_printf(seq, "System MAC address: %pM\n",
> +			   &BOND_AD_INFO(bond).system.sys_mac_addr);
>  
>  		if (__bond_3ad_get_active_agg_info(bond, &ad_info)) {
>  			seq_printf(seq, "bond %s has no active aggregator\n",
> @@ -200,6 +202,8 @@ static void bond_info_show_slave(struct seq_file *seq,
>  			seq_puts(seq, "details actor lacp pdu:\n");
>  			seq_printf(seq, "    system priority: %d\n",
>  				   port->actor_system_priority);
> +			seq_printf(seq, "    system mac address: %pM\n",
> +				   &port->actor_system);
>  			seq_printf(seq, "    port key: %d\n",
>  				   port->actor_oper_port_key);
>  			seq_printf(seq, "    port priority: %d\n",
> @@ -212,6 +216,8 @@ static void bond_info_show_slave(struct seq_file *seq,
>  			seq_puts(seq, "details partner lacp pdu:\n");
>  			seq_printf(seq, "    system priority: %d\n",
>  				   port->partner_oper.system_priority);
> +			seq_printf(seq, "    system mac address: %pM\n",
> +				   &port->partner_oper.system);
>  			seq_printf(seq, "    oper key: %d\n",
>  				   port->partner_oper.key);
>  			seq_printf(seq, "    port priority: %d\n",
> diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
> index 4a76266..5e4c2ea 100644
> --- a/drivers/net/bonding/bond_sysfs.c
> +++ b/drivers/net/bonding/bond_sysfs.c
> @@ -706,6 +706,44 @@ static ssize_t bonding_show_ad_actor_sys_prio(struct device *d,
>  static DEVICE_ATTR(ad_actor_sys_prio, S_IRUGO | S_IWUSR,
>  		   bonding_show_ad_actor_sys_prio, bonding_sysfs_store_option);
>  
> +static ssize_t bonding_show_ad_actor_system(struct device *d,
> +					    struct device_attribute *attr,
> +					    char *buf)
> +{
> +	struct bonding *bond = to_bond(d);
> +
> +	if (BOND_MODE(bond) == BOND_MODE_8023AD)
> +		return sprintf(buf, "%pM\n", bond->params.ad_actor_system);
> +
> +	return 0;
> +}
> +
> +static ssize_t bonding_store_ad_actor_system(struct device *d,
> +					     struct device_attribute *attr,
> +					     const char *buffer, size_t count)
> +{
> +	struct bonding *bond = to_bond(d);
> +	u8 macaddr[ETH_ALEN];
> +	int ret;
> +
> +	ret = sscanf(buffer, "%hhx:%hhx:%hhx:%hhx:%hhx:%hhx",
> +		     &macaddr[0], &macaddr[1], &macaddr[2],
> +		     &macaddr[3], &macaddr[4], &macaddr[5]);
> +	if (ret != ETH_ALEN) {
> +		netdev_err(bond->dev, "Invalid MAC address.\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = bond_opt_tryset_rtnl(bond, BOND_OPT_AD_ACTOR_SYSTEM, macaddr);
> +	if (!ret)
> +		ret = count;
> +
> +	return ret;
> +}
> +
> +static DEVICE_ATTR(ad_actor_system, S_IRUGO | S_IWUSR,
> +		   bonding_show_ad_actor_system, bonding_store_ad_actor_system);
> +
Hi,
I must've missed this part the first time around. Could you please explain
why can't you do all the checks from the set function and you need a
special sysfs set one for this option here ?
The generic bonding sysfs set function was introduced in order to remove
these and make use of the new option API, and this looks like a step backwards.

Nik

>  static struct attribute *per_bond_attrs[] = {
>  	&dev_attr_slaves.attr,
>  	&dev_attr_mode.attr,
> @@ -740,6 +778,7 @@ static struct attribute *per_bond_attrs[] = {
>  	&dev_attr_packets_per_slave.attr,
>  	&dev_attr_tlb_dynamic_lb.attr,
>  	&dev_attr_ad_actor_sys_prio.attr,
> +	&dev_attr_ad_actor_system.attr,
>  	NULL,
>  };
>  
> diff --git a/include/net/bond_options.h b/include/net/bond_options.h
> index 894002a..eeeefa1 100644
> --- a/include/net/bond_options.h
> +++ b/include/net/bond_options.h
> @@ -64,6 +64,7 @@ enum {
>  	BOND_OPT_SLAVES,
>  	BOND_OPT_TLB_DYNAMIC_LB,
>  	BOND_OPT_AD_ACTOR_SYS_PRIO,
> +	BOND_OPT_AD_ACTOR_SYSTEM,
>  	BOND_OPT_LAST
>  };
>  
> diff --git a/include/net/bonding.h b/include/net/bonding.h
> index 405cf87..650f386 100644
> --- a/include/net/bonding.h
> +++ b/include/net/bonding.h
> @@ -137,6 +137,7 @@ struct bond_params {
>  	int tlb_dynamic_lb;
>  	struct reciprocal_value reciprocal_packets_per_slave;
>  	u16 ad_actor_sys_prio;
> +	u8 ad_actor_system[ETH_ALEN];
>  };
>  
>  struct bond_parm_tbl {
>