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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]<

 



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@xxxxxxxxxx>
> > Signed-off-by: Xie He <xie.he.0141@xxxxxxxxx>  
> 
> 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).





[Index of Archives]     [Linux ACPI]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Photo]     [Yosemite Photos]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux