From mboxrd@z Thu Jan  1 00:00:00 1970
From: Florian Westphal <fw@strlen.de>
Subject: Re: [PATCH net-next 2/8] rtnetlink: add rtnl_register_module
Date: Mon, 13 Nov 2017 08:21:59 +0100
Message-ID: <20171113072159.GN5512@breakpoint.cc>
References: <20171106105113.20476-1-fw@strlen.de>
 <20171106105113.20476-3-fw@strlen.de>
 <20171106124454.GI3165@worktop.lehotels.local>
 <20171107061156.GK9424@breakpoint.cc>
 <20171107091004.GI3326@worktop>
 <20171107092458.GF3857@worktop>
 <20171107094751.GM9424@breakpoint.cc>
 <20171107145736.GN3165@worktop.lehotels.local>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Florian Westphal <fw@strlen.de>, netdev@vger.kernel.org
To: Peter Zijlstra <peterz@infradead.org>
Return-path: <netdev-owner@vger.kernel.org>
Received: from Chamillionaire.breakpoint.cc ([146.0.238.67]:45350 "EHLO
        Chamillionaire.breakpoint.cc" rhost-flags-OK-OK-OK-OK)
        by vger.kernel.org with ESMTP id S1751377AbdKMHWp (ORCPT
        <rfc822;netdev@vger.kernel.org>); Mon, 13 Nov 2017 02:22:45 -0500
Content-Disposition: inline
In-Reply-To: <20171107145736.GN3165@worktop.lehotels.local>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, Nov 07, 2017 at 10:47:51AM +0100, Florian Westphal wrote:
> > I would expect this to trigger all the time, due to
> > 
> > rtnl_register(AF_INET, RTM_GETROUTE, ...
> > rtnl_register(AF_INET, RTM_GETADDR, ...
> 
> Ah, sure, then something like so then...
> 
> There's bound to be bugs there too, as I pretty much typed this without
> thinking, but it should show the idea.

Just o let you know, I am backlogged at the moment so I Will not have
time to work on this for the time being.

> ---
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 5ace48926b19..de1336775602 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -63,6 +63,7 @@ struct rtnl_link {
>  	rtnl_doit_func		doit;
>  	rtnl_dumpit_func	dumpit;
>  	unsigned int		flags;
> +	struct rcu_head		rcu;
>  };

This will need to be split:

struct rtnl_link {
	rtnl_doit_func          doit;
	unsigned int		flags;
	struct rcu_head		rcu;
};
struct rtnl_link_dump {
  	rtnl_dumpit_func	dumpit;
	struct rcu_head		rcu;
};

> -static struct rtnl_link __rcu *rtnl_msg_handlers[RTNL_FAMILY_MAX + 1];
> +static struct rtnl_link __rcu **rtnl_msg_handlers[RTNL_FAMILY_MAX + 1];

So this will need to be two arrays.

Reason is that some places do this:

rtnl_register(pf, RTM_FOO, doit, NULL, 0);
rtnl_register(pf, RTM_FOO, NULL, dumpit, 0);

(from different call sites in the stack).
> -	if (doit)
> -		tab[msgindex].doit = doit;
> -	if (dumpit)
> -		tab[msgindex].dumpit = dumpit;

Which is the reason for these if () tests.