2021-01-21 03:10:25

by Xie He

[permalink] [raw]
Subject: [PATCH net v5] net: lapb: Add locking to the lapb module

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. Let lapb_unregister wait for other API functions and running timers
to stop.

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 <[email protected]>
Signed-off-by: Xie He <[email protected]>
---

Change from v4:
Make lapb_unregister wait for other refs to "lapb" to drop, to ensure
that other LAPB API calls have all finished.

Change from v3:
In lapb_unregister make sure the self-restarting t1timer has really been
stopped.

Change from v2:
Create a new __lapb_disconnect_request function to reduce redundant code.

Change from v1:
Broke long lines to keep the line lengths within 80 characters.

---
include/net/lapb.h | 2 ++
net/lapb/lapb_iface.c | 70 +++++++++++++++++++++++++++++++++----------
net/lapb/lapb_timer.c | 30 ++++++++++++++++---
3 files changed, 82 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..b028dfc438ce 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,23 @@ int lapb_unregister(struct net_device *dev)
goto out;
lapb_put(lapb);

+ /* Wait for other refs to "lapb" to drop */
+ while (refcount_read(&lapb->refcnt) > 2)
+ ;
+
+ spin_lock_bh(&lapb->lock);
+
lapb_stop_t1timer(lapb);
lapb_stop_t2timer(lapb);

lapb_clear_queues(lapb);

+ spin_unlock_bh(&lapb->lock);
+
+ /* Wait for running timers to stop */
+ del_timer_sync(&lapb->t1timer);
+ del_timer_sync(&lapb->t2timer);
+
__lapb_remove_cb(lapb);

lapb_put(lapb);
@@ -201,6 +217,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 +237,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 +253,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 +277,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 +292,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 +309,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 +328,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 +344,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 +374,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 +384,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 +397,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 +470,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 +491,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 +526,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


2021-01-22 09:43:18

by Martin Schiller

[permalink] [raw]
Subject: Re: [PATCH net v5] net: lapb: Add locking to the lapb module

On 2021-01-21 01:21, 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. Let lapb_unregister wait for other API functions and running timers
> to stop.
>
> 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 <[email protected]>
> Signed-off-by: Xie He <[email protected]>


I don't have the opportunity to test this at the moment, but code looks
reasonable so far. Have you tested this at runtime?

> ---
>
> Change from v4:
> Make lapb_unregister wait for other refs to "lapb" to drop, to ensure
> that other LAPB API calls have all finished.
>
> Change from v3:
> In lapb_unregister make sure the self-restarting t1timer has really
> been
> stopped.
>
> Change from v2:
> Create a new __lapb_disconnect_request function to reduce redundant
> code.
>
> Change from v1:
> Broke long lines to keep the line lengths within 80 characters.
>
> ---
> include/net/lapb.h | 2 ++
> net/lapb/lapb_iface.c | 70 +++++++++++++++++++++++++++++++++----------
> net/lapb/lapb_timer.c | 30 ++++++++++++++++---
> 3 files changed, 82 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..b028dfc438ce 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,23 @@ int lapb_unregister(struct net_device *dev)
> goto out;
> lapb_put(lapb);
>
> + /* Wait for other refs to "lapb" to drop */
> + while (refcount_read(&lapb->refcnt) > 2)
> + ;
> +
> + spin_lock_bh(&lapb->lock);
> +
> lapb_stop_t1timer(lapb);
> lapb_stop_t2timer(lapb);
>
> lapb_clear_queues(lapb);
>
> + spin_unlock_bh(&lapb->lock);
> +
> + /* Wait for running timers to stop */
> + del_timer_sync(&lapb->t1timer);
> + del_timer_sync(&lapb->t2timer);
> +
> __lapb_remove_cb(lapb);
>
> lapb_put(lapb);
> @@ -201,6 +217,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 +237,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 +253,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 +277,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 +292,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 +309,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 +328,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 +344,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 +374,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 +384,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 +397,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 +470,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 +491,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 +526,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);
> }

2021-01-22 21:52:43

by Xie He

[permalink] [raw]
Subject: Re: [PATCH net v5] net: lapb: Add locking to the lapb module

On Fri, Jan 22, 2021 at 1:07 AM Martin Schiller <[email protected]> wrote:
>
> I don't have the opportunity to test this at the moment, but code looks
> reasonable so far. Have you tested this at runtime?

Thanks! Yes, I have tested this using hdlc_x25.c, lapbether.c and (the
deleted) x25_asy.c drivers.

2021-01-24 04:50:25

by Jakub Kicinski

[permalink] [raw]
Subject: Re: [PATCH net v5] net: lapb: Add locking to the lapb module

On Fri, 22 Jan 2021 10:07:05 +0100 Martin Schiller wrote:
> On 2021-01-21 01:21, 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. Let lapb_unregister wait for other API functions and running timers
> > to stop.
> >
> > 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 <[email protected]>
> > Signed-off-by: Xie He <[email protected]>
>
> I don't have the opportunity to test this at the moment, but code looks
> reasonable so far. Have you tested this at runtime?

Are you okay with this being merged or would you like to review
further/test?

Nothing jumps out to me either (other than a few nit picks).

> > Change from v4:
> > Make lapb_unregister wait for other refs to "lapb" to drop, to ensure
> > that other LAPB API calls have all finished.
> >
> > Change from v3:
> > In lapb_unregister make sure the self-restarting t1timer has really
> > been
> > stopped.
> >
> > Change from v2:
> > Create a new __lapb_disconnect_request function to reduce redundant
> > code.
> >
> > Change from v1:
> > Broke long lines to keep the line lengths within 80 characters.

> > @@ -178,11 +182,23 @@ int lapb_unregister(struct net_device *dev)
> > goto out;
> > lapb_put(lapb);
> >
> > + /* Wait for other refs to "lapb" to drop */
> > + while (refcount_read(&lapb->refcnt) > 2)
> > + ;

Tight loop like this is a little scary, perhaps add a small
usleep_range() here?

> > +
> > + spin_lock_bh(&lapb->lock);
> > +
> > lapb_stop_t1timer(lapb);
> > lapb_stop_t2timer(lapb);
> >
> > lapb_clear_queues(lapb);
> >
> > + spin_unlock_bh(&lapb->lock);
> > +
> > + /* Wait for running timers to stop */
> > + del_timer_sync(&lapb->t1timer);
> > + del_timer_sync(&lapb->t2timer);
> > +
> > __lapb_remove_cb(lapb);
> >
> > lapb_put(lapb);

> > -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 +328,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 +344,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;
> > +}

Since this is a fix for net, I'd advise against converting the goto
into direct returns (as much as I generally like such conversion).


2021-01-24 11:22:25

by Xie He

[permalink] [raw]
Subject: Re: [PATCH net v5] net: lapb: Add locking to the lapb module

On Sat, Jan 23, 2021 at 8:45 PM Jakub Kicinski <[email protected]> wrote:
>
> > > @@ -178,11 +182,23 @@ int lapb_unregister(struct net_device *dev)
> > > goto out;
> > > lapb_put(lapb);
> > >
> > > + /* Wait for other refs to "lapb" to drop */
> > > + while (refcount_read(&lapb->refcnt) > 2)
> > > + ;
>
> Tight loop like this is a little scary, perhaps add a small
> usleep_range() here?

OK, sure. I'll add a usleep_range(1, 10) here.

> > > -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 +328,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 +344,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;
> > > +}
>
> Since this is a fix for net, I'd advise against converting the goto
> into direct returns (as much as I generally like such conversion).

This part is actually splitting "lapb_disconnect_request" into two
functions - a "__lapb_disconnect_request" without locking, and a
"lapb_disconnect_request" which provides the locking and calls
"__lapb_disconnect_request". The splitting is necessary for
"lapb_device_event" to directly call "__lapb_disconnect_request" with
the lock already held. After the splitting, the "out_put" tag would
actually be in the caller function so there's nowhere we can "goto".

2021-01-25 06:59:48

by Martin Schiller

[permalink] [raw]
Subject: Re: [PATCH net v5] net: lapb: Add locking to the lapb module

On 2021-01-24 05:45, Jakub Kicinski wrote:
> On Fri, 22 Jan 2021 10:07:05 +0100 Martin Schiller wrote:
>> On 2021-01-21 01:21, 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. Let lapb_unregister wait for other API functions and running timers
>> > to stop.
>> >
>> > 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 <[email protected]>
>> > Signed-off-by: Xie He <[email protected]>
>>
>> I don't have the opportunity to test this at the moment, but code
>> looks
>> reasonable so far. Have you tested this at runtime?
>
> Are you okay with this being merged or would you like to review
> further/test?
>
> Nothing jumps out to me either (other than a few nit picks).

Adding a small delay in the while loop is a good idea.
Otherwise: Yes, I agree with merging this.

>
>> > Change from v4:
>> > Make lapb_unregister wait for other refs to "lapb" to drop, to ensure
>> > that other LAPB API calls have all finished.
>> >
>> > Change from v3:
>> > In lapb_unregister make sure the self-restarting t1timer has really
>> > been
>> > stopped.
>> >
>> > Change from v2:
>> > Create a new __lapb_disconnect_request function to reduce redundant
>> > code.
>> >
>> > Change from v1:
>> > Broke long lines to keep the line lengths within 80 characters.
>
>> > @@ -178,11 +182,23 @@ int lapb_unregister(struct net_device *dev)
>> > goto out;
>> > lapb_put(lapb);
>> >
>> > + /* Wait for other refs to "lapb" to drop */
>> > + while (refcount_read(&lapb->refcnt) > 2)
>> > + ;
>
> Tight loop like this is a little scary, perhaps add a small
> usleep_range() here?
>
>> > +
>> > + spin_lock_bh(&lapb->lock);
>> > +
>> > lapb_stop_t1timer(lapb);
>> > lapb_stop_t2timer(lapb);
>> >
>> > lapb_clear_queues(lapb);
>> >
>> > + spin_unlock_bh(&lapb->lock);
>> > +
>> > + /* Wait for running timers to stop */
>> > + del_timer_sync(&lapb->t1timer);
>> > + del_timer_sync(&lapb->t2timer);
>> > +
>> > __lapb_remove_cb(lapb);
>> >
>> > lapb_put(lapb);
>
>> > -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 +328,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 +344,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;
>> > +}
>
> Since this is a fix for net, I'd advise against converting the goto
> into direct returns (as much as I generally like such conversion).