Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Tue, 19 Jan 2021 12:34:46 +0100
From:   Martin Schiller <ms@dev.tdt.de>
To:     Xie He <xie.he.0141@gmail.com>
Cc:     "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>, linux-x25@vger.kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH net v2] net: lapb: Add locking to the lapb module
Organization: TDT AG
In-Reply-To: <20210117224750.6572-1-xie.he.0141@gmail.com>
References: <20210117224750.6572-1-xie.he.0141@gmail.com>
Message-ID: <6fb2a40bf347997416cd38454d1b194a@dev.tdt.de>
User-Agent: Roundcube Webmail/1.3.16
Precedence: bulk

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);
>  }