* [PATCH net v2] net: lapb: Add locking to the lapb module
@ 2021-01-17 22:47 Xie He
2021-01-19 11:34 ` Martin Schiller
0 siblings, 1 reply; 3+ messages in thread
From: Xie He @ 2021-01-17 22:47 UTC (permalink / raw)
To: David S. Miller, Jakub Kicinski, linux-x25, netdev, linux-kernel,
Martin Schiller
Cc: 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, change "lapb_stop_t1timer" and "lapb_stop_t2timer"
to "del_timer_sync" to make sure all running timers have exited.
4. In lapb_device_event, replace the "lapb_disconnect_request" call with
the content of "lapb_disconnect_request", to avoid trying to hold the
lock twice. When I do this, I removed "lapb_start_t1timer" because I
don't think it's necessary to start the timer when "NETDEV_GOING_DOWN".
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 | 56 +++++++++++++++++++++++++++++++++++++++----
net/lapb/lapb_timer.c | 30 +++++++++++++++++++----
3 files changed, 80 insertions(+), 8 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..45f332607685 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,8 +182,8 @@ int lapb_unregister(struct net_device *dev)
goto out;
lapb_put(lapb);
- lapb_stop_t1timer(lapb);
- lapb_stop_t2timer(lapb);
+ del_timer_sync(&lapb->t1timer);
+ del_timer_sync(&lapb->t2timer);
lapb_clear_queues(lapb);
@@ -201,6 +205,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 +225,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 +241,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 +265,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 +280,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,6 +297,7 @@ int lapb_connect_request(struct net_device *dev)
rc = LAPB_OK;
out_put:
+ spin_unlock_bh(&lapb->lock);
lapb_put(lapb);
out:
return rc;
@@ -299,6 +312,8 @@ int lapb_disconnect_request(struct net_device *dev)
if (!lapb)
goto out;
+ spin_lock_bh(&lapb->lock);
+
switch (lapb->state) {
case LAPB_STATE_0:
rc = LAPB_NOTCONNECTED;
@@ -330,6 +345,7 @@ int lapb_disconnect_request(struct net_device *dev)
rc = LAPB_OK;
out_put:
+ spin_unlock_bh(&lapb->lock);
lapb_put(lapb);
out:
return rc;
@@ -344,6 +360,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 +370,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 +383,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 +456,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);
@@ -453,8 +476,32 @@ 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);
+ switch (lapb->state) {
+ case LAPB_STATE_0:
+ break;
+
+ case LAPB_STATE_1:
+ lapb_dbg(1, "(%p) S1 TX DISC(1)\n", lapb->dev);
+ lapb_dbg(0, "(%p) S1 -> S0\n", lapb->dev);
+ lapb_send_control(lapb, LAPB_DISC, LAPB_POLLON,
+ LAPB_COMMAND);
+ lapb->state = LAPB_STATE_0;
+ break;
+
+ case LAPB_STATE_2:
+ break;
+
+ default:
+ lapb_clear_queues(lapb);
+ lapb->n2count = 0;
+ lapb_send_control(lapb, LAPB_DISC, LAPB_POLLON,
+ LAPB_COMMAND);
+ lapb_stop_t2timer(lapb);
+ lapb->state = LAPB_STATE_2;
+ lapb_dbg(1, "(%p) S3 DISC(1)\n", lapb->dev);
+ lapb_dbg(0, "(%p) S3 -> S2\n", lapb->dev);
+ }
+
break;
case NETDEV_DOWN:
lapb_dbg(0, "(%p) Interface down: %s\n", dev, dev->name);
@@ -489,6 +536,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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH net v2] net: lapb: Add locking to the lapb module
2021-01-17 22:47 [PATCH net v2] net: lapb: Add locking to the lapb module Xie He
@ 2021-01-19 11:34 ` Martin Schiller
2021-01-19 13:54 ` Xie He
0 siblings, 1 reply; 3+ messages in thread
From: Martin Schiller @ 2021-01-19 11:34 UTC (permalink / raw)
To: Xie He; +Cc: David S. Miller, Jakub Kicinski, linux-x25, netdev, linux-kernel
On 2021-01-17 23:47, 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, change "lapb_stop_t1timer" and
> "lapb_stop_t2timer"
> to "del_timer_sync" to make sure all running timers have exited.
>
> 4. In lapb_device_event, replace the "lapb_disconnect_request" call
> with
> the content of "lapb_disconnect_request", to avoid trying to hold the
> lock twice. When I do this, I removed "lapb_start_t1timer" because I
> don't think it's necessary to start the timer when "NETDEV_GOING_DOWN".
I don't like the code redundancy this creates. Maybe you should move the
actual functionality from lapb_disconnect_request() to a
__lapb_disconnect_request(), and in lapb_disconnect_request() call this
function including locking around it and also in lapb_device_event
(without locking).
Calling lapb_start_t1timer() on a "NETDEV_GOING_DOWN" event does not
hurt and is correct from a protocol flow point of view after sending
the DISC.
>
> 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 | 56 +++++++++++++++++++++++++++++++++++++++----
> net/lapb/lapb_timer.c | 30 +++++++++++++++++++----
> 3 files changed, 80 insertions(+), 8 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..45f332607685 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,8 +182,8 @@ int lapb_unregister(struct net_device *dev)
> goto out;
> lapb_put(lapb);
>
> - lapb_stop_t1timer(lapb);
> - lapb_stop_t2timer(lapb);
> + del_timer_sync(&lapb->t1timer);
> + del_timer_sync(&lapb->t2timer);
>
> lapb_clear_queues(lapb);
>
> @@ -201,6 +205,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 +225,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 +241,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 +265,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 +280,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,6 +297,7 @@ int lapb_connect_request(struct net_device *dev)
>
> rc = LAPB_OK;
> out_put:
> + spin_unlock_bh(&lapb->lock);
> lapb_put(lapb);
> out:
> return rc;
> @@ -299,6 +312,8 @@ int lapb_disconnect_request(struct net_device *dev)
> if (!lapb)
> goto out;
>
> + spin_lock_bh(&lapb->lock);
> +
> switch (lapb->state) {
> case LAPB_STATE_0:
> rc = LAPB_NOTCONNECTED;
> @@ -330,6 +345,7 @@ int lapb_disconnect_request(struct net_device *dev)
>
> rc = LAPB_OK;
> out_put:
> + spin_unlock_bh(&lapb->lock);
> lapb_put(lapb);
> out:
> return rc;
> @@ -344,6 +360,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 +370,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 +383,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 +456,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);
> @@ -453,8 +476,32 @@ 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);
> + switch (lapb->state) {
> + case LAPB_STATE_0:
> + break;
> +
> + case LAPB_STATE_1:
> + lapb_dbg(1, "(%p) S1 TX DISC(1)\n", lapb->dev);
> + lapb_dbg(0, "(%p) S1 -> S0\n", lapb->dev);
> + lapb_send_control(lapb, LAPB_DISC, LAPB_POLLON,
> + LAPB_COMMAND);
> + lapb->state = LAPB_STATE_0;
> + break;
> +
> + case LAPB_STATE_2:
> + break;
> +
> + default:
> + lapb_clear_queues(lapb);
> + lapb->n2count = 0;
> + lapb_send_control(lapb, LAPB_DISC, LAPB_POLLON,
> + LAPB_COMMAND);
> + lapb_stop_t2timer(lapb);
> + lapb->state = LAPB_STATE_2;
> + lapb_dbg(1, "(%p) S3 DISC(1)\n", lapb->dev);
> + lapb_dbg(0, "(%p) S3 -> S2\n", lapb->dev);
> + }
> +
> break;
> case NETDEV_DOWN:
> lapb_dbg(0, "(%p) Interface down: %s\n", dev, dev->name);
> @@ -489,6 +536,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);
> }
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH net v2] net: lapb: Add locking to the lapb module
2021-01-19 11:34 ` Martin Schiller
@ 2021-01-19 13:54 ` Xie He
0 siblings, 0 replies; 3+ messages in thread
From: Xie He @ 2021-01-19 13:54 UTC (permalink / raw)
To: Martin Schiller
Cc: David S. Miller, Jakub Kicinski, Linux X25,
Linux Kernel Network Developers, LKML
On Tue, Jan 19, 2021 at 3:34 AM Martin Schiller <ms@dev.tdt.de> wrote:
>
> > 4. In lapb_device_event, replace the "lapb_disconnect_request" call
> > with
> > the content of "lapb_disconnect_request", to avoid trying to hold the
> > lock twice. When I do this, I removed "lapb_start_t1timer" because I
> > don't think it's necessary to start the timer when "NETDEV_GOING_DOWN".
>
> I don't like the code redundancy this creates. Maybe you should move the
> actual functionality from lapb_disconnect_request() to a
> __lapb_disconnect_request(), and in lapb_disconnect_request() call this
> function including locking around it and also in lapb_device_event
> (without locking).
>
> Calling lapb_start_t1timer() on a "NETDEV_GOING_DOWN" event does not
> hurt and is correct from a protocol flow point of view after sending
> the DISC.
Thanks! I created a new __lapb_disconnect_request function and the
code indeed looked cleaner. I'll send a new version.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-01-20 0:11 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-17 22:47 [PATCH net v2] net: lapb: Add locking to the lapb module Xie He
2021-01-19 11:34 ` Martin Schiller
2021-01-19 13:54 ` Xie He
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).