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

Hi David,

On 8/11/21 6:15 PM, David Ahern wrote:

On 8/11/21 8:31 AM, Dmitry Safonov wrote:

quoted

On 8/11/21 9:29 AM, Leonard Crestez wrote:

quoted

On 8/10/21 11:41 PM, Dmitry Safonov wrote:

[..]

quoted

quoted

quoted

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?

It can work, but sounds like adding a new syscall every time the old one
can't be extended. I can see that with current limitations on TCP-AO RFC
the ABI in these patches will have to be extended.

The second commit started using new cmd.tcpm_flags, where unknown flags
are still at this moment silently ignored by the kernel. So 6b102db50cdd
could have introduced a regression in userspace. By luck and by reason
that md5 isn't probably frequently used it didn't.
Not nice at all example for newer APIs.

quoted

Another issue with your approach

+       /* If userspace optlen is too short fill the rest with zeros */
+       if (optlen > sizeof(opt))
+               return -EINVAL;
+       memset(&opt, 0, sizeof(opt));
+       if (copy_from_sockptr(&opt, optval, optlen))
+               return -EFAULT;

is that userspace compiled with updated/grew structure will fail on
older kernel. So, no extension without breaking something is possible.
Which also reminds me that currently you don't validate (opt.flags) for
unknown by kernel flags.

Extending syscalls is impossible without breaking userspace if ABI is
not designed with extensibility in mind. That was quite a big problem,
and still is. Please, read this carefully:
https://lwn.net/Articles/830666/

That is why I'm suggesting you all those changes that will be harder to
fix when/if your patches get accepted.
As an example how it should work see in copy_clone_args_from_user().

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