* [RFC PATCH] net/tls: Fix skb memory leak when running kTLS traffic
@ 2022-01-02 8:12 Gal Pressman
2022-01-07 18:51 ` Jakub Kicinski
0 siblings, 1 reply; 6+ messages in thread
From: Gal Pressman @ 2022-01-02 8:12 UTC (permalink / raw)
To: Eric Dumazet, Jakub Kicinski, David S. Miller
Cc: netdev, Tariq Toukan, Saeed Mahameed, Gal Pressman
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(+)
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;
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] net/tls: Fix skb memory leak when running kTLS traffic
2022-01-02 8:12 [RFC PATCH] net/tls: Fix skb memory leak when running kTLS traffic Gal Pressman
@ 2022-01-07 18:51 ` Jakub Kicinski
2022-01-07 19:12 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2022-01-07 18:51 UTC (permalink / raw)
To: Eric Dumazet
Cc: Gal Pressman, David S. Miller, netdev, Tariq Toukan, Saeed Mahameed
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;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] net/tls: Fix skb memory leak when running kTLS traffic
2022-01-07 18:51 ` Jakub Kicinski
@ 2022-01-07 19:12 ` Eric Dumazet
2022-01-08 2:44 ` Jakub Kicinski
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2022-01-07 19:12 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Gal Pressman, David S. Miller, netdev, Tariq Toukan, Saeed Mahameed
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;
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] net/tls: Fix skb memory leak when running kTLS traffic
2022-01-07 19:12 ` Eric Dumazet
@ 2022-01-08 2:44 ` Jakub Kicinski
2022-01-09 11:49 ` Gal Pressman
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2022-01-08 2:44 UTC (permalink / raw)
To: Eric Dumazet
Cc: Gal Pressman, David S. Miller, netdev, Tariq Toukan, Saeed Mahameed
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] net/tls: Fix skb memory leak when running kTLS traffic
2022-01-08 2:44 ` Jakub Kicinski
@ 2022-01-09 11:49 ` Gal Pressman
2022-01-09 21:16 ` Jakub Kicinski
0 siblings, 1 reply; 6+ messages in thread
From: Gal Pressman @ 2022-01-09 11:49 UTC (permalink / raw)
To: Jakub Kicinski, Eric Dumazet
Cc: David S. Miller, netdev, Tariq Toukan, Saeed Mahameed
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()?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] net/tls: Fix skb memory leak when running kTLS traffic
2022-01-09 11:49 ` Gal Pressman
@ 2022-01-09 21:16 ` Jakub Kicinski
0 siblings, 0 replies; 6+ messages in thread
From: Jakub Kicinski @ 2022-01-09 21:16 UTC (permalink / raw)
To: Gal Pressman
Cc: Eric Dumazet, David S. Miller, netdev, Tariq Toukan, Saeed Mahameed
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-01-09 21:16 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-02 8:12 [RFC PATCH] net/tls: Fix skb memory leak when running kTLS traffic Gal Pressman
2022-01-07 18:51 ` Jakub Kicinski
2022-01-07 19:12 ` Eric Dumazet
2022-01-08 2:44 ` Jakub Kicinski
2022-01-09 11:49 ` Gal Pressman
2022-01-09 21:16 ` Jakub Kicinski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).