From: Jonathan Toppins <jtoppins@cumulusnetworks•com>
To: Nikolay Aleksandrov <razor@blackwall•org>,
netdev@vger•kernel.org, Jay Vosburgh <j.vosburgh@gmail•com>,
Veaceslav Falico <vfalico@gmail•com>,
Andy Gospodarek <gospo@cumulusnetworks•com>,
shm@cumulusnetworks•com, David Miller <davem@davemloft•net>
Cc: Mahesh Bandewar <maheshb@google•com>
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 [thread overview]
Message-ID: <554CE815.80905@cumulusnetworks.com> (raw)
In-Reply-To: <554CC45E.7040208@blackwall.org>
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 <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
>>
> 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 {
>>>
>>
>
next prev parent reply other threads:[~2015-05-08 16:45 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-05-06 20:41 [PATCH linux v2 net-next 0/5] add netlink support for new lacp bonding parameters Jonathan Toppins
[not found] ` <cover.1430944053.git.jtoppins@cumulusnetworks.com>
2015-05-06 20:41 ` [PATCH linux v2 net-next 1/4] bonding: Allow userspace to set actors' system_priority in AD system Jonathan Toppins
2015-05-06 20:41 ` [PATCH linux v2 net-next 2/4] bonding: Allow userspace to set actors' macaddr in an AD-system Jonathan Toppins
2015-05-08 9:09 ` Nikolay Aleksandrov
2015-05-08 14:12 ` Nikolay Aleksandrov
2015-05-08 16:45 ` Jonathan Toppins [this message]
2015-05-08 17:03 ` Nikolay Aleksandrov
2015-05-08 17:14 ` Jonathan Toppins
2015-05-06 20:41 ` [PATCH linux v2 net-next 3/4] bonding: Implement user key part of port_key in an AD system Jonathan Toppins
2015-05-06 20:41 ` [PATCH linux v2 net-next 4/4] bonding: add netlink support for sys prio, actor sys mac, and port key Jonathan Toppins
2015-05-07 22:56 ` Mahesh Bandewar
2015-05-06 20:41 ` [PATCH iproute2 v2 net-next] iplink_bond: add support for ad_actor and port_key options Jonathan Toppins
2015-05-07 22:58 ` Mahesh Bandewar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=554CE815.80905@cumulusnetworks.com \
--to=jtoppins@cumulusnetworks$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=gospo@cumulusnetworks$(echo .)com \
--cc=j.vosburgh@gmail$(echo .)com \
--cc=maheshb@google$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=razor@blackwall$(echo .)org \
--cc=shm@cumulusnetworks$(echo .)com \
--cc=vfalico@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox