Re: [RFCv2 1/9] tcp: authopt: Initial support and key management

[David Ahern]

On 8/11/21 2:12 PM, Dmitry Safonov wrote:
> Hi David,
> 
> On 8/11/21 6:15 PM, David Ahern wrote:
>> On 8/11/21 8:31 AM, Dmitry Safonov wrote:
>>> On 8/11/21 9:29 AM, Leonard Crestez wrote:
>>>> On 8/10/21 11:41 PM, Dmitry Safonov wrote:
> [..]
>>>>> I'm pretty sure it's not a good choice to write partly tcp_authopt.
>>>>> And a user has no way to check what's the correct len on this kernel.
>>>>> Instead of len = min_t(unsigned int, len, sizeof(info)), it should be
>>>>> if (len != sizeof(info))
>>>>>      return -EINVAL;
>>>>
>>>> Purpose is to allow sockopts to grow as md5 has grown.
>>>
>>> md5 has not grown. See above.
>>
>> MD5 uapi has - e.g., 8917a777be3ba and  6b102db50cdde. We want similar
>> capabilities for growth with this API.
> 
> So, you mean adding a new setsockopt when the struct has to be extended?
> Like TCP_AUTHOPT_EXT?

uh, no. That was needed because of failures with the original
implementation wrt checking that all unused bits are 0. If checking is
not done from day 1, that field can never be used in the future.

My point here was only that MD5 uapi was extended.

My second point is more relevant to Leonard as a very recent example of
how to build an extendable struct.


>>
>> Look at how TCP_ZEROCOPY_RECEIVE has grown over releases as an example
>> of how to properly handle this.
> 
> Exactly.
> 
> : switch (len) {
> :		case offsetofend(...)
> :		case offsetofend(...)
> 
> And than also:
> :		if (unlikely(len > sizeof(zc))) {
> :			err = check_zeroed_user(optval + sizeof(zc),
> :						len - sizeof(zc));
> 
> Does it sound similar to what I've written in my ABI review?
> And what the LWN article has in it.
> Please, look again at the patch I replied to.
> 
> Thanks,
>          Dmitry
>