[PATCH net v4] net: lapb: Add locking to the lapb module

[PATCH net v4] net: lapb: Add locking to the lapb module

[Xie He]

In the lapb module, the timers may run concurrently with other code in
this module, and there is currently no locking to prevent the code from
racing on "struct lapb_cb". This patch adds locking to prevent racing.

1. Add "spinlock_t lock" to "struct lapb_cb"; Add "spin_lock_bh" and
"spin_unlock_bh" to APIs, timer functions and notifier functions.

2. Add "bool t1timer_stop, t2timer_stop" to "struct lapb_cb" to make us
able to ask running timers to abort; Modify "lapb_stop_t1timer" and
"lapb_stop_t2timer" to make them able to abort running timers;
Modify "lapb_t2timer_expiry" and "lapb_t1timer_expiry" to make them
abort after they are stopped by "lapb_stop_t1timer", "lapb_stop_t2timer",
and "lapb_start_t1timer", "lapb_start_t2timer".

3. In lapb_unregister, add "del_timer_sync" calls to make sure all
running timers have exited.

4. The lapb_device_event function calls lapb_disconnect_request. In
order to avoid trying to hold the lock twice, add a new function named
"__lapb_disconnect_request" which assumes the lock is held, and make
it called by lapb_disconnect_request and lapb_device_event.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: Martin Schiller <ms@dev.tdt.de>
Signed-off-by: Xie He <xie.he.0141@gmail.com>
---
 include/net/lapb.h    |  2 ++
 net/lapb/lapb_iface.c | 65 ++++++++++++++++++++++++++++++++-----------
 net/lapb/lapb_timer.c | 30 +++++++++++++++++---
 3 files changed, 77 insertions(+), 20 deletions(-)

diff --git a/include/net/lapb.h b/include/net/lapb.h
index ccc3d1f020b0..eee73442a1ba 100644
--- a/include/net/lapb.h
+++ b/include/net/lapb.h
@@ -92,6 +92,7 @@ struct lapb_cb {
 	unsigned short		n2, n2count;
 	unsigned short		t1, t2;
 	struct timer_list	t1timer, t2timer;
+	bool			t1timer_stop, t2timer_stop;
 
 	/* Internal control information */
 	struct sk_buff_head	write_queue;
@@ -103,6 +104,7 @@ struct lapb_cb {
 	struct lapb_frame	frmr_data;
 	unsigned char		frmr_type;
 
+	spinlock_t		lock;
 	refcount_t		refcnt;
 };
 
diff --git a/net/lapb/lapb_iface.c b/net/lapb/lapb_iface.c
index 40961889e9c0..205f8736e68b 100644
--- a/net/lapb/lapb_iface.c
+++ b/net/lapb/lapb_iface.c
@@ -122,6 +122,8 @@ static struct lapb_cb *lapb_create_cb(void)
 
 	timer_setup(&lapb->t1timer, NULL, 0);
 	timer_setup(&lapb->t2timer, NULL, 0);
+	lapb->t1timer_stop = true;
+	lapb->t2timer_stop = true;
 
 	lapb->t1      = LAPB_DEFAULT_T1;
 	lapb->t2      = LAPB_DEFAULT_T2;
@@ -129,6 +131,8 @@ static struct lapb_cb *lapb_create_cb(void)
 	lapb->mode    = LAPB_DEFAULT_MODE;
 	lapb->window  = LAPB_DEFAULT_WINDOW;
 	lapb->state   = LAPB_STATE_0;
+
+	spin_lock_init(&lapb->lock);
 	refcount_set(&lapb->refcnt, 1);
 out:
 	return lapb;
@@ -178,11 +182,18 @@ int lapb_unregister(struct net_device *dev)
 		goto out;
 	lapb_put(lapb);
 
+	spin_lock_bh(&lapb->lock);
+
 	lapb_stop_t1timer(lapb);
 	lapb_stop_t2timer(lapb);
 
 	lapb_clear_queues(lapb);
 
+	spin_unlock_bh(&lapb->lock);
+
+	del_timer_sync(&lapb->t1timer);
+	del_timer_sync(&lapb->t2timer);
+
 	__lapb_remove_cb(lapb);
 
 	lapb_put(lapb);
@@ -201,6 +212,8 @@ int lapb_getparms(struct net_device *dev, struct lapb_parms_struct *parms)
 	if (!lapb)
 		goto out;
 
+	spin_lock_bh(&lapb->lock);
+
 	parms->t1      = lapb->t1 / HZ;
 	parms->t2      = lapb->t2 / HZ;
 	parms->n2      = lapb->n2;
@@ -219,6 +232,7 @@ int lapb_getparms(struct net_device *dev, struct lapb_parms_struct *parms)
 	else
 		parms->t2timer = (lapb->t2timer.expires - jiffies) / HZ;
 
+	spin_unlock_bh(&lapb->lock);
 	lapb_put(lapb);
 	rc = LAPB_OK;
 out:
@@ -234,6 +248,8 @@ int lapb_setparms(struct net_device *dev, struct lapb_parms_struct *parms)
 	if (!lapb)
 		goto out;
 
+	spin_lock_bh(&lapb->lock);
+
 	rc = LAPB_INVALUE;
 	if (parms->t1 < 1 || parms->t2 < 1 || parms->n2 < 1)
 		goto out_put;
@@ -256,6 +272,7 @@ int lapb_setparms(struct net_device *dev, struct lapb_parms_struct *parms)
 
 	rc = LAPB_OK;
 out_put:
+	spin_unlock_bh(&lapb->lock);
 	lapb_put(lapb);
 out:
 	return rc;
@@ -270,6 +287,8 @@ int lapb_connect_request(struct net_device *dev)
 	if (!lapb)
 		goto out;
 
+	spin_lock_bh(&lapb->lock);
+
 	rc = LAPB_OK;
 	if (lapb->state == LAPB_STATE_1)
 		goto out_put;
@@ -285,24 +304,18 @@ int lapb_connect_request(struct net_device *dev)
 
 	rc = LAPB_OK;
 out_put:
+	spin_unlock_bh(&lapb->lock);
 	lapb_put(lapb);
 out:
 	return rc;
 }
 EXPORT_SYMBOL(lapb_connect_request);
 
-int lapb_disconnect_request(struct net_device *dev)
+static int __lapb_disconnect_request(struct lapb_cb *lapb)
 {
-	struct lapb_cb *lapb = lapb_devtostruct(dev);
-	int rc = LAPB_BADTOKEN;
-
-	if (!lapb)
-		goto out;
-
 	switch (lapb->state) {
 	case LAPB_STATE_0:
-		rc = LAPB_NOTCONNECTED;
-		goto out_put;
+		return LAPB_NOTCONNECTED;
 
 	case LAPB_STATE_1:
 		lapb_dbg(1, "(%p) S1 TX DISC(1)\n", lapb->dev);
@@ -310,12 +323,10 @@ int lapb_disconnect_request(struct net_device *dev)
 		lapb_send_control(lapb, LAPB_DISC, LAPB_POLLON, LAPB_COMMAND);
 		lapb->state = LAPB_STATE_0;
 		lapb_start_t1timer(lapb);
-		rc = LAPB_NOTCONNECTED;
-		goto out_put;
+		return LAPB_NOTCONNECTED;
 
 	case LAPB_STATE_2:
-		rc = LAPB_OK;
-		goto out_put;
+		return LAPB_OK;
 	}
 
 	lapb_clear_queues(lapb);
@@ -328,8 +339,22 @@ int lapb_disconnect_request(struct net_device *dev)
 	lapb_dbg(1, "(%p) S3 DISC(1)\n", lapb->dev);
 	lapb_dbg(0, "(%p) S3 -> S2\n", lapb->dev);
 
-	rc = LAPB_OK;
-out_put:
+	return LAPB_OK;
+}
+
+int lapb_disconnect_request(struct net_device *dev)
+{
+	struct lapb_cb *lapb = lapb_devtostruct(dev);
+	int rc = LAPB_BADTOKEN;
+
+	if (!lapb)
+		goto out;
+
+	spin_lock_bh(&lapb->lock);
+
+	rc = __lapb_disconnect_request(lapb);
+
+	spin_unlock_bh(&lapb->lock);
 	lapb_put(lapb);
 out:
 	return rc;
@@ -344,6 +369,8 @@ int lapb_data_request(struct net_device *dev, struct sk_buff *skb)
 	if (!lapb)
 		goto out;
 
+	spin_lock_bh(&lapb->lock);
+
 	rc = LAPB_NOTCONNECTED;
 	if (lapb->state != LAPB_STATE_3 && lapb->state != LAPB_STATE_4)
 		goto out_put;
@@ -352,6 +379,7 @@ int lapb_data_request(struct net_device *dev, struct sk_buff *skb)
 	lapb_kick(lapb);
 	rc = LAPB_OK;
 out_put:
+	spin_unlock_bh(&lapb->lock);
 	lapb_put(lapb);
 out:
 	return rc;
@@ -364,7 +392,9 @@ int lapb_data_received(struct net_device *dev, struct sk_buff *skb)
 	int rc = LAPB_BADTOKEN;
 
 	if (lapb) {
+		spin_lock_bh(&lapb->lock);
 		lapb_data_input(lapb, skb);
+		spin_unlock_bh(&lapb->lock);
 		lapb_put(lapb);
 		rc = LAPB_OK;
 	}
@@ -435,6 +465,8 @@ static int lapb_device_event(struct notifier_block *this, unsigned long event,
 	if (!lapb)
 		return NOTIFY_DONE;
 
+	spin_lock_bh(&lapb->lock);
+
 	switch (event) {
 	case NETDEV_UP:
 		lapb_dbg(0, "(%p) Interface up: %s\n", dev, dev->name);
@@ -454,7 +486,7 @@ static int lapb_device_event(struct notifier_block *this, unsigned long event,
 		break;
 	case NETDEV_GOING_DOWN:
 		if (netif_carrier_ok(dev))
-			lapb_disconnect_request(dev);
+			__lapb_disconnect_request(lapb);
 		break;
 	case NETDEV_DOWN:
 		lapb_dbg(0, "(%p) Interface down: %s\n", dev, dev->name);
@@ -489,6 +521,7 @@ static int lapb_device_event(struct notifier_block *this, unsigned long event,
 		break;
 	}
 
+	spin_unlock_bh(&lapb->lock);
 	lapb_put(lapb);
 	return NOTIFY_DONE;
 }
diff --git a/net/lapb/lapb_timer.c b/net/lapb/lapb_timer.c
index baa247fe4ed0..0230b272b7d1 100644
--- a/net/lapb/lapb_timer.c
+++ b/net/lapb/lapb_timer.c
@@ -40,6 +40,7 @@ void lapb_start_t1timer(struct lapb_cb *lapb)
 	lapb->t1timer.function = lapb_t1timer_expiry;
 	lapb->t1timer.expires  = jiffies + lapb->t1;
 
+	lapb->t1timer_stop = false;
 	add_timer(&lapb->t1timer);
 }
 
@@ -50,16 +51,19 @@ void lapb_start_t2timer(struct lapb_cb *lapb)
 	lapb->t2timer.function = lapb_t2timer_expiry;
 	lapb->t2timer.expires  = jiffies + lapb->t2;
 
+	lapb->t2timer_stop = false;
 	add_timer(&lapb->t2timer);
 }
 
 void lapb_stop_t1timer(struct lapb_cb *lapb)
 {
+	lapb->t1timer_stop = true;
 	del_timer(&lapb->t1timer);
 }
 
 void lapb_stop_t2timer(struct lapb_cb *lapb)
 {
+	lapb->t2timer_stop = true;
 	del_timer(&lapb->t2timer);
 }
 
@@ -72,16 +76,31 @@ static void lapb_t2timer_expiry(struct timer_list *t)
 {
 	struct lapb_cb *lapb = from_timer(lapb, t, t2timer);
 
+	spin_lock_bh(&lapb->lock);
+	if (timer_pending(&lapb->t2timer)) /* A new timer has been set up */
+		goto out;
+	if (lapb->t2timer_stop) /* The timer has been stopped */
+		goto out;
+
 	if (lapb->condition & LAPB_ACK_PENDING_CONDITION) {
 		lapb->condition &= ~LAPB_ACK_PENDING_CONDITION;
 		lapb_timeout_response(lapb);
 	}
+
+out:
+	spin_unlock_bh(&lapb->lock);
 }
 
 static void lapb_t1timer_expiry(struct timer_list *t)
 {
 	struct lapb_cb *lapb = from_timer(lapb, t, t1timer);
 
+	spin_lock_bh(&lapb->lock);
+	if (timer_pending(&lapb->t1timer)) /* A new timer has been set up */
+		goto out;
+	if (lapb->t1timer_stop) /* The timer has been stopped */
+		goto out;
+
 	switch (lapb->state) {
 
 		/*
@@ -108,7 +127,7 @@ static void lapb_t1timer_expiry(struct timer_list *t)
 				lapb->state = LAPB_STATE_0;
 				lapb_disconnect_indication(lapb, LAPB_TIMEDOUT);
 				lapb_dbg(0, "(%p) S1 -> S0\n", lapb->dev);
-				return;
+				goto out;
 			} else {
 				lapb->n2count++;
 				if (lapb->mode & LAPB_EXTENDED) {
@@ -132,7 +151,7 @@ static void lapb_t1timer_expiry(struct timer_list *t)
 				lapb->state = LAPB_STATE_0;
 				lapb_disconnect_confirmation(lapb, LAPB_TIMEDOUT);
 				lapb_dbg(0, "(%p) S2 -> S0\n", lapb->dev);
-				return;
+				goto out;
 			} else {
 				lapb->n2count++;
 				lapb_dbg(1, "(%p) S2 TX DISC(1)\n", lapb->dev);
@@ -150,7 +169,7 @@ static void lapb_t1timer_expiry(struct timer_list *t)
 				lapb_stop_t2timer(lapb);
 				lapb_disconnect_indication(lapb, LAPB_TIMEDOUT);
 				lapb_dbg(0, "(%p) S3 -> S0\n", lapb->dev);
-				return;
+				goto out;
 			} else {
 				lapb->n2count++;
 				lapb_requeue_frames(lapb);
@@ -167,7 +186,7 @@ static void lapb_t1timer_expiry(struct timer_list *t)
 				lapb->state = LAPB_STATE_0;
 				lapb_disconnect_indication(lapb, LAPB_TIMEDOUT);
 				lapb_dbg(0, "(%p) S4 -> S0\n", lapb->dev);
-				return;
+				goto out;
 			} else {
 				lapb->n2count++;
 				lapb_transmit_frmr(lapb);
@@ -176,4 +195,7 @@ static void lapb_t1timer_expiry(struct timer_list *t)
 	}
 
 	lapb_start_t1timer(lapb);
+
+out:
+	spin_unlock_bh(&lapb->lock);
 }
-- 
2.27.0

Re: [PATCH net v4] net: lapb: Add locking to the lapb module

[Martin Schiller]

On 2021-01-20 11:28, Xie He wrote:
> In the lapb module, the timers may run concurrently with other code in
> this module, and there is currently no locking to prevent the code from
> racing on "struct lapb_cb". This patch adds locking to prevent racing.
> 
> 1. Add "spinlock_t lock" to "struct lapb_cb"; Add "spin_lock_bh" and
> "spin_unlock_bh" to APIs, timer functions and notifier functions.
> 
> 2. Add "bool t1timer_stop, t2timer_stop" to "struct lapb_cb" to make us
> able to ask running timers to abort; Modify "lapb_stop_t1timer" and
> "lapb_stop_t2timer" to make them able to abort running timers;
> Modify "lapb_t2timer_expiry" and "lapb_t1timer_expiry" to make them
> abort after they are stopped by "lapb_stop_t1timer", 
> "lapb_stop_t2timer",
> and "lapb_start_t1timer", "lapb_start_t2timer".
> 
> 3. In lapb_unregister, add "del_timer_sync" calls to make sure all
> running timers have exited.
> 
> 4. The lapb_device_event function calls lapb_disconnect_request. In
> order to avoid trying to hold the lock twice, add a new function named
> "__lapb_disconnect_request" which assumes the lock is held, and make
> it called by lapb_disconnect_request and lapb_device_event.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: Martin Schiller <ms@dev.tdt.de>
> Signed-off-by: Xie He <xie.he.0141@gmail.com>


Can you please add a Changelog. What was changed in v4?

> ---
>  include/net/lapb.h    |  2 ++
>  net/lapb/lapb_iface.c | 65 ++++++++++++++++++++++++++++++++-----------
>  net/lapb/lapb_timer.c | 30 +++++++++++++++++---
>  3 files changed, 77 insertions(+), 20 deletions(-)
> 
> diff --git a/include/net/lapb.h b/include/net/lapb.h
> index ccc3d1f020b0..eee73442a1ba 100644
> --- a/include/net/lapb.h
> +++ b/include/net/lapb.h
> @@ -92,6 +92,7 @@ struct lapb_cb {
>  	unsigned short		n2, n2count;
>  	unsigned short		t1, t2;
>  	struct timer_list	t1timer, t2timer;
> +	bool			t1timer_stop, t2timer_stop;
> 
>  	/* Internal control information */
>  	struct sk_buff_head	write_queue;
> @@ -103,6 +104,7 @@ struct lapb_cb {
>  	struct lapb_frame	frmr_data;
>  	unsigned char		frmr_type;
> 
> +	spinlock_t		lock;
>  	refcount_t		refcnt;
>  };
> 
> diff --git a/net/lapb/lapb_iface.c b/net/lapb/lapb_iface.c
> index 40961889e9c0..205f8736e68b 100644
> --- a/net/lapb/lapb_iface.c
> +++ b/net/lapb/lapb_iface.c
> @@ -122,6 +122,8 @@ static struct lapb_cb *lapb_create_cb(void)
> 
>  	timer_setup(&lapb->t1timer, NULL, 0);
>  	timer_setup(&lapb->t2timer, NULL, 0);
> +	lapb->t1timer_stop = true;
> +	lapb->t2timer_stop = true;
> 
>  	lapb->t1      = LAPB_DEFAULT_T1;
>  	lapb->t2      = LAPB_DEFAULT_T2;
> @@ -129,6 +131,8 @@ static struct lapb_cb *lapb_create_cb(void)
>  	lapb->mode    = LAPB_DEFAULT_MODE;
>  	lapb->window  = LAPB_DEFAULT_WINDOW;
>  	lapb->state   = LAPB_STATE_0;
> +
> +	spin_lock_init(&lapb->lock);
>  	refcount_set(&lapb->refcnt, 1);
>  out:
>  	return lapb;
> @@ -178,11 +182,18 @@ int lapb_unregister(struct net_device *dev)
>  		goto out;
>  	lapb_put(lapb);
> 
> +	spin_lock_bh(&lapb->lock);
> +
>  	lapb_stop_t1timer(lapb);
>  	lapb_stop_t2timer(lapb);
> 
>  	lapb_clear_queues(lapb);
> 
> +	spin_unlock_bh(&lapb->lock);
> +
> +	del_timer_sync(&lapb->t1timer);
> +	del_timer_sync(&lapb->t2timer);
> +
>  	__lapb_remove_cb(lapb);
> 
>  	lapb_put(lapb);
> @@ -201,6 +212,8 @@ int lapb_getparms(struct net_device *dev, struct
> lapb_parms_struct *parms)
>  	if (!lapb)
>  		goto out;
> 
> +	spin_lock_bh(&lapb->lock);
> +
>  	parms->t1      = lapb->t1 / HZ;
>  	parms->t2      = lapb->t2 / HZ;
>  	parms->n2      = lapb->n2;
> @@ -219,6 +232,7 @@ int lapb_getparms(struct net_device *dev, struct
> lapb_parms_struct *parms)
>  	else
>  		parms->t2timer = (lapb->t2timer.expires - jiffies) / HZ;
> 
> +	spin_unlock_bh(&lapb->lock);
>  	lapb_put(lapb);
>  	rc = LAPB_OK;
>  out:
> @@ -234,6 +248,8 @@ int lapb_setparms(struct net_device *dev, struct
> lapb_parms_struct *parms)
>  	if (!lapb)
>  		goto out;
> 
> +	spin_lock_bh(&lapb->lock);
> +
>  	rc = LAPB_INVALUE;
>  	if (parms->t1 < 1 || parms->t2 < 1 || parms->n2 < 1)
>  		goto out_put;
> @@ -256,6 +272,7 @@ int lapb_setparms(struct net_device *dev, struct
> lapb_parms_struct *parms)
> 
>  	rc = LAPB_OK;
>  out_put:
> +	spin_unlock_bh(&lapb->lock);
>  	lapb_put(lapb);
>  out:
>  	return rc;
> @@ -270,6 +287,8 @@ int lapb_connect_request(struct net_device *dev)
>  	if (!lapb)
>  		goto out;
> 
> +	spin_lock_bh(&lapb->lock);
> +
>  	rc = LAPB_OK;
>  	if (lapb->state == LAPB_STATE_1)
>  		goto out_put;
> @@ -285,24 +304,18 @@ int lapb_connect_request(struct net_device *dev)
> 
>  	rc = LAPB_OK;
>  out_put:
> +	spin_unlock_bh(&lapb->lock);
>  	lapb_put(lapb);
>  out:
>  	return rc;
>  }
>  EXPORT_SYMBOL(lapb_connect_request);
> 
> -int lapb_disconnect_request(struct net_device *dev)
> +static int __lapb_disconnect_request(struct lapb_cb *lapb)
>  {
> -	struct lapb_cb *lapb = lapb_devtostruct(dev);
> -	int rc = LAPB_BADTOKEN;
> -
> -	if (!lapb)
> -		goto out;
> -
>  	switch (lapb->state) {
>  	case LAPB_STATE_0:
> -		rc = LAPB_NOTCONNECTED;
> -		goto out_put;
> +		return LAPB_NOTCONNECTED;
> 
>  	case LAPB_STATE_1:
>  		lapb_dbg(1, "(%p) S1 TX DISC(1)\n", lapb->dev);
> @@ -310,12 +323,10 @@ int lapb_disconnect_request(struct net_device 
> *dev)
>  		lapb_send_control(lapb, LAPB_DISC, LAPB_POLLON, LAPB_COMMAND);
>  		lapb->state = LAPB_STATE_0;
>  		lapb_start_t1timer(lapb);
> -		rc = LAPB_NOTCONNECTED;
> -		goto out_put;
> +		return LAPB_NOTCONNECTED;
> 
>  	case LAPB_STATE_2:
> -		rc = LAPB_OK;
> -		goto out_put;
> +		return LAPB_OK;
>  	}
> 
>  	lapb_clear_queues(lapb);
> @@ -328,8 +339,22 @@ int lapb_disconnect_request(struct net_device 
> *dev)
>  	lapb_dbg(1, "(%p) S3 DISC(1)\n", lapb->dev);
>  	lapb_dbg(0, "(%p) S3 -> S2\n", lapb->dev);
> 
> -	rc = LAPB_OK;
> -out_put:
> +	return LAPB_OK;
> +}
> +
> +int lapb_disconnect_request(struct net_device *dev)
> +{
> +	struct lapb_cb *lapb = lapb_devtostruct(dev);
> +	int rc = LAPB_BADTOKEN;
> +
> +	if (!lapb)
> +		goto out;
> +
> +	spin_lock_bh(&lapb->lock);
> +
> +	rc = __lapb_disconnect_request(lapb);
> +
> +	spin_unlock_bh(&lapb->lock);
>  	lapb_put(lapb);
>  out:
>  	return rc;
> @@ -344,6 +369,8 @@ int lapb_data_request(struct net_device *dev,
> struct sk_buff *skb)
>  	if (!lapb)
>  		goto out;
> 
> +	spin_lock_bh(&lapb->lock);
> +
>  	rc = LAPB_NOTCONNECTED;
>  	if (lapb->state != LAPB_STATE_3 && lapb->state != LAPB_STATE_4)
>  		goto out_put;
> @@ -352,6 +379,7 @@ int lapb_data_request(struct net_device *dev,
> struct sk_buff *skb)
>  	lapb_kick(lapb);
>  	rc = LAPB_OK;
>  out_put:
> +	spin_unlock_bh(&lapb->lock);
>  	lapb_put(lapb);
>  out:
>  	return rc;
> @@ -364,7 +392,9 @@ int lapb_data_received(struct net_device *dev,
> struct sk_buff *skb)
>  	int rc = LAPB_BADTOKEN;
> 
>  	if (lapb) {
> +		spin_lock_bh(&lapb->lock);
>  		lapb_data_input(lapb, skb);
> +		spin_unlock_bh(&lapb->lock);
>  		lapb_put(lapb);
>  		rc = LAPB_OK;
>  	}
> @@ -435,6 +465,8 @@ static int lapb_device_event(struct notifier_block
> *this, unsigned long event,
>  	if (!lapb)
>  		return NOTIFY_DONE;
> 
> +	spin_lock_bh(&lapb->lock);
> +
>  	switch (event) {
>  	case NETDEV_UP:
>  		lapb_dbg(0, "(%p) Interface up: %s\n", dev, dev->name);
> @@ -454,7 +486,7 @@ static int lapb_device_event(struct notifier_block
> *this, unsigned long event,
>  		break;
>  	case NETDEV_GOING_DOWN:
>  		if (netif_carrier_ok(dev))
> -			lapb_disconnect_request(dev);
> +			__lapb_disconnect_request(lapb);
>  		break;
>  	case NETDEV_DOWN:
>  		lapb_dbg(0, "(%p) Interface down: %s\n", dev, dev->name);
> @@ -489,6 +521,7 @@ static int lapb_device_event(struct notifier_block
> *this, unsigned long event,
>  		break;
>  	}
> 
> +	spin_unlock_bh(&lapb->lock);
>  	lapb_put(lapb);
>  	return NOTIFY_DONE;
>  }
> diff --git a/net/lapb/lapb_timer.c b/net/lapb/lapb_timer.c
> index baa247fe4ed0..0230b272b7d1 100644
> --- a/net/lapb/lapb_timer.c
> +++ b/net/lapb/lapb_timer.c
> @@ -40,6 +40,7 @@ void lapb_start_t1timer(struct lapb_cb *lapb)
>  	lapb->t1timer.function = lapb_t1timer_expiry;
>  	lapb->t1timer.expires  = jiffies + lapb->t1;
> 
> +	lapb->t1timer_stop = false;
>  	add_timer(&lapb->t1timer);
>  }
> 
> @@ -50,16 +51,19 @@ void lapb_start_t2timer(struct lapb_cb *lapb)
>  	lapb->t2timer.function = lapb_t2timer_expiry;
>  	lapb->t2timer.expires  = jiffies + lapb->t2;
> 
> +	lapb->t2timer_stop = false;
>  	add_timer(&lapb->t2timer);
>  }
> 
>  void lapb_stop_t1timer(struct lapb_cb *lapb)
>  {
> +	lapb->t1timer_stop = true;
>  	del_timer(&lapb->t1timer);
>  }
> 
>  void lapb_stop_t2timer(struct lapb_cb *lapb)
>  {
> +	lapb->t2timer_stop = true;
>  	del_timer(&lapb->t2timer);
>  }
> 
> @@ -72,16 +76,31 @@ static void lapb_t2timer_expiry(struct timer_list 
> *t)
>  {
>  	struct lapb_cb *lapb = from_timer(lapb, t, t2timer);
> 
> +	spin_lock_bh(&lapb->lock);
> +	if (timer_pending(&lapb->t2timer)) /* A new timer has been set up */
> +		goto out;
> +	if (lapb->t2timer_stop) /* The timer has been stopped */
> +		goto out;
> +
>  	if (lapb->condition & LAPB_ACK_PENDING_CONDITION) {
>  		lapb->condition &= ~LAPB_ACK_PENDING_CONDITION;
>  		lapb_timeout_response(lapb);
>  	}
> +
> +out:
> +	spin_unlock_bh(&lapb->lock);
>  }
> 
>  static void lapb_t1timer_expiry(struct timer_list *t)
>  {
>  	struct lapb_cb *lapb = from_timer(lapb, t, t1timer);
> 
> +	spin_lock_bh(&lapb->lock);
> +	if (timer_pending(&lapb->t1timer)) /* A new timer has been set up */
> +		goto out;
> +	if (lapb->t1timer_stop) /* The timer has been stopped */
> +		goto out;
> +
>  	switch (lapb->state) {
> 
>  		/*
> @@ -108,7 +127,7 @@ static void lapb_t1timer_expiry(struct timer_list 
> *t)
>  				lapb->state = LAPB_STATE_0;
>  				lapb_disconnect_indication(lapb, LAPB_TIMEDOUT);
>  				lapb_dbg(0, "(%p) S1 -> S0\n", lapb->dev);
> -				return;
> +				goto out;
>  			} else {
>  				lapb->n2count++;
>  				if (lapb->mode & LAPB_EXTENDED) {
> @@ -132,7 +151,7 @@ static void lapb_t1timer_expiry(struct timer_list 
> *t)
>  				lapb->state = LAPB_STATE_0;
>  				lapb_disconnect_confirmation(lapb, LAPB_TIMEDOUT);
>  				lapb_dbg(0, "(%p) S2 -> S0\n", lapb->dev);
> -				return;
> +				goto out;
>  			} else {
>  				lapb->n2count++;
>  				lapb_dbg(1, "(%p) S2 TX DISC(1)\n", lapb->dev);
> @@ -150,7 +169,7 @@ static void lapb_t1timer_expiry(struct timer_list 
> *t)
>  				lapb_stop_t2timer(lapb);
>  				lapb_disconnect_indication(lapb, LAPB_TIMEDOUT);
>  				lapb_dbg(0, "(%p) S3 -> S0\n", lapb->dev);
> -				return;
> +				goto out;
>  			} else {
>  				lapb->n2count++;
>  				lapb_requeue_frames(lapb);
> @@ -167,7 +186,7 @@ static void lapb_t1timer_expiry(struct timer_list 
> *t)
>  				lapb->state = LAPB_STATE_0;
>  				lapb_disconnect_indication(lapb, LAPB_TIMEDOUT);
>  				lapb_dbg(0, "(%p) S4 -> S0\n", lapb->dev);
> -				return;
> +				goto out;
>  			} else {
>  				lapb->n2count++;
>  				lapb_transmit_frmr(lapb);
> @@ -176,4 +195,7 @@ static void lapb_t1timer_expiry(struct timer_list 
> *t)
>  	}
> 
>  	lapb_start_t1timer(lapb);
> +
> +out:
> +	spin_unlock_bh(&lapb->lock);
>  }

Re: [PATCH net v4] net: lapb: Add locking to the lapb module

[Xie He]

On Wed, Jan 20, 2021 at 2:58 AM Martin Schiller <ms@dev.tdt.de> wrote:
>
> Can you please add a Changelog. What was changed in v4?

Sorry, I forgot this. Here is the change log:

--- Changes from v3 to v4 ---

Only lapb_unregister has been changed.

v3 has a problem. When "del_timer_sync(&lapb->t1timer)" is called, if
the t1timer is running, it may restart itself by calling
lapb_start_t1timer. This way, del_timer_sync would not be able to
guarantee the t1timer has been completely stopped.

v4 fixed this problem by first calling lapb_stop_t1timer, making use
of its (new) ability of aborting running timers, and then calling
del_timer_sync to guarantee the t1timer has been stopped.

--- Changes from v2 to v3 ---

Created a new __lapb_disconnect_request function and made it be called
from both lapb_disconnect_request and lapb_device_event. This reduced
redundant code.

--- Changes from v1 to v2 ---

Broke long lines to keep the line lengths within 80 characters.

Re: [PATCH net v4] net: lapb: Add locking to the lapb module

[Xie He]

With this patch, there is still a problem that lapb_unregister may run
concurrently with other LAPB API functions (such as
lapb_data_received). This other LAPB API function can get the
lapb->lock after lapb->lock is released by lapb_unregister, and
continue to do its work. This is not correct.

We can fix this problem by adding a new field "bool stop" to "struct
lapb_cb" (just like "bool t1timer_stop, t2timer_stop"), and make every
API function abort whenever it sees lapb->stop == true after getting
the lock.

Alternatively we can also require the callers (the LAPB drivers) to
never call lapb_unregister concurrently with other LAPB APIs. They
should make sure all LAPB API functions are only called after
lapb_register ends and before lapb_unregister starts. This is a
reasonable requirement, because if they don't follow this requirement,
even if we do the fix in the LAPB module (as said above), the LAPB
driver will still get the "LAPB_BADTOKEN" error from the LAPB module.
This is not desirable and I think LAPB drivers should avoid this from
happening.

So I think this problem may not need to be fixed here in the LAPB
module because the LAPB drivers should deal with this problem anyway.

Please feel free to share your comment. Thanks!

Re: [PATCH net v4] net: lapb: Add locking to the lapb module

[Xie He]

On Wed, Jan 20, 2021 at 12:42 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> With this patch, there is still a problem that lapb_unregister may run
> concurrently with other LAPB API functions (such as
> lapb_data_received). This other LAPB API function can get the
> lapb->lock after lapb->lock is released by lapb_unregister, and
> continue to do its work. This is not correct.
>
> We can fix this problem by adding a new field "bool stop" to "struct
> lapb_cb" (just like "bool t1timer_stop, t2timer_stop"), and make every
> API function abort whenever it sees lapb->stop == true after getting
> the lock.
>
> Alternatively we can also require the callers (the LAPB drivers) to
> never call lapb_unregister concurrently with other LAPB APIs. They
> should make sure all LAPB API functions are only called after
> lapb_register ends and before lapb_unregister starts. This is a
> reasonable requirement, because if they don't follow this requirement,
> even if we do the fix in the LAPB module (as said above), the LAPB
> driver will still get the "LAPB_BADTOKEN" error from the LAPB module.
> This is not desirable and I think LAPB drivers should avoid this from
> happening.
>
> So I think this problem may not need to be fixed here in the LAPB
> module because the LAPB drivers should deal with this problem anyway.

Never mind, I have sent a v5 to deal with this problem. In v5, I made
lapb_unregister wait for the "lapb" refcnt to drop, so that we can
make sure all other API calls have finished. Please see my v5.