Re: [PATCH bpf-next] bpf, sockmap: Manual deletion of sockmap elements in user mode is not allowed

wangyufen wrote:

在 2022/3/16 0:25, Daniel Borkmann 写道:

quoted

On 3/15/22 1:12 PM, Jakub Sitnicki wrote:

quoted

On Tue, Mar 15, 2022 at 03:24 PM +08, wangyufen wrote:

quoted

在 2022/3/14 23:30, Jakub Sitnicki 写道:

quoted

On Mon, Mar 14, 2022 at 08:44 PM +08, Wang Yufen wrote:

quoted

A tcp socket in a sockmap. If user invokes bpf_map_delete_elem to 
delete
the sockmap element, the tcp socket will switch to use the TCP 
protocol
stack to send and receive packets. The switching process may cause 
some
issues, such as if some msgs exist in the ingress queue and are 
cleared
by sk_psock_drop(), the packets are lost, and the tcp data is 
abnormal.

Signed-off-by: Wang Yufen <redacted>
---

Can you please tell us a bit more about the life-cycle of the 
socket in
your workload? Questions that come to mind:

1) What triggers the removal of the socket from sockmap in your case?

We use sk_msg to redirect with sock hash, like this:

  skA   redirect    skB
  Tx <-----------> skB,Rx

And construct a scenario where the packet sending speed is high, the
packet receiving speed is slow, so the packets are stacked in the 
ingress
queue on the receiving side. In this case, if run 
bpf_map_delete_elem() to
delete the sockmap entry, will trigger the following procedure:

sock_hash_delete_elem()
   sock_map_unref()
     sk_psock_put()
       sk_psock_drop()
         sk_psock_stop()
           __sk_psock_zap_ingress()
             __sk_psock_purge_ingress_msg()

quoted

2) Would it still be a problem if removal from sockmap did not 
cause any
packets to get dropped?

Yes, it still be a problem. If removal from sockmap  did not cause any
packets to get dropped, packet receiving process switches to use TCP
protocol stack. The packets in the psock ingress queue cannot be 
received

by the user.

Thanks for the context. So, if I understand correctly, you want to avoid
breaking the network pipe by updating the sockmap from user-space.

This sounds awfully similar to BPF_MAP_FREEZE. Have you considered that?

+1

Aside from that, the patch as-is also fails BPF CI in a lot of places, 
please
make sure to check selftests:

https://github.com/kernel-patches/bpf/runs/5537367301?check_suite_focus=true 


  [...]
  #145/73 sockmap_listen/sockmap IPv6 test_udp_redir:OK
  #145/74 sockmap_listen/sockmap IPv6 test_udp_unix_redir:OK
  #145/75 sockmap_listen/sockmap Unix test_unix_redir:OK
  #145/76 sockmap_listen/sockmap Unix test_unix_redir:OK
  ./test_progs:test_ops_cleanup:1424: map_delete: expected 
EINVAL/ENOENT: Operation not supported
  test_ops_cleanup:FAIL:1424
  ./test_progs:test_ops_cleanup:1424: map_delete: expected 
EINVAL/ENOENT: Operation not supported
  test_ops_cleanup:FAIL:1424
  #145/77 sockmap_listen/sockhash IPv4 TCP test_insert_invalid:FAIL
  ./test_progs:test_ops_cleanup:1424: map_delete: expected 
EINVAL/ENOENT: Operation not supported
  test_ops_cleanup:FAIL:1424
  ./test_progs:test_ops_cleanup:1424: map_delete: expected 
EINVAL/ENOENT: Operation not supported
  test_ops_cleanup:FAIL:1424
  #145/78 sockmap_listen/sockhash IPv4 TCP test_insert_opened:FAIL
  ./test_progs:test_ops_cleanup:1424: map_delete: expected 
EINVAL/ENOENT: Operation not supported
  test_ops_cleanup:FAIL:1424
  ./test_progs:test_ops_cleanup:1424: map_delete: expected 
EINVAL/ENOENT: Operation not supported
  test_ops_cleanup:FAIL:1424
  #145/79 sockmap_listen/sockhash IPv4 TCP test_insert_bound:FAIL
  ./test_progs:test_ops_cleanup:1424: map_delete: expected 
EINVAL/ENOENT: Operation not supported
  test_ops_cleanup:FAIL:1424
  ./test_progs:test_ops_cleanup:1424: map_delete: expected 
EINVAL/ENOENT: Operation not supported
  test_ops_cleanup:FAIL:1424
  [...]

Thanks,
Daniel
.

I'm not sure about this patch. The main purpose is to point out the 
possible problems

when the socket is deleted from the map.I'm sorry for the trouble.

Thanks.

If you want to delete a socket you should flush it first. To do this
stop redirecting traffic to it and then read all the data out. At
the moment its a bit tricky to know when the recieving socket is
empty though. Adding a flag on delete to only delete when the
ingress qlen == 0 might be a possibility if you need delete to
work and are trying to work out how to safely delete sockets.