netdev - Re: [PATCHv2 net-next] Bonding: add per-port priority for failover re-selection

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Yqw1iheXg0fT3QcU@Laptop-X1>
Date:   Fri, 17 Jun 2022 16:04:26 +0800
From:   Hangbin Liu <liuhangbin@...il.com>
To:     Jonathan Toppins <jtoppins@...hat.com>
Cc:     netdev@...r.kernel.org, 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>,
        Paolo Abeni <pabeni@...hat.com>,
        David Ahern <dsahern@...il.com>
Subject: Re: [PATCHv2 net-next] Bonding: add per-port priority for failover
 re-selection

On Thu, Jun 16, 2022 at 10:58:12AM +0800, Hangbin Liu wrote:
> > > @@ -157,6 +162,20 @@ static int bond_slave_changelink(struct net_device *bond_dev,
> > >   			return err;
> > >   	}
> > > +	if (data[IFLA_BOND_SLAVE_PRIO]) { > +		int prio = nla_get_s32(data[IFLA_BOND_SLAVE_PRIO]);
> > > +		char prio_str[IFNAMSIZ + 7];
> > > +
> > > +		/* prio option setting expects slave_name:prio */
> > > +		snprintf(prio_str, sizeof(prio_str), "%s:%d\n",
> > > +			 slave_dev->name, prio);
> > > +
> > > +		bond_opt_initstr(&newval, prio_str);
> > 
> > It might be less code and a little cleaner to extend struct bond_opt_value
> > with a slave pointer.
> > 
> > 	struct bond_opt_value {
> > 		char *string;
> > 		u64 value;
> > 		u32 flags;
> > 		union {
> > 			char cextra[BOND_OPT_EXTRA_MAXLEN];
> > 			struct net_device *slave_dev;
> > 		} extra;
> > 	};
> > 
> > Then modify __bond_opt_init to set the slave pointer, basically a set of
> > bond_opt_slave_init{} macros. This would remove the need to parse the slave
> > interface name in the set function. Setting .flags = BOND_OPTFLAG_RAWVAL
> > (already done I see) in the option definition to avoid bond_opt_parse() from
> > loosing our extra information by pointing to a .values table entry. Now in
> > the option specific set function we can just find the slave entry and set
> > the value, no more string parsing code needed.
> 
> This looks reasonable to me. It would make all slave options setting easier
> for future usage.

Hi Jan, Jay,

I have updated the slave option setting like the following. I didn't add
a extra name for the union, so we don't need to edit the existing code. I think
the slave_dev should be safe as it's protected by rtnl lock. But I'm
not sure if I missed anything. Do you think if it's OK to store/get slave_dev
pointer like this?

diff --git a/include/net/bond_options.h b/include/net/bond_options.h
index 1618b76f4903..f65be547a73d 100644
--- a/include/net/bond_options.h
+++ b/include/net/bond_options.h
@@ -83,7 +83,10 @@ struct bond_opt_value {
 	char *string;
 	u64 value;
 	u32 flags;
-	char extra[BOND_OPT_EXTRA_MAXLEN];
+	union {
+		char extra[BOND_OPT_EXTRA_MAXLEN];
+		struct net_device *slave_dev;
+	};
 };
 
 struct bonding;
@@ -133,13 +136,16 @@ static inline void __bond_opt_init(struct bond_opt_value *optval,
 		optval->value = value;
 	else if (string)
 		optval->string = string;
-	else if (extra_len <= BOND_OPT_EXTRA_MAXLEN)
+
+	if (extra && extra_len <= BOND_OPT_EXTRA_MAXLEN)
 		memcpy(optval->extra, extra, extra_len);
 }
 #define bond_opt_initval(optval, value) __bond_opt_init(optval, NULL, value, NULL, 0)
 #define bond_opt_initstr(optval, str) __bond_opt_init(optval, str, ULLONG_MAX, NULL, 0)
 #define bond_opt_initextra(optval, extra, extra_len) \
 	__bond_opt_init(optval, NULL, ULLONG_MAX, extra, extra_len)
+#define bond_opt_initslave(optval, value, slave_dev) \
+	__bond_opt_init(optval, NULL, value, slave_dev, sizeof(struct net_device *))
 
 void bond_option_arp_ip_targets_clear(struct bonding *bond);
 #if IS_ENABLED(CONFIG_IPV6)


diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
index 5a6f44455b95..f0d3f36739ea 100644
--- a/drivers/net/bonding/bond_netlink.c
+++ b/drivers/net/bonding/bond_netlink.c
@@ -157,6 +162,16 @@ static int bond_slave_changelink(struct net_device *bond_dev,
                        return err;
        }

+       if (data[IFLA_BOND_SLAVE_PRIO]) {
+               int prio = nla_get_s32(data[IFLA_BOND_SLAVE_PRIO]);
+
+               bond_opt_initslave(&newval, prio, &slave_dev);
+               err = __bond_opt_set(bond, BOND_OPT_PRIO, &newval,
+                                    data[IFLA_BOND_SLAVE_PRIO], extack);
+               if (err)
+                       return err;
+       }
+
        return 0;
 }

diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 96eef19cffc4..473cedb0cb0b 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
+static int bond_option_prio_set(struct bonding *bond,
+                               const struct bond_opt_value *newval)
+{
+       struct slave *slave;
+
+       slave = bond_slave_get_rtnl(newval->slave_dev);
+       if (!slave) {
+               netdev_dbg(newval->slave_dev, "%s called on NULL slave\n", __func__);
+               return -ENODEV;
+       }
+       slave->prio = newval->value;
+
+       if (rtnl_dereference(bond->primary_slave))
+               slave_warn(bond->dev, slave->dev,
+                          "prio updated, but will not affect failover re-selection as primary slave have been set\n",
+                          slave->prio);
+       else
+               bond_select_active_slave(bond);
+
+       return 0;
+}

Thanks
Hangbin