All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] migration/rdma: Fix qemu_rdma_cleanup null check
@ 2019-02-14 18:53 Dr. David Alan Gilbert (git)
  2019-02-15  1:55 ` Peter Xu
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2019-02-14 18:53 UTC (permalink / raw)
  To: qemu-devel, quintela, peterx, peter.maydell

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

If the migration fails before the channel is open (e.g. a bad
address) we end up in the cleanup with rdma->channel==NULL.

Spotted by Coverity: CID 1398634
Fixes: fbbaacab2758cb3f32a0
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/rdma.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/migration/rdma.c b/migration/rdma.c
index 54a3c11540..9fa3b176eb 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -2321,7 +2321,9 @@ static void qemu_rdma_cleanup(RDMAContext *rdma)
         rdma->connected = false;
     }
 
-    qemu_set_fd_handler(rdma->channel->fd, NULL, NULL, NULL);
+    if (rdma->channel) {
+        qemu_set_fd_handler(rdma->channel->fd, NULL, NULL, NULL);
+    }
     g_free(rdma->dest_blocks);
     rdma->dest_blocks = NULL;
 
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] migration/rdma: Fix qemu_rdma_cleanup null check
  2019-02-14 18:53 [Qemu-devel] [PATCH] migration/rdma: Fix qemu_rdma_cleanup null check Dr. David Alan Gilbert (git)
@ 2019-02-15  1:55 ` Peter Xu
  2019-02-15 11:00   ` Dr. David Alan Gilbert
  2019-02-15 11:30 ` Philippe Mathieu-Daudé
  2019-03-05 13:23 ` Dr. David Alan Gilbert
  2 siblings, 1 reply; 6+ messages in thread
From: Peter Xu @ 2019-02-15  1:55 UTC (permalink / raw)
  To: Dr. David Alan Gilbert (git); +Cc: qemu-devel, quintela, peter.maydell

On Thu, Feb 14, 2019 at 06:53:51PM +0000, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> If the migration fails before the channel is open (e.g. a bad
> address) we end up in the cleanup with rdma->channel==NULL.
> 
> Spotted by Coverity: CID 1398634
> Fixes: fbbaacab2758cb3f32a0
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  migration/rdma.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 54a3c11540..9fa3b176eb 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -2321,7 +2321,9 @@ static void qemu_rdma_cleanup(RDMAContext *rdma)
>          rdma->connected = false;
>      }
>  
> -    qemu_set_fd_handler(rdma->channel->fd, NULL, NULL, NULL);
> +    if (rdma->channel) {
> +        qemu_set_fd_handler(rdma->channel->fd, NULL, NULL, NULL);
> +    }

IIUC there's no strict ordering constraint on resetting the fd
handler, then how about simply moving this line into the below "if
(rdma->channel)" altogether?

Regards,

-- 
Peter Xu

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] migration/rdma: Fix qemu_rdma_cleanup null check
  2019-02-15  1:55 ` Peter Xu
@ 2019-02-15 11:00   ` Dr. David Alan Gilbert
  2019-02-15 12:01     ` Peter Xu
  0 siblings, 1 reply; 6+ messages in thread
From: Dr. David Alan Gilbert @ 2019-02-15 11:00 UTC (permalink / raw)
  To: Peter Xu; +Cc: qemu-devel, quintela, peter.maydell

* Peter Xu (peterx@redhat.com) wrote:
> On Thu, Feb 14, 2019 at 06:53:51PM +0000, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > If the migration fails before the channel is open (e.g. a bad
> > address) we end up in the cleanup with rdma->channel==NULL.
> > 
> > Spotted by Coverity: CID 1398634
> > Fixes: fbbaacab2758cb3f32a0
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  migration/rdma.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/migration/rdma.c b/migration/rdma.c
> > index 54a3c11540..9fa3b176eb 100644
> > --- a/migration/rdma.c
> > +++ b/migration/rdma.c
> > @@ -2321,7 +2321,9 @@ static void qemu_rdma_cleanup(RDMAContext *rdma)
> >          rdma->connected = false;
> >      }
> >  
> > -    qemu_set_fd_handler(rdma->channel->fd, NULL, NULL, NULL);
> > +    if (rdma->channel) {
> > +        qemu_set_fd_handler(rdma->channel->fd, NULL, NULL, NULL);
> > +    }
> 
> IIUC there's no strict ordering constraint on resetting the fd
> handler, then how about simply moving this line into the below "if
> (rdma->channel)" altogether?

The logic around the closing of the return path makes that check later a
bit messy; rdma->channel can get set to Null before the other check.

Dave

> Regards,
> 
> -- 
> Peter Xu
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] migration/rdma: Fix qemu_rdma_cleanup null check
  2019-02-14 18:53 [Qemu-devel] [PATCH] migration/rdma: Fix qemu_rdma_cleanup null check Dr. David Alan Gilbert (git)
  2019-02-15  1:55 ` Peter Xu
@ 2019-02-15 11:30 ` Philippe Mathieu-Daudé
  2019-03-05 13:23 ` Dr. David Alan Gilbert
  2 siblings, 0 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-02-15 11:30 UTC (permalink / raw)
  To: Dr. David Alan Gilbert (git),
	qemu-devel, quintela, peterx, peter.maydell

On 2/14/19 7:53 PM, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> If the migration fails before the channel is open (e.g. a bad
> address) we end up in the cleanup with rdma->channel==NULL.
> 
> Spotted by Coverity: CID 1398634
> Fixes: fbbaacab2758cb3f32a0
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  migration/rdma.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 54a3c11540..9fa3b176eb 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -2321,7 +2321,9 @@ static void qemu_rdma_cleanup(RDMAContext *rdma)
>          rdma->connected = false;
>      }
>  
> -    qemu_set_fd_handler(rdma->channel->fd, NULL, NULL, NULL);
> +    if (rdma->channel) {
> +        qemu_set_fd_handler(rdma->channel->fd, NULL, NULL, NULL);
> +    }
>      g_free(rdma->dest_blocks);
>      rdma->dest_blocks = NULL;
>  
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] migration/rdma: Fix qemu_rdma_cleanup null check
  2019-02-15 11:00   ` Dr. David Alan Gilbert
@ 2019-02-15 12:01     ` Peter Xu
  0 siblings, 0 replies; 6+ messages in thread
From: Peter Xu @ 2019-02-15 12:01 UTC (permalink / raw)
  To: Dr. David Alan Gilbert; +Cc: qemu-devel, quintela, peter.maydell

On Fri, Feb 15, 2019 at 11:00:56AM +0000, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > On Thu, Feb 14, 2019 at 06:53:51PM +0000, Dr. David Alan Gilbert (git) wrote:
> > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > 
> > > If the migration fails before the channel is open (e.g. a bad
> > > address) we end up in the cleanup with rdma->channel==NULL.
> > > 
> > > Spotted by Coverity: CID 1398634
> > > Fixes: fbbaacab2758cb3f32a0
> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > ---
> > >  migration/rdma.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/migration/rdma.c b/migration/rdma.c
> > > index 54a3c11540..9fa3b176eb 100644
> > > --- a/migration/rdma.c
> > > +++ b/migration/rdma.c
> > > @@ -2321,7 +2321,9 @@ static void qemu_rdma_cleanup(RDMAContext *rdma)
> > >          rdma->connected = false;
> > >      }
> > >  
> > > -    qemu_set_fd_handler(rdma->channel->fd, NULL, NULL, NULL);
> > > +    if (rdma->channel) {
> > > +        qemu_set_fd_handler(rdma->channel->fd, NULL, NULL, NULL);
> > > +    }
> > 
> > IIUC there's no strict ordering constraint on resetting the fd
> > handler, then how about simply moving this line into the below "if
> > (rdma->channel)" altogether?
> 
> The logic around the closing of the return path makes that check later a
> bit messy; rdma->channel can get set to Null before the other check.

Ah I see, it's the mess by sharing listen_id and channel on
destination side...  Maybe we can clean them up along the way?  I gave
it a shot:

    if (rdma->listen_id && rdma->is_return_path) {
        /*
         * The return path on the destination side, both listen_id and
         * channel are shared with the other context so we skip
         * freeing those but simply clear the pointers no matter what.
         * The main context will help us to clean these.
         */
        rdma->listen_id = NULL;
        rdma->channel = NULL;
    } else {
        /*
         * Either the source side, or the main context of the
         * destination side: we are responsible for listen_id/channel
         */
        if (rdma->listen_id) {
            rdma_destroy_id(rdma->listen_id);
            rdma->listen_id = NULL;
        }
        if (rdma->channel) {
            qemu_set_fd_handler(rdma->channel->fd, NULL, NULL, NULL);
            rdma_destroy_event_channel(rdma->channel);
            rdma->channel = NULL;
        }
    }

I slightly prefer to clean it up (if someone is still going to
maintain the RDMA code... :), but either way is fine to me.

No matter what you prefer:

Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks,

-- 
Peter Xu

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] migration/rdma: Fix qemu_rdma_cleanup null check
  2019-02-14 18:53 [Qemu-devel] [PATCH] migration/rdma: Fix qemu_rdma_cleanup null check Dr. David Alan Gilbert (git)
  2019-02-15  1:55 ` Peter Xu
  2019-02-15 11:30 ` Philippe Mathieu-Daudé
@ 2019-03-05 13:23 ` Dr. David Alan Gilbert
  2 siblings, 0 replies; 6+ messages in thread
From: Dr. David Alan Gilbert @ 2019-03-05 13:23 UTC (permalink / raw)
  To: qemu-devel, quintela, peterx, peter.maydell

* Dr. David Alan Gilbert (git) (dgilbert@redhat.com) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> If the migration fails before the channel is open (e.g. a bad
> address) we end up in the cleanup with rdma->channel==NULL.
> 
> Spotted by Coverity: CID 1398634
> Fixes: fbbaacab2758cb3f32a0
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Queued

> ---
>  migration/rdma.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 54a3c11540..9fa3b176eb 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -2321,7 +2321,9 @@ static void qemu_rdma_cleanup(RDMAContext *rdma)
>          rdma->connected = false;
>      }
>  
> -    qemu_set_fd_handler(rdma->channel->fd, NULL, NULL, NULL);
> +    if (rdma->channel) {
> +        qemu_set_fd_handler(rdma->channel->fd, NULL, NULL, NULL);
> +    }
>      g_free(rdma->dest_blocks);
>      rdma->dest_blocks = NULL;
>  
> -- 
> 2.20.1
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-03-05 13:24 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-14 18:53 [Qemu-devel] [PATCH] migration/rdma: Fix qemu_rdma_cleanup null check Dr. David Alan Gilbert (git)
2019-02-15  1:55 ` Peter Xu
2019-02-15 11:00   ` Dr. David Alan Gilbert
2019-02-15 12:01     ` Peter Xu
2019-02-15 11:30 ` Philippe Mathieu-Daudé
2019-03-05 13:23 ` Dr. David Alan Gilbert

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.