[PATCH] n_hdlc fix read and write locking

[PATCH] n_hdlc fix read and write locking

[Paul Fulghum]

Fix locking in read and write code of n_hdlc line discipline.

2.6.36 replaced lock_kernel() with tty_lock().
The tty mutex is not dropped automatically when the thread
sleeps like the BKL. This results in a blocked read or write holding
the tty mutex and stalling operations by other devices that use
the tty mutex.

A review of n_hdlc read and write code shows:
1. neither BKL or tty mutex are required for correct operation
2. read can block while read data is available if data is posted
   between availability check and call to interruptible_sleep_on()
3. write does not set process state to TASK_INTERRUPTIBLE
   on each pass through the processing loop which can cause
   unneeded scheduling of the thread

The unnecessary tty mutex references have been removed.

Read changed to use same code as n_tty read
for completing reads and blocking.

Write corrected to set process state to
TASK_INTERRUPTIBLE on each pass through processing loop.

--- a/drivers/char/n_hdlc.c	2010-10-22 14:22:22.000000000 -0500
+++ b/drivers/char/n_hdlc.c	2010-10-25 12:36:12.000000000 -0500
@@ -581,8 +581,9 @@ static ssize_t n_hdlc_tty_read(struct tt
 			   __u8 __user *buf, size_t nr)
 {
 	struct n_hdlc *n_hdlc = tty2n_hdlc(tty);
-	int ret;
+	int ret = 0;
 	struct n_hdlc_buf *rbuf;
+	DECLARE_WAITQUEUE(wait, current);
 
 	if (debuglevel >= DEBUG_LEVEL_INFO)	
 		printk("%s(%d)n_hdlc_tty_read() called\n",__FILE__,__LINE__);
@@ -598,57 +599,55 @@ static ssize_t n_hdlc_tty_read(struct tt
 		return -EFAULT;
 	}
 
-	tty_lock();
+	add_wait_queue(&tty->read_wait, &wait);
 
 	for (;;) {
 		if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) {
-			tty_unlock();
-			return -EIO;
+			ret = -EIO;
+			break;
 		}
+		if (tty_hung_up_p(file))
+			break;
 
-		n_hdlc = tty2n_hdlc (tty);
-		if (!n_hdlc || n_hdlc->magic != HDLC_MAGIC ||
-			 tty != n_hdlc->tty) {
-			tty_unlock();
-			return 0;
-		}
+		set_current_state(TASK_INTERRUPTIBLE);
 
 		rbuf = n_hdlc_buf_get(&n_hdlc->rx_buf_list);
-		if (rbuf)
+		if (rbuf) {
+			if (rbuf->count > nr) {
+				/* too large for caller's buffer */
+				ret = -EOVERFLOW;
+			} else {
+				if (copy_to_user(buf, rbuf->buf, rbuf->count))
+					ret = -EFAULT;
+				else
+					ret = rbuf->count;
+			}
+
+			if (n_hdlc->rx_free_buf_list.count >
+			    DEFAULT_RX_BUF_COUNT)
+				kfree(rbuf);
+			else
+				n_hdlc_buf_put(&n_hdlc->rx_free_buf_list, rbuf);
 			break;
+		}
 			
 		/* no data */
 		if (file->f_flags & O_NONBLOCK) {
-			tty_unlock();
-			return -EAGAIN;
+			ret = -EAGAIN;
+			break;
 		}
-			
-		interruptible_sleep_on (&tty->read_wait);
+
+		schedule();
+
 		if (signal_pending(current)) {
-			tty_unlock();
-			return -EINTR;
+			ret = -EINTR;
+			break;
 		}
 	}
-		
-	if (rbuf->count > nr)
-		/* frame too large for caller's buffer (discard frame) */
-		ret = -EOVERFLOW;
-	else {
-		/* Copy the data to the caller's buffer */
-		if (copy_to_user(buf, rbuf->buf, rbuf->count))
-			ret = -EFAULT;
-		else
-			ret = rbuf->count;
-	}
-	
-	/* return HDLC buffer to free list unless the free list */
-	/* count has exceeded the default value, in which case the */
-	/* buffer is freed back to the OS to conserve memory */
-	if (n_hdlc->rx_free_buf_list.count > DEFAULT_RX_BUF_COUNT)
-		kfree(rbuf);
-	else	
-		n_hdlc_buf_put(&n_hdlc->rx_free_buf_list,rbuf);
-	tty_unlock();
+
+	remove_wait_queue(&tty->read_wait, &wait);
+	set_current_state(TASK_RUNNING);
+
 	return ret;
 	
 }	/* end of n_hdlc_tty_read() */
@@ -691,14 +690,15 @@ static ssize_t n_hdlc_tty_write(struct t
 		count = maxframe;
 	}
 	
-	tty_lock();
-
 	add_wait_queue(&tty->write_wait, &wait);
-	set_current_state(TASK_INTERRUPTIBLE);
+
+	for (;;) {
+		set_current_state(TASK_INTERRUPTIBLE);
 	
-	/* Allocate transmit buffer */
-	/* sleep until transmit buffer available */		
-	while (!(tbuf = n_hdlc_buf_get(&n_hdlc->tx_free_buf_list))) {
+		tbuf = n_hdlc_buf_get(&n_hdlc->tx_free_buf_list);
+		if (tbuf)
+			break;
+
 		if (file->f_flags & O_NONBLOCK) {
 			error = -EAGAIN;
 			break;
@@ -731,7 +731,7 @@ static ssize_t n_hdlc_tty_write(struct t
 		n_hdlc_buf_put(&n_hdlc->tx_buf_list,tbuf);
 		n_hdlc_send_frames(n_hdlc,tty);
 	}
-	tty_unlock();
+
 	return error;
 	
 }	/* end of n_hdlc_tty_write() */

Re: [PATCH] n_hdlc fix read and write locking

[Andrew Morton]

On Mon, 25 Oct 2010 13:22:39 -0500
Paul Fulghum <paulkf@microgate.com> wrote:

> Fix locking in read and write code of n_hdlc line discipline.

Missing a couple of cc's here...

> 2.6.36 replaced lock_kernel() with tty_lock().
> The tty mutex is not dropped automatically when the thread
> sleeps like the BKL. This results in a blocked read or write holding
> the tty mutex and stalling operations by other devices that use
> the tty mutex.
> 
> A review of n_hdlc read and write code shows:
> 1. neither BKL or tty mutex are required for correct operation
> 2. read can block while read data is available if data is posted
>    between availability check and call to interruptible_sleep_on()
> 3. write does not set process state to TASK_INTERRUPTIBLE
>    on each pass through the processing loop which can cause
>    unneeded scheduling of the thread
> 
> The unnecessary tty mutex references have been removed.
> 
> Read changed to use same code as n_tty read
> for completing reads and blocking.
> 
> Write corrected to set process state to
> TASK_INTERRUPTIBLE on each pass through processing loop.
> 

And a signed-off-by:, please.

> --- a/drivers/char/n_hdlc.c	2010-10-22 14:22:22.000000000 -0500
> +++ b/drivers/char/n_hdlc.c	2010-10-25 12:36:12.000000000 -0500
> @@ -581,8 +581,9 @@ static ssize_t n_hdlc_tty_read(struct tt
>  			   __u8 __user *buf, size_t nr)
>  {
>  	struct n_hdlc *n_hdlc = tty2n_hdlc(tty);
> -	int ret;
> +	int ret = 0;
>  	struct n_hdlc_buf *rbuf;
> +	DECLARE_WAITQUEUE(wait, current);
>  
>  	if (debuglevel >= DEBUG_LEVEL_INFO)	
>  		printk("%s(%d)n_hdlc_tty_read() called\n",__FILE__,__LINE__);
> @@ -598,57 +599,55 @@ static ssize_t n_hdlc_tty_read(struct tt
>  		return -EFAULT;
>  	}
>  
> -	tty_lock();
> +	add_wait_queue(&tty->read_wait, &wait);
>  
>  	for (;;) {
>  		if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) {
> -			tty_unlock();
> -			return -EIO;
> +			ret = -EIO;
> +			break;
>  		}
> +		if (tty_hung_up_p(file))
> +			break;
>  
> -		n_hdlc = tty2n_hdlc (tty);
> -		if (!n_hdlc || n_hdlc->magic != HDLC_MAGIC ||
> -			 tty != n_hdlc->tty) {
> -			tty_unlock();
> -			return 0;
> -		}
> +		set_current_state(TASK_INTERRUPTIBLE);
>  
>  		rbuf = n_hdlc_buf_get(&n_hdlc->rx_buf_list);
> -		if (rbuf)
> +		if (rbuf) {
> +			if (rbuf->count > nr) {
> +				/* too large for caller's buffer */
> +				ret = -EOVERFLOW;
> +			} else {
> +				if (copy_to_user(buf, rbuf->buf, rbuf->count))

It's not a bug afaict, but beware that a copy_to_user() will
unconditionally flip this task back into TASK_RUNNING state if it takes
a pagefault.  This means that the below schedule() will fall straight
through.  It looks like the code will handle this correctly?  If so,
it's just a little suboptimal.

> +					ret = -EFAULT;
> +				else
> +					ret = rbuf->count;
> +			}
> +
> +			if (n_hdlc->rx_free_buf_list.count >
> +			    DEFAULT_RX_BUF_COUNT)
> +				kfree(rbuf);
> +			else
> +				n_hdlc_buf_put(&n_hdlc->rx_free_buf_list, rbuf);
>  			break;
> +		}
>  			
>  		/* no data */
>  		if (file->f_flags & O_NONBLOCK) {
> -			tty_unlock();
> -			return -EAGAIN;
> +			ret = -EAGAIN;
> +			break;
>  		}
> -			
> -		interruptible_sleep_on (&tty->read_wait);
> +
> +		schedule();
> +
>  		if (signal_pending(current)) {
> -			tty_unlock();
> -			return -EINTR;
> +			ret = -EINTR;
> +			break;
>  		}
>  	}
> -		
> -	if (rbuf->count > nr)
> -		/* frame too large for caller's buffer (discard frame) */
> -		ret = -EOVERFLOW;
> -	else {
> -		/* Copy the data to the caller's buffer */
> -		if (copy_to_user(buf, rbuf->buf, rbuf->count))
> -			ret = -EFAULT;
> -		else
> -			ret = rbuf->count;
> -	}
> -	
> -	/* return HDLC buffer to free list unless the free list */
> -	/* count has exceeded the default value, in which case the */
> -	/* buffer is freed back to the OS to conserve memory */
> -	if (n_hdlc->rx_free_buf_list.count > DEFAULT_RX_BUF_COUNT)
> -		kfree(rbuf);
> -	else	
> -		n_hdlc_buf_put(&n_hdlc->rx_free_buf_list,rbuf);
> -	tty_unlock();
> +
> +	remove_wait_queue(&tty->read_wait, &wait);
> +	set_current_state(TASK_RUNNING);

We normally use __set_current_state() here - it saves a few cycles.

>  	return ret;
>  	
>  }	/* end of n_hdlc_tty_read() */
>
> ...
>

[PATCH] n_hdlc fix read and write locking

[Paul Fulghum]

Fix locking in read and write code of n_hdlc line discipline.

2.6.36 replaced lock_kernel() with tty_lock().
The tty mutex is not dropped automatically when the thread
sleeps like the BKL. This results in a blocked read or write holding
the tty mutex and stalling operations by other devices that use
the tty mutex.

A review of n_hdlc read and write code shows:
1. neither BKL or tty mutex are required for correct operation
2. read can block while read data is available if data is posted
   between availability check and call to interruptible_sleep_on()
3. write does not set process state to TASK_INTERRUPTIBLE
   on each pass through the processing loop which can cause
   unneeded scheduling of the thread

The unnecessary tty mutex references have been removed.

Read changed to use same code as n_tty read
for completing reads and blocking.

Write corrected to set process state to
TASK_INTERRUPTIBLE on each pass through processing loop.

Signed-off-by: Paul Fulghum <paulkf@microgate.com>

--- a/drivers/char/n_hdlc.c	2010-10-22 14:22:22.000000000 -0500
+++ b/drivers/char/n_hdlc.c	2010-10-25 15:18:22.000000000 -0500
@@ -581,8 +581,9 @@ static ssize_t n_hdlc_tty_read(struct tt
 			   __u8 __user *buf, size_t nr)
 {
 	struct n_hdlc *n_hdlc = tty2n_hdlc(tty);
-	int ret;
+	int ret = 0;
 	struct n_hdlc_buf *rbuf;
+	DECLARE_WAITQUEUE(wait, current);
 
 	if (debuglevel >= DEBUG_LEVEL_INFO)	
 		printk("%s(%d)n_hdlc_tty_read() called\n",__FILE__,__LINE__);
@@ -598,57 +599,55 @@ static ssize_t n_hdlc_tty_read(struct tt
 		return -EFAULT;
 	}
 
-	tty_lock();
+	add_wait_queue(&tty->read_wait, &wait);
 
 	for (;;) {
 		if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) {
-			tty_unlock();
-			return -EIO;
+			ret = -EIO;
+			break;
 		}
+		if (tty_hung_up_p(file))
+			break;
 
-		n_hdlc = tty2n_hdlc (tty);
-		if (!n_hdlc || n_hdlc->magic != HDLC_MAGIC ||
-			 tty != n_hdlc->tty) {
-			tty_unlock();
-			return 0;
-		}
+		set_current_state(TASK_INTERRUPTIBLE);
 
 		rbuf = n_hdlc_buf_get(&n_hdlc->rx_buf_list);
-		if (rbuf)
+		if (rbuf) {
+			if (rbuf->count > nr) {
+				/* too large for caller's buffer */
+				ret = -EOVERFLOW;
+			} else {
+				if (copy_to_user(buf, rbuf->buf, rbuf->count))
+					ret = -EFAULT;
+				else
+					ret = rbuf->count;
+			}
+
+			if (n_hdlc->rx_free_buf_list.count >
+			    DEFAULT_RX_BUF_COUNT)
+				kfree(rbuf);
+			else
+				n_hdlc_buf_put(&n_hdlc->rx_free_buf_list, rbuf);
 			break;
+		}
 			
 		/* no data */
 		if (file->f_flags & O_NONBLOCK) {
-			tty_unlock();
-			return -EAGAIN;
+			ret = -EAGAIN;
+			break;
 		}
-			
-		interruptible_sleep_on (&tty->read_wait);
+
+		schedule();
+
 		if (signal_pending(current)) {
-			tty_unlock();
-			return -EINTR;
+			ret = -EINTR;
+			break;
 		}
 	}
-		
-	if (rbuf->count > nr)
-		/* frame too large for caller's buffer (discard frame) */
-		ret = -EOVERFLOW;
-	else {
-		/* Copy the data to the caller's buffer */
-		if (copy_to_user(buf, rbuf->buf, rbuf->count))
-			ret = -EFAULT;
-		else
-			ret = rbuf->count;
-	}
-	
-	/* return HDLC buffer to free list unless the free list */
-	/* count has exceeded the default value, in which case the */
-	/* buffer is freed back to the OS to conserve memory */
-	if (n_hdlc->rx_free_buf_list.count > DEFAULT_RX_BUF_COUNT)
-		kfree(rbuf);
-	else	
-		n_hdlc_buf_put(&n_hdlc->rx_free_buf_list,rbuf);
-	tty_unlock();
+
+	remove_wait_queue(&tty->read_wait, &wait);
+	__set_current_state(TASK_RUNNING);
+
 	return ret;
 	
 }	/* end of n_hdlc_tty_read() */
@@ -691,14 +690,15 @@ static ssize_t n_hdlc_tty_write(struct t
 		count = maxframe;
 	}
 	
-	tty_lock();
-
 	add_wait_queue(&tty->write_wait, &wait);
-	set_current_state(TASK_INTERRUPTIBLE);
+
+	for (;;) {
+		set_current_state(TASK_INTERRUPTIBLE);
 	
-	/* Allocate transmit buffer */
-	/* sleep until transmit buffer available */		
-	while (!(tbuf = n_hdlc_buf_get(&n_hdlc->tx_free_buf_list))) {
+		tbuf = n_hdlc_buf_get(&n_hdlc->tx_free_buf_list);
+		if (tbuf)
+			break;
+
 		if (file->f_flags & O_NONBLOCK) {
 			error = -EAGAIN;
 			break;
@@ -719,7 +719,7 @@ static ssize_t n_hdlc_tty_write(struct t
 		}
 	}
 
-	set_current_state(TASK_RUNNING);
+	__set_current_state(TASK_RUNNING);
 	remove_wait_queue(&tty->write_wait, &wait);
 
 	if (!error) {		
@@ -731,7 +731,7 @@ static ssize_t n_hdlc_tty_write(struct t
 		n_hdlc_buf_put(&n_hdlc->tx_buf_list,tbuf);
 		n_hdlc_send_frames(n_hdlc,tty);
 	}
-	tty_unlock();
+
 	return error;
 	
 }	/* end of n_hdlc_tty_write() */

Re: [PATCH] n_hdlc fix read and write locking

[Arnd Bergmann]

On Monday 25 October 2010 20:22:39 Paul Fulghum wrote:
> Fix locking in read and write code of n_hdlc line discipline.
> 
> 2.6.36 replaced lock_kernel() with tty_lock().
> The tty mutex is not dropped automatically when the thread
> sleeps like the BKL. This results in a blocked read or write holding
> the tty mutex and stalling operations by other devices that use
> the tty mutex.
> 
> A review of n_hdlc read and write code shows:
> 1. neither BKL or tty mutex are required for correct operation
> 2. read can block while read data is available if data is posted
>    between availability check and call to interruptible_sleep_on()
> 3. write does not set process state to TASK_INTERRUPTIBLE
>    on each pass through the processing loop which can cause
>    unneeded scheduling of the thread

Right. I must have missed this when I was not checking for
interruptible_sleep_on(). I did systematically check for
this problem with the wait_event family as well as
work_queues, mutexes, semaphores and hand-written schedule
loops, but for some reason I did not check for sleep_on :(

I've double-checked it now, and it seems that all other
instances of sleep_on are waiting for close_wait in
block_til_ready or open functions, and I remember that
I did check those and convinced myself that they are fine.

> Write corrected to set process state to
> TASK_INTERRUPTIBLE on each pass through processing loop.

Would it be possible to express the same using
wait_event_interruptible()?

	Arnd

Re: [PATCH] n_hdlc fix read and write locking

[Paul Fulghum]

On 10/25/2010 2:05 PM, Andrew Morton wrote:
> And a signed-off-by:, please.

OK

> It's not a bug afaict, but beware that a copy_to_user() will
> unconditionally flip this task back into TASK_RUNNING state if it takes
> a pagefault.  This means that the below schedule() will fall straight
> through.  It looks like the code will handle this correctly?  If so,
> it's just a little suboptimal.

copy_to_user is only called when data is available to
complete the read and break terminates the loop without
calling schedule().

>> +	set_current_state(TASK_RUNNING);
> 
> We normally use __set_current_state() here - it saves a few cycles.

I'll change it to __set_current_state() and resubmit.

-- 
Paul Fulghum
MicroGate Systems, Ltd.
=Customer Driven, by Design=
(800)444-1982
(512)345-7791 (Direct)
(512)343-9046 (Fax)
Central Time Zone (GMT -6h)
www.microgate.com

Re: [PATCH] n_hdlc fix read and write locking

[Paul Fulghum]

On 10/25/2010 3:31 PM, Arnd Bergmann wrote:

> Would it be possible to express the same using
> wait_event_interruptible()?

Yes if I embed an assignment in the event expression:

rc = wait_event_interruptible(&wait, (buf = get_buf()));
if (!rc)
	process_buf(buf);

Is that considered acceptable?

-- 
Paul Fulghum
MicroGate Systems, Ltd.
=Customer Driven, by Design=
(800)444-1982
(512)345-7791 (Direct)
(512)343-9046 (Fax)
Central Time Zone (GMT -5h)
www.microgate.com

Re: [PATCH] n_hdlc fix read and write locking

[Arnd Bergmann]

On Tuesday 26 October 2010, Paul Fulghum wrote:
> On 10/25/2010 3:31 PM, Arnd Bergmann wrote:
> 
> > Would it be possible to express the same using
> > wait_event_interruptible()?
> 
> Yes if I embed an assignment in the event expression:
> 
> rc = wait_event_interruptible(&wait, (buf = get_buf()));
> if (!rc)
>         process_buf(buf);
> 
> Is that considered acceptable?

I'd do it if it makes the code more readable. We need the patch
for the stable release, so it really should be obvious not to
introduce new bugs. I could not fully understand the version
you posted, so if you can make it easier to understand by using
wait_event, please do so. If you think that putting the
assignment into the macro argument outweighs the advantage from
the wait_event, just keep your version.

	Arnd