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 <address@hidden>
>>>>>> ---
>>>>>>    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?
>