[bug report] iommu: iommu_get_group_resv_regions

[bug report] iommu: iommu_get_group_resv_regions

[Dan Carpenter]

Hello Eric Auger,

The patch 6c65fb318e8b: "iommu: iommu_get_group_resv_regions" from
Jan 19, 2017, leads to the following static checker warning:

	drivers/iommu/iommu.c:215 iommu_insert_device_resv_regions()
	error: uninitialized symbol 'ret'.

drivers/iommu/iommu.c
   203  static int
   204  iommu_insert_device_resv_regions(struct list_head *dev_resv_regions,
   205                                   struct list_head *group_resv_regions)
   206  {
   207          struct iommu_resv_region *entry;
   208          int ret;
   209  
   210          list_for_each_entry(entry, dev_resv_regions, list) {
   211                  ret = iommu_insert_resv_region(entry, group_resv_regions);
   212                  if (ret)
   213                          break;
   214          }
   215          return ret;

On the one hand, it probably doesn't make sense that the dev_resv_regions
would ever be empty, but on the other hand, there some code that assumes
it is possible.  What I mean is that iommu_get_resv_regions() can
basically do nothing if ->get_resv_regions() isn't implemented.

I guess we should probably set ret = -EINVAL here?

   216  }
   217  
   218  int iommu_get_group_resv_regions(struct iommu_group *group,
   219                                   struct list_head *head)
   220  {
   221          struct iommu_device *device;
   222          int ret = 0;
   223  
   224          mutex_lock(&group->mutex);
   225          list_for_each_entry(device, &group->devices, list) {
   226                  struct list_head dev_resv_regions;
   227  
   228                  INIT_LIST_HEAD(&dev_resv_regions);
   229                  iommu_get_resv_regions(device->dev, &dev_resv_regions);
   230                  ret = iommu_insert_device_resv_regions(&dev_resv_regions, head);
   231                  iommu_put_resv_regions(device->dev, &dev_resv_regions);
   232                  if (ret)
   233                          break;
   234          }
   235          mutex_unlock(&group->mutex);
   236          return ret;
   237  }
   238  EXPORT_SYMBOL_GPL(iommu_get_group_resv_regions);


regards,
dan carpenter

Re: [bug report] iommu: iommu_get_group_resv_regions

[Auger Eric]

Hi Dan,

On 03/02/2017 10:14, Dan Carpenter wrote:
> Hello Eric Auger,
> 
> The patch 6c65fb318e8b: "iommu: iommu_get_group_resv_regions" from
> Jan 19, 2017, leads to the following static checker warning:
> 
> 	drivers/iommu/iommu.c:215 iommu_insert_device_resv_regions()
> 	error: uninitialized symbol 'ret'.
> 
> drivers/iommu/iommu.c
>    203  static int
>    204  iommu_insert_device_resv_regions(struct list_head *dev_resv_regions,
>    205                                   struct list_head *group_resv_regions)
>    206  {
>    207          struct iommu_resv_region *entry;
>    208          int ret;
>    209  
>    210          list_for_each_entry(entry, dev_resv_regions, list) {
>    211                  ret = iommu_insert_resv_region(entry, group_resv_regions);
>    212                  if (ret)
>    213                          break;
>    214          }
>    215          return ret;
> 
> On the one hand, it probably doesn't make sense that the dev_resv_regions
> would ever be empty, but on the other hand, there some code that assumes
> it is possible.  What I mean is that iommu_get_resv_regions() can
> basically do nothing if ->get_resv_regions() isn't implemented.
> 
> I guess we should probably set ret = -EINVAL here?

we should rather initialize ret to 0. The dev_resv_regions can be void
if a device has no reserved region or if the IOMMU does not implement
the ops. We only return -ENOMEM error if we failed allocating.

Do you want to send the fix or shall I do?

Thanks for reporting

Eric
-
> 
>    216  }
>    217  
>    218  int iommu_get_group_resv_regions(struct iommu_group *group,
>    219                                   struct list_head *head)
>    220  {
>    221          struct iommu_device *device;
>    222          int ret = 0;
>    223  
>    224          mutex_lock(&group->mutex);
>    225          list_for_each_entry(device, &group->devices, list) {
>    226                  struct list_head dev_resv_regions;
>    227  
>    228                  INIT_LIST_HEAD(&dev_resv_regions);
>    229                  iommu_get_resv_regions(device->dev, &dev_resv_regions);
>    230                  ret = iommu_insert_device_resv_regions(&dev_resv_regions, head);
>    231                  iommu_put_resv_regions(device->dev, &dev_resv_regions);
>    232                  if (ret)
>    233                          break;
>    234          }
>    235          mutex_unlock(&group->mutex);
>    236          return ret;
>    237  }
>    238  EXPORT_SYMBOL_GPL(iommu_get_group_resv_regions);
> 
> 
> regards,
> dan carpenter
> 

[patch] iommu: silence an uninintialized variable warning

[Dan Carpenter]

My static checker complains that we return an uninitialized scalar if
the list is empty.  If that's the case then we should return zero.

Fixes: 6c65fb318e8b ("iommu: iommu_get_group_resv_regions")
Signed-off-by: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index b752c3d91cfc..0e1750481ebc 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -210,9 +210,9 @@ iommu_insert_device_resv_regions(struct list_head *dev_resv_regions,
 	list_for_each_entry(entry, dev_resv_regions, list) {
 		ret = iommu_insert_resv_region(entry, group_resv_regions);
 		if (ret)
-			break;
+			return ret;
 	}
-	return ret;
+	return 0;
 }
 
 int iommu_get_group_resv_regions(struct iommu_group *group,

Re: [patch] iommu: silence an uninintialized variable warning

[Dan Carpenter]

Never mind. Eric just sent his own patch.

regards,
dan carpenter