* [PATCH linux v2 net-next 1/4] bonding: Allow userspace to set actors' system_priority in AD system
[not found] ` <cover.1430944053.git.jtoppins@cumulusnetworks.com>
@ 2015-05-06 20:41 ` 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
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: Jonathan Toppins @ 2015-05-06 20:41 UTC (permalink / raw)
To: netdev, Jay Vosburgh, Veaceslav Falico, Andy Gospodarek, shm,
David Miller
Cc: Mahesh Bandewar
From: Mahesh Bandewar <maheshb@google.com>
This patch allows user to randomize the system-priority in an ad-system.
The allowed range is 1 - 0xFFFF while default value is 0xFFFF. If user
does not specify this value, the system defaults to 0xFFFF, which is
what it was before this patch.
Following example code could set the value -
# modprobe bonding mode=4
# sys_prio=$(( 1 + RANDOM + RANDOM ))
# echo $sys_prio > /sys/class/net/bond0/bonding/ad_actor_sys_prio
# 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
* changed how the default value is set in bond_check_params(), this
makes the default consistent between what gets set for a new bond
and what the default is claimed to be in the bonding options.]
Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
---
v2:
* rebased
* aligned the default value to be the same for new bonds and what is
specified in bonding options table as the default value for the
parameter. Previously the "default" for new bonds was zero which is
not the default specified in the options table. Noted fixup in my
changelog notes above.
Documentation/networking/bonding.txt | 9 +++++++++
drivers/net/bonding/bond_3ad.c | 5 ++++-
drivers/net/bonding/bond_main.c | 12 ++++++++++++
drivers/net/bonding/bond_options.c | 28 +++++++++++++++++++++++++++-
drivers/net/bonding/bond_procfs.c | 2 ++
drivers/net/bonding/bond_sysfs.c | 15 +++++++++++++++
include/net/bond_options.h | 1 +
include/net/bonding.h | 1 +
8 files changed, 71 insertions(+), 2 deletions(-)
diff --git a/Documentation/networking/bonding.txt b/Documentation/networking/bonding.txt
index 83bf498..3494611 100644
--- a/Documentation/networking/bonding.txt
+++ b/Documentation/networking/bonding.txt
@@ -178,6 +178,15 @@ active_slave
active slave, or the empty string if there is no active slave or
the current mode does not use an active slave.
+ad_actor_sys_prio
+
+ In an AD system, this specifies the system priority. The allowed range
+ is 1 - 65535. If the value is not specified, it takes 65535 as the
+ default value.
+
+ This parameter has effect only in 802.3ad mode and is available through
+ SysFs interface.
+
ad_select
Specifies the 802.3ad aggregation selection logic to use. The
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index fbd54f0..4c003bc 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -1908,7 +1908,8 @@ void bond_3ad_initialize(struct bonding *bond, u16 tick_resolution)
BOND_AD_INFO(bond).aggregator_identifier = 0;
- BOND_AD_INFO(bond).system.sys_priority = 0xFFFF;
+ BOND_AD_INFO(bond).system.sys_priority =
+ bond->params.ad_actor_sys_prio;
BOND_AD_INFO(bond).system.sys_mac_addr = *((struct mac_addr *)bond->dev->dev_addr);
/* initialize how many times this module is called in one
@@ -1959,6 +1960,8 @@ void bond_3ad_bind_slave(struct slave *slave)
port->sm_vars &= ~AD_PORT_LACP_ENABLED;
/* actor system is the bond's system */
port->actor_system = BOND_AD_INFO(bond).system.sys_mac_addr;
+ port->actor_system_priority =
+ BOND_AD_INFO(bond).system.sys_priority;
/* tx timer(to verify that no more than MAX_TX_IN_SECOND
* lacpdu's are sent in one second)
*/
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index d5fe5d5..5f2f28f 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4140,6 +4140,7 @@ static int bond_check_params(struct bond_params *params)
struct bond_opt_value newval;
const struct bond_opt_value *valptr;
int arp_all_targets_value;
+ u16 ad_actor_sys_prio = 0;
/* Convert string parameters. */
if (mode) {
@@ -4434,6 +4435,16 @@ static int bond_check_params(struct bond_params *params)
fail_over_mac_value = BOND_FOM_NONE;
}
+ bond_opt_initstr(&newval, "default");
+ valptr = bond_opt_parse(
+ bond_opt_get(BOND_OPT_AD_ACTOR_SYS_PRIO),
+ &newval);
+ if (!valptr) {
+ pr_err("Error: No ad_actor_sys_prio default value");
+ return -EINVAL;
+ }
+ ad_actor_sys_prio = valptr->value;
+
if (lp_interval == 0) {
pr_warn("Warning: ip_interval must be between 1 and %d, so it was reset to %d\n",
INT_MAX, BOND_ALB_DEFAULT_LP_INTERVAL);
@@ -4462,6 +4473,7 @@ static int bond_check_params(struct bond_params *params)
params->lp_interval = lp_interval;
params->packets_per_slave = packets_per_slave;
params->tlb_dynamic_lb = 1; /* Default value */
+ params->ad_actor_sys_prio = ad_actor_sys_prio;
if (packets_per_slave > 0) {
params->reciprocal_packets_per_slave =
reciprocal_value(packets_per_slave);
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 4df2894..d2b47e5 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -70,6 +70,8 @@ static int bond_option_slaves_set(struct bonding *bond,
const struct bond_opt_value *newval);
static int bond_option_tlb_dynamic_lb_set(struct bonding *bond,
const struct bond_opt_value *newval);
+static int bond_option_ad_actor_sys_prio_set(struct bonding *bond,
+ const struct bond_opt_value *newval);
static const struct bond_opt_value bond_mode_tbl[] = {
@@ -186,6 +188,12 @@ static const struct bond_opt_value bond_tlb_dynamic_lb_tbl[] = {
{ NULL, -1, 0}
};
+static const struct bond_opt_value bond_ad_actor_sys_prio_tbl[] = {
+ { "minval", 1, BOND_VALFLAG_MIN},
+ { "maxval", 65535, BOND_VALFLAG_MAX | BOND_VALFLAG_DEFAULT},
+ { NULL, -1, 0},
+};
+
static const struct bond_option bond_opts[BOND_OPT_LAST] = {
[BOND_OPT_MODE] = {
.id = BOND_OPT_MODE,
@@ -379,7 +387,15 @@ static const struct bond_option bond_opts[BOND_OPT_LAST] = {
.values = bond_tlb_dynamic_lb_tbl,
.flags = BOND_OPTFLAG_IFDOWN,
.set = bond_option_tlb_dynamic_lb_set,
- }
+ },
+ [BOND_OPT_AD_ACTOR_SYS_PRIO] = {
+ .id = BOND_OPT_AD_ACTOR_SYS_PRIO,
+ .name = "ad_actor_sys_prio",
+ .unsuppmodes = BOND_MODE_ALL_EX(BIT(BOND_MODE_8023AD)),
+ .flags = BOND_OPTFLAG_IFDOWN,
+ .values = bond_ad_actor_sys_prio_tbl,
+ .set = bond_option_ad_actor_sys_prio_set,
+ },
};
/* Searches for an option by name */
@@ -1349,3 +1365,13 @@ static int bond_option_tlb_dynamic_lb_set(struct bonding *bond,
return 0;
}
+
+static int bond_option_ad_actor_sys_prio_set(struct bonding *bond,
+ const struct bond_opt_value *newval)
+{
+ netdev_info(bond->dev, "Setting ad_actor_sys_prio to (%llu)\n",
+ newval->value);
+
+ bond->params.ad_actor_sys_prio = newval->value;
+ return 0;
+}
diff --git a/drivers/net/bonding/bond_procfs.c b/drivers/net/bonding/bond_procfs.c
index b20b35ac..1136929 100644
--- a/drivers/net/bonding/bond_procfs.c
+++ b/drivers/net/bonding/bond_procfs.c
@@ -135,6 +135,8 @@ static void bond_info_show_master(struct seq_file *seq)
bond->params.ad_select);
seq_printf(seq, "Aggregator selection policy (ad_select): %s\n",
optval->string);
+ seq_printf(seq, "System priority: %d\n",
+ BOND_AD_INFO(bond).system.sys_priority);
if (__bond_3ad_get_active_agg_info(bond, &ad_info)) {
seq_printf(seq, "bond %s has no active aggregator\n",
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index 7e9e151..4a76266 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -692,6 +692,20 @@ static ssize_t bonding_show_packets_per_slave(struct device *d,
static DEVICE_ATTR(packets_per_slave, S_IRUGO | S_IWUSR,
bonding_show_packets_per_slave, bonding_sysfs_store_option);
+static ssize_t bonding_show_ad_actor_sys_prio(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, "%hu\n", bond->params.ad_actor_sys_prio);
+
+ return 0;
+}
+static DEVICE_ATTR(ad_actor_sys_prio, S_IRUGO | S_IWUSR,
+ bonding_show_ad_actor_sys_prio, bonding_sysfs_store_option);
+
static struct attribute *per_bond_attrs[] = {
&dev_attr_slaves.attr,
&dev_attr_mode.attr,
@@ -725,6 +739,7 @@ static struct attribute *per_bond_attrs[] = {
&dev_attr_lp_interval.attr,
&dev_attr_packets_per_slave.attr,
&dev_attr_tlb_dynamic_lb.attr,
+ &dev_attr_ad_actor_sys_prio.attr,
NULL,
};
diff --git a/include/net/bond_options.h b/include/net/bond_options.h
index ea6546d..894002a 100644
--- a/include/net/bond_options.h
+++ b/include/net/bond_options.h
@@ -63,6 +63,7 @@ enum {
BOND_OPT_LP_INTERVAL,
BOND_OPT_SLAVES,
BOND_OPT_TLB_DYNAMIC_LB,
+ BOND_OPT_AD_ACTOR_SYS_PRIO,
BOND_OPT_LAST
};
diff --git a/include/net/bonding.h b/include/net/bonding.h
index 78ed135..405cf87 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -136,6 +136,7 @@ struct bond_params {
int packets_per_slave;
int tlb_dynamic_lb;
struct reciprocal_value reciprocal_packets_per_slave;
+ u16 ad_actor_sys_prio;
};
struct bond_parm_tbl {
--
1.7.10.4
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH linux v2 net-next 2/4] bonding: Allow userspace to set actors' macaddr in an AD-system.
[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 ` Jonathan Toppins
2015-05-08 9:09 ` Nikolay Aleksandrov
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
3 siblings, 1 reply; 13+ messages in thread
From: Jonathan Toppins @ 2015-05-06 20:41 UTC (permalink / raw)
To: netdev, Jay Vosburgh, Veaceslav Falico, Andy Gospodarek, shm,
David Miller
Cc: Mahesh Bandewar
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(-)
diff --git a/Documentation/networking/bonding.txt b/Documentation/networking/bonding.txt
index 3494611..2c197b6 100644
--- a/Documentation/networking/bonding.txt
+++ b/Documentation/networking/bonding.txt
@@ -187,6 +187,18 @@ ad_actor_sys_prio
This parameter has effect only in 802.3ad mode and is available through
SysFs interface.
+ad_actor_system
+
+ In an AD system, this specifies the mac-address for the actor in
+ protocol packet exchanges (LACPDUs). The value cannot be NULL or
+ multicast. It is preferred to have the local-admin bit set for this
+ mac but driver does not enforce it. If the value is not given then
+ system defaults to using the masters' mac address as actors' system
+ address.
+
+ This parameter has effect only in 802.3ad mode and is available through
+ SysFs interface.
+
ad_select
Specifies the 802.3ad aggregation selection logic to use. The
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 4c003bc..012f7bc 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -1910,7 +1910,12 @@ void bond_3ad_initialize(struct bonding *bond, u16 tick_resolution)
BOND_AD_INFO(bond).system.sys_priority =
bond->params.ad_actor_sys_prio;
- BOND_AD_INFO(bond).system.sys_mac_addr = *((struct mac_addr *)bond->dev->dev_addr);
+ if (is_zero_ether_addr(bond->params.ad_actor_system))
+ BOND_AD_INFO(bond).system.sys_mac_addr =
+ *((struct mac_addr *)bond->dev->dev_addr);
+ else
+ BOND_AD_INFO(bond).system.sys_mac_addr =
+ *((struct mac_addr *)bond->params.ad_actor_system);
/* initialize how many times this module is called in one
* second (should be about every 100ms)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 5f2f28f..a4e2f27 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4474,6 +4474,7 @@ static int bond_check_params(struct bond_params *params)
params->packets_per_slave = packets_per_slave;
params->tlb_dynamic_lb = 1; /* Default value */
params->ad_actor_sys_prio = ad_actor_sys_prio;
+ eth_zero_addr(params->ad_actor_system);
if (packets_per_slave > 0) {
params->reciprocal_packets_per_slave =
reciprocal_value(packets_per_slave);
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index d2b47e5..978a46a 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -72,6 +72,8 @@ static int bond_option_tlb_dynamic_lb_set(struct bonding *bond,
const struct bond_opt_value *newval);
static int bond_option_ad_actor_sys_prio_set(struct bonding *bond,
const struct bond_opt_value *newval);
+static int bond_option_ad_actor_system_set(struct bonding *bond,
+ const struct bond_opt_value *newval);
static const struct bond_opt_value bond_mode_tbl[] = {
@@ -396,6 +398,13 @@ static const struct bond_option bond_opts[BOND_OPT_LAST] = {
.values = bond_ad_actor_sys_prio_tbl,
.set = bond_option_ad_actor_sys_prio_set,
},
+ [BOND_OPT_AD_ACTOR_SYSTEM] = {
+ .id = BOND_OPT_AD_ACTOR_SYSTEM,
+ .name = "ad_actor_system",
+ .unsuppmodes = BOND_MODE_ALL_EX(BIT(BOND_MODE_8023AD)),
+ .flags = BOND_OPTFLAG_RAWVAL | BOND_OPTFLAG_IFDOWN,
+ .set = bond_option_ad_actor_system_set,
+ },
};
/* 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);
+
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 {
--
1.7.10.4
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH linux v2 net-next 2/4] bonding: Allow userspace to set actors' macaddr in an AD-system.
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
0 siblings, 1 reply; 13+ messages in thread
From: Nikolay Aleksandrov @ 2015-05-08 9:09 UTC (permalink / raw)
To: Jonathan Toppins, netdev, Jay Vosburgh, Veaceslav Falico,
Andy Gospodarek, shm, David Miller
Cc: Mahesh Bandewar
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 {
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH linux v2 net-next 2/4] bonding: Allow userspace to set actors' macaddr in an AD-system.
2015-05-08 9:09 ` Nikolay Aleksandrov
@ 2015-05-08 14:12 ` Nikolay Aleksandrov
2015-05-08 16:45 ` Jonathan Toppins
0 siblings, 1 reply; 13+ messages in thread
From: Nikolay Aleksandrov @ 2015-05-08 14:12 UTC (permalink / raw)
To: Jonathan Toppins, netdev, Jay Vosburgh, Veaceslav Falico,
Andy Gospodarek, shm, David Miller
Cc: Mahesh Bandewar
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.
>> 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 {
>>
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH linux v2 net-next 2/4] bonding: Allow userspace to set actors' macaddr in an AD-system.
2015-05-08 14:12 ` Nikolay Aleksandrov
@ 2015-05-08 16:45 ` Jonathan Toppins
2015-05-08 17:03 ` Nikolay Aleksandrov
0 siblings, 1 reply; 13+ messages in thread
From: Jonathan Toppins @ 2015-05-08 16:45 UTC (permalink / raw)
To: Nikolay Aleksandrov, netdev, Jay Vosburgh, Veaceslav Falico,
Andy Gospodarek, shm, David Miller
Cc: Mahesh Bandewar
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 {
>>>
>>
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH linux v2 net-next 2/4] bonding: Allow userspace to set actors' macaddr in an AD-system.
2015-05-08 16:45 ` Jonathan Toppins
@ 2015-05-08 17:03 ` Nikolay Aleksandrov
2015-05-08 17:14 ` Jonathan Toppins
0 siblings, 1 reply; 13+ messages in thread
From: Nikolay Aleksandrov @ 2015-05-08 17:03 UTC (permalink / raw)
To: Jonathan Toppins, netdev, Jay Vosburgh, Veaceslav Falico,
Andy Gospodarek, shm, David Miller
Cc: Mahesh Bandewar
On 05/08/2015 06:45 PM, Jonathan Toppins wrote:
> 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.
>
I realize it won't be pretty, but this is currently the only option that
needs such workaround. I think we can later change the value storage to be
a union so it will be easier to use as needed.
It'd be nice to have some more opinions on this, but the general direction
has been (and still is afaik) to remove the per-option sysfs functions and
to reduce code duplication, for reference see commit dc3e5d18f2a2
("bonding: make a generic sysfs option store and fix comments").
So I think the extra-work is worth it.
Cheers,
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 {
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH linux v2 net-next 2/4] bonding: Allow userspace to set actors' macaddr in an AD-system.
2015-05-08 17:03 ` Nikolay Aleksandrov
@ 2015-05-08 17:14 ` Jonathan Toppins
0 siblings, 0 replies; 13+ messages in thread
From: Jonathan Toppins @ 2015-05-08 17:14 UTC (permalink / raw)
To: Nikolay Aleksandrov, netdev, Jay Vosburgh, Veaceslav Falico,
Andy Gospodarek, shm, David Miller
Cc: Mahesh Bandewar
On 5/8/15 1:03 PM, Nikolay Aleksandrov wrote:
> On 05/08/2015 06:45 PM, Jonathan Toppins wrote:
>> 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.
>>
> I realize it won't be pretty, but this is currently the only option that
> needs such workaround. I think we can later change the value storage to be
> a union so it will be easier to use as needed.
> It'd be nice to have some more opinions on this, but the general direction
> has been (and still is afaik) to remove the per-option sysfs functions and
> to reduce code duplication, for reference see commit dc3e5d18f2a2
> ("bonding: make a generic sysfs option store and fix comments").
> So I think the extra-work is worth it.
Thanks for the input. Will work on changing it to stuff the binary
version of the MAC into the u64 and move back the scanf call into the
option specific set. Agree on the general principle of increasing code
reuse.
Maybe changing bond_opt_value to something like:
struct bond_opt_value {
void *data;
int dlen;
int type;
};
Obviously with some unions thrown in there so we don't have to rewrite
every set function.
>
> Cheers,
> 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 {
>>>>>
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH linux v2 net-next 3/4] bonding: Implement user key part of port_key in an AD system.
[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-06 20:41 ` 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
3 siblings, 0 replies; 13+ messages in thread
From: Jonathan Toppins @ 2015-05-06 20:41 UTC (permalink / raw)
To: netdev, Jay Vosburgh, Veaceslav Falico, Andy Gospodarek, shm,
David Miller
Cc: Mahesh Bandewar
From: Mahesh Bandewar <maheshb@google.com>
The port key has three components - user-key, speed-part, and duplex-part.
The LSBit is for the duplex-part, next 5 bits are for the speed while the
remaining 10 bits are the user defined key bits. Get these 10 bits
from the user-space (through the SysFs interface) and use it to form the
admin port-key. Allowed range for the user-key is 0 - 1023 (10 bits). If
it is not provided then use zero for the user-key-bits (default).
It can set using following example code -
# modprobe bonding mode=4
# usr_port_key=$(( RANDOM & 0x3FF ))
# echo $usr_port_key > /sys/class/net/bond0/bonding/ad_user_port_key
# 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]
Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
---
v2:
* rebased
* fixed up context from change in ad_actor_sys_prio patch
Documentation/networking/bonding.txt | 63 ++++++++++++++++++++++++++++++++++
drivers/net/bonding/bond_3ad.c | 14 ++++----
drivers/net/bonding/bond_main.c | 10 ++++++
drivers/net/bonding/bond_options.c | 26 ++++++++++++++
drivers/net/bonding/bond_sysfs.c | 15 ++++++++
include/net/bond_options.h | 1 +
include/net/bonding.h | 1 +
7 files changed, 123 insertions(+), 7 deletions(-)
diff --git a/Documentation/networking/bonding.txt b/Documentation/networking/bonding.txt
index 2c197b6..334b49e 100644
--- a/Documentation/networking/bonding.txt
+++ b/Documentation/networking/bonding.txt
@@ -51,6 +51,7 @@ Table of Contents
3.4 Configuring Bonding Manually via Sysfs
3.5 Configuration with Interfaces Support
3.6 Overriding Configuration for Special Cases
+3.7 Configuring LACP for 802.3ad mode in a more secure way
4. Querying Bonding Configuration
4.1 Bonding Configuration
@@ -241,6 +242,21 @@ ad_select
This option was added in bonding version 3.4.0.
+ad_user_port_key
+
+ In an AD system, the port-key has three parts as shown below -
+
+ Bits Use
+ 00 Duplex
+ 01-05 Speed
+ 06-15 User-defined
+
+ This defines the upper 10 bits of the port key. The values can be
+ from 0 - 1023. If not given, the system defaults to 0.
+
+ This parameter has effect only in 802.3ad mode and is available through
+ SysFs interface.
+
all_slaves_active
Specifies that duplicate frames (received on inactive ports) should be
@@ -1643,6 +1659,53 @@ output port selection.
This feature first appeared in bonding driver version 3.7.0 and support for
output slave selection was limited to round-robin and active-backup modes.
+3.7 Configuring LACP for 802.3ad mode in a more secure way
+----------------------------------------------------------
+
+When using 802.3ad bonding mode, the Actor (host) and Partner (switch)
+exchange LACPDUs. These LACPDUs cannot be sniffed, because they are
+destined to link local mac addresses (which switches/bridges are not
+supposed to forward). However, most of the values are easily predictable
+or are simply the machine's MAC address (which is trivially known to all
+other hosts in the same L2). This implies that other machines in the L2
+domain can spoof LACPDU packets from other hosts to the switch and potentially
+cause mayhem by joining (from the point of view of the switch) another
+machine's aggregate, thus receiving a portion of that hosts incoming
+traffic and / or spoofing traffic from that machine themselves (potentially
+even successfully terminating some portion of flows). Though this is not
+a likely scenario, one could avoid this possibility by simply configuring
+few bonding parameters:
+
+ (a) ad_actor_system : You can set a random mac-address that can be used for
+ these LACPDU exchanges. The value can not be either NULL or Multicast.
+ Also it's preferable to set the local-admin bit. Following shell code
+ generates a random mac-address as described above.
+
+ # 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
+
+ (b) ad_actor_sys_prio : Randomize the system priority. The default value
+ is 65535, but system can take the value from 1 - 65535. Following shell
+ code generates random priority and sets it.
+
+ # sys_prio=$(( 1 + RANDOM + RANDOM ))
+ # echo $sys_prio > /sys/class/net/bond0/bonding/ad_actor_sys_prio
+
+ (c) ad_user_port_key : Use the user portion of the port-key. The default
+ keeps this empty. These are the upper 10 bits of the port-key and value
+ ranges from 0 - 1023. Following shell code generates these 10 bits and
+ sets it.
+
+ # usr_port_key=$(( RANDOM & 0x3FF ))
+ # echo $usr_port_key > /sys/class/net/bond0/bonding/ad_user_port_key
+
+
4 Querying Bonding Configuration
=================================
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 012f7bc..7fde4d5 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -75,10 +75,10 @@
/* Port Key definitions
* key is determined according to the link speed, duplex and
* user key (which is yet not supported)
- * --------------------------------------------------------------
- * Port key : | User key | Speed | Duplex |
- * --------------------------------------------------------------
- * 16 6 1 0
+ * --------------------------------------------------------------
+ * Port key | User key (10 bits) | Speed (5 bits) | Duplex|
+ * --------------------------------------------------------------
+ * |15 6|5 1|0
*/
#define AD_DUPLEX_KEY_MASKS 0x1
#define AD_SPEED_KEY_MASKS 0x3E
@@ -1951,10 +1951,10 @@ void bond_3ad_bind_slave(struct slave *slave)
port->slave = slave;
port->actor_port_number = SLAVE_AD_INFO(slave)->id;
- /* key is determined according to the link speed, duplex and user key(which
- * is yet not supported)
+ /* key is determined according to the link speed, duplex and
+ * user key
*/
- port->actor_admin_port_key = 0;
+ port->actor_admin_port_key = bond->params.ad_user_port_key << 6;
port->actor_admin_port_key |= __get_duplex(port);
port->actor_admin_port_key |= (__get_link_speed(port) << 1);
port->actor_oper_port_key = port->actor_admin_port_key;
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index a4e2f27..2ee13be 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4141,6 +4141,7 @@ static int bond_check_params(struct bond_params *params)
const struct bond_opt_value *valptr;
int arp_all_targets_value;
u16 ad_actor_sys_prio = 0;
+ u16 ad_user_port_key = 0;
/* Convert string parameters. */
if (mode) {
@@ -4445,6 +4446,14 @@ static int bond_check_params(struct bond_params *params)
}
ad_actor_sys_prio = valptr->value;
+ valptr = bond_opt_parse(bond_opt_get(BOND_OPT_AD_USER_PORT_KEY),
+ &newval);
+ if (!valptr) {
+ pr_err("Error: No ad_user_port_key default value");
+ return -EINVAL;
+ }
+ ad_user_port_key = valptr->value;
+
if (lp_interval == 0) {
pr_warn("Warning: ip_interval must be between 1 and %d, so it was reset to %d\n",
INT_MAX, BOND_ALB_DEFAULT_LP_INTERVAL);
@@ -4475,6 +4484,7 @@ static int bond_check_params(struct bond_params *params)
params->tlb_dynamic_lb = 1; /* Default value */
params->ad_actor_sys_prio = ad_actor_sys_prio;
eth_zero_addr(params->ad_actor_system);
+ params->ad_user_port_key = ad_user_port_key;
if (packets_per_slave > 0) {
params->reciprocal_packets_per_slave =
reciprocal_value(packets_per_slave);
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 978a46a..dccc432 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -74,6 +74,8 @@ static int bond_option_ad_actor_sys_prio_set(struct bonding *bond,
const struct bond_opt_value *newval);
static int bond_option_ad_actor_system_set(struct bonding *bond,
const struct bond_opt_value *newval);
+static int bond_option_ad_user_port_key_set(struct bonding *bond,
+ const struct bond_opt_value *newval);
static const struct bond_opt_value bond_mode_tbl[] = {
@@ -196,6 +198,12 @@ static const struct bond_opt_value bond_ad_actor_sys_prio_tbl[] = {
{ NULL, -1, 0},
};
+static const struct bond_opt_value bond_ad_user_port_key_tbl[] = {
+ { "minval", 0, BOND_VALFLAG_MIN | BOND_VALFLAG_DEFAULT},
+ { "maxval", 1023, BOND_VALFLAG_MAX},
+ { NULL, -1, 0},
+};
+
static const struct bond_option bond_opts[BOND_OPT_LAST] = {
[BOND_OPT_MODE] = {
.id = BOND_OPT_MODE,
@@ -405,6 +413,14 @@ static const struct bond_option bond_opts[BOND_OPT_LAST] = {
.flags = BOND_OPTFLAG_RAWVAL | BOND_OPTFLAG_IFDOWN,
.set = bond_option_ad_actor_system_set,
},
+ [BOND_OPT_AD_USER_PORT_KEY] = {
+ .id = BOND_OPT_AD_USER_PORT_KEY,
+ .name = "ad_user_port_key",
+ .unsuppmodes = BOND_MODE_ALL_EX(BIT(BOND_MODE_8023AD)),
+ .flags = BOND_OPTFLAG_IFDOWN,
+ .values = bond_ad_user_port_key_tbl,
+ .set = bond_option_ad_user_port_key_set,
+ }
};
/* Searches for an option by name */
@@ -1396,3 +1412,13 @@ static int bond_option_ad_actor_system_set(struct bonding *bond,
ether_addr_copy(bond->params.ad_actor_system, newval->string);
return 0;
}
+
+static int bond_option_ad_user_port_key_set(struct bonding *bond,
+ const struct bond_opt_value *newval)
+{
+ netdev_info(bond->dev, "Setting ad_user_port_key to (%llu)\n",
+ newval->value);
+
+ bond->params.ad_user_port_key = newval->value;
+ return 0;
+}
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index 5e4c2ea..00109dc 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -744,6 +744,20 @@ static ssize_t bonding_store_ad_actor_system(struct device *d,
static DEVICE_ATTR(ad_actor_system, S_IRUGO | S_IWUSR,
bonding_show_ad_actor_system, bonding_store_ad_actor_system);
+static ssize_t bonding_show_ad_user_port_key(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, "%hu\n", bond->params.ad_user_port_key);
+
+ return 0;
+}
+static DEVICE_ATTR(ad_user_port_key, S_IRUGO | S_IWUSR,
+ bonding_show_ad_user_port_key, bonding_sysfs_store_option);
+
static struct attribute *per_bond_attrs[] = {
&dev_attr_slaves.attr,
&dev_attr_mode.attr,
@@ -779,6 +793,7 @@ static struct attribute *per_bond_attrs[] = {
&dev_attr_tlb_dynamic_lb.attr,
&dev_attr_ad_actor_sys_prio.attr,
&dev_attr_ad_actor_system.attr,
+ &dev_attr_ad_user_port_key.attr,
NULL,
};
diff --git a/include/net/bond_options.h b/include/net/bond_options.h
index eeeefa1..c28aca2 100644
--- a/include/net/bond_options.h
+++ b/include/net/bond_options.h
@@ -65,6 +65,7 @@ enum {
BOND_OPT_TLB_DYNAMIC_LB,
BOND_OPT_AD_ACTOR_SYS_PRIO,
BOND_OPT_AD_ACTOR_SYSTEM,
+ BOND_OPT_AD_USER_PORT_KEY,
BOND_OPT_LAST
};
diff --git a/include/net/bonding.h b/include/net/bonding.h
index 650f386..20defc0 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;
+ u16 ad_user_port_key;
u8 ad_actor_system[ETH_ALEN];
};
--
1.7.10.4
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH linux v2 net-next 4/4] bonding: add netlink support for sys prio, actor sys mac, and port key
[not found] ` <cover.1430944053.git.jtoppins@cumulusnetworks.com>
` (2 preceding siblings ...)
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 ` Jonathan Toppins
2015-05-07 22:56 ` Mahesh Bandewar
3 siblings, 1 reply; 13+ messages in thread
From: Jonathan Toppins @ 2015-05-06 20:41 UTC (permalink / raw)
To: netdev, Jay Vosburgh, Veaceslav Falico, Andy Gospodarek, shm,
David Miller
From: Andy Gospodarek <gospo@cumulusnetworks.com>
Adds netlink support for the following bonding options:
* BOND_OPT_AD_ACTOR_SYS_PRIO
* BOND_OPT_AD_ACTOR_SYSTEM
* BOND_OPT_AD_USER_PORT_KEY
When setting the actor system mac address we assume the netlink message
contains a binary mac and not a string representation of a mac.
Signed-off-by: Andy Gospodarek <gospo@cumulusnetworks.com>
[jt: completed the setting side of the netlink attributes]
Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
---
v2:
* rebased
drivers/net/bonding/bond_netlink.c | 50 ++++++++++++++++++++++++++++++++++++
include/uapi/linux/if_link.h | 3 +++
2 files changed, 53 insertions(+)
diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
index 7b11243..c41be5f 100644
--- a/drivers/net/bonding/bond_netlink.c
+++ b/drivers/net/bonding/bond_netlink.c
@@ -94,6 +94,10 @@ static const struct nla_policy bond_policy[IFLA_BOND_MAX + 1] = {
[IFLA_BOND_AD_LACP_RATE] = { .type = NLA_U8 },
[IFLA_BOND_AD_SELECT] = { .type = NLA_U8 },
[IFLA_BOND_AD_INFO] = { .type = NLA_NESTED },
+ [IFLA_BOND_AD_ACTOR_SYS_PRIO] = { .type = NLA_U16 },
+ [IFLA_BOND_AD_USER_PORT_KEY] = { .type = NLA_U16 },
+ [IFLA_BOND_AD_ACTOR_SYSTEM] = { .type = NLA_BINARY,
+ .len = ETH_ALEN },
};
static const struct nla_policy bond_slave_policy[IFLA_BOND_SLAVE_MAX + 1] = {
@@ -379,6 +383,36 @@ static int bond_changelink(struct net_device *bond_dev,
if (err)
return err;
}
+ if (data[IFLA_BOND_AD_ACTOR_SYS_PRIO]) {
+ int actor_sys_prio =
+ nla_get_u16(data[IFLA_BOND_AD_ACTOR_SYS_PRIO]);
+
+ bond_opt_initval(&newval, actor_sys_prio);
+ err = __bond_opt_set(bond, BOND_OPT_AD_ACTOR_SYS_PRIO, &newval);
+ if (err)
+ return err;
+ }
+
+ if (data[IFLA_BOND_AD_USER_PORT_KEY]) {
+ int port_key =
+ nla_get_u16(data[IFLA_BOND_AD_USER_PORT_KEY]);
+
+ bond_opt_initval(&newval, port_key);
+ err = __bond_opt_set(bond, BOND_OPT_AD_USER_PORT_KEY, &newval);
+ if (err)
+ return err;
+ }
+
+ if (data[IFLA_BOND_AD_ACTOR_SYSTEM]) {
+ if (nla_len(data[IFLA_BOND_AD_ACTOR_SYSTEM]) != ETH_ALEN)
+ return -EINVAL;
+
+ bond_opt_initstr(&newval,
+ nla_data(data[IFLA_BOND_AD_ACTOR_SYSTEM]));
+ err = __bond_opt_set(bond, BOND_OPT_AD_ACTOR_SYSTEM, &newval);
+ if (err)
+ return err;
+ }
return 0;
}
@@ -426,6 +460,9 @@ static size_t bond_get_size(const struct net_device *bond_dev)
nla_total_size(sizeof(u16)) + /* IFLA_BOND_AD_INFO_ACTOR_KEY */
nla_total_size(sizeof(u16)) + /* IFLA_BOND_AD_INFO_PARTNER_KEY*/
nla_total_size(ETH_ALEN) + /* IFLA_BOND_AD_INFO_PARTNER_MAC*/
+ nla_total_size(sizeof(u16)) + /* IFLA_BOND_AD_ACTOR_SYS_PRIO */
+ nla_total_size(sizeof(u16)) + /* IFLA_BOND_AD_USER_PORT_KEY */
+ nla_total_size(ETH_ALEN) + /* IFLA_BOND_AD_ACTOR_SYSTEM */
0;
}
@@ -551,6 +588,19 @@ static int bond_fill_info(struct sk_buff *skb,
if (BOND_MODE(bond) == BOND_MODE_8023AD) {
struct ad_info info;
+ if (nla_put_u16(skb, IFLA_BOND_AD_ACTOR_SYS_PRIO,
+ bond->params.ad_actor_sys_prio))
+ goto nla_put_failure;
+
+ if (nla_put_u16(skb, IFLA_BOND_AD_USER_PORT_KEY,
+ bond->params.ad_user_port_key))
+ goto nla_put_failure;
+
+ if (nla_put(skb, IFLA_BOND_AD_ACTOR_SYSTEM,
+ sizeof(bond->params.ad_actor_system),
+ &bond->params.ad_actor_system))
+ goto nla_put_failure;
+
if (!bond_3ad_get_active_agg_info(bond, &info)) {
struct nlattr *nest;
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index d9cd192..6d6e502 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -417,6 +417,9 @@ enum {
IFLA_BOND_AD_LACP_RATE,
IFLA_BOND_AD_SELECT,
IFLA_BOND_AD_INFO,
+ IFLA_BOND_AD_ACTOR_SYS_PRIO,
+ IFLA_BOND_AD_USER_PORT_KEY,
+ IFLA_BOND_AD_ACTOR_SYSTEM,
__IFLA_BOND_MAX,
};
--
1.7.10.4
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH linux v2 net-next 4/4] bonding: add netlink support for sys prio, actor sys mac, and port key
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
0 siblings, 0 replies; 13+ messages in thread
From: Mahesh Bandewar @ 2015-05-07 22:56 UTC (permalink / raw)
To: Jonathan Toppins
Cc: linux-netdev, Jay Vosburgh, Veaceslav Falico, Andy Gospodarek,
shm, David Miller
On Wed, May 6, 2015 at 1:41 PM, Jonathan Toppins
<jtoppins@cumulusnetworks.com> wrote:
> From: Andy Gospodarek <gospo@cumulusnetworks.com>
>
> Adds netlink support for the following bonding options:
> * BOND_OPT_AD_ACTOR_SYS_PRIO
> * BOND_OPT_AD_ACTOR_SYSTEM
> * BOND_OPT_AD_USER_PORT_KEY
>
> When setting the actor system mac address we assume the netlink message
> contains a binary mac and not a string representation of a mac.
>
> Signed-off-by: Andy Gospodarek <gospo@cumulusnetworks.com>
> [jt: completed the setting side of the netlink attributes]
> Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
Acked-by: Mahesh Bandewar <maheshb@google.com>
Thanks for doing this.
--mahesh..
> ---
> v2:
> * rebased
>
> drivers/net/bonding/bond_netlink.c | 50 ++++++++++++++++++++++++++++++++++++
> include/uapi/linux/if_link.h | 3 +++
> 2 files changed, 53 insertions(+)
>
> diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
> index 7b11243..c41be5f 100644
> --- a/drivers/net/bonding/bond_netlink.c
> +++ b/drivers/net/bonding/bond_netlink.c
> @@ -94,6 +94,10 @@ static const struct nla_policy bond_policy[IFLA_BOND_MAX + 1] = {
> [IFLA_BOND_AD_LACP_RATE] = { .type = NLA_U8 },
> [IFLA_BOND_AD_SELECT] = { .type = NLA_U8 },
> [IFLA_BOND_AD_INFO] = { .type = NLA_NESTED },
> + [IFLA_BOND_AD_ACTOR_SYS_PRIO] = { .type = NLA_U16 },
> + [IFLA_BOND_AD_USER_PORT_KEY] = { .type = NLA_U16 },
> + [IFLA_BOND_AD_ACTOR_SYSTEM] = { .type = NLA_BINARY,
> + .len = ETH_ALEN },
> };
>
> static const struct nla_policy bond_slave_policy[IFLA_BOND_SLAVE_MAX + 1] = {
> @@ -379,6 +383,36 @@ static int bond_changelink(struct net_device *bond_dev,
> if (err)
> return err;
> }
> + if (data[IFLA_BOND_AD_ACTOR_SYS_PRIO]) {
> + int actor_sys_prio =
> + nla_get_u16(data[IFLA_BOND_AD_ACTOR_SYS_PRIO]);
> +
> + bond_opt_initval(&newval, actor_sys_prio);
> + err = __bond_opt_set(bond, BOND_OPT_AD_ACTOR_SYS_PRIO, &newval);
> + if (err)
> + return err;
> + }
> +
> + if (data[IFLA_BOND_AD_USER_PORT_KEY]) {
> + int port_key =
> + nla_get_u16(data[IFLA_BOND_AD_USER_PORT_KEY]);
> +
> + bond_opt_initval(&newval, port_key);
> + err = __bond_opt_set(bond, BOND_OPT_AD_USER_PORT_KEY, &newval);
> + if (err)
> + return err;
> + }
> +
> + if (data[IFLA_BOND_AD_ACTOR_SYSTEM]) {
> + if (nla_len(data[IFLA_BOND_AD_ACTOR_SYSTEM]) != ETH_ALEN)
> + return -EINVAL;
> +
> + bond_opt_initstr(&newval,
> + nla_data(data[IFLA_BOND_AD_ACTOR_SYSTEM]));
> + err = __bond_opt_set(bond, BOND_OPT_AD_ACTOR_SYSTEM, &newval);
> + if (err)
> + return err;
> + }
> return 0;
> }
>
> @@ -426,6 +460,9 @@ static size_t bond_get_size(const struct net_device *bond_dev)
> nla_total_size(sizeof(u16)) + /* IFLA_BOND_AD_INFO_ACTOR_KEY */
> nla_total_size(sizeof(u16)) + /* IFLA_BOND_AD_INFO_PARTNER_KEY*/
> nla_total_size(ETH_ALEN) + /* IFLA_BOND_AD_INFO_PARTNER_MAC*/
> + nla_total_size(sizeof(u16)) + /* IFLA_BOND_AD_ACTOR_SYS_PRIO */
> + nla_total_size(sizeof(u16)) + /* IFLA_BOND_AD_USER_PORT_KEY */
> + nla_total_size(ETH_ALEN) + /* IFLA_BOND_AD_ACTOR_SYSTEM */
> 0;
> }
>
> @@ -551,6 +588,19 @@ static int bond_fill_info(struct sk_buff *skb,
> if (BOND_MODE(bond) == BOND_MODE_8023AD) {
> struct ad_info info;
>
> + if (nla_put_u16(skb, IFLA_BOND_AD_ACTOR_SYS_PRIO,
> + bond->params.ad_actor_sys_prio))
> + goto nla_put_failure;
> +
> + if (nla_put_u16(skb, IFLA_BOND_AD_USER_PORT_KEY,
> + bond->params.ad_user_port_key))
> + goto nla_put_failure;
> +
> + if (nla_put(skb, IFLA_BOND_AD_ACTOR_SYSTEM,
> + sizeof(bond->params.ad_actor_system),
> + &bond->params.ad_actor_system))
> + goto nla_put_failure;
> +
> if (!bond_3ad_get_active_agg_info(bond, &info)) {
> struct nlattr *nest;
>
> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> index d9cd192..6d6e502 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -417,6 +417,9 @@ enum {
> IFLA_BOND_AD_LACP_RATE,
> IFLA_BOND_AD_SELECT,
> IFLA_BOND_AD_INFO,
> + IFLA_BOND_AD_ACTOR_SYS_PRIO,
> + IFLA_BOND_AD_USER_PORT_KEY,
> + IFLA_BOND_AD_ACTOR_SYSTEM,
> __IFLA_BOND_MAX,
> };
>
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 13+ messages in thread