* [PATCH] tty: fix data races on tty_buffer.commit
@ 2015-09-04 19:09 Dmitry Vyukov
2015-09-04 19:34 ` Peter Hurley
0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Vyukov @ 2015-09-04 19:09 UTC (permalink / raw)
To: gregkh, peter, jslaby, linux-kernel
Cc: jslaby, andreyknvl, kcc, glider, paulmck, hboehm, Dmitry Vyukov
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 <dvyukov@google.com>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] tty: fix data races on tty_buffer.commit
2015-09-04 19:09 [PATCH] tty: fix data races on tty_buffer.commit Dmitry Vyukov
@ 2015-09-04 19:34 ` Peter Hurley
2015-09-04 19:37 ` Dmitry Vyukov
0 siblings, 1 reply; 6+ messages in thread
From: Peter Hurley @ 2015-09-04 19:34 UTC (permalink / raw)
To: Dmitry Vyukov, gregkh
Cc: jslaby, linux-kernel, jslaby, andreyknvl, kcc, glider, paulmck,
hboehm
Hi Dmitry,
On 09/04/2015 03:09 PM, Dmitry Vyukov wrote:
> 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.
Please submit one patch for each "fix", because it is not possible
to review what you believe you're fixing.
See below for an example.
> 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 <dvyukov@google.com>
> ---
> 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);
schedule_work() is an implied barrier for obvious reasons.
Regards.
Peter Hurley
> }
> 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);
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tty: fix data races on tty_buffer.commit
2015-09-04 19:34 ` Peter Hurley
@ 2015-09-04 19:37 ` Dmitry Vyukov
2015-09-04 19:49 ` Peter Hurley
0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Vyukov @ 2015-09-04 19:37 UTC (permalink / raw)
To: Peter Hurley
Cc: Greg Kroah-Hartman, jslaby, LKML, Jiri Slaby, Andrey Konovalov,
Kostya Serebryany, Alexander Potapenko, Paul McKenney, Hans Boehm
On Fri, Sep 4, 2015 at 9:34 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
> Hi Dmitry,
>
> On 09/04/2015 03:09 PM, Dmitry Vyukov wrote:
>> 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.
>
> Please submit one patch for each "fix", because it is not possible
> to review what you believe you're fixing.
>
> See below for an example.
>
>> 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 <dvyukov@google.com>
>> ---
>> 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);
>
> schedule_work() is an implied barrier for obvious reasons.
OK, I will split.
To answer this particular question: you need release/write barrier
_before_ the synchronizing store, not _after_. Once the store to
commit happened, another thread can start reading buffer data, this
thread has not yet executed schedule_work at this point.
>> }
>> 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);
>>
>
--
Dmitry Vyukov, Software Engineer, dvyukov@google.com
Google Germany GmbH, Dienerstraße 12, 80331, München
Geschäftsführer: Graham Law, Christine Elizabeth Flores
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat
sind, leiten Sie diese bitte nicht weiter, informieren Sie den
Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank.
This e-mail is confidential. If you are not the right addressee please
do not forward it, please inform the sender, and please erase this
e-mail including any attachments. Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tty: fix data races on tty_buffer.commit
2015-09-04 19:37 ` Dmitry Vyukov
@ 2015-09-04 19:49 ` Peter Hurley
2015-09-04 20:13 ` Dmitry Vyukov
0 siblings, 1 reply; 6+ messages in thread
From: Peter Hurley @ 2015-09-04 19:49 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Greg Kroah-Hartman, jslaby, LKML, Jiri Slaby, Andrey Konovalov,
Kostya Serebryany, Alexander Potapenko, Paul McKenney, Hans Boehm
On 09/04/2015 03:37 PM, Dmitry Vyukov wrote:
> On Fri, Sep 4, 2015 at 9:34 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
>> Hi Dmitry,
>>
>> On 09/04/2015 03:09 PM, Dmitry Vyukov wrote:
>>> 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.
>>
>> Please submit one patch for each "fix", because it is not possible
>> to review what you believe you're fixing.
>>
>> See below for an example.
>>
>>> 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 <dvyukov@google.com>
>>> ---
>>> 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);
>>
>> schedule_work() is an implied barrier for obvious reasons.
>
> OK, I will split.
> To answer this particular question: you need release/write barrier
> _before_ the synchronizing store, not _after_. Once the store to
> commit happened, another thread can start reading buffer data, this
> thread has not yet executed schedule_work at this point.
No.
If the work is already running, a new work will be scheduled, and the
new work will pick up the changed commit index.
If the work is already running /and it happens to see the new commit index/,
it will process the buffer. The new work will start and discover there is
nothing to do.
Regards,
Peter Hurley
PS - You need to base your patches on current mainline. You'll see that I already
converted the smp_rmb()/smp_wmb() of 'next' to load_acquire/store_release. FWIW,
that's not a fix, but a minor optimization. Commit sha 069f38b4983efaea9
>>> }
>>> 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);
>>>
>>
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tty: fix data races on tty_buffer.commit
2015-09-04 19:49 ` Peter Hurley
@ 2015-09-04 20:13 ` Dmitry Vyukov
2015-09-07 12:31 ` Dmitry Vyukov
0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Vyukov @ 2015-09-04 20:13 UTC (permalink / raw)
To: Peter Hurley
Cc: Greg Kroah-Hartman, jslaby, LKML, Jiri Slaby, Andrey Konovalov,
Kostya Serebryany, Alexander Potapenko, Paul McKenney, Hans Boehm
On Fri, Sep 4, 2015 at 9:49 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
> On 09/04/2015 03:37 PM, Dmitry Vyukov wrote:
>> On Fri, Sep 4, 2015 at 9:34 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
>>> Hi Dmitry,
>>>
>>> On 09/04/2015 03:09 PM, Dmitry Vyukov wrote:
>>>> 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.
>>>
>>> Please submit one patch for each "fix", because it is not possible
>>> to review what you believe you're fixing.
>>>
>>> See below for an example.
>>>
>>>> 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 <dvyukov@google.com>
>>>> ---
>>>> 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);
>>>
>>> schedule_work() is an implied barrier for obvious reasons.
>>
>> OK, I will split.
>> To answer this particular question: you need release/write barrier
>> _before_ the synchronizing store, not _after_. Once the store to
>> commit happened, another thread can start reading buffer data, this
>> thread has not yet executed schedule_work at this point.
>
> No.
>
> If the work is already running, a new work will be scheduled, and the
> new work will pick up the changed commit index.
>
> If the work is already running /and it happens to see the new commit index/,
> it will process the buffer.
Problem is with this case /\/\/\
The old work picks up the new commit, but this thread did not execute
release barrier in between storing the data and storing the commit.
Neither work executed acquire barrier between load of commit and load
of data.
> The new work will start and discover there is
> nothing to do.
>
> Regards,
> Peter Hurley
>
> PS - You need to base your patches on current mainline. You'll see that I already
> converted the smp_rmb()/smp_wmb() of 'next' to load_acquire/store_release. FWIW,
> that's not a fix, but a minor optimization. Commit sha 069f38b4983efaea9
Just to make sure, you mean master branch of
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git ?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tty: fix data races on tty_buffer.commit
2015-09-04 20:13 ` Dmitry Vyukov
@ 2015-09-07 12:31 ` Dmitry Vyukov
0 siblings, 0 replies; 6+ messages in thread
From: Dmitry Vyukov @ 2015-09-07 12:31 UTC (permalink / raw)
To: Peter Hurley
Cc: Greg Kroah-Hartman, jslaby, LKML, Jiri Slaby, Andrey Konovalov,
Kostya Serebryany, Alexander Potapenko, Paul McKenney, Hans Boehm
Split this into 2 separate fixes (mailed separately).
Also dropped some READ/WRITE_ONCE. We would like to commit them as
well for the following reasons:
https://github.com/google/ktsan/wiki/READ_ONCE-and-WRITE_ONCE
What do you think about using READ/WRITE_ONCE for all concurrent
accesses to non-constant shared state in tty code?
After discovering some low-hanging data races we will proceed to
suppressing functions with "benign" data races, otherwise it becomes
unmanageable:
https://github.com/google/ktsan/blob/tsan/mm/ktsan/supp.c
Thank you
On Fri, Sep 4, 2015 at 10:13 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Fri, Sep 4, 2015 at 9:49 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
>> On 09/04/2015 03:37 PM, Dmitry Vyukov wrote:
>>> On Fri, Sep 4, 2015 at 9:34 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
>>>> Hi Dmitry,
>>>>
>>>> On 09/04/2015 03:09 PM, Dmitry Vyukov wrote:
>>>>> 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.
>>>>
>>>> Please submit one patch for each "fix", because it is not possible
>>>> to review what you believe you're fixing.
>>>>
>>>> See below for an example.
>>>>
>>>>> 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 <dvyukov@google.com>
>>>>> ---
>>>>> 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);
>>>>
>>>> schedule_work() is an implied barrier for obvious reasons.
>>>
>>> OK, I will split.
>>> To answer this particular question: you need release/write barrier
>>> _before_ the synchronizing store, not _after_. Once the store to
>>> commit happened, another thread can start reading buffer data, this
>>> thread has not yet executed schedule_work at this point.
>>
>> No.
>>
>> If the work is already running, a new work will be scheduled, and the
>> new work will pick up the changed commit index.
>>
>> If the work is already running /and it happens to see the new commit index/,
>> it will process the buffer.
>
> Problem is with this case /\/\/\
> The old work picks up the new commit, but this thread did not execute
> release barrier in between storing the data and storing the commit.
> Neither work executed acquire barrier between load of commit and load
> of data.
>
>
>> The new work will start and discover there is
>> nothing to do.
>>
>> Regards,
>> Peter Hurley
>>
>> PS - You need to base your patches on current mainline. You'll see that I already
>> converted the smp_rmb()/smp_wmb() of 'next' to load_acquire/store_release. FWIW,
>> that's not a fix, but a minor optimization. Commit sha 069f38b4983efaea9
>
> Just to make sure, you mean master branch of
> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git ?
--
Dmitry Vyukov, Software Engineer, dvyukov@google.com
Google Germany GmbH, Dienerstraße 12, 80331, München
Geschäftsführer: Graham Law, Christine Elizabeth Flores
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat
sind, leiten Sie diese bitte nicht weiter, informieren Sie den
Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank.
This e-mail is confidential. If you are not the right addressee please
do not forward it, please inform the sender, and please erase this
e-mail including any attachments. Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-09-07 12:31 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-04 19:09 [PATCH] tty: fix data races on tty_buffer.commit Dmitry Vyukov
2015-09-04 19:34 ` Peter Hurley
2015-09-04 19:37 ` Dmitry Vyukov
2015-09-04 19:49 ` Peter Hurley
2015-09-04 20:13 ` Dmitry Vyukov
2015-09-07 12:31 ` Dmitry Vyukov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).