[Intel-gfx] [bug report] drm/i915/gvt/kvmgt: add vfio/mdev support to KVMGT

[Jike Song]

On 01/26/2017 02:50 PM, Dan Carpenter wrote:
> Hello Jike Song,
> 
> The patch 659643f7d814: "drm/i915/gvt/kvmgt: add vfio/mdev support to
> KVMGT" from Dec 8, 2016, leads to the following static checker
> warning:
> 
> 	drivers/gpu/drm/i915/gvt/kvmgt.c:969 intel_vgpu_ioctl()
> 	warn: calling kfree() when 'caps.buf' is always NULL.
> 
> drivers/gpu/drm/i915/gvt/kvmgt.c
>    909          } else if (cmd == VFIO_DEVICE_GET_REGION_INFO) {
>    910                  struct vfio_region_info info;
>    911                  struct vfio_info_cap caps = { .buf = NULL, .size = 0 };
>                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Set here.
> 
>    912                  int i, ret;
>    913                  struct vfio_region_info_cap_sparse_mmap *sparse = NULL;
>    914                  size_t size;
>    915                  int nr_areas = 1;
>    916                  int cap_type_id;
>    917  
>    918                  minsz = offsetofend(struct vfio_region_info, offset);
>    919  
>    920                  if (copy_from_user(&info, (void __user *)arg, minsz))
>    921                          return -EFAULT;
>    922  
>    923                  if (info.argsz < minsz)
>    924                          return -EINVAL;
>    925  
>    926                  switch (info.index) {
>    927                  case VFIO_PCI_CONFIG_REGION_INDEX:
>    928                          info.offset = VFIO_PCI_INDEX_TO_OFFSET(info.index);
>    929                          info.size = INTEL_GVT_MAX_CFG_SPACE_SZ;
>    930                          info.flags = VFIO_REGION_INFO_FLAG_READ |
>    931                                       VFIO_REGION_INFO_FLAG_WRITE;
>    932                          break;
>    933                  case VFIO_PCI_BAR0_REGION_INDEX:
>    934                          info.offset = VFIO_PCI_INDEX_TO_OFFSET(info.index);
>    935                          info.size = vgpu->cfg_space.bar[info.index].size;
>    936                          if (!info.size) {
>    937                                  info.flags = 0;
>    938                                  break;
>    939                          }
>    940  
>    941                          info.flags = VFIO_REGION_INFO_FLAG_READ |
>    942                                       VFIO_REGION_INFO_FLAG_WRITE;
>    943                          break;
>    944                  case VFIO_PCI_BAR1_REGION_INDEX:
>    945                          info.offset = VFIO_PCI_INDEX_TO_OFFSET(info.index);
>    946                          info.size = 0;
>    947                          info.flags = 0;
>    948                          break;
>    949                  case VFIO_PCI_BAR2_REGION_INDEX:
>    950                          info.offset = VFIO_PCI_INDEX_TO_OFFSET(info.index);
>    951                          info.flags = VFIO_REGION_INFO_FLAG_CAPS |
>    952                                          VFIO_REGION_INFO_FLAG_MMAP |
>    953                                          VFIO_REGION_INFO_FLAG_READ |
>    954                                          VFIO_REGION_INFO_FLAG_WRITE;
>    955                          info.size = gvt_aperture_sz(vgpu->gvt);
>    956  
>    957                          size = sizeof(*sparse) +
>    958                                          (nr_areas * sizeof(*sparse->areas));
>    959                          sparse = kzalloc(size, GFP_KERNEL);
>    960                          if (!sparse)
>    961                                  return -ENOMEM;
>    962  
>    963                          sparse->nr_areas = nr_areas;
>    964                          cap_type_id = VFIO_REGION_INFO_CAP_SPARSE_MMAP;
>    965                          sparse->areas[0].offset =
>    966                                          PAGE_ALIGN(vgpu_aperture_offset(vgpu));
>    967                          sparse->areas[0].size = vgpu_aperture_sz(vgpu);
>    968                          if (!caps.buf) {
>                                      ^^^^^^^^
> It's always NULL.
> 
>    969                                  kfree(caps.buf);
> 
> Freeing NULL is pointless.
> 
>    970                                  caps.buf = NULL;
>    971                                  caps.size = 0;
> 
> These were already zeroed out at the start of the function.  What was
> intended here?  Probably you could just delete these lines.
> 

Hi Dan,

Yes you are right, these are useless, probably a mistake in the development.

Curious what static checker do you use? I actually checked it with sparse but
this was not reported.

I'll submit a patch to remove that, thanks!


--
Thanks,
Jike

>    972                          }
>    973                          break;
>    974  
>    975                  case VFIO_PCI_BAR3_REGION_INDEX ... VFIO_PCI_BAR5_REGION_INDEX:
> 
> regards,
> dan carpenter
>