netdev - Re: [PATCH net-next 2/8] rtnetlink: add rtnl_register

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: <20171107094339.GL9424@breakpoint.cc>
Date:   Tue, 7 Nov 2017 10:43:39 +0100
From:   Florian Westphal <fw@...len.de>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Florian Westphal <fw@...len.de>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 2/8] rtnetlink: add rtnl_register_module

Peter Zijlstra <peterz@...radead.org> wrote:
> > rtnetlink_rcv_msg:
> > 
> > 4406                         dumpit = READ_ONCE(handlers[type].dumpit);
> > 4407                         if (!dumpit)
> > 4408                                 goto err_unlock;
> > 4409                         owner = READ_ONCE(handlers[type].owner);
> 
> So what stops the CPU from hoisting this load before the dumpit load?

I was under impression READ_ONCE also includes rmb but I see i was
wrong.

> > I don't want dumpit function address to be visible before owner.
> > Does that make sense?
> 
> And no. That's insane, how can it ever observe an incomplete tab in the
> first place.
> 
> The problem is that __rtnl_register() and rtnl_unregister are broken.
> 
> __rtnl_register() publishes the tab before it initializes it; allowing
> people to observe the thing incomplete.
>
> Also, are we required to hold rtnl_lock() across __rtnl_register()? I'd
> hope so, otherwise what stops concurrent allocations and leaking of tab?

I don't think these ever acquired rtnl mutex.
Hostorically the rtnl callbacks were statically allocated and only ran
from initcalls.

Use of of kmalloc came later, and then use in modules.

> rtnl_unregister() should then RCU free the tab.

I do not think that will work since that will make it behave like
rtnl_unregister_all(), i.e. removes all callbacks of the family.

> None of that is happening, so what is that RCU stuff supposed to do?

Its supposed to delay rmmod until all places that are still executing a
registered callback are done.