Michael S. Tsirkin
2022-Aug-11 13:21 UTC
[bug report] vdpa/mlx5: Support different address spaces for control and data
On Thu, Aug 11, 2022 at 01:11:11PM +0000, Eli Cohen wrote:> > From: Michael S. Tsirkin <mst at redhat.com> > > Sent: Thursday, August 11, 2022 4:09 PM > > To: Eli Cohen <elic at nvidia.com> > > Cc: Dan Carpenter <dan.carpenter at oracle.com>; Jason Wang <jasowang at redhat.com>; Eugenio Perez Martin > > <eperezma at redhat.com>; virtualization at 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 at oracle.com> > > > > Sent: Thursday, August 11, 2022 1:40 PM > > > > To: Eli Cohen <elic at nvidia.com> > > > > Cc: virtualization at 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