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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Thu, 27 Jan 2022 14:14:16 +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 14:09, Hangbin Liu wrote:
> On Thu, Jan 27, 2022 at 10:56:29AM +0200, Nikolay Aleksandrov wrote:
>> 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
> 
> Not sure if I understand your suggestion correctly. Do you mean add a field
> in bond_opt_value like:
> 
> #define	MAX_LEN	128
> 
> struct bond_opt_value {
>         char *string;
>         u64 value;
>         u32 flags;
>         char extra[MAX_LEN];
> };
> 
> And init it before using?
> 
> or define a char *extra and alloc/init the memory when using it?
> 
> Thanks
> Hangbin
> 

Yeah, something like that so you can pass around larger items in the extra storage, but please
keep it to the minimum e.g. 16 bytes for this case as we have many static bond_opt_values
and pass it around a lot.

>> 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
>>
>>
>>
>>