Revert "tty: Fix race condition between __tty_buffer_request_room and flush_to_ldisc"
Subsystems (2)
PATCH 1/2 Revert "tty: Fix race condition between __tty_buffer_request_room and flush_to_ldisc"
This reverts commit 6a20dbd6caa2358716136144bf524331d70b1e03.
Although the commit correctly identifies an unsafe race condition between __tty_buffer_request_room() and flush_to_ldisc(), the commit fixes the race with an unnecessary spinlock in a lockless algorithm.
The follow-on commit, "tty: Fix lockless tty buffer race" fixes the race locklessly.
--- drivers/tty/tty_buffer.c | 16 ++-------------- include/linux/tty.h | 1 - 2 files changed, 2 insertions(+), 15 deletions(-)
drivers/tty/tty_buffer.c +2 -14
@@ -255,16 +255,11 @@ static int __tty_buffer_request_room(struct tty_port *port, size_t size,if (change || left < size) {/* This is the slow path - looking for new buffers to use */ if ((n =tty_buffer_alloc (port, size)) !=NULL ) { -unsigned long iflags; - n->flags = flags; buf->tail = n; - -spin_lock_irqsave (&buf->flush_lock, iflags); b->commit = b->used; +smp_mb (); b->next = n; -spin_unlock_irqrestore (&buf->flush_lock, iflags); - }else if (change) size =0 ;else
@@ -448,7 +443,6 @@ static void flush_to_ldisc(struct work_struct *work)mutex_lock (&buf->lock);while (1 ) { -unsigned long flags;struct tty_buffer *head = buf->head;int count;
@@ -456,19 +450,14 @@ static void flush_to_ldisc(struct work_struct *work)if (atomic_read (&buf->priority))break ; -spin_lock_irqsave (&buf->flush_lock, flags); count = head->commit - head->read;if (!count) { -if (head->next ==NULL ) { -spin_unlock_irqrestore (&buf->flush_lock, flags); +if (head->next ==NULL )break ; - } buf->head = head->next; -spin_unlock_irqrestore (&buf->flush_lock, flags);tty_buffer_free (port, head);continue ; } -spin_unlock_irqrestore (&buf->flush_lock, flags); count =receive_buf (tty, head, count);if (!count)
@@ -523,7 +512,6 @@ void tty_buffer_init(struct tty_port *port)struct tty_bufhead *buf = &port->buf;mutex_init (&buf->lock); -spin_lock_init (&buf->flush_lock);tty_buffer_reset (&buf->sentinel,0 ); buf->head = &buf->sentinel; buf->tail = &buf->sentinel;
include/linux/tty.h +0 -1
@@ -61,7 +61,6 @@ struct tty_bufhead {struct tty_buffer *head;/* Queue head */ struct work_struct work;struct mutex lock; -spinlock_t flush_lock;atomic_t priority;struct tty_buffer sentinel;struct llist_head free;/* Free queue head */
Signature · 1 line
1.8.1.2
PATCH 2/2 tty: Fix lockless tty buffer race
Commit 6a20dbd6caa2358716136144bf524331d70b1e03,
"tty: Fix race condition between __tty_buffer_request_room and flush_to_ldisc" correctly identifies an unsafe race condition between __tty_buffer_request_room() and flush_to_ldisc(), where the consumer flush_to_ldisc() prematurely advances the head before consuming the last of the data committed. For example:
CPU 0 | CPU 1
__tty_buffer_request_room | flush_to_ldisc
... | ...
| count = head->commit - head->read
n = tty_buffer_alloc() |
b->commit = b->used |
b->next = n |
| if (!count) /* T */
| if (head->next == NULL) /* F */
| buf->head = head->nextIn this case, buf->head has been advanced but head->commit may have been updated with a new value.
Instead of reintroducing an unnecessary lock, fix the race locklessly. Read the commit-next pair in the reverse order of writing, which guarantees the commit value read is the latest value written if the head is advancing.
--- drivers/tty/tty_buffer.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)
drivers/tty/tty_buffer.c +14 -3
@@ -258,7 +258,11 @@ static int __tty_buffer_request_room(struct tty_port *port, size_t size, n->flags = flags; buf->tail = n; b->commit = b->used; -smp_mb (); +/* paired w/ barrier in flush_to_ldisc(); ensures the +* latest commit value can be read before the head is +* advanced to the next buffer +*/ +smp_wmb (); b->next = n; }else if (change) size =0 ;
@@ -444,17 +448,24 @@ static void flush_to_ldisc(struct work_struct *work)while (1 ) {struct tty_buffer *head = buf->head; +struct tty_buffer *next;int count;/* Ldisc or user is trying to gain exclusive access */ if (atomic_read (&buf->priority))break ; + next = head->next; +/* paired w/ barrier in __tty_buffer_request_room(); +* ensures commit value read is not stale if the head +* is advancing to the next buffer +*/ +smp_rmb (); count = head->commit - head->read;if (!count) { -if (head->next ==NULL ) +if (next ==NULL )break ; - buf->head = head->next; + buf->head = next;tty_buffer_free (port, head);continue ; }
Signature · 1 line
1.8.1.2
Re: [PATCH 2/2] tty: Fix lockless tty buffer race
Quoted 21 lines · Peter Hurley
Commit
6a20dbd6caa2358716136144bf524331d70b1e03,
"tty: Fix race condition between__tty_buffer_request_roomandflush_to_ldisc" correctly identifies an unsafe race condition between__tty_buffer_request_room()andflush_to_ldisc(), where the consumerflush_to_ldisc()prematurely advances the head before consuming the last of the data committed. For example:CPU 0 | CPU 1
__tty_buffer_request_room|flush_to_ldisc... | ... | count = head->commit - head->read n = tty_buffer_alloc() | b->commit = b->used | b->next = n | | if (!count) /* T */ | if (head->next == NULL) /* F */ | buf->head = head->nextIn this case,
buf->headhas been advanced buthead->commitmay have been updated with a new value.
Quoted 7 lines
Instead of reintroducing an unnecessary lock, fix the race locklessly. Read the commit-next pair in the reverse order of writing, which guarantees the commit value read is the latest value written if the head is advancing.
Reported-by Manfred Schlaegl <[email protected]>Cc <[email protected]> # 3.12.x+
The patch submitted by Manfred notes the commits which introduced the race [1], but attributes those commits to the 3.11 cycle. Those commits were merged in the 3.12 cycle.
Regards,
Peter Hurley
[1] commits e8437d7ecbc50198705331449367d401ebb3181f, "tty: Make driver-side flip buffers lockless", and e9975fdec0138f1b2a85b9624e41660abd9865d4,
"tty: Ensure single-threaded flip buffer consumer with mutex"
Re: [PATCH 2/2] tty: Fix lockless tty buffer race
Quoted 19 lines · Peter Hurley
Quoted 18 lines · Peter Hurley
Commit
6a20dbd6caa2358716136144bf524331d70b1e03,
"tty: Fix race condition between__tty_buffer_request_roomandflush_to_ldisc" correctly identifies an unsafe race condition between__tty_buffer_request_room()andflush_to_ldisc(), where the consumerflush_to_ldisc()prematurely advances the head before consuming the last of the data committed. For example:CPU 0 | CPU 1
__tty_buffer_request_room|flush_to_ldisc... | ... | count = head->commit - head->read n = tty_buffer_alloc() | b->commit = b->used | b->next = n | | if (!count) /* T */ | if (head->next == NULL) /* F */ | buf->head = head->next
Quoted 7 lines
Quoted 7 lines
In this case,
buf->headhas been advanced buthead->commitmay have been updated with a new value.Instead of reintroducing an unnecessary lock, fix the race locklessly. Read the commit-next pair in the reverse order of writing, which guarantees the commit value read is the latest value written if the head is advancing.
This is a fine solution! I'll verify this against my previous experimental setup (3.12.x and 3.12.x-rt25), but I dont't expect any problems.
Quoted 7 lines
Quoted 3 lines
Reported-by Manfred Schlaegl <[email protected]>Cc <[email protected]> # 3.12.x+The patch submitted by Manfred notes the commits which introduced the race [1], but attributes those commits to the 3.11 cycle. Those commits were merged in the 3.12 cycle.
You are right. I'm sorry for this.
Regars,
Manfred