[bug report] vdpa/mlx5: Support different address spaces for control and data

[bug report] vdpa/mlx5: Support different address spaces for control and data

[Dan Carpenter]

Hello Eli Cohen,

The patch d5358cd0e369: "vdpa/mlx5: Support different address spaces
for control and data" from Jul 14, 2022, leads to the following
Smatch static checker warning:

	drivers/vdpa/mlx5/net/mlx5_vnet.c:2676 mlx5_vdpa_set_map()
	error: uninitialized symbol 'err'.

drivers/vdpa/mlx5/net/mlx5_vnet.c
    2657 static int mlx5_vdpa_set_map(struct vdpa_device *vdev, unsigned int asid,
    2658                              struct vhost_iotlb *iotlb)
    2659 {
    2660         struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
    2661         struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
    2662         int err;
    2663 
    2664         down_write(&ndev->reslock);
    2665         if (mvdev->group2asid[MLX5_VDPA_DATAVQ_GROUP] == asid) {
    2666                 err = set_map_data(mvdev, iotlb);
    2667                 if (err)
    2668                         goto out;
    2669         }
    2670 
    2671         if (mvdev->group2asid[MLX5_VDPA_CVQ_GROUP] == asid)
    2672                 err = set_map_control(mvdev, iotlb);

err not initialized on else path.  My guess is that one or both of these
conditions has to be true and this is a false positive but I don't know
the code well enough to be sure.

    2673 
    2674 out:
    2675         up_write(&ndev->reslock);
--> 2676         return err;
    2677 }

regards,
dan carpenter
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [bug report] vdpa/mlx5: Support different address spaces for control and data

[Michael S. Tsirkin]

On Thu, Aug 11, 2022 at 12:40:09PM +0000, Eli Cohen wrote:
> > From: Dan Carpenter <dan.carpenter@oracle.com>
> > Sent: Thursday, August 11, 2022 1:40 PM
> > To: Eli Cohen <elic@nvidia.com>
> > Cc: virtualization@lists.linux-foundation.org
> > Subject: [bug report] vdpa/mlx5: Support different address spaces for control and data
> > 
> > Hello Eli Cohen,
> > 
> > The patch d5358cd0e369: "vdpa/mlx5: Support different address spaces
> > for control and data" from Jul 14, 2022, leads to the following
> > Smatch static checker warning:
> > 
> > 	drivers/vdpa/mlx5/net/mlx5_vnet.c:2676 mlx5_vdpa_set_map()
> > 	error: uninitialized symbol 'err'.
> > 
> > drivers/vdpa/mlx5/net/mlx5_vnet.c
> >     2657 static int mlx5_vdpa_set_map(struct vdpa_device *vdev, unsigned int asid,
> >     2658                              struct vhost_iotlb *iotlb)
> >     2659 {
> >     2660         struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> >     2661         struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> >     2662         int err;
> >     2663
> >     2664         down_write(&ndev->reslock);
> >     2665         if (mvdev->group2asid[MLX5_VDPA_DATAVQ_GROUP] == asid) {
> >     2666                 err = set_map_data(mvdev, iotlb);
> >     2667                 if (err)
> >     2668                         goto out;
> >     2669         }
> >     2670
> >     2671         if (mvdev->group2asid[MLX5_VDPA_CVQ_GROUP] == asid)
> >     2672                 err = set_map_control(mvdev, iotlb);
> > 
> > err not initialized on else path.  My guess is that one or both of these
> > conditions has to be true and this is a false positive but I don't know
> > the code well enough to be sure.
> 
> Thanks for reporting this.
> I think it would be better to return an error if the provided asid is not recognized.
> 
> Therefore I am thinking about adding something like this:
> 
>         if (asid != mvdev->group2asid[MLX5_VDPA_DATAVQ_GROUP] &&
>             asid != mvdev->group2asid[MLX5_VDPA_CVQ_GROUP]) {
>                 err = -EINVAL;
>                 goto out;
>         }

Patch on top pls? Time's running short.


> > 
> >     2673
> >     2674 out:
> >     2675         up_write(&ndev->reslock);
> > --> 2676         return err;
> >     2677 }
> > 
> > regards,
> > dan carpenter

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [bug report] vdpa/mlx5: Support different address spaces for control and data

[Michael S. Tsirkin]

On Thu, Aug 11, 2022 at 12:40:09PM +0000, Eli Cohen wrote:
> > From: Dan Carpenter <dan.carpenter@oracle.com>
> > Sent: Thursday, August 11, 2022 1:40 PM
> > To: Eli Cohen <elic@nvidia.com>
> > Cc: virtualization@lists.linux-foundation.org
> > Subject: [bug report] vdpa/mlx5: Support different address spaces for control and data
> > 
> > Hello Eli Cohen,
> > 
> > The patch d5358cd0e369: "vdpa/mlx5: Support different address spaces
> > for control and data" from Jul 14, 2022, leads to the following
> > Smatch static checker warning:
> > 
> > 	drivers/vdpa/mlx5/net/mlx5_vnet.c:2676 mlx5_vdpa_set_map()
> > 	error: uninitialized symbol 'err'.
> > 
> > drivers/vdpa/mlx5/net/mlx5_vnet.c
> >     2657 static int mlx5_vdpa_set_map(struct vdpa_device *vdev, unsigned int asid,
> >     2658                              struct vhost_iotlb *iotlb)
> >     2659 {
> >     2660         struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> >     2661         struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> >     2662         int err;
> >     2663
> >     2664         down_write(&ndev->reslock);
> >     2665         if (mvdev->group2asid[MLX5_VDPA_DATAVQ_GROUP] == asid) {
> >     2666                 err = set_map_data(mvdev, iotlb);
> >     2667                 if (err)
> >     2668                         goto out;
> >     2669         }
> >     2670
> >     2671         if (mvdev->group2asid[MLX5_VDPA_CVQ_GROUP] == asid)
> >     2672                 err = set_map_control(mvdev, iotlb);
> > 
> > err not initialized on else path.  My guess is that one or both of these
> > conditions has to be true and this is a false positive but I don't know
> > the code well enough to be sure.
> 
> Thanks for reporting this.
> I think it would be better to return an error if the provided asid is not recognized.
> 
> Therefore I am thinking about adding something like this:
> 
>         if (asid != mvdev->group2asid[MLX5_VDPA_DATAVQ_GROUP] &&
>             asid != mvdev->group2asid[MLX5_VDPA_CVQ_GROUP]) {
>                 err = -EINVAL;
>                 goto out;
>         }

I would probably chain the conditions:

if (mvdev->group2asid[MLX5_VDPA_DATAVQ_GROUP] == asid) {
} else if (mvdev->group2asid[MLX5_VDPA_CVQ_GROUP] == asid) {
} else {
                 err = -EINVAL;
                 goto out;
}


or alternatively initialize err with -EINVAL and be done with it.


> > 
> >     2673
> >     2674 out:
> >     2675         up_write(&ndev->reslock);
> > --> 2676         return err;
> >     2677 }
> > 
> > regards,
> > dan carpenter

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [bug report] vdpa/mlx5: Support different address spaces for control and data

[Michael S. Tsirkin]

On Thu, Aug 11, 2022 at 01:11:11PM +0000, Eli Cohen wrote:
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Thursday, August 11, 2022 4:09 PM
> > To: Eli Cohen <elic@nvidia.com>
> > Cc: Dan Carpenter <dan.carpenter@oracle.com>; Jason Wang <jasowang@redhat.com>; Eugenio Perez Martin
> > <eperezma@redhat.com>; virtualization@lists.linux-foundation.org
> > Subject: Re: [bug report] vdpa/mlx5: Support different address spaces for control and data
> > 
> > On Thu, Aug 11, 2022 at 12:40:09PM +0000, Eli Cohen wrote:
> > > > From: Dan Carpenter <dan.carpenter@oracle.com>
> > > > Sent: Thursday, August 11, 2022 1:40 PM
> > > > To: Eli Cohen <elic@nvidia.com>
> > > > Cc: virtualization@lists.linux-foundation.org
> > > > Subject: [bug report] vdpa/mlx5: Support different address spaces for control and data
> > > >
> > > > Hello Eli Cohen,
> > > >
> > > > The patch d5358cd0e369: "vdpa/mlx5: Support different address spaces
> > > > for control and data" from Jul 14, 2022, leads to the following
> > > > Smatch static checker warning:
> > > >
> > > > 	drivers/vdpa/mlx5/net/mlx5_vnet.c:2676 mlx5_vdpa_set_map()
> > > > 	error: uninitialized symbol 'err'.
> > > >
> > > > drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > >     2657 static int mlx5_vdpa_set_map(struct vdpa_device *vdev, unsigned int asid,
> > > >     2658                              struct vhost_iotlb *iotlb)
> > > >     2659 {
> > > >     2660         struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > >     2661         struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > >     2662         int err;
> > > >     2663
> > > >     2664         down_write(&ndev->reslock);
> > > >     2665         if (mvdev->group2asid[MLX5_VDPA_DATAVQ_GROUP] == asid) {
> > > >     2666                 err = set_map_data(mvdev, iotlb);
> > > >     2667                 if (err)
> > > >     2668                         goto out;
> > > >     2669         }
> > > >     2670
> > > >     2671         if (mvdev->group2asid[MLX5_VDPA_CVQ_GROUP] == asid)
> > > >     2672                 err = set_map_control(mvdev, iotlb);
> > > >
> > > > err not initialized on else path.  My guess is that one or both of these
> > > > conditions has to be true and this is a false positive but I don't know
> > > > the code well enough to be sure.
> > >
> > > Thanks for reporting this.
> > > I think it would be better to return an error if the provided asid is not recognized.
> > >
> > > Therefore I am thinking about adding something like this:
> > >
> > >         if (asid != mvdev->group2asid[MLX5_VDPA_DATAVQ_GROUP] &&
> > >             asid != mvdev->group2asid[MLX5_VDPA_CVQ_GROUP]) {
> > >                 err = -EINVAL;
> > >                 goto out;
> > >         }
> > 
> > I would probably chain the conditions:
> > 
> > if (mvdev->group2asid[MLX5_VDPA_DATAVQ_GROUP] == asid) {
> > } else if (mvdev->group2asid[MLX5_VDPA_CVQ_GROUP] == asid) {
> > } else {
> >                  err = -EINVAL;
> >                  goto out;
> > }
> > 
> > 
> > or alternatively initialize err with -EINVAL and be done with it.
> > 
> This makes more sense.
> Will send a patch in an hour.


Just making sure, the only result without this patch is that
and iotlb might be created without a mapping.
But nothing terrible bad will happen things just won't work
but this userspace is already buggy.
Right?

> > 
> > > >
> > > >     2673
> > > >     2674 out:
> > > >     2675         up_write(&ndev->reslock);
> > > > --> 2676         return err;
> > > >     2677 }
> > > >
> > > > regards,
> > > > dan carpenter

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization