| Message ID | 20220304081145.2037182-3-wangyufen@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9c34e38c4a870eb30b13f42f5b44f42e9d19ccb8 |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf, sockmap: Fix memleaks and issues of mem charge/uncharge | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 2 this patch: 2 |
| netdev/cc_maintainers | success | CCed 14 of 14 maintainers |
| netdev/build_clang | success | Errors and warnings before: 18 this patch: 18 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 7 this patch: 7 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 38 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
| bpf/vmtest-bpf-next-PR | fail | PR summary |
| bpf/vmtest-bpf-next | success | VM_Test |
Wang Yufen wrote: > If tcp_bpf_sendmsg() is running while sk msg is full. When sk_msg_alloc() > returns -ENOMEM error, tcp_bpf_sendmsg() goes to wait_for_memory. If partial > memory has been alloced by sk_msg_alloc(), that is, msg_tx->sg.size is > greater than osize after sk_msg_alloc(), memleak occurs. To fix we use > sk_msg_trim() to release the allocated memory, then goto wait for memory. > > Other call paths of sk_msg_alloc() have the similar issue, such as > tls_sw_sendmsg(), so handle sk_msg_trim logic inside sk_msg_alloc(), > as Cong Wang suggested. > > This issue can cause the following info: > WARNING: CPU: 3 PID: 7950 at net/core/stream.c:208 sk_stream_kill_queues+0xd4/0x1a0 > Call Trace: > <TASK> > inet_csk_destroy_sock+0x55/0x110 > __tcp_close+0x279/0x470 > tcp_close+0x1f/0x60 > inet_release+0x3f/0x80 > __sock_release+0x3d/0xb0 > sock_close+0x11/0x20 > __fput+0x92/0x250 > task_work_run+0x6a/0xa0 > do_exit+0x33b/0xb60 > do_group_exit+0x2f/0xa0 > get_signal+0xb6/0x950 > arch_do_signal_or_restart+0xac/0x2a0 > exit_to_user_mode_prepare+0xa9/0x200 > syscall_exit_to_user_mode+0x12/0x30 > do_syscall_64+0x46/0x80 > entry_SYSCALL_64_after_hwframe+0x44/0xae > </TASK> > > WARNING: CPU: 3 PID: 2094 at net/ipv4/af_inet.c:155 inet_sock_destruct+0x13c/0x260 > Call Trace: > <TASK> > __sk_destruct+0x24/0x1f0 > sk_psock_destroy+0x19b/0x1c0 > process_one_work+0x1b3/0x3c0 > kthread+0xe6/0x110 > ret_from_fork+0x22/0x30 > </TASK> > > Fixes: 604326b41a6f ("bpf, sockmap: convert to generic sk_msg interface") > Signed-off-by: Wang Yufen <wangyufen@huawei.com> > Acked-by: John Fastabend <john.fastabend@gmail.com> > --- Still LGTM Thanks! Next time drop the ack though if you rewrite the patch this much. Appreciate the fixes and helpful commit messages though.
diff --git a/net/core/skmsg.c b/net/core/skmsg.c index 8eb671c827f9..38ee73f453f8 100644 --- a/net/core/skmsg.c +++ b/net/core/skmsg.c @@ -27,6 +27,7 @@ int sk_msg_alloc(struct sock *sk, struct sk_msg *msg, int len, int elem_first_coalesce) { struct page_frag *pfrag = sk_page_frag(sk); + u32 osize = msg->sg.size; int ret = 0; len -= msg->sg.size; @@ -35,13 +36,17 @@ int sk_msg_alloc(struct sock *sk, struct sk_msg *msg, int len, u32 orig_offset; int use, i; - if (!sk_page_frag_refill(sk, pfrag)) - return -ENOMEM; + if (!sk_page_frag_refill(sk, pfrag)) { + ret = -ENOMEM; + goto msg_trim; + } orig_offset = pfrag->offset; use = min_t(int, len, pfrag->size - orig_offset); - if (!sk_wmem_schedule(sk, use)) - return -ENOMEM; + if (!sk_wmem_schedule(sk, use)) { + ret = -ENOMEM; + goto msg_trim; + } i = msg->sg.end; sk_msg_iter_var_prev(i); @@ -71,6 +76,10 @@ int sk_msg_alloc(struct sock *sk, struct sk_msg *msg, int len, } return ret; + +msg_trim: + sk_msg_trim(sk, msg, osize); + return ret; } EXPORT_SYMBOL_GPL(sk_msg_alloc);
If tcp_bpf_sendmsg() is running while sk msg is full. When sk_msg_alloc() returns -ENOMEM error, tcp_bpf_sendmsg() goes to wait_for_memory. If partial memory has been alloced by sk_msg_alloc(), that is, msg_tx->sg.size is greater than osize after sk_msg_alloc(), memleak occurs. To fix we use sk_msg_trim() to release the allocated memory, then goto wait for memory. Other call paths of sk_msg_alloc() have the similar issue, such as tls_sw_sendmsg(), so handle sk_msg_trim logic inside sk_msg_alloc(), as Cong Wang suggested. This issue can cause the following info: WARNING: CPU: 3 PID: 7950 at net/core/stream.c:208 sk_stream_kill_queues+0xd4/0x1a0 Call Trace: <TASK> inet_csk_destroy_sock+0x55/0x110 __tcp_close+0x279/0x470 tcp_close+0x1f/0x60 inet_release+0x3f/0x80 __sock_release+0x3d/0xb0 sock_close+0x11/0x20 __fput+0x92/0x250 task_work_run+0x6a/0xa0 do_exit+0x33b/0xb60 do_group_exit+0x2f/0xa0 get_signal+0xb6/0x950 arch_do_signal_or_restart+0xac/0x2a0 exit_to_user_mode_prepare+0xa9/0x200 syscall_exit_to_user_mode+0x12/0x30 do_syscall_64+0x46/0x80 entry_SYSCALL_64_after_hwframe+0x44/0xae </TASK> WARNING: CPU: 3 PID: 2094 at net/ipv4/af_inet.c:155 inet_sock_destruct+0x13c/0x260 Call Trace: <TASK> __sk_destruct+0x24/0x1f0 sk_psock_destroy+0x19b/0x1c0 process_one_work+0x1b3/0x3c0 kthread+0xe6/0x110 ret_from_fork+0x22/0x30 </TASK> Fixes: 604326b41a6f ("bpf, sockmap: convert to generic sk_msg interface") Signed-off-by: Wang Yufen <wangyufen@huawei.com> Acked-by: John Fastabend <john.fastabend@gmail.com> --- net/core/skmsg.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)