| Message ID | 20210121002129.93754-1-xie.he.0141@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v5] net: lapb: Add locking to the lapb module | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 5 of 5 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | CHECK: spinlock_t definition without comment |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
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 <ms@dev.tdt.de> > Signed-off-by: Xie He <xie.he.0141@gmail.com> 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); > }
On Fri, Jan 22, 2021 at 1:07 AM Martin Schiller <ms@dev.tdt.de> 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.
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 <ms@dev.tdt.de> > > Signed-off-by: Xie He <xie.he.0141@gmail.com> > > 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).
On Sat, Jan 23, 2021 at 8:45 PM Jakub Kicinski <kuba@kernel.org> 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".
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 <ms@dev.tdt.de> >> > Signed-off-by: Xie He <xie.he.0141@gmail.com> >> >> 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).
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); }
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 <ms@dev.tdt.de> Signed-off-by: Xie He <xie.he.0141@gmail.com> --- 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(-)