netdev - Re: [Patch bpf-next v4 1/5] bpf: clean up sockmap related Kconfigs

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]
Date:   Fri, 19 Feb 2021 23:00:30 +0100
From:   Jakub Sitnicki <jakub@...udflare.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>, duanxiongchun@...edance.com,
        Dongdong Wang <wangdongdong.6@...edance.com>,
        Jiang Wang <jiang.wang@...edance.com>,
        Cong Wang <cong.wang@...edance.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Lorenz Bauer <lmb@...udflare.com>,
        John Fastabend <john.fastabend@...il.com>
Subject: Re: [Patch bpf-next v4 1/5] bpf: clean up sockmap related Kconfigs

On Fri, Feb 19, 2021 at 07:46 PM CET, Cong Wang wrote:
> On Fri, Feb 19, 2021 at 10:25 AM Jakub Sitnicki <jakub@...udflare.com> wrote:
>>
>> On Tue, Feb 16, 2021 at 07:42 AM CET, Cong Wang wrote:
>> > From: Cong Wang <cong.wang@...edance.com>
>> >
>> > 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 <daniel@...earbox.net>
>> > Cc: Jakub Sitnicki <jakub@...udflare.com>
>> > Reviewed-by: Lorenz Bauer <lmb@...udflare.com>
>> > Acked-by: John Fastabend <john.fastabend@...il.com>
>> > Signed-off-by: Cong Wang <cong.wang@...edance.com>
>> > ---
>>
>> 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.