[Qemu-devel] [PATCH] net: cleanup before character devices

[Qemu-devel] [PATCH] net: cleanup before character devices

[Paolo Bonzini]

Otherwise, vhost-user causes a use-after-free.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 vl.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/vl.c b/vl.c
index d0b9ff9..005162d 100644
--- a/vl.c
+++ b/vl.c
@@ -4327,9 +4327,6 @@ int main(int argc, char **argv, char **envp)
         qemu_opts_del(icount_opts);
     }
 
-    /* clean up network at qemu process termination */
-    atexit(&net_cleanup);
-
     if (default_net) {
         QemuOptsList *net = qemu_find_opts("net");
         qemu_opts_set(net, NULL, "type", "nic", &error_abort);
@@ -4596,6 +4593,9 @@ int main(int argc, char **argv, char **envp)
 #ifdef CONFIG_TPM
     tpm_cleanup();
 #endif
+
+    /* vhost-user must be cleaned up before chardevs.  */
+    net_cleanup();
     qemu_chr_cleanup();
 
     return 0;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] net: cleanup before character devices

[Marc-André Lureau]

Hi

On Fri, Jul 8, 2016 at 5:27 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Otherwise, vhost-user causes a use-after-free.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Sorry I didn't see the criticals in make check after my patch...
Thanks for fixing it this quickly

> ---
>  vl.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index d0b9ff9..005162d 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4327,9 +4327,6 @@ int main(int argc, char **argv, char **envp)
>          qemu_opts_del(icount_opts);
>      }
>
> -    /* clean up network at qemu process termination */
> -    atexit(&net_cleanup);
> -
>      if (default_net) {
>          QemuOptsList *net = qemu_find_opts("net");
>          qemu_opts_set(net, NULL, "type", "nic", &error_abort);
> @@ -4596,6 +4593,9 @@ int main(int argc, char **argv, char **envp)
>  #ifdef CONFIG_TPM
>      tpm_cleanup();
>  #endif
> +
> +    /* vhost-user must be cleaned up before chardevs.  */
> +    net_cleanup();

Alternatively, if it's better to keep the atexit(net_cleanup), we
could have atexit(qemu_chr_cleanup) here, so it's only called on
normal exit and after net_cleanup.


Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


>      qemu_chr_cleanup();
>
>      return 0;
> --
> 1.8.3.1
>
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] net: cleanup before character devices

[Paolo Bonzini]

On 08/07/2016 17:42, Marc-André Lureau wrote:
> Alternatively, if it's better to keep the atexit(net_cleanup), we
> could have atexit(qemu_chr_cleanup) here, so it's only called on
> normal exit and after net_cleanup.

No, atexit functions are called in LIFO order (which actually makes
sense if you think about it :)).

Paolo

Re: [Qemu-devel] [PATCH] net: cleanup before character devices

[Jason Wang]

On 2016年07月08日 23:27, Paolo Bonzini wrote:
> Otherwise, vhost-user causes a use-after-free.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   vl.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index d0b9ff9..005162d 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4327,9 +4327,6 @@ int main(int argc, char **argv, char **envp)
>           qemu_opts_del(icount_opts);
>       }
>   
> -    /* clean up network at qemu process termination */
> -    atexit(&net_cleanup);
> -
>       if (default_net) {
>           QemuOptsList *net = qemu_find_opts("net");
>           qemu_opts_set(net, NULL, "type", "nic", &error_abort);
> @@ -4596,6 +4593,9 @@ int main(int argc, char **argv, char **envp)
>   #ifdef CONFIG_TPM
>       tpm_cleanup();
>   #endif
> +
> +    /* vhost-user must be cleaned up before chardevs.  */
> +    net_cleanup();
>       qemu_chr_cleanup();
>   
>       return 0;

Patch does not seems to be applied on master. Which tree is this patch 
based?

Thanks

Re: [Qemu-devel] [PATCH] net: cleanup before character devices

[Paolo Bonzini]

On 11/07/2016 11:06, Jason Wang wrote:
> 
> 
> On 2016年07月08日 23:27, Paolo Bonzini wrote:
>> Otherwise, vhost-user causes a use-after-free.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   vl.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/vl.c b/vl.c
>> index d0b9ff9..005162d 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -4327,9 +4327,6 @@ int main(int argc, char **argv, char **envp)
>>           qemu_opts_del(icount_opts);
>>       }
>>   -    /* clean up network at qemu process termination */
>> -    atexit(&net_cleanup);
>> -
>>       if (default_net) {
>>           QemuOptsList *net = qemu_find_opts("net");
>>           qemu_opts_set(net, NULL, "type", "nic", &error_abort);
>> @@ -4596,6 +4593,9 @@ int main(int argc, char **argv, char **envp)
>>   #ifdef CONFIG_TPM
>>       tpm_cleanup();
>>   #endif
>> +
>> +    /* vhost-user must be cleaned up before chardevs.  */
>> +    net_cleanup();
>>       qemu_chr_cleanup();
>>         return 0;
> 
> Patch does not seems to be applied on master. Which tree is this patch
> based?

It's applied on top of Marc-André's character device cleanup series.

Paolo

Re: [Qemu-devel] [PATCH] net: cleanup before character devices

[Jason Wang]

On 2016年07月11日 17:18, Paolo Bonzini wrote:
>
> On 11/07/2016 11:06, Jason Wang wrote:
>>
>> On 2016年07月08日 23:27, Paolo Bonzini wrote:
>>> Otherwise, vhost-user causes a use-after-free.
>>>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>    vl.c | 6 +++---
>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/vl.c b/vl.c
>>> index d0b9ff9..005162d 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -4327,9 +4327,6 @@ int main(int argc, char **argv, char **envp)
>>>            qemu_opts_del(icount_opts);
>>>        }
>>>    -    /* clean up network at qemu process termination */
>>> -    atexit(&net_cleanup);
>>> -
>>>        if (default_net) {
>>>            QemuOptsList *net = qemu_find_opts("net");
>>>            qemu_opts_set(net, NULL, "type", "nic", &error_abort);
>>> @@ -4596,6 +4593,9 @@ int main(int argc, char **argv, char **envp)
>>>    #ifdef CONFIG_TPM
>>>        tpm_cleanup();
>>>    #endif
>>> +
>>> +    /* vhost-user must be cleaned up before chardevs.  */
>>> +    net_cleanup();
>>>        qemu_chr_cleanup();
>>>          return 0;
>> Patch does not seems to be applied on master. Which tree is this patch
>> based?
> It's applied on top of Marc-André's character device cleanup series.
>
> Paolo
>

Ok, but looks like the issue we tried to solve with commit 
f30dbae63a46f23116715dff8d130c7d01ff02be will appear again?

Re: [Qemu-devel] [PATCH] net: cleanup before character devices

[Paolo Bonzini]

On 11/07/2016 13:23, Jason Wang wrote:
> 
> 
> On 2016年07月11日 17:18, Paolo Bonzini wrote:
>>
>> On 11/07/2016 11:06, Jason Wang wrote:
>>>
>>> On 2016年07月08日 23:27, Paolo Bonzini wrote:
>>>> Otherwise, vhost-user causes a use-after-free.
>>>>
>>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>>> ---
>>>>    vl.c | 6 +++---
>>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/vl.c b/vl.c
>>>> index d0b9ff9..005162d 100644
>>>> --- a/vl.c
>>>> +++ b/vl.c
>>>> @@ -4327,9 +4327,6 @@ int main(int argc, char **argv, char **envp)
>>>>            qemu_opts_del(icount_opts);
>>>>        }
>>>>    -    /* clean up network at qemu process termination */
>>>> -    atexit(&net_cleanup);
>>>> -
>>>>        if (default_net) {
>>>>            QemuOptsList *net = qemu_find_opts("net");
>>>>            qemu_opts_set(net, NULL, "type", "nic", &error_abort);
>>>> @@ -4596,6 +4593,9 @@ int main(int argc, char **argv, char **envp)
>>>>    #ifdef CONFIG_TPM
>>>>        tpm_cleanup();
>>>>    #endif
>>>> +
>>>> +    /* vhost-user must be cleaned up before chardevs.  */
>>>> +    net_cleanup();
>>>>        qemu_chr_cleanup();
>>>>          return 0;
>>> Patch does not seems to be applied on master. Which tree is this patch
>>> based?
>> It's applied on top of Marc-André's character device cleanup series.
> 
> Ok, but looks like the issue we tried to solve with commit
> f30dbae63a46f23116715dff8d130c7d01ff02be will appear again?

You're right. :/

Marc-André, I think a possible solution is this:

1) add a Notifier to struct TAPState and register it with
qemu_add_exit_notifier.  The notifier, when called, invokes the down_script.

2) in tap_cleanup, invoke the down_script and unregister the notifier
with qemu_remove_exit_notifier.

Can you implement this?

Paolo

Re: [Qemu-devel] [PATCH] net: cleanup before character devices

[Marc-André Lureau]

Hi

----- Original Message -----
> 
> 
> On 11/07/2016 13:23, Jason Wang wrote:
> > 
> > 
> > On 2016年07月11日 17:18, Paolo Bonzini wrote:
> >>
> >> On 11/07/2016 11:06, Jason Wang wrote:
> >>>
> >>> On 2016年07月08日 23:27, Paolo Bonzini wrote:
> >>>> Otherwise, vhost-user causes a use-after-free.
> >>>>
> >>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >>>> ---
> >>>>    vl.c | 6 +++---
> >>>>    1 file changed, 3 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/vl.c b/vl.c
> >>>> index d0b9ff9..005162d 100644
> >>>> --- a/vl.c
> >>>> +++ b/vl.c
> >>>> @@ -4327,9 +4327,6 @@ int main(int argc, char **argv, char **envp)
> >>>>            qemu_opts_del(icount_opts);
> >>>>        }
> >>>>    -    /* clean up network at qemu process termination */
> >>>> -    atexit(&net_cleanup);
> >>>> -
> >>>>        if (default_net) {
> >>>>            QemuOptsList *net = qemu_find_opts("net");
> >>>>            qemu_opts_set(net, NULL, "type", "nic", &error_abort);
> >>>> @@ -4596,6 +4593,9 @@ int main(int argc, char **argv, char **envp)
> >>>>    #ifdef CONFIG_TPM
> >>>>        tpm_cleanup();
> >>>>    #endif
> >>>> +
> >>>> +    /* vhost-user must be cleaned up before chardevs.  */
> >>>> +    net_cleanup();
> >>>>        qemu_chr_cleanup();
> >>>>          return 0;
> >>> Patch does not seems to be applied on master. Which tree is this patch
> >>> based?
> >> It's applied on top of Marc-André's character device cleanup series.
> > 
> > Ok, but looks like the issue we tried to solve with commit
> > f30dbae63a46f23116715dff8d130c7d01ff02be will appear again?
> 
> You're right. :/
> 
> Marc-André, I think a possible solution is this:
> 
> 1) add a Notifier to struct TAPState and register it with
> qemu_add_exit_notifier.  The notifier, when called, invokes the down_script.
> 
> 2) in tap_cleanup, invoke the down_script and unregister the notifier
> with qemu_remove_exit_notifier.
> 
> Can you implement this?

What if we register an exit notifier right before leaving main() ? (qemu_add_exit_notifier(&chr_cleanup_notifier))

So that qemu_chr_cleanup() is only invoked at the last time when doing main() exit?

Re: [Qemu-devel] [PATCH] net: cleanup before character devices

[Paolo Bonzini]

On 11/07/2016 14:51, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
>>
>>
>> On 11/07/2016 13:23, Jason Wang wrote:
>>>
>>>
>>> On 2016年07月11日 17:18, Paolo Bonzini wrote:
>>>>
>>>> On 11/07/2016 11:06, Jason Wang wrote:
>>>>>
>>>>> On 2016年07月08日 23:27, Paolo Bonzini wrote:
>>>>>> Otherwise, vhost-user causes a use-after-free.
>>>>>>
>>>>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>>>>> ---
>>>>>>    vl.c | 6 +++---
>>>>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/vl.c b/vl.c
>>>>>> index d0b9ff9..005162d 100644
>>>>>> --- a/vl.c
>>>>>> +++ b/vl.c
>>>>>> @@ -4327,9 +4327,6 @@ int main(int argc, char **argv, char **envp)
>>>>>>            qemu_opts_del(icount_opts);
>>>>>>        }
>>>>>>    -    /* clean up network at qemu process termination */
>>>>>> -    atexit(&net_cleanup);
>>>>>> -
>>>>>>        if (default_net) {
>>>>>>            QemuOptsList *net = qemu_find_opts("net");
>>>>>>            qemu_opts_set(net, NULL, "type", "nic", &error_abort);
>>>>>> @@ -4596,6 +4593,9 @@ int main(int argc, char **argv, char **envp)
>>>>>>    #ifdef CONFIG_TPM
>>>>>>        tpm_cleanup();
>>>>>>    #endif
>>>>>> +
>>>>>> +    /* vhost-user must be cleaned up before chardevs.  */
>>>>>> +    net_cleanup();
>>>>>>        qemu_chr_cleanup();
>>>>>>          return 0;
>>>>> Patch does not seems to be applied on master. Which tree is this patch
>>>>> based?
>>>> It's applied on top of Marc-André's character device cleanup series.
>>>
>>> Ok, but looks like the issue we tried to solve with commit
>>> f30dbae63a46f23116715dff8d130c7d01ff02be will appear again?
>>
>> You're right. :/
>>
>> Marc-André, I think a possible solution is this:
>>
>> 1) add a Notifier to struct TAPState and register it with
>> qemu_add_exit_notifier.  The notifier, when called, invokes the down_script.
>>
>> 2) in tap_cleanup, invoke the down_script and unregister the notifier
>> with qemu_remove_exit_notifier.
>>
>> Can you implement this?
> 
> What if we register an exit notifier right before leaving main() ? (qemu_add_exit_notifier(&chr_cleanup_notifier))

Exit notifiers are run with atexit (the difference is that you can
remove them, which is exactly what we need here!).  You would have the
same issue as before, where you cannot rely on the order of execution.

The logic is:

1) main tears down objects in the "correct" order, which is net before char

2) if something wants to do cleanup at exit() time, they can register an
exit notifier on creation and unregister it on cleanup.  It can be
TAPState, or I guess Unix socket chardevs could also do this and it
would be nice.  However, it's okay for me if you only change TAPState.

So an exit() does the bare minimum necessary cleanup, whereas a "quit"
or SIGTERM brings down everything.

Thanks,

Paolo

> So that qemu_chr_cleanup() is only invoked at the last time when doing main() exit?
>