mbox series

[bpf,0/4] bpf, sockmap: Fix some issues with using apply_bytes

Message ID 1668598161-15455-1-git-send-email-yangpc@wangsu.com (mailing list archive)
Headers show
Series bpf, sockmap: Fix some issues with using apply_bytes | expand

Message

Pengcheng Yang Nov. 16, 2022, 11:29 a.m. UTC
Patch 0001~0003 fixes three issues with using apply_bytes when redirecting.
Patch 0004 adds ingress tests for txmsg with apply_bytes in selftests.

Pengcheng Yang (4):
  bpf, sockmap: Fix repeated calls to sock_put() when msg has more_data
  bpf, sockmap: Fix missing BPF_F_INGRESS flag when using apply_bytes
  bpf, sockmap: Fix data loss caused by using apply_bytes on ingress
    redirect
  selftests/bpf: Add ingress tests for txmsg with apply_bytes

 include/linux/skmsg.h                      |  1 +
 net/core/skmsg.c                           |  1 +
 net/ipv4/tcp_bpf.c                         |  9 +++++++--
 net/tls/tls_sw.c                           |  1 +
 tools/testing/selftests/bpf/test_sockmap.c | 18 ++++++++++++++++++
 5 files changed, 28 insertions(+), 2 deletions(-)

Comments

Jakub Sitnicki Nov. 21, 2022, 9:35 a.m. UTC | #1
On Wed, Nov 16, 2022 at 07:29 PM +08, Pengcheng Yang wrote:
> Patch 0001~0003 fixes three issues with using apply_bytes when redirecting.
> Patch 0004 adds ingress tests for txmsg with apply_bytes in selftests.
>
> Pengcheng Yang (4):
>   bpf, sockmap: Fix repeated calls to sock_put() when msg has more_data
>   bpf, sockmap: Fix missing BPF_F_INGRESS flag when using apply_bytes
>   bpf, sockmap: Fix data loss caused by using apply_bytes on ingress
>     redirect
>   selftests/bpf: Add ingress tests for txmsg with apply_bytes
>
>  include/linux/skmsg.h                      |  1 +
>  net/core/skmsg.c                           |  1 +
>  net/ipv4/tcp_bpf.c                         |  9 +++++++--
>  net/tls/tls_sw.c                           |  1 +
>  tools/testing/selftests/bpf/test_sockmap.c | 18 ++++++++++++++++++
>  5 files changed, 28 insertions(+), 2 deletions(-)

Thanks for the patches. I need a bit more time to review them.
Daniel Borkmann Nov. 21, 2022, 4:38 p.m. UTC | #2
On 11/16/22 12:29 PM, Pengcheng Yang wrote:
> Patch 0001~0003 fixes three issues with using apply_bytes when redirecting.
> Patch 0004 adds ingress tests for txmsg with apply_bytes in selftests.
> 
> Pengcheng Yang (4):
>    bpf, sockmap: Fix repeated calls to sock_put() when msg has more_data
>    bpf, sockmap: Fix missing BPF_F_INGRESS flag when using apply_bytes
>    bpf, sockmap: Fix data loss caused by using apply_bytes on ingress
>      redirect
>    selftests/bpf: Add ingress tests for txmsg with apply_bytes

Patch 1 & 3 didn't make it to the list [0], could you resend your series?

   [0] https://lore.kernel.org/bpf/1668598161-15455-1-git-send-email-yangpc@wangsu.com/

>   include/linux/skmsg.h                      |  1 +
>   net/core/skmsg.c                           |  1 +
>   net/ipv4/tcp_bpf.c                         |  9 +++++++--
>   net/tls/tls_sw.c                           |  1 +
>   tools/testing/selftests/bpf/test_sockmap.c | 18 ++++++++++++++++++
>   5 files changed, 28 insertions(+), 2 deletions(-)
>
John Fastabend Nov. 22, 2022, 2:16 a.m. UTC | #3
Pengcheng Yang wrote:
> In tcp_bpf_send_verdict() redirection, the eval variable is assigned to
> __SK_REDIRECT after the apply_bytes data is sent, if msg has more_data,
> sock_put() will be called multiple times.
> We should reset the eval variable to __SK_NONE every time more_data
> starts.
> 
> This causes:
> 
> IPv4: Attempt to release TCP socket in state 1 00000000b4c925d7
> ------------[ cut here ]------------
> refcount_t: addition on 0; use-after-free.
> WARNING: CPU: 5 PID: 4482 at lib/refcount.c:25 refcount_warn_saturate+0x7d/0x110
> Modules linked in:
> CPU: 5 PID: 4482 Comm: sockhash_bypass Kdump: loaded Not tainted 6.0.0 #1
> Hardware name: Red Hat KVM, BIOS 1.11.0-2.el7 04/01/2014
> Call Trace:
>  <TASK>
>  __tcp_transmit_skb+0xa1b/0xb90
>  ? __alloc_skb+0x8c/0x1a0
>  ? __kmalloc_node_track_caller+0x184/0x320
>  tcp_write_xmit+0x22a/0x1110
>  __tcp_push_pending_frames+0x32/0xf0
>  do_tcp_sendpages+0x62d/0x640
>  tcp_bpf_push+0xae/0x2c0
>  tcp_bpf_sendmsg_redir+0x260/0x410
>  ? preempt_count_add+0x70/0xa0
>  tcp_bpf_send_verdict+0x386/0x4b0
>  tcp_bpf_sendmsg+0x21b/0x3b0
>  sock_sendmsg+0x58/0x70
>  __sys_sendto+0xfa/0x170
>  ? xfd_validate_state+0x1d/0x80
>  ? switch_fpu_return+0x59/0xe0
>  __x64_sys_sendto+0x24/0x30
>  do_syscall_64+0x37/0x90
>  entry_SYSCALL_64_after_hwframe+0x63/0xcd
> 
> Fixes: cd9733f5d75c ("tcp_bpf: Fix one concurrency problem in the tcp_bpf_send_verdict function")
> Signed-off-by: Pengcheng Yang <yangpc@wangsu.com>

I'll wait for the resend with all the patches but for this one.

Acked-by: John Fastabend <john.fastabend@gmail.com>