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] [day] [month] [year] [list]

Message-ID: <be1629bc-26c2-12d3-45b3-f18d7abead4c@redhat.com>
Date:   Fri, 17 Jun 2022 14:12:34 -0400
From:   Jonathan Toppins <jtoppins@...hat.com>
To:     Hangbin Liu <liuhangbin@...il.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 6/17/22 04:04, Hangbin Liu wrote:
> 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 *))

To keep the naming consistent with the other helpers I would have chosen:

#define bond_opt_slave_initval(optval, slave_dev, value) \

>   
>   void bond_option_arp_ip_targets_clear(struct bonding *bond);
>   #if IS_ENABLED(CONFIG_IPV6)
> 
> 

[...]

The rest looks good to me.

-Jon