netdev - Re: [RFC PATCH] net/tls: Fix skb memory leak when running kTLS traffic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 9 Jan 2022 13:49:42 +0200
From:   Gal Pressman <gal@...dia.com>
To:     Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>
Cc:     "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        Tariq Toukan <tariqt@...dia.com>,
        Saeed Mahameed <saeedm@...dia.com>
Subject: Re: [RFC PATCH] net/tls: Fix skb memory leak when running kTLS
 traffic


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@...nel.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@...gle.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()?