netdev - Re: [PATCH RFC net-next 3/5] bonding: add ip6_addr for bond_opt

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 27 Jan 2022 10:56:29 +0200
From:   Nikolay Aleksandrov <nikolay@...dia.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>,
        David Ahern <dsahern@...il.com>
Subject: Re: [PATCH RFC net-next 3/5] bonding: add ip6_addr for bond_opt_value

On 27/01/2022 09:02, Hangbin Liu wrote:
> On Wed, Jan 26, 2022 at 01:35:46PM +0200, Nikolay Aleksandrov wrote:
>>> diff --git a/include/net/bond_options.h b/include/net/bond_options.h
>>> index dd75c071f67e..a9e68e88ff73 100644
>>> --- a/include/net/bond_options.h
>>> +++ b/include/net/bond_options.h
>>> @@ -79,6 +79,7 @@ struct bond_opt_value {
>>>  	char *string;
>>>  	u64 value;
>>>  	u32 flags;
>>> +	struct in6_addr ip6_addr;
>>>  };
>>>  
>>>  struct bonding;
>>> @@ -118,17 +119,20 @@ const struct bond_opt_value *bond_opt_get_val(unsigned int option, u64 val);
>>>   * When value is ULLONG_MAX then string will be used.
>>>   */
>>>  static inline void __bond_opt_init(struct bond_opt_value *optval,
>>> -				   char *string, u64 value)
>>> +				   char *string, u64 value, struct in6_addr *addr)
>>>  {
>>>  	memset(optval, 0, sizeof(*optval));
>>>  	optval->value = ULLONG_MAX;
>>> -	if (value == ULLONG_MAX)
>>> +	if (string)
>>>  		optval->string = string;
>>> +	else if (addr)
>>> +		optval->ip6_addr = *addr;
>>>  	else
>>>  		optval->value = value;
>>>  }
>>> -#define bond_opt_initval(optval, value) __bond_opt_init(optval, NULL, value)
>>> -#define bond_opt_initstr(optval, str) __bond_opt_init(optval, str, ULLONG_MAX)
>>> +#define bond_opt_initval(optval, value) __bond_opt_init(optval, NULL, value, NULL)
>>> +#define bond_opt_initstr(optval, str) __bond_opt_init(optval, str, ULLONG_MAX, NULL)
>>> +#define bond_opt_initaddr(optval, addr) __bond_opt_init(optval, NULL, ULLONG_MAX, addr)
>>>  
>>>  void bond_option_arp_ip_targets_clear(struct bonding *bond);
>>>  
>>
>> Please don't add arbitrary fields to struct bond_opt_value. As the comment above it states:
>> /* This structure is used for storing option values and for passing option
>>  * values when changing an option. The logic when used as an arg is as follows:
>>  * - if string != NULL -> parse it, if the opt is RAW type then return it, else
>>  *   return the parse result
>>  * - if string == NULL -> parse value
>>  */
>>
>> You can use an anonymous union to extend value's size and use the extra room for storage
>> only, that should keep most of the current logic intact.
> 
> Hi Nikolay,
> 
> The current checking for string is (value == ULLONG_MAX). If we use
> a union for IPv6 address and value, what about the address like
> 
> ffff:ffff:ffff:ffff::/64?
> 
> Thanks
> Hangbin

You're right in that we shouldn't overload value. My point was that bond_opt_val is supposed to be generic,
also it wouldn't work as expected for bond_opt_parse(). Perhaps a better solution would be to add a generic
extra storage field and length and initialize them with a helper that copies the needed bytes there. As for
value in that case you can just set it to 0, since all of this would be used internally the options which
support this new extra storage would expect it and should error out if it's missing (wrong/zero length).
Maybe something like:
static inline void __bond_opt_init(struct bond_opt_value *optval,
-				   char *string, u64 value)
+				   char *string, u64 value,
+				   void *extra, size_t extra_len)

with sanity and length checks of course, and:
+#define bond_opt_initextra(optval, extra, len) __bond_opt_init(optval, NULL, 0, extra, len)

It is similar to your solution, but it can be used by other options to store larger values and
it uses the value field as indicator that string shouldn't be parsed.

There are other alternatives like using the bond_opt_val flags to denote what has been set instead
of using the current struct field checks, but they would cause much more changes that seems
unnecessary just for this case.

Cheers,
 Nik