Re: [PATCH v8.1 net-next 03/23] net/tcp: Introduce TCP_AO setsockopt()s

[Simon Horman]

Hi Dimitry,

On Tue, Jul 25, 2023 at 09:16:37PM +0100, Dmitry Safonov wrote:
> Hi Simon,
> 
> On 7/24/23 20:31, Simon Horman wrote:
> > On Fri, Jul 21, 2023 at 05:18:54PM +0100, Dmitry Safonov wrote:
> > 
> > ...
> > 
> > Hi Dimitry,
> > 
> >> diff --git a/include/linux/sockptr.h b/include/linux/sockptr.h
> >> index bae5e2369b4f..307961b41541 100644
> >> --- a/include/linux/sockptr.h
> >> +++ b/include/linux/sockptr.h
> >> @@ -55,6 +55,29 @@ static inline int copy_from_sockptr(void *dst, sockptr_t src, size_t size)
> >>  	return copy_from_sockptr_offset(dst, src, 0, size);
> >>  }
> >>  
> >> +static inline int copy_struct_from_sockptr(void *dst, size_t ksize,
> >> +		sockptr_t src, size_t usize)
> > 
> > The indentation of the two lines above is not correct,
> > they should be aligned to the inside of the opening '('
> > on the preceding line.
> > 
> > In order to stop things being too far to the left,
> > which is perhaps the intent of the current indention scheme,
> > the return type of the function can be moved to it's own line.
> > 
> > static inline int
> > copy_struct_from_sockptr(void *dst, size_t ksize, sockptr_t src, size_t usize)
> 
> Well, that would be a bit more GNU coding-style alike. Which I don't
> mind, I can do that. Albeit it's a bit contrary to an example from
> kernel's coding-style, where it seems preferred to keep it on the same
> line with function name and rather not to indent argument list, see
> (6.1), second example with action().
> 
> Yet, I don't feel particularly strong on either of options, so I can
> just do as you suggest.

For some reason I thought the style I suggested is acceptable,
at least in (some) Networking code.

In any case, I also don't feel strongly about this.

> > ...
> > 
> >> diff --git a/include/net/tcp.h b/include/net/tcp.h
> > 
> > ...
> > 
> >> +static inline int ipv4_prefix_cmp(const struct in_addr *addr1,
> >> +				  const struct in_addr *addr2,
> >> +				  unsigned int prefixlen)
> >> +{
> >> +	__be32 mask = inet_make_mask(prefixlen);
> >> +
> >> +	if ((addr1->s_addr & mask) == (addr2->s_addr & mask))
> >> +		return 0;
> >> +	return ((addr1->s_addr & mask) > (addr2->s_addr & mask)) ? 1 : -1;
> >> +}
> > 
> > Above, '>' is operating on two big endian values.
> > But typically such maths operates on host byte order values.
> > 
> > Flagged by Sparse.
> 
> Yeah, the function just has to provide any way to compare keys.
> So, it's not very important, but just to silence Sparse I can convert
> them to host's byte order before the comparison.

If you just care about equality, then perhaps you can use an equality
operator and avoid converting the endieness.

But, unless I am mistaken, '<' will give the wrong answer a little-endian host.
And this feels, well ..., wrong.

> > ...
> > 
> >> +static struct tcp_ao_key *__tcp_ao_do_lookup(const struct sock *sk,
> >> +		const union tcp_ao_addr *addr, int family, u8 prefix,
> >> +		int sndid, int rcvid, u16 port)
> > 
> > Same comment about indentation as above.
> > 
> > static struct tcp_ao_key *
> > __tcp_ao_do_lookup(const struct sock *sk, const union tcp_ao_addr *addr,
> > 		   int family, u8 prefix, int sndid, int rcvid, u16 port)
> > 
> > ...
> > 
> >> +struct tcp_ao_key *tcp_ao_do_lookup(const struct sock *sk,
> >> +				    const union tcp_ao_addr *addr,
> >> +				    int family, int sndid, int rcvid, u16 port)
> > 
> > Should tcp_ao_do_lookup be static?
> > It seems to only be used in this file.
> 
> Yeah, indeed. I think, I noticed previously, but probably managed to
> forget. Will fix.
> 
> > 
> > ...
> > 
> >> +static int tcp_ao_verify_port(struct sock *sk, u16 port)
> >> +{
> >> +	struct inet_sock *inet = inet_sk(sk);
> >> +
> >> +	if (port != 0) /* FIXME */
> > 
> > I guess this should be fixed :)
> 
> Fair enough. I think, what I'll do is to remove from these initial
> patches TCP-port from uAPI: we've expected that it will be useful to
> implement port-matching, but so far none from customers requested it.
> So, it was left as reserved member in uAPI, not meant to be used just yet.

Sounds good to me.
Thanks.

> Separately, as I've made UAPI structures for setsockopt() extendable,
> see copy_struct_from_sockptr() and the extendable syscall ideas
> (unfortunately, not in Documentation/):
> https://lpc.events/event/7/contributions/657/attachments/639/1159/extensible_syscalls.pdf
> https://lwn.net/Articles/830666/
> 
> So, as those structs can be extended in future, it won't be any hard to
> add port-matching on the top of the patch set. RFC5925 is permissive on
> how IP address and TCP-port matching may be performed:
> 
> : TCP connection identifier. A TCP socket pair, i.e., a local IP
> : address, a remote IP address, a TCP local port, and a TCP remote port.
> : Values can be partially specified using ranges (e.g., 2-30), masks
> : (e.g., 0xF0), wildcards (e.g., "*"), or any other suitable indication.
> 
> I can see some utility of TCP-AO key port-range matching and it seems
> most useful/flexible, so I'll add that. Unsure if that will go in
> version 9 or rather later (even post-merge).
> 
> I probably have to add something on that mater to
> Documentation/networking/tcp_ao.rst as well.
> 
> >> +		return -EINVAL;
> >> +
> >> +	/* Check that MKT port is consistent with socket */
> >> +	if (port != 0 && inet->inet_dport != 0 && port != inet->inet_dport)
> > 
> > port is host byte order, but inet->inet_dport is big endian.
> > This does not seem correct.
> 
> Thanks.

...