[net-next] ipv4: initialize ra_mutex in inet_init_net()

Message ID 20180914203242.2712-1-xiyou.wangcong@gmail.com
State Changes Requested
Delegated to: David Miller
Headers show
Series
  • [net-next] ipv4: initialize ra_mutex in inet_init_net()
Related show

Commit Message

Cong Wang Sept. 14, 2018, 8:32 p.m.
ra_mutex is a IPv4 specific mutex, it is inside struct netns_ipv4,
but its initialization is in the generic netns code, setup_net().

Move it to IPv4 specific net init code, inet_init_net().

Fixes: d9ff3049739e ("net: Replace ip_ra_lock with per-net mutex")
Cc: Kirill Tkhai <ktkhai@virtuozzo.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/core/net_namespace.c | 1 -
 net/ipv4/af_inet.c       | 2 ++
 2 files changed, 2 insertions(+), 1 deletion(-)

Comments

Kirill Tkhai Sept. 17, 2018, 7:25 a.m. | #1
On 14.09.2018 23:32, Cong Wang wrote:
> ra_mutex is a IPv4 specific mutex, it is inside struct netns_ipv4,
> but its initialization is in the generic netns code, setup_net().
> 
> Move it to IPv4 specific net init code, inet_init_net().
> 
> Fixes: d9ff3049739e ("net: Replace ip_ra_lock with per-net mutex")
> Cc: Kirill Tkhai <ktkhai@virtuozzo.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
>  net/core/net_namespace.c | 1 -
>  net/ipv4/af_inet.c       | 2 ++
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
> index 670c84b1bfc2..b272ccfcbf63 100644
> --- a/net/core/net_namespace.c
> +++ b/net/core/net_namespace.c
> @@ -308,7 +308,6 @@ static __net_init int setup_net(struct net *net, struct user_namespace *user_ns)
>  	net->user_ns = user_ns;
>  	idr_init(&net->netns_ids);
>  	spin_lock_init(&net->nsid_lock);
> -	mutex_init(&net->ipv4.ra_mutex);
>  
>  	list_for_each_entry(ops, &pernet_list, list) {
>  		error = ops_init(ops, net);
> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index 20fda8fb8ffd..57b7bffb93e5 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -1817,6 +1817,8 @@ static __net_init int inet_init_net(struct net *net)
>  	net->ipv4.sysctl_igmp_llm_reports = 1;
>  	net->ipv4.sysctl_igmp_qrv = 2;
>  
> +	mutex_init(&net->ipv4.ra_mutex);
> +

In inet_init() the order of registration is:

	ip_mr_init();
	init_inet_pernet_ops();

This means, ipmr_net_ops pernet operations are before af_inet_ops
in pernet_list. So, there is a theoretical probability, sometimes
in the future, we will have a problem during a fail of net initialization.

Say,

setup_net():
	ipmr_net_ops->init() returns 0
	xxx->init()          returns error
and then we do:
	ipmr_net_ops->exit(),

which could touch ra_mutex (theoretically).

Your patch is OK, but since you do this, we may also swap the order
of registration of ipmr_net_ops and af_inet_ops better too.

Kirill
David Miller Sept. 17, 2018, 3:02 p.m. | #2
From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Fri, 14 Sep 2018 13:32:42 -0700

> ra_mutex is a IPv4 specific mutex, it is inside struct netns_ipv4,
> but its initialization is in the generic netns code, setup_net().
> 
> Move it to IPv4 specific net init code, inet_init_net().
> 
> Fixes: d9ff3049739e ("net: Replace ip_ra_lock with per-net mutex")
> Cc: Kirill Tkhai <ktkhai@virtuozzo.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Please take into consideration Kirill's feedback.

Thank you.
Cong Wang Sept. 18, 2018, 8:17 p.m. | #3
On Mon, Sep 17, 2018 at 12:25 AM Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
> In inet_init() the order of registration is:
>
>         ip_mr_init();
>         init_inet_pernet_ops();
>
> This means, ipmr_net_ops pernet operations are before af_inet_ops
> in pernet_list. So, there is a theoretical probability, sometimes
> in the future, we will have a problem during a fail of net initialization.
>
> Say,
>
> setup_net():
>         ipmr_net_ops->init() returns 0
>         xxx->init()          returns error
> and then we do:
>         ipmr_net_ops->exit(),
>
> which could touch ra_mutex (theoretically).

How could ra_mutex be touched in this scenario?

ra_mutex is only used in ip_ra_control() which is called
only by {get,set}sockopt(). I don't see anything related
to netns exit() path here.

Patch

diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index 670c84b1bfc2..b272ccfcbf63 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -308,7 +308,6 @@  static __net_init int setup_net(struct net *net, struct user_namespace *user_ns)
 	net->user_ns = user_ns;
 	idr_init(&net->netns_ids);
 	spin_lock_init(&net->nsid_lock);
-	mutex_init(&net->ipv4.ra_mutex);
 
 	list_for_each_entry(ops, &pernet_list, list) {
 		error = ops_init(ops, net);
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 20fda8fb8ffd..57b7bffb93e5 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -1817,6 +1817,8 @@  static __net_init int inet_init_net(struct net *net)
 	net->ipv4.sysctl_igmp_llm_reports = 1;
 	net->ipv4.sysctl_igmp_qrv = 2;
 
+	mutex_init(&net->ipv4.ra_mutex);
+
 	return 0;
 }