| Message ID | 20190410180432.13852-4-jakub.kicinski@netronome.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: tls: fix memory leaks and freeing skbs | expand |
On 04/10/2019 11:04 AM, Jakub Kicinski wrote: > This reverts the first part of commit 4e485d06bb8c ("strparser: Call > skb_unclone conditionally"). To build a message with multiple > fragments we need our own root of frag_list. We can't simply > use the frag_list of orig_skb, because it will lead to linking > all orig_skbs together creating very long frag chains, and causing > stack overflow on kfree_skb() (which is called recursively on > the frag_lists). > > BUG: stack guard page was hit at 00000000d40fad41 (stack is 0000000029dde9f4..000000008cce03d5) > kernel stack overflow (double-fault): 0000 [#1] PREEMPT SMP > RIP: 0010:free_one_page+0x2b/0x490 > > Call Trace: > __free_pages_ok+0x143/0x2c0 > skb_release_data+0x8e/0x140 > ? skb_release_data+0xad/0x140 > kfree_skb+0x32/0xb0 > > [...] > > skb_release_data+0xad/0x140 > ? skb_release_data+0xad/0x140 ... > tcp_disconnect+0xd6/0x4d0 > tcp_close+0xf4/0x430 > ? tcp_check_oom+0xf0/0xf0 > tls_sk_proto_close+0xe4/0x1e0 [tls] > inet_release+0x36/0x60 > __sock_release+0x37/0xa0 > sock_close+0x11/0x20 > __fput+0xa2/0x1d0 > task_work_run+0x89/0xb0 > exit_to_usermode_loop+0x9a/0xa0 > do_syscall_64+0xc0/0xf0 > entry_SYSCALL_64_after_hwframe+0x44/0xa9 > > Let's leave the second unclone conditional, as I'm not entirely > sure what is its purpose :) > > Fixes: 4e485d06bb8c ("strparser: Call skb_unclone conditionally") > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> > Reviewed-by: Dirk van der Merwe <dirk.vandermerwe@netronome.com> Thanks for tracking the root cause of this problem. Reviewed-by: Eric Dumazet <edumazet@google.com>
diff --git a/net/strparser/strparser.c b/net/strparser/strparser.c index 860dcfb95ee4..fa6c977b4c41 100644 --- a/net/strparser/strparser.c +++ b/net/strparser/strparser.c @@ -140,13 +140,11 @@ static int __strp_recv(read_descriptor_t *desc, struct sk_buff *orig_skb, /* We are going to append to the frags_list of head. * Need to unshare the frag_list. */ - if (skb_has_frag_list(head)) { - err = skb_unclone(head, GFP_ATOMIC); - if (err) { - STRP_STATS_INCR(strp->stats.mem_fail); - desc->error = err; - return 0; - } + err = skb_unclone(head, GFP_ATOMIC); + if (err) { + STRP_STATS_INCR(strp->stats.mem_fail); + desc->error = err; + return 0; } if (unlikely(skb_shinfo(head)->frag_list)) {
This reverts the first part of commit 4e485d06bb8c ("strparser: Call skb_unclone conditionally"). To build a message with multiple fragments we need our own root of frag_list. We can't simply use the frag_list of orig_skb, because it will lead to linking all orig_skbs together creating very long frag chains, and causing stack overflow on kfree_skb() (which is called recursively on the frag_lists). BUG: stack guard page was hit at 00000000d40fad41 (stack is 0000000029dde9f4..000000008cce03d5) kernel stack overflow (double-fault): 0000 [#1] PREEMPT SMP RIP: 0010:free_one_page+0x2b/0x490 Call Trace: __free_pages_ok+0x143/0x2c0 skb_release_data+0x8e/0x140 ? skb_release_data+0xad/0x140 kfree_skb+0x32/0xb0 [...] skb_release_data+0xad/0x140 ? skb_release_data+0xad/0x140 kfree_skb+0x32/0xb0 skb_release_data+0xad/0x140 ? skb_release_data+0xad/0x140 kfree_skb+0x32/0xb0 skb_release_data+0xad/0x140 ? skb_release_data+0xad/0x140 kfree_skb+0x32/0xb0 skb_release_data+0xad/0x140 ? skb_release_data+0xad/0x140 kfree_skb+0x32/0xb0 skb_release_data+0xad/0x140 __kfree_skb+0xe/0x20 tcp_disconnect+0xd6/0x4d0 tcp_close+0xf4/0x430 ? tcp_check_oom+0xf0/0xf0 tls_sk_proto_close+0xe4/0x1e0 [tls] inet_release+0x36/0x60 __sock_release+0x37/0xa0 sock_close+0x11/0x20 __fput+0xa2/0x1d0 task_work_run+0x89/0xb0 exit_to_usermode_loop+0x9a/0xa0 do_syscall_64+0xc0/0xf0 entry_SYSCALL_64_after_hwframe+0x44/0xa9 Let's leave the second unclone conditional, as I'm not entirely sure what is its purpose :) Fixes: 4e485d06bb8c ("strparser: Call skb_unclone conditionally") Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> Reviewed-by: Dirk van der Merwe <dirk.vandermerwe@netronome.com> --- net/strparser/strparser.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)