Re: problem with rtnetlink 'reference' count

[Peter Zijlstra]

On Mon, Oct 23, 2017 at 06:37:44PM +0200, Florian Westphal wrote:

> Is refcount_t only supposed to be used with dec_and_test patterns?

Yes, for reference counting objects.

> > This rtnetlink_rcv_msg() is called from softirq-context, right? Also,
> > all that stuff happens with rcu_read_lock() held.
> 
> No, its called from process context.

OK, so then why not do something like so?


---
 net/core/rtnetlink.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index d4bcdcc68e92..473cabd0a551 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -128,7 +128,6 @@ bool lockdep_rtnl_is_held(void)
 #endif /* #ifdef CONFIG_PROVE_LOCKING */
 
 static struct rtnl_link __rcu *rtnl_msg_handlers[RTNL_FAMILY_MAX + 1];
-static refcount_t rtnl_msg_handlers_ref[RTNL_FAMILY_MAX + 1];
 
 static inline int rtm_msgindex(int msgtype)
 {
@@ -260,10 +259,18 @@ void rtnl_unregister_all(int protocol)
        RCU_INIT_POINTER(rtnl_msg_handlers[protocol], NULL);
        rtnl_unlock();
 
+       /*
+        * XXX explain what this is for...
+        */
        synchronize_net();
 
-       while (refcount_read(&rtnl_msg_handlers_ref[protocol]) > 1)
-               schedule();
+       /*
+        * This serializes against the rcu_read_lock() section in
+        * rtnetlink_rcv_msg() such that after this, all prior instances have
+        * completed and future instances must observe the NULL written above.
+        */
+       synchronize_rcu();
+
        kfree(handlers);
 }
 EXPORT_SYMBOL_GPL(rtnl_unregister_all);
@@ -4203,8 +4210,6 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct 
nlmsghdr *nlh,
                                goto err_unlock;
                }
 
-               refcount_inc(&rtnl_msg_handlers_ref[family]);
-
                if (type == RTM_GETLINK - RTM_BASE)
                        min_dump_alloc = rtnl_calcit(skb, nlh);
 
@@ -4218,7 +4223,6 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct 
nlmsghdr *nlh,
                        };
                        err = netlink_dump_start(rtnl, skb, nlh, &c);
                }
-               refcount_dec(&rtnl_msg_handlers_ref[family]);
                return err;
        }
 
@@ -4230,12 +4234,10 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, 
struct nlmsghdr *nlh,
 
        flags = READ_ONCE(handlers[type].flags);
        if (flags & RTNL_FLAG_DOIT_UNLOCKED) {
-               refcount_inc(&rtnl_msg_handlers_ref[family]);
                doit = READ_ONCE(handlers[type].doit);
                rcu_read_unlock();
                if (doit)
                        err = doit(skb, nlh, extack);
-               refcount_dec(&rtnl_msg_handlers_ref[family]);
                return err;
        }
 
@@ -4333,9 +4335,6 @@ void __init rtnetlink_init(void)
 {
        int i;
 
-       for (i = 0; i < ARRAY_SIZE(rtnl_msg_handlers_ref); i++)
-               refcount_set(&rtnl_msg_handlers_ref[i], 1);
-
        if (register_pernet_subsys(&rtnetlink_net_ops))
                panic("rtnetlink_init: cannot initialize rtnetlink\n");