mbox series

[v3,0/3] char-socket: Fix race condition

Message ID cover.1550842915.git.berto@igalia.com
Headers show
Series char-socket: Fix race condition | expand

Message

Alberto Garcia Feb. 22, 2019, 1:46 p.m. UTC
This fixes a race condition in which the tcp_chr_read() ioc handler
can close a connection that is being written to from another thread.

Note: vhost-user-test still fails if QTEST_VHOST_USER_FIXME is set.

Berto

RFC: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01510.html

v1: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01834.html
- Fixes memory leaks and adds a qemu_idle_add() function

v2: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg06137.html
- Rebased on top of the current master (fc3dbb90f2eb069801bfb4cfe9cbc)
- Patches 1 and 2: Remove the changes in char-pty.c, they're not
                   needed after the rebase.
- Patch 3: Fix conflicts after the rebase.

v3:
- Patch 3: Add tcp_chr_disconnect_locked() [Daniel]

Alberto Garcia (3):
  main-loop: Fix GSource leak in qio_task_thread_worker()
  main-loop: Add qemu_idle_add()
  char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()

 chardev/char-socket.c    | 25 +++++++++++++++++++++----
 include/qemu/main-loop.h | 12 ++++++++++++
 io/task.c                |  9 +++------
 util/main-loop.c         |  9 +++++++++
 4 files changed, 45 insertions(+), 10 deletions(-)

Comments

Alberto Garcia March 5, 2019, 3:52 p.m. UTC | #1
ping

On Fri 22 Feb 2019 02:46:23 PM CET, Alberto Garcia wrote:
> This fixes a race condition in which the tcp_chr_read() ioc handler
> can close a connection that is being written to from another thread.
>
> Note: vhost-user-test still fails if QTEST_VHOST_USER_FIXME is set.
>
> Berto
Marc-André Lureau March 5, 2019, 3:55 p.m. UTC | #2
Hi

On Tue, Mar 5, 2019 at 4:52 PM Alberto Garcia <berto@igalia.com> wrote:
>
> ping

I can queue this in the next chardev PR. This can probably wait until
the softfreeze though. I think Paolo had some plans to look at chardev
issues.

>
> On Fri 22 Feb 2019 02:46:23 PM CET, Alberto Garcia wrote:
> > This fixes a race condition in which the tcp_chr_read() ioc handler
> > can close a connection that is being written to from another thread.
> >
> > Note: vhost-user-test still fails if QTEST_VHOST_USER_FIXME is set.
> >
> > Berto
Paolo Bonzini March 6, 2019, 10:58 a.m. UTC | #3
On 05/03/19 16:55, Marc-André Lureau wrote:
>> ping
> I can queue this in the next chardev PR. This can probably wait until
> the softfreeze though. I think Paolo had some plans to look at chardev
> issues.
> 

Yes, I still do.  This week I'm a bit busy with Linux and babysitting...

Paolo
Alberto Garcia March 31, 2019, 11:04 a.m. UTC | #4
On Tue 05 Mar 2019 04:55:56 PM CET, Marc-André Lureau wrote:
> Hi
>
> On Tue, Mar 5, 2019 at 4:52 PM Alberto Garcia <berto@igalia.com> wrote:
>>
>> ping
>
> I can queue this in the next chardev PR. This can probably wait until
> the softfreeze though. I think Paolo had some plans to look at chardev
> issues.

I don't know what the plans for this are, but I still run into this
crash easily when I run the iotests in v4.0.0-rc1.

Berto
Daniel P. Berrangé April 23, 2019, 4:55 p.m. UTC | #5
ping - paolo/marc-andré - unless I'm missing something, it looks like
this chardev series slipped through the cracks and missed 4.0

On Fri, Feb 22, 2019 at 03:46:23PM +0200, Alberto Garcia wrote:
> This fixes a race condition in which the tcp_chr_read() ioc handler
> can close a connection that is being written to from another thread.
> 
> Note: vhost-user-test still fails if QTEST_VHOST_USER_FIXME is set.
> 
> Berto
> 
> RFC: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01510.html
> 
> v1: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01834.html
> - Fixes memory leaks and adds a qemu_idle_add() function
> 
> v2: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg06137.html
> - Rebased on top of the current master (fc3dbb90f2eb069801bfb4cfe9cbc)
> - Patches 1 and 2: Remove the changes in char-pty.c, they're not
>                    needed after the rebase.
> - Patch 3: Fix conflicts after the rebase.
> 
> v3:
> - Patch 3: Add tcp_chr_disconnect_locked() [Daniel]
> 
> Alberto Garcia (3):
>   main-loop: Fix GSource leak in qio_task_thread_worker()
>   main-loop: Add qemu_idle_add()
>   char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()
> 
>  chardev/char-socket.c    | 25 +++++++++++++++++++++----
>  include/qemu/main-loop.h | 12 ++++++++++++
>  io/task.c                |  9 +++------
>  util/main-loop.c         |  9 +++++++++
>  4 files changed, 45 insertions(+), 10 deletions(-)
> 
> -- 
> 2.11.0
> 

Regards,
Daniel
Paolo Bonzini April 26, 2019, 7:51 a.m. UTC | #6
On 23/04/19 18:55, Daniel P. Berrangé wrote:
> ping - paolo/marc-andré - unless I'm missing something, it looks like
> this chardev series slipped through the cracks and missed 4.0

Yeah, it had a bug unfortunately.  I'm looking at it RSN.

Paolo

> 
> On Fri, Feb 22, 2019 at 03:46:23PM +0200, Alberto Garcia wrote:
>> This fixes a race condition in which the tcp_chr_read() ioc handler
>> can close a connection that is being written to from another thread.
>>
>> Note: vhost-user-test still fails if QTEST_VHOST_USER_FIXME is set.
>>
>> Berto
>>
>> RFC: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01510.html
>>
>> v1: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01834.html
>> - Fixes memory leaks and adds a qemu_idle_add() function
>>
>> v2: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg06137.html
>> - Rebased on top of the current master (fc3dbb90f2eb069801bfb4cfe9cbc)
>> - Patches 1 and 2: Remove the changes in char-pty.c, they're not
>>                    needed after the rebase.
>> - Patch 3: Fix conflicts after the rebase.
>>
>> v3:
>> - Patch 3: Add tcp_chr_disconnect_locked() [Daniel]
>>
>> Alberto Garcia (3):
>>   main-loop: Fix GSource leak in qio_task_thread_worker()
>>   main-loop: Add qemu_idle_add()
>>   char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()
>>
>>  chardev/char-socket.c    | 25 +++++++++++++++++++++----
>>  include/qemu/main-loop.h | 12 ++++++++++++
>>  io/task.c                |  9 +++------
>>  util/main-loop.c         |  9 +++++++++
>>  4 files changed, 45 insertions(+), 10 deletions(-)
>>
>> -- 
>> 2.11.0
>>
> 
> Regards,
> Daniel
>
Max Reitz June 5, 2019, 7:36 p.m. UTC | #7
On 26.04.19 09:51, Paolo Bonzini wrote:
> On 23/04/19 18:55, Daniel P. Berrangé wrote:
>> ping - paolo/marc-andré - unless I'm missing something, it looks like
>> this chardev series slipped through the cracks and missed 4.0
> 
> Yeah, it had a bug unfortunately.  I'm looking at it RSN.

I’ll just leave another ping here

> Paolo
> 
>>
>> On Fri, Feb 22, 2019 at 03:46:23PM +0200, Alberto Garcia wrote:
>>> This fixes a race condition in which the tcp_chr_read() ioc handler
>>> can close a connection that is being written to from another thread.
>>>
>>> Note: vhost-user-test still fails if QTEST_VHOST_USER_FIXME is set.
>>>
>>> Berto
>>>
>>> RFC: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01510.html
>>>
>>> v1: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01834.html
>>> - Fixes memory leaks and adds a qemu_idle_add() function
>>>
>>> v2: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg06137.html
>>> - Rebased on top of the current master (fc3dbb90f2eb069801bfb4cfe9cbc)
>>> - Patches 1 and 2: Remove the changes in char-pty.c, they're not
>>>                    needed after the rebase.
>>> - Patch 3: Fix conflicts after the rebase.
>>>
>>> v3:
>>> - Patch 3: Add tcp_chr_disconnect_locked() [Daniel]
>>>
>>> Alberto Garcia (3):
>>>   main-loop: Fix GSource leak in qio_task_thread_worker()
>>>   main-loop: Add qemu_idle_add()
>>>   char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()
>>>
>>>  chardev/char-socket.c    | 25 +++++++++++++++++++++----
>>>  include/qemu/main-loop.h | 12 ++++++++++++
>>>  io/task.c                |  9 +++------
>>>  util/main-loop.c         |  9 +++++++++
>>>  4 files changed, 45 insertions(+), 10 deletions(-)
>>>
>>> -- 
>>> 2.11.0
>>>
>>
>> Regards,
>> Daniel
>>
> 
> 
>
Max Reitz July 3, 2019, 5:51 p.m. UTC | #8
On 05.06.19 21:36, Max Reitz wrote:
> On 26.04.19 09:51, Paolo Bonzini wrote:
>> On 23/04/19 18:55, Daniel P. Berrangé wrote:
>>> ping - paolo/marc-andré - unless I'm missing something, it looks like
>>> this chardev series slipped through the cracks and missed 4.0
>>
>> Yeah, it had a bug unfortunately.  I'm looking at it RSN.
> 
> I’ll just leave another ping here

And another one.

Max

>> Paolo
>>
>>>
>>> On Fri, Feb 22, 2019 at 03:46:23PM +0200, Alberto Garcia wrote:
>>>> This fixes a race condition in which the tcp_chr_read() ioc handler
>>>> can close a connection that is being written to from another thread.
>>>>
>>>> Note: vhost-user-test still fails if QTEST_VHOST_USER_FIXME is set.
>>>>
>>>> Berto
>>>>
>>>> RFC: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01510.html
>>>>
>>>> v1: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01834.html
>>>> - Fixes memory leaks and adds a qemu_idle_add() function
>>>>
>>>> v2: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg06137.html
>>>> - Rebased on top of the current master (fc3dbb90f2eb069801bfb4cfe9cbc)
>>>> - Patches 1 and 2: Remove the changes in char-pty.c, they're not
>>>>                    needed after the rebase.
>>>> - Patch 3: Fix conflicts after the rebase.
>>>>
>>>> v3:
>>>> - Patch 3: Add tcp_chr_disconnect_locked() [Daniel]
>>>>
>>>> Alberto Garcia (3):
>>>>   main-loop: Fix GSource leak in qio_task_thread_worker()
>>>>   main-loop: Add qemu_idle_add()
>>>>   char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()
>>>>
>>>>  chardev/char-socket.c    | 25 +++++++++++++++++++++----
>>>>  include/qemu/main-loop.h | 12 ++++++++++++
>>>>  io/task.c                |  9 +++------
>>>>  util/main-loop.c         |  9 +++++++++
>>>>  4 files changed, 45 insertions(+), 10 deletions(-)
>>>>
>>>> -- 
>>>> 2.11.0
>>>>
>>>
>>> Regards,
>>> Daniel
>>>
>>
>>
>>
> 
>
Paolo Bonzini July 4, 2019, 9:59 a.m. UTC | #9
On 03/07/19 19:51, Max Reitz wrote:
> On 05.06.19 21:36, Max Reitz wrote:
>> On 26.04.19 09:51, Paolo Bonzini wrote:
>>> On 23/04/19 18:55, Daniel P. Berrangé wrote:
>>>> ping - paolo/marc-andré - unless I'm missing something, it looks like
>>>> this chardev series slipped through the cracks and missed 4.0
>>>
>>> Yeah, it had a bug unfortunately.  I'm looking at it RSN.
>>
>> I’ll just leave another ping here

I haven't forgotten. :(

Paolo
Andrey Shinkevich July 16, 2019, 12:55 p.m. UTC | #10
On 22/02/2019 16:46, Alberto Garcia wrote:
> This fixes a race condition in which the tcp_chr_read() ioc handler
> can close a connection that is being written to from another thread.
> 
> Note: vhost-user-test still fails if QTEST_VHOST_USER_FIXME is set.
> 
> Berto
> 
> RFC: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01510.html
> 
> v1: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01834.html
> - Fixes memory leaks and adds a qemu_idle_add() function
> 
> v2: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg06137.html
> - Rebased on top of the current master (fc3dbb90f2eb069801bfb4cfe9cbc)
> - Patches 1 and 2: Remove the changes in char-pty.c, they're not
>                     needed after the rebase.
> - Patch 3: Fix conflicts after the rebase.
> 
> v3:
> - Patch 3: Add tcp_chr_disconnect_locked() [Daniel]
> 
> Alberto Garcia (3):
>    main-loop: Fix GSource leak in qio_task_thread_worker()
>    main-loop: Add qemu_idle_add()
>    char-socket: Lock tcp_chr_disconnect() and socket_reconnect_timeout()
> 
>   chardev/char-socket.c    | 25 +++++++++++++++++++++----
>   include/qemu/main-loop.h | 12 ++++++++++++
>   io/task.c                |  9 +++------
>   util/main-loop.c         |  9 +++++++++
>   4 files changed, 45 insertions(+), 10 deletions(-)
> 

Please add me to cc.
I confirm that check-unit: tests/test-char does not pass.

Andrey