On Tue, Sep 8, 2015 at 9:03 AM, Dmitry Vyukov <[email protected]> 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 <[email protected]> > --- > 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
