netdev - Re: [RFC -next 1/2] lib: add reference counting infrastructure

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]

Message-ID: <CANn89iJ8HLjjpBPyFOn3xTXSnOJCbOGq5gORgPnsws-+sB8ipA@mail.gmail.com>
Date:   Wed, 17 Nov 2021 12:16:15 -0800
From:   Eric Dumazet <edumazet@...gle.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Eric Dumazet <eric.dumazet@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>
Subject: Re: [RFC -next 1/2] lib: add reference counting infrastructure

On Wed, Nov 17, 2021 at 12:03 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Wed, 17 Nov 2021 11:20:30 -0800 Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@...gle.com>
> >
> > It can be hard to track where references are taken and released.
> >
> > In networking, we have annoying issues at device dismantles,
> > and we had various proposals to ease root causing them.
> >
> > This patch adds new infrastructure pairing refcount increases
> > and decreases. This will self document code, because programmer
> > will have to associate increments/decrements.
> >
> > This is controled by CONFIG_REF_TRACKER which can be selected
> > by users of this feature.
> >
> > This adds both cpu and memory costs, and thus should be reserved
> > for debug kernel builds, or be enabled on demand with a static key.
> >
> > Signed-off-by: Eric Dumazet <edumazet@...gle.com>
>
> Looks great, this is what I had in mind when I said:
>
> | In the future we can extend this structure to also catch those
> | who fail to release the ref on unregistering notification.
>
> I realized today we can get quite a lot of coverage by just plugging
> in object debug infra.
>
> The main differences I see:
>  - do we ever want to use this in prod? - if not why allocate the
>    tracker itself dynamically? The double pointer interface seems
>    harder to compile out completely

I think that maintaining the tracking state in separate storage would
detect cases
where the object has been freed, without the help of KASAN.

>  - whether one stored netdev ptr can hold multiple refs

For a same stack depot then ?

Problem is that at the time of dev_hold(), we do not know if
there is one associated dev_put() or multiple ones (different stack depot)


>  - do we want to wrap the pointer itself or have the "tracker" object
>    be a separate entity
>  - do we want to catch "use after free" when ref is accessed after
>    it was already released
>
> No strong preference either way.

BTW my current suspicion about reported leaks is in
rt6_uncached_list_flush_dev()

I was considering something like

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 5e8f2f15607db7e6589b8bdb984e62512ad30589..233931b7c547d852ed3adeaa15f0a48f437b6596
100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -163,9 +163,6 @@ static void rt6_uncached_list_flush_dev(struct net
*net, struct net_device *dev)
        struct net_device *loopback_dev = net->loopback_dev;
        int cpu;

-       if (dev == loopback_dev)
-               return;
-
        for_each_possible_cpu(cpu) {
                struct uncached_list *ul = per_cpu_ptr(&rt6_uncached_list, cpu);
                struct rt6_info *rt;
@@ -175,7 +172,7 @@ static void rt6_uncached_list_flush_dev(struct net
*net, struct net_device *dev)
                        struct inet6_dev *rt_idev = rt->rt6i_idev;
                        struct net_device *rt_dev = rt->dst.dev;

-                       if (rt_idev->dev == dev) {
+                       if (rt_idev->dev == dev && dev != loopback_dev) {
                                rt->rt6i_idev = in6_dev_get(loopback_dev);
                                in6_dev_put(rt_idev);
                        }