[PATCH net 0/3] llc2 station handler fixes

[PATCH net 0/3] llc2 station handler fixes

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 673 bytes --]

I was insprired by commit 91d27a8650d5359a7a320daeb35b88cdea15e3a8
('llc: free the right skb') to look into llc_station.c, and didn't like
what I saw.

These are *only* compile tested; hopefully you have some way to test
LLC.

Ben.

Ben Hutchings (3):
  llc2: Fix silent failure of llc_station_init()
  llc2: Call llc_station_exit() on llc2_init() failure path
  llc: Fix races between llc2 handler use and (un)registration

 include/net/llc.h     |    2 +-
 net/llc/af_llc.c      |    5 +++--
 net/llc/llc_input.c   |   21 +++++++++++++++++----
 net/llc/llc_station.c |   21 +++------------------
 4 files changed, 24 insertions(+), 25 deletions(-)



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

[PATCH net 1/3] llc2: Fix silent failure of llc_station_init()

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2284 bytes --]

llc_station_init() creates and processes an event skb with no effect
other than to change the state from DOWN to UP.  Allocation failure is
reported, but then ignored by its caller, llc2_init().  Remove this
possibility by simply initialising the state as UP.

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 include/net/llc.h     |    2 +-
 net/llc/llc_station.c |   19 ++-----------------
 2 files changed, 3 insertions(+), 18 deletions(-)

diff --git a/include/net/llc.h b/include/net/llc.h
index 226c846..f2d0fc5 100644
--- a/include/net/llc.h
+++ b/include/net/llc.h
@@ -133,7 +133,7 @@ extern int llc_build_and_send_ui_pkt(struct llc_sap *sap, struct sk_buff *skb,
 extern void llc_sap_handler(struct llc_sap *sap, struct sk_buff *skb);
 extern void llc_conn_handler(struct llc_sap *sap, struct sk_buff *skb);
 
-extern int llc_station_init(void);
+extern void llc_station_init(void);
 extern void llc_station_exit(void);
 
 #ifdef CONFIG_PROC_FS
diff --git a/net/llc/llc_station.c b/net/llc/llc_station.c
index 6828e39..45ddbb9 100644
--- a/net/llc/llc_station.c
+++ b/net/llc/llc_station.c
@@ -687,12 +687,8 @@ static void llc_station_rcv(struct sk_buff *skb)
 	llc_station_state_process(skb);
 }
 
-int __init llc_station_init(void)
+void __init llc_station_init(void)
 {
-	int rc = -ENOBUFS;
-	struct sk_buff *skb;
-	struct llc_station_state_ev *ev;
-
 	skb_queue_head_init(&llc_main_station.mac_pdu_q);
 	skb_queue_head_init(&llc_main_station.ev_q.list);
 	spin_lock_init(&llc_main_station.ev_q.lock);
@@ -700,20 +696,9 @@ int __init llc_station_init(void)
 			(unsigned long)&llc_main_station);
 	llc_main_station.ack_timer.expires  = jiffies +
 						sysctl_llc_station_ack_timeout;
-	skb = alloc_skb(0, GFP_ATOMIC);
-	if (!skb)
-		goto out;
-	rc = 0;
 	llc_set_station_handler(llc_station_rcv);
-	ev = llc_station_ev(skb);
-	memset(ev, 0, sizeof(*ev));
 	llc_main_station.maximum_retry	= 1;
-	llc_main_station.state		= LLC_STATION_STATE_DOWN;
-	ev->type	= LLC_STATION_EV_TYPE_SIMPLE;
-	ev->prim_type	= LLC_STATION_EV_ENABLE_WITHOUT_DUP_ADDR_CHECK;
-	rc = llc_station_next_state(skb);
-out:
-	return rc;
+	llc_main_station.state		= LLC_STATION_STATE_UP;
 }
 
 void __exit llc_station_exit(void)



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH net 1/3] llc2: Fix silent failure of llc_station_init()

[David Miller]

From: Ben Hutchings <ben@decadent.org.uk>
Date: Mon, 13 Aug 2012 13:49:59 +0100

> llc_station_init() creates and processes an event skb with no effect
> other than to change the state from DOWN to UP.  Allocation failure is
> reported, but then ignored by its caller, llc2_init().  Remove this
> possibility by simply initialising the state as UP.
> 
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>

Applied.

[PATCH net 2/3] llc2: Call llc_station_exit() on llc2_init() failure path

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 811 bytes --]

Otherwise the station packet handler will remain registered even though
the module is unloaded.

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 net/llc/af_llc.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/llc/af_llc.c b/net/llc/af_llc.c
index f6fe4d4..8c2919c 100644
--- a/net/llc/af_llc.c
+++ b/net/llc/af_llc.c
@@ -1206,7 +1206,7 @@ static int __init llc2_init(void)
 	rc = llc_proc_init();
 	if (rc != 0) {
 		printk(llc_proc_err_msg);
-		goto out_unregister_llc_proto;
+		goto out_station;
 	}
 	rc = llc_sysctl_init();
 	if (rc) {
@@ -1226,7 +1226,8 @@ out_sysctl:
 	llc_sysctl_exit();
 out_proc:
 	llc_proc_exit();
-out_unregister_llc_proto:
+out_station:
+	llc_station_exit();
 	proto_unregister(&llc_proto);
 	goto out;
 }



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH net 2/3] llc2: Call llc_station_exit() on llc2_init() failure path

[David Miller]

From: Ben Hutchings <ben@decadent.org.uk>
Date: Mon, 13 Aug 2012 13:50:43 +0100

> Otherwise the station packet handler will remain registered even though
> the module is unloaded.
> 
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>

This will cause section mismatch errors, you'll need to remove
the __exit tag from llc_station_exit() if you want to start
invoking it from llc2_init() which is __init.

I took care of this when applying this patch to 'net'.

Re: [PATCH net 2/3] llc2: Call llc_station_exit() on llc2_init() failure path

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 844 bytes --]

On Tue, 2012-08-14 at 16:52 -0700, David Miller wrote:
> From: Ben Hutchings <ben@decadent.org.uk>
> Date: Mon, 13 Aug 2012 13:50:43 +0100
> 
> > Otherwise the station packet handler will remain registered even though
> > the module is unloaded.
> > 
> > Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> 
> This will cause section mismatch errors, you'll need to remove
> the __exit tag from llc_station_exit() if you want to start
> invoking it from llc2_init() which is __init.
> 
> I took care of this when applying this patch to 'net'.

Thanks, David.

llc_station.c turns out to be mostly dead code, and I have some more
radical changes to remove that which I'll send after you next merge net
into net-next.

Ben.

-- 
Ben Hutchings
I say we take off; nuke the site from orbit.  It's the only way to be sure.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

[PATCH net 3/3] llc: Fix races between llc2 handler use and (un)registration

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 3199 bytes --]

When registering the handlers, any state they rely on must be
completely initialised first.  When unregistering, we must wait until
they are definitely no longer running.  llc_rcv() must also avoid
reading the handler pointers again after checking for NULL.

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 net/llc/llc_input.c   |   21 +++++++++++++++++----
 net/llc/llc_station.c |    2 +-
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/net/llc/llc_input.c b/net/llc/llc_input.c
index e32cab4..dd3e833 100644
--- a/net/llc/llc_input.c
+++ b/net/llc/llc_input.c
@@ -42,6 +42,7 @@ static void (*llc_type_handlers[2])(struct llc_sap *sap,
 void llc_add_pack(int type, void (*handler)(struct llc_sap *sap,
 					    struct sk_buff *skb))
 {
+	smp_wmb(); /* ensure initialisation is complete before it's called */
 	if (type == LLC_DEST_SAP || type == LLC_DEST_CONN)
 		llc_type_handlers[type - 1] = handler;
 }
@@ -50,11 +51,19 @@ void llc_remove_pack(int type)
 {
 	if (type == LLC_DEST_SAP || type == LLC_DEST_CONN)
 		llc_type_handlers[type - 1] = NULL;
+	synchronize_net();
 }
 
 void llc_set_station_handler(void (*handler)(struct sk_buff *skb))
 {
+	/* Ensure initialisation is complete before it's called */
+	if (handler)
+		smp_wmb();
+
 	llc_station_handler = handler;
+
+	if (!handler)
+		synchronize_net();
 }
 
 /**
@@ -150,6 +159,8 @@ int llc_rcv(struct sk_buff *skb, struct net_device *dev,
 	int dest;
 	int (*rcv)(struct sk_buff *, struct net_device *,
 		   struct packet_type *, struct net_device *);
+	void (*sta_handler)(struct sk_buff *skb);
+	void (*sap_handler)(struct llc_sap *sap, struct sk_buff *skb);
 
 	if (!net_eq(dev_net(dev), &init_net))
 		goto drop;
@@ -182,7 +193,8 @@ int llc_rcv(struct sk_buff *skb, struct net_device *dev,
 	 */
 	rcv = rcu_dereference(sap->rcv_func);
 	dest = llc_pdu_type(skb);
-	if (unlikely(!dest || !llc_type_handlers[dest - 1])) {
+	sap_handler = dest ? ACCESS_ONCE(llc_type_handlers[dest - 1]) : NULL;
+	if (unlikely(!sap_handler)) {
 		if (rcv)
 			rcv(skb, dev, pt, orig_dev);
 		else
@@ -193,7 +205,7 @@ int llc_rcv(struct sk_buff *skb, struct net_device *dev,
 			if (cskb)
 				rcv(cskb, dev, pt, orig_dev);
 		}
-		llc_type_handlers[dest - 1](sap, skb);
+		sap_handler(sap, skb);
 	}
 	llc_sap_put(sap);
 out:
@@ -202,9 +214,10 @@ drop:
 	kfree_skb(skb);
 	goto out;
 handle_station:
-	if (!llc_station_handler)
+	sta_handler = ACCESS_ONCE(llc_station_handler);
+	if (!sta_handler)
 		goto drop;
-	llc_station_handler(skb);
+	sta_handler(skb);
 	goto out;
 }
 
diff --git a/net/llc/llc_station.c b/net/llc/llc_station.c
index 45ddbb9..81da71a 100644
--- a/net/llc/llc_station.c
+++ b/net/llc/llc_station.c
@@ -696,9 +696,9 @@ void __init llc_station_init(void)
 			(unsigned long)&llc_main_station);
 	llc_main_station.ack_timer.expires  = jiffies +
 						sysctl_llc_station_ack_timeout;
-	llc_set_station_handler(llc_station_rcv);
 	llc_main_station.maximum_retry	= 1;
 	llc_main_station.state		= LLC_STATION_STATE_UP;
+	llc_set_station_handler(llc_station_rcv);
 }
 
 void __exit llc_station_exit(void)


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH net 3/3] llc: Fix races between llc2 handler use and (un)registration

[David Miller]

From: Ben Hutchings <ben@decadent.org.uk>
Date: Mon, 13 Aug 2012 13:50:55 +0100

> When registering the handlers, any state they rely on must be
> completely initialised first.  When unregistering, we must wait until
> they are definitely no longer running.  llc_rcv() must also avoid
> reading the handler pointers again after checking for NULL.
> 
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>

Applied.