Re: [PATCH] n_hdlc fix read and write locking

[Andrew Morton <akpm@linux-foundation.org>]

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() */
>
> ...
>