netdev - Re: [PATCH bpf-next v5 5/5] bpf/selftests: add selftest for bpf_smc

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: <20250108135606.GB86266@j66a10360.sqa.eu95>
Date: Wed, 8 Jan 2025 21:56:06 +0800
From: "D. Wythe" <alibuda@...ux.alibaba.com    >
To: Martin KaFai Lau <martin.lau@...ux.dev>
Cc: "D. Wythe" <alibuda@...ux.alibaba.com>, kgraul@...ux.ibm.com,
	wenjia@...ux.ibm.com, jaka@...ux.ibm.com, ast@...nel.org,
	daniel@...earbox.net, andrii@...nel.org, pabeni@...hat.com,
	song@...nel.org, sdf@...gle.com, haoluo@...gle.com, yhs@...com,
	edumazet@...gle.com, john.fastabend@...il.com, kpsingh@...nel.org,
	jolsa@...nel.org, guwen@...ux.alibaba.com, kuba@...nel.org,
	davem@...emloft.net, netdev@...r.kernel.org,
	linux-s390@...r.kernel.org, linux-rdma@...r.kernel.org,
	bpf@...r.kernel.org
Subject: Re: [PATCH bpf-next v5 5/5] bpf/selftests: add selftest for
 bpf_smc_ops

On Tue, Jan 07, 2025 at 04:48:51PM -0800, Martin KaFai Lau wrote:
> On 1/6/25 8:17 PM, D. Wythe wrote:
> >+static int send_cmd(int fd, __u16 nlmsg_type, __u32 nlmsg_pid, __u16 nlmsg_flags,
> >+			__u8 genl_cmd, __u16 nla_type,
> >+	while ((r = sendto(fd, buf, buflen, 0, (struct sockaddr *) &nladdr,
> >+			   sizeof(nladdr))) < buflen) {
> >+		if (r > 0) {
> >+			buf += r;
> >+			buflen -= r;
> >+		} else if (errno != EAGAIN)
> >+			return -1;
> >+		}
> 
> The "}" indentation is off.
> 
> I was wondering if it missed a "}" for the while loop. Turns out the
> "else if" does not have braces while the "if" has. I would add
> braces to the "else if" also to avoid confusion like this.
> 
Take it. I fix change it in next version.
> >+	return 0;
> >+}
> >+
> >+static bool get_smc_nl_family_id(void)
> >+{
> >+	struct sockaddr_nl nl_src;
> >+	struct msgtemplate msg;
> >+	ret = send_cmd(fd, smc_nl_family_id, pid,
> >+		       NLM_F_REQUEST | NLM_F_ACK, op, SMC_NLA_EID_TABLE_ENTRY,
> >+	(void *)test_ueid, sizeof(test_ueid));
> 
> Same. Indentation is off.
Take it. Thanks for pointing it out.
> 
> >+	if (!ASSERT_EQ(ret, 0, "ueid cmd"))
> >+		goto fail;
> >+
> >+	nstoken = open_netns(TEST_NS);
> 
> Instead of make_netns and then immediately open_netns, try
> netns_new(TEST_NS, true) from the test_progs.c.
Got it, I'll try it in next version.
> 
> >+	if (!ASSERT_OK_PTR(nstoken, "open net namespace"))
> >+		goto fail_open_netns;
> >+
> >+	if (!ASSERT_OK(system("ip addr add 127.0.1.0/8 dev lo"), "add server node"))
> >+		goto fail_ip;
> >+
> >+	if (!ASSERT_OK(system("ip addr add 127.0.2.0/8 dev lo"), "server via risk path"))
> >+	close_netns(nstoken);
> >+	return false;
> >+}
> >+
> >+	/* Configure ip strat */
> >+	block_link(map_fd, CLIENT_IP, SERVER_IP_VIA_RISK_PATH);
> >+	block_link(map_fd, SERVER_IP, SERVER_IP);
> >+	close(map_fd);
> 
> No need to close(map-fd) here. bpf_smc__destroy(skel) will do it.
Got it. Many thanks.
> 
> It seems the new selftest fails also. not always though which is concerning.
> 
This might not be a random failure, but rather related to s390x, which
carries a seid by default, which may affect my action of deleting ueid.
I am requesting IBM folks to help me analyze this issue since i have no
s390x machine.

Anyway, I will solve it in the next version.

Best wishes,
D. Wythe

> pw-bot: cr
> 
> >+
> >+	/* should go with smc */
> >+	run_link(CLIENT_IP, SERVER_IP, SERVICE_1);
> >+	/* should go with smc fallback */
> >+	run_link(SERVER_IP, SERVER_IP, SERVICE_2);
> >+
> >+	ASSERT_EQ(skel->bss->smc_cnt, 2, "smc count");
> >+	ASSERT_EQ(skel->bss->fallback_cnt, 1, "fallback count");
> >+
> >+	/* should go with smc */
> >+	run_link(CLIENT_IP, SERVER_IP, SERVICE_2);
> >+
> >+	ASSERT_EQ(skel->bss->smc_cnt, 3, "smc count");
> >+	ASSERT_EQ(skel->bss->fallback_cnt, 1, "fallback count");
> >+
> >+	/* should go with smc fallback */
> >+	run_link(CLIENT_IP, SERVER_IP_VIA_RISK_PATH, SERVICE_3);
> >+
> >+	ASSERT_EQ(skel->bss->smc_cnt, 4, "smc count");
> >+	ASSERT_EQ(skel->bss->fallback_cnt, 2, "fallback count");
> >+
> >+fail:
> >+	bpf_smc__destroy(skel);
> >+}
>