From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933527AbbIDTJx (ORCPT ); Fri, 4 Sep 2015 15:09:53 -0400 Received: from mail-wi0-f173.google.com ([209.85.212.173]:35856 "EHLO mail-wi0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932987AbbIDTJw (ORCPT ); Fri, 4 Sep 2015 15:09:52 -0400 From: Dmitry Vyukov To: gregkh@linuxfoundation.org, peter@hurleysoftware.com, jslaby@suse.com, linux-kernel@vger.kernel.org Cc: jslaby@suse.cz, andreyknvl@google.com, kcc@google.com, glider@google.com, paulmck@linux.vnet.ibm.com, hboehm@google.com, Dmitry Vyukov Subject: [PATCH] tty: fix data races on tty_buffer.commit Date: Fri, 4 Sep 2015 21:09:46 +0200 Message-Id: <1441393786-97709-1-git-send-email-dvyukov@google.com> X-Mailer: git-send-email 2.5.0.457.gab17608 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Race on buffer data happens 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. Another race happens in tty_buffer_flush. It uses plain reads to read tty_buffer.next, as the result it can free a buffer which has pending writes in __tty_buffer_request_room thread. For example, tty_buffer_flush calls tty_buffer_free which reads b->size, the size may not be visible to this thread. As the result a large buffer can hang in the freelist. 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 --- drivers/tty/tty_buffer.c | 38 ++++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c index 4cf263d..4fae5d1 100644 --- a/drivers/tty/tty_buffer.c +++ b/drivers/tty/tty_buffer.c @@ -89,7 +89,7 @@ void tty_buffer_unlock_exclusive(struct tty_port *port) struct tty_bufhead *buf = &port->buf; int restart; - restart = buf->head->commit != buf->head->read; + restart = READ_ONCE(buf->head->commit) != buf->head->read; atomic_dec(&buf->priority); mutex_unlock(&buf->lock); @@ -242,11 +242,14 @@ void tty_buffer_flush(struct tty_struct *tty, struct tty_ldisc *ld) atomic_inc(&buf->priority); mutex_lock(&buf->lock); - while ((next = buf->head->next) != NULL) { + /* paired with smp_store_release in __tty_buffer_request_room(); + * ensures there are no outstanding writes to buf->head when we free it + */ + while ((next = smp_load_acquire(&buf->head->next)) != NULL) { tty_buffer_free(port, buf->head); buf->head = next; } - buf->head->read = buf->head->commit; + buf->head->read = READ_ONCE(buf->head->commit); if (ld && ld->ops->flush_buffer) ld->ops->flush_buffer(tty); @@ -290,13 +293,15 @@ 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/ barrier in flush_to_ldisc(); ensures the - * latest commit value can be read before the head is - * advanced to the next buffer + /* paired with smp_load_acquire in flush_to_ldisc(); + * ensures flush_to_ldisc() sees buffer data. */ - smp_wmb(); - b->next = n; + smp_store_release(&b->commit, b->used); + /* paired with smp_load_acquire in flush_to_ldisc(); + * ensures the latest commit value can be read before + * the head is advanced to the next buffer + */ + smp_store_release(&b->next, n); } else if (change) size = 0; else @@ -394,7 +399,10 @@ void tty_schedule_flip(struct tty_port *port) { struct tty_bufhead *buf = &port->buf; - buf->tail->commit = buf->tail->used; + /* paired with smp_load_acquire in flush_to_ldisc(); ensures the + * committed data is visible to flush_to_ldisc() + */ + smp_store_release(&buf->tail->commit, buf->tail->used); schedule_work(&buf->work); } EXPORT_SYMBOL(tty_schedule_flip); @@ -488,13 +496,15 @@ static void flush_to_ldisc(struct work_struct *work) if (atomic_read(&buf->priority)) break; - next = head->next; - /* paired w/ barrier in __tty_buffer_request_room(); + /* paired with smp_store_release 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; + next = smp_load_acquire(&head->next); + /* paired with smp_store_release in __tty_buffer_request_room(); + * 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