Message-ID: <8fb27342-2b32-b0e7-09d9-622ce16e8e76@arista.com>
Date:   Tue, 6 Sep 2022 17:34:41 +0100
From:   Dmitry Safonov <dima@...sta.com>
To:     Shuah Khan <skhan@...uxfoundation.org>,
        Eric Dumazet <edumazet@...gle.com>,
        "David S. Miller" <davem@...emloft.net>,
        linux-kernel@...r.kernel.org
Cc:     Andy Lutomirski <luto@...capital.net>,
        Ard Biesheuvel <ardb@...nel.org>,
        Bob Gilligan <gilligan@...sta.com>,
        David Ahern <dsahern@...nel.org>,
        Dmitry Safonov <0x7f454c46@...il.com>,
        Eric Biggers <ebiggers@...nel.org>,
        Francesco Ruggeri <fruggeri@...sta.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        Ivan Delalande <colona@...sta.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Leonard Crestez <cdleonard@...il.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Salam Noureddine <noureddine@...sta.com>,
        Shuah Khan <shuah@...nel.org>, netdev@...r.kernel.org,
        linux-crypto@...r.kernel.org
Subject: Re: [PATCH 25/31] selftests/net: Add TCP-AO library

On 8/23/22 16:47, Shuah Khan wrote:
> On 8/18/22 10:59 AM, Dmitry Safonov wrote:
[..]
>> +
>> +__attribute__((__format__(__printf__, 2, 3)))
>> +static inline void __test_print(void (*fn)(const char *), const char
>> *fmt, ...)
>> +{
>> +#define TEST_MSG_BUFFER_SIZE 4096
>> +    char buf[TEST_MSG_BUFFER_SIZE];
>> +    va_list arg;
>> +
>> +    va_start(arg, fmt);
>> +    vsnprintf(buf, sizeof(buf), fmt, arg);
>> +    va_end(arg);
>> +    fn(buf);
>> +}
>> +
> 
> Is there a reason add these instead of using kselftest_* print
> functions?

Inside __test_ok(), __test_msg(), __test_fail() and __test_error() are
calling ksft_*() functions. kselftest_*() by themselves are not
thread-safe and I was not sure if you would want them to be.

> 
>> +#define test_print(fmt, ...)                        \
>> +    __test_print(__test_msg, "%ld[%s:%u] " fmt "\n",        \
>> +             syscall(SYS_gettid),                \
>> +             __FILE__, __LINE__, ##__VA_ARGS__)
>> +
>> +#define test_ok(fmt, ...)                        \
>> +    __test_print(__test_ok, fmt "\n", ##__VA_ARGS__)
>> +
>> +#define test_fail(fmt, ...)                        \
>> +do {                                    \
>> +    if (errno)                            \
>> +        __test_print(__test_fail, fmt ": %m\n", ##__VA_ARGS__);    \
>> +    else                                \
>> +        __test_print(__test_fail, fmt "\n", ##__VA_ARGS__);    \
>> +    test_failed();                            \
>> +} while(0)
>> +
>> +#define KSFT_FAIL  1
>> +#define test_error(fmt, ...)                        \
>> +do {                                    \
>> +    if (errno)                            \
>> +        __test_print(__test_error, "%ld[%s:%u] " fmt ": %m\n",    \
>> +                 syscall(SYS_gettid), __FILE__, __LINE__,    \
>> +                 ##__VA_ARGS__);                \
>> +    else                                \
>> +        __test_print(__test_error, "%ld[%s:%u] " fmt "\n",    \
>> +                 syscall(SYS_gettid), __FILE__, __LINE__,    \
>> +                 ##__VA_ARGS__);                \
>> +    exit(KSFT_FAIL);                        \
>> +} while(0)
>> +
> 
> Is there a reason add these instead of using kselftest_* print
> functions?

The same reason: two or more threads my fail the test at the same
moment, I needed some way of protecting the output.

>> + * Timeout on syscalls where failure is not expected.
>> + * You may want to rise it if the test machine is very busy.
>> + */
>> +#ifndef TEST_TIMEOUT_SEC
>> +#define TEST_TIMEOUT_SEC    5
>> +#endif
>> +
> 
> Where is the TEST_TIMEOUT_SEC usually defined? Does this come
> from shell wrapper that runs this test? Can we add a message before
> starting the test print the timeout used?

Usually it's not re-defined and used as-is. Ifndef here is only to make
it easier to recompile with another timeout const: one can just add
CFLAGS+=-DTEST_TIMEOUT_SEC=10 and check if that helps on the busy hardware.


>> +/*
>> + * Timeout on connect() where a failure is expected.
>> + * If set to 0 - kernel will try to retransmit SYN number of times,
>> set in
>> + * /proc/sys/net/ipv4/tcp_syn_retries
>> + * By default set to 1 to make tests pass faster on non-busy machine.
>> + */
>> +#ifndef TEST_RETRANSMIT_SEC
>> +#define TEST_RETRANSMIT_SEC    1
>> +#endif
>> +
> 
> Where would this TEST_RETRANSMIT_SEC defined usually?

The same: I always used the default value, but protected by ifndef if
one wants to increase the value.

>> +
>> +static inline int _test_connect_socket(int sk, const union tcp_addr
>> taddr,
>> +                    unsigned port, time_t timeout)
>> +{
>> +#ifdef IPV6_TEST
>> +    struct sockaddr_in6 addr = {
>> +        .sin6_family    = AF_INET6,
>> +        .sin6_port    = htons(port),
>> +        .sin6_addr    = taddr.a6,
>> +    };
>> +#else
>> +    struct sockaddr_in addr = {
>> +        .sin_family    = AF_INET,
>> +        .sin_port    = htons(port),
>> +        .sin_addr    = taddr.a4,
>> +    };
>> +#endif
> 
> Why do we defined these here - are they also defined in a kernel
> header?

No, those functions are helpers that process family-specific members.
IPV6_TEST is coming from Makefile:
$(OUTPUT)/%_ipv6: %.c
	$(LINK.c) -DIPV6_TEST $^ $(LDLIBS) -o $@

This way all tests can be compiled from the same code for both address
families, resulting in *_ipv4 and *_ipv6 binaries that reuse all the
code, but just have those #ifdef IPV6_TEST in places where test converts
or produces IP addresses.

[..]
>> +static inline int test_prepare_ao(struct tcp_ao *ao,
>> +        const char *alg, uint16_t flags,
>> +        union tcp_addr in_addr, uint8_t prefix,
>> +        uint8_t sndid, uint8_t rcvid, uint8_t maclen,
>> +        uint8_t keyflags, uint8_t keylen, const char *key)
>> +{
>> +#ifdef IPV6_TEST
>> +    struct sockaddr_in6 addr = {
>> +        .sin6_family    = AF_INET6,
>> +        .sin6_port    = 0,
>> +        .sin6_addr    = in_addr.a6,
>> +    };
>> +#else
>> +    struct sockaddr_in addr = {
>> +        .sin_family    = AF_INET,
>> +        .sin_port    = 0,
>> +        .sin_addr    = in_addr.a4,
>> +    };
>> +#endif
>> +
> 
> Same question here. In general having these ifdefs isn't ideal without
> a good reason.

Same as above.

> 
>> +    return test_prepare_ao_sockaddr(ao, alg, flags,
>> +            (void *)&addr, sizeof(addr), prefix, sndid, rcvid,
>> +            maclen, keyflags, keylen, key);
>> +}
>> +
>> +static inline int test_prepare_def_ao(struct tcp_ao *ao,
>> +        const char *key, uint16_t flags,
>> +        union tcp_addr in_addr, uint8_t prefix,
>> +        uint8_t sndid, uint8_t rcvid)
>> +{
>> +    if (prefix > DEFAULT_TEST_PREFIX)
>> +        prefix = DEFAULT_TEST_PREFIX;
>> +
>> +    return test_prepare_ao(ao, DEFAULT_TEST_ALGO, flags, in_addr,
>> +            prefix, sndid, rcvid, 0, 0, strlen(key), key);
>> +}
>> +
>> +extern int test_get_one_ao(int sk, struct tcp_ao_getsockopt *out,
>> +               uint16_t flags, void *addr, size_t addr_sz,
>> +               uint8_t prefix, uint8_t sndid, uint8_t rcvid);
>> +extern int test_cmp_getsockopt_setsockopt(const struct tcp_ao *a,
>> +                      const struct tcp_ao_getsockopt *b);
>> +
>> +static inline int test_verify_socket_ao(int sk, struct tcp_ao *ao)
>> +{
>> +    struct tcp_ao_getsockopt tmp;
>> +    int err;
>> +
>> +    err = test_get_one_ao(sk, &tmp, 0, &ao->tcpa_addr,
>> +            sizeof(ao->tcpa_addr), ao->tcpa_prefix,
>> +            ao->tcpa_sndid, ao->tcpa_rcvid);
>> +    if (err)
>> +        return err;
> 
> Is this always an error or could this a skip if dependencies aren't
> met to run the test? This is a global comment for all error cases.

Yeah, at this moment all tests will FAIL if CONFIG_TCP_AO is disabled.
I'll look into making them SKIP when getsockopt()/setsockopt() returns
ENOPROTOOPT in next versions of patches.

> 
>> +
>> +    return test_cmp_getsockopt_setsockopt(ao, &tmp);
>> +}
>> +
>> +static inline int test_set_ao(int sk, const char *key, uint16_t flags,
>> +                  union tcp_addr in_addr, uint8_t prefix,
>> +                  uint8_t sndid, uint8_t rcvid)
>> +{
>> +    struct tcp_ao tmp;
>> +    int err;
>> +
>> +    err = test_prepare_def_ao(&tmp, key, flags, in_addr,
>> +            prefix, sndid, rcvid);
>> +    if (err)
>> +        return err;
> 
> Same comment as above here.
> 
>> +
>> +    if (setsockopt(sk, IPPROTO_TCP, TCP_AO, &tmp, sizeof(tmp)) < 0)
>> +        return -errno;
>> +
>> +    return test_verify_socket_ao(sk, &tmp);
>> +}
>> +
>> +extern ssize_t test_server_run(int sk, ssize_t quota, time_t
>> timeout_sec);
>> +extern ssize_t test_client_loop(int sk, char *buf, size_t buf_sz,
>> +                const size_t msg_len, time_t timeout_sec);
>> +extern int test_client_verify(int sk, const size_t msg_len, const
>> size_t nr,
>> +                  time_t timeout_sec);
>> +
>> +struct netstat;
>> +extern struct netstat *netstat_read(void);
>> +extern void netstat_free(struct netstat *ns);
>> +extern void netstat_print_diff(struct netstat *nsa, struct netstat
>> *nsb);
>> +extern uint64_t netstat_get(struct netstat *ns,
>> +                const char *name, bool *not_found);
>> +
>> +static inline uint64_t netstat_get_one(const char *name, bool
>> *not_found)
>> +{
>> +    struct netstat *ns = netstat_read();
>> +    uint64_t ret;
>> +
>> +    ret = netstat_get(ns, name, not_found);
>> +
>> +    netstat_free(ns);
>> +    return ret;
>> +}
>> +
>> +#endif /* _AOLIB_H_ */
>> diff --git a/tools/testing/selftests/net/tcp_ao/lib/netlink.c
>> b/tools/testing/selftests/net/tcp_ao/lib/netlink.c
>> new file mode 100644
>> index 000000000000..f04757c921d0
>> --- /dev/null
>> +++ b/tools/testing/selftests/net/tcp_ao/lib/netlink.c
>> @@ -0,0 +1,341 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Original from tools/testing/selftests/net/ipsec.c */
>> +#include <linux/netlink.h>
>> +#include <linux/random.h>
>> +#include <linux/rtnetlink.h>
>> +#include <linux/veth.h>
>> +#include <net/if.h>
>> +#include <stdint.h>
>> +#include <string.h>
>> +#include <sys/socket.h>
>> +
>> +#include "aolib.h"
>> +
>> +#define MAX_PAYLOAD        2048
> 
> tools/testing/selftests/net/gro.c seem to define this as:
> 
> #define MAX_PAYLOAD (IP_MAXPACKET - sizeof(struct tcphdr) -
> sizeof(struct ipv6hdr))
> 
> Can you do the same instead of hard-coding?

I think I could look into way of dynamically allocate netlink buffer for
requests, but I would say it's not as bad as it looks: the functions
always use constant size of messages for the netlink messages, so the
buffer size is always constant. And if anything doesn't fit, the helper
rtattr_pack() will just fail the test and it'll be visible straight away.
So, in my point of view, it could have been nicer, but this is the
easiest and simplest way by allocating const buffer on stack, rather
than dynamically.

>> +
>> +const struct sockaddr_in6 addr_any6 = {
>> +    .sin6_family    = AF_INET6,
>> +};
>> +
>> +const struct sockaddr_in addr_any4 = {
>> +    .sin_family    = AF_INET,
>> +};
>>
> 
> A couple of things to look at closely. For some failures such as
> memory allocation for the test or not being able to open a file
> 
> fnetstat = fopen("/proc/net/netstat", "r");
> 
> Is this a failure or missing config or not having the right permissions
> to open the fail. All of these cases would be a SKIP and not a test fail.

That makes sense, I'll look into making tests SKIP on failed memory
allocations or failed fopen()s.

Thank you for the review,
          Dmitry

Powered by blists - more mailing lists