Skip to content

Commit

Permalink
tty: n_hdlc: get rid of racy n_hdlc.tbuf
Browse files Browse the repository at this point in the history 
Currently N_HDLC line discipline uses a self-made singly linked list for
data buffers and has n_hdlc.tbuf pointer for buffer retransmitting after
an error.

The commit be10eb7
("tty: n_hdlc add buffer flushing") introduced racy access to n_hdlc.tbuf.
After tx error concurrent flush_tx_queue() and n_hdlc_send_frames() can put
one data buffer to tx_free_buf_list twice. That causes double free in
n_hdlc_release().

Let's use standard kernel linked list and get rid of n_hdlc.tbuf:
in case of tx error put current data buffer after the head of tx_buf_list.

Signed-off-by: Alexander Popov <alex.popov@linux.com>
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
  • Loading branch information
Alexander Popov authored and Greg Kroah-Hartman committed Mar 7, 2017
1 parent c1ae3cf commit 82f2341
Showing 1 changed file with 69 additions and 63 deletions.
132 changes: 69 additions & 63 deletions drivers/tty/n_hdlc.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,16 +114,15 @@
#define DEFAULT_TX_BUF_COUNT 3

struct n_hdlc_buf {
struct n_hdlc_buf *link;
struct list_head list_item;
int count;
char buf[1];
};

#define N_HDLC_BUF_SIZE (sizeof(struct n_hdlc_buf) + maxframe)

struct n_hdlc_buf_list {
struct n_hdlc_buf *head;
struct n_hdlc_buf *tail;
struct list_head list;
int count;
spinlock_t spinlock;
};
Expand All @@ -136,7 +135,6 @@ struct n_hdlc_buf_list {
* @backup_tty - TTY to use if tty gets closed
* @tbusy - reentrancy flag for tx wakeup code
* @woke_up - FIXME: describe this field
* @tbuf - currently transmitting tx buffer
* @tx_buf_list - list of pending transmit frame buffers
* @rx_buf_list - list of received frame buffers
* @tx_free_buf_list - list unused transmit frame buffers
Expand All @@ -149,7 +147,6 @@ struct n_hdlc {
struct tty_struct *backup_tty;
int tbusy;
int woke_up;
struct n_hdlc_buf *tbuf;
struct n_hdlc_buf_list tx_buf_list;
struct n_hdlc_buf_list rx_buf_list;
struct n_hdlc_buf_list tx_free_buf_list;
Expand All @@ -159,6 +156,8 @@ struct n_hdlc {
/*
* HDLC buffer list manipulation functions
*/
static void n_hdlc_buf_return(struct n_hdlc_buf_list *buf_list,
struct n_hdlc_buf *buf);
static void n_hdlc_buf_put(struct n_hdlc_buf_list *list,
struct n_hdlc_buf *buf);
static struct n_hdlc_buf *n_hdlc_buf_get(struct n_hdlc_buf_list *list);
Expand Down Expand Up @@ -208,16 +207,9 @@ static void flush_tx_queue(struct tty_struct *tty)
{
struct n_hdlc *n_hdlc = tty2n_hdlc(tty);
struct n_hdlc_buf *buf;
unsigned long flags;

while ((buf = n_hdlc_buf_get(&n_hdlc->tx_buf_list)))
n_hdlc_buf_put(&n_hdlc->tx_free_buf_list, buf);
spin_lock_irqsave(&n_hdlc->tx_buf_list.spinlock, flags);
if (n_hdlc->tbuf) {
n_hdlc_buf_put(&n_hdlc->tx_free_buf_list, n_hdlc->tbuf);
n_hdlc->tbuf = NULL;
}
spin_unlock_irqrestore(&n_hdlc->tx_buf_list.spinlock, flags);
}

static struct tty_ldisc_ops n_hdlc_ldisc = {
Expand Down Expand Up @@ -283,7 +275,6 @@ static void n_hdlc_release(struct n_hdlc *n_hdlc)
} else
break;
}
kfree(n_hdlc->tbuf);
kfree(n_hdlc);

} /* end of n_hdlc_release() */
Expand Down Expand Up @@ -402,13 +393,7 @@ static void n_hdlc_send_frames(struct n_hdlc *n_hdlc, struct tty_struct *tty)
n_hdlc->woke_up = 0;
spin_unlock_irqrestore(&n_hdlc->tx_buf_list.spinlock, flags);

/* get current transmit buffer or get new transmit */
/* buffer from list of pending transmit buffers */

tbuf = n_hdlc->tbuf;
if (!tbuf)
tbuf = n_hdlc_buf_get(&n_hdlc->tx_buf_list);

tbuf = n_hdlc_buf_get(&n_hdlc->tx_buf_list);
while (tbuf) {
if (debuglevel >= DEBUG_LEVEL_INFO)
printk("%s(%d)sending frame %p, count=%d\n",
Expand All @@ -420,7 +405,7 @@ static void n_hdlc_send_frames(struct n_hdlc *n_hdlc, struct tty_struct *tty)

/* rollback was possible and has been done */
if (actual == -ERESTARTSYS) {
n_hdlc->tbuf = tbuf;
n_hdlc_buf_return(&n_hdlc->tx_buf_list, tbuf);
break;
}
/* if transmit error, throw frame away by */
Expand All @@ -435,10 +420,7 @@ static void n_hdlc_send_frames(struct n_hdlc *n_hdlc, struct tty_struct *tty)

/* free current transmit buffer */
n_hdlc_buf_put(&n_hdlc->tx_free_buf_list, tbuf);

/* this tx buffer is done */
n_hdlc->tbuf = NULL;


/* wait up sleeping writers */
wake_up_interruptible(&tty->write_wait);

Expand All @@ -448,10 +430,12 @@ static void n_hdlc_send_frames(struct n_hdlc *n_hdlc, struct tty_struct *tty)
if (debuglevel >= DEBUG_LEVEL_INFO)
printk("%s(%d)frame %p pending\n",
__FILE__,__LINE__,tbuf);

/* buffer not accepted by driver */
/* set this buffer as pending buffer */
n_hdlc->tbuf = tbuf;

/*
* the buffer was not accepted by driver,
* return it back into tx queue
*/
n_hdlc_buf_return(&n_hdlc->tx_buf_list, tbuf);
break;
}
}
Expand Down Expand Up @@ -749,7 +733,8 @@ static int n_hdlc_tty_ioctl(struct tty_struct *tty, struct file *file,
int error = 0;
int count;
unsigned long flags;

struct n_hdlc_buf *buf = NULL;

if (debuglevel >= DEBUG_LEVEL_INFO)
printk("%s(%d)n_hdlc_tty_ioctl() called %d\n",
__FILE__,__LINE__,cmd);
Expand All @@ -763,8 +748,10 @@ static int n_hdlc_tty_ioctl(struct tty_struct *tty, struct file *file,
/* report count of read data available */
/* in next available frame (if any) */
spin_lock_irqsave(&n_hdlc->rx_buf_list.spinlock,flags);
if (n_hdlc->rx_buf_list.head)
count = n_hdlc->rx_buf_list.head->count;
buf = list_first_entry_or_null(&n_hdlc->rx_buf_list.list,
struct n_hdlc_buf, list_item);
if (buf)
count = buf->count;
else
count = 0;
spin_unlock_irqrestore(&n_hdlc->rx_buf_list.spinlock,flags);
Expand All @@ -776,8 +763,10 @@ static int n_hdlc_tty_ioctl(struct tty_struct *tty, struct file *file,
count = tty_chars_in_buffer(tty);
/* add size of next output frame in queue */
spin_lock_irqsave(&n_hdlc->tx_buf_list.spinlock,flags);
if (n_hdlc->tx_buf_list.head)
count += n_hdlc->tx_buf_list.head->count;
buf = list_first_entry_or_null(&n_hdlc->tx_buf_list.list,
struct n_hdlc_buf, list_item);
if (buf)
count += buf->count;
spin_unlock_irqrestore(&n_hdlc->tx_buf_list.spinlock,flags);
error = put_user(count, (int __user *)arg);
break;
Expand Down Expand Up @@ -825,14 +814,14 @@ static unsigned int n_hdlc_tty_poll(struct tty_struct *tty, struct file *filp,
poll_wait(filp, &tty->write_wait, wait);

/* set bits for operations that won't block */
if (n_hdlc->rx_buf_list.head)
if (!list_empty(&n_hdlc->rx_buf_list.list))
mask |= POLLIN | POLLRDNORM; /* readable */
if (test_bit(TTY_OTHER_CLOSED, &tty->flags))
mask |= POLLHUP;
if (tty_hung_up_p(filp))
mask |= POLLHUP;
if (!tty_is_writelocked(tty) &&
n_hdlc->tx_free_buf_list.head)
!list_empty(&n_hdlc->tx_free_buf_list.list))
mask |= POLLOUT | POLLWRNORM; /* writable */
}
return mask;
Expand All @@ -856,7 +845,12 @@ static struct n_hdlc *n_hdlc_alloc(void)
spin_lock_init(&n_hdlc->tx_free_buf_list.spinlock);
spin_lock_init(&n_hdlc->rx_buf_list.spinlock);
spin_lock_init(&n_hdlc->tx_buf_list.spinlock);


INIT_LIST_HEAD(&n_hdlc->rx_free_buf_list.list);
INIT_LIST_HEAD(&n_hdlc->tx_free_buf_list.list);
INIT_LIST_HEAD(&n_hdlc->rx_buf_list.list);
INIT_LIST_HEAD(&n_hdlc->tx_buf_list.list);

/* allocate free rx buffer list */
for(i=0;i<DEFAULT_RX_BUF_COUNT;i++) {
buf = kmalloc(N_HDLC_BUF_SIZE, GFP_KERNEL);
Expand All @@ -883,54 +877,66 @@ static struct n_hdlc *n_hdlc_alloc(void)

} /* end of n_hdlc_alloc() */

/**
* n_hdlc_buf_return - put the HDLC buffer after the head of the specified list
* @buf_list - pointer to the buffer list
* @buf - pointer to the buffer
*/
static void n_hdlc_buf_return(struct n_hdlc_buf_list *buf_list,
struct n_hdlc_buf *buf)
{
unsigned long flags;

spin_lock_irqsave(&buf_list->spinlock, flags);

list_add(&buf->list_item, &buf_list->list);
buf_list->count++;

spin_unlock_irqrestore(&buf_list->spinlock, flags);
}

/**
* n_hdlc_buf_put - add specified HDLC buffer to tail of specified list
* @list - pointer to buffer list
* @buf_list - pointer to buffer list
* @buf - pointer to buffer
*/
static void n_hdlc_buf_put(struct n_hdlc_buf_list *list,
static void n_hdlc_buf_put(struct n_hdlc_buf_list *buf_list,
struct n_hdlc_buf *buf)
{
unsigned long flags;
spin_lock_irqsave(&list->spinlock,flags);

buf->link=NULL;
if (list->tail)
list->tail->link = buf;
else
list->head = buf;
list->tail = buf;
(list->count)++;

spin_unlock_irqrestore(&list->spinlock,flags);


spin_lock_irqsave(&buf_list->spinlock, flags);

list_add_tail(&buf->list_item, &buf_list->list);
buf_list->count++;

spin_unlock_irqrestore(&buf_list->spinlock, flags);
} /* end of n_hdlc_buf_put() */

/**
* n_hdlc_buf_get - remove and return an HDLC buffer from list
* @list - pointer to HDLC buffer list
* @buf_list - pointer to HDLC buffer list
*
* Remove and return an HDLC buffer from the head of the specified HDLC buffer
* list.
* Returns a pointer to HDLC buffer if available, otherwise %NULL.
*/
static struct n_hdlc_buf* n_hdlc_buf_get(struct n_hdlc_buf_list *list)
static struct n_hdlc_buf *n_hdlc_buf_get(struct n_hdlc_buf_list *buf_list)
{
unsigned long flags;
struct n_hdlc_buf *buf;
spin_lock_irqsave(&list->spinlock,flags);

buf = list->head;

spin_lock_irqsave(&buf_list->spinlock, flags);

buf = list_first_entry_or_null(&buf_list->list,
struct n_hdlc_buf, list_item);
if (buf) {
list->head = buf->link;
(list->count)--;
list_del(&buf->list_item);
buf_list->count--;
}
if (!list->head)
list->tail = NULL;

spin_unlock_irqrestore(&list->spinlock,flags);

spin_unlock_irqrestore(&buf_list->spinlock, flags);
return buf;

} /* end of n_hdlc_buf_get() */

static char hdlc_banner[] __initdata =
Expand Down

0 comments on commit 82f2341

Please sign in to comment.