From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jonathan Toppins Subject: Re: [PATCH linux v2 net-next 2/4] bonding: Allow userspace to set actors' macaddr in an AD-system. Date: Fri, 08 May 2015 12:45:09 -0400 Message-ID: <554CE815.80905@cumulusnetworks.com> References: <554C7D66.20506@blackwall.org> <554CC45E.7040208@blackwall.org> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Cc: Mahesh Bandewar To: Nikolay Aleksandrov , netdev@vger.kernel.org, Jay Vosburgh , Veaceslav Falico , Andy Gospodarek , shm@cumulusnetworks.com, David Miller Return-path: Received: from mail-qc0-f182.google.com ([209.85.216.182]:34944 "EHLO mail-qc0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752930AbbEHQpM (ORCPT ); Fri, 8 May 2015 12:45:12 -0400 Received: by qcbgu10 with SMTP id gu10so39779298qcb.2 for ; Fri, 08 May 2015 09:45:12 -0700 (PDT) In-Reply-To: <554CC45E.7040208@blackwall.org> Sender: netdev-owner@vger.kernel.org List-ID: On 5/8/15 10:12 AM, Nikolay Aleksandrov wrote: > On 05/08/2015 11:09 AM, Nikolay Aleksandrov wrote: >> On 05/06/2015 10:41 PM, Jonathan Toppins wrote: >>> From: Mahesh Bandewar >>> >>> 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 >>> Reviewed-by: Nikolay Aleksandrov >>> [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 >>> --- >>> 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(-) >>> >> <<>> >>> /* 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 >> > If you did this to re-use the set function in the netlink code, you can > take a look at how arp_ip_targets is handled (same issue) and do something > similar. True arp_ip_targets does do something similar, it can use the string to represent the string of the IPv4 address and then a u32 to represent the binary version. That appears to be how it differentiates. Unless I stuff the MAC inside the u64 value I could not take advantage in the same way. If it seems acceptable to do this I can try that. > > >>> 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 { >>> >> >