lkml.org 
[lkml]   [2021]   [Jan]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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 <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).


    \
     
     \ /
      Last update: 2021-01-24 05:47    [W:2.567 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site