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 X-Spam-Level: X-Spam-Status: No, score=-15.8 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2B402C433E6 for ; Fri, 19 Feb 2021 22:01:58 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id EEE1564EBD for ; Fri, 19 Feb 2021 22:01:57 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229886AbhBSWBc (ORCPT ); Fri, 19 Feb 2021 17:01:32 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55174 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229689AbhBSWBY (ORCPT ); Fri, 19 Feb 2021 17:01:24 -0500 Received: from mail-wr1-x429.google.com (mail-wr1-x429.google.com [IPv6:2a00:1450:4864:20::429]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5C054C061574 for ; Fri, 19 Feb 2021 14:00:33 -0800 (PST) Received: by mail-wr1-x429.google.com with SMTP id a4so8310994wro.8 for ; Fri, 19 Feb 2021 14:00:33 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cloudflare.com; s=google; h=references:user-agent:from:to:cc:subject:in-reply-to:date :message-id:mime-version; bh=RBNlqRoSiJ1xlHuBA7FYF5J/Cpp57bs5HnF5tAm96Ik=; b=Aat0kMzB8javRMjqRvCXy9p//j6SlxwNxuQsGPSW1Zy+hfcEuPCRTBLKyOa0C8+mNJ 4U+UeE197azmFZmGUxaO1oVTRjBgE3WEVkNMEXOwX7BOGvb5jw8msY/cVNc0CR5O5phy 3a+HJgQQisdbDN49z4YRarQ6dxf7HNKv6u18E= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:references:user-agent:from:to:cc:subject :in-reply-to:date:message-id:mime-version; bh=RBNlqRoSiJ1xlHuBA7FYF5J/Cpp57bs5HnF5tAm96Ik=; b=joal8VmpcYup0+FSV4BGzagh/GLy6zDku5vt59ozdrW4DH+8e4eqHu6oCrFsd8BiFy hMDeU4OHWLhcot4OfSVZrSOjhfSt0r0eWNmV01xUCYPZeYw4dKlZUrDBQ270O/bWEw4A EYqBHFADn8QSrdRC1wyBgCfcnS/11GhcWwTYrqkJg371fKfzvxSEj/uG+kHbhRVypi6X BkOtkr7UFbUdLeDouscsxwSHQIspKoz6lhKOPKx+iwWYMetln2ofAKjkjjVPGUKWJjz9 Pz+tUq4rO5LoReQfWGCGLm79chPmey9/Fx2L/nNuhlxUVAwECk/23oLVrQK0Kl4oYwpJ 5NDA== X-Gm-Message-State: AOAM530gV1ZNUkfEJGzTr/XJAwAD+3queu8k/FqlL9iUQZIYPuLwvFhO dmcinx41+UVs9r90HFHmZCaqcw== X-Google-Smtp-Source: ABdhPJxtg0peN7p7p19gQOeg+3PQbduWC3a4KiH4obAhZ+Dk3XO48RXRxkxYEZoLhyivVgYht3OORg== X-Received: by 2002:adf:e80d:: with SMTP id o13mr11415701wrm.113.1613772031998; Fri, 19 Feb 2021 14:00:31 -0800 (PST) Received: from cloudflare.com (83.24.165.208.ipv4.supernova.orange.pl. [83.24.165.208]) by smtp.gmail.com with ESMTPSA id s14sm16326161wmj.23.2021.02.19.14.00.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 19 Feb 2021 14:00:31 -0800 (PST) References: <20210216064250.38331-1-xiyou.wangcong@gmail.com> <20210216064250.38331-2-xiyou.wangcong@gmail.com> <87im6n52zx.fsf@cloudflare.com> User-agent: mu4e 1.1.0; emacs 27.1 From: Jakub Sitnicki To: Cong Wang Cc: Linux Kernel Network Developers , bpf , duanxiongchun@bytedance.com, Dongdong Wang , Jiang Wang , Cong Wang , Daniel Borkmann , Lorenz Bauer , John Fastabend Subject: Re: [Patch bpf-next v4 1/5] bpf: clean up sockmap related Kconfigs In-reply-to: Date: Fri, 19 Feb 2021 23:00:30 +0100 Message-ID: <87h7m74t1d.fsf@cloudflare.com> MIME-Version: 1.0 Content-Type: text/plain Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org On Fri, Feb 19, 2021 at 07:46 PM CET, Cong Wang wrote: > On Fri, Feb 19, 2021 at 10:25 AM Jakub Sitnicki wrote: >> >> On Tue, Feb 16, 2021 at 07:42 AM CET, Cong Wang wrote: >> > From: Cong Wang >> > >> > As suggested by John, clean up sockmap related Kconfigs: >> > >> > Reduce the scope of CONFIG_BPF_STREAM_PARSER down to TCP stream >> > parser, to reflect its name. >> > >> > Make the rest sockmap code simply depend on CONFIG_BPF_SYSCALL. >> > And leave CONFIG_NET_SOCK_MSG untouched, as it is used by >> > non-sockmap cases. >> > >> > Cc: Daniel Borkmann >> > Cc: Jakub Sitnicki >> > Reviewed-by: Lorenz Bauer >> > Acked-by: John Fastabend >> > Signed-off-by: Cong Wang >> > --- >> >> Sorry for the delay. There's a lot happening here. Took me a while to >> dig through it. >> >> I have a couple of nit-picks, which easily can be addressed as >> follow-ups, and one comment. > > No problem, it is not merged, so V5 is definitely not a problem. > >> >> sock_map_prog_update and sk_psock_done_strp are only used in >> net/core/sock_map.c and can be static. > > 1. This seems to be unrelated to this patch? But I am still happy to > address it. Completely unrelated. Just a nit-pick. Feel free to ignore. > 2. sk_psock_done_strp() is in skmsg.c, hence why it is non-static. > And I believe it fits in skmsg.c better than in sock_map.c, because > it operates on psock rather than sock_map itself. I wrote that sk_psock_done_strp is used only in net/core/sock_map.c, while I should have written that it's used only in net/core/skmsg.c. Sorry, a mistake when copying from my notes. Also, feel free to ignore. > So, I can make sock_map_prog_update() static in a separate patch > and carry it in V5. Completely up to you. I don't insist. >> >> [...] >> >> > diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c >> > index bc7d2a586e18..b2c4865eb39b 100644 >> > --- a/net/ipv4/tcp_bpf.c >> > +++ b/net/ipv4/tcp_bpf.c >> > @@ -229,7 +229,6 @@ int tcp_bpf_sendmsg_redir(struct sock *sk, struct sk_msg *msg, >> > } >> > EXPORT_SYMBOL_GPL(tcp_bpf_sendmsg_redir); >> > >> > -#ifdef CONFIG_BPF_STREAM_PARSER >> > static bool tcp_bpf_stream_read(const struct sock *sk) >> > { >> > struct sk_psock *psock; >> > @@ -561,8 +560,10 @@ static void tcp_bpf_rebuild_protos(struct proto prot[TCP_BPF_NUM_CFGS], >> > struct proto *base) >> > { >> > prot[TCP_BPF_BASE] = *base; >> > +#if defined(CONFIG_BPF_SYSCALL) >> > prot[TCP_BPF_BASE].unhash = sock_map_unhash; >> > prot[TCP_BPF_BASE].close = sock_map_close; >> > +#endif >> > prot[TCP_BPF_BASE].recvmsg = tcp_bpf_recvmsg; >> > prot[TCP_BPF_BASE].stream_memory_read = tcp_bpf_stream_read; >> > >> > @@ -629,4 +630,3 @@ void tcp_bpf_clone(const struct sock *sk, struct sock *newsk) >> > if (prot == &tcp_bpf_prots[family][TCP_BPF_BASE]) >> > newsk->sk_prot = sk->sk_prot_creator; >> > } >> > -#endif /* CONFIG_BPF_STREAM_PARSER */ >> >> net/core/sock_map.o now is built only when CONFIG_BPF_SYSCALL is set. >> While tcp_bpf_get_proto is only called from net/core/sock_map.o. >> >> Seems there is no sense in compiling tcp_bpf_get_proto, and everything >> it depends on which was enclosed by CONFIG_BPF_STREAM_PARSER check, when >> CONFIG_BPF_SYSCALL is unset. > > I can try but I am definitely not sure whether kTLS is happy about > it, clearly kTLS at least uses __tcp_bpf_recvmsg() and > tcp_bpf_sendmsg_redir(). I think kTLS will be fine because that's the situation today. __tcp_bpf_recvmsg and tcp_bpf_sendmsg_redir need to be left out, naturally, is it is now. (Although I think they could event be stubbed out. But that would be unrelated to this change.) After all how would we end up on code path in kTLS that utilizes sockmap API, if sockmap can't be created when CONFIG_BPF_SYSCALL is unset. > >> >> > diff --git a/net/ipv4/udp_bpf.c b/net/ipv4/udp_bpf.c >> > index 7a94791efc1a..e635ccc175ca 100644 >> > --- a/net/ipv4/udp_bpf.c >> > +++ b/net/ipv4/udp_bpf.c >> > @@ -18,8 +18,10 @@ static struct proto udp_bpf_prots[UDP_BPF_NUM_PROTS]; >> > static void udp_bpf_rebuild_protos(struct proto *prot, const struct proto *base) >> > { >> > *prot = *base; >> > +#if defined(CONFIG_BPF_SYSCALL) >> > prot->unhash = sock_map_unhash; >> > prot->close = sock_map_close; >> > +#endif >> > } >> > >> > static void udp_bpf_check_v6_needs_rebuild(struct proto *ops) >> >> Same situation here but for udp_bpf_get_proto. > > UDP is different, as kTLS certainly doesn't and won't use it. I think > udp_bpf.c can be just put under CONFIG_BPF_SYSCALL. > > Thanks.