Re: [PATCH] tty: Correct tty buffer flush.
On Mon, 2013-03-04 at 23:19 +0400, Ilya Zykov wrote: > On 03.12.2012 13:54, Ilya Zykov wrote: > > The root of problem is carelessly zeroing pointer(in function > > __tty_buffer_flush()), > > when another thread can use it. It can be cause of "NULL pointer > > dereference". > > Main idea of the patch, this is never release last (struct tty_buffer) in > > the active buffer. > > Only flush data for ldisc(tty->buf.head->read = tty->buf.head->commit). > > At that moment driver can collect(write) data in buffer without conflict. > > It is repeat behavior of flush_to_ldisc(), only without feeding data to > > ldisc. > > Test program and bug report you can see: > > https://lkml.org/lkml/2012/11/29/368 > > > > Cc: sta...@vger.kernel.org > > Signed-off-by: Ilya Zykov > > --- > > diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c > > index 6c9b7cd..4f02f9c 100644 > > --- a/drivers/tty/tty_buffer.c > > +++ b/drivers/tty/tty_buffer.c > > @@ -114,11 +114,14 @@ static void __tty_buffer_flush(struct tty_struct *tty) > > { > > struct tty_buffer *thead; > > > > - while ((thead = tty->buf.head) != NULL) { > > - tty->buf.head = thead->next; > > - tty_buffer_free(tty, thead); > > + if (tty->buf.head == NULL) > > + return; > > + while ((thead = tty->buf.head->next) != NULL) { > > + tty_buffer_free(tty, tty->buf.head); > > + tty->buf.head = thead; > > } > > - tty->buf.tail = NULL; > > + WARN_ON(tty->buf.head != tty->buf.tail); > > + tty->buf.head->read = tty->buf.head->commit; > > } > > > > /** > > > > You can include this patch, in 3.2 series , for improve stability, > it would be merged in upstream 3.9-rc1. Added to the queue, thanks. Ben. -- Ben Hutchings Usenet is essentially a HUGE group of people passing notes in class. - Rachel Kadel, `A Quick Guide to Newsgroup Etiquette' signature.asc Description: This is a digitally signed message part
Re: [PATCH] tty: Correct tty buffer flush.
On Mon, 2013-03-04 at 23:19 +0400, Ilya Zykov wrote: On 03.12.2012 13:54, Ilya Zykov wrote: The root of problem is carelessly zeroing pointer(in function __tty_buffer_flush()), when another thread can use it. It can be cause of NULL pointer dereference. Main idea of the patch, this is never release last (struct tty_buffer) in the active buffer. Only flush data for ldisc(tty-buf.head-read = tty-buf.head-commit). At that moment driver can collect(write) data in buffer without conflict. It is repeat behavior of flush_to_ldisc(), only without feeding data to ldisc. Test program and bug report you can see: https://lkml.org/lkml/2012/11/29/368 Cc: sta...@vger.kernel.org Signed-off-by: Ilya Zykov i...@ilyx.ru --- diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c index 6c9b7cd..4f02f9c 100644 --- a/drivers/tty/tty_buffer.c +++ b/drivers/tty/tty_buffer.c @@ -114,11 +114,14 @@ static void __tty_buffer_flush(struct tty_struct *tty) { struct tty_buffer *thead; - while ((thead = tty-buf.head) != NULL) { - tty-buf.head = thead-next; - tty_buffer_free(tty, thead); + if (tty-buf.head == NULL) + return; + while ((thead = tty-buf.head-next) != NULL) { + tty_buffer_free(tty, tty-buf.head); + tty-buf.head = thead; } - tty-buf.tail = NULL; + WARN_ON(tty-buf.head != tty-buf.tail); + tty-buf.head-read = tty-buf.head-commit; } /** You can include this patch, in 3.2 series , for improve stability, it would be merged in upstream 3.9-rc1. Added to the queue, thanks. Ben. -- Ben Hutchings Usenet is essentially a HUGE group of people passing notes in class. - Rachel Kadel, `A Quick Guide to Newsgroup Etiquette' signature.asc Description: This is a digitally signed message part
Re: [PATCH] tty: Correct tty buffer flush.
On 03.12.2012 13:54, Ilya Zykov wrote: > The root of problem is carelessly zeroing pointer(in function > __tty_buffer_flush()), > when another thread can use it. It can be cause of "NULL pointer dereference". > Main idea of the patch, this is never release last (struct tty_buffer) in > the active buffer. > Only flush data for ldisc(tty->buf.head->read = tty->buf.head->commit). > At that moment driver can collect(write) data in buffer without conflict. > It is repeat behavior of flush_to_ldisc(), only without feeding data to ldisc. > Test program and bug report you can see: > https://lkml.org/lkml/2012/11/29/368 > > Cc: sta...@vger.kernel.org > Signed-off-by: Ilya Zykov > --- > diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c > index 6c9b7cd..4f02f9c 100644 > --- a/drivers/tty/tty_buffer.c > +++ b/drivers/tty/tty_buffer.c > @@ -114,11 +114,14 @@ static void __tty_buffer_flush(struct tty_struct *tty) > { > struct tty_buffer *thead; > > - while ((thead = tty->buf.head) != NULL) { > - tty->buf.head = thead->next; > - tty_buffer_free(tty, thead); > + if (tty->buf.head == NULL) > + return; > + while ((thead = tty->buf.head->next) != NULL) { > + tty_buffer_free(tty, tty->buf.head); > + tty->buf.head = thead; > } > - tty->buf.tail = NULL; > + WARN_ON(tty->buf.head != tty->buf.tail); > + tty->buf.head->read = tty->buf.head->commit; > } > > /** > You can include this patch, in 3.2 series , for improve stability, it would be merged in upstream 3.9-rc1. Thank you. Ilya. https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/tty/tty_buffer.c?id=64325a3be08d364a62ee8f84b2cf86934bc2544a -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tty: Correct tty buffer flush.
On 03.12.2012 13:54, Ilya Zykov wrote: The root of problem is carelessly zeroing pointer(in function __tty_buffer_flush()), when another thread can use it. It can be cause of NULL pointer dereference. Main idea of the patch, this is never release last (struct tty_buffer) in the active buffer. Only flush data for ldisc(tty-buf.head-read = tty-buf.head-commit). At that moment driver can collect(write) data in buffer without conflict. It is repeat behavior of flush_to_ldisc(), only without feeding data to ldisc. Test program and bug report you can see: https://lkml.org/lkml/2012/11/29/368 Cc: sta...@vger.kernel.org Signed-off-by: Ilya Zykov i...@ilyx.ru --- diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c index 6c9b7cd..4f02f9c 100644 --- a/drivers/tty/tty_buffer.c +++ b/drivers/tty/tty_buffer.c @@ -114,11 +114,14 @@ static void __tty_buffer_flush(struct tty_struct *tty) { struct tty_buffer *thead; - while ((thead = tty-buf.head) != NULL) { - tty-buf.head = thead-next; - tty_buffer_free(tty, thead); + if (tty-buf.head == NULL) + return; + while ((thead = tty-buf.head-next) != NULL) { + tty_buffer_free(tty, tty-buf.head); + tty-buf.head = thead; } - tty-buf.tail = NULL; + WARN_ON(tty-buf.head != tty-buf.tail); + tty-buf.head-read = tty-buf.head-commit; } /** You can include this patch, in 3.2 series , for improve stability, it would be merged in upstream 3.9-rc1. Thank you. Ilya. https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/tty/tty_buffer.c?id=64325a3be08d364a62ee8f84b2cf86934bc2544a -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tty: Correct tty buffer flush.
On Sat, Jan 19, 2013 at 4:12 PM, Sedat Dilek wrote: > On Sat, Jan 19, 2013 at 3:16 PM, Ilya Zykov wrote: >> The root of problem is carelessly zeroing pointer(in function >> __tty_buffer_flush()), >> when another thread can use it. It can be cause of "NULL pointer >> dereference". >> Main idea of the patch, this is never free last (struct tty_buffer) in the >> active buffer. >> Only flush the data for ldisc(buf->head->read = buf->head->commit). >> At that moment driver can collect(write) data in buffer without conflict. >> It is repeat behavior of flush_to_ldisc(), only without feeding data to >> ldisc. >> >> Also revert: >> commit c56a00a165712fd73081f40044b1e64407bb1875 >> tty: hold lock across tty buffer finding and buffer filling >> In order to delete the unneeded locks any more. >> >> Signed-off-by: Ilya Zykov >> > > I have tested your patch on top of Linux-Next (next-20130118) plus my > own patchset (see file attachments)! > > [ TESTCASE ] > > # grep CONFIG_PM_DEBUG /boot/config-$(uname -r) > CONFIG_PM_DEBUG=y > > # echo "freezer" > /sys/power/pm_test > > # cat /sys/power/pm_test > none core processors platform devices [freezer] > > # echo mem > /sys/power/state && sleep 1 <--- 1st S/R: OK > > # echo mem > /sys/power/state && sleep 1 <--- 2nd S/R: OK, but caused > a call-trace (see above)! > > [ /TESTCASE ] > > Unfortunately, I get now a different call-trace on the 2nd freezer/pm-test... > > +[ 368.891613] PM: Preparing system for mem sleep > +[ 373.886722] Freezing user space processes ... (elapsed 0.01 seconds) done. > +[ 373.902741] Freezing remaining freezable tasks ... > +[ 393.904587] Freezing of tasks failed after 20.01 seconds (1 tasks > refusing to freeze, wq_busy=0): > +[ 393.904714] jbd2/loop0-8D 0003 0 304 2 > 0x > +[ 393.904723] 880117525b68 0046 > 8801195d9400 > +[ 393.904731] 880117d74560 880117525fd8 880117525fd8 > 880117525fd8 > +[ 393.904738] 88004212c560 880117d74560 880117525b68 > 88011fad4738 > +[ 393.904745] Call Trace: > +[ 393.904764] [] ? __wait_on_buffer+0x30/0x30 > +[ 393.904772] [] schedule+0x29/0x70 > +[ 393.904778] [] io_schedule+0x8f/0xd0 > +[ 393.904785] [] sleep_on_buffer+0xe/0x20 > +[ 393.904794] [] __wait_on_bit+0x5f/0x90 > +[ 393.904801] [] ? __wait_on_buffer+0x30/0x30 > +[ 393.904809] [] out_of_line_wait_on_bit+0x7c/0x90 > +[ 393.904817] [] ? autoremove_wake_function+0x40/0x40 > +[ 393.904824] [] __wait_on_buffer+0x2e/0x30 > +[ 393.904836] [] > jbd2_journal_commit_transaction+0xdef/0x1960 > +[ 393.904844] [] ? _raw_spin_lock_irqsave+0x2e/0x40 > +[ 393.904853] [] ? try_to_del_timer_sync+0x4f/0x70 > +[ 393.904859] [] kjournald2+0xb8/0x240 > +[ 393.904865] [] ? add_wait_queue+0x60/0x60 > +[ 393.904871] [] ? commit_timeout+0x10/0x10 > +[ 393.904877] [] kthread+0xc0/0xd0 > +[ 393.904883] [] ? flush_kthread_worker+0xb0/0xb0 > +[ 393.904889] [] ret_from_fork+0x7c/0xb0 > +[ 393.904894] [] ? flush_kthread_worker+0xb0/0xb0 > +[ 393.904972] > +[ 393.904975] Restarting kernel threads ... done. > +[ 393.905163] Restarting tasks ... done. > +[ 393.914283] video LNXVIDEO:00: Restoring backlight state > > If this ring one or more bells to you let me know! > > Please, have a look at the file attachments! > Thanks. > > Hope this helps us to track the problem! > I turned on... CONFIG_EXT4_DEBUG=y CONFIG_JBD2_DEBUG=y ...to see more light in the dark. - Sedat - > - Sedat - > >> --- >> drivers/tty/tty_buffer.c | 92 >> +++--- >> 1 files changed, 22 insertions(+), 70 deletions(-) >> >> diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c >> index d6969f6..61ec4dd 100644 >> --- a/drivers/tty/tty_buffer.c >> +++ b/drivers/tty/tty_buffer.c >> @@ -119,11 +119,14 @@ static void __tty_buffer_flush(struct tty_port *port) >> struct tty_bufhead *buf = >buf; >> struct tty_buffer *thead; >> >> - while ((thead = buf->head) != NULL) { >> - buf->head = thead->next; >> - tty_buffer_free(port, thead); >> + if (unlikely(buf->head == NULL)) >> + return; >> + while ((thead = buf->head->next) != NULL) { >> + tty_buffer_free(port, buf->head); >> + buf->head = thead; >> } >> - buf->tail = NULL; >> + WARN_ON(buf->head != buf->tail); >> + buf->head->read = buf->head->commit; >> } >> >> /** >> @@ -194,19 +197,22 @@ static struct tty_buffer *tty_buffer_find(struct >> tty_port *port, size_t size) >>have queued and recycle that ? */ >> } >> /** >> - * __tty_buffer_request_room - grow tty buffer if >> needed >> + * tty_buffer_request_room - grow tty buffer if needed >> * @tty: tty structure >> * @size: size desired >> * >> * Make at least size bytes of linear space available
[PATCH] tty: Correct tty buffer flush.
The root of problem is carelessly zeroing pointer(in function __tty_buffer_flush()), when another thread can use it. It can be cause of "NULL pointer dereference". Main idea of the patch, this is never free last (struct tty_buffer) in the active buffer. Only flush the data for ldisc(buf->head->read = buf->head->commit). At that moment driver can collect(write) data in buffer without conflict. It is repeat behavior of flush_to_ldisc(), only without feeding data to ldisc. Also revert: commit c56a00a165712fd73081f40044b1e64407bb1875 tty: hold lock across tty buffer finding and buffer filling In order to delete the unneeded locks any more. Signed-off-by: Ilya Zykov --- drivers/tty/tty_buffer.c | 92 +++--- 1 files changed, 22 insertions(+), 70 deletions(-) diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c index d6969f6..61ec4dd 100644 --- a/drivers/tty/tty_buffer.c +++ b/drivers/tty/tty_buffer.c @@ -119,11 +119,14 @@ static void __tty_buffer_flush(struct tty_port *port) struct tty_bufhead *buf = >buf; struct tty_buffer *thead; - while ((thead = buf->head) != NULL) { - buf->head = thead->next; - tty_buffer_free(port, thead); + if (unlikely(buf->head == NULL)) + return; + while ((thead = buf->head->next) != NULL) { + tty_buffer_free(port, buf->head); + buf->head = thead; } - buf->tail = NULL; + WARN_ON(buf->head != buf->tail); + buf->head->read = buf->head->commit; } /** @@ -194,19 +197,22 @@ static struct tty_buffer *tty_buffer_find(struct tty_port *port, size_t size) have queued and recycle that ? */ } /** - * __tty_buffer_request_room - grow tty buffer if needed + * tty_buffer_request_room - grow tty buffer if needed * @tty: tty structure * @size: size desired * * Make at least size bytes of linear space available for the tty * buffer. If we fail return the size we managed to find. - * Locking: Caller must hold port->buf.lock + * + * Locking: Takes port->buf.lock */ -static int __tty_buffer_request_room(struct tty_port *port, size_t size) +int tty_buffer_request_room(struct tty_port *port, size_t size) { struct tty_bufhead *buf = >buf; struct tty_buffer *b, *n; int left; + unsigned long flags; + spin_lock_irqsave(>lock, flags); /* OPTIMISATION: We could keep a per tty "zero" sized buffer to remove this conditional if its worth it. This would be invisible to the callers */ @@ -228,31 +234,9 @@ static int __tty_buffer_request_room(struct tty_port *port, size_t size) } else size = left; } - + spin_unlock_irqrestore(>lock, flags); return size; } - - -/** - * tty_buffer_request_room - grow tty buffer if needed - * @port: tty port structure - * @size: size desired - * - * Make at least size bytes of linear space available for the tty - * buffer. If we fail return the size we managed to find. - * - * Locking: Takes port->buf.lock - */ -int tty_buffer_request_room(struct tty_port *port, size_t size) -{ - unsigned long flags; - int length; - - spin_lock_irqsave(>buf.lock, flags); - length = __tty_buffer_request_room(port, size); - spin_unlock_irqrestore(>buf.lock, flags); - return length; -} EXPORT_SYMBOL_GPL(tty_buffer_request_room); /** @@ -271,26 +255,18 @@ EXPORT_SYMBOL_GPL(tty_buffer_request_room); int tty_insert_flip_string_fixed_flag(struct tty_port *port, const unsigned char *chars, char flag, size_t size) { - struct tty_bufhead *buf = >buf; int copied = 0; do { int goal = min_t(size_t, size - copied, TTY_BUFFER_PAGE); - int space; - unsigned long flags; - struct tty_buffer *tb; - - spin_lock_irqsave(>lock, flags); - space = __tty_buffer_request_room(port, goal); - tb = buf->tail; + int space = tty_buffer_request_room(port, goal); + struct tty_buffer *tb = port->buf.tail; /* If there is no space then tb may be NULL */ if (unlikely(space == 0)) { - spin_unlock_irqrestore(>lock, flags); break; } memcpy(tb->char_buf_ptr + tb->used, chars, space); memset(tb->flag_buf_ptr + tb->used, flag, space); tb->used += space; - spin_unlock_irqrestore(>lock, flags); copied += space; chars += space; /* There is a small chance that we need to split the data over @@ -317,26 +293,18 @@ EXPORT_SYMBOL(tty_insert_flip_string_fixed_flag); int
[PATCH] tty: Correct tty buffer flush.
The root of problem is carelessly zeroing pointer(in function __tty_buffer_flush()), when another thread can use it. It can be cause of NULL pointer dereference. Main idea of the patch, this is never free last (struct tty_buffer) in the active buffer. Only flush the data for ldisc(buf-head-read = buf-head-commit). At that moment driver can collect(write) data in buffer without conflict. It is repeat behavior of flush_to_ldisc(), only without feeding data to ldisc. Also revert: commit c56a00a165712fd73081f40044b1e64407bb1875 tty: hold lock across tty buffer finding and buffer filling In order to delete the unneeded locks any more. Signed-off-by: Ilya Zykov i...@ilyx.ru --- drivers/tty/tty_buffer.c | 92 +++--- 1 files changed, 22 insertions(+), 70 deletions(-) diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c index d6969f6..61ec4dd 100644 --- a/drivers/tty/tty_buffer.c +++ b/drivers/tty/tty_buffer.c @@ -119,11 +119,14 @@ static void __tty_buffer_flush(struct tty_port *port) struct tty_bufhead *buf = port-buf; struct tty_buffer *thead; - while ((thead = buf-head) != NULL) { - buf-head = thead-next; - tty_buffer_free(port, thead); + if (unlikely(buf-head == NULL)) + return; + while ((thead = buf-head-next) != NULL) { + tty_buffer_free(port, buf-head); + buf-head = thead; } - buf-tail = NULL; + WARN_ON(buf-head != buf-tail); + buf-head-read = buf-head-commit; } /** @@ -194,19 +197,22 @@ static struct tty_buffer *tty_buffer_find(struct tty_port *port, size_t size) have queued and recycle that ? */ } /** - * __tty_buffer_request_room - grow tty buffer if needed + * tty_buffer_request_room - grow tty buffer if needed * @tty: tty structure * @size: size desired * * Make at least size bytes of linear space available for the tty * buffer. If we fail return the size we managed to find. - * Locking: Caller must hold port-buf.lock + * + * Locking: Takes port-buf.lock */ -static int __tty_buffer_request_room(struct tty_port *port, size_t size) +int tty_buffer_request_room(struct tty_port *port, size_t size) { struct tty_bufhead *buf = port-buf; struct tty_buffer *b, *n; int left; + unsigned long flags; + spin_lock_irqsave(buf-lock, flags); /* OPTIMISATION: We could keep a per tty zero sized buffer to remove this conditional if its worth it. This would be invisible to the callers */ @@ -228,31 +234,9 @@ static int __tty_buffer_request_room(struct tty_port *port, size_t size) } else size = left; } - + spin_unlock_irqrestore(buf-lock, flags); return size; } - - -/** - * tty_buffer_request_room - grow tty buffer if needed - * @port: tty port structure - * @size: size desired - * - * Make at least size bytes of linear space available for the tty - * buffer. If we fail return the size we managed to find. - * - * Locking: Takes port-buf.lock - */ -int tty_buffer_request_room(struct tty_port *port, size_t size) -{ - unsigned long flags; - int length; - - spin_lock_irqsave(port-buf.lock, flags); - length = __tty_buffer_request_room(port, size); - spin_unlock_irqrestore(port-buf.lock, flags); - return length; -} EXPORT_SYMBOL_GPL(tty_buffer_request_room); /** @@ -271,26 +255,18 @@ EXPORT_SYMBOL_GPL(tty_buffer_request_room); int tty_insert_flip_string_fixed_flag(struct tty_port *port, const unsigned char *chars, char flag, size_t size) { - struct tty_bufhead *buf = port-buf; int copied = 0; do { int goal = min_t(size_t, size - copied, TTY_BUFFER_PAGE); - int space; - unsigned long flags; - struct tty_buffer *tb; - - spin_lock_irqsave(buf-lock, flags); - space = __tty_buffer_request_room(port, goal); - tb = buf-tail; + int space = tty_buffer_request_room(port, goal); + struct tty_buffer *tb = port-buf.tail; /* If there is no space then tb may be NULL */ if (unlikely(space == 0)) { - spin_unlock_irqrestore(buf-lock, flags); break; } memcpy(tb-char_buf_ptr + tb-used, chars, space); memset(tb-flag_buf_ptr + tb-used, flag, space); tb-used += space; - spin_unlock_irqrestore(buf-lock, flags); copied += space; chars += space; /* There is a small chance that we need to split the data over @@ -317,26 +293,18 @@ EXPORT_SYMBOL(tty_insert_flip_string_fixed_flag); int
Re: [PATCH] tty: Correct tty buffer flush.
On Sat, Jan 19, 2013 at 4:12 PM, Sedat Dilek sedat.di...@gmail.com wrote: On Sat, Jan 19, 2013 at 3:16 PM, Ilya Zykov i...@ilyx.ru wrote: The root of problem is carelessly zeroing pointer(in function __tty_buffer_flush()), when another thread can use it. It can be cause of NULL pointer dereference. Main idea of the patch, this is never free last (struct tty_buffer) in the active buffer. Only flush the data for ldisc(buf-head-read = buf-head-commit). At that moment driver can collect(write) data in buffer without conflict. It is repeat behavior of flush_to_ldisc(), only without feeding data to ldisc. Also revert: commit c56a00a165712fd73081f40044b1e64407bb1875 tty: hold lock across tty buffer finding and buffer filling In order to delete the unneeded locks any more. Signed-off-by: Ilya Zykov i...@ilyx.ru I have tested your patch on top of Linux-Next (next-20130118) plus my own patchset (see file attachments)! [ TESTCASE ] # grep CONFIG_PM_DEBUG /boot/config-$(uname -r) CONFIG_PM_DEBUG=y # echo freezer /sys/power/pm_test # cat /sys/power/pm_test none core processors platform devices [freezer] # echo mem /sys/power/state sleep 1 --- 1st S/R: OK # echo mem /sys/power/state sleep 1 --- 2nd S/R: OK, but caused a call-trace (see above)! [ /TESTCASE ] Unfortunately, I get now a different call-trace on the 2nd freezer/pm-test... +[ 368.891613] PM: Preparing system for mem sleep +[ 373.886722] Freezing user space processes ... (elapsed 0.01 seconds) done. +[ 373.902741] Freezing remaining freezable tasks ... +[ 393.904587] Freezing of tasks failed after 20.01 seconds (1 tasks refusing to freeze, wq_busy=0): +[ 393.904714] jbd2/loop0-8D 0003 0 304 2 0x +[ 393.904723] 880117525b68 0046 8801195d9400 +[ 393.904731] 880117d74560 880117525fd8 880117525fd8 880117525fd8 +[ 393.904738] 88004212c560 880117d74560 880117525b68 88011fad4738 +[ 393.904745] Call Trace: +[ 393.904764] [811c6830] ? __wait_on_buffer+0x30/0x30 +[ 393.904772] [816b5079] schedule+0x29/0x70 +[ 393.904778] [816b514f] io_schedule+0x8f/0xd0 +[ 393.904785] [811c683e] sleep_on_buffer+0xe/0x20 +[ 393.904794] [816b394f] __wait_on_bit+0x5f/0x90 +[ 393.904801] [811c6830] ? __wait_on_buffer+0x30/0x30 +[ 393.904809] [816b39fc] out_of_line_wait_on_bit+0x7c/0x90 +[ 393.904817] [8107eb00] ? autoremove_wake_function+0x40/0x40 +[ 393.904824] [811c682e] __wait_on_buffer+0x2e/0x30 +[ 393.904836] [8128a14f] jbd2_journal_commit_transaction+0xdef/0x1960 +[ 393.904844] [816b620e] ? _raw_spin_lock_irqsave+0x2e/0x40 +[ 393.904853] [81069fbf] ? try_to_del_timer_sync+0x4f/0x70 +[ 393.904859] [8128e938] kjournald2+0xb8/0x240 +[ 393.904865] [8107eac0] ? add_wait_queue+0x60/0x60 +[ 393.904871] [8128e880] ? commit_timeout+0x10/0x10 +[ 393.904877] [8107ded0] kthread+0xc0/0xd0 +[ 393.904883] [8107de10] ? flush_kthread_worker+0xb0/0xb0 +[ 393.904889] [816bea2c] ret_from_fork+0x7c/0xb0 +[ 393.904894] [8107de10] ? flush_kthread_worker+0xb0/0xb0 +[ 393.904972] +[ 393.904975] Restarting kernel threads ... done. +[ 393.905163] Restarting tasks ... done. +[ 393.914283] video LNXVIDEO:00: Restoring backlight state If this ring one or more bells to you let me know! Please, have a look at the file attachments! Thanks. Hope this helps us to track the problem! I turned on... CONFIG_EXT4_DEBUG=y CONFIG_JBD2_DEBUG=y ...to see more light in the dark. - Sedat - - Sedat - --- drivers/tty/tty_buffer.c | 92 +++--- 1 files changed, 22 insertions(+), 70 deletions(-) diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c index d6969f6..61ec4dd 100644 --- a/drivers/tty/tty_buffer.c +++ b/drivers/tty/tty_buffer.c @@ -119,11 +119,14 @@ static void __tty_buffer_flush(struct tty_port *port) struct tty_bufhead *buf = port-buf; struct tty_buffer *thead; - while ((thead = buf-head) != NULL) { - buf-head = thead-next; - tty_buffer_free(port, thead); + if (unlikely(buf-head == NULL)) + return; + while ((thead = buf-head-next) != NULL) { + tty_buffer_free(port, buf-head); + buf-head = thead; } - buf-tail = NULL; + WARN_ON(buf-head != buf-tail); + buf-head-read = buf-head-commit; } /** @@ -194,19 +197,22 @@ static struct tty_buffer *tty_buffer_find(struct tty_port *port, size_t size) have queued and recycle that ? */ } /** - * __tty_buffer_request_room - grow tty buffer if needed + * tty_buffer_request_room - grow tty buffer if
Re: [PATCH] tty: Correct tty buffer flush.
On Wed, Jan 16, 2013 at 12:55:00PM +0400, Ilya Zykov wrote: > The root of problem is carelessly zeroing pointer(in function > __tty_buffer_flush()), > when another thread can use it. It can be cause of "NULL pointer dereference". > Main idea of the patch, this is never free last (struct tty_buffer) in the > active buffer. > Only flush the data for ldisc(buf->head->read = buf->head->commit). > At that moment driver can collect(write) data in buffer without conflict. > It is repeat behavior of flush_to_ldisc(), only without feeding data to ldisc. > > Also revert: > commit c56a00a165712fd73081f40044b1e64407bb1875 > tty: hold lock across tty buffer finding and buffer filling > In order to delete the unneeded locks any more. This patch doesn't apply to my tty-next branch, can you redo it against linux-next so that I can apply it? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] tty: Correct tty buffer flush.
On Wed, Jan 16, 2013 at 12:55:00PM +0400, Ilya Zykov wrote: The root of problem is carelessly zeroing pointer(in function __tty_buffer_flush()), when another thread can use it. It can be cause of NULL pointer dereference. Main idea of the patch, this is never free last (struct tty_buffer) in the active buffer. Only flush the data for ldisc(buf-head-read = buf-head-commit). At that moment driver can collect(write) data in buffer without conflict. It is repeat behavior of flush_to_ldisc(), only without feeding data to ldisc. Also revert: commit c56a00a165712fd73081f40044b1e64407bb1875 tty: hold lock across tty buffer finding and buffer filling In order to delete the unneeded locks any more. This patch doesn't apply to my tty-next branch, can you redo it against linux-next so that I can apply it? thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] tty: Correct tty buffer flush.
The root of problem is carelessly zeroing pointer(in function __tty_buffer_flush()), when another thread can use it. It can be cause of "NULL pointer dereference". Main idea of the patch, this is never free last (struct tty_buffer) in the active buffer. Only flush the data for ldisc(buf->head->read = buf->head->commit). At that moment driver can collect(write) data in buffer without conflict. It is repeat behavior of flush_to_ldisc(), only without feeding data to ldisc. Also revert: commit c56a00a165712fd73081f40044b1e64407bb1875 tty: hold lock across tty buffer finding and buffer filling In order to delete the unneeded locks any more. Signed-off-by: Ilya Zykov --- drivers/tty/tty_buffer.c | 105 +- 1 files changed, 29 insertions(+), 76 deletions(-) diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c index 45d9161..8ad 100644 --- a/drivers/tty/tty_buffer.c +++ b/drivers/tty/tty_buffer.c @@ -119,11 +119,14 @@ static void __tty_buffer_flush(struct tty_port *port) struct tty_bufhead *buf = >buf; struct tty_buffer *thead; - while ((thead = buf->head) != NULL) { - buf->head = thead->next; - tty_buffer_free(port, thead); + if (unlikely(buf->head == NULL)) + return; + while ((thead = buf->head->next) != NULL) { + tty_buffer_free(port, buf->head); + buf->head = thead; } - buf->tail = NULL; + WARN_ON(buf->head != buf->tail); + buf->head->read = buf->head->commit; } /** @@ -193,20 +196,26 @@ static struct tty_buffer *tty_buffer_find(struct tty_port *port, size_t size) /* Should possibly check if this fails for the largest buffer we have queued and recycle that ? */ } + /** - * __tty_buffer_request_room - grow tty buffer if needed + * tty_buffer_request_room - grow tty buffer if needed * @tty: tty structure * @size: size desired * * Make at least size bytes of linear space available for the tty * buffer. If we fail return the size we managed to find. - * Locking: Caller must hold port->buf.lock + * + * Locking: Takes tty->port->buf.lock */ -static int __tty_buffer_request_room(struct tty_port *port, size_t size) +int tty_buffer_request_room(struct tty_struct *tty, size_t size) { - struct tty_bufhead *buf = >buf; + struct tty_bufhead *buf = >port->buf; struct tty_buffer *b, *n; int left; + unsigned long flags; + + spin_lock_irqsave(>lock, flags); + /* OPTIMISATION: We could keep a per tty "zero" sized buffer to remove this conditional if its worth it. This would be invisible to the callers */ @@ -218,7 +227,7 @@ static int __tty_buffer_request_room(struct tty_port *port, size_t size) if (left < size) { /* This is the slow path - looking for new buffers to use */ - if ((n = tty_buffer_find(port, size)) != NULL) { + if ((n = tty_buffer_find(tty->port, size)) != NULL) { if (b != NULL) { b->next = n; b->commit = b->used; @@ -229,31 +238,9 @@ static int __tty_buffer_request_room(struct tty_port *port, size_t size) size = left; } + spin_unlock_irqrestore(>lock, flags); return size; } - - -/** - * tty_buffer_request_room - grow tty buffer if needed - * @tty: tty structure - * @size: size desired - * - * Make at least size bytes of linear space available for the tty - * buffer. If we fail return the size we managed to find. - * - * Locking: Takes port->buf.lock - */ -int tty_buffer_request_room(struct tty_struct *tty, size_t size) -{ - struct tty_port *port = tty->port; - unsigned long flags; - int length; - - spin_lock_irqsave(>buf.lock, flags); - length = __tty_buffer_request_room(port, size); - spin_unlock_irqrestore(>buf.lock, flags); - return length; -} EXPORT_SYMBOL_GPL(tty_buffer_request_room); /** @@ -272,26 +259,17 @@ EXPORT_SYMBOL_GPL(tty_buffer_request_room); int tty_insert_flip_string_fixed_flag(struct tty_struct *tty, const unsigned char *chars, char flag, size_t size) { - struct tty_bufhead *buf = >port->buf; int copied = 0; do { int goal = min_t(size_t, size - copied, TTY_BUFFER_PAGE); - int space; - unsigned long flags; - struct tty_buffer *tb; - - spin_lock_irqsave(>lock, flags); - space = __tty_buffer_request_room(tty->port, goal); - tb = buf->tail; + int space = tty_buffer_request_room(tty, goal); + struct tty_buffer *tb = tty->port->buf.tail; /* If there is no space then tb
[PATCH] tty: Correct tty buffer flush.
The root of problem is carelessly zeroing pointer(in function __tty_buffer_flush()), when another thread can use it. It can be cause of NULL pointer dereference. Main idea of the patch, this is never free last (struct tty_buffer) in the active buffer. Only flush the data for ldisc(buf-head-read = buf-head-commit). At that moment driver can collect(write) data in buffer without conflict. It is repeat behavior of flush_to_ldisc(), only without feeding data to ldisc. Also revert: commit c56a00a165712fd73081f40044b1e64407bb1875 tty: hold lock across tty buffer finding and buffer filling In order to delete the unneeded locks any more. Signed-off-by: Ilya Zykov i...@ilyx.ru --- drivers/tty/tty_buffer.c | 105 +- 1 files changed, 29 insertions(+), 76 deletions(-) diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c index 45d9161..8ad 100644 --- a/drivers/tty/tty_buffer.c +++ b/drivers/tty/tty_buffer.c @@ -119,11 +119,14 @@ static void __tty_buffer_flush(struct tty_port *port) struct tty_bufhead *buf = port-buf; struct tty_buffer *thead; - while ((thead = buf-head) != NULL) { - buf-head = thead-next; - tty_buffer_free(port, thead); + if (unlikely(buf-head == NULL)) + return; + while ((thead = buf-head-next) != NULL) { + tty_buffer_free(port, buf-head); + buf-head = thead; } - buf-tail = NULL; + WARN_ON(buf-head != buf-tail); + buf-head-read = buf-head-commit; } /** @@ -193,20 +196,26 @@ static struct tty_buffer *tty_buffer_find(struct tty_port *port, size_t size) /* Should possibly check if this fails for the largest buffer we have queued and recycle that ? */ } + /** - * __tty_buffer_request_room - grow tty buffer if needed + * tty_buffer_request_room - grow tty buffer if needed * @tty: tty structure * @size: size desired * * Make at least size bytes of linear space available for the tty * buffer. If we fail return the size we managed to find. - * Locking: Caller must hold port-buf.lock + * + * Locking: Takes tty-port-buf.lock */ -static int __tty_buffer_request_room(struct tty_port *port, size_t size) +int tty_buffer_request_room(struct tty_struct *tty, size_t size) { - struct tty_bufhead *buf = port-buf; + struct tty_bufhead *buf = tty-port-buf; struct tty_buffer *b, *n; int left; + unsigned long flags; + + spin_lock_irqsave(buf-lock, flags); + /* OPTIMISATION: We could keep a per tty zero sized buffer to remove this conditional if its worth it. This would be invisible to the callers */ @@ -218,7 +227,7 @@ static int __tty_buffer_request_room(struct tty_port *port, size_t size) if (left size) { /* This is the slow path - looking for new buffers to use */ - if ((n = tty_buffer_find(port, size)) != NULL) { + if ((n = tty_buffer_find(tty-port, size)) != NULL) { if (b != NULL) { b-next = n; b-commit = b-used; @@ -229,31 +238,9 @@ static int __tty_buffer_request_room(struct tty_port *port, size_t size) size = left; } + spin_unlock_irqrestore(buf-lock, flags); return size; } - - -/** - * tty_buffer_request_room - grow tty buffer if needed - * @tty: tty structure - * @size: size desired - * - * Make at least size bytes of linear space available for the tty - * buffer. If we fail return the size we managed to find. - * - * Locking: Takes port-buf.lock - */ -int tty_buffer_request_room(struct tty_struct *tty, size_t size) -{ - struct tty_port *port = tty-port; - unsigned long flags; - int length; - - spin_lock_irqsave(port-buf.lock, flags); - length = __tty_buffer_request_room(port, size); - spin_unlock_irqrestore(port-buf.lock, flags); - return length; -} EXPORT_SYMBOL_GPL(tty_buffer_request_room); /** @@ -272,26 +259,17 @@ EXPORT_SYMBOL_GPL(tty_buffer_request_room); int tty_insert_flip_string_fixed_flag(struct tty_struct *tty, const unsigned char *chars, char flag, size_t size) { - struct tty_bufhead *buf = tty-port-buf; int copied = 0; do { int goal = min_t(size_t, size - copied, TTY_BUFFER_PAGE); - int space; - unsigned long flags; - struct tty_buffer *tb; - - spin_lock_irqsave(buf-lock, flags); - space = __tty_buffer_request_room(tty-port, goal); - tb = buf-tail; + int space = tty_buffer_request_room(tty, goal); + struct tty_buffer *tb = tty-port-buf.tail; /* If there is no space then
Re: [PATCH] tty: Correct tty buffer flush.
On Tue, 04 Dec 2012 17:10:57 +0400 Ilya Zykov wrote: > The root of problem is carelessly zeroing pointer(in function > __tty_buffer_flush()), > when another thread can use it. It can be cause of "NULL pointer dereference". > Main idea of the patch, this is never free last (struct tty_buffer) in the > active buffer. > Only flush the data for ldisc(tty->buf.head->read = tty->buf.head->commit). > At that moment driver can collect(write) data in buffer without conflict. > It is repeat behavior of flush_to_ldisc(), only without feeding data to ldisc. > > Also revert: > commit c56a00a165712fd73081f40044b1e64407bb1875 > tty: hold lock across tty buffer finding and buffer filling > In order to delete the unneeded locks any more. > > Signed-off-by: Ilya Zykov Acked-by: Alan Cox -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] tty: Correct tty buffer flush.
The root of problem is carelessly zeroing pointer(in function __tty_buffer_flush()), when another thread can use it. It can be cause of "NULL pointer dereference". Main idea of the patch, this is never free last (struct tty_buffer) in the active buffer. Only flush the data for ldisc(tty->buf.head->read = tty->buf.head->commit). At that moment driver can collect(write) data in buffer without conflict. It is repeat behavior of flush_to_ldisc(), only without feeding data to ldisc. Also revert: commit c56a00a165712fd73081f40044b1e64407bb1875 tty: hold lock across tty buffer finding and buffer filling In order to delete the unneeded locks any more. Signed-off-by: Ilya Zykov --- drivers/tty/tty_buffer.c | 96 +- 1 files changed, 27 insertions(+), 69 deletions(-) diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c index 91e326f..4f02f9c 100644 --- a/drivers/tty/tty_buffer.c +++ b/drivers/tty/tty_buffer.c @@ -114,11 +114,14 @@ static void __tty_buffer_flush(struct tty_struct *tty) { struct tty_buffer *thead; - while ((thead = tty->buf.head) != NULL) { - tty->buf.head = thead->next; - tty_buffer_free(tty, thead); + if (tty->buf.head == NULL) + return; + while ((thead = tty->buf.head->next) != NULL) { + tty_buffer_free(tty, tty->buf.head); + tty->buf.head = thead; } - tty->buf.tail = NULL; + WARN_ON(tty->buf.head != tty->buf.tail); + tty->buf.head->read = tty->buf.head->commit; } /** @@ -185,19 +188,25 @@ static struct tty_buffer *tty_buffer_find(struct tty_struct *tty, size_t size) /* Should possibly check if this fails for the largest buffer we have queued and recycle that ? */ } + /** - * __tty_buffer_request_room - grow tty buffer if needed + * tty_buffer_request_room - grow tty buffer if needed * @tty: tty structure * @size: size desired * * Make at least size bytes of linear space available for the tty * buffer. If we fail return the size we managed to find. - * Locking: Caller must hold tty->buf.lock + * + * Locking: Takes tty->buf.lock */ -static int __tty_buffer_request_room(struct tty_struct *tty, size_t size) +int tty_buffer_request_room(struct tty_struct *tty, size_t size) { struct tty_buffer *b, *n; int left; + unsigned long flags; + + spin_lock_irqsave(>buf.lock, flags); + /* OPTIMISATION: We could keep a per tty "zero" sized buffer to remove this conditional if its worth it. This would be invisible to the callers */ @@ -219,29 +228,8 @@ static int __tty_buffer_request_room(struct tty_struct *tty, size_t size) size = left; } - return size; -} - - -/** - * tty_buffer_request_room - grow tty buffer if needed - * @tty: tty structure - * @size: size desired - * - * Make at least size bytes of linear space available for the tty - * buffer. If we fail return the size we managed to find. - * - * Locking: Takes tty->buf.lock - */ -int tty_buffer_request_room(struct tty_struct *tty, size_t size) -{ - unsigned long flags; - int length; - - spin_lock_irqsave(>buf.lock, flags); - length = __tty_buffer_request_room(tty, size); spin_unlock_irqrestore(>buf.lock, flags); - return length; + return size; } EXPORT_SYMBOL_GPL(tty_buffer_request_room); @@ -264,22 +252,14 @@ int tty_insert_flip_string_fixed_flag(struct tty_struct *tty, int copied = 0; do { int goal = min_t(size_t, size - copied, TTY_BUFFER_PAGE); - int space; - unsigned long flags; - struct tty_buffer *tb; - - spin_lock_irqsave(>buf.lock, flags); - space = __tty_buffer_request_room(tty, goal); - tb = tty->buf.tail; + int space = tty_buffer_request_room(tty, goal); + struct tty_buffer *tb = tty->buf.tail; /* If there is no space then tb may be NULL */ - if (unlikely(space == 0)) { - spin_unlock_irqrestore(>buf.lock, flags); + if (unlikely(space == 0)) break; - } memcpy(tb->char_buf_ptr + tb->used, chars, space); memset(tb->flag_buf_ptr + tb->used, flag, space); tb->used += space; - spin_unlock_irqrestore(>buf.lock, flags); copied += space; chars += space; /* There is a small chance that we need to split the data over @@ -309,22 +289,14 @@ int tty_insert_flip_string_flags(struct tty_struct *tty, int copied = 0; do { int goal = min_t(size_t, size - copied, TTY_BUFFER_PAGE); - int
[PATCH] tty: Correct tty buffer flush.
The root of problem is carelessly zeroing pointer(in function __tty_buffer_flush()), when another thread can use it. It can be cause of NULL pointer dereference. Main idea of the patch, this is never free last (struct tty_buffer) in the active buffer. Only flush the data for ldisc(tty-buf.head-read = tty-buf.head-commit). At that moment driver can collect(write) data in buffer without conflict. It is repeat behavior of flush_to_ldisc(), only without feeding data to ldisc. Also revert: commit c56a00a165712fd73081f40044b1e64407bb1875 tty: hold lock across tty buffer finding and buffer filling In order to delete the unneeded locks any more. Signed-off-by: Ilya Zykov i...@ilyx.ru --- drivers/tty/tty_buffer.c | 96 +- 1 files changed, 27 insertions(+), 69 deletions(-) diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c index 91e326f..4f02f9c 100644 --- a/drivers/tty/tty_buffer.c +++ b/drivers/tty/tty_buffer.c @@ -114,11 +114,14 @@ static void __tty_buffer_flush(struct tty_struct *tty) { struct tty_buffer *thead; - while ((thead = tty-buf.head) != NULL) { - tty-buf.head = thead-next; - tty_buffer_free(tty, thead); + if (tty-buf.head == NULL) + return; + while ((thead = tty-buf.head-next) != NULL) { + tty_buffer_free(tty, tty-buf.head); + tty-buf.head = thead; } - tty-buf.tail = NULL; + WARN_ON(tty-buf.head != tty-buf.tail); + tty-buf.head-read = tty-buf.head-commit; } /** @@ -185,19 +188,25 @@ static struct tty_buffer *tty_buffer_find(struct tty_struct *tty, size_t size) /* Should possibly check if this fails for the largest buffer we have queued and recycle that ? */ } + /** - * __tty_buffer_request_room - grow tty buffer if needed + * tty_buffer_request_room - grow tty buffer if needed * @tty: tty structure * @size: size desired * * Make at least size bytes of linear space available for the tty * buffer. If we fail return the size we managed to find. - * Locking: Caller must hold tty-buf.lock + * + * Locking: Takes tty-buf.lock */ -static int __tty_buffer_request_room(struct tty_struct *tty, size_t size) +int tty_buffer_request_room(struct tty_struct *tty, size_t size) { struct tty_buffer *b, *n; int left; + unsigned long flags; + + spin_lock_irqsave(tty-buf.lock, flags); + /* OPTIMISATION: We could keep a per tty zero sized buffer to remove this conditional if its worth it. This would be invisible to the callers */ @@ -219,29 +228,8 @@ static int __tty_buffer_request_room(struct tty_struct *tty, size_t size) size = left; } - return size; -} - - -/** - * tty_buffer_request_room - grow tty buffer if needed - * @tty: tty structure - * @size: size desired - * - * Make at least size bytes of linear space available for the tty - * buffer. If we fail return the size we managed to find. - * - * Locking: Takes tty-buf.lock - */ -int tty_buffer_request_room(struct tty_struct *tty, size_t size) -{ - unsigned long flags; - int length; - - spin_lock_irqsave(tty-buf.lock, flags); - length = __tty_buffer_request_room(tty, size); spin_unlock_irqrestore(tty-buf.lock, flags); - return length; + return size; } EXPORT_SYMBOL_GPL(tty_buffer_request_room); @@ -264,22 +252,14 @@ int tty_insert_flip_string_fixed_flag(struct tty_struct *tty, int copied = 0; do { int goal = min_t(size_t, size - copied, TTY_BUFFER_PAGE); - int space; - unsigned long flags; - struct tty_buffer *tb; - - spin_lock_irqsave(tty-buf.lock, flags); - space = __tty_buffer_request_room(tty, goal); - tb = tty-buf.tail; + int space = tty_buffer_request_room(tty, goal); + struct tty_buffer *tb = tty-buf.tail; /* If there is no space then tb may be NULL */ - if (unlikely(space == 0)) { - spin_unlock_irqrestore(tty-buf.lock, flags); + if (unlikely(space == 0)) break; - } memcpy(tb-char_buf_ptr + tb-used, chars, space); memset(tb-flag_buf_ptr + tb-used, flag, space); tb-used += space; - spin_unlock_irqrestore(tty-buf.lock, flags); copied += space; chars += space; /* There is a small chance that we need to split the data over @@ -309,22 +289,14 @@ int tty_insert_flip_string_flags(struct tty_struct *tty, int copied = 0; do { int goal = min_t(size_t, size - copied, TTY_BUFFER_PAGE); - int
Re: [PATCH] tty: Correct tty buffer flush.
On Tue, 04 Dec 2012 17:10:57 +0400 Ilya Zykov i...@ilyx.ru wrote: The root of problem is carelessly zeroing pointer(in function __tty_buffer_flush()), when another thread can use it. It can be cause of NULL pointer dereference. Main idea of the patch, this is never free last (struct tty_buffer) in the active buffer. Only flush the data for ldisc(tty-buf.head-read = tty-buf.head-commit). At that moment driver can collect(write) data in buffer without conflict. It is repeat behavior of flush_to_ldisc(), only without feeding data to ldisc. Also revert: commit c56a00a165712fd73081f40044b1e64407bb1875 tty: hold lock across tty buffer finding and buffer filling In order to delete the unneeded locks any more. Signed-off-by: Ilya Zykov i...@ilyx.ru Acked-by: Alan Cox a...@linux.intel.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] tty: Correct tty buffer flush.
The root of problem is carelessly zeroing pointer(in function __tty_buffer_flush()), when another thread can use it. It can be cause of "NULL pointer dereference". Main idea of the patch, this is never release last (struct tty_buffer) in the active buffer. Only flush data for ldisc(tty->buf.head->read = tty->buf.head->commit). At that moment driver can collect(write) data in buffer without conflict. It is repeat behavior of flush_to_ldisc(), only without feeding data to ldisc. Test program and bug report you can see: https://lkml.org/lkml/2012/11/29/368 Cc: sta...@vger.kernel.org Signed-off-by: Ilya Zykov --- diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c index 6c9b7cd..4f02f9c 100644 --- a/drivers/tty/tty_buffer.c +++ b/drivers/tty/tty_buffer.c @@ -114,11 +114,14 @@ static void __tty_buffer_flush(struct tty_struct *tty) { struct tty_buffer *thead; - while ((thead = tty->buf.head) != NULL) { - tty->buf.head = thead->next; - tty_buffer_free(tty, thead); + if (tty->buf.head == NULL) + return; + while ((thead = tty->buf.head->next) != NULL) { + tty_buffer_free(tty, tty->buf.head); + tty->buf.head = thead; } - tty->buf.tail = NULL; + WARN_ON(tty->buf.head != tty->buf.tail); + tty->buf.head->read = tty->buf.head->commit; } /** -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] tty: Correct tty buffer flush.
The root of problem is carelessly zeroing pointer(in function __tty_buffer_flush()), when another thread can use it. It can be cause of NULL pointer dereference. Main idea of the patch, this is never release last (struct tty_buffer) in the active buffer. Only flush data for ldisc(tty-buf.head-read = tty-buf.head-commit). At that moment driver can collect(write) data in buffer without conflict. It is repeat behavior of flush_to_ldisc(), only without feeding data to ldisc. Test program and bug report you can see: https://lkml.org/lkml/2012/11/29/368 Cc: sta...@vger.kernel.org Signed-off-by: Ilya Zykov i...@ilyx.ru --- diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c index 6c9b7cd..4f02f9c 100644 --- a/drivers/tty/tty_buffer.c +++ b/drivers/tty/tty_buffer.c @@ -114,11 +114,14 @@ static void __tty_buffer_flush(struct tty_struct *tty) { struct tty_buffer *thead; - while ((thead = tty-buf.head) != NULL) { - tty-buf.head = thead-next; - tty_buffer_free(tty, thead); + if (tty-buf.head == NULL) + return; + while ((thead = tty-buf.head-next) != NULL) { + tty_buffer_free(tty, tty-buf.head); + tty-buf.head = thead; } - tty-buf.tail = NULL; + WARN_ON(tty-buf.head != tty-buf.tail); + tty-buf.head-read = tty-buf.head-commit; } /** -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/