netdev - Re: [PATCH v2] 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: <20241018093204.GC1697@kernel.org>
Date: Fri, 18 Oct 2024 10:32:04 +0100
From: Simon Horman <horms@...nel.org>
To: Leo Stone <leocstone@...il.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, shuah@...nel.org, 0x7f454c46@...il.com,
	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
Subject: Re: [PATCH v2] selftest/tcp-ao: Add filter tests

On Tue, Oct 15, 2024 at 10:51:52PM -0700, Leo Stone 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 is_current
> > # ok 116 filter keys: by is_rnext
> > # ok 117 filter keys: by sndid, rcvid
> > # ok 118 filter keys: correct nkeys when in.nkeys < matched_keys
> 
> Signed-off-by: Leo Stone <leocstone@...il.com>
> ---
> Changes in v2:
> - Changed 2 unnecessary test_error calls to test_fail
> - Added another test to make sure getsockopt returns the right nkeys
>   value when the input nkeys is smaller than the number of matching keys
> - Removed the TODO that this patch addresses
> 
> Thank you for your feedback.
> ---
>  .../selftests/net/tcp_ao/setsockopt-closed.c  | 180 +++++++++++++++++-
>  1 file changed, 175 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/tcp_ao/setsockopt-closed.c b/tools/testing/selftests/net/tcp_ao/setsockopt-closed.c
> index 084db4ecdff6..4bfa76c28e4e 100644
> --- a/tools/testing/selftests/net/tcp_ao/setsockopt-closed.c
> +++ b/tools/testing/selftests/net/tcp_ao/setsockopt-closed.c
> @@ -6,6 +6,8 @@
>  
>  static union tcp_addr tcp_md5_client;
>  
> +#define FILTER_TEST_NKEYS 16
> +
>  static int test_port = 7788;
>  static void make_listen(int sk)
>  {
> @@ -813,23 +815,191 @@ static void duplicate_tests(void)
>  	setsockopt_checked(sk, TCP_AO_ADD_KEY, &ao, EEXIST, "duplicate: SendID differs");
>  }
>  
> +
> +static void fetch_all_keys(int sk, struct tcp_ao_getsockopt *keys)
> +{
> +	socklen_t optlen = sizeof(struct tcp_ao_getsockopt);
> +
> +	memset(keys, 0, sizeof(struct tcp_ao_getsockopt) * FILTER_TEST_NKEYS);
> +	keys[0].get_all = 1;
> +	keys[0].nkeys = FILTER_TEST_NKEYS;
> +	if (getsockopt(sk, IPPROTO_TCP, TCP_AO_GET_KEYS, &keys[0], &optlen))
> +		test_error("getsockopt");
> +}
> +
> +static int prepare_test_keys(struct tcp_ao_getsockopt *keys)
> +{
> +	struct tcp_ao_add test_ao[FILTER_TEST_NKEYS];
> +	u8 rcvid = 100, sndid = 100;
> +	const char *test_password = "Test password number ";
> +	char test_password_scratch[64] = {};
> +	int sk = socket(test_family, SOCK_STREAM, IPPROTO_TCP);

Hi Leo,

In Networking code it is preferred to arrange local variables in
reverse xmas tree order. In this case I think that could be as
follows (completely untested!).

Also, as the sk needs to be checked for errors, I would
separate it's assignment form it's declaration

	const char *test_password = "Test password number ";
	struct tcp_ao_add test_ao[FILTER_TEST_NKEYS];
	char test_password_scratch[64] = {};
	u8 rcvid = 100, sndid = 100;
	int sk;

	sk = socket(test_family, SOCK_STREAM, IPPROTO_TCP);
	if (sk < 0)
		test_error("socket()");

This tool can be of assistance here:
https://github.com/ecree-solarflare/xmastree

> +
> +	if (sk < 0)
> +		test_error("socket()");
> +
> +	for (int i = 0; i < FILTER_TEST_NKEYS; i++) {
> +		snprintf(test_password_scratch, 64, "%s %d", test_password, i);
> +		test_prepare_key(&test_ao[i], DEFAULT_TEST_ALGO, this_ip_dest, false, false,
> +				 DEFAULT_TEST_PREFIX, 0, sndid++, rcvid++, 0, 0,
> +				 strlen(test_password_scratch), test_password_scratch);

Likewise, in Networking code it is still preferred to keep lines at or
below 80 columns wide, where it can trivially be achieved: don't split
strings across or otherwise make the code less readable because of this
guideline.

		test_prepare_key(&test_ao[i], DEFAULT_TEST_ALGO, this_ip_dest,
				 false, false, DEFAULT_TEST_PREFIX, 0, sndid++,
				 rcvid++, 0, 0, strlen(test_password_scratch),
				 test_password_scratch);

You can check for this using:
./scripts/checkpatch.pl --strict --max-line-length=80

I think it would be good if you could do a pass over this patch with the
above in mind.

Lastly, please include the target tree, net or net-next, in the subject
when posting patches for Networking.

	Subject: [PATCH net-next] ...

More information on processes for netdev can be found here:
https://docs.kernel.org/process/maintainer-netdev.html

-- 
pw-bot: changes-requested