Re: [bpf-next PATCH] bpf: sockmap, fix crash when ipv6 sock is added

[John Fastabend <john.fastabend@gmail.com>]

On 06/04/2018 12:59 PM, Daniel Borkmann wrote:
> On 06/04/2018 05:21 PM, John Fastabend wrote:
>> This fixes a crash where we assign tcp_prot to IPv6 sockets instead
>> of tcpv6_prot.
>>
>> Previously we overwrote the sk->prot field with tcp_prot even in the
>> AF_INET6 case. This patch ensures the correct tcp_prot and tcpv6_prot
>> are used. Further, only allow ESTABLISHED connections to join the
>> map per note in TLS ULP,
>>
>>    /* The TLS ulp is currently supported only for TCP sockets
>>     * in ESTABLISHED state.
>>     * Supporting sockets in LISTEN state will require us
>>     * to modify the accept implementation to clone rather then
>>     * share the ulp context.
>>     */
>>
>> Also tested with 'netserver -6' and 'netperf -H [IPv6]' as well as
>> 'netperf -H [IPv4]'. The ESTABLISHED check resolves the previously
>> crashing case here.
>>
>> Fixes: 174a79ff9515 ("bpf: sockmap with sk redirect support")
>> Reported-by: syzbot+5c063698bdbfac19f363@syzkaller.appspotmail.com
>> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
>> Signed-off-by: Wei Wang <weiwan@google.com>
> 
> Applied to bpf-next, thanks everyone!
> 

Thanks Daniel, this has the unfortunate side-effect though of
making it hard to add sockets transitioning from LISTEN into
ESTABLISHED states to a sockmap. Before this patch we could add
sockets from the sock_ops event BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB
to a sock{map|hash}. However, this is before a socket is in established
state so risked crashing and wasn't valid at all per this thread. So
I believe its correct to block this action, seeing it will crash a
system in many (most!) cases.

That said we still would like to support pushing sockets into a
sock{map|hash} in this case. I thought about adding a new hook but
we already have a few sock op hooks in the TCP stack so its too bad we
don't have one that fires after the ESTABLISHED state has transitioned.
Right now I'm looking into seeing if the following would have any
issues,

--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2206,9 +2206,6 @@ void tcp_set_state(struct sock *sk, int state)
        BUILD_BUG_ON((int)BPF_TCP_NEW_SYN_RECV != (int)TCP_NEW_SYN_RECV);
        BUILD_BUG_ON((int)BPF_TCP_MAX_STATES != (int)TCP_MAX_STATES);
 
-       if (BPF_SOCK_OPS_TEST_FLAG(tcp_sk(sk), BPF_SOCK_OPS_STATE_CB_FLAG))
-               tcp_call_bpf_2arg(sk, BPF_SOCK_OPS_STATE_CB, oldstate, state);
-
        switch (state) {
        case TCP_ESTABLISHED:
                if (oldstate != TCP_ESTABLISHED)
@@ -2234,6 +2231,9 @@ void tcp_set_state(struct sock *sk, int state)
         */
        inet_sk_state_store(sk, state);
 
+       if (BPF_SOCK_OPS_TEST_FLAG(tcp_sk(sk), BPF_SOCK_OPS_STATE_CB_FLAG))
+               tcp_call_bpf_2arg(sk, BPF_SOCK_OPS_STATE_CB, oldstate, state);
+
 #ifdef STATE_TRACE
        SOCK_DEBUG(sk, "TCP sk=%p, State %s -> %s\n", sk, statename[oldstate], statename[state]);
 #endif


This would change the call hook slightly, moving it to after the state
change. However unless the unhash is some how visible from the bpf program
I don't think it should impact existing BPF programs.

Thanks,
John