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:

quoted

On Fri, Jul 21, 2023 at 05:18:54PM +0100, Dmitry Safonov wrote:

...

Hi Dimitry,

quoted

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.

quoted

...

quoted

diff --git a/include/net/tcp.h b/include/net/tcp.h

...

quoted

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

quoted

...

quoted

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

...

quoted

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

quoted

...

quoted

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

quoted

quoted

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

...