Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1217063 > unrolled thread

[PATCH] tty: fix data race in flush_to_ldisc

Started byDmitry Vyukov <dvyukov@google.com>
First post2015-09-01 22:20 +0200
Last post2015-09-02 20:00 +0200
Articles 8 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] tty: fix data race in flush_to_ldisc Dmitry Vyukov <dvyukov@google.com> - 2015-09-01 22:20 +0200
    Re: [PATCH] tty: fix data race in flush_to_ldisc Peter Hurley <peter@hurleysoftware.com> - 2015-09-02 16:20 +0200
      Re: [PATCH] tty: fix data race in flush_to_ldisc Dmitry Vyukov <dvyukov@google.com> - 2015-09-02 16:50 +0200
        Re: [PATCH] tty: fix data race in flush_to_ldisc Peter Hurley <peter@hurleysoftware.com> - 2015-09-02 17:30 +0200
          [PATCH] tty: fix data race in flush_to_ldisc Dmitry Vyukov <dvyukov@google.com> - 2015-09-02 20:00 +0200
            Re: [PATCH] tty: fix data race in flush_to_ldisc Peter Hurley <peter@hurleysoftware.com> - 2015-09-03 03:00 +0200
              Re: [PATCH] tty: fix data race in flush_to_ldisc Dmitry Vyukov <dvyukov@google.com> - 2015-09-04 21:30 +0200
          Re: [PATCH] tty: fix data race in flush_to_ldisc Dmitry Vyukov <dvyukov@google.com> - 2015-09-02 20:00 +0200

#1217063 — [PATCH] tty: fix data race in flush_to_ldisc

FromDmitry Vyukov <dvyukov@google.com>
Date2015-09-01 22:20 +0200
Subject[PATCH] tty: fix data race in flush_to_ldisc
Message-ID<q40ca-7Z-23@gated-at.bofh.it>
The data race is found with KernelThreadSanitizer (on rev 21bdb584af8c):

ThreadSanitizer: data-race in release_tty
Write of size 8 by thread T325 (K2579):
  release_tty+0xf3/0x1c0 drivers/tty/tty_io.c:1688
  tty_release+0x698/0x7c0 drivers/tty/tty_io.c:1920
  __fput+0x15f/0x310 fs/file_table.c:207
  ____fput+0x1d/0x30 fs/file_table.c:243
  task_work_run+0x115/0x130 kernel/task_work.c:123
  do_notify_resume+0x73/0x80
    tracehook_notify_resume include/linux/tracehook.h:190
  do_notify_resume+0x73/0x80 arch/x86/kernel/signal.c:757
  int_signal+0x12/0x17 arch/x86/entry/entry_64.S:326
Previous read of size 8 by thread T19 (K16):
  flush_to_ldisc+0x29/0x300 drivers/tty/tty_buffer.c:472
  process_one_work+0x47e/0x930 kernel/workqueue.c:2036
  worker_thread+0xb0/0x900 kernel/workqueue.c:2170
  kthread+0x150/0x170 kernel/kthread.c:207

flush_to_ldisc reads port->itty and checks that it is not NULL,
concurrently release_tty sets port->itty to NULL. It is possible
that flush_to_ldisc loads port->itty once, ensures that it is
not NULL, but then reloads it again and uses. The second load
can already return NULL, which will cause a crash.

Set port->itty to NULL after shutting down flush_to_ldisc work.

Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
---
 drivers/tty/tty_io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 57fc6ee..0df24c1 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1684,9 +1684,9 @@ static void release_tty(struct tty_struct *tty, int idx)
 	tty_free_termios(tty);
 	tty_driver_remove_tty(tty->driver, tty);
 	tty->port->itty = NULL;
+	cancel_work_sync(&tty->port->buf.work);
 	if (tty->link)
 		tty->link->port->itty = NULL;
-	cancel_work_sync(&tty->port->buf.work);
 
 	tty_kref_put(tty->link);
 	tty_kref_put(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]

#1217627

FromPeter Hurley <peter@hurleysoftware.com>
Date2015-09-02 16:20 +0200
Message-ID<q4h3j-7tc-5@gated-at.bofh.it>
In reply to#1217063
On 09/01/2015 04:11 PM, Dmitry Vyukov wrote:
> The data race is found with KernelThreadSanitizer (on rev 21bdb584af8c):
> 
> ThreadSanitizer: data-race in release_tty
> Write of size 8 by thread T325 (K2579):
>   release_tty+0xf3/0x1c0 drivers/tty/tty_io.c:1688
>   tty_release+0x698/0x7c0 drivers/tty/tty_io.c:1920
>   __fput+0x15f/0x310 fs/file_table.c:207
>   ____fput+0x1d/0x30 fs/file_table.c:243
>   task_work_run+0x115/0x130 kernel/task_work.c:123
>   do_notify_resume+0x73/0x80
>     tracehook_notify_resume include/linux/tracehook.h:190
>   do_notify_resume+0x73/0x80 arch/x86/kernel/signal.c:757
>   int_signal+0x12/0x17 arch/x86/entry/entry_64.S:326
> Previous read of size 8 by thread T19 (K16):
>   flush_to_ldisc+0x29/0x300 drivers/tty/tty_buffer.c:472
>   process_one_work+0x47e/0x930 kernel/workqueue.c:2036
>   worker_thread+0xb0/0x900 kernel/workqueue.c:2170
>   kthread+0x150/0x170 kernel/kthread.c:207
> 
> flush_to_ldisc reads port->itty and checks that it is not NULL,
> concurrently release_tty sets port->itty to NULL. It is possible
> that flush_to_ldisc loads port->itty once, ensures that it is
> not NULL, but then reloads it again and uses. The second load
> can already return NULL, which will cause a crash.
> 
> Set port->itty to NULL after shutting down flush_to_ldisc work.
> 
> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
> ---
>  drivers/tty/tty_io.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 57fc6ee..0df24c1 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -1684,9 +1684,9 @@ static void release_tty(struct tty_struct *tty, int idx)
>  	tty_free_termios(tty);
>  	tty_driver_remove_tty(tty->driver, tty);
>  	tty->port->itty = NULL;
> +	cancel_work_sync(&tty->port->buf.work);
>  	if (tty->link)
>  		tty->link->port->itty = NULL;
> -	cancel_work_sync(&tty->port->buf.work);

This patch doesn't do anything.

Regards,
Peter Hurley

>  
>  	tty_kref_put(tty->link);
>  	tty_kref_put(tty);
> 

--
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]

#1217674

FromDmitry Vyukov <dvyukov@google.com>
Date2015-09-02 16:50 +0200
Message-ID<q4hwn-81p-29@gated-at.bofh.it>
In reply to#1217627
On Wed, Sep 2, 2015 at 4:18 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
> On 09/01/2015 04:11 PM, Dmitry Vyukov wrote:
>> The data race is found with KernelThreadSanitizer (on rev 21bdb584af8c):
>>
>> ThreadSanitizer: data-race in release_tty
>> Write of size 8 by thread T325 (K2579):
>>   release_tty+0xf3/0x1c0 drivers/tty/tty_io.c:1688
>>   tty_release+0x698/0x7c0 drivers/tty/tty_io.c:1920
>>   __fput+0x15f/0x310 fs/file_table.c:207
>>   ____fput+0x1d/0x30 fs/file_table.c:243
>>   task_work_run+0x115/0x130 kernel/task_work.c:123
>>   do_notify_resume+0x73/0x80
>>     tracehook_notify_resume include/linux/tracehook.h:190
>>   do_notify_resume+0x73/0x80 arch/x86/kernel/signal.c:757
>>   int_signal+0x12/0x17 arch/x86/entry/entry_64.S:326
>> Previous read of size 8 by thread T19 (K16):
>>   flush_to_ldisc+0x29/0x300 drivers/tty/tty_buffer.c:472
>>   process_one_work+0x47e/0x930 kernel/workqueue.c:2036
>>   worker_thread+0xb0/0x900 kernel/workqueue.c:2170
>>   kthread+0x150/0x170 kernel/kthread.c:207
>>
>> flush_to_ldisc reads port->itty and checks that it is not NULL,
>> concurrently release_tty sets port->itty to NULL. It is possible
>> that flush_to_ldisc loads port->itty once, ensures that it is
>> not NULL, but then reloads it again and uses. The second load
>> can already return NULL, which will cause a crash.
>>
>> Set port->itty to NULL after shutting down flush_to_ldisc work.
>>
>> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
>> ---
>>  drivers/tty/tty_io.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
>> index 57fc6ee..0df24c1 100644
>> --- a/drivers/tty/tty_io.c
>> +++ b/drivers/tty/tty_io.c
>> @@ -1684,9 +1684,9 @@ static void release_tty(struct tty_struct *tty, int idx)
>>       tty_free_termios(tty);
>>       tty_driver_remove_tty(tty->driver, tty);
>>       tty->port->itty = NULL;
>> +     cancel_work_sync(&tty->port->buf.work);
>>       if (tty->link)
>>               tty->link->port->itty = NULL;
>> -     cancel_work_sync(&tty->port->buf.work);
>
> This patch doesn't do anything.

It sets tty->link->port->itty to NULL _after_ waiting for
flush_to_ldisc work to finish. At least this is the intention.
Can you explain why it does not do anything?

Thank you.
--
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]

#1217696

FromPeter Hurley <peter@hurleysoftware.com>
Date2015-09-02 17:30 +0200
Message-ID<q4i94-yB-9@gated-at.bofh.it>
In reply to#1217674
On 09/02/2015 10:41 AM, Dmitry Vyukov wrote:
> On Wed, Sep 2, 2015 at 4:18 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
>> On 09/01/2015 04:11 PM, Dmitry Vyukov wrote:
>>> The data race is found with KernelThreadSanitizer (on rev 21bdb584af8c):
>>>
>>> ThreadSanitizer: data-race in release_tty
>>> Write of size 8 by thread T325 (K2579):
>>>   release_tty+0xf3/0x1c0 drivers/tty/tty_io.c:1688
>>>   tty_release+0x698/0x7c0 drivers/tty/tty_io.c:1920
>>>   __fput+0x15f/0x310 fs/file_table.c:207
>>>   ____fput+0x1d/0x30 fs/file_table.c:243
>>>   task_work_run+0x115/0x130 kernel/task_work.c:123
>>>   do_notify_resume+0x73/0x80
>>>     tracehook_notify_resume include/linux/tracehook.h:190
>>>   do_notify_resume+0x73/0x80 arch/x86/kernel/signal.c:757
>>>   int_signal+0x12/0x17 arch/x86/entry/entry_64.S:326
>>> Previous read of size 8 by thread T19 (K16):
>>>   flush_to_ldisc+0x29/0x300 drivers/tty/tty_buffer.c:472
>>>   process_one_work+0x47e/0x930 kernel/workqueue.c:2036
>>>   worker_thread+0xb0/0x900 kernel/workqueue.c:2170
>>>   kthread+0x150/0x170 kernel/kthread.c:207
>>>
>>> flush_to_ldisc reads port->itty and checks that it is not NULL,
>>> concurrently release_tty sets port->itty to NULL. It is possible
>>> that flush_to_ldisc loads port->itty once, ensures that it is
>>> not NULL, but then reloads it again and uses. The second load
>>> can already return NULL, which will cause a crash.
>>>
>>> Set port->itty to NULL after shutting down flush_to_ldisc work.
>>>
>>> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
>>> ---
>>>  drivers/tty/tty_io.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
>>> index 57fc6ee..0df24c1 100644
>>> --- a/drivers/tty/tty_io.c
>>> +++ b/drivers/tty/tty_io.c
>>> @@ -1684,9 +1684,9 @@ static void release_tty(struct tty_struct *tty, int idx)
>>>       tty_free_termios(tty);
>>>       tty_driver_remove_tty(tty->driver, tty);
>>>       tty->port->itty = NULL;
>>> +     cancel_work_sync(&tty->port->buf.work);
>>>       if (tty->link)
>>>               tty->link->port->itty = NULL;
>>> -     cancel_work_sync(&tty->port->buf.work);
>>
>> This patch doesn't do anything.
> 
> It sets tty->link->port->itty to NULL _after_ waiting for
> flush_to_ldisc work to finish. At least this is the intention.
> Can you explain why it does not do anything?

Because tty->link is a different tty than that corresponding
to the buffer work being cancelled.

Regards,
Peter Hurley

--
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]

#1217778

FromDmitry Vyukov <dvyukov@google.com>
Date2015-09-02 20:00 +0200
Message-ID<q4kue-3Oz-21@gated-at.bofh.it>
In reply to#1217696
The data race is found with KernelThreadSanitizer (on rev 21bdb584af8c):

ThreadSanitizer: data-race in release_tty
Write of size 8 by thread T325 (K2579):
  release_tty+0xf3/0x1c0 drivers/tty/tty_io.c:1688
  tty_release+0x698/0x7c0 drivers/tty/tty_io.c:1920
  __fput+0x15f/0x310 fs/file_table.c:207
  ____fput+0x1d/0x30 fs/file_table.c:243
  task_work_run+0x115/0x130 kernel/task_work.c:123
  do_notify_resume+0x73/0x80
    tracehook_notify_resume include/linux/tracehook.h:190
  do_notify_resume+0x73/0x80 arch/x86/kernel/signal.c:757
  int_signal+0x12/0x17 arch/x86/entry/entry_64.S:326
Previous read of size 8 by thread T19 (K16):
  flush_to_ldisc+0x29/0x300 drivers/tty/tty_buffer.c:472
  process_one_work+0x47e/0x930 kernel/workqueue.c:2036
  worker_thread+0xb0/0x900 kernel/workqueue.c:2170
  kthread+0x150/0x170 kernel/kthread.c:207

flush_to_ldisc reads port->itty and checks that it is not NULL,
concurrently release_tty sets port->itty to NULL. It is possible
that flush_to_ldisc loads port->itty once, ensures that it is
not NULL, but then reloads it again and uses. The second load
can already return NULL, which will cause a crash.

Use READ_ONCE/WRITE_ONCE to read/update port->itty.

Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
---
 drivers/tty/tty_buffer.c | 2 +-
 drivers/tty/tty_io.c     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 4cf263d..1f1031d 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -469,7 +469,7 @@ static void flush_to_ldisc(struct work_struct *work)
 	struct tty_struct *tty;
 	struct tty_ldisc *disc;
 
-	tty = port->itty;
+	tty = READ_ONCE(port->itty);
 	if (tty == NULL)
 		return;
 
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 57fc6ee..aad47df 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1685,7 +1685,7 @@ static void release_tty(struct tty_struct *tty, int idx)
 	tty_driver_remove_tty(tty->driver, tty);
 	tty->port->itty = NULL;
 	if (tty->link)
-		tty->link->port->itty = NULL;
+		WRITE_ONCE(tty->link->port->itty, NULL);
 	cancel_work_sync(&tty->port->buf.work);
 
 	tty_kref_put(tty->link);
-- 
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]

#1217974

FromPeter Hurley <peter@hurleysoftware.com>
Date2015-09-03 03:00 +0200
Message-ID<q4r2F-4Ip-3@gated-at.bofh.it>
In reply to#1217778
On 09/02/2015 01:53 PM, Dmitry Vyukov wrote:
> The data race is found with KernelThreadSanitizer (on rev 21bdb584af8c):
> 
> ThreadSanitizer: data-race in release_tty
> Write of size 8 by thread T325 (K2579):
>   release_tty+0xf3/0x1c0 drivers/tty/tty_io.c:1688
>   tty_release+0x698/0x7c0 drivers/tty/tty_io.c:1920
>   __fput+0x15f/0x310 fs/file_table.c:207
>   ____fput+0x1d/0x30 fs/file_table.c:243
>   task_work_run+0x115/0x130 kernel/task_work.c:123
>   do_notify_resume+0x73/0x80
>     tracehook_notify_resume include/linux/tracehook.h:190
>   do_notify_resume+0x73/0x80 arch/x86/kernel/signal.c:757
>   int_signal+0x12/0x17 arch/x86/entry/entry_64.S:326
> Previous read of size 8 by thread T19 (K16):
>   flush_to_ldisc+0x29/0x300 drivers/tty/tty_buffer.c:472
>   process_one_work+0x47e/0x930 kernel/workqueue.c:2036
>   worker_thread+0xb0/0x900 kernel/workqueue.c:2170
>   kthread+0x150/0x170 kernel/kthread.c:207

The stack traces are not really helpful in describing how the race
occurs; I would leave it out of the changelog.


> flush_to_ldisc reads port->itty and checks that it is not NULL,
> concurrently release_tty sets port->itty to NULL. It is possible
> that flush_to_ldisc loads port->itty once, ensures that it is
> not NULL, but then reloads it again and uses. The second load
> can already return NULL, which will cause a crash.
> 
> Use READ_ONCE/WRITE_ONCE to read/update port->itty.

See below.

> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
> ---
>  drivers/tty/tty_buffer.c | 2 +-
>  drivers/tty/tty_io.c     | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
> index 4cf263d..1f1031d 100644
> --- a/drivers/tty/tty_buffer.c
> +++ b/drivers/tty/tty_buffer.c
> @@ -469,7 +469,7 @@ static void flush_to_ldisc(struct work_struct *work)
>  	struct tty_struct *tty;
>  	struct tty_ldisc *disc;
>  
> -	tty = port->itty;
> +	tty = READ_ONCE(port->itty);

This is fine.

>  	if (tty == NULL)
>  		return;
>  
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 57fc6ee..aad47df 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -1685,7 +1685,7 @@ static void release_tty(struct tty_struct *tty, int idx)
>  	tty_driver_remove_tty(tty->driver, tty);
>  	tty->port->itty = NULL;
>  	if (tty->link)
> -		tty->link->port->itty = NULL;
> +		WRITE_ONCE(tty->link->port->itty, NULL);

This isn't doing anything useful.

1. The compiler can't push the store past the cancel_work_sync() (because the
   compiler has no visibility into cancel_work_sync()), and,
2. There's no effect if the compiler hoists the store higher in the release_tty()
   because the line discipline has already been closed and killed (so the
   tty_ldisc_ref() in flush_to_ldisc() returns NULL anyway).


Regards,
Peter Hurley

>  	cancel_work_sync(&tty->port->buf.work);
>  
>  	tty_kref_put(tty->link);
> 

--
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]

#1219264

FromDmitry Vyukov <dvyukov@google.com>
Date2015-09-04 21:30 +0200
Message-ID<q54Qq-2UZ-9@gated-at.bofh.it>
In reply to#1217974
On Thu, Sep 3, 2015 at 2:50 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
> On 09/02/2015 01:53 PM, Dmitry Vyukov wrote:
>> The data race is found with KernelThreadSanitizer (on rev 21bdb584af8c):
>>
>> ThreadSanitizer: data-race in release_tty
>> Write of size 8 by thread T325 (K2579):
>>   release_tty+0xf3/0x1c0 drivers/tty/tty_io.c:1688
>>   tty_release+0x698/0x7c0 drivers/tty/tty_io.c:1920
>>   __fput+0x15f/0x310 fs/file_table.c:207
>>   ____fput+0x1d/0x30 fs/file_table.c:243
>>   task_work_run+0x115/0x130 kernel/task_work.c:123
>>   do_notify_resume+0x73/0x80
>>     tracehook_notify_resume include/linux/tracehook.h:190
>>   do_notify_resume+0x73/0x80 arch/x86/kernel/signal.c:757
>>   int_signal+0x12/0x17 arch/x86/entry/entry_64.S:326
>> Previous read of size 8 by thread T19 (K16):
>>   flush_to_ldisc+0x29/0x300 drivers/tty/tty_buffer.c:472
>>   process_one_work+0x47e/0x930 kernel/workqueue.c:2036
>>   worker_thread+0xb0/0x900 kernel/workqueue.c:2170
>>   kthread+0x150/0x170 kernel/kthread.c:207
>
> The stack traces are not really helpful in describing how the race
> occurs; I would leave it out of the changelog.

ok

>> flush_to_ldisc reads port->itty and checks that it is not NULL,
>> concurrently release_tty sets port->itty to NULL. It is possible
>> that flush_to_ldisc loads port->itty once, ensures that it is
>> not NULL, but then reloads it again and uses. The second load
>> can already return NULL, which will cause a crash.
>>
>> Use READ_ONCE/WRITE_ONCE to read/update port->itty.
>
> See below.
>
>> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
>> ---
>>  drivers/tty/tty_buffer.c | 2 +-
>>  drivers/tty/tty_io.c     | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
>> index 4cf263d..1f1031d 100644
>> --- a/drivers/tty/tty_buffer.c
>> +++ b/drivers/tty/tty_buffer.c
>> @@ -469,7 +469,7 @@ static void flush_to_ldisc(struct work_struct *work)
>>       struct tty_struct *tty;
>>       struct tty_ldisc *disc;
>>
>> -     tty = port->itty;
>> +     tty = READ_ONCE(port->itty);
>
> This is fine.
>
>>       if (tty == NULL)
>>               return;
>>
>> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
>> index 57fc6ee..aad47df 100644
>> --- a/drivers/tty/tty_io.c
>> +++ b/drivers/tty/tty_io.c
>> @@ -1685,7 +1685,7 @@ static void release_tty(struct tty_struct *tty, int idx)
>>       tty_driver_remove_tty(tty->driver, tty);
>>       tty->port->itty = NULL;
>>       if (tty->link)
>> -             tty->link->port->itty = NULL;
>> +             WRITE_ONCE(tty->link->port->itty, NULL);
>
> This isn't doing anything useful.
>
> 1. The compiler can't push the store past the cancel_work_sync() (because the
>    compiler has no visibility into cancel_work_sync()), and,
> 2. There's no effect if the compiler hoists the store higher in the release_tty()
>    because the line discipline has already been closed and killed (so the
>    tty_ldisc_ref() in flush_to_ldisc() returns NULL anyway).

OK, let me do one try at convincing you that WRITE_ONCE here is a good
idea. If you are not convinced then I will remove it.

WRITE_ONCE/READ_ONCE for all shared memory accesses:
1. Make the code more readable but highlighting important aspects.
2. Required by relevant standards and relieve you, me and everybody
else reading this code from spending time on proving that it cannot
break (think of multi-file compilation mode, store tearing which
compilers indeed known to do in some contexts, and compiler
transformations that we don't know of).
3. Allow tooling that finds undoubtedly harmful bugs, like this one,
or in fact, I've just mailed another fix for missed memory barriers in
this file (also found by KTSAN).

I've described these aspects in more detail here:
https://github.com/google/ktsan/wiki/READ_ONCE-and-WRITE_ONCE

I don't see any negative aspects to it. Do you see any? Because if you
see at least some value in at least on these points and don't see any
negative aspects, then it is worth doing.


-- 
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.
--
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]

#1217779

FromDmitry Vyukov <dvyukov@google.com>
Date2015-09-02 20:00 +0200
Message-ID<q4kuf-3Oz-27@gated-at.bofh.it>
In reply to#1217696
On Wed, Sep 2, 2015 at 5:28 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
> On 09/02/2015 10:41 AM, Dmitry Vyukov wrote:
>> On Wed, Sep 2, 2015 at 4:18 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
>>> On 09/01/2015 04:11 PM, Dmitry Vyukov wrote:
>>>> The data race is found with KernelThreadSanitizer (on rev 21bdb584af8c):
>>>>
>>>> ThreadSanitizer: data-race in release_tty
>>>> Write of size 8 by thread T325 (K2579):
>>>>   release_tty+0xf3/0x1c0 drivers/tty/tty_io.c:1688
>>>>   tty_release+0x698/0x7c0 drivers/tty/tty_io.c:1920
>>>>   __fput+0x15f/0x310 fs/file_table.c:207
>>>>   ____fput+0x1d/0x30 fs/file_table.c:243
>>>>   task_work_run+0x115/0x130 kernel/task_work.c:123
>>>>   do_notify_resume+0x73/0x80
>>>>     tracehook_notify_resume include/linux/tracehook.h:190
>>>>   do_notify_resume+0x73/0x80 arch/x86/kernel/signal.c:757
>>>>   int_signal+0x12/0x17 arch/x86/entry/entry_64.S:326
>>>> Previous read of size 8 by thread T19 (K16):
>>>>   flush_to_ldisc+0x29/0x300 drivers/tty/tty_buffer.c:472
>>>>   process_one_work+0x47e/0x930 kernel/workqueue.c:2036
>>>>   worker_thread+0xb0/0x900 kernel/workqueue.c:2170
>>>>   kthread+0x150/0x170 kernel/kthread.c:207
>>>>
>>>> flush_to_ldisc reads port->itty and checks that it is not NULL,
>>>> concurrently release_tty sets port->itty to NULL. It is possible
>>>> that flush_to_ldisc loads port->itty once, ensures that it is
>>>> not NULL, but then reloads it again and uses. The second load
>>>> can already return NULL, which will cause a crash.
>>>>
>>>> Set port->itty to NULL after shutting down flush_to_ldisc work.
>>>>
>>>> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
>>>> ---
>>>>  drivers/tty/tty_io.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
>>>> index 57fc6ee..0df24c1 100644
>>>> --- a/drivers/tty/tty_io.c
>>>> +++ b/drivers/tty/tty_io.c
>>>> @@ -1684,9 +1684,9 @@ static void release_tty(struct tty_struct *tty, int idx)
>>>>       tty_free_termios(tty);
>>>>       tty_driver_remove_tty(tty->driver, tty);
>>>>       tty->port->itty = NULL;
>>>> +     cancel_work_sync(&tty->port->buf.work);
>>>>       if (tty->link)
>>>>               tty->link->port->itty = NULL;
>>>> -     cancel_work_sync(&tty->port->buf.work);
>>>
>>> This patch doesn't do anything.
>>
>> It sets tty->link->port->itty to NULL _after_ waiting for
>> flush_to_ldisc work to finish. At least this is the intention.
>> Can you explain why it does not do anything?
>
> Because tty->link is a different tty than that corresponding
> to the buffer work being cancelled.

Got it.
Please take a look at the updated version of the patch.
--
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