Messages in this thread | | | Date | Wed, 26 Jul 2023 09:56:36 +0200 | From | Simon Horman <> | Subject | Re: [PATCH v8.1 net-next 03/23] net/tcp: Introduce TCP_AO setsockopt()s |
| |
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.
...
| |