| Message ID | 20220102081253.9123-1-gal@nvidia.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC] net/tls: Fix skb memory leak when running kTLS traffic | expand |
On Sun, 2 Jan 2022 10:12:53 +0200 Gal Pressman wrote: > The cited Fixes commit introduced a memory leak when running kTLS > traffic (with/without hardware offloads). > I'm running nginx on the server side and wrk on the client side and get > the following: > > unreferenced object 0xffff8881935e9b80 (size 224): > comm "softirq", pid 0, jiffies 4294903611 (age 43.204s) > hex dump (first 32 bytes): > 80 9b d0 36 81 88 ff ff 00 00 00 00 00 00 00 00 ...6............ > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > backtrace: > [<00000000efe2a999>] build_skb+0x1f/0x170 > [<00000000ef521785>] mlx5e_skb_from_cqe_mpwrq_linear+0x2bc/0x610 [mlx5_core] > [<00000000945d0ffe>] mlx5e_handle_rx_cqe_mpwrq+0x264/0x9e0 [mlx5_core] > [<00000000cb675b06>] mlx5e_poll_rx_cq+0x3ad/0x17a0 [mlx5_core] > [<0000000018aac6a9>] mlx5e_napi_poll+0x28c/0x1b60 [mlx5_core] > [<000000001f3369d1>] __napi_poll+0x9f/0x560 > [<00000000cfa11f72>] net_rx_action+0x357/0xa60 > [<000000008653b8d7>] __do_softirq+0x282/0x94e > [<00000000644923c6>] __irq_exit_rcu+0x11f/0x170 > [<00000000d4085f8f>] irq_exit_rcu+0xa/0x20 > [<00000000d412fef4>] common_interrupt+0x7d/0xa0 > [<00000000bfb0cebc>] asm_common_interrupt+0x1e/0x40 > [<00000000d80d0890>] default_idle+0x53/0x70 > [<00000000f2b9780e>] default_idle_call+0x8c/0xd0 > [<00000000c7659e15>] do_idle+0x394/0x450 > > I'm not familiar with these areas of the code, but I've added this > sk_defer_free_flush() to tls_sw_recvmsg() based on a hunch and it > resolved the issue. > > Eric, do you think this is the correct fix? Maybe we're missing a call > to sk_defer_free_flush() in other places as well? Any thoughts, Eric? Since the merge window is coming soon should we purge the defer free queue when socket is destroyed at least? All the .read_sock callers will otherwise risk the leaks, it seems. > Fixes: f35f821935d8 ("tcp: defer skb freeing after socket lock is released") > Signed-off-by: Gal Pressman <gal@nvidia.com> > --- > net/tls/tls_sw.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c > index 3f271e29812f..95e774f1b91f 100644 > --- a/net/tls/tls_sw.c > +++ b/net/tls/tls_sw.c > @@ -1990,6 +1990,7 @@ int tls_sw_recvmsg(struct sock *sk, > > end: > release_sock(sk); > + sk_defer_free_flush(sk); > if (psock) > sk_psock_put(sk, psock); > return copied ? : err;
On Fri, Jan 7, 2022 at 10:51 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Sun, 2 Jan 2022 10:12:53 +0200 Gal Pressman wrote: > > The cited Fixes commit introduced a memory leak when running kTLS > > traffic (with/without hardware offloads). > > I'm running nginx on the server side and wrk on the client side and get > > the following: > > > > unreferenced object 0xffff8881935e9b80 (size 224): > > comm "softirq", pid 0, jiffies 4294903611 (age 43.204s) > > hex dump (first 32 bytes): > > 80 9b d0 36 81 88 ff ff 00 00 00 00 00 00 00 00 ...6............ > > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > > backtrace: > > [<00000000efe2a999>] build_skb+0x1f/0x170 > > [<00000000ef521785>] mlx5e_skb_from_cqe_mpwrq_linear+0x2bc/0x610 [mlx5_core] > > [<00000000945d0ffe>] mlx5e_handle_rx_cqe_mpwrq+0x264/0x9e0 [mlx5_core] > > [<00000000cb675b06>] mlx5e_poll_rx_cq+0x3ad/0x17a0 [mlx5_core] > > [<0000000018aac6a9>] mlx5e_napi_poll+0x28c/0x1b60 [mlx5_core] > > [<000000001f3369d1>] __napi_poll+0x9f/0x560 > > [<00000000cfa11f72>] net_rx_action+0x357/0xa60 > > [<000000008653b8d7>] __do_softirq+0x282/0x94e > > [<00000000644923c6>] __irq_exit_rcu+0x11f/0x170 > > [<00000000d4085f8f>] irq_exit_rcu+0xa/0x20 > > [<00000000d412fef4>] common_interrupt+0x7d/0xa0 > > [<00000000bfb0cebc>] asm_common_interrupt+0x1e/0x40 > > [<00000000d80d0890>] default_idle+0x53/0x70 > > [<00000000f2b9780e>] default_idle_call+0x8c/0xd0 > > [<00000000c7659e15>] do_idle+0x394/0x450 > > > > I'm not familiar with these areas of the code, but I've added this > > sk_defer_free_flush() to tls_sw_recvmsg() based on a hunch and it > > resolved the issue. > > > > Eric, do you think this is the correct fix? Maybe we're missing a call > > to sk_defer_free_flush() in other places as well? > > Any thoughts, Eric? Since the merge window is coming soon should > we purge the defer free queue when socket is destroyed at least? > All the .read_sock callers will otherwise risk the leaks, it seems. It seems I missed this patch. We might merge it, and eventually add another WARN_ON_ONCE(!llist_empty(sk->defer_list)) sk_defer_free_flush(sk); at socket destroy as you suggested ? Reviewed-by: Eric Dumazet <edumazet@google.com> > > > Fixes: f35f821935d8 ("tcp: defer skb freeing after socket lock is released") > > Signed-off-by: Gal Pressman <gal@nvidia.com> > > --- > > net/tls/tls_sw.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c > > index 3f271e29812f..95e774f1b91f 100644 > > --- a/net/tls/tls_sw.c > > +++ b/net/tls/tls_sw.c > > @@ -1990,6 +1990,7 @@ int tls_sw_recvmsg(struct sock *sk, > > > > end: > > release_sock(sk); > > + sk_defer_free_flush(sk); > > if (psock) > > sk_psock_put(sk, psock); > > return copied ? : err; >
On Fri, 7 Jan 2022 11:12:28 -0800 Eric Dumazet wrote: > On Fri, Jan 7, 2022 at 10:51 AM Jakub Kicinski <kuba@kernel.org> wrote: > > > > On Sun, 2 Jan 2022 10:12:53 +0200 Gal Pressman wrote: > > > The cited Fixes commit introduced a memory leak when running kTLS > > > traffic (with/without hardware offloads). > > > I'm running nginx on the server side and wrk on the client side and get > > > the following: > > > > > > unreferenced object 0xffff8881935e9b80 (size 224): > > > comm "softirq", pid 0, jiffies 4294903611 (age 43.204s) > > > hex dump (first 32 bytes): > > > 80 9b d0 36 81 88 ff ff 00 00 00 00 00 00 00 00 ...6............ > > > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > > > backtrace: > > > [<00000000efe2a999>] build_skb+0x1f/0x170 > > > [<00000000ef521785>] mlx5e_skb_from_cqe_mpwrq_linear+0x2bc/0x610 [mlx5_core] > > > [<00000000945d0ffe>] mlx5e_handle_rx_cqe_mpwrq+0x264/0x9e0 [mlx5_core] > > > [<00000000cb675b06>] mlx5e_poll_rx_cq+0x3ad/0x17a0 [mlx5_core] > > > [<0000000018aac6a9>] mlx5e_napi_poll+0x28c/0x1b60 [mlx5_core] > > > [<000000001f3369d1>] __napi_poll+0x9f/0x560 > > > [<00000000cfa11f72>] net_rx_action+0x357/0xa60 > > > [<000000008653b8d7>] __do_softirq+0x282/0x94e > > > [<00000000644923c6>] __irq_exit_rcu+0x11f/0x170 > > > [<00000000d4085f8f>] irq_exit_rcu+0xa/0x20 > > > [<00000000d412fef4>] common_interrupt+0x7d/0xa0 > > > [<00000000bfb0cebc>] asm_common_interrupt+0x1e/0x40 > > > [<00000000d80d0890>] default_idle+0x53/0x70 > > > [<00000000f2b9780e>] default_idle_call+0x8c/0xd0 > > > [<00000000c7659e15>] do_idle+0x394/0x450 > > > > > > I'm not familiar with these areas of the code, but I've added this > > > sk_defer_free_flush() to tls_sw_recvmsg() based on a hunch and it > > > resolved the issue. > > > > > > Eric, do you think this is the correct fix? Maybe we're missing a call > > > to sk_defer_free_flush() in other places as well? > > > > Any thoughts, Eric? Since the merge window is coming soon should > > we purge the defer free queue when socket is destroyed at least? > > All the .read_sock callers will otherwise risk the leaks, it seems. > > It seems I missed this patch. > > We might merge it, and eventually add another > > WARN_ON_ONCE(!llist_empty(sk->defer_list)) > sk_defer_free_flush(sk); > > at socket destroy as you suggested ? > > Reviewed-by: Eric Dumazet <edumazet@google.com> Thanks, applied! Gal please follow up as suggested, for TLS similar treatment to what you have done here will be necessary in the splice_read handler.
On 08/01/2022 04:44, Jakub Kicinski wrote: > On Fri, 7 Jan 2022 11:12:28 -0800 Eric Dumazet wrote: >> On Fri, Jan 7, 2022 at 10:51 AM Jakub Kicinski <kuba@kernel.org> wrote: >>> On Sun, 2 Jan 2022 10:12:53 +0200 Gal Pressman wrote: >>>> The cited Fixes commit introduced a memory leak when running kTLS >>>> traffic (with/without hardware offloads). >>>> I'm running nginx on the server side and wrk on the client side and get >>>> the following: >>>> >>>> unreferenced object 0xffff8881935e9b80 (size 224): >>>> comm "softirq", pid 0, jiffies 4294903611 (age 43.204s) >>>> hex dump (first 32 bytes): >>>> 80 9b d0 36 81 88 ff ff 00 00 00 00 00 00 00 00 ...6............ >>>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ >>>> backtrace: >>>> [<00000000efe2a999>] build_skb+0x1f/0x170 >>>> [<00000000ef521785>] mlx5e_skb_from_cqe_mpwrq_linear+0x2bc/0x610 [mlx5_core] >>>> [<00000000945d0ffe>] mlx5e_handle_rx_cqe_mpwrq+0x264/0x9e0 [mlx5_core] >>>> [<00000000cb675b06>] mlx5e_poll_rx_cq+0x3ad/0x17a0 [mlx5_core] >>>> [<0000000018aac6a9>] mlx5e_napi_poll+0x28c/0x1b60 [mlx5_core] >>>> [<000000001f3369d1>] __napi_poll+0x9f/0x560 >>>> [<00000000cfa11f72>] net_rx_action+0x357/0xa60 >>>> [<000000008653b8d7>] __do_softirq+0x282/0x94e >>>> [<00000000644923c6>] __irq_exit_rcu+0x11f/0x170 >>>> [<00000000d4085f8f>] irq_exit_rcu+0xa/0x20 >>>> [<00000000d412fef4>] common_interrupt+0x7d/0xa0 >>>> [<00000000bfb0cebc>] asm_common_interrupt+0x1e/0x40 >>>> [<00000000d80d0890>] default_idle+0x53/0x70 >>>> [<00000000f2b9780e>] default_idle_call+0x8c/0xd0 >>>> [<00000000c7659e15>] do_idle+0x394/0x450 >>>> >>>> I'm not familiar with these areas of the code, but I've added this >>>> sk_defer_free_flush() to tls_sw_recvmsg() based on a hunch and it >>>> resolved the issue. >>>> >>>> Eric, do you think this is the correct fix? Maybe we're missing a call >>>> to sk_defer_free_flush() in other places as well? >>> Any thoughts, Eric? Since the merge window is coming soon should >>> we purge the defer free queue when socket is destroyed at least? >>> All the .read_sock callers will otherwise risk the leaks, it seems. >> It seems I missed this patch. >> >> We might merge it, and eventually add another >> >> WARN_ON_ONCE(!llist_empty(sk->defer_list)) >> sk_defer_free_flush(sk); >> >> at socket destroy as you suggested ? >> >> Reviewed-by: Eric Dumazet <edumazet@google.com> > Thanks, applied! > > Gal please follow up as suggested, for TLS similar treatment to what > you have done here will be necessary in the splice_read handler. Thanks Eric and Jakub! So you want one patch that adds a sk_defer_free_flush() call in tls_sw_splice_read(), and a second one that adds the WARN_ON_ONCE to sk_free()?
On Sun, 9 Jan 2022 13:49:42 +0200 Gal Pressman wrote: > So you want one patch that adds a sk_defer_free_flush() call in > tls_sw_splice_read(), and a second one that adds the WARN_ON_ONCE to > sk_free()? Two separate patches is probably the best way to go.
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index 3f271e29812f..95e774f1b91f 100644 --- a/net/tls/tls_sw.c +++ b/net/tls/tls_sw.c @@ -1990,6 +1990,7 @@ int tls_sw_recvmsg(struct sock *sk, end: release_sock(sk); + sk_defer_free_flush(sk); if (psock) sk_psock_put(sk, psock); return copied ? : err;
The cited Fixes commit introduced a memory leak when running kTLS traffic (with/without hardware offloads). I'm running nginx on the server side and wrk on the client side and get the following: unreferenced object 0xffff8881935e9b80 (size 224): comm "softirq", pid 0, jiffies 4294903611 (age 43.204s) hex dump (first 32 bytes): 80 9b d0 36 81 88 ff ff 00 00 00 00 00 00 00 00 ...6............ 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace: [<00000000efe2a999>] build_skb+0x1f/0x170 [<00000000ef521785>] mlx5e_skb_from_cqe_mpwrq_linear+0x2bc/0x610 [mlx5_core] [<00000000945d0ffe>] mlx5e_handle_rx_cqe_mpwrq+0x264/0x9e0 [mlx5_core] [<00000000cb675b06>] mlx5e_poll_rx_cq+0x3ad/0x17a0 [mlx5_core] [<0000000018aac6a9>] mlx5e_napi_poll+0x28c/0x1b60 [mlx5_core] [<000000001f3369d1>] __napi_poll+0x9f/0x560 [<00000000cfa11f72>] net_rx_action+0x357/0xa60 [<000000008653b8d7>] __do_softirq+0x282/0x94e [<00000000644923c6>] __irq_exit_rcu+0x11f/0x170 [<00000000d4085f8f>] irq_exit_rcu+0xa/0x20 [<00000000d412fef4>] common_interrupt+0x7d/0xa0 [<00000000bfb0cebc>] asm_common_interrupt+0x1e/0x40 [<00000000d80d0890>] default_idle+0x53/0x70 [<00000000f2b9780e>] default_idle_call+0x8c/0xd0 [<00000000c7659e15>] do_idle+0x394/0x450 I'm not familiar with these areas of the code, but I've added this sk_defer_free_flush() to tls_sw_recvmsg() based on a hunch and it resolved the issue. Eric, do you think this is the correct fix? Maybe we're missing a call to sk_defer_free_flush() in other places as well? Thanks! Fixes: f35f821935d8 ("tcp: defer skb freeing after socket lock is released") Signed-off-by: Gal Pressman <gal@nvidia.com> --- net/tls/tls_sw.c | 1 + 1 file changed, 1 insertion(+)