Re: [PATCH v4 3/5] media: Refactor copying IOCTL arguments from and to user space

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Thursday 11 Aug 2016 23:29:16 Sakari Ailus wrote:
> Refactor copying the IOCTL argument structs from the user space and back,
> in order to reduce code copied around and make the implementation more
> robust.
> 
> As a result, the copying is done while not holding the graph mutex.
> 
> Signed-off-by: Sakari Ailus 
> Acked-by: Hans Verkuil 

Acked-by: Laurent Pinchart 

> ---
>  drivers/media/media-device.c | 190 +--
>  1 file changed, 90 insertions(+), 100 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index cc6c566..d1b47a4 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -59,27 +59,24 @@ static int media_device_close(struct file *filp)
>  }
> 
>  static int media_device_get_info(struct media_device *dev,
> -  struct media_device_info __user *__info)
> +  struct media_device_info *info)
>  {
> - struct media_device_info info;
> -
> - memset(, 0, sizeof(info));
> + memset(info, 0, sizeof(*info));
> 
>   if (dev->driver_name[0])
> - strlcpy(info.driver, dev->driver_name, sizeof(info.driver));
> + strlcpy(info->driver, dev->driver_name, sizeof(info->driver));
>   else
> - strlcpy(info.driver, dev->dev->driver->name, 
sizeof(info.driver));
> + strlcpy(info->driver, dev->dev->driver->name,
> + sizeof(info->driver));
> 
> - strlcpy(info.model, dev->model, sizeof(info.model));
> - strlcpy(info.serial, dev->serial, sizeof(info.serial));
> - strlcpy(info.bus_info, dev->bus_info, sizeof(info.bus_info));
> + strlcpy(info->model, dev->model, sizeof(info->model));
> + strlcpy(info->serial, dev->serial, sizeof(info->serial));
> + strlcpy(info->bus_info, dev->bus_info, sizeof(info->bus_info));
> 
> - info.media_version = MEDIA_API_VERSION;
> - info.hw_revision = dev->hw_revision;
> - info.driver_version = dev->driver_version;
> + info->media_version = MEDIA_API_VERSION;
> + info->hw_revision = dev->hw_revision;
> + info->driver_version = dev->driver_version;
> 
> - if (copy_to_user(__info, , sizeof(*__info)))
> - return -EFAULT;
>   return 0;
>  }
> 
> @@ -101,29 +98,25 @@ static struct media_entity *find_entity(struct
> media_device *mdev, u32 id) }
> 
>  static long media_device_enum_entities(struct media_device *mdev,
> -struct media_entity_desc __user *uent)
> +struct media_entity_desc *entd)
>  {
>   struct media_entity *ent;
> - struct media_entity_desc u_ent;
> -
> - memset(_ent, 0, sizeof(u_ent));
> - if (copy_from_user(_ent.id, >id, sizeof(u_ent.id)))
> - return -EFAULT;
> -
> - ent = find_entity(mdev, u_ent.id);
> 
> + ent = find_entity(mdev, entd->id);
>   if (ent == NULL)
>   return -EINVAL;
> 
> - u_ent.id = media_entity_id(ent);
> + memset(entd, 0, sizeof(*entd));
> +
> + entd->id = media_entity_id(ent);
>   if (ent->name)
> - strlcpy(u_ent.name, ent->name, sizeof(u_ent.name));
> - u_ent.type = ent->function;
> - u_ent.revision = 0; /* Unused */
> - u_ent.flags = ent->flags;
> - u_ent.group_id = 0; /* Unused */
> - u_ent.pads = ent->num_pads;
> - u_ent.links = ent->num_links - ent->num_backlinks;
> + strlcpy(entd->name, ent->name, sizeof(entd->name));
> + entd->type = ent->function;
> + entd->revision = 0; /* Unused */
> + entd->flags = ent->flags;
> + entd->group_id = 0; /* Unused */
> + entd->pads = ent->num_pads;
> + entd->links = ent->num_links - ent->num_backlinks;
> 
>   /*
>* Workaround for a bug at media-ctl <= v1.10 that makes it to
> @@ -139,14 +132,13 @@ static long media_device_enum_entities(struct
> media_device *mdev, if (ent->function < MEDIA_ENT_F_OLD_BASE ||
>   ent->function > MEDIA_ENT_T_DEVNODE_UNKNOWN) {
>   if (is_media_entity_v4l2_subdev(ent))
> - u_ent.type = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
> + entd->type = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
>   else if (ent->function != MEDIA_ENT_F_IO_V4L)
> - u_ent.type = MEDIA_ENT_T_DEVNODE_UNKNOWN;
> + entd->type = MEDIA_ENT_T_DEVNODE_UNKNOWN;
>   }
> 
> - memcpy(_ent.raw, >info, sizeof(ent->info));
> - if (copy_to_user(uent, _ent, sizeof(u_ent)))
> - return -EFAULT;
> + memcpy(>raw, >info, sizeof(ent->info));
> +
>   return 0;
>  }
> 
> @@ -158,8 +150,8 @@ static void media_device_kpad_to_upad(const struct
> media_pad *kpad, upad->flags = kpad->flags;
>  }
> 
> -static long 

[PATCH v4 3/5] media: Refactor copying IOCTL arguments from and to user space

[Sakari Ailus]

Refactor copying the IOCTL argument structs from the user space and back,
in order to reduce code copied around and make the implementation more
robust.

As a result, the copying is done while not holding the graph mutex.

Signed-off-by: Sakari Ailus 
Acked-by: Hans Verkuil 
---
 drivers/media/media-device.c | 190 ---
 1 file changed, 90 insertions(+), 100 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index cc6c566..d1b47a4 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -59,27 +59,24 @@ static int media_device_close(struct file *filp)
 }
 
 static int media_device_get_info(struct media_device *dev,
-struct media_device_info __user *__info)
+struct media_device_info *info)
 {
-   struct media_device_info info;
-
-   memset(, 0, sizeof(info));
+   memset(info, 0, sizeof(*info));
 
if (dev->driver_name[0])
-   strlcpy(info.driver, dev->driver_name, sizeof(info.driver));
+   strlcpy(info->driver, dev->driver_name, sizeof(info->driver));
else
-   strlcpy(info.driver, dev->dev->driver->name, 
sizeof(info.driver));
+   strlcpy(info->driver, dev->dev->driver->name,
+   sizeof(info->driver));
 
-   strlcpy(info.model, dev->model, sizeof(info.model));
-   strlcpy(info.serial, dev->serial, sizeof(info.serial));
-   strlcpy(info.bus_info, dev->bus_info, sizeof(info.bus_info));
+   strlcpy(info->model, dev->model, sizeof(info->model));
+   strlcpy(info->serial, dev->serial, sizeof(info->serial));
+   strlcpy(info->bus_info, dev->bus_info, sizeof(info->bus_info));
 
-   info.media_version = MEDIA_API_VERSION;
-   info.hw_revision = dev->hw_revision;
-   info.driver_version = dev->driver_version;
+   info->media_version = MEDIA_API_VERSION;
+   info->hw_revision = dev->hw_revision;
+   info->driver_version = dev->driver_version;
 
-   if (copy_to_user(__info, , sizeof(*__info)))
-   return -EFAULT;
return 0;
 }
 
@@ -101,29 +98,25 @@ static struct media_entity *find_entity(struct 
media_device *mdev, u32 id)
 }
 
 static long media_device_enum_entities(struct media_device *mdev,
-  struct media_entity_desc __user *uent)
+  struct media_entity_desc *entd)
 {
struct media_entity *ent;
-   struct media_entity_desc u_ent;
-
-   memset(_ent, 0, sizeof(u_ent));
-   if (copy_from_user(_ent.id, >id, sizeof(u_ent.id)))
-   return -EFAULT;
-
-   ent = find_entity(mdev, u_ent.id);
 
+   ent = find_entity(mdev, entd->id);
if (ent == NULL)
return -EINVAL;
 
-   u_ent.id = media_entity_id(ent);
+   memset(entd, 0, sizeof(*entd));
+
+   entd->id = media_entity_id(ent);
if (ent->name)
-   strlcpy(u_ent.name, ent->name, sizeof(u_ent.name));
-   u_ent.type = ent->function;
-   u_ent.revision = 0; /* Unused */
-   u_ent.flags = ent->flags;
-   u_ent.group_id = 0; /* Unused */
-   u_ent.pads = ent->num_pads;
-   u_ent.links = ent->num_links - ent->num_backlinks;
+   strlcpy(entd->name, ent->name, sizeof(entd->name));
+   entd->type = ent->function;
+   entd->revision = 0; /* Unused */
+   entd->flags = ent->flags;
+   entd->group_id = 0; /* Unused */
+   entd->pads = ent->num_pads;
+   entd->links = ent->num_links - ent->num_backlinks;
 
/*
 * Workaround for a bug at media-ctl <= v1.10 that makes it to
@@ -139,14 +132,13 @@ static long media_device_enum_entities(struct 
media_device *mdev,
if (ent->function < MEDIA_ENT_F_OLD_BASE ||
ent->function > MEDIA_ENT_T_DEVNODE_UNKNOWN) {
if (is_media_entity_v4l2_subdev(ent))
-   u_ent.type = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
+   entd->type = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
else if (ent->function != MEDIA_ENT_F_IO_V4L)
-   u_ent.type = MEDIA_ENT_T_DEVNODE_UNKNOWN;
+   entd->type = MEDIA_ENT_T_DEVNODE_UNKNOWN;
}
 
-   memcpy(_ent.raw, >info, sizeof(ent->info));
-   if (copy_to_user(uent, _ent, sizeof(u_ent)))
-   return -EFAULT;
+   memcpy(>raw, >info, sizeof(ent->info));
+
return 0;
 }
 
@@ -158,8 +150,8 @@ static void media_device_kpad_to_upad(const struct 
media_pad *kpad,
upad->flags = kpad->flags;
 }
 
-static long __media_device_enum_links(struct media_device *mdev,
- struct media_links_enum *links)
+static long media_device_enum_links(struct media_device *mdev,
+   struct