| Message ID | 20211103193129.23930-1-quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | postocpy: Check that postocpy fd's are not NULL | expand |
* Juan Quintela (quintela@redhat.com) wrote: > If postcopy has finished, it frees the array. > But vhost-user unregister it at cleanup time. I must admit to being confused as the double migrate case vs migrate once and shutdown. It might just be an ordering thing? I notice that 'vhost_user_backend_cleanup' does: if (u->postcopy_fd.handler) { postcopy_unregister_shared_ufd(&u->postcopy_fd); close(u->postcopy_fd.fd); u->postcopy_fd.handler = NULL; } where as the other caller, 'vhost_user_postcopy_end' does: postcopy_unregister_shared_ufd(&u->postcopy_fd); close(u->postcopy_fd.fd); u->postcopy_fd.handler = NULL; maybe it would be better to fix them to do the same if check? (Also note 'post*o*cpy' typo in title, and probably worth a Fixes: c4f7538 ?) Dave > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > migration/postcopy-ram.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c > index e721f69d0f..d18b5d05b2 100644 > --- a/migration/postcopy-ram.c > +++ b/migration/postcopy-ram.c > @@ -1457,6 +1457,10 @@ void postcopy_unregister_shared_ufd(struct PostCopyFD *pcfd) > MigrationIncomingState *mis = migration_incoming_get_current(); > GArray *pcrfds = mis->postcopy_remote_fds; > > + if (!pcrfds) { > + /* migration has already finished and freed the array */ > + return; > + } > for (i = 0; i < pcrfds->len; i++) { > struct PostCopyFD *cur = &g_array_index(pcrfds, struct PostCopyFD, i); > if (cur->fd == pcfd->fd) { > -- > 2.33.1 >
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > * Juan Quintela (quintela@redhat.com) wrote: >> If postcopy has finished, it frees the array. >> But vhost-user unregister it at cleanup time. > > I must admit to being confused as the double migrate case vs migrate > once and shutdown. It might just be an ordering thing? > > I notice that 'vhost_user_backend_cleanup' does: > if (u->postcopy_fd.handler) { > postcopy_unregister_shared_ufd(&u->postcopy_fd); > close(u->postcopy_fd.fd); > u->postcopy_fd.handler = NULL; > } > > where as the other caller, 'vhost_user_postcopy_end' > does: > postcopy_unregister_shared_ufd(&u->postcopy_fd); > close(u->postcopy_fd.fd); > u->postcopy_fd.handler = NULL; I think that we want ta make here the check to see if it has already been freed. > maybe it would be better to fix them to do the same if check? But even there, I think that it is more robust that we don't try to access a NULL pointer. I.e. there are two things that we can fix here: - postcopy unregister - vhost use of postcopy unregister > (Also note 'post*o*cpy' typo in title, and probably worth a > Fixes: c4f7538 ?) Sure. What do you think? Later, Juan. > Dave > > >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> --- >> migration/postcopy-ram.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c >> index e721f69d0f..d18b5d05b2 100644 >> --- a/migration/postcopy-ram.c >> +++ b/migration/postcopy-ram.c >> @@ -1457,6 +1457,10 @@ void postcopy_unregister_shared_ufd(struct PostCopyFD *pcfd) >> MigrationIncomingState *mis = migration_incoming_get_current(); >> GArray *pcrfds = mis->postcopy_remote_fds; >> >> + if (!pcrfds) { >> + /* migration has already finished and freed the array */ >> + return; >> + } >> for (i = 0; i < pcrfds->len; i++) { >> struct PostCopyFD *cur = &g_array_index(pcrfds, struct PostCopyFD, i); >> if (cur->fd == pcfd->fd) { >> -- >> 2.33.1 >>
* Juan Quintela (quintela@redhat.com) wrote: > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > > * Juan Quintela (quintela@redhat.com) wrote: > >> If postcopy has finished, it frees the array. > >> But vhost-user unregister it at cleanup time. > > > > I must admit to being confused as the double migrate case vs migrate > > once and shutdown. It might just be an ordering thing? > > > > I notice that 'vhost_user_backend_cleanup' does: > > if (u->postcopy_fd.handler) { > > postcopy_unregister_shared_ufd(&u->postcopy_fd); > > close(u->postcopy_fd.fd); > > u->postcopy_fd.handler = NULL; > > } > > > > where as the other caller, 'vhost_user_postcopy_end' > > does: > > postcopy_unregister_shared_ufd(&u->postcopy_fd); > > close(u->postcopy_fd.fd); > > u->postcopy_fd.handler = NULL; > > I think that we want ta make here the check to see if it has already > been freed. > > > maybe it would be better to fix them to do the same if check? > > But even there, I think that it is more robust that we don't try to > access a NULL pointer. > > I.e. there are two things that we can fix here: > - postcopy unregister > - vhost use of postcopy unregister True we could indeed fix both as a belt-and-braces. > > (Also note 'post*o*cpy' typo in title, and probably worth a > > Fixes: c4f7538 ?) > > Sure. > > What do you think? So yeh, Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> Please do the other if as a separate patch sometime. Dave > Later, Juan. > > > Dave > > > > > >> Signed-off-by: Juan Quintela <quintela@redhat.com> > >> --- > >> migration/postcopy-ram.c | 4 ++++ > >> 1 file changed, 4 insertions(+) > >> > >> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c > >> index e721f69d0f..d18b5d05b2 100644 > >> --- a/migration/postcopy-ram.c > >> +++ b/migration/postcopy-ram.c > >> @@ -1457,6 +1457,10 @@ void postcopy_unregister_shared_ufd(struct PostCopyFD *pcfd) > >> MigrationIncomingState *mis = migration_incoming_get_current(); > >> GArray *pcrfds = mis->postcopy_remote_fds; > >> > >> + if (!pcrfds) { > >> + /* migration has already finished and freed the array */ > >> + return; > >> + } > >> for (i = 0; i < pcrfds->len; i++) { > >> struct PostCopyFD *cur = &g_array_index(pcrfds, struct PostCopyFD, i); > >> if (cur->fd == pcfd->fd) { > >> -- > >> 2.33.1 > >> >
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c index e721f69d0f..d18b5d05b2 100644 --- a/migration/postcopy-ram.c +++ b/migration/postcopy-ram.c @@ -1457,6 +1457,10 @@ void postcopy_unregister_shared_ufd(struct PostCopyFD *pcfd) MigrationIncomingState *mis = migration_incoming_get_current(); GArray *pcrfds = mis->postcopy_remote_fds; + if (!pcrfds) { + /* migration has already finished and freed the array */ + return; + } for (i = 0; i < pcrfds->len; i++) { struct PostCopyFD *cur = &g_array_index(pcrfds, struct PostCopyFD, i); if (cur->fd == pcfd->fd) {
If postcopy has finished, it frees the array. But vhost-user unregister it at cleanup time. Signed-off-by: Juan Quintela <quintela@redhat.com> --- migration/postcopy-ram.c | 4 ++++ 1 file changed, 4 insertions(+)