netdev - Re: [PATCH 2/2] netfilter: revert "conntrack: silent a memory leak warning"

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 linux-cve-announce 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:   Thu, 10 Oct 2019 11:36:30 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Maciej Żenczykowski <zenczykowski@...il.com>
Cc:     Florian Westphal <fw@...len.de>,
        "David S . Miller" <davem@...emloft.net>,
        Linux NetDev <netdev@...r.kernel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Pablo Neira Ayuso <pablo@...filter.org>
Subject: Re: [PATCH 2/2] netfilter: revert "conntrack: silent a memory leak warning"

On Tue, Oct 8, 2019 at 12:10 AM Maciej Żenczykowski
<zenczykowski@...il.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.