[PATCH] postocpy: Check that postocpy fd's are not NULL

[PATCH] postocpy: Check that postocpy fd's are not NULL

[Juan Quintela]

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(+)

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

Re: [PATCH] postocpy: Check that postocpy fd's are not NULL

[Dr. David Alan Gilbert]

* 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 / Manchester, UK

Re: [PATCH] postocpy: Check that postocpy fd's are not NULL

[Juan Quintela]

"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
>> 

Re: [PATCH] postocpy: Check that postocpy fd's are not NULL

[Dr. David Alan Gilbert]

* 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
> >> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK