| Message ID | 20220302022755.3876705-2-wangyufen@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf, sockmap: Fix memleaks and issues of mem charge/uncharge | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-next-PR | success | PR summary |
| 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: 76 this patch: 76 |
| netdev/cc_maintainers | success | CCed 12 of 12 maintainers |
| netdev/build_clang | success | Errors and warnings before: 22 this patch: 22 |
| 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: 81 this patch: 81 |
| netdev/checkpatch | warning | CHECK: Unbalanced braces around else statement |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
| bpf/vmtest-bpf-next | success | VM_Test |
On Wed, Mar 02, 2022 at 10:27:52AM +0800, Wang Yufen wrote: > diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h > index fdb5375f0562..c5a2d6f50f25 100644 > --- a/include/linux/skmsg.h > +++ b/include/linux/skmsg.h > @@ -304,21 +304,16 @@ static inline void sock_drop(struct sock *sk, struct sk_buff *skb) > kfree_skb(skb); > } > > -static inline void drop_sk_msg(struct sk_psock *psock, struct sk_msg *msg) > -{ > - if (msg->skb) > - sock_drop(psock->sk, msg->skb); > - kfree(msg); > -} > - > static inline void sk_psock_queue_msg(struct sk_psock *psock, > struct sk_msg *msg) > { > spin_lock_bh(&psock->ingress_lock); > if (sk_psock_test_state(psock, SK_PSOCK_TX_ENABLED)) > list_add_tail(&msg->list, &psock->ingress_msg); > - else > - drop_sk_msg(psock, msg); > + else { > + sk_msg_free(psock->sk, msg); __sk_msg_free() calls sk_msg_init() at the end. > + kfree(msg); Now you free it, hence the above sk_msg_init() is completely unnecessary. Thanks.
在 2022/3/3 8:41, Cong Wang 写道: > On Wed, Mar 02, 2022 at 10:27:52AM +0800, Wang Yufen wrote: >> diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h >> index fdb5375f0562..c5a2d6f50f25 100644 >> --- a/include/linux/skmsg.h >> +++ b/include/linux/skmsg.h >> @@ -304,21 +304,16 @@ static inline void sock_drop(struct sock *sk, struct sk_buff *skb) >> kfree_skb(skb); >> } >> >> -static inline void drop_sk_msg(struct sk_psock *psock, struct sk_msg *msg) >> -{ >> - if (msg->skb) >> - sock_drop(psock->sk, msg->skb); >> - kfree(msg); >> -} >> - >> static inline void sk_psock_queue_msg(struct sk_psock *psock, >> struct sk_msg *msg) >> { >> spin_lock_bh(&psock->ingress_lock); >> if (sk_psock_test_state(psock, SK_PSOCK_TX_ENABLED)) >> list_add_tail(&msg->list, &psock->ingress_msg); >> - else >> - drop_sk_msg(psock, msg); >> + else { >> + sk_msg_free(psock->sk, msg); > __sk_msg_free() calls sk_msg_init() at the end. > >> + kfree(msg); > Now you free it, hence the above sk_msg_init() is completely > unnecessary. Invoking of sk_msg_free() does not always follow kfree(). That is, sk_msg needs to be reused in some cases. We can implement sk_msg_free_xx() without sk_msg_init(), but I don't think it is necessary. Thanks > > Thanks. > .
diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h index fdb5375f0562..c5a2d6f50f25 100644 --- a/include/linux/skmsg.h +++ b/include/linux/skmsg.h @@ -304,21 +304,16 @@ static inline void sock_drop(struct sock *sk, struct sk_buff *skb) kfree_skb(skb); } -static inline void drop_sk_msg(struct sk_psock *psock, struct sk_msg *msg) -{ - if (msg->skb) - sock_drop(psock->sk, msg->skb); - kfree(msg); -} - static inline void sk_psock_queue_msg(struct sk_psock *psock, struct sk_msg *msg) { spin_lock_bh(&psock->ingress_lock); if (sk_psock_test_state(psock, SK_PSOCK_TX_ENABLED)) list_add_tail(&msg->list, &psock->ingress_msg); - else - drop_sk_msg(psock, msg); + else { + sk_msg_free(psock->sk, msg); + kfree(msg); + } spin_unlock_bh(&psock->ingress_lock); }
If tcp_bpf_sendmsg is running during a tear down operation we may enqueue data on the ingress msg queue while tear down is trying to free it. sk1 (redirect sk2) sk2 ------------------- --------------- tcp_bpf_sendmsg() tcp_bpf_send_verdict() tcp_bpf_sendmsg_redir() bpf_tcp_ingress() sock_map_close() lock_sock() lock_sock() ... blocking sk_psock_stop sk_psock_clear_state(psock, SK_PSOCK_TX_ENABLED); release_sock(sk); lock_sock() sk_mem_charge() get_page() sk_psock_queue_msg() sk_psock_test_state(psock, SK_PSOCK_TX_ENABLED); drop_sk_msg() release_sock() While drop_sk_msg(), the msg has charged memory form sk by sk_mem_charge and has sg pages need to put. To fix we use sk_msg_free() and then kfee() msg. This issue can cause the following info: WARNING: CPU: 0 PID: 9202 at net/core/stream.c:205 sk_stream_kill_queues+0xc8/0xe0 Call Trace: <IRQ> inet_csk_destroy_sock+0x55/0x110 tcp_rcv_state_process+0xe5f/0xe90 ? sk_filter_trim_cap+0x10d/0x230 ? tcp_v4_do_rcv+0x161/0x250 tcp_v4_do_rcv+0x161/0x250 tcp_v4_rcv+0xc3a/0xce0 ip_protocol_deliver_rcu+0x3d/0x230 ip_local_deliver_finish+0x54/0x60 ip_local_deliver+0xfd/0x110 ? ip_protocol_deliver_rcu+0x230/0x230 ip_rcv+0xd6/0x100 ? ip_local_deliver+0x110/0x110 __netif_receive_skb_one_core+0x85/0xa0 process_backlog+0xa4/0x160 __napi_poll+0x29/0x1b0 net_rx_action+0x287/0x300 __do_softirq+0xff/0x2fc do_softirq+0x79/0x90 </IRQ> WARNING: CPU: 0 PID: 531 at net/ipv4/af_inet.c:154 inet_sock_destruct+0x175/0x1b0 Call Trace: <TASK> __sk_destruct+0x24/0x1f0 sk_psock_destroy+0x19b/0x1c0 process_one_work+0x1b3/0x3c0 ? process_one_work+0x3c0/0x3c0 worker_thread+0x30/0x350 ? process_one_work+0x3c0/0x3c0 kthread+0xe6/0x110 ? kthread_complete_and_exit+0x20/0x20 ret_from_fork+0x22/0x30 </TASK> Fixes: 9635720b7c88 ("bpf, sockmap: Fix memleak on ingress msg enqueue") Signed-off-by: Wang Yufen <wangyufen@huawei.com> Acked-by: John Fastabend <john.fastabend@gmail.com> --- include/linux/skmsg.h | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)