| Message ID | 1467991671-93391-1-git-send-email-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > >
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
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
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
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?
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
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?
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? >
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;
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(-)