| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1d6de134-c14e-4170-d2ad-873db62d8275@redhat.com>
Date: Tue, 12 Apr 2022 10:23:27 -0400
From: Jonathan Toppins <jtoppins@...hat.com>
To: Hangbin Liu <liuhangbin@...il.com>, netdev@...r.kernel.org
Cc: Jay Vosburgh <j.vosburgh@...il.com>,
Veaceslav Falico <vfalico@...il.com>,
Andy Gospodarek <andy@...yhouse.net>,
"David S . Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
David Ahern <dsahern@...il.com>,
Nikolay Aleksandrov <nikolay@...dia.com>,
Eric Dumazet <eric.dumazet@...il.com>,
Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next] Bonding: add per port priority support
On 4/12/22 00:13, Hangbin Liu wrote:
> Add per port priority support for bonding. A higher number means higher
> priority. The primary slave still has the highest priority. This option
> also follows the primary_reselect rules.
>
> This option could only be configured via netlink.
>
> Signed-off-by: Hangbin Liu <liuhangbin@...il.com>
> ---
> Documentation/networking/bonding.rst | 9 +++++++++
> drivers/net/bonding/bond_main.c | 27 +++++++++++++++++++++++++++
> drivers/net/bonding/bond_netlink.c | 12 ++++++++++++
> include/net/bonding.h | 1 +
> include/uapi/linux/if_link.h | 1 +
> tools/include/uapi/linux/if_link.h | 1 +
> 6 files changed, 51 insertions(+)
>
> diff --git a/Documentation/networking/bonding.rst b/Documentation/networking/bonding.rst
> index 525e6842dd33..103e292a04a1 100644
> --- a/Documentation/networking/bonding.rst
> +++ b/Documentation/networking/bonding.rst
> @@ -780,6 +780,15 @@ peer_notif_delay
> value is 0 which means to match the value of the link monitor
> interval.
>
> +prio
> + Slave priority. A higher number means higher priority.
> + The primary slave has the highest priority. This option also
> + follows the primary_reselect rules.
> +
> + This option could only be configured via netlink, and is only valid
^^ can
> + for active-backup(1), balance-tlb (5) and balance-alb (6) mode.
> + The default value is 0.
> +
> primary
>
> A string (eth0, eth2, etc) specifying which slave is the
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 15eddca7b4b6..4211b79ac619 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1026,12 +1026,38 @@ static void bond_do_fail_over_mac(struct bonding *bond,
>
> }
>
> +/**
> + * bond_choose_primary_or_current - select the primary or high priority slave
> + * @bond: our bonding struct
> + *
> + * - Check if there is a primary link. If the primary link was set and is up,
> + * go on and do link reselection.
> + *
> + * - If primary link is not set or down, find the highest priority link.
> + * If the highest priority link is not current slave, set it as primary
> + * link and do link reselection.
> + */
> static struct slave *bond_choose_primary_or_current(struct bonding *bond)
> {
> struct slave *prim = rtnl_dereference(bond->primary_slave);
> struct slave *curr = rtnl_dereference(bond->curr_active_slave);
> + struct slave *slave, *hprio = NULL;
> + struct list_head *iter;
>
> if (!prim || prim->link != BOND_LINK_UP) {
> + bond_for_each_slave(bond, slave, iter) {
> + if (slave->link == BOND_LINK_UP) {
> + hprio = hprio ? hprio : slave;
> + if (slave->prio > hprio->prio)
> + hprio = slave;
> + }
> + }
> +
> + if (hprio && hprio != curr) {
> + prim = hprio;
> + goto link_reselect;
> + }
> +
> if (!curr || curr->link != BOND_LINK_UP)
> return NULL;
> return curr;
> @@ -1042,6 +1068,7 @@ static struct slave *bond_choose_primary_or_current(struct bonding *bond)
> return prim;
> }
>
> +link_reselect:
> if (!curr || curr->link != BOND_LINK_UP)
> return prim;
>
> diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
> index f427fa1737c7..63066559e7d6 100644
> --- a/drivers/net/bonding/bond_netlink.c
> +++ b/drivers/net/bonding/bond_netlink.c
> @@ -27,6 +27,7 @@ static size_t bond_get_slave_size(const struct net_device *bond_dev,
> nla_total_size(sizeof(u16)) + /* IFLA_BOND_SLAVE_AD_AGGREGATOR_ID */
> nla_total_size(sizeof(u8)) + /* IFLA_BOND_SLAVE_AD_ACTOR_OPER_PORT_STATE */
> nla_total_size(sizeof(u16)) + /* IFLA_BOND_SLAVE_AD_PARTNER_OPER_PORT_STATE */
> + nla_total_size(sizeof(s32)) + /* IFLA_BOND_SLAVE_PRIO */
> 0;
> }
>
> @@ -53,6 +54,9 @@ static int bond_fill_slave_info(struct sk_buff *skb,
> if (nla_put_u16(skb, IFLA_BOND_SLAVE_QUEUE_ID, slave->queue_id))
> goto nla_put_failure;
>
> + if (nla_put_s32(skb, IFLA_BOND_SLAVE_PRIO, slave->prio))
> + goto nla_put_failure;
> +
> if (BOND_MODE(slave->bond) == BOND_MODE_8023AD) {
> const struct aggregator *agg;
> const struct port *ad_port;
> @@ -117,6 +121,7 @@ static const struct nla_policy bond_policy[IFLA_BOND_MAX + 1] = {
>
> static const struct nla_policy bond_slave_policy[IFLA_BOND_SLAVE_MAX + 1] = {
> [IFLA_BOND_SLAVE_QUEUE_ID] = { .type = NLA_U16 },
> + [IFLA_BOND_SLAVE_PRIO] = { .type = NLA_S32 },
> };
>
> static int bond_validate(struct nlattr *tb[], struct nlattr *data[],
> @@ -136,6 +141,7 @@ static int bond_slave_changelink(struct net_device *bond_dev,
> struct nlattr *tb[], struct nlattr *data[],
> struct netlink_ext_ack *extack)
> {
> + struct slave *slave = bond_slave_get_rtnl(slave_dev);
> struct bonding *bond = netdev_priv(bond_dev);
> struct bond_opt_value newval;
> int err;
> @@ -156,6 +162,12 @@ static int bond_slave_changelink(struct net_device *bond_dev,
> return err;
> }
>
> + /* No need to bother __bond_opt_set as we only support netlink config */
Not sure this comment is necessary, it doesn't add any value. Also I
would recommend using bonding's options management, as it would allow
for checking if the value is in a defined range. That might not be
particularly useful in this context since it appears +/-INT_MAX is the
range.
Also, in the Documentation it is mentioned that this parameter is only
used in modes active-backup and balance-alb/tlb. Do we need to send an
error message back preventing the modification of this value when not in
these modes?
> + if (data[IFLA_BOND_SLAVE_PRIO]) {
> + slave->prio = nla_get_s32(data[IFLA_BOND_SLAVE_PRIO]);
> + bond_select_active_slave(bond);
> + }
> +
> return 0;
> }
>
> diff --git a/include/net/bonding.h b/include/net/bonding.h
> index b14f4c0b4e9e..4ff093fb2289 100644
> --- a/include/net/bonding.h
> +++ b/include/net/bonding.h
> @@ -176,6 +176,7 @@ struct slave {
> u32 speed;
> u16 queue_id;
> u8 perm_hwaddr[MAX_ADDR_LEN];
> + int prio;
Do we want a struct slave_params here instead? That would allow us to
define defaults in a central place and set them once instead of setting
each parameter.
> struct ad_slave_info *ad_info;
> struct tlb_slave_info tlb_info;
> #ifdef CONFIG_NET_POLL_CONTROLLER
> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> index cc284c048e69..204e342b107a 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -956,6 +956,7 @@ enum {
> IFLA_BOND_SLAVE_AD_AGGREGATOR_ID,
> IFLA_BOND_SLAVE_AD_ACTOR_OPER_PORT_STATE,
> IFLA_BOND_SLAVE_AD_PARTNER_OPER_PORT_STATE,
> + IFLA_BOND_SLAVE_PRIO,
> __IFLA_BOND_SLAVE_MAX,
> };
>
> diff --git a/tools/include/uapi/linux/if_link.h b/tools/include/uapi/linux/if_link.h
> index e1ba2d51b717..ee5de9f3700b 100644
> --- a/tools/include/uapi/linux/if_link.h
> +++ b/tools/include/uapi/linux/if_link.h
> @@ -888,6 +888,7 @@ enum {
> IFLA_BOND_SLAVE_AD_AGGREGATOR_ID,
> IFLA_BOND_SLAVE_AD_ACTOR_OPER_PORT_STATE,
> IFLA_BOND_SLAVE_AD_PARTNER_OPER_PORT_STATE,
> + IFLA_BOND_SLAVE_PRIO,
> __IFLA_BOND_SLAVE_MAX,
> };
>
Powered by blists - more mailing lists