From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 782FAC04A6A for ; Tue, 15 Aug 2023 18:59:02 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S239549AbjHOS6b (ORCPT ); Tue, 15 Aug 2023 14:58:31 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40916 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S239643AbjHOS6H (ORCPT ); Tue, 15 Aug 2023 14:58:07 -0400 Received: from out-22.mta0.migadu.com (out-22.mta0.migadu.com [91.218.175.22]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 227D7D1 for ; Tue, 15 Aug 2023 11:58:01 -0700 (PDT) Message-ID: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1692125880; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=I5DcgThCjjtE1+nYureWow+fA/SCBcwrGd6zc9WACKM=; b=rSvIj7fRML9vmcrA9nhPpNeVu4VEXDPVQZGAKxuORdlmijW7hssGuyZy9lT+z9uiFzAG5p 89liqVk4ra5EJjydxE7yrMUDLiQ1MOXke6kbk+6Ajwkeg7Bejl7HC+Akd7JykIlbGd/GM7 5tWVZG8W0DWS9XWMzfXR98ebD5W3PWE= Date: Tue, 15 Aug 2023 11:57:52 -0700 MIME-Version: 1.0 Subject: Re: [PATCH mptcp-next v13 4/4] selftests/bpf: Add mptcpify test To: Geliang Tang Cc: Alexei Starovoitov , Daniel Borkmann , Andrii Nakryiko , Song Liu , Yonghong Song , John Fastabend , KP Singh , Stanislav Fomichev , Hao Luo , Jiri Olsa , Florent Revest , Brendan Jackman , Matthieu Baerts , Mat Martineau , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , John Johansen , Paul Moore , James Morris , "Serge E. Hallyn" , Stephen Smalley , Eric Paris , Mykola Lysenko , Shuah Khan , Simon Horman , bpf@vger.kernel.org, netdev@vger.kernel.org, mptcp@lists.linux.dev, linux-security-module@vger.kernel.org, selinux@vger.kernel.org, linux-kselftest@vger.kernel.org References: <15a618b03f65177166adf2850d4159cd4b77dfb1.1691808484.git.geliang.tang@suse.com> <00809f4a-e7ca-bf53-7824-e22791ee6738@linux.dev> <20230815100816.GA24858@bogon> Content-Language: en-US X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Martin KaFai Lau In-Reply-To: <20230815100816.GA24858@bogon> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Migadu-Flow: FLOW_OUT Precedence: bulk List-ID: On 8/15/23 3:08 AM, Geliang Tang wrote: > On Mon, Aug 14, 2023 at 11:23:49PM -0700, Martin KaFai Lau wrote: >> On 8/11/23 7:54 PM, Geliang Tang wrote: >>> +static int verify_mptcpify(int server_fd) >>> +{ >>> + socklen_t optlen; >>> + char cmd[256]; >>> + int protocol; >>> + int err = 0; >>> + >>> + optlen = sizeof(protocol); >>> + if (!ASSERT_OK(getsockopt(server_fd, SOL_SOCKET, SO_PROTOCOL, &protocol, &optlen), >>> + "getsockopt(SOL_PROTOCOL)")) >>> + return -1; >>> + >>> + if (!ASSERT_EQ(protocol, IPPROTO_MPTCP, "protocol isn't MPTCP")) >>> + err++; >>> + >>> + /* Output of nstat: >>> + * >>> + * #kernel >>> + * MPTcpExtMPCapableSYNACKRX 1 0.0 >>> + */ >>> + snprintf(cmd, sizeof(cmd), >>> + "ip netns exec %s nstat -asz %s | awk '%s' | grep -q '%s'", >>> + NS_TEST, "MPTcpExtMPCapableSYNACKRX", >>> + "NR==1 {next} {print $2}", "1"); >> >> Is the mp-capable something that the regular mptcp user want to learn from a >> fd also? Does it have a simpler way like to learn this, eg. getsockopt(fd, >> SOL_MPTCP, MPTCP_xxx), instead of parsing text output? > > Thanks Martin. Yes, you're right. A better one is using getsockopt > (MPTCP_INFO) to get the mptcpi_flags, then test the FALLBACK bit to make > sure this MPTCP connection didn't fallback. This is, in other word, this > MPTCP connection has been established correctly. Something like this: > > + optlen = sizeof(info); > + if (!ASSERT_OK(getsockopt(fd, SOL_MPTCP, MPTCP_INFO, &info, &optlen), > + "getsockopt(MPTCP_INFO)")) > + return -1; > + > + if (!ASSERT_FALSE(info.mptcpi_flags & MPTCP_INFO_FLAG_FALLBACK, > + "MPTCP fallback")) > + err++; > > It's necessary to add this further check after the MPTCP protocol check > using getsockopt(SOL_PROTOCOL). Since in some cases, the MPTCP protocol > check is not enough. Say, if we change TCP protocol into MPTCP using > "cgroup/sock_create", the hook of BPF_CGROUP_RUN_PROG_INET_SOCK in > inet_create(), this place is too late to change the protocol. Although > sk->sk_protocol is set to MPTCP correctly, and the MPTCP protocol check > using getsockopt(SOL_PROTOCOL) will pass. This MPTCP connection will > fallback to TCP connection. So this further check is needed. If I read it correctly, it sounds like the "ip netns... nstat.... awk...grep..." can be replaced with the getsockopt(MPTCP_INFO)? If that is the case, could you respin one more time to do getsockopt(MPTCP_INFO) instead? I would like the test to avoid parsing text output and have less dependency on external binary/library if possible (On top of 'ip', the above uses nstat, awk, grep...). This will make the bpf CI and other developers' environment tricky to maintain. eg. fwiw, although unrelated to the commands used above, my dev environment suddenly like to give this text output when I run "e"grep: "egrep: warning: egrep is obsolescent; using grep -E" >> >>> + if (!ASSERT_OK(system(cmd), "No MPTcpExtMPCapableSYNACKRX found!")) >> >>