Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1220104 > unrolled thread
| Started by | Dmitry Vyukov <dvyukov@google.com> |
|---|---|
| First post | 2015-09-07 14:20 +0200 |
| Last post | 2015-09-16 04:00 +0200 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
[PATCH] tty: fix data race on tty_buffer.commit Dmitry Vyukov <dvyukov@google.com> - 2015-09-07 14:20 +0200
[PATCH] tty: fix data race on tty_buffer.commit Dmitry Vyukov <dvyukov@google.com> - 2015-09-08 15:10 +0200
Re: [PATCH] tty: fix data race on tty_buffer.commit Peter Hurley <peter@hurleysoftware.com> - 2015-09-16 04:00 +0200
| From | Dmitry Vyukov <dvyukov@google.com> |
|---|---|
| Date | 2015-09-07 14:20 +0200 |
| Subject | [PATCH] tty: fix data race on tty_buffer.commit |
| Message-ID | <q63yV-5hl-9@gated-at.bofh.it> |
Race on buffer data happens when newly committed data is
picked up by an old flush work in the following scenario:
__tty_buffer_request_room does a plain write of tail->commit,
no barriers were executed before that.
At this point flush_to_ldisc reads this new value of commit,
and reads buffer data, no barriers in between.
The committed buffer data is not necessary visible to flush_to_ldisc.
Similar bug happens when tty_schedule_flip commits data.
Update commit with smp_store_release and read commit with
smp_load_acquire, as it is commit that signals data readiness.
This is orthogonal to the existing synchronization on tty_buffer.next,
which is required to not dismiss a buffer with unconsumed data.
The data race was found with KernelThreadSanitizer (KTSAN).
Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
---
drivers/tty/tty_buffer.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 5a3fa89..0e39d96 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -290,7 +290,10 @@ static int __tty_buffer_request_room(struct tty_port *port, size_t size,
if (n != NULL) {
n->flags = flags;
buf->tail = n;
- b->commit = b->used;
+ /* paired w/ acquire in flush_to_ldisc(); ensures
+ * flush_to_ldisc() sees buffer data.
+ */
+ smp_store_release(&b->commit, b->used);
/* paired w/ acquire in flush_to_ldisc(); ensures the
* latest commit value can be read before the head is
* advanced to the next buffer
@@ -393,7 +396,10 @@ void tty_schedule_flip(struct tty_port *port)
{
struct tty_bufhead *buf = &port->buf;
- buf->tail->commit = buf->tail->used;
+ /* paired w/ acquire in flush_to_ldisc(); ensures
+ * flush_to_ldisc() sees buffer data.
+ */
+ smp_store_release(&b->commit, buf->tail->used);
schedule_work(&buf->work);
}
EXPORT_SYMBOL(tty_schedule_flip);
@@ -491,7 +497,10 @@ static void flush_to_ldisc(struct work_struct *work)
* is advancing to the next buffer
*/
next = smp_load_acquire(&head->next);
- count = head->commit - head->read;
+ /* paired w/ release in __tty_buffer_request_room() or in
+ * tty_buffer_flush(); ensures we see the committed buffer data
+ */
+ count = smp_load_acquire(&head->commit) - head->read;
if (!count) {
if (next == NULL) {
check_other_closed(tty);
--
2.5.0.457.gab17608
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Dmitry Vyukov <dvyukov@google.com> |
|---|---|
| Date | 2015-09-08 15:10 +0200 |
| Message-ID | <q6qOU-57i-53@gated-at.bofh.it> |
| In reply to | #1220104 |
Race on buffer data happens when newly committed data is
picked up by an old flush work in the following scenario:
__tty_buffer_request_room does a plain write of tail->commit,
no barriers were executed before that.
At this point flush_to_ldisc reads this new value of commit,
and reads buffer data, no barriers in between.
The committed buffer data is not necessary visible to flush_to_ldisc.
Similar bug happens when tty_schedule_flip commits data.
Update commit with smp_store_release and read commit with
smp_load_acquire, as it is commit that signals data readiness.
This is orthogonal to the existing synchronization on tty_buffer.next,
which is required to not dismiss a buffer with unconsumed data.
The data race was found with KernelThreadSanitizer (KTSAN).
Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
---
drivers/tty/tty_buffer.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 5a3fa89..7ed2006 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -290,7 +290,10 @@ static int __tty_buffer_request_room(struct tty_port *port, size_t size,
if (n != NULL) {
n->flags = flags;
buf->tail = n;
- b->commit = b->used;
+ /* paired w/ acquire in flush_to_ldisc(); ensures
+ * flush_to_ldisc() sees buffer data.
+ */
+ smp_store_release(&b->commit, b->used);
/* paired w/ acquire in flush_to_ldisc(); ensures the
* latest commit value can be read before the head is
* advanced to the next buffer
@@ -393,7 +396,10 @@ void tty_schedule_flip(struct tty_port *port)
{
struct tty_bufhead *buf = &port->buf;
- buf->tail->commit = buf->tail->used;
+ /* paired w/ acquire in flush_to_ldisc(); ensures
+ * flush_to_ldisc() sees buffer data.
+ */
+ smp_store_release(&buf->tail->commit, buf->tail->used);
schedule_work(&buf->work);
}
EXPORT_SYMBOL(tty_schedule_flip);
@@ -491,7 +497,10 @@ static void flush_to_ldisc(struct work_struct *work)
* is advancing to the next buffer
*/
next = smp_load_acquire(&head->next);
- count = head->commit - head->read;
+ /* paired w/ release in __tty_buffer_request_room() or in
+ * tty_buffer_flush(); ensures we see the committed buffer data
+ */
+ count = smp_load_acquire(&head->commit) - head->read;
if (!count) {
if (next == NULL) {
check_other_closed(tty);
--
2.5.0.457.gab17608
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Peter Hurley <peter@hurleysoftware.com> |
|---|---|
| Date | 2015-09-16 04:00 +0200 |
| Message-ID | <q9aaS-6Rm-17@gated-at.bofh.it> |
| In reply to | #1220769 |
On Tue, Sep 8, 2015 at 9:03 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
> Race on buffer data happens when newly committed data is
> picked up by an old flush work in the following scenario:
> __tty_buffer_request_room does a plain write of tail->commit,
> no barriers were executed before that.
> At this point flush_to_ldisc reads this new value of commit,
> and reads buffer data, no barriers in between.
> The committed buffer data is not necessary visible to flush_to_ldisc.
>
> Similar bug happens when tty_schedule_flip commits data.
>
> Update commit with smp_store_release and read commit with
> smp_load_acquire, as it is commit that signals data readiness.
> This is orthogonal to the existing synchronization on tty_buffer.next,
> which is required to not dismiss a buffer with unconsumed data.
>
> The data race was found with KernelThreadSanitizer (KTSAN).
Looks good.
Same comments as the other patch; needs changelog revision
information.
Thanks for your work on this; really good stuff.
Regards,
Peter Hurley
> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
> ---
> drivers/tty/tty_buffer.c | 15 ++++++++++++---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
> index 5a3fa89..7ed2006 100644
> --- a/drivers/tty/tty_buffer.c
> +++ b/drivers/tty/tty_buffer.c
> @@ -290,7 +290,10 @@ static int __tty_buffer_request_room(struct tty_port *port, size_t size,
> if (n != NULL) {
> n->flags = flags;
> buf->tail = n;
> - b->commit = b->used;
> + /* paired w/ acquire in flush_to_ldisc(); ensures
> + * flush_to_ldisc() sees buffer data.
> + */
> + smp_store_release(&b->commit, b->used);
> /* paired w/ acquire in flush_to_ldisc(); ensures the
> * latest commit value can be read before the head is
> * advanced to the next buffer
> @@ -393,7 +396,10 @@ void tty_schedule_flip(struct tty_port *port)
> {
> struct tty_bufhead *buf = &port->buf;
>
> - buf->tail->commit = buf->tail->used;
> + /* paired w/ acquire in flush_to_ldisc(); ensures
> + * flush_to_ldisc() sees buffer data.
> + */
> + smp_store_release(&buf->tail->commit, buf->tail->used);
> schedule_work(&buf->work);
> }
> EXPORT_SYMBOL(tty_schedule_flip);
> @@ -491,7 +497,10 @@ static void flush_to_ldisc(struct work_struct *work)
> * is advancing to the next buffer
> */
> next = smp_load_acquire(&head->next);
> - count = head->commit - head->read;
> + /* paired w/ release in __tty_buffer_request_room() or in
> + * tty_buffer_flush(); ensures we see the committed buffer data
> + */
> + count = smp_load_acquire(&head->commit) - head->read;
> if (!count) {
> if (next == NULL) {
> check_other_closed(tty);
> --
> 2.5.0.457.gab17608
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web