diff mbox series

[PULL,02/13] hw/rdma/vmw/pvrdma_cmd: Use correct struct in query_port()

Message ID 20231011112054.1031975-3-mjt@tls.msk.ru
State New
Headers show
Series None | expand

Commit Message

Michael Tokarev Oct. 11, 2023, 11:20 a.m. UTC 
From: Peter Maydell <peter.maydell@linaro.org>

In query_port() we pass the address of a local pvrdma_port_attr
struct to the rdma_query_backend_port() function.  Unfortunately,
rdma_backend_query_port() wants a pointer to a struct ibv_port_attr,
and the two are not the same length.

Coverity spotted this (CID 1507146): pvrdma_port_attr is 48 bytes
long, and ibv_port_attr is 52 bytes, because it has a few extra
fields at the end.

Fortunately, all we do with the attrs struct after the call is to
read a few specific fields out of it which are all at the same
offsets in both structs, so we can simply make the local variable the
correct type.  This also lets us drop the cast (which should have
been a bit of a warning flag that we were doing something wrong
here).

Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 hw/rdma/vmw/pvrdma_cmd.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Stefan Hajnoczi Oct. 11, 2023, 3:38 p.m. UTC | #1 
On Wed, 11 Oct 2023 at 07:23, Michael Tokarev <mjt@tls.msk.ru> wrote:
>
> From: Peter Maydell <peter.maydell@linaro.org>
>
> In query_port() we pass the address of a local pvrdma_port_attr
> struct to the rdma_query_backend_port() function.  Unfortunately,
> rdma_backend_query_port() wants a pointer to a struct ibv_port_attr,
> and the two are not the same length.
>
> Coverity spotted this (CID 1507146): pvrdma_port_attr is 48 bytes
> long, and ibv_port_attr is 52 bytes, because it has a few extra
> fields at the end.
>
> Fortunately, all we do with the attrs struct after the call is to
> read a few specific fields out of it which are all at the same
> offsets in both structs, so we can simply make the local variable the
> correct type.  This also lets us drop the cast (which should have
> been a bit of a warning flag that we were doing something wrong
> here).
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  hw/rdma/vmw/pvrdma_cmd.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)

The following CI failure has occurred:

../hw/rdma/vmw/pvrdma_cmd.c:144:59: error: implicit conversion from
enumeration type 'enum ibv_port_state' to different enumeration type
'enum pvrdma_port_state' [-Werror,-Wenum-conversion]
resp->attrs.state = dev->func0->device_active ? attrs.state :
~ ~~~~~~^~~~~
../hw/rdma/vmw/pvrdma_cmd.c:146:33: error: implicit conversion from
enumeration type 'enum ibv_mtu' to different enumeration type 'enum
pvrdma_mtu' [-Werror,-Wenum-conversion]
resp->attrs.max_mtu = attrs.max_mtu;
~ ~~~~~~^~~~~~~
../hw/rdma/vmw/pvrdma_cmd.c:147:36: error: implicit conversion from
enumeration type 'enum ibv_mtu' to different enumeration type 'enum
pvrdma_mtu' [-Werror,-Wenum-conversion]
resp->attrs.active_mtu = attrs.active_mtu;
~ ~~~~~~^~~~~~~~~~

https://gitlab.com/qemu-project/qemu/-/jobs/5270666420

Please take a look.

Thanks,
Stefan

>
> diff --git a/hw/rdma/vmw/pvrdma_cmd.c b/hw/rdma/vmw/pvrdma_cmd.c
> index c6ed025982..d31c187593 100644
> --- a/hw/rdma/vmw/pvrdma_cmd.c
> +++ b/hw/rdma/vmw/pvrdma_cmd.c
> @@ -129,14 +129,13 @@ static int query_port(PVRDMADev *dev, union pvrdma_cmd_req *req,
>  {
>      struct pvrdma_cmd_query_port *cmd = &req->query_port;
>      struct pvrdma_cmd_query_port_resp *resp = &rsp->query_port_resp;
> -    struct pvrdma_port_attr attrs = {};
> +    struct ibv_port_attr attrs = {};
>
>      if (cmd->port_num > MAX_PORTS) {
>          return -EINVAL;
>      }
>
> -    if (rdma_backend_query_port(&dev->backend_dev,
> -                                (struct ibv_port_attr *)&attrs)) {
> +    if (rdma_backend_query_port(&dev->backend_dev, &attrs)) {
>          return -ENOMEM;
>      }
>
> --
> 2.39.2
>
>
Thomas Huth Oct. 11, 2023, 4:29 p.m. UTC | #2 
On 11/10/2023 17.38, Stefan Hajnoczi wrote:
> On Wed, 11 Oct 2023 at 07:23, Michael Tokarev <mjt@tls.msk.ru> wrote:
>>
>> From: Peter Maydell <peter.maydell@linaro.org>
>>
>> In query_port() we pass the address of a local pvrdma_port_attr
>> struct to the rdma_query_backend_port() function.  Unfortunately,
>> rdma_backend_query_port() wants a pointer to a struct ibv_port_attr,
>> and the two are not the same length.
>>
>> Coverity spotted this (CID 1507146): pvrdma_port_attr is 48 bytes
>> long, and ibv_port_attr is 52 bytes, because it has a few extra
>> fields at the end.
>>
>> Fortunately, all we do with the attrs struct after the call is to
>> read a few specific fields out of it which are all at the same
>> offsets in both structs, so we can simply make the local variable the
>> correct type.  This also lets us drop the cast (which should have
>> been a bit of a warning flag that we were doing something wrong
>> here).
>>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
>> ---
>>   hw/rdma/vmw/pvrdma_cmd.c | 5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> The following CI failure has occurred:
> 
> ../hw/rdma/vmw/pvrdma_cmd.c:144:59: error: implicit conversion from
> enumeration type 'enum ibv_port_state' to different enumeration type
> 'enum pvrdma_port_state' [-Werror,-Wenum-conversion]
> resp->attrs.state = dev->func0->device_active ? attrs.state :
> ~ ~~~~~~^~~~~
> ../hw/rdma/vmw/pvrdma_cmd.c:146:33: error: implicit conversion from
> enumeration type 'enum ibv_mtu' to different enumeration type 'enum
> pvrdma_mtu' [-Werror,-Wenum-conversion]
> resp->attrs.max_mtu = attrs.max_mtu;
> ~ ~~~~~~^~~~~~~
> ../hw/rdma/vmw/pvrdma_cmd.c:147:36: error: implicit conversion from
> enumeration type 'enum ibv_mtu' to different enumeration type 'enum
> pvrdma_mtu' [-Werror,-Wenum-conversion]
> resp->attrs.active_mtu = attrs.active_mtu;
> ~ ~~~~~~^~~~~~~~~~
> 
> https://gitlab.com/qemu-project/qemu/-/jobs/5270666420
> 
> Please take a look.

That pvrdma patch unfortunately does not work with Clang, see also:

 
https://lore.kernel.org/qemu-devel/781330fc-85b3-4ef1-8f07-1cc5fc5e4ad2@redhat.com/T/#t

  Thomas
Peter Maydell Oct. 19, 2023, 2:41 p.m. UTC | #3 
On Wed, 11 Oct 2023 at 17:29, Thomas Huth <thuth@redhat.com> wrote:
>
> On 11/10/2023 17.38, Stefan Hajnoczi wrote:
> > The following CI failure has occurred:
> >
> > ../hw/rdma/vmw/pvrdma_cmd.c:144:59: error: implicit conversion from
> > enumeration type 'enum ibv_port_state' to different enumeration type
> > 'enum pvrdma_port_state' [-Werror,-Wenum-conversion]
> > resp->attrs.state = dev->func0->device_active ? attrs.state :
> > ~ ~~~~~~^~~~~
> > ../hw/rdma/vmw/pvrdma_cmd.c:146:33: error: implicit conversion from
> > enumeration type 'enum ibv_mtu' to different enumeration type 'enum
> > pvrdma_mtu' [-Werror,-Wenum-conversion]
> > resp->attrs.max_mtu = attrs.max_mtu;
> > ~ ~~~~~~^~~~~~~
> > ../hw/rdma/vmw/pvrdma_cmd.c:147:36: error: implicit conversion from
> > enumeration type 'enum ibv_mtu' to different enumeration type 'enum
> > pvrdma_mtu' [-Werror,-Wenum-conversion]
> > resp->attrs.active_mtu = attrs.active_mtu;
> > ~ ~~~~~~^~~~~~~~~~
> >
> > https://gitlab.com/qemu-project/qemu/-/jobs/5270666420
> >
> > Please take a look.
>
> That pvrdma patch unfortunately does not work with Clang, see also:
>
>
> https://lore.kernel.org/qemu-devel/781330fc-85b3-4ef1-8f07-1cc5fc5e4ad2@redhat.com/T/#t

Explicitly casting to the new enum type is OK and fixes the clang
warnings; I'll send out a v2 of the patch in a moment.

thanks
-- PMM
diff mbox series

Patch

diff --git a/hw/rdma/vmw/pvrdma_cmd.c b/hw/rdma/vmw/pvrdma_cmd.c
index c6ed025982..d31c187593 100644
--- a/hw/rdma/vmw/pvrdma_cmd.c
+++ b/hw/rdma/vmw/pvrdma_cmd.c
@@ -129,14 +129,13 @@  static int query_port(PVRDMADev *dev, union pvrdma_cmd_req *req,
 {
     struct pvrdma_cmd_query_port *cmd = &req->query_port;
     struct pvrdma_cmd_query_port_resp *resp = &rsp->query_port_resp;
-    struct pvrdma_port_attr attrs = {};
+    struct ibv_port_attr attrs = {};
 
     if (cmd->port_num > MAX_PORTS) {
         return -EINVAL;
     }
 
-    if (rdma_backend_query_port(&dev->backend_dev,
-                                (struct ibv_port_attr *)&attrs)) {
+    if (rdma_backend_query_port(&dev->backend_dev, &attrs)) {
         return -ENOMEM;
     }