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]

Date:   Wed, 17 Nov 2021 12:47:06 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Eric Dumazet <edumazet@...gle.com>
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, 17 Nov 2021 12:16:15 -0800 Eric Dumazet wrote:
> On Wed, Nov 17, 2021 at 12:03 PM Jakub Kicinski <kuba@...nel.org> wrote:
> > 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.

Makes sense, I guess we can hang more of the information of a secondary
object?

Maybe I'm missing a trick on how to make the feature consume no space
when disabled via Kconfig.

> >  - whether one stored netdev ptr can hold multiple refs  
> 
> For a same stack depot then ?

Not necessarily.

> 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)

Ack. My thinking was hold all stacks until tracker is completely
drained of refs. We'd have to collect both hold and put stacks in
that case and if ref leak happens try to match them up manually 
later (manually == human).

But if we can get away without allowing multiple refs with one tracker
that makes life easier, and is probably a cleaner API, anyway.

> >  - 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);
>                         }

Interesting.