Re: [Qemu-devel] [PATCH] rdma: Fix qemu crash when IPv6 address is used for migration

Re: [Qemu-devel] [PATCH] rdma: Fix qemu crash when IPv6 address is used for migration

[Juan Quintela]

Padmanabh Ratnakar <padmanabh.ratnakar@avagotech.com> wrote:
> Qemu crashes when IPv6 address is specified for migration and access
> to any RDMA uverbs device available on the system is blocked using cgroups.
> Fix the crash by checking the return value of ibv_open_device routine.
>
> Signed-off-by: Meghana Cheripady <meghana.cheripady@avagotech.com>
> Signed-off-by: Padmanabh Ratnakar <padmanabh.ratnakar@avagotech.com>
> ---
>  migration/rdma.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 77e3444..3671903 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -790,6 +790,13 @@ static int qemu_rdma_broken_ipv6_kernel(Error **errp, struct ibv_context *verbs)
>  
>          for (x = 0; x < num_devices; x++) {
>              verbs = ibv_open_device(dev_list[x]);
> +            if (!verbs) {
> +                if (errno == EPERM) {
> +                    continue;

Why do we want to continue here?


> +                } else {
> +                    return -EINVAL;
> +                }
> +            }
>  
>              if (ibv_query_port(verbs, 1, &port_attr)) {


Reading the documentation, my understandig is that if verbs is NULL, we
don't have a context to do anything useful here, no?

>                  ibv_close_device(verbs);


Thanks, Juan.

Re: [Qemu-devel] [PATCH] rdma: Fix qemu crash when IPv6 address is used for migration

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> Padmanabh Ratnakar <padmanabh.ratnakar@avagotech.com> wrote:
> > Qemu crashes when IPv6 address is specified for migration and access
> > to any RDMA uverbs device available on the system is blocked using cgroups.
> > Fix the crash by checking the return value of ibv_open_device routine.
> >
> > Signed-off-by: Meghana Cheripady <meghana.cheripady@avagotech.com>
> > Signed-off-by: Padmanabh Ratnakar <padmanabh.ratnakar@avagotech.com>
> > ---
> >  migration/rdma.c |    7 +++++++
> >  1 files changed, 7 insertions(+), 0 deletions(-)
> >
> > diff --git a/migration/rdma.c b/migration/rdma.c
> > index 77e3444..3671903 100644
> > --- a/migration/rdma.c
> > +++ b/migration/rdma.c
> > @@ -790,6 +790,13 @@ static int qemu_rdma_broken_ipv6_kernel(Error **errp, struct ibv_context *verbs)
> >  
> >          for (x = 0; x < num_devices; x++) {
> >              verbs = ibv_open_device(dev_list[x]);
> > +            if (!verbs) {
> > +                if (errno == EPERM) {
> > +                    continue;
> 
> Why do we want to continue here?

I think the idea is to loop through the set of devices and find if any of them
are 'roce';  if one of the devices iwe don't have permission to use then it's probably
ok to skip and move onto the next one?

> > +                } else {
> > +                    return -EINVAL;
> > +                }
> > +            }
> >  
> >              if (ibv_query_port(verbs, 1, &port_attr)) {
> 
> 
> Reading the documentation, my understandig is that if verbs is NULL, we
> don't have a context to do anything useful here, no?

But that's OK because of the if (!verbs) above?

> >                  ibv_close_device(verbs);
> 
> 
> Thanks, Juan.

Dave

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

Re: [Qemu-devel] [PATCH] rdma: Fix qemu crash when IPv6 address is used for migration

[Juan Quintela]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> Padmanabh Ratnakar <padmanabh.ratnakar@avagotech.com> wrote:
>> > Qemu crashes when IPv6 address is specified for migration and access
>> > to any RDMA uverbs device available on the system is blocked using cgroups.
>> > Fix the crash by checking the return value of ibv_open_device routine.
>> >
>> > Signed-off-by: Meghana Cheripady <meghana.cheripady@avagotech.com>
>> > Signed-off-by: Padmanabh Ratnakar <padmanabh.ratnakar@avagotech.com>
>> > ---
>> >  migration/rdma.c |    7 +++++++
>> >  1 files changed, 7 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/migration/rdma.c b/migration/rdma.c
>> > index 77e3444..3671903 100644
>> > --- a/migration/rdma.c
>> > +++ b/migration/rdma.c
>> > @@ -790,6 +790,13 @@ static int qemu_rdma_broken_ipv6_kernel(Error **errp, struct ibv_context *verbs)
>> >  
>> >          for (x = 0; x < num_devices; x++) {
>> >              verbs = ibv_open_device(dev_list[x]);
>> > +            if (!verbs) {
>> > +                if (errno == EPERM) {
>> > +                    continue;
>> 
>> Why do we want to continue here?
>
> I think the idea is to loop through the set of devices and find if any of them
> are 'roce'; if one of the devices iwe don't have permission to use
> then it's probably
> ok to skip and move onto the next one?
>
>> > +                } else {
>> > +                    return -EINVAL;
>> > +                }
>> > +            }
>> >  
>> >              if (ibv_query_port(verbs, 1, &port_attr)) {
>> 
>> 
>> Reading the documentation, my understandig is that if verbs is NULL, we
>> don't have a context to do anything useful here, no?
>
> But that's OK because of the if (!verbs) above?
>
>> >                  ibv_close_device(verbs);
>> 
>> 
>> Thanks, Juan.

I got a brown paper bag over my head.  Just got patch contexct wrong.


>
> Dave
>
>> 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] rdma: Fix qemu crash when IPv6 address is used for migration

[Juan Quintela]

Padmanabh Ratnakar <padmanabh.ratnakar@avagotech.com> wrote:
> Qemu crashes when IPv6 address is specified for migration and access
> to any RDMA uverbs device available on the system is blocked using cgroups.
> Fix the crash by checking the return value of ibv_open_device routine.
>
> Signed-off-by: Meghana Cheripady <meghana.cheripady@avagotech.com>
> Signed-off-by: Padmanabh Ratnakar <padmanabh.ratnakar@avagotech.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Applied, thanks.

Sorry for the previous comment, I just mixed up context and my mail made
zero sense.


> ---
>  migration/rdma.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 77e3444..3671903 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -790,6 +790,13 @@ static int qemu_rdma_broken_ipv6_kernel(Error **errp, struct ibv_context *verbs)
>  
>          for (x = 0; x < num_devices; x++) {
>              verbs = ibv_open_device(dev_list[x]);
> +            if (!verbs) {
> +                if (errno == EPERM) {
> +                    continue;
> +                } else {
> +                    return -EINVAL;
> +                }
> +            }
>  
>              if (ibv_query_port(verbs, 1, &port_attr)) {
>                  ibv_close_device(verbs);