[PATCH v3 0/5] Refactor media IOCTL handling, add variable length arguments

[PATCH v3 0/5] Refactor media IOCTL handling, add variable length arguments

[Sakari Ailus]

Hi,

This is the third version of the media IOCTL handling rework set. What's
changed since v2:

patch 3:

- Remove function to calculate maximum argument size, replace by a char         
  array of 256 or kmalloc() if that's too small.                                

- info->arg_from_user() may fail. Check the return code.                        

- Instead of providing a no-operation of a copy function, check whether one is  
  defined. If not, don't call one.                                              

patch 4:

- Arrange the flags field next to cmd, which is an integer. This avoids         
  creating extra holes in the struct memory layout.                             

patch 5:

- Use a list of supported argument sizes instead of a minimum value.

---

The patches themselves have been reworked so I don't detail the changes         
in this set. What's noteworthy however is that the set adds support for         
variable length IOCTL arguments.                                                
                                                                                
(The motivation for these patches is having found myself pondering whether      
to have nine or thirteen reserved fields for the request IOCTL. I decided       
to address the problem instead. If this is found workable on the media          
controller we could follow the same model on V4L2.)                             

-- 
Kind regards,
Sakari

[PATCH v3 1/5] media: Determine early whether an IOCTL is supported

[Sakari Ailus]

Preparation for refactoring media IOCTL handling to unify common parts.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/media-device.c | 48 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 46 insertions(+), 2 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 1795abe..3ac526d 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -419,13 +419,33 @@ static long media_device_get_topology(struct media_device *mdev,
 	return 0;
 }
 
-static long media_device_ioctl(struct file *filp, unsigned int cmd,
-			       unsigned long arg)
+#define MEDIA_IOC(__cmd) \
+	[_IOC_NR(MEDIA_IOC_##__cmd)] = { .cmd = MEDIA_IOC_##__cmd }
+
+/* the table is indexed by _IOC_NR(cmd) */
+struct media_ioctl_info {
+	unsigned int cmd;
+};
+
+static inline long is_valid_ioctl(const struct media_ioctl_info *info,
+				  unsigned int len, unsigned int cmd)
+{
+	return (_IOC_NR(cmd) >= len
+		|| info[_IOC_NR(cmd)].cmd != cmd) ? -ENOIOCTLCMD : 0;
+}
+
+static long __media_device_ioctl(
+	struct file *filp, unsigned int cmd, void __user *arg,
+	const struct media_ioctl_info *info_array, unsigned int info_array_len)
 {
 	struct media_devnode *devnode = media_devnode_data(filp);
 	struct media_device *dev = devnode->media_dev;
 	long ret;
 
+	ret = is_valid_ioctl(info_array, info_array_len, cmd);
+	if (ret)
+		return ret;
+
 	mutex_lock(&dev->graph_mutex);
 	switch (cmd) {
 	case MEDIA_IOC_DEVICE_INFO:
@@ -461,6 +481,22 @@ static long media_device_ioctl(struct file *filp, unsigned int cmd,
 	return ret;
 }
 
+static const struct media_ioctl_info ioctl_info[] = {
+	MEDIA_IOC(DEVICE_INFO),
+	MEDIA_IOC(ENUM_ENTITIES),
+	MEDIA_IOC(ENUM_LINKS),
+	MEDIA_IOC(SETUP_LINK),
+	MEDIA_IOC(G_TOPOLOGY),
+};
+
+static long media_device_ioctl(struct file *filp, unsigned int cmd,
+			       unsigned long arg)
+{
+	return __media_device_ioctl(
+		filp, cmd, (void __user *)arg,
+		ioctl_info, ARRAY_SIZE(ioctl_info));
+}
+
 #ifdef CONFIG_COMPAT
 
 struct media_links_enum32 {
@@ -491,6 +527,14 @@ static long media_device_enum_links32(struct media_device *mdev,
 
 #define MEDIA_IOC_ENUM_LINKS32		_IOWR('|', 0x02, struct media_links_enum32)
 
+static const struct media_ioctl_info compat_ioctl_info[] = {
+	MEDIA_IOC(DEVICE_INFO),
+	MEDIA_IOC(ENUM_ENTITIES),
+	MEDIA_IOC(ENUM_LINKS32),
+	MEDIA_IOC(SETUP_LINK),
+	MEDIA_IOC(G_TOPOLOGY),
+};
+
 static long media_device_compat_ioctl(struct file *filp, unsigned int cmd,
 				      unsigned long arg)
 {
-- 
2.7.4

Re: [PATCH v3 1/5] media: Determine early whether an IOCTL is supported

[Hans Verkuil]

Hi Sakari,

On 07/21/2016 01:14 PM, Sakari Ailus wrote:
> Preparation for refactoring media IOCTL handling to unify common parts.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/media-device.c | 48 ++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 46 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 1795abe..3ac526d 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -419,13 +419,33 @@ static long media_device_get_topology(struct media_device *mdev,
>  	return 0;
>  }
>  
> -static long media_device_ioctl(struct file *filp, unsigned int cmd,
> -			       unsigned long arg)
> +#define MEDIA_IOC(__cmd) \
> +	[_IOC_NR(MEDIA_IOC_##__cmd)] = { .cmd = MEDIA_IOC_##__cmd }
> +
> +/* the table is indexed by _IOC_NR(cmd) */
> +struct media_ioctl_info {
> +	unsigned int cmd;
> +};
> +
> +static inline long is_valid_ioctl(const struct media_ioctl_info *info,
> +				  unsigned int len, unsigned int cmd)
> +{
> +	return (_IOC_NR(cmd) >= len
> +		|| info[_IOC_NR(cmd)].cmd != cmd) ? -ENOIOCTLCMD : 0;
> +}
> +
> +static long __media_device_ioctl(
> +	struct file *filp, unsigned int cmd, void __user *arg,
> +	const struct media_ioctl_info *info_array, unsigned int info_array_len)
>  {
>  	struct media_devnode *devnode = media_devnode_data(filp);
>  	struct media_device *dev = devnode->media_dev;
>  	long ret;
>  
> +	ret = is_valid_ioctl(info_array, info_array_len, cmd);
> +	if (ret)
> +		return ret;
> +
>  	mutex_lock(&dev->graph_mutex);
>  	switch (cmd) {
>  	case MEDIA_IOC_DEVICE_INFO:
> @@ -461,6 +481,22 @@ static long media_device_ioctl(struct file *filp, unsigned int cmd,
>  	return ret;
>  }
>  
> +static const struct media_ioctl_info ioctl_info[] = {
> +	MEDIA_IOC(DEVICE_INFO),
> +	MEDIA_IOC(ENUM_ENTITIES),
> +	MEDIA_IOC(ENUM_LINKS),
> +	MEDIA_IOC(SETUP_LINK),
> +	MEDIA_IOC(G_TOPOLOGY),
> +};

Why not move this up and use ARRAY_SIZE instead of having to pass the length around?

> +
> +static long media_device_ioctl(struct file *filp, unsigned int cmd,
> +			       unsigned long arg)
> +{
> +	return __media_device_ioctl(
> +		filp, cmd, (void __user *)arg,
> +		ioctl_info, ARRAY_SIZE(ioctl_info));
> +}
> +
>  #ifdef CONFIG_COMPAT
>  
>  struct media_links_enum32 {
> @@ -491,6 +527,14 @@ static long media_device_enum_links32(struct media_device *mdev,
>  
>  #define MEDIA_IOC_ENUM_LINKS32		_IOWR('|', 0x02, struct media_links_enum32)
>  
> +static const struct media_ioctl_info compat_ioctl_info[] = {
> +	MEDIA_IOC(DEVICE_INFO),
> +	MEDIA_IOC(ENUM_ENTITIES),
> +	MEDIA_IOC(ENUM_LINKS32),
> +	MEDIA_IOC(SETUP_LINK),
> +	MEDIA_IOC(G_TOPOLOGY),
> +};

I assume the size of the compat array will always be the same as that of the 'regular' array.
In fact, you should probably test for that (the compiler should be able to catch that).

> +
>  static long media_device_compat_ioctl(struct file *filp, unsigned int cmd,
>  				      unsigned long arg)
>  {
> 

Regards,

	Hans

Re: [PATCH v3 1/5] media: Determine early whether an IOCTL is supported

[Sakari Ailus]

Hi Hans,

Hans Verkuil wrote:
> Hi Sakari,
>
> On 07/21/2016 01:14 PM, Sakari Ailus wrote:
>> Preparation for refactoring media IOCTL handling to unify common parts.
>>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>> ---
>>   drivers/media/media-device.c | 48 ++++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 46 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>> index 1795abe..3ac526d 100644
>> --- a/drivers/media/media-device.c
>> +++ b/drivers/media/media-device.c
>> @@ -419,13 +419,33 @@ static long media_device_get_topology(struct media_device *mdev,
>>   	return 0;
>>   }
>>
>> -static long media_device_ioctl(struct file *filp, unsigned int cmd,
>> -			       unsigned long arg)
>> +#define MEDIA_IOC(__cmd) \
>> +	[_IOC_NR(MEDIA_IOC_##__cmd)] = { .cmd = MEDIA_IOC_##__cmd }
>> +
>> +/* the table is indexed by _IOC_NR(cmd) */
>> +struct media_ioctl_info {
>> +	unsigned int cmd;
>> +};
>> +
>> +static inline long is_valid_ioctl(const struct media_ioctl_info *info,
>> +				  unsigned int len, unsigned int cmd)
>> +{
>> +	return (_IOC_NR(cmd) >= len
>> +		|| info[_IOC_NR(cmd)].cmd != cmd) ? -ENOIOCTLCMD : 0;
>> +}
>> +
>> +static long __media_device_ioctl(
>> +	struct file *filp, unsigned int cmd, void __user *arg,
>> +	const struct media_ioctl_info *info_array, unsigned int info_array_len)
>>   {
>>   	struct media_devnode *devnode = media_devnode_data(filp);
>>   	struct media_device *dev = devnode->media_dev;
>>   	long ret;
>>
>> +	ret = is_valid_ioctl(info_array, info_array_len, cmd);
>> +	if (ret)
>> +		return ret;
>> +
>>   	mutex_lock(&dev->graph_mutex);
>>   	switch (cmd) {
>>   	case MEDIA_IOC_DEVICE_INFO:
>> @@ -461,6 +481,22 @@ static long media_device_ioctl(struct file *filp, unsigned int cmd,
>>   	return ret;
>>   }
>>
>> +static const struct media_ioctl_info ioctl_info[] = {
>> +	MEDIA_IOC(DEVICE_INFO),
>> +	MEDIA_IOC(ENUM_ENTITIES),
>> +	MEDIA_IOC(ENUM_LINKS),
>> +	MEDIA_IOC(SETUP_LINK),
>> +	MEDIA_IOC(G_TOPOLOGY),
>> +};
>
> Why not move this up and use ARRAY_SIZE instead of having to pass the length around?
>
>> +
>> +static long media_device_ioctl(struct file *filp, unsigned int cmd,
>> +			       unsigned long arg)
>> +{
>> +	return __media_device_ioctl(
>> +		filp, cmd, (void __user *)arg,
>> +		ioctl_info, ARRAY_SIZE(ioctl_info));
>> +}
>> +
>>   #ifdef CONFIG_COMPAT
>>
>>   struct media_links_enum32 {
>> @@ -491,6 +527,14 @@ static long media_device_enum_links32(struct media_device *mdev,
>>
>>   #define MEDIA_IOC_ENUM_LINKS32		_IOWR('|', 0x02, struct media_links_enum32)
>>
>> +static const struct media_ioctl_info compat_ioctl_info[] = {
>> +	MEDIA_IOC(DEVICE_INFO),
>> +	MEDIA_IOC(ENUM_ENTITIES),
>> +	MEDIA_IOC(ENUM_LINKS32),
>> +	MEDIA_IOC(SETUP_LINK),
>> +	MEDIA_IOC(G_TOPOLOGY),
>> +};
>
> I assume the size of the compat array will always be the same as that of the 'regular' array.
> In fact, you should probably test for that (the compiler should be able to catch that).

Yeah, the sizes will be the same, so ARRAY_SIZE() works well. I'll add a 
BUILD_BUG_ON() check for the sizes.

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

[PATCH v3 2/5] media: Unify IOCTL handler calling

[Sakari Ailus]

Each IOCTL handler can be listed in an array instead of using a large and
cumbersome switch. Do that.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/media-device.c | 81 +++++++++++++-------------------------------
 1 file changed, 23 insertions(+), 58 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 3ac526d..6fd9b77 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -419,12 +419,16 @@ static long media_device_get_topology(struct media_device *mdev,
 	return 0;
 }
 
-#define MEDIA_IOC(__cmd) \
-	[_IOC_NR(MEDIA_IOC_##__cmd)] = { .cmd = MEDIA_IOC_##__cmd }
+#define MEDIA_IOC(__cmd, func)						\
+	[_IOC_NR(MEDIA_IOC_##__cmd)] = {				\
+		.cmd = MEDIA_IOC_##__cmd,				\
+		.fn = (long (*)(struct media_device *, void __user *))func,    \
+	}
 
 /* the table is indexed by _IOC_NR(cmd) */
 struct media_ioctl_info {
 	unsigned int cmd;
+	long (*fn)(struct media_device *dev, void __user *arg);
 };
 
 static inline long is_valid_ioctl(const struct media_ioctl_info *info,
@@ -440,53 +444,28 @@ static long __media_device_ioctl(
 {
 	struct media_devnode *devnode = media_devnode_data(filp);
 	struct media_device *dev = devnode->media_dev;
+	const struct media_ioctl_info *info;
 	long ret;
 
 	ret = is_valid_ioctl(info_array, info_array_len, cmd);
 	if (ret)
 		return ret;
 
+	info = &info_array[_IOC_NR(cmd)];
+
 	mutex_lock(&dev->graph_mutex);
-	switch (cmd) {
-	case MEDIA_IOC_DEVICE_INFO:
-		ret = media_device_get_info(dev,
-				(struct media_device_info __user *)arg);
-		break;
-
-	case MEDIA_IOC_ENUM_ENTITIES:
-		ret = media_device_enum_entities(dev,
-				(struct media_entity_desc __user *)arg);
-		break;
-
-	case MEDIA_IOC_ENUM_LINKS:
-		ret = media_device_enum_links(dev,
-				(struct media_links_enum __user *)arg);
-		break;
-
-	case MEDIA_IOC_SETUP_LINK:
-		ret = media_device_setup_link(dev,
-				(struct media_link_desc __user *)arg);
-		break;
-
-	case MEDIA_IOC_G_TOPOLOGY:
-		ret = media_device_get_topology(dev,
-				(struct media_v2_topology __user *)arg);
-		break;
-
-	default:
-		ret = -ENOIOCTLCMD;
-	}
+	ret = info->fn(dev, arg);
 	mutex_unlock(&dev->graph_mutex);
 
 	return ret;
 }
 
 static const struct media_ioctl_info ioctl_info[] = {
-	MEDIA_IOC(DEVICE_INFO),
-	MEDIA_IOC(ENUM_ENTITIES),
-	MEDIA_IOC(ENUM_LINKS),
-	MEDIA_IOC(SETUP_LINK),
-	MEDIA_IOC(G_TOPOLOGY),
+	MEDIA_IOC(DEVICE_INFO, media_device_get_info),
+	MEDIA_IOC(ENUM_ENTITIES, media_device_enum_entities),
+	MEDIA_IOC(ENUM_LINKS, media_device_enum_links),
+	MEDIA_IOC(SETUP_LINK, media_device_setup_link),
+	MEDIA_IOC(G_TOPOLOGY, media_device_get_topology),
 };
 
 static long media_device_ioctl(struct file *filp, unsigned int cmd,
@@ -528,33 +507,19 @@ static long media_device_enum_links32(struct media_device *mdev,
 #define MEDIA_IOC_ENUM_LINKS32		_IOWR('|', 0x02, struct media_links_enum32)
 
 static const struct media_ioctl_info compat_ioctl_info[] = {
-	MEDIA_IOC(DEVICE_INFO),
-	MEDIA_IOC(ENUM_ENTITIES),
-	MEDIA_IOC(ENUM_LINKS32),
-	MEDIA_IOC(SETUP_LINK),
-	MEDIA_IOC(G_TOPOLOGY),
+	MEDIA_IOC(DEVICE_INFO, media_device_get_info),
+	MEDIA_IOC(ENUM_ENTITIES, media_device_enum_entities),
+	MEDIA_IOC(ENUM_LINKS32, media_device_enum_links32),
+	MEDIA_IOC(SETUP_LINK, media_device_setup_link),
+	MEDIA_IOC(G_TOPOLOGY, media_device_get_topology),
 };
 
 static long media_device_compat_ioctl(struct file *filp, unsigned int cmd,
 				      unsigned long arg)
 {
-	struct media_devnode *devnode = media_devnode_data(filp);
-	struct media_device *dev = devnode->media_dev;
-	long ret;
-
-	switch (cmd) {
-	case MEDIA_IOC_ENUM_LINKS32:
-		mutex_lock(&dev->graph_mutex);
-		ret = media_device_enum_links32(dev,
-				(struct media_links_enum32 __user *)arg);
-		mutex_unlock(&dev->graph_mutex);
-		break;
-
-	default:
-		return media_device_ioctl(filp, cmd, arg);
-	}
-
-	return ret;
+	return __media_device_ioctl(
+		filp, cmd, (void __user *)arg,
+		compat_ioctl_info, ARRAY_SIZE(compat_ioctl_info));
 }
 #endif /* CONFIG_COMPAT */
 
-- 
2.7.4

Re: [PATCH v3 2/5] media: Unify IOCTL handler calling

[Hans Verkuil]

On 07/21/2016 01:14 PM, Sakari Ailus wrote:
> Each IOCTL handler can be listed in an array instead of using a large and
> cumbersome switch. Do that.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Thanks!

	Hans

> ---
>  drivers/media/media-device.c | 81 +++++++++++++-------------------------------
>  1 file changed, 23 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 3ac526d..6fd9b77 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -419,12 +419,16 @@ static long media_device_get_topology(struct media_device *mdev,
>  	return 0;
>  }
>  
> -#define MEDIA_IOC(__cmd) \
> -	[_IOC_NR(MEDIA_IOC_##__cmd)] = { .cmd = MEDIA_IOC_##__cmd }
> +#define MEDIA_IOC(__cmd, func)						\
> +	[_IOC_NR(MEDIA_IOC_##__cmd)] = {				\
> +		.cmd = MEDIA_IOC_##__cmd,				\
> +		.fn = (long (*)(struct media_device *, void __user *))func,    \
> +	}
>  
>  /* the table is indexed by _IOC_NR(cmd) */
>  struct media_ioctl_info {
>  	unsigned int cmd;
> +	long (*fn)(struct media_device *dev, void __user *arg);
>  };
>  
>  static inline long is_valid_ioctl(const struct media_ioctl_info *info,
> @@ -440,53 +444,28 @@ static long __media_device_ioctl(
>  {
>  	struct media_devnode *devnode = media_devnode_data(filp);
>  	struct media_device *dev = devnode->media_dev;
> +	const struct media_ioctl_info *info;
>  	long ret;
>  
>  	ret = is_valid_ioctl(info_array, info_array_len, cmd);
>  	if (ret)
>  		return ret;
>  
> +	info = &info_array[_IOC_NR(cmd)];
> +
>  	mutex_lock(&dev->graph_mutex);
> -	switch (cmd) {
> -	case MEDIA_IOC_DEVICE_INFO:
> -		ret = media_device_get_info(dev,
> -				(struct media_device_info __user *)arg);
> -		break;
> -
> -	case MEDIA_IOC_ENUM_ENTITIES:
> -		ret = media_device_enum_entities(dev,
> -				(struct media_entity_desc __user *)arg);
> -		break;
> -
> -	case MEDIA_IOC_ENUM_LINKS:
> -		ret = media_device_enum_links(dev,
> -				(struct media_links_enum __user *)arg);
> -		break;
> -
> -	case MEDIA_IOC_SETUP_LINK:
> -		ret = media_device_setup_link(dev,
> -				(struct media_link_desc __user *)arg);
> -		break;
> -
> -	case MEDIA_IOC_G_TOPOLOGY:
> -		ret = media_device_get_topology(dev,
> -				(struct media_v2_topology __user *)arg);
> -		break;
> -
> -	default:
> -		ret = -ENOIOCTLCMD;
> -	}
> +	ret = info->fn(dev, arg);
>  	mutex_unlock(&dev->graph_mutex);
>  
>  	return ret;
>  }
>  
>  static const struct media_ioctl_info ioctl_info[] = {
> -	MEDIA_IOC(DEVICE_INFO),
> -	MEDIA_IOC(ENUM_ENTITIES),
> -	MEDIA_IOC(ENUM_LINKS),
> -	MEDIA_IOC(SETUP_LINK),
> -	MEDIA_IOC(G_TOPOLOGY),
> +	MEDIA_IOC(DEVICE_INFO, media_device_get_info),
> +	MEDIA_IOC(ENUM_ENTITIES, media_device_enum_entities),
> +	MEDIA_IOC(ENUM_LINKS, media_device_enum_links),
> +	MEDIA_IOC(SETUP_LINK, media_device_setup_link),
> +	MEDIA_IOC(G_TOPOLOGY, media_device_get_topology),
>  };
>  
>  static long media_device_ioctl(struct file *filp, unsigned int cmd,
> @@ -528,33 +507,19 @@ static long media_device_enum_links32(struct media_device *mdev,
>  #define MEDIA_IOC_ENUM_LINKS32		_IOWR('|', 0x02, struct media_links_enum32)
>  
>  static const struct media_ioctl_info compat_ioctl_info[] = {
> -	MEDIA_IOC(DEVICE_INFO),
> -	MEDIA_IOC(ENUM_ENTITIES),
> -	MEDIA_IOC(ENUM_LINKS32),
> -	MEDIA_IOC(SETUP_LINK),
> -	MEDIA_IOC(G_TOPOLOGY),
> +	MEDIA_IOC(DEVICE_INFO, media_device_get_info),
> +	MEDIA_IOC(ENUM_ENTITIES, media_device_enum_entities),
> +	MEDIA_IOC(ENUM_LINKS32, media_device_enum_links32),
> +	MEDIA_IOC(SETUP_LINK, media_device_setup_link),
> +	MEDIA_IOC(G_TOPOLOGY, media_device_get_topology),
>  };
>  
>  static long media_device_compat_ioctl(struct file *filp, unsigned int cmd,
>  				      unsigned long arg)
>  {
> -	struct media_devnode *devnode = media_devnode_data(filp);
> -	struct media_device *dev = devnode->media_dev;
> -	long ret;
> -
> -	switch (cmd) {
> -	case MEDIA_IOC_ENUM_LINKS32:
> -		mutex_lock(&dev->graph_mutex);
> -		ret = media_device_enum_links32(dev,
> -				(struct media_links_enum32 __user *)arg);
> -		mutex_unlock(&dev->graph_mutex);
> -		break;
> -
> -	default:
> -		return media_device_ioctl(filp, cmd, arg);
> -	}
> -
> -	return ret;
> +	return __media_device_ioctl(
> +		filp, cmd, (void __user *)arg,
> +		compat_ioctl_info, ARRAY_SIZE(compat_ioctl_info));
>  }
>  #endif /* CONFIG_COMPAT */
>  
> 

[PATCH v3 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 <sakari.ailus@linux.intel.com>
---
 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 6fd9b77..87d17a0 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(&info, 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, &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(&u_ent, 0, sizeof(u_ent));
-	if (copy_from_user(&u_ent.id, &uent->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(&u_ent.raw, &ent->info, sizeof(ent->info));
-	if (copy_to_user(uent, &u_ent, sizeof(u_ent)))
-		return -EFAULT;
+	memcpy(&entd->raw, &ent->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 media_links_enum *links)
 {
 	struct media_entity *entity;
 
@@ -206,64 +198,35 @@ static long __media_device_enum_links(struct media_device *mdev,
 	return 0;
 }
 
-static long media_device_enum_links(struct media_device *mdev,
-				    struct media_links_enum __user *ulinks)
-{
-	struct media_links_enum links;
-	int rval;
-
-	if (copy_from_user(&links, ulinks, sizeof(links)))
-		return -EFAULT;
-
-	rval = __media_device_enum_links(mdev, &links);
-	if (rval < 0)
-		return rval;
-
-	if (copy_to_user(ulinks, &links, sizeof(*ulinks)))
-		return -EFAULT;
-
-	return 0;
-}
-
 static long media_device_setup_link(struct media_device *mdev,
-				    struct media_link_desc __user *_ulink)
+				    struct media_link_desc *linkd)
 {
 	struct media_link *link = NULL;
-	struct media_link_desc ulink;
 	struct media_entity *source;
 	struct media_entity *sink;
-	int ret;
-
-	if (copy_from_user(&ulink, _ulink, sizeof(ulink)))
-		return -EFAULT;
 
 	/* Find the source and sink entities and link.
 	 */
-	source = find_entity(mdev, ulink.source.entity);
-	sink = find_entity(mdev, ulink.sink.entity);
+	source = find_entity(mdev, linkd->source.entity);
+	sink = find_entity(mdev, linkd->sink.entity);
 
 	if (source == NULL || sink == NULL)
 		return -EINVAL;
 
-	if (ulink.source.index >= source->num_pads ||
-	    ulink.sink.index >= sink->num_pads)
+	if (linkd->source.index >= source->num_pads ||
+	    linkd->sink.index >= sink->num_pads)
 		return -EINVAL;
 
-	link = media_entity_find_link(&source->pads[ulink.source.index],
-				      &sink->pads[ulink.sink.index]);
+	link = media_entity_find_link(&source->pads[linkd->source.index],
+				      &sink->pads[linkd->sink.index]);
 	if (link == NULL)
 		return -EINVAL;
 
 	/* Setup the link on both entities. */
-	ret = __media_entity_setup_link(link, ulink.flags);
-
-	if (copy_to_user(_ulink, &ulink, sizeof(ulink)))
-		return -EFAULT;
-
-	return ret;
+	return __media_entity_setup_link(link, linkd->flags);
 }
 
-static long __media_device_get_topology(struct media_device *mdev,
+static long media_device_get_topology(struct media_device *mdev,
 				      struct media_v2_topology *topo)
 {
 	struct media_entity *entity;
@@ -400,35 +363,41 @@ static long __media_device_get_topology(struct media_device *mdev,
 	return ret;
 }
 
-static long media_device_get_topology(struct media_device *mdev,
-				      struct media_v2_topology __user *utopo)
+static long copy_arg_from_user(void *karg, void __user *uarg, unsigned int cmd)
 {
-	struct media_v2_topology ktopo;
-	int ret;
-
-	if (copy_from_user(&ktopo, utopo, sizeof(ktopo)))
+	/* All media IOCTLs are _IOWR() */
+	if (copy_from_user(karg, uarg, _IOC_SIZE(cmd)))
 		return -EFAULT;
 
-	ret = __media_device_get_topology(mdev, &ktopo);
-	if (ret < 0)
-		return ret;
+	return 0;
+}
 
-	if (copy_to_user(utopo, &ktopo, sizeof(*utopo)))
+static long copy_arg_to_user(void __user *uarg, void *karg, unsigned int cmd)
+{
+	/* All media IOCTLs are _IOWR() */
+	if (copy_to_user(uarg, karg, _IOC_SIZE(cmd)))
 		return -EFAULT;
 
 	return 0;
 }
 
-#define MEDIA_IOC(__cmd, func)						\
-	[_IOC_NR(MEDIA_IOC_##__cmd)] = {				\
-		.cmd = MEDIA_IOC_##__cmd,				\
-		.fn = (long (*)(struct media_device *, void __user *))func,    \
+#define MEDIA_IOC_ARG(__cmd, func, from_user, to_user)	\
+	[_IOC_NR(MEDIA_IOC_##__cmd)] = {		\
+		.cmd = MEDIA_IOC_##__cmd,		\
+		.fn = (long (*)(struct media_device *, void *))func,	\
+		.arg_from_user = from_user,		\
+		.arg_to_user = to_user,			\
 	}
 
+#define MEDIA_IOC(__cmd, func)						\
+	MEDIA_IOC_ARG(__cmd, func, copy_arg_from_user, copy_arg_to_user)
+
 /* the table is indexed by _IOC_NR(cmd) */
 struct media_ioctl_info {
 	unsigned int cmd;
-	long (*fn)(struct media_device *dev, void __user *arg);
+	long (*fn)(struct media_device *dev, void *arg);
+	long (*arg_from_user)(void *karg, void __user *uarg, unsigned int cmd);
+	long (*arg_to_user)(void __user *uarg, void *karg, unsigned int cmd);
 };
 
 static inline long is_valid_ioctl(const struct media_ioctl_info *info,
@@ -445,6 +414,7 @@ static long __media_device_ioctl(
 	struct media_devnode *devnode = media_devnode_data(filp);
 	struct media_device *dev = devnode->media_dev;
 	const struct media_ioctl_info *info;
+	char __karg[256], *karg = __karg;
 	long ret;
 
 	ret = is_valid_ioctl(info_array, info_array_len, cmd);
@@ -453,10 +423,29 @@ static long __media_device_ioctl(
 
 	info = &info_array[_IOC_NR(cmd)];
 
+	if (_IOC_SIZE(info->cmd) > sizeof(__karg)) {
+		karg = kmalloc(_IOC_SIZE(info->cmd), GFP_KERNEL);
+		if (!karg)
+			return -ENOMEM;
+	}
+
+	if (info->arg_from_user) {
+		ret = info->arg_from_user(karg, arg, cmd);
+		if (ret)
+			goto out_free;
+	}
+
 	mutex_lock(&dev->graph_mutex);
-	ret = info->fn(dev, arg);
+	ret = info->fn(dev, karg);
 	mutex_unlock(&dev->graph_mutex);
 
+	if (!ret && info->arg_to_user)
+		ret = info->arg_to_user(arg, karg, cmd);
+
+out_free:
+	if (karg != __karg)
+		kfree(karg);
+
 	return ret;
 }
 
@@ -485,23 +474,24 @@ struct media_links_enum32 {
 	__u32 reserved[4];
 };
 
-static long media_device_enum_links32(struct media_device *mdev,
-				      struct media_links_enum32 __user *ulinks)
+static long from_user_enum_links32(void *karg, void __user *uarg,
+				   unsigned int cmd)
 {
-	struct media_links_enum links;
+	struct media_links_enum *links = karg;
+	struct media_links_enum32 __user *ulinks = uarg;
 	compat_uptr_t pads_ptr, links_ptr;
 
-	memset(&links, 0, sizeof(links));
+	memset(links, 0, sizeof(*links));
 
-	if (get_user(links.entity, &ulinks->entity)
+	if (get_user(links->entity, &ulinks->entity)
 	    || get_user(pads_ptr, &ulinks->pads)
 	    || get_user(links_ptr, &ulinks->links))
 		return -EFAULT;
 
-	links.pads = compat_ptr(pads_ptr);
-	links.links = compat_ptr(links_ptr);
+	links->pads = compat_ptr(pads_ptr);
+	links->links = compat_ptr(links_ptr);
 
-	return __media_device_enum_links(mdev, &links);
+	return 0;
 }
 
 #define MEDIA_IOC_ENUM_LINKS32		_IOWR('|', 0x02, struct media_links_enum32)
@@ -509,7 +499,7 @@ static long media_device_enum_links32(struct media_device *mdev,
 static const struct media_ioctl_info compat_ioctl_info[] = {
 	MEDIA_IOC(DEVICE_INFO, media_device_get_info),
 	MEDIA_IOC(ENUM_ENTITIES, media_device_enum_entities),
-	MEDIA_IOC(ENUM_LINKS32, media_device_enum_links32),
+	MEDIA_IOC_ARG(ENUM_LINKS32, media_device_enum_links, from_user_enum_links32, NULL),
 	MEDIA_IOC(SETUP_LINK, media_device_setup_link),
 	MEDIA_IOC(G_TOPOLOGY, media_device_get_topology),
 };
-- 
2.7.4

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

[Hans Verkuil]

On 07/21/2016 01:14 PM, 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 <sakari.ailus@linux.intel.com>

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Thanks!

	Hans

> ---
>  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 6fd9b77..87d17a0 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(&info, 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, &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(&u_ent, 0, sizeof(u_ent));
> -	if (copy_from_user(&u_ent.id, &uent->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(&u_ent.raw, &ent->info, sizeof(ent->info));
> -	if (copy_to_user(uent, &u_ent, sizeof(u_ent)))
> -		return -EFAULT;
> +	memcpy(&entd->raw, &ent->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 media_links_enum *links)
>  {
>  	struct media_entity *entity;
>  
> @@ -206,64 +198,35 @@ static long __media_device_enum_links(struct media_device *mdev,
>  	return 0;
>  }
>  
> -static long media_device_enum_links(struct media_device *mdev,
> -				    struct media_links_enum __user *ulinks)
> -{
> -	struct media_links_enum links;
> -	int rval;
> -
> -	if (copy_from_user(&links, ulinks, sizeof(links)))
> -		return -EFAULT;
> -
> -	rval = __media_device_enum_links(mdev, &links);
> -	if (rval < 0)
> -		return rval;
> -
> -	if (copy_to_user(ulinks, &links, sizeof(*ulinks)))
> -		return -EFAULT;
> -
> -	return 0;
> -}
> -
>  static long media_device_setup_link(struct media_device *mdev,
> -				    struct media_link_desc __user *_ulink)
> +				    struct media_link_desc *linkd)
>  {
>  	struct media_link *link = NULL;
> -	struct media_link_desc ulink;
>  	struct media_entity *source;
>  	struct media_entity *sink;
> -	int ret;
> -
> -	if (copy_from_user(&ulink, _ulink, sizeof(ulink)))
> -		return -EFAULT;
>  
>  	/* Find the source and sink entities and link.
>  	 */
> -	source = find_entity(mdev, ulink.source.entity);
> -	sink = find_entity(mdev, ulink.sink.entity);
> +	source = find_entity(mdev, linkd->source.entity);
> +	sink = find_entity(mdev, linkd->sink.entity);
>  
>  	if (source == NULL || sink == NULL)
>  		return -EINVAL;
>  
> -	if (ulink.source.index >= source->num_pads ||
> -	    ulink.sink.index >= sink->num_pads)
> +	if (linkd->source.index >= source->num_pads ||
> +	    linkd->sink.index >= sink->num_pads)
>  		return -EINVAL;
>  
> -	link = media_entity_find_link(&source->pads[ulink.source.index],
> -				      &sink->pads[ulink.sink.index]);
> +	link = media_entity_find_link(&source->pads[linkd->source.index],
> +				      &sink->pads[linkd->sink.index]);
>  	if (link == NULL)
>  		return -EINVAL;
>  
>  	/* Setup the link on both entities. */
> -	ret = __media_entity_setup_link(link, ulink.flags);
> -
> -	if (copy_to_user(_ulink, &ulink, sizeof(ulink)))
> -		return -EFAULT;
> -
> -	return ret;
> +	return __media_entity_setup_link(link, linkd->flags);
>  }
>  
> -static long __media_device_get_topology(struct media_device *mdev,
> +static long media_device_get_topology(struct media_device *mdev,
>  				      struct media_v2_topology *topo)
>  {
>  	struct media_entity *entity;
> @@ -400,35 +363,41 @@ static long __media_device_get_topology(struct media_device *mdev,
>  	return ret;
>  }
>  
> -static long media_device_get_topology(struct media_device *mdev,
> -				      struct media_v2_topology __user *utopo)
> +static long copy_arg_from_user(void *karg, void __user *uarg, unsigned int cmd)
>  {
> -	struct media_v2_topology ktopo;
> -	int ret;
> -
> -	if (copy_from_user(&ktopo, utopo, sizeof(ktopo)))
> +	/* All media IOCTLs are _IOWR() */
> +	if (copy_from_user(karg, uarg, _IOC_SIZE(cmd)))
>  		return -EFAULT;
>  
> -	ret = __media_device_get_topology(mdev, &ktopo);
> -	if (ret < 0)
> -		return ret;
> +	return 0;
> +}
>  
> -	if (copy_to_user(utopo, &ktopo, sizeof(*utopo)))
> +static long copy_arg_to_user(void __user *uarg, void *karg, unsigned int cmd)
> +{
> +	/* All media IOCTLs are _IOWR() */
> +	if (copy_to_user(uarg, karg, _IOC_SIZE(cmd)))
>  		return -EFAULT;
>  
>  	return 0;
>  }
>  
> -#define MEDIA_IOC(__cmd, func)						\
> -	[_IOC_NR(MEDIA_IOC_##__cmd)] = {				\
> -		.cmd = MEDIA_IOC_##__cmd,				\
> -		.fn = (long (*)(struct media_device *, void __user *))func,    \
> +#define MEDIA_IOC_ARG(__cmd, func, from_user, to_user)	\
> +	[_IOC_NR(MEDIA_IOC_##__cmd)] = {		\
> +		.cmd = MEDIA_IOC_##__cmd,		\
> +		.fn = (long (*)(struct media_device *, void *))func,	\
> +		.arg_from_user = from_user,		\
> +		.arg_to_user = to_user,			\
>  	}
>  
> +#define MEDIA_IOC(__cmd, func)						\
> +	MEDIA_IOC_ARG(__cmd, func, copy_arg_from_user, copy_arg_to_user)
> +
>  /* the table is indexed by _IOC_NR(cmd) */
>  struct media_ioctl_info {
>  	unsigned int cmd;
> -	long (*fn)(struct media_device *dev, void __user *arg);
> +	long (*fn)(struct media_device *dev, void *arg);
> +	long (*arg_from_user)(void *karg, void __user *uarg, unsigned int cmd);
> +	long (*arg_to_user)(void __user *uarg, void *karg, unsigned int cmd);
>  };
>  
>  static inline long is_valid_ioctl(const struct media_ioctl_info *info,
> @@ -445,6 +414,7 @@ static long __media_device_ioctl(
>  	struct media_devnode *devnode = media_devnode_data(filp);
>  	struct media_device *dev = devnode->media_dev;
>  	const struct media_ioctl_info *info;
> +	char __karg[256], *karg = __karg;
>  	long ret;
>  
>  	ret = is_valid_ioctl(info_array, info_array_len, cmd);
> @@ -453,10 +423,29 @@ static long __media_device_ioctl(
>  
>  	info = &info_array[_IOC_NR(cmd)];
>  
> +	if (_IOC_SIZE(info->cmd) > sizeof(__karg)) {
> +		karg = kmalloc(_IOC_SIZE(info->cmd), GFP_KERNEL);
> +		if (!karg)
> +			return -ENOMEM;
> +	}
> +
> +	if (info->arg_from_user) {
> +		ret = info->arg_from_user(karg, arg, cmd);
> +		if (ret)
> +			goto out_free;
> +	}
> +
>  	mutex_lock(&dev->graph_mutex);
> -	ret = info->fn(dev, arg);
> +	ret = info->fn(dev, karg);
>  	mutex_unlock(&dev->graph_mutex);
>  
> +	if (!ret && info->arg_to_user)
> +		ret = info->arg_to_user(arg, karg, cmd);
> +
> +out_free:
> +	if (karg != __karg)
> +		kfree(karg);
> +
>  	return ret;
>  }
>  
> @@ -485,23 +474,24 @@ struct media_links_enum32 {
>  	__u32 reserved[4];
>  };
>  
> -static long media_device_enum_links32(struct media_device *mdev,
> -				      struct media_links_enum32 __user *ulinks)
> +static long from_user_enum_links32(void *karg, void __user *uarg,
> +				   unsigned int cmd)
>  {
> -	struct media_links_enum links;
> +	struct media_links_enum *links = karg;
> +	struct media_links_enum32 __user *ulinks = uarg;
>  	compat_uptr_t pads_ptr, links_ptr;
>  
> -	memset(&links, 0, sizeof(links));
> +	memset(links, 0, sizeof(*links));
>  
> -	if (get_user(links.entity, &ulinks->entity)
> +	if (get_user(links->entity, &ulinks->entity)
>  	    || get_user(pads_ptr, &ulinks->pads)
>  	    || get_user(links_ptr, &ulinks->links))
>  		return -EFAULT;
>  
> -	links.pads = compat_ptr(pads_ptr);
> -	links.links = compat_ptr(links_ptr);
> +	links->pads = compat_ptr(pads_ptr);
> +	links->links = compat_ptr(links_ptr);
>  
> -	return __media_device_enum_links(mdev, &links);
> +	return 0;
>  }
>  
>  #define MEDIA_IOC_ENUM_LINKS32		_IOWR('|', 0x02, struct media_links_enum32)
> @@ -509,7 +499,7 @@ static long media_device_enum_links32(struct media_device *mdev,
>  static const struct media_ioctl_info compat_ioctl_info[] = {
>  	MEDIA_IOC(DEVICE_INFO, media_device_get_info),
>  	MEDIA_IOC(ENUM_ENTITIES, media_device_enum_entities),
> -	MEDIA_IOC(ENUM_LINKS32, media_device_enum_links32),
> +	MEDIA_IOC_ARG(ENUM_LINKS32, media_device_enum_links, from_user_enum_links32, NULL),
>  	MEDIA_IOC(SETUP_LINK, media_device_setup_link),
>  	MEDIA_IOC(G_TOPOLOGY, media_device_get_topology),
>  };
> 

[PATCH v3 4/5] media: Add flags to tell whether to take graph mutex for an IOCTL

[Sakari Ailus]

New IOCTLs (especially for the request API) do not necessarily need the
graph mutex acquired. Leave this up to the drivers.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/media-device.c | 47 ++++++++++++++++++++++++++------------------
 1 file changed, 28 insertions(+), 19 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 87d17a0..6dfcc50 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -381,20 +381,25 @@ static long copy_arg_to_user(void __user *uarg, void *karg, unsigned int cmd)
 	return 0;
 }
 
-#define MEDIA_IOC_ARG(__cmd, func, from_user, to_user)	\
-	[_IOC_NR(MEDIA_IOC_##__cmd)] = {		\
-		.cmd = MEDIA_IOC_##__cmd,		\
+/* Do acquire the graph mutex */
+#define MEDIA_IOC_FL_GRAPH_MUTEX	BIT(0)
+
+#define MEDIA_IOC_ARG(__cmd, func, fl, from_user, to_user)		\
+	[_IOC_NR(MEDIA_IOC_##__cmd)] = {				\
+		.cmd = MEDIA_IOC_##__cmd,				\
 		.fn = (long (*)(struct media_device *, void *))func,	\
-		.arg_from_user = from_user,		\
-		.arg_to_user = to_user,			\
+		.flags = fl,						\
+		.arg_from_user = from_user,				\
+		.arg_to_user = to_user,					\
 	}
 
-#define MEDIA_IOC(__cmd, func)						\
-	MEDIA_IOC_ARG(__cmd, func, copy_arg_from_user, copy_arg_to_user)
+#define MEDIA_IOC(__cmd, func, fl)					\
+	MEDIA_IOC_ARG(__cmd, func, fl, copy_arg_from_user, copy_arg_to_user)
 
 /* the table is indexed by _IOC_NR(cmd) */
 struct media_ioctl_info {
 	unsigned int cmd;
+	unsigned short flags;
 	long (*fn)(struct media_device *dev, void *arg);
 	long (*arg_from_user)(void *karg, void __user *uarg, unsigned int cmd);
 	long (*arg_to_user)(void __user *uarg, void *karg, unsigned int cmd);
@@ -435,9 +440,13 @@ static long __media_device_ioctl(
 			goto out_free;
 	}
 
-	mutex_lock(&dev->graph_mutex);
+	if (info->flags & MEDIA_IOC_FL_GRAPH_MUTEX)
+		mutex_lock(&dev->graph_mutex);
+
 	ret = info->fn(dev, karg);
-	mutex_unlock(&dev->graph_mutex);
+
+	if (info->flags & MEDIA_IOC_FL_GRAPH_MUTEX)
+		mutex_unlock(&dev->graph_mutex);
 
 	if (!ret && info->arg_to_user)
 		ret = info->arg_to_user(arg, karg, cmd);
@@ -450,11 +459,11 @@ out_free:
 }
 
 static const struct media_ioctl_info ioctl_info[] = {
-	MEDIA_IOC(DEVICE_INFO, media_device_get_info),
-	MEDIA_IOC(ENUM_ENTITIES, media_device_enum_entities),
-	MEDIA_IOC(ENUM_LINKS, media_device_enum_links),
-	MEDIA_IOC(SETUP_LINK, media_device_setup_link),
-	MEDIA_IOC(G_TOPOLOGY, media_device_get_topology),
+	MEDIA_IOC(DEVICE_INFO, media_device_get_info, MEDIA_IOC_FL_GRAPH_MUTEX),
+	MEDIA_IOC(ENUM_ENTITIES, media_device_enum_entities, MEDIA_IOC_FL_GRAPH_MUTEX),
+	MEDIA_IOC(ENUM_LINKS, media_device_enum_links, MEDIA_IOC_FL_GRAPH_MUTEX),
+	MEDIA_IOC(SETUP_LINK, media_device_setup_link, MEDIA_IOC_FL_GRAPH_MUTEX),
+	MEDIA_IOC(G_TOPOLOGY, media_device_get_topology, MEDIA_IOC_FL_GRAPH_MUTEX),
 };
 
 static long media_device_ioctl(struct file *filp, unsigned int cmd,
@@ -497,11 +506,11 @@ static long from_user_enum_links32(void *karg, void __user *uarg,
 #define MEDIA_IOC_ENUM_LINKS32		_IOWR('|', 0x02, struct media_links_enum32)
 
 static const struct media_ioctl_info compat_ioctl_info[] = {
-	MEDIA_IOC(DEVICE_INFO, media_device_get_info),
-	MEDIA_IOC(ENUM_ENTITIES, media_device_enum_entities),
-	MEDIA_IOC_ARG(ENUM_LINKS32, media_device_enum_links, from_user_enum_links32, NULL),
-	MEDIA_IOC(SETUP_LINK, media_device_setup_link),
-	MEDIA_IOC(G_TOPOLOGY, media_device_get_topology),
+	MEDIA_IOC(DEVICE_INFO, media_device_get_info, MEDIA_IOC_FL_GRAPH_MUTEX),
+	MEDIA_IOC(ENUM_ENTITIES, media_device_enum_entities, MEDIA_IOC_FL_GRAPH_MUTEX),
+	MEDIA_IOC_ARG(ENUM_LINKS32, media_device_enum_links, MEDIA_IOC_FL_GRAPH_MUTEX, from_user_enum_links32, NULL),
+	MEDIA_IOC(SETUP_LINK, media_device_setup_link, MEDIA_IOC_FL_GRAPH_MUTEX),
+	MEDIA_IOC(G_TOPOLOGY, media_device_get_topology, MEDIA_IOC_FL_GRAPH_MUTEX),
 };
 
 static long media_device_compat_ioctl(struct file *filp, unsigned int cmd,
-- 
2.7.4

[PATCH v3 5/5] media: Support variable size IOCTL arguments

[Sakari Ailus]

Instead of checking for a strict size for the IOCTL arguments, place
minimum and maximum limits.

As an additional bonus, IOCTL handlers will be able to check whether the
caller actually set (using the argument size) the field vs. assigning it
to zero. Separate macro can be provided for that.

This will be easier for applications as well since there is no longer the
problem of setting the reserved fields zero, or at least it is a lesser
problem.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/media-device.c | 52 +++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 47 insertions(+), 5 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 6dfcc50..c7be2ce 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -384,32 +384,71 @@ static long copy_arg_to_user(void __user *uarg, void *karg, unsigned int cmd)
 /* Do acquire the graph mutex */
 #define MEDIA_IOC_FL_GRAPH_MUTEX	BIT(0)
 
-#define MEDIA_IOC_ARG(__cmd, func, fl, from_user, to_user)		\
+#define MEDIA_IOC_SZ_ARG(__cmd, func, fl, alt_sz, from_user, to_user)	\
 	[_IOC_NR(MEDIA_IOC_##__cmd)] = {				\
 		.cmd = MEDIA_IOC_##__cmd,				\
 		.fn = (long (*)(struct media_device *, void *))func,	\
 		.flags = fl,						\
+		.alt_arg_sizes = alt_sz,				\
 		.arg_from_user = from_user,				\
 		.arg_to_user = to_user,					\
 	}
 
-#define MEDIA_IOC(__cmd, func, fl)					\
-	MEDIA_IOC_ARG(__cmd, func, fl, copy_arg_from_user, copy_arg_to_user)
+#define MEDIA_IOC_ARG(__cmd, func, fl, from_user, to_user)		\
+	MEDIA_IOC_SZ_ARG(__cmd, func, fl, NULL, from_user, to_user)
+
+#define MEDIA_IOC_SZ(__cmd, func, fl, alt_sz)			\
+	MEDIA_IOC_SZ_ARG(__cmd, func, fl, alt_sz,		\
+			 copy_arg_from_user, copy_arg_to_user)
+
+#define MEDIA_IOC(__cmd, func, fl)				\
+	MEDIA_IOC_ARG(__cmd, func, fl,				\
+		      copy_arg_from_user, copy_arg_to_user)
 
 /* the table is indexed by _IOC_NR(cmd) */
 struct media_ioctl_info {
 	unsigned int cmd;
 	unsigned short flags;
+	const unsigned short *alt_arg_sizes;
 	long (*fn)(struct media_device *dev, void *arg);
 	long (*arg_from_user)(void *karg, void __user *uarg, unsigned int cmd);
 	long (*arg_to_user)(void __user *uarg, void *karg, unsigned int cmd);
 };
 
+#define MASK_IOC_SIZE(cmd) \
+	((cmd) & ~(_IOC_SIZEMASK << _IOC_SIZESHIFT))
+
 static inline long is_valid_ioctl(const struct media_ioctl_info *info,
 				  unsigned int len, unsigned int cmd)
 {
-	return (_IOC_NR(cmd) >= len
-		|| info[_IOC_NR(cmd)].cmd != cmd) ? -ENOIOCTLCMD : 0;
+	const unsigned short *alt_arg_sizes;
+
+	if (unlikely(_IOC_NR(cmd) >= len))
+		return -ENOIOCTLCMD;
+
+	info += _IOC_NR(cmd);
+
+	if (info->cmd == cmd)
+		return 0;
+
+	/*
+	 * Verify that the size-dependent patch of the IOCTL command
+	 * matches and that the size does not exceed the principal
+	 * argument size.
+	 */
+	if (unlikely(MASK_IOC_SIZE(info->cmd) != MASK_IOC_SIZE(cmd)
+		     || _IOC_SIZE(info->cmd) < _IOC_SIZE(cmd)))
+		return -ENOIOCTLCMD;
+
+	alt_arg_sizes = info->alt_arg_sizes;
+	if (unlikely(!alt_arg_sizes))
+		return -ENOIOCTLCMD;
+
+	for (; *alt_arg_sizes; alt_arg_sizes++)
+		if (_IOC_SIZE(cmd) == *alt_arg_sizes)
+			return 0;
+
+	return -ENOIOCTLCMD;
 }
 
 static long __media_device_ioctl(
@@ -440,6 +479,9 @@ static long __media_device_ioctl(
 			goto out_free;
 	}
 
+	/* Set the rest of the argument struct to zero */
+	memset(karg + _IOC_SIZE(cmd), 0, _IOC_SIZE(info->cmd) - _IOC_SIZE(cmd));
+
 	if (info->flags & MEDIA_IOC_FL_GRAPH_MUTEX)
 		mutex_lock(&dev->graph_mutex);
 
-- 
2.7.4

Re: [PATCH v3 5/5] media: Support variable size IOCTL arguments

[Hans Verkuil]

On 07/21/2016 01:17 PM, Sakari Ailus wrote:
> Instead of checking for a strict size for the IOCTL arguments, place
> minimum and maximum limits.

This sentence is out of date: it checks for alternative smaller sizes, not
min/max.

> 
> As an additional bonus, IOCTL handlers will be able to check whether the
> caller actually set (using the argument size) the field vs. assigning it
> to zero. Separate macro can be provided for that.
> 
> This will be easier for applications as well since there is no longer the
> problem of setting the reserved fields zero, or at least it is a lesser
> problem.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/media-device.c | 52 +++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 47 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 6dfcc50..c7be2ce 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -384,32 +384,71 @@ static long copy_arg_to_user(void __user *uarg, void *karg, unsigned int cmd)
>  /* Do acquire the graph mutex */
>  #define MEDIA_IOC_FL_GRAPH_MUTEX	BIT(0)
>  
> -#define MEDIA_IOC_ARG(__cmd, func, fl, from_user, to_user)		\
> +#define MEDIA_IOC_SZ_ARG(__cmd, func, fl, alt_sz, from_user, to_user)	\
>  	[_IOC_NR(MEDIA_IOC_##__cmd)] = {				\
>  		.cmd = MEDIA_IOC_##__cmd,				\
>  		.fn = (long (*)(struct media_device *, void *))func,	\
>  		.flags = fl,						\
> +		.alt_arg_sizes = alt_sz,				\
>  		.arg_from_user = from_user,				\
>  		.arg_to_user = to_user,					\
>  	}
>  
> -#define MEDIA_IOC(__cmd, func, fl)					\
> -	MEDIA_IOC_ARG(__cmd, func, fl, copy_arg_from_user, copy_arg_to_user)
> +#define MEDIA_IOC_ARG(__cmd, func, fl, from_user, to_user)		\
> +	MEDIA_IOC_SZ_ARG(__cmd, func, fl, NULL, from_user, to_user)
> +
> +#define MEDIA_IOC_SZ(__cmd, func, fl, alt_sz)			\
> +	MEDIA_IOC_SZ_ARG(__cmd, func, fl, alt_sz,		\
> +			 copy_arg_from_user, copy_arg_to_user)
> +
> +#define MEDIA_IOC(__cmd, func, fl)				\
> +	MEDIA_IOC_ARG(__cmd, func, fl,				\
> +		      copy_arg_from_user, copy_arg_to_user)
>  
>  /* the table is indexed by _IOC_NR(cmd) */
>  struct media_ioctl_info {
>  	unsigned int cmd;
>  	unsigned short flags;
> +	const unsigned short *alt_arg_sizes;

I think an additional comment would be useful here.

>  	long (*fn)(struct media_device *dev, void *arg);
>  	long (*arg_from_user)(void *karg, void __user *uarg, unsigned int cmd);
>  	long (*arg_to_user)(void __user *uarg, void *karg, unsigned int cmd);
>  };
>  
> +#define MASK_IOC_SIZE(cmd) \
> +	((cmd) & ~(_IOC_SIZEMASK << _IOC_SIZESHIFT))
> +
>  static inline long is_valid_ioctl(const struct media_ioctl_info *info,
>  				  unsigned int len, unsigned int cmd)
>  {
> -	return (_IOC_NR(cmd) >= len
> -		|| info[_IOC_NR(cmd)].cmd != cmd) ? -ENOIOCTLCMD : 0;
> +	const unsigned short *alt_arg_sizes;
> +
> +	if (unlikely(_IOC_NR(cmd) >= len))

Please don't use 'unlikely'. Unless you can prove with hard numbers that it actually
make a performance difference it only pollutes the code.

> +		return -ENOIOCTLCMD;
> +
> +	info += _IOC_NR(cmd);
> +
> +	if (info->cmd == cmd)
> +		return 0;
> +
> +	/*
> +	 * Verify that the size-dependent patch of the IOCTL command
> +	 * matches and that the size does not exceed the principal
> +	 * argument size.
> +	 */
> +	if (unlikely(MASK_IOC_SIZE(info->cmd) != MASK_IOC_SIZE(cmd)
> +		     || _IOC_SIZE(info->cmd) < _IOC_SIZE(cmd)))
> +		return -ENOIOCTLCMD;
> +
> +	alt_arg_sizes = info->alt_arg_sizes;
> +	if (unlikely(!alt_arg_sizes))
> +		return -ENOIOCTLCMD;
> +
> +	for (; *alt_arg_sizes; alt_arg_sizes++)
> +		if (_IOC_SIZE(cmd) == *alt_arg_sizes)
> +			return 0;
> +
> +	return -ENOIOCTLCMD;
>  }
>  
>  static long __media_device_ioctl(
> @@ -440,6 +479,9 @@ static long __media_device_ioctl(
>  			goto out_free;
>  	}
>  
> +	/* Set the rest of the argument struct to zero */
> +	memset(karg + _IOC_SIZE(cmd), 0, _IOC_SIZE(info->cmd) - _IOC_SIZE(cmd));
> +
>  	if (info->flags & MEDIA_IOC_FL_GRAPH_MUTEX)
>  		mutex_lock(&dev->graph_mutex);
>  
> 

I'm in two minds: I like this idea, but on the other hand it is not actually needed at
the moment. So should we just park this in patchwork until needed, or apply anyway?

Comments?

Personally I think it is better to park it. I don't like code that isn't actually needed.

Regards,

	Hans

Re: [PATCH v3 5/5] media: Support variable size IOCTL arguments

[Sakari Ailus]

Hi Hans,

On Fri, Jul 22, 2016 at 12:36:59PM +0200, Hans Verkuil wrote:
> 
> 
> On 07/21/2016 01:17 PM, Sakari Ailus wrote:
> > Instead of checking for a strict size for the IOCTL arguments, place
> > minimum and maximum limits.
> 
> This sentence is out of date: it checks for alternative smaller sizes, not
> min/max.
> 
> > 
> > As an additional bonus, IOCTL handlers will be able to check whether the
> > caller actually set (using the argument size) the field vs. assigning it
> > to zero. Separate macro can be provided for that.
> > 
> > This will be easier for applications as well since there is no longer the
> > problem of setting the reserved fields zero, or at least it is a lesser
> > problem.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/media-device.c | 52 +++++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 47 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > index 6dfcc50..c7be2ce 100644
> > --- a/drivers/media/media-device.c
> > +++ b/drivers/media/media-device.c
> > @@ -384,32 +384,71 @@ static long copy_arg_to_user(void __user *uarg, void *karg, unsigned int cmd)
> >  /* Do acquire the graph mutex */
> >  #define MEDIA_IOC_FL_GRAPH_MUTEX	BIT(0)
> >  
> > -#define MEDIA_IOC_ARG(__cmd, func, fl, from_user, to_user)		\
> > +#define MEDIA_IOC_SZ_ARG(__cmd, func, fl, alt_sz, from_user, to_user)	\
> >  	[_IOC_NR(MEDIA_IOC_##__cmd)] = {				\
> >  		.cmd = MEDIA_IOC_##__cmd,				\
> >  		.fn = (long (*)(struct media_device *, void *))func,	\
> >  		.flags = fl,						\
> > +		.alt_arg_sizes = alt_sz,				\
> >  		.arg_from_user = from_user,				\
> >  		.arg_to_user = to_user,					\
> >  	}
> >  
> > -#define MEDIA_IOC(__cmd, func, fl)					\
> > -	MEDIA_IOC_ARG(__cmd, func, fl, copy_arg_from_user, copy_arg_to_user)
> > +#define MEDIA_IOC_ARG(__cmd, func, fl, from_user, to_user)		\
> > +	MEDIA_IOC_SZ_ARG(__cmd, func, fl, NULL, from_user, to_user)
> > +
> > +#define MEDIA_IOC_SZ(__cmd, func, fl, alt_sz)			\
> > +	MEDIA_IOC_SZ_ARG(__cmd, func, fl, alt_sz,		\
> > +			 copy_arg_from_user, copy_arg_to_user)
> > +
> > +#define MEDIA_IOC(__cmd, func, fl)				\
> > +	MEDIA_IOC_ARG(__cmd, func, fl,				\
> > +		      copy_arg_from_user, copy_arg_to_user)
> >  
> >  /* the table is indexed by _IOC_NR(cmd) */
> >  struct media_ioctl_info {
> >  	unsigned int cmd;
> >  	unsigned short flags;
> > +	const unsigned short *alt_arg_sizes;
> 
> I think an additional comment would be useful here.
> 
> >  	long (*fn)(struct media_device *dev, void *arg);
> >  	long (*arg_from_user)(void *karg, void __user *uarg, unsigned int cmd);
> >  	long (*arg_to_user)(void __user *uarg, void *karg, unsigned int cmd);
> >  };
> >  
> > +#define MASK_IOC_SIZE(cmd) \
> > +	((cmd) & ~(_IOC_SIZEMASK << _IOC_SIZESHIFT))
> > +
> >  static inline long is_valid_ioctl(const struct media_ioctl_info *info,
> >  				  unsigned int len, unsigned int cmd)
> >  {
> > -	return (_IOC_NR(cmd) >= len
> > -		|| info[_IOC_NR(cmd)].cmd != cmd) ? -ENOIOCTLCMD : 0;
> > +	const unsigned short *alt_arg_sizes;
> > +
> > +	if (unlikely(_IOC_NR(cmd) >= len))
> 
> Please don't use 'unlikely'. Unless you can prove with hard numbers that it actually
> make a performance difference it only pollutes the code.
> 
> > +		return -ENOIOCTLCMD;
> > +
> > +	info += _IOC_NR(cmd);
> > +
> > +	if (info->cmd == cmd)
> > +		return 0;
> > +
> > +	/*
> > +	 * Verify that the size-dependent patch of the IOCTL command
> > +	 * matches and that the size does not exceed the principal
> > +	 * argument size.
> > +	 */
> > +	if (unlikely(MASK_IOC_SIZE(info->cmd) != MASK_IOC_SIZE(cmd)
> > +		     || _IOC_SIZE(info->cmd) < _IOC_SIZE(cmd)))
> > +		return -ENOIOCTLCMD;
> > +
> > +	alt_arg_sizes = info->alt_arg_sizes;
> > +	if (unlikely(!alt_arg_sizes))
> > +		return -ENOIOCTLCMD;
> > +
> > +	for (; *alt_arg_sizes; alt_arg_sizes++)
> > +		if (_IOC_SIZE(cmd) == *alt_arg_sizes)
> > +			return 0;
> > +
> > +	return -ENOIOCTLCMD;
> >  }
> >  
> >  static long __media_device_ioctl(
> > @@ -440,6 +479,9 @@ static long __media_device_ioctl(
> >  			goto out_free;
> >  	}
> >  
> > +	/* Set the rest of the argument struct to zero */
> > +	memset(karg + _IOC_SIZE(cmd), 0, _IOC_SIZE(info->cmd) - _IOC_SIZE(cmd));
> > +
> >  	if (info->flags & MEDIA_IOC_FL_GRAPH_MUTEX)
> >  		mutex_lock(&dev->graph_mutex);
> >  
> > 
> 
> I'm in two minds: I like this idea, but on the other hand it is not actually needed at
> the moment. So should we just park this in patchwork until needed, or apply anyway?
> 
> Comments?
> 
> Personally I think it is better to park it. I don't like code that isn't actually needed.

Nothing really needs the patch right now, I agree.

My concern is that will enough many people remember this patch when the next
new IOCTL is added? If not, we'll have reserved fields again. When this
patch is needed is not a question of "if", it is a question of "when".

I just updated the set. We can then decided whether we want to merge all of
the patches at once (or not).

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH v3 4/5] media: Add flags to tell whether to take graph mutex for an IOCTL

[Hans Verkuil]

On 07/21/2016 01:17 PM, Sakari Ailus wrote:
> New IOCTLs (especially for the request API) do not necessarily need the
> graph mutex acquired. Leave this up to the drivers.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Thanks!

	Hans

> ---
>  drivers/media/media-device.c | 47 ++++++++++++++++++++++++++------------------
>  1 file changed, 28 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 87d17a0..6dfcc50 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -381,20 +381,25 @@ static long copy_arg_to_user(void __user *uarg, void *karg, unsigned int cmd)
>  	return 0;
>  }
>  
> -#define MEDIA_IOC_ARG(__cmd, func, from_user, to_user)	\
> -	[_IOC_NR(MEDIA_IOC_##__cmd)] = {		\
> -		.cmd = MEDIA_IOC_##__cmd,		\
> +/* Do acquire the graph mutex */
> +#define MEDIA_IOC_FL_GRAPH_MUTEX	BIT(0)
> +
> +#define MEDIA_IOC_ARG(__cmd, func, fl, from_user, to_user)		\
> +	[_IOC_NR(MEDIA_IOC_##__cmd)] = {				\
> +		.cmd = MEDIA_IOC_##__cmd,				\
>  		.fn = (long (*)(struct media_device *, void *))func,	\
> -		.arg_from_user = from_user,		\
> -		.arg_to_user = to_user,			\
> +		.flags = fl,						\
> +		.arg_from_user = from_user,				\
> +		.arg_to_user = to_user,					\
>  	}
>  
> -#define MEDIA_IOC(__cmd, func)						\
> -	MEDIA_IOC_ARG(__cmd, func, copy_arg_from_user, copy_arg_to_user)
> +#define MEDIA_IOC(__cmd, func, fl)					\
> +	MEDIA_IOC_ARG(__cmd, func, fl, copy_arg_from_user, copy_arg_to_user)
>  
>  /* the table is indexed by _IOC_NR(cmd) */
>  struct media_ioctl_info {
>  	unsigned int cmd;
> +	unsigned short flags;
>  	long (*fn)(struct media_device *dev, void *arg);
>  	long (*arg_from_user)(void *karg, void __user *uarg, unsigned int cmd);
>  	long (*arg_to_user)(void __user *uarg, void *karg, unsigned int cmd);
> @@ -435,9 +440,13 @@ static long __media_device_ioctl(
>  			goto out_free;
>  	}
>  
> -	mutex_lock(&dev->graph_mutex);
> +	if (info->flags & MEDIA_IOC_FL_GRAPH_MUTEX)
> +		mutex_lock(&dev->graph_mutex);
> +
>  	ret = info->fn(dev, karg);
> -	mutex_unlock(&dev->graph_mutex);
> +
> +	if (info->flags & MEDIA_IOC_FL_GRAPH_MUTEX)
> +		mutex_unlock(&dev->graph_mutex);
>  
>  	if (!ret && info->arg_to_user)
>  		ret = info->arg_to_user(arg, karg, cmd);
> @@ -450,11 +459,11 @@ out_free:
>  }
>  
>  static const struct media_ioctl_info ioctl_info[] = {
> -	MEDIA_IOC(DEVICE_INFO, media_device_get_info),
> -	MEDIA_IOC(ENUM_ENTITIES, media_device_enum_entities),
> -	MEDIA_IOC(ENUM_LINKS, media_device_enum_links),
> -	MEDIA_IOC(SETUP_LINK, media_device_setup_link),
> -	MEDIA_IOC(G_TOPOLOGY, media_device_get_topology),
> +	MEDIA_IOC(DEVICE_INFO, media_device_get_info, MEDIA_IOC_FL_GRAPH_MUTEX),
> +	MEDIA_IOC(ENUM_ENTITIES, media_device_enum_entities, MEDIA_IOC_FL_GRAPH_MUTEX),
> +	MEDIA_IOC(ENUM_LINKS, media_device_enum_links, MEDIA_IOC_FL_GRAPH_MUTEX),
> +	MEDIA_IOC(SETUP_LINK, media_device_setup_link, MEDIA_IOC_FL_GRAPH_MUTEX),
> +	MEDIA_IOC(G_TOPOLOGY, media_device_get_topology, MEDIA_IOC_FL_GRAPH_MUTEX),
>  };
>  
>  static long media_device_ioctl(struct file *filp, unsigned int cmd,
> @@ -497,11 +506,11 @@ static long from_user_enum_links32(void *karg, void __user *uarg,
>  #define MEDIA_IOC_ENUM_LINKS32		_IOWR('|', 0x02, struct media_links_enum32)
>  
>  static const struct media_ioctl_info compat_ioctl_info[] = {
> -	MEDIA_IOC(DEVICE_INFO, media_device_get_info),
> -	MEDIA_IOC(ENUM_ENTITIES, media_device_enum_entities),
> -	MEDIA_IOC_ARG(ENUM_LINKS32, media_device_enum_links, from_user_enum_links32, NULL),
> -	MEDIA_IOC(SETUP_LINK, media_device_setup_link),
> -	MEDIA_IOC(G_TOPOLOGY, media_device_get_topology),
> +	MEDIA_IOC(DEVICE_INFO, media_device_get_info, MEDIA_IOC_FL_GRAPH_MUTEX),
> +	MEDIA_IOC(ENUM_ENTITIES, media_device_enum_entities, MEDIA_IOC_FL_GRAPH_MUTEX),
> +	MEDIA_IOC_ARG(ENUM_LINKS32, media_device_enum_links, MEDIA_IOC_FL_GRAPH_MUTEX, from_user_enum_links32, NULL),
> +	MEDIA_IOC(SETUP_LINK, media_device_setup_link, MEDIA_IOC_FL_GRAPH_MUTEX),
> +	MEDIA_IOC(G_TOPOLOGY, media_device_get_topology, MEDIA_IOC_FL_GRAPH_MUTEX),
>  };
>  
>  static long media_device_compat_ioctl(struct file *filp, unsigned int cmd,
>