netdev - Re: [RFC PATCH] selftest/tcp-ao: Add filter tests

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAJwJo6anjg4rBDLhgHL+vVtQ0FTWvK089p3D_xNNmcDTrXRL+w@mail.gmail.com>
Date: Mon, 14 Oct 2024 23:07:04 +0100
From: Dmitry Safonov <0x7f454c46@...il.com>
To: Leo Stone <leocstone@...il.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, 
	pabeni@...hat.com, shuah@...nel.org, rdunlap@...radead.org, 
	mnassiri@...na.com, jiapeng.chong@...ux.alibaba.com, colin.i.king@...il.com, 
	netdev@...r.kernel.org, linux-kselftest@...r.kernel.org, 
	linux-kernel@...r.kernel.org, anupnewsmail@...il.com, 
	linux-kernel-mentees@...ts.linuxfoundation.org
Subject: Re: [RFC PATCH] selftest/tcp-ao: Add filter tests

Hi Leo,

On Mon, 14 Oct 2024 at 22:37, Leo Stone <leocstone@...il.com> wrote:
>
> Add tests that check if getsockopt(TCP_AO_GET_KEYS) returns the right
> keys when using different filters.
>
> Sample output:
>
> > # ok 114 filter keys: by sndid, rcvid, address
> > # ok 115 filter keys: by sndid, rcvid
> > # ok 116 filter keys: by is_current
> > # ok 117 filter keys: by is_rnext
>
> Signed-off-by: Leo Stone <leocstone@...il.com>
> ---
> This patch is meant to address the TODO in setsockopt-closed.c:
> > /*
> >  * TODO: check getsockopt(TCP_AO_GET_KEYS) with different filters
> >  * returning proper nr & keys;
> >  */
>
> Is this a reasonable way to do these tests? If so, what cases should I
> add?

Your change does look reasonable to me.
I think you could add one more test here for passing
(FILTER_TEST_NKEYS/2) to getsockopt() as tcp_ao_getsockopt::nkeys with
get_all = 1, and check that the value in tcp_ao_getsockopt::nkeys
after getsockopt() reflects the number of matched keys
(FILTER_TEST_NKEYS).

See also minor nits inline.

[..]
> +static void filter_keys_checked(int sk, struct tcp_ao_getsockopt *filter,
> +                               struct tcp_ao_getsockopt *expected,
> +                               unsigned int nexpected, const char *tst)
> +{
> +       struct tcp_ao_getsockopt all_keys[FILTER_TEST_NKEYS] = {};
> +       struct tcp_ao_getsockopt filtered_keys[FILTER_TEST_NKEYS] = {};
> +       socklen_t len = sizeof(struct tcp_ao_getsockopt);
> +
> +       fetch_all_keys(sk, all_keys);
> +       memcpy(&filtered_keys[0], filter, sizeof(struct tcp_ao_getsockopt));
> +       filtered_keys[0].nkeys = FILTER_TEST_NKEYS;
> +       if (getsockopt(sk, IPPROTO_TCP, TCP_AO_GET_KEYS, filtered_keys, &len))
> +               test_error("getsockopt");

I think the following two checks would be better s/test_error/test_fail/.
The difference between _error() and _fail() is that for the later
exit() is not called, which allows the person running the test to
gather all "not okay" cases.
So, in tcp_ao selftests I used _error() only for failures where
nothing meaningful could be done afterwards, i.e. memory allocation or
socket() creation.

> +       if (filtered_keys[0].nkeys != nexpected)
> +               test_error("wrong nr of keys, expected %u got %u", nexpected,
> +                          filtered_keys[0].nkeys);
> +       if (compare_mkts(expected, nexpected, filtered_keys, filtered_keys[0].nkeys))
> +               test_error("got wrong keys back");

^ in those two it seems to be better to do
: test_fail("...")
: goto out_close;

which would allow to go through other "filter" and "duplicate"
selftests even if one of the "filter" tests has failed.

[..]
>  static void *client_fn(void *arg)
>  {
>         if (inet_pton(TEST_FAMILY, __TEST_CLIENT_IP(2), &tcp_md5_client) != 1)
>                 test_error("Can't convert ip address");
>         extend_tests();
>         einval_tests();
> +       filter_tests();
>         duplicate_tests();
>         /*
>          * TODO: check getsockopt(TCP_AO_GET_KEYS) with different filters

^ please, remove the related TODO comment, I think you just fixed it :-)

Thank you for the patch,
             Dmitry