* [PATCH 1/2] netfilter: fix a memory leak in nf_conntrack_in @ 2019-10-08 5:35 Maciej Żenczykowski 2019-10-08 5:35 ` [PATCH 2/2] netfilter: revert "conntrack: silent a memory leak warning" Maciej Żenczykowski ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Maciej Żenczykowski @ 2019-10-08 5:35 UTC (permalink / raw) To: Maciej Żenczykowski, David S . Miller Cc: netdev, Cong Wang, Eric Dumazet, Pablo Neira Ayuso From: Maciej Żenczykowski <maze@google.com> Cc: Cong Wang <xiyou.wangcong@gmail.com> Cc: Eric Dumazet <edumazet@google.com> Cc: Pablo Neira Ayuso <pablo@netfilter.org> Signed-off-by: Maciej Żenczykowski <maze@google.com> --- net/netfilter/nf_conntrack_core.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c index 0c63120b2db2..35459d04a050 100644 --- a/net/netfilter/nf_conntrack_core.c +++ b/net/netfilter/nf_conntrack_core.c @@ -1679,7 +1679,8 @@ nf_conntrack_in(struct sk_buff *skb, const struct nf_hook_state *state) if ((tmpl && !nf_ct_is_template(tmpl)) || ctinfo == IP_CT_UNTRACKED) { NF_CT_STAT_INC_ATOMIC(state->net, ignore); - return NF_ACCEPT; + ret = NF_ACCEPT; + goto out; } skb->_nfct = 0; } -- 2.23.0.581.g78d2f28ef7-goog ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] netfilter: revert "conntrack: silent a memory leak warning" 2019-10-08 5:35 [PATCH 1/2] netfilter: fix a memory leak in nf_conntrack_in Maciej Żenczykowski @ 2019-10-08 5:35 ` Maciej Żenczykowski 2019-10-08 5:38 ` Maciej Żenczykowski ` (2 more replies) 2019-10-08 6:01 ` [PATCH 1/2] netfilter: fix a memory leak in nf_conntrack_in Florian Westphal 2019-10-08 6:05 ` Cong Wang 2 siblings, 3 replies; 11+ messages in thread From: Maciej Żenczykowski @ 2019-10-08 5:35 UTC (permalink / raw) To: Maciej Żenczykowski, David S . Miller Cc: netdev, Cong Wang, Eric Dumazet, Pablo Neira Ayuso From: Maciej Żenczykowski <maze@google.com> This reverts commit 114aa35d06d4920c537b72f9fa935de5dd205260. By my understanding of kmemleak the reasoning for this patch is incorrect. If kmemleak couldn't handle rcu we'd have it reporting leaks all over the place. My belief is that this was instead papering over a real leak. Cc: Cong Wang <xiyou.wangcong@gmail.com> Cc: Eric Dumazet <edumazet@google.com> Cc: Pablo Neira Ayuso <pablo@netfilter.org> Signed-off-by: Maciej Żenczykowski <maze@google.com> --- net/netfilter/nf_conntrack_extend.c | 1 - 1 file changed, 1 deletion(-) diff --git a/net/netfilter/nf_conntrack_extend.c b/net/netfilter/nf_conntrack_extend.c index d4ed1e197921..fb208877338a 100644 --- a/net/netfilter/nf_conntrack_extend.c +++ b/net/netfilter/nf_conntrack_extend.c @@ -68,7 +68,6 @@ void *nf_ct_ext_add(struct nf_conn *ct, enum nf_ct_ext_id id, gfp_t gfp) rcu_read_unlock(); alloc = max(newlen, NF_CT_EXT_PREALLOC); - kmemleak_not_leak(old); new = __krealloc(old, alloc, gfp); if (!new) return NULL; -- 2.23.0.581.g78d2f28ef7-goog ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] netfilter: revert "conntrack: silent a memory leak warning" 2019-10-08 5:35 ` [PATCH 2/2] netfilter: revert "conntrack: silent a memory leak warning" Maciej Żenczykowski @ 2019-10-08 5:38 ` Maciej Żenczykowski 2019-10-08 5:58 ` Cong Wang 2019-10-08 6:04 ` Florian Westphal 2 siblings, 0 replies; 11+ messages in thread From: Maciej Żenczykowski @ 2019-10-08 5:38 UTC (permalink / raw) To: Maciej Żenczykowski, David S . Miller Cc: Linux NetDev, Cong Wang, Eric Dumazet, Pablo Neira Ayuso Please think both these patches through. I'm not going to claim I'm 100% certain of their correctness. I'm confused by: include/net/netfilter/nf_conntrack.h:65: * beware nf_ct_get() is different and don't inc refcnt. and maybe there's some subtlety to this krealloc+rcu+kmemleak thing I'm missing. On Mon, Oct 7, 2019 at 10:35 PM Maciej Żenczykowski <zenczykowski@gmail.com> wrote: > > From: Maciej Żenczykowski <maze@google.com> > > This reverts commit 114aa35d06d4920c537b72f9fa935de5dd205260. > > By my understanding of kmemleak the reasoning for this patch > is incorrect. If kmemleak couldn't handle rcu we'd have it > reporting leaks all over the place. My belief is that this > was instead papering over a real leak. > > Cc: Cong Wang <xiyou.wangcong@gmail.com> > Cc: Eric Dumazet <edumazet@google.com> > Cc: Pablo Neira Ayuso <pablo@netfilter.org> > Signed-off-by: Maciej Żenczykowski <maze@google.com> > --- > net/netfilter/nf_conntrack_extend.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/net/netfilter/nf_conntrack_extend.c b/net/netfilter/nf_conntrack_extend.c > index d4ed1e197921..fb208877338a 100644 > --- a/net/netfilter/nf_conntrack_extend.c > +++ b/net/netfilter/nf_conntrack_extend.c > @@ -68,7 +68,6 @@ void *nf_ct_ext_add(struct nf_conn *ct, enum nf_ct_ext_id id, gfp_t gfp) > rcu_read_unlock(); > > alloc = max(newlen, NF_CT_EXT_PREALLOC); > - kmemleak_not_leak(old); > new = __krealloc(old, alloc, gfp); > if (!new) > return NULL; > -- > 2.23.0.581.g78d2f28ef7-goog > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] netfilter: revert "conntrack: silent a memory leak warning" 2019-10-08 5:35 ` [PATCH 2/2] netfilter: revert "conntrack: silent a memory leak warning" Maciej Żenczykowski 2019-10-08 5:38 ` Maciej Żenczykowski @ 2019-10-08 5:58 ` Cong Wang 2019-10-08 6:04 ` Florian Westphal 2 siblings, 0 replies; 11+ messages in thread From: Cong Wang @ 2019-10-08 5:58 UTC (permalink / raw) To: Maciej Żenczykowski Cc: Maciej Żenczykowski, David S . Miller, Linux Kernel Network Developers, Eric Dumazet, Pablo Neira Ayuso On Mon, Oct 7, 2019 at 10:35 PM Maciej Żenczykowski <zenczykowski@gmail.com> wrote: > > From: Maciej Żenczykowski <maze@google.com> > > This reverts commit 114aa35d06d4920c537b72f9fa935de5dd205260. > > By my understanding of kmemleak the reasoning for this patch > is incorrect. If kmemleak couldn't handle rcu we'd have it > reporting leaks all over the place. My belief is that this > was instead papering over a real leak. If you have this belief, please: 1. Explain in details why you believe it or what supports it. For example, did you see any kernel memory continue growing? 2. You want to fix the comment right above __krealloc() before fixing anything else. Thanks. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] netfilter: revert "conntrack: silent a memory leak warning" 2019-10-08 5:35 ` [PATCH 2/2] netfilter: revert "conntrack: silent a memory leak warning" Maciej Żenczykowski 2019-10-08 5:38 ` Maciej Żenczykowski 2019-10-08 5:58 ` Cong Wang @ 2019-10-08 6:04 ` Florian Westphal 2019-10-08 6:45 ` Maciej Żenczykowski 2 siblings, 1 reply; 11+ messages in thread From: Florian Westphal @ 2019-10-08 6:04 UTC (permalink / raw) To: Maciej Żenczykowski Cc: Maciej Żenczykowski, David S . Miller, netdev, Cong Wang, Eric Dumazet, Pablo Neira Ayuso Maciej Żenczykowski <zenczykowski@gmail.com> wrote: > From: Maciej Żenczykowski <maze@google.com> > > This reverts commit 114aa35d06d4920c537b72f9fa935de5dd205260. > > By my understanding of kmemleak the reasoning for this patch > is incorrect. If kmemleak couldn't handle rcu we'd have it > reporting leaks all over the place. My belief is that this > was instead papering over a real leak. Perhaps, but note that this is related to nfct->ext, not nfct itself. I think we could remove __krealloc and use krealloc directly with a bit of changes in the nf_conntrack core to make sure we do not access nfct->ext without holding a reference to nfct, and then drop rcu protection of nfct->ext, I don't think its strictly required anymore. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] netfilter: revert "conntrack: silent a memory leak warning" 2019-10-08 6:04 ` Florian Westphal @ 2019-10-08 6:45 ` Maciej Żenczykowski 2019-10-08 7:10 ` Maciej Żenczykowski 0 siblings, 1 reply; 11+ messages in thread From: Maciej Żenczykowski @ 2019-10-08 6:45 UTC (permalink / raw) To: Florian Westphal Cc: David S . Miller, Linux NetDev, Cong Wang, Eric Dumazet, Pablo Neira Ayuso Right, so I'm not claiming I'm following what's happening here. This is on a 4.14 LTS + stuff android device kernel... which isn't exactly trustworthy... backporting this patch (ie. the one I proposed reverting) would potentially fix things, but I don't really understand why we need this kmemleak_not_leak. comment#5, 1) kmemleak_U3_8.txt unreferenced object 0xffffffe9b1246d80 (size 320): comm "Chrome_ChildIOT", pid 14510, jiffies 4296946994 (age 6554.257s) hex dump (first 32 bytes): 00 00 00 00 8a 00 8a 00 00 00 76 00 04 00 00 00 ..........v..... 03 00 00 80 00 00 00 00 00 02 00 00 00 00 ad de ................ backtrace: [<00000000e8443b2a>] __nf_conntrack_alloc+0x68/0x154 [<00000000ad74cb6d>] init_conntrack+0xec/0x870 <------------------- caller [<0000000021ec0fc5>] nf_conntrack_in+0x5a8/0xc20 [<0000000037b289f7>] ipv6_conntrack_local+0x58/0x64 [<00000000fb301f7c>] nf_hook_slow+0x84/0x124 [<00000000cada0355>] ip6_xmit+0x580/0xaec [<00000000f35a7b78>] inet6_csk_xmit+0xc0/0x12c [<00000000c6e68096>] __tcp_transmit_skb+0x8e0/0xd64 [<00000000ab150d11>] tcp_connect+0x888/0x123c [<00000000311815d4>] tcp_v6_connect+0x7c8/0x81c [<000000005abc5c46>] __inet_stream_connect+0xb8/0x3e4 [<000000001036b35e>] inet_stream_connect+0x48/0x70 [<00000000463ca3cd>] SyS_connect+0x138/0x1ec [<0000000048eb8c0e>] __sys_trace_return+0x0/0x4 [<00000000a87abfc9>] 0xffffffffffffffff unreferenced object 0xffffffeac8de7e00 (size 128): comm "Chrome_ChildIOT", pid 14510, jiffies 4296946994 (age 6554.257s) hex dump (first 32 bytes): 69 6f 6e 2d 73 79 73 74 65 6d 2d 36 30 32 2d 61 ion-system-602-a 6c 6c 6f 63 61 74 6f 72 2d 73 65 72 76 69 00 de llocator-servi.. backtrace: [<00000000620a1869>] __krealloc+0xc0/0x134 [<000000007c526d10>] nf_ct_ext_add+0xd0/0x1b8 [<00000000e7d08252>] init_conntrack+0x2e8/0x870 [<0000000021ec0fc5>] nf_conntrack_in+0x5a8/0xc20 [<0000000037b289f7>] ipv6_conntrack_local+0x58/0x64 [<00000000fb301f7c>] nf_hook_slow+0x84/0x124 [<00000000cada0355>] ip6_xmit+0x580/0xaec [<00000000f35a7b78>] inet6_csk_xmit+0xc0/0x12c [<00000000c6e68096>] __tcp_transmit_skb+0x8e0/0xd64 [<00000000ab150d11>] tcp_connect+0x888/0x123c [<00000000311815d4>] tcp_v6_connect+0x7c8/0x81c [<000000005abc5c46>] __inet_stream_connect+0xb8/0x3e4 [<000000001036b35e>] inet_stream_connect+0x48/0x70 [<00000000463ca3cd>] SyS_connect+0x138/0x1ec [<0000000048eb8c0e>] __sys_trace_return+0x0/0x4 [<00000000a87abfc9>] 0xffffffffffffffff comment#5, 2) kmemleak_U3_24.txt private/msm-google/net/netfilter/nf_conntrack_core.c unreferenced object 0xffffffca447d0780 (size 320): comm "NetworkService", pid 11981, jiffies 4296499821 (age 639.230s) hex dump (first 32 bytes): 01 00 00 00 11 00 11 00 04 00 67 70 00 00 00 00 ..........gp.... 6f 53 00 00 00 00 00 00 b8 4d 89 e4 cb ff ff ff oS.......M...... backtrace: [<00000000be8a10e5>] __nf_conntrack_alloc+0x68/0x154 [<0000000086bc83ff>] init_conntrack+0xec/0x870 <---------------- caller [<00000000a2e722fc>] nf_conntrack_in+0x5a8/0xc20 [<0000000092c99949>] ipv4_conntrack_local+0x110/0x158 [<00000000cbc301a4>] nf_hook_slow+0x84/0x124 [<00000000ca439d6c>] __ip_local_out+0xf8/0x16c [<000000004ee66db3>] ip_queue_xmit+0x3f8/0x510 [<00000000a7155c11>] __tcp_transmit_skb+0x8e0/0xd64 [<000000009f5bf497>] tcp_connect+0x888/0x123c [<00000000b593e23f>] tcp_v4_connect+0x69c/0x7e8 [<000000009cf8bbe8>] __inet_stream_connect+0xb8/0x3e4 [<00000000dd889c48>] inet_stream_connect+0x48/0x70 [<00000000f8db9417>] SyS_connect+0x138/0x1ec [<00000000f587c037>] __sys_trace_return+0x0/0x4 [<00000000a409c2be>] 0xffffffffffffffff unreferenced object 0xffffffcb46ad9380 (size 128): comm "NetworkService", pid 11981, jiffies 4296499821 (age 639.230s) hex dump (first 32 bytes): 00 00 00 00 00 00 00 00 00 02 00 00 00 00 ad de ................ 00 00 00 00 18 30 00 00 00 00 00 00 00 00 00 00 .....0.......... backtrace: [<000000007102ad50>] __krealloc+0xc0/0x134 [<00000000274813fb>] nf_ct_ext_add+0xd0/0x1b8 [<0000000029bb07b0>] init_conntrack+0x2e8/0x870 <------------ caller [<00000000a2e722fc>] nf_conntrack_in+0x5a8/0xc20 [<0000000092c99949>] ipv4_conntrack_local+0x110/0x158 [<00000000cbc301a4>] nf_hook_slow+0x84/0x124 [<00000000ca439d6c>] __ip_local_out+0xf8/0x16c [<000000004ee66db3>] ip_queue_xmit+0x3f8/0x510 [<00000000a7155c11>] __tcp_transmit_skb+0x8e0/0xd64 [<000000009f5bf497>] tcp_connect+0x888/0x123c [<00000000b593e23f>] tcp_v4_connect+0x69c/0x7e8 [<000000009cf8bbe8>] __inet_stream_connect+0xb8/0x3e4 [<00000000dd889c48>] inet_stream_connect+0x48/0x70 [<00000000f8db9417>] SyS_connect+0x138/0x1ec [<00000000f587c037>] __sys_trace_return+0x0/0x4 [<00000000a409c2be>] 0xffffffffffffffff --- and another one from a different device, but also 4.14 lts unreferenced object 0xfffffffbd5d2a180 (size 320): comm "GCMWriter", pid 12045, jiffies 4296160038 (age 105.074s) hex dump (first 32 bytes): 01 00 00 00 0f 00 0f 00 05 00 fa ef bf ff ff ff ................ 81 f4 01 00 00 00 00 00 00 d2 6f e6 fc ff ff ff ..........o..... backtrace: [<00000000274ac54c>] kmem_cache_alloc+0x188/0x2ac [<00000000b8014b3d>] __nf_conntrack_alloc+0x68/0x144 [<0000000049a70b0e>] init_conntrack+0xc0/0x4a0 [<000000009e67654a>] nf_conntrack_in+0x2b4/0x7b4 [<00000000ca4d0a3e>] ipv4_conntrack_local+0xa4/0xac [<0000000081d01fa2>] nf_hook_slow+0x4c/0xdc [<0000000097b73dfa>] __ip_local_out+0xfc/0x140 [<00000000b21eaa8e>] ip_queue_xmit+0x344/0x3a0 [<00000000b24cb740>] __tcp_transmit_skb+0x710/0x9c8 [<00000000feb142b2>] tcp_connect+0x9d8/0xc74 [<000000007f161285>] tcp_v4_connect+0x388/0x3fc [<00000000adf5f95c>] tcp_v6_connect+0x254/0x4e0 [<0000000072d33635>] __inet_stream_connect+0x90/0x2d8 [<0000000058fab473>] inet_stream_connect+0x48/0x70 [<00000000da64280e>] SyS_connect+0xb8/0x134 [<0000000044b2efde>] __sys_trace_return+0x0/0x4 unreferenced object 0xfffffffce5cc4a00 (size 128): comm "GCMWriter", pid 12045, jiffies 4296160038 (age 105.074s) hex dump (first 32 bytes): 80 45 cc e5 fc ff ff ff 00 00 00 00 00 00 00 00 .E.............. 00 00 00 00 18 30 00 00 00 00 00 00 00 00 00 00 .....0.......... backtrace: [<00000000eb5f9abf>] __kmalloc_track_caller+0x1b4/0x314 [<0000000093176ee9>] __krealloc+0x54/0xa8 [<000000006dcdc738>] nf_ct_ext_add+0xac/0x13c [<00000000afc95893>] init_conntrack+0x278/0x4a0 [<000000009e67654a>] nf_conntrack_in+0x2b4/0x7b4 [<00000000ca4d0a3e>] ipv4_conntrack_local+0xa4/0xac [<0000000081d01fa2>] nf_hook_slow+0x4c/0xdc [<0000000097b73dfa>] __ip_local_out+0xfc/0x140 [<00000000b21eaa8e>] ip_queue_xmit+0x344/0x3a0 [<00000000b24cb740>] __tcp_transmit_skb+0x710/0x9c8 [<00000000feb142b2>] tcp_connect+0x9d8/0xc74 [<000000007f161285>] tcp_v4_connect+0x388/0x3fc [<00000000adf5f95c>] tcp_v6_connect+0x254/0x4e0 [<0000000072d33635>] __inet_stream_connect+0x90/0x2d8 [<0000000058fab473>] inet_stream_connect+0x48/0x70 [<00000000da64280e>] SyS_connect+0xb8/0x134 On Mon, Oct 7, 2019 at 11:04 PM Florian Westphal <fw@strlen.de> wrote: > > Maciej Żenczykowski <zenczykowski@gmail.com> wrote: > > From: Maciej Żenczykowski <maze@google.com> > > > > This reverts commit 114aa35d06d4920c537b72f9fa935de5dd205260. > > > > By my understanding of kmemleak the reasoning for this patch > > is incorrect. If kmemleak couldn't handle rcu we'd have it > > reporting leaks all over the place. My belief is that this > > was instead papering over a real leak. > > Perhaps, but note that this is related to nfct->ext, not nfct itself. > > I think we could remove __krealloc and use krealloc directly with > a bit of changes in the nf_conntrack core to make sure we do not > access nfct->ext without holding a reference to nfct, and then drop > rcu protection of nfct->ext, I don't think its strictly required anymore. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] netfilter: revert "conntrack: silent a memory leak warning" 2019-10-08 6:45 ` Maciej Żenczykowski @ 2019-10-08 7:10 ` Maciej Żenczykowski 2019-10-10 18:36 ` Cong Wang 0 siblings, 1 reply; 11+ messages in thread From: Maciej Żenczykowski @ 2019-10-08 7:10 UTC (permalink / raw) To: Florian Westphal Cc: David S . Miller, Linux NetDev, Cong Wang, Eric Dumazet, Pablo Neira Ayuso Here's my reasoning: old = ct->ext; //... stuff that doesn't change old. alloc = max(newlen, NF_CT_EXT_PREALLOC); <-- will be >= 128, so not zero kmemleak_not_leak(old); new = __krealloc(old, alloc, gfp); if (!new) return NULL; <--- if we return here, ct->ext still holds old, so no leak. if (!old) { memset(new->offset, 0, sizeof(new->offset)); ct->ext = new; <--- old is NULL so can't leak } else if (new != old) { kfree_rcu(old, rcu); <-- we free old, so doesn't leak rcu_assign_pointer(ct->ext, new); } <--- else new == old && it's still in ct->ext, so it doesn't leak Basically AFAICT our use of __krealloc() is exactly like krealloc() except instead of kfree() we do kfree_rcu(). And thus I don't understand the need for kmemleak_not_leak(old). So... what's my mistake? ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] netfilter: revert "conntrack: silent a memory leak warning" 2019-10-08 7:10 ` Maciej Żenczykowski @ 2019-10-10 18:36 ` Cong Wang 2019-10-17 18:30 ` Maciej Żenczykowski 0 siblings, 1 reply; 11+ messages in thread From: Cong Wang @ 2019-10-10 18:36 UTC (permalink / raw) To: Maciej Żenczykowski Cc: Florian Westphal, David S . Miller, Linux NetDev, Eric Dumazet, Pablo Neira Ayuso On Tue, Oct 8, 2019 at 12:10 AM Maciej Żenczykowski <zenczykowski@gmail.com> wrote: > > Here's my reasoning: > > old = ct->ext; > > //... stuff that doesn't change old. > > alloc = max(newlen, NF_CT_EXT_PREALLOC); <-- will be >= 128, > so not zero > kmemleak_not_leak(old); > new = __krealloc(old, alloc, gfp); > if (!new) > return NULL; <--- if we return here, ct->ext still > holds old, so no leak. > > if (!old) { > memset(new->offset, 0, sizeof(new->offset)); > ct->ext = new; <--- old is NULL so can't leak > } else if (new != old) { > kfree_rcu(old, rcu); <-- we free old, so doesn't leak > rcu_assign_pointer(ct->ext, new); > } <--- else new == old && it's still in ct->ext, so it doesn't leak > So you conclude as it is not leak too? Then what are you trying to fix? I am becoming more confused after this. :-/ > Basically AFAICT our use of __krealloc() is exactly like krealloc() > except instead of kfree() we do kfree_rcu(). > > And thus I don't understand the need for kmemleak_not_leak(old). kfree_rcu() is a callback deferred after a grace period, so if we allocate the memory again before that callback, it is reported to kmemleak as a memory leak unless we mark it as not, right? Or kfree_rcu() works nicely with kmemleak which I am not aware of? Thanks. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] netfilter: revert "conntrack: silent a memory leak warning" 2019-10-10 18:36 ` Cong Wang @ 2019-10-17 18:30 ` Maciej Żenczykowski 0 siblings, 0 replies; 11+ messages in thread From: Maciej Żenczykowski @ 2019-10-17 18:30 UTC (permalink / raw) To: Cong Wang Cc: Florian Westphal, David S . Miller, Linux NetDev, Eric Dumazet, Pablo Neira Ayuso > So you conclude as it is not leak too? Then what are you trying to > fix? I conclude there is no easily *visible* leak. At least not at first glance - not with single threaded code. > I am becoming more confused after this. :-/ I think adding kmemleak_not_leak() is hiding the fact that there actually is a leak. I think the leak is far more subtle. Possibly some sort of race condition or something. I don't see it though. The rcu doesn't seem entirely kosher, but I know little about such things. And I think the leak is *still* here. After all kmemleak_not_leak is purely annotation. It doesn't fix any leaks, it just makes us not warn about them. > > Basically AFAICT our use of __krealloc() is exactly like krealloc() > > except instead of kfree() we do kfree_rcu(). > > > > And thus I don't understand the need for kmemleak_not_leak(old). > > kfree_rcu() is a callback deferred after a grace period, so if we > allocate the memory again before that callback, it is reported to > kmemleak as a memory leak unless we mark it as not, right? > > Or kfree_rcu() works nicely with kmemleak which I am not aware > of? We have kfree_rcu() all over the kernel, but there's very few kmemleak_not_leak's. I don't see how kfree_rcu() could not work nicely with kmemleak. If it didn't we'd have it reporting leaks all over the place... ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] netfilter: fix a memory leak in nf_conntrack_in 2019-10-08 5:35 [PATCH 1/2] netfilter: fix a memory leak in nf_conntrack_in Maciej Żenczykowski 2019-10-08 5:35 ` [PATCH 2/2] netfilter: revert "conntrack: silent a memory leak warning" Maciej Żenczykowski @ 2019-10-08 6:01 ` Florian Westphal 2019-10-08 6:05 ` Cong Wang 2 siblings, 0 replies; 11+ messages in thread From: Florian Westphal @ 2019-10-08 6:01 UTC (permalink / raw) To: Maciej Żenczykowski Cc: Maciej Żenczykowski, David S . Miller, netdev, Cong Wang, Eric Dumazet, Pablo Neira Ayuso Maciej Żenczykowski <zenczykowski@gmail.com> wrote: > From: Maciej Żenczykowski <maze@google.com> > > Cc: Cong Wang <xiyou.wangcong@gmail.com> > Cc: Eric Dumazet <edumazet@google.com> > Cc: Pablo Neira Ayuso <pablo@netfilter.org> > Signed-off-by: Maciej Żenczykowski <maze@google.com> > --- > net/netfilter/nf_conntrack_core.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c > index 0c63120b2db2..35459d04a050 100644 > --- a/net/netfilter/nf_conntrack_core.c > +++ b/net/netfilter/nf_conntrack_core.c > @@ -1679,7 +1679,8 @@ nf_conntrack_in(struct sk_buff *skb, const struct nf_hook_state *state) > if ((tmpl && !nf_ct_is_template(tmpl)) || > ctinfo == IP_CT_UNTRACKED) { > NF_CT_STAT_INC_ATOMIC(state->net, ignore); > - return NF_ACCEPT; > + ret = NF_ACCEPT; This looks wrong. > + goto out; This puts tmpl, causing underflow of skb->nfct. When we enter nf_conntrack_in and this branch, then 'tmpl' is already assigned to skb->nfct, it will be put when skb is free'd. nf_ct_get() doesn't increment the refcnt. tmpl only needs to be put in case of ... > } > skb->_nfct = 0; ...this. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] netfilter: fix a memory leak in nf_conntrack_in 2019-10-08 5:35 [PATCH 1/2] netfilter: fix a memory leak in nf_conntrack_in Maciej Żenczykowski 2019-10-08 5:35 ` [PATCH 2/2] netfilter: revert "conntrack: silent a memory leak warning" Maciej Żenczykowski 2019-10-08 6:01 ` [PATCH 1/2] netfilter: fix a memory leak in nf_conntrack_in Florian Westphal @ 2019-10-08 6:05 ` Cong Wang 2 siblings, 0 replies; 11+ messages in thread From: Cong Wang @ 2019-10-08 6:05 UTC (permalink / raw) To: Maciej Żenczykowski Cc: Maciej Żenczykowski, David S . Miller, Linux Kernel Network Developers, Eric Dumazet, Pablo Neira Ayuso On Mon, Oct 7, 2019 at 10:35 PM Maciej Żenczykowski <zenczykowski@gmail.com> wrote: > > From: Maciej Żenczykowski <maze@google.com> Please, at least a simple copy-n-paste of kmemleak report will help a lot here. A changelog would save your time and mine too. Thanks. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-10-17 18:31 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-08 5:35 [PATCH 1/2] netfilter: fix a memory leak in nf_conntrack_in Maciej Żenczykowski 2019-10-08 5:35 ` [PATCH 2/2] netfilter: revert "conntrack: silent a memory leak warning" Maciej Żenczykowski 2019-10-08 5:38 ` Maciej Żenczykowski 2019-10-08 5:58 ` Cong Wang 2019-10-08 6:04 ` Florian Westphal 2019-10-08 6:45 ` Maciej Żenczykowski 2019-10-08 7:10 ` Maciej Żenczykowski 2019-10-10 18:36 ` Cong Wang 2019-10-17 18:30 ` Maciej Żenczykowski 2019-10-08 6:01 ` [PATCH 1/2] netfilter: fix a memory leak in nf_conntrack_in Florian Westphal 2019-10-08 6:05 ` Cong Wang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.