[PATCH net-next] selftests/net: fix bugs in cfg_port initialization

[PATCH net-next] selftests/net: fix bugs in cfg_port initialization

[Sowmini Varadhan]

If -S is not used in the command line, we should
be binding to *.<cfg-port>. Similarly, cfg_port should be
used to connect to the remote host even if it is processed
after -D. Thus we need to make sure that the cfg_port in
cfg_src_addr and cfg_dst_addr are always initialized
after all other command line options are parsed.

Store cfg_port in host-byte order,  and use htons()
to set up the sin_port/sin6_port before bind/connect,
so that the network system calls get the correct values
in network-byte order.

Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
---
 tools/testing/selftests/net/msg_zerocopy.c |   25 ++++++++++++++++++++++++-
 1 files changed, 24 insertions(+), 1 deletions(-)

diff --git a/tools/testing/selftests/net/msg_zerocopy.c b/tools/testing/selftests/net/msg_zerocopy.c
index 3ab6ec4..561fff7 100644
--- a/tools/testing/selftests/net/msg_zerocopy.c
+++ b/tools/testing/selftests/net/msg_zerocopy.c
@@ -259,6 +259,27 @@ static int setup_ip6h(struct ipv6hdr *ip6h, uint16_t payload_len)
 	return sizeof(*ip6h);
 }
 
+static void init_sockaddr_port(sa_family_t af,
+			       struct sockaddr_storage *sockaddr)
+{
+	struct sockaddr_in6 *addr6 = (struct sockaddr_in6 *) sockaddr;
+	struct sockaddr_in *addr4 = (struct sockaddr_in *) sockaddr;
+
+	switch (af) {
+	case PF_INET:
+		addr4->sin_family = PF_INET;
+		addr4->sin_port = htons(cfg_port);
+		break;
+	case PF_INET6:
+		addr4->sin_family = PF_INET6;
+		addr6->sin6_port = htons(cfg_port);
+		break;
+	default:
+		error(1, 0, "illegal domain");
+		break;
+	}
+}
+
 static void setup_sockaddr(int domain, const char *str_addr, void *sockaddr)
 {
 	struct sockaddr_in6 *addr6 = (void *) sockaddr;
@@ -638,7 +659,7 @@ static void parse_opts(int argc, char **argv)
 			cfg_cork_mixed = true;
 			break;
 		case 'p':
-			cfg_port = htons(strtoul(optarg, NULL, 0));
+			cfg_port = strtoul(optarg, NULL, 0);
 			break;
 		case 'r':
 			cfg_rx = true;
@@ -660,6 +681,8 @@ static void parse_opts(int argc, char **argv)
 			break;
 		}
 	}
+	init_sockaddr_port(cfg_family, &cfg_dst_addr);
+	init_sockaddr_port(cfg_family, &cfg_src_addr);
 
 	if (cfg_payload_len > max_payload_len)
 		error(1, 0, "-s: payload exceeds max (%d)", max_payload_len);
-- 
1.7.1

Re: [PATCH net-next] selftests/net: fix bugs in cfg_port initialization

[Willem de Bruijn]

On Sun, Dec 24, 2017 at 12:23 PM, Sowmini Varadhan
<sowmini.varadhan@oracle.com> wrote:
> If -S is not used in the command line, we should
> be binding to *.<cfg-port>. Similarly, cfg_port should be
> used to connect to the remote host even if it is processed
> after -D. Thus we need to make sure that the cfg_port in
> cfg_src_addr and cfg_dst_addr are always initialized
> after all other command line options are parsed.

Thanks, Sowmini. Yes, input parsing as is is dependent on
order, which is surprising and fragile.

It would be good to also address the other instance of this at the
same time: protocol family (-4 or -6) has to be set before a call to
setup_sockaddr. Unlike this case, it hits an error() if called in the
"incorrect" order, but that is still a crutch.

The new init_sockaddr_port duplicates most of setup_sockaddr.
More concise is to add a branch to that to skip inet_pton if !str_addr.

But even better will be to save cfg_port, and src + dst addr optargs
as local variables, then call the init function only after parsing when
all state is available. Where this patch adds calls to
init_sockaddr_port, indeed.

Re: [PATCH net-next] selftests/net: fix bugs in cfg_port initialization

[Sowmini Varadhan]

On (12/25/17 15:49), Willem de Bruijn wrote:
> 
> It would be good to also address the other instance of this at the
> same time: protocol family (-4 or -6) has to be set before a call to
> setup_sockaddr. Unlike this case, it hits an error() if called in the
> "incorrect" order, but that is still a crutch.

Yeah, I thought of that one too, but "usually" (at least
that's what is instinctive to me) you bind to INADDR_ANY
or ::, so this only becomes an issue on the client side. 
(And there, most people put the -4 or -6 before the address,
but I agree it would be nice to fix this..)

> But even better will be to save cfg_port, and src + dst addr optargs
> as local variables, then call the init function only after parsing when
> all state is available. Where this patch adds calls to
> init_sockaddr_port, indeed.

sure, I can put that out on V2 later this week.