diff mbox

[net-next,1/6] GRE: Use RTNL for atomic protocol registration.

Message ID 1363214907-3257-1-git-send-email-pshelar@nicira.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Pravin B Shelar March 13, 2013, 10:48 p.m. UTC 
Currently gre demultiplexer uses its own spin-lock. We should
rather RTNL lock.

Suggested-by: Stephen Hemminger <--cc stephen@networkplumber.org>
Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
---
 drivers/net/ppp/pptp.c |    4 ++++
 net/ipv4/gre.c         |   18 +++++-------------
 net/ipv4/ip_gre.c      |    6 ++++++
 3 files changed, 15 insertions(+), 13 deletions(-)

Comments

Stephen Hemminger March 13, 2013, 10:57 p.m. UTC | #1 
On Wed, 13 Mar 2013 15:48:27 -0700
Pravin B Shelar <pshelar@nicira.com> wrote:

> Currently gre demultiplexer uses its own spin-lock. We should
> rather RTNL lock.
> 
> Suggested-by: Stephen Hemminger <--cc stephen@networkplumber.org>
> Signed-off-by: Pravin B Shelar <pshelar@nicira.com>

Contradicting myself :-)
This does have added safety but some people eventually want to get rid
of the big network lock. That said, it makes sense that you would need a
supersized lock when playing with multiple devices.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
=?ISO-8859-2?Q?Micha=B3_Miros=B3aw?= March 13, 2013, 11:22 p.m. UTC | #2 
2013/3/13 Pravin B Shelar <pshelar@nicira.com>:
> Currently gre demultiplexer uses its own spin-lock. We should
> rather RTNL lock.
[...]
> +       rtnl_lock();
>         err = gre_add_protocol(&gre_pptp_protocol, GREPROTO_PPTP);
> +       rtnl_unlock();

Why not keep lock/unlock inside gre_add/del_protocol()?

Best Regards,
Michał Mirosław
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pravin B Shelar March 15, 2013, 3:37 a.m. UTC | #3 
On Wed, Mar 13, 2013 at 4:22 PM, Michał Mirosław <mirqus@gmail.com> wrote:
> 2013/3/13 Pravin B Shelar <pshelar@nicira.com>:
>> Currently gre demultiplexer uses its own spin-lock. We should
>> rather RTNL lock.
> [...]
>> +       rtnl_lock();
>>         err = gre_add_protocol(&gre_pptp_protocol, GREPROTO_PPTP);
>> +       rtnl_unlock();
>
> Why not keep lock/unlock inside gre_add/del_protocol()?
>
As Stephen suggested I am planning on dropping this patch.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/ppp/pptp.c b/drivers/net/ppp/pptp.c
index 162464f..672e7e3 100644
--- a/drivers/net/ppp/pptp.c
+++ b/drivers/net/ppp/pptp.c
@@ -673,7 +673,9 @@  static int __init pptp_init_module(void)
 	if (!callid_sock)
 		return -ENOMEM;
 
+	rtnl_lock();
 	err = gre_add_protocol(&gre_pptp_protocol, GREPROTO_PPTP);
+	rtnl_unlock();
 	if (err) {
 		pr_err("PPTP: can't add gre protocol\n");
 		goto out_mem_free;
@@ -707,7 +709,9 @@  static void __exit pptp_exit_module(void)
 {
 	unregister_pppox_proto(PX_PROTO_PPTP);
 	proto_unregister(&pptp_sk_proto);
+	rtnl_lock();
 	gre_del_protocol(&gre_pptp_protocol, GREPROTO_PPTP);
+	rtnl_unlock();
 	vfree(callid_sock);
 }
 
diff --git a/net/ipv4/gre.c b/net/ipv4/gre.c
index 7a4c710..fbaf33d 100644
--- a/net/ipv4/gre.c
+++ b/net/ipv4/gre.c
@@ -26,7 +26,6 @@ 
 
 
 static const struct gre_protocol __rcu *gre_proto[GREPROTO_MAX] __read_mostly;
-static DEFINE_SPINLOCK(gre_proto_lock);
 struct gre_base_hdr {
 	__be16 flags;
 	__be16 protocol;
@@ -35,19 +34,16 @@  struct gre_base_hdr {
 
 int gre_add_protocol(const struct gre_protocol *proto, u8 version)
 {
+	ASSERT_RTNL();
 	if (version >= GREPROTO_MAX)
 		goto err_out;
 
-	spin_lock(&gre_proto_lock);
 	if (gre_proto[version])
-		goto err_out_unlock;
+		goto err_out;
 
 	RCU_INIT_POINTER(gre_proto[version], proto);
-	spin_unlock(&gre_proto_lock);
 	return 0;
 
-err_out_unlock:
-	spin_unlock(&gre_proto_lock);
 err_out:
 	return -1;
 }
@@ -55,20 +51,16 @@  EXPORT_SYMBOL_GPL(gre_add_protocol);
 
 int gre_del_protocol(const struct gre_protocol *proto, u8 version)
 {
+	ASSERT_RTNL();
 	if (version >= GREPROTO_MAX)
 		goto err_out;
 
-	spin_lock(&gre_proto_lock);
-	if (rcu_dereference_protected(gre_proto[version],
-			lockdep_is_held(&gre_proto_lock)) != proto)
-		goto err_out_unlock;
+	if (rtnl_dereference(gre_proto[version]) != proto)
+		goto err_out;
 	RCU_INIT_POINTER(gre_proto[version], NULL);
-	spin_unlock(&gre_proto_lock);
 	synchronize_rcu();
 	return 0;
 
-err_out_unlock:
-	spin_unlock(&gre_proto_lock);
 err_out:
 	return -1;
 }
diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index a13a097..99cf2b4 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -1852,7 +1852,9 @@  static int __init ipgre_init(void)
 	if (err < 0)
 		return err;
 
+	rtnl_lock();
 	err = gre_add_protocol(&ipgre_protocol, GREPROTO_CISCO);
+	rtnl_unlock();
 	if (err < 0) {
 		pr_info("%s: can't add protocol\n", __func__);
 		goto add_proto_failed;
@@ -1872,7 +1874,9 @@  out:
 tap_ops_failed:
 	rtnl_link_unregister(&ipgre_link_ops);
 rtnl_link_failed:
+	rtnl_lock();
 	gre_del_protocol(&ipgre_protocol, GREPROTO_CISCO);
+	rtnl_unlock();
 add_proto_failed:
 	unregister_pernet_device(&ipgre_net_ops);
 	goto out;
@@ -1882,8 +1886,10 @@  static void __exit ipgre_fini(void)
 {
 	rtnl_link_unregister(&ipgre_tap_ops);
 	rtnl_link_unregister(&ipgre_link_ops);
+	rtnl_lock();
 	if (gre_del_protocol(&ipgre_protocol, GREPROTO_CISCO) < 0)
 		pr_info("%s: can't remove protocol\n", __func__);
+	rtnl_unlock();
 	unregister_pernet_device(&ipgre_net_ops);
 }