netdev - Re: [Patch net] ipv4: restore rt->fi for reference counting

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 for Android: free password hash cracker in your pocket

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

Date:   Tue, 16 May 2017 10:53:24 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Julian Anastasov <ja@....bg>
Cc:     Eric Dumazet <eric.dumazet@...il.com>,
        David Miller <davem@...emloft.net>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Andrey Konovalov <andreyknvl@...gle.com>,
        Eric Dumazet <edumazet@...gle.com>
Subject: Re: [Patch net] ipv4: restore rt->fi for reference counting

On Tue, May 16, 2017 at 12:46 AM, Julian Anastasov <ja@....bg> wrote:
>
>         Hello,
>
> On Mon, 15 May 2017, Cong Wang wrote:
>
>> On Mon, May 15, 2017 at 1:37 PM, Julian Anastasov <ja@....bg> wrote:
>> >         Any user that does not set FIB_LOOKUP_NOREF
>> > will need nh_dev refcounts. The assumption is that the
>> > NHs are accessed, who knows, may be even after RCU grace
>> > period. As result, we can not use dev_put on NETDEV_UNREGISTER.
>> > So, we should check if there are users that do not
>> > set FIB_LOOKUP_NOREF, at first look, I don't see such ones
>> > for IPv4.
>>
>> I see, although we do have FIB_LOOKUP_NOREF set all the times,
>> there are other places we hold fib_clntref too, for example
>> mlxsw_sp_router_fib_event_work(), it actually uses nh_dev too...
>>
>> So I am afraid moving dev_put() to fib_release_info() is not a solution
>> here. I have to rethink about it.
>
>         At first look, they use fib_info_hold() to get fib_clntref
> reference from places where fib_treeref is not fatally decreased
> to 0 but later a work is used which finishes the job. I guess, we
> can convert such places to use just a fib_treeref reference.
> They can use such new method instead of fib_info_hold:
>
> void fib_treeref_get(struct fib_info *fi)
> {
>         spin_lock_bh(&fib_info_lock);
>         fi->fib_treeref++;
>         spin_unlock_bh(&fib_info_lock);
> }
>
> They will use fib_release_info() to put the reference. But
> on FIB_EVENT_ENTRY_DEL there is a small window where the
> scheduled work delays the unlink of fib info from the
> hash tables, i.e. there is a risk fib_find_info to reuse
> a dead fib info.
>

Right, that seems risky.

How about adding a check for ->fib_dead in these work's?
If the last treeref is gone, probably it is no longer needed
to continue to do the offloading work.