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

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

[Sakari Ailus]

Hi everyone,

I've updated my patchset for refactoring the media device IOCTL handling.

The original set is here:

<URL:http://www.spinics.net/lists/linux-media/msg100041.html>

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 patches have been tested on x86_64 and ARM and compat code has been
tested on x86_64. The variable length argument support (in case a struct
grows due to added fields) has been tested, including returning
-ENOIOCTLCMD on bad argument length, on x86_64 with the request API
patches I'll post in the near future.

(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.)

Reviews would be very welcome.

-- 
Kind regards,
Sakari

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

[Sakari Ailus]

Preparation for refactoring media IOCTL handling to unify common parts.

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

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 898a3cf..c24149d 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 = to_media_device(devnode);
 	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)
 {
@@ -503,7 +547,9 @@ static long media_device_compat_ioctl(struct file *filp, unsigned int cmd,
 	case MEDIA_IOC_ENUM_ENTITIES:
 	case MEDIA_IOC_SETUP_LINK:
 	case MEDIA_IOC_G_TOPOLOGY:
-		return media_device_ioctl(filp, cmd, arg);
+		return __media_device_ioctl(
+			filp, cmd, (void __user *)arg,
+			compat_ioctl_info, ARRAY_SIZE(compat_ioctl_info));
 
 	case MEDIA_IOC_ENUM_LINKS32:
 		mutex_lock(&dev->graph_mutex);
-- 
1.9.1

[PATCH v2 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.

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

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index c24149d..9b5a88d 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 = to_media_device(devnode);
+	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,41 +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 = to_media_device(devnode);
-	long ret;
-
-	switch (cmd) {
-	case MEDIA_IOC_DEVICE_INFO:
-	case MEDIA_IOC_ENUM_ENTITIES:
-	case MEDIA_IOC_SETUP_LINK:
-	case MEDIA_IOC_G_TOPOLOGY:
-		return __media_device_ioctl(
-			filp, cmd, (void __user *)arg,
-			compat_ioctl_info, ARRAY_SIZE(compat_ioctl_info));
-
-	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:
-		ret = -ENOIOCTLCMD;
-	}
-
-	return ret;
+	return __media_device_ioctl(
+		filp, cmd, (void __user *)arg,
+		compat_ioctl_info, ARRAY_SIZE(compat_ioctl_info));
 }
 #endif /* CONFIG_COMPAT */
 
-- 
1.9.1

[PATCH v2 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 | 214 ++++++++++++++++++++++---------------------
 1 file changed, 110 insertions(+), 104 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 9b5a88d..39fe07f 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,50 @@ 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,    \
+#ifdef CONFIG_COMPAT
+/* Only compat IOCTLs need this right now. */
+static long copy_arg_to_user_nop(void __user *uarg, void *karg,
+				 unsigned int cmd)
+{
+	return 0;
+}
+#endif
+
+#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,
@@ -438,13 +416,29 @@ static inline long is_valid_ioctl(const struct media_ioctl_info *info,
 		|| info[_IOC_NR(cmd)].cmd != cmd) ? -ENOIOCTLCMD : 0;
 }
 
+static unsigned int media_ioctl_max_arg_size(
+	const struct media_ioctl_info *info, unsigned int len,
+	unsigned int *max_size)
+{
+	if (*max_size)
+		return *max_size;
+
+	for (; len; len--, info++)
+		*max_size = max(_IOC_SIZE(info->cmd), *max_size);
+
+	return *max_size;
+}
+
 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)
+	const struct media_ioctl_info *info_array, unsigned int info_array_len,
+	unsigned int *max_arg_size)
 {
 	struct media_devnode *devnode = media_devnode_data(filp);
 	struct media_device *dev = to_media_device(devnode);
 	const struct media_ioctl_info *info;
+	char karg[media_ioctl_max_arg_size(info_array, info_array_len,
+					   max_arg_size)];
 	long ret;
 
 	ret = is_valid_ioctl(info_array, info_array_len, cmd);
@@ -453,11 +447,16 @@ static long __media_device_ioctl(
 
 	info = &info_array[_IOC_NR(cmd)];
 
+	info->arg_from_user(karg, arg, cmd);
+
 	mutex_lock(&dev->graph_mutex);
-	ret = info->fn(dev, arg);
+	ret = info->fn(dev, karg);
 	mutex_unlock(&dev->graph_mutex);
 
-	return ret;
+	if (ret)
+		return ret;
+
+	return info->arg_to_user(arg, karg, cmd);
 }
 
 static const struct media_ioctl_info ioctl_info[] = {
@@ -471,9 +470,12 @@ static const struct media_ioctl_info ioctl_info[] = {
 static long media_device_ioctl(struct file *filp, unsigned int cmd,
 			       unsigned long arg)
 {
+	static unsigned int max_arg_size;
+
 	return __media_device_ioctl(
 		filp, cmd, (void __user *)arg,
-		ioctl_info, ARRAY_SIZE(ioctl_info));
+		ioctl_info, ARRAY_SIZE(ioctl_info),
+		&max_arg_size);
 }
 
 #ifdef CONFIG_COMPAT
@@ -485,23 +487,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 +512,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, copy_arg_to_user_nop),
 	MEDIA_IOC(SETUP_LINK, media_device_setup_link),
 	MEDIA_IOC(G_TOPOLOGY, media_device_get_topology),
 };
@@ -517,9 +520,12 @@ static const struct media_ioctl_info compat_ioctl_info[] = {
 static long media_device_compat_ioctl(struct file *filp, unsigned int cmd,
 				      unsigned long arg)
 {
+	static unsigned int max_arg_size;
+
 	return __media_device_ioctl(
 		filp, cmd, (void __user *)arg,
-		compat_ioctl_info, ARRAY_SIZE(compat_ioctl_info));
+		compat_ioctl_info, ARRAY_SIZE(compat_ioctl_info),
+		&max_arg_size);
 }
 #endif /* CONFIG_COMPAT */
 
-- 
1.9.1

[PATCH v2 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>
---
 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 39fe07f..8aef5b8 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -390,21 +390,26 @@ static long copy_arg_to_user_nop(void __user *uarg, void *karg,
 }
 #endif
 
-#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;
 	long (*fn)(struct media_device *dev, void *arg);
+	unsigned short flags;
 	long (*arg_from_user)(void *karg, void __user *uarg, unsigned int cmd);
 	long (*arg_to_user)(void __user *uarg, void *karg, unsigned int cmd);
 };
@@ -449,9 +454,13 @@ static long __media_device_ioctl(
 
 	info->arg_from_user(karg, arg, cmd);
 
-	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)
 		return ret;
@@ -460,11 +469,11 @@ static long __media_device_ioctl(
 }
 
 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,
@@ -510,11 +519,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, copy_arg_to_user_nop),
-	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, copy_arg_to_user_nop),
+	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,
-- 
1.9.1

[PATCH v2 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 | 45 +++++++++++++++++++++++++++++---------------
 1 file changed, 30 insertions(+), 15 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 8aef5b8..c638d3b 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -393,32 +393,44 @@ static long copy_arg_to_user_nop(void __user *uarg, void *karg,
 /* 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_ARG(__cmd, arg_type, func, fl, from_user, to_user)	\
 	[_IOC_NR(MEDIA_IOC_##__cmd)] = {				\
 		.cmd = MEDIA_IOC_##__cmd,				\
 		.fn = (long (*)(struct media_device *, void *))func,	\
 		.flags = fl,						\
+		.min_arg_size = sizeof(arg_type),			\
 		.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(__cmd, arg_type, func, fl)				\
+	MEDIA_IOC_ARG(__cmd, arg_type, 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;
 	long (*fn)(struct media_device *dev, void *arg);
 	unsigned short flags;
+	unsigned short min_arg_size;
 	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;
+	if (_IOC_NR(cmd) >= len)
+		return -ENOIOCTLCMD;
+
+	info += _IOC_NR(cmd);
+
+	return (MASK_IOC_SIZE(info->cmd) != MASK_IOC_SIZE(cmd)
+		|| _IOC_SIZE(cmd) < info->min_arg_size
+		|| _IOC_SIZE(cmd) > _IOC_SIZE(info->cmd)) ? -ENOIOCTLCMD : 0;
 }
 
 static unsigned int media_ioctl_max_arg_size(
@@ -454,6 +466,9 @@ static long __media_device_ioctl(
 
 	info->arg_from_user(karg, arg, cmd);
 
+	/* 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);
 
@@ -469,11 +484,11 @@ static long __media_device_ioctl(
 }
 
 static const struct media_ioctl_info ioctl_info[] = {
-	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),
+	MEDIA_IOC(DEVICE_INFO, struct media_device_info, media_device_get_info, MEDIA_IOC_FL_GRAPH_MUTEX),
+	MEDIA_IOC(ENUM_ENTITIES, struct media_entity_desc, media_device_enum_entities, MEDIA_IOC_FL_GRAPH_MUTEX),
+	MEDIA_IOC(ENUM_LINKS, struct media_links_enum, media_device_enum_links, MEDIA_IOC_FL_GRAPH_MUTEX),
+	MEDIA_IOC(SETUP_LINK, struct media_link_desc, media_device_setup_link, MEDIA_IOC_FL_GRAPH_MUTEX),
+	MEDIA_IOC(G_TOPOLOGY, struct media_v2_topology, media_device_get_topology, MEDIA_IOC_FL_GRAPH_MUTEX),
 };
 
 static long media_device_ioctl(struct file *filp, unsigned int cmd,
@@ -519,11 +534,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_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, copy_arg_to_user_nop),
-	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),
+	MEDIA_IOC(DEVICE_INFO, struct media_device_info, media_device_get_info, MEDIA_IOC_FL_GRAPH_MUTEX),
+	MEDIA_IOC(ENUM_ENTITIES, struct media_entity_desc, media_device_enum_entities, MEDIA_IOC_FL_GRAPH_MUTEX),
+	MEDIA_IOC_ARG(ENUM_LINKS32, struct media_links_enum32, media_device_enum_links, MEDIA_IOC_FL_GRAPH_MUTEX, from_user_enum_links32, copy_arg_to_user_nop),
+	MEDIA_IOC(SETUP_LINK, struct media_link_desc, media_device_setup_link, MEDIA_IOC_FL_GRAPH_MUTEX),
+	MEDIA_IOC(G_TOPOLOGY, struct media_v2_topology, media_device_get_topology, MEDIA_IOC_FL_GRAPH_MUTEX),
 };
 
 static long media_device_compat_ioctl(struct file *filp, unsigned int cmd,
-- 
1.9.1

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

[Hans Verkuil]

Hi Sakari,

Thanks for working on this!

I've got one comment:

On 05/04/2016 01:20 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>
> ---
>  drivers/media/media-device.c | 214 ++++++++++++++++++++++---------------------
>  1 file changed, 110 insertions(+), 104 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 9b5a88d..39fe07f 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c

<snip>

>  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)
> +	const struct media_ioctl_info *info_array, unsigned int info_array_len,
> +	unsigned int *max_arg_size)
>  {
>  	struct media_devnode *devnode = media_devnode_data(filp);
>  	struct media_device *dev = to_media_device(devnode);
>  	const struct media_ioctl_info *info;
> +	char karg[media_ioctl_max_arg_size(info_array, info_array_len,
> +					   max_arg_size)];

This isn't going to work. Sparse (and/or smatch) will complain about this. I recommend
doing the same as videodev does: have a fixed array on the stack, and use kmalloc if
more is needed.

I don't like the max_arg_size anyway :-)

>  	long ret;
>  
>  	ret = is_valid_ioctl(info_array, info_array_len, cmd);

Regards,

	Hans

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

[Sakari Ailus]

Hi Hans,

On Wed, May 04, 2016 at 02:24:47PM +0200, Hans Verkuil wrote:
> Hi Sakari,
> 
> Thanks for working on this!
> 
> I've got one comment:
> 
> On 05/04/2016 01:20 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>
> > ---
> >  drivers/media/media-device.c | 214 ++++++++++++++++++++++---------------------
> >  1 file changed, 110 insertions(+), 104 deletions(-)
> > 
> > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > index 9b5a88d..39fe07f 100644
> > --- a/drivers/media/media-device.c
> > +++ b/drivers/media/media-device.c
> 
> <snip>
> 
> >  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)
> > +	const struct media_ioctl_info *info_array, unsigned int info_array_len,
> > +	unsigned int *max_arg_size)
> >  {
> >  	struct media_devnode *devnode = media_devnode_data(filp);
> >  	struct media_device *dev = to_media_device(devnode);
> >  	const struct media_ioctl_info *info;
> > +	char karg[media_ioctl_max_arg_size(info_array, info_array_len,
> > +					   max_arg_size)];
> 
> This isn't going to work. Sparse (and/or smatch) will complain about this. I recommend
> doing the same as videodev does: have a fixed array on the stack, and use kmalloc if
> more is needed.
> 
> I don't like the max_arg_size anyway :-)

Sparse might warn, yes, but it doesn't make the solution worse. :-)

That would leave it up to the developers to maintain the argument size sane.
Sure I can change this.

-- 
Cheers,

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

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

[Hans Verkuil]

Hi Sakari,

On 05/04/2016 01:20 PM, Sakari Ailus wrote:
> 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 | 45 +++++++++++++++++++++++++++++---------------
>  1 file changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 8aef5b8..c638d3b 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -393,32 +393,44 @@ static long copy_arg_to_user_nop(void __user *uarg, void *karg,
>  /* 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_ARG(__cmd, arg_type, func, fl, from_user, to_user)	\
>  	[_IOC_NR(MEDIA_IOC_##__cmd)] = {				\
>  		.cmd = MEDIA_IOC_##__cmd,				\
>  		.fn = (long (*)(struct media_device *, void *))func,	\
>  		.flags = fl,						\
> +		.min_arg_size = sizeof(arg_type),			\

Hmm. I would prefer to change this to a pointer to an array of possible sizes.

E.g.

	u16 media_ioc_foo_sizes[] = {
		sizeof(struct foo_v1),
		sizeof(struct foo_v2),
		sizeof(struct foo_v3),
		0
	};

And pass this array to MEDIA_IOC_ARG, or NULL if there are no other versions.
In that case the code can use _IOC_SIZE.

That way the ioctl check is precise instead of a simple range check.

So add a size field for the default handling:

		.size = _IOC_SIZE(MEDIA_IOC_##__cmd),

and a old_sizes pointer:

		.old_sizes = oldsizesptr,

In the ioctl check code first compare _IOC_SIZE with the size field, and
if different walk the old_sizes array (if that pointer is != NULL).

Regards,

	Hans

>  		.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(__cmd, arg_type, func, fl)				\
> +	MEDIA_IOC_ARG(__cmd, arg_type, 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;
>  	long (*fn)(struct media_device *dev, void *arg);
>  	unsigned short flags;
> +	unsigned short min_arg_size;
>  	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;
> +	if (_IOC_NR(cmd) >= len)
> +		return -ENOIOCTLCMD;
> +
> +	info += _IOC_NR(cmd);
> +
> +	return (MASK_IOC_SIZE(info->cmd) != MASK_IOC_SIZE(cmd)
> +		|| _IOC_SIZE(cmd) < info->min_arg_size
> +		|| _IOC_SIZE(cmd) > _IOC_SIZE(info->cmd)) ? -ENOIOCTLCMD : 0;
>  }
>  
>  static unsigned int media_ioctl_max_arg_size(
> @@ -454,6 +466,9 @@ static long __media_device_ioctl(
>  
>  	info->arg_from_user(karg, arg, cmd);
>  
> +	/* 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);
>  
> @@ -469,11 +484,11 @@ static long __media_device_ioctl(
>  }
>  
>  static const struct media_ioctl_info ioctl_info[] = {
> -	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),
> +	MEDIA_IOC(DEVICE_INFO, struct media_device_info, media_device_get_info, MEDIA_IOC_FL_GRAPH_MUTEX),
> +	MEDIA_IOC(ENUM_ENTITIES, struct media_entity_desc, media_device_enum_entities, MEDIA_IOC_FL_GRAPH_MUTEX),
> +	MEDIA_IOC(ENUM_LINKS, struct media_links_enum, media_device_enum_links, MEDIA_IOC_FL_GRAPH_MUTEX),
> +	MEDIA_IOC(SETUP_LINK, struct media_link_desc, media_device_setup_link, MEDIA_IOC_FL_GRAPH_MUTEX),
> +	MEDIA_IOC(G_TOPOLOGY, struct media_v2_topology, media_device_get_topology, MEDIA_IOC_FL_GRAPH_MUTEX),
>  };
>  
>  static long media_device_ioctl(struct file *filp, unsigned int cmd,
> @@ -519,11 +534,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_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, copy_arg_to_user_nop),
> -	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),
> +	MEDIA_IOC(DEVICE_INFO, struct media_device_info, media_device_get_info, MEDIA_IOC_FL_GRAPH_MUTEX),
> +	MEDIA_IOC(ENUM_ENTITIES, struct media_entity_desc, media_device_enum_entities, MEDIA_IOC_FL_GRAPH_MUTEX),
> +	MEDIA_IOC_ARG(ENUM_LINKS32, struct media_links_enum32, media_device_enum_links, MEDIA_IOC_FL_GRAPH_MUTEX, from_user_enum_links32, copy_arg_to_user_nop),
> +	MEDIA_IOC(SETUP_LINK, struct media_link_desc, media_device_setup_link, MEDIA_IOC_FL_GRAPH_MUTEX),
> +	MEDIA_IOC(G_TOPOLOGY, struct media_v2_topology, media_device_get_topology, MEDIA_IOC_FL_GRAPH_MUTEX),
>  };
>  
>  static long media_device_compat_ioctl(struct file *filp, unsigned int cmd,
> 

[PATCH v2.1 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>
---
since v2:

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

 drivers/media/media-device.c | 194 +++++++++++++++++++++----------------------
 1 file changed, 94 insertions(+), 100 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 9b5a88d..0797e4b 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,50 @@ 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,    \
+#ifdef CONFIG_COMPAT
+/* Only compat IOCTLs need this right now. */
+static long copy_arg_to_user_nop(void __user *uarg, void *karg,
+				 unsigned int cmd)
+{
+	return 0;
+}
+#endif
+
+#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 +423,7 @@ static long __media_device_ioctl(
 	struct media_devnode *devnode = media_devnode_data(filp);
 	struct media_device *dev = to_media_device(devnode);
 	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 +432,24 @@ 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;
+	}
+
+	info->arg_from_user(karg, arg, cmd);
+
 	mutex_lock(&dev->graph_mutex);
-	ret = info->fn(dev, arg);
+	ret = info->fn(dev, karg);
 	mutex_unlock(&dev->graph_mutex);
 
+	if (!ret)
+		ret = info->arg_to_user(arg, karg, cmd);
+
+	if (karg != __karg)
+		kfree(karg);
+
 	return ret;
 }
 
@@ -485,23 +478,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 +503,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, copy_arg_to_user_nop),
 	MEDIA_IOC(SETUP_LINK, media_device_setup_link),
 	MEDIA_IOC(G_TOPOLOGY, media_device_get_topology),
 };
-- 
1.9.1

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

[Shuah Khan]

On 05/04/2016 05:20 AM, Sakari Ailus wrote:
> New IOCTLs (especially for the request API) do not necessarily need the
> graph mutex acquired. Leave this up to the drivers.

Sakari,

Does this mean drivers have to hold the graph mutex as needed?
My concern with this is that we will have graph_mutex holds in
driver code in addition to the ones we have now. My concern with
referencing graph_mutex from driver code is lack of abstraction.
If we ever need to change grahp-mutex in the media-core, if it
is exposed to drivers, then there will be lot of changes.

Could we look into avoiding drivers referencing graph_mutex
directly?

thanks,
-- Shuah

> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.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 39fe07f..8aef5b8 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -390,21 +390,26 @@ static long copy_arg_to_user_nop(void __user *uarg, void *karg,
>  }
>  #endif
>  
> -#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;
>  	long (*fn)(struct media_device *dev, void *arg);
> +	unsigned short flags;
>  	long (*arg_from_user)(void *karg, void __user *uarg, unsigned int cmd);
>  	long (*arg_to_user)(void __user *uarg, void *karg, unsigned int cmd);
>  };
> @@ -449,9 +454,13 @@ static long __media_device_ioctl(
>  
>  	info->arg_from_user(karg, arg, cmd);
>  
> -	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)
>  		return ret;
> @@ -460,11 +469,11 @@ static long __media_device_ioctl(
>  }
>  
>  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,
> @@ -510,11 +519,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, copy_arg_to_user_nop),
> -	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, copy_arg_to_user_nop),
> +	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,
> 

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

[Sakari Ailus]

Hi Shuah,

Thanks for the review!

On Wed, May 04, 2016 at 08:50:56AM -0600, Shuah Khan wrote:
> On 05/04/2016 05:20 AM, Sakari Ailus wrote:
> > New IOCTLs (especially for the request API) do not necessarily need the
> > graph mutex acquired. Leave this up to the drivers.
> 
> Sakari,
> 
> Does this mean drivers have to hold the graph mutex as needed?
> My concern with this is that we will have graph_mutex holds in
> driver code in addition to the ones we have now. My concern with
> referencing graph_mutex from driver code is lack of abstraction.
> If we ever need to change grahp-mutex in the media-core, if it
> is exposed to drivers, then there will be lot of changes.
> 
> Could we look into avoiding drivers referencing graph_mutex
> directly?

I think we rather need to get rid of the graph mutex in the end; it's a bit
like the big kernel lock right now: most operations on the graph, whatever
they are, need it.

The case for not acquiring it (I have request API and events in mind, in
particular) for some IOCTLs is there. Drivers may need to acquire other
mutexes while holding the graph mutex, and the locking order has to be
maintained in order to avoid deadlocks.

Dequeueing events does not need the graph mutex, whereas requests changing
the graph state would need it for the time being.

The reason there's a flag to acquire the graph mutex (rather than not
acquiring it) is that it'd be easier to spot where it's needed.

-- 
Kind regards,

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

[PATCH v2.1 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>
---
since v2:

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

 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 e88e6d3..5cfeccf 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -393,32 +393,71 @@ static long copy_arg_to_user_nop(void __user *uarg, void *karg,
 /* 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;
 	long (*fn)(struct media_device *dev, void *arg);
 	unsigned short flags;
+	const unsigned short *alt_arg_sizes;
 	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(
@@ -445,6 +484,9 @@ static long __media_device_ioctl(
 
 	info->arg_from_user(karg, arg, cmd);
 
+	/* 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.1.4

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

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Wednesday 04 May 2016 16:09:51 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>
> ---
> since v2:
> 
> - Remove function to calculate maximum argument size, replace by a char
>   array of 256 or kmalloc() if that's too small.
> 
>  drivers/media/media-device.c | 194 +++++++++++++++++++---------------------
>  1 file changed, 94 insertions(+), 100 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 9b5a88d..0797e4b 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,50 @@ 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,    \
> +#ifdef CONFIG_COMPAT
> +/* Only compat IOCTLs need this right now. */
> +static long copy_arg_to_user_nop(void __user *uarg, void *karg,
> +				 unsigned int cmd)
> +{
> +	return 0;
> +}
> +#endif
> +
> +#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 +423,7 @@ static long __media_device_ioctl(
>  	struct media_devnode *devnode = media_devnode_data(filp);
>  	struct media_device *dev = to_media_device(devnode);
>  	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 +432,24 @@ 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;
> +	}
> +
> +	info->arg_from_user(karg, arg, cmd);
> +
>  	mutex_lock(&dev->graph_mutex);
> -	ret = info->fn(dev, arg);
> +	ret = info->fn(dev, karg);
>  	mutex_unlock(&dev->graph_mutex);
> 
> +	if (!ret)

How about if (!ret && info->arg_to_user) instead, and getting rid of 
copy_arg_to_user_nop() ?

Apart from that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +		ret = info->arg_to_user(arg, karg, cmd);
> +
> +	if (karg != __karg)
> +		kfree(karg);
> +
>  	return ret;
>  }
> 
> @@ -485,23 +478,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 +503,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, copy_arg_to_user_nop), MEDIA_IOC(SETUP_LINK,
> media_device_setup_link),
>  	MEDIA_IOC(G_TOPOLOGY, media_device_get_topology),
>  };

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Wednesday 04 May 2016 14:20:51 Sakari Ailus wrote:
> Preparation for refactoring media IOCTL handling to unify common parts.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/media-device.c | 52 ++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 49 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 898a3cf..c24149d 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 = to_media_device(devnode);
>  	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)
>  {
> @@ -503,7 +547,9 @@ static long media_device_compat_ioctl(struct file *filp,
> unsigned int cmd, case MEDIA_IOC_ENUM_ENTITIES:
>  	case MEDIA_IOC_SETUP_LINK:
>  	case MEDIA_IOC_G_TOPOLOGY:
> -		return media_device_ioctl(filp, cmd, arg);
> +		return __media_device_ioctl(
> +			filp, cmd, (void __user *)arg,
> +			compat_ioctl_info, ARRAY_SIZE(compat_ioctl_info));
> 
>  	case MEDIA_IOC_ENUM_LINKS32:
>  		mutex_lock(&dev->graph_mutex);

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Wednesday 04 May 2016 14:20:52 Sakari Ailus wrote:
> Each IOCTL handler can be listed in an array instead of using a large and
> cumbersome switch. Do that.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/media-device.c | 89 +++++++++++------------------------------
>  1 file changed, 23 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index c24149d..9b5a88d 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 = to_media_device(devnode);
> +	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,41 +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 = to_media_device(devnode);
> -	long ret;
> -
> -	switch (cmd) {
> -	case MEDIA_IOC_DEVICE_INFO:
> -	case MEDIA_IOC_ENUM_ENTITIES:
> -	case MEDIA_IOC_SETUP_LINK:
> -	case MEDIA_IOC_G_TOPOLOGY:
> -		return __media_device_ioctl(
> -			filp, cmd, (void __user *)arg,
> -			compat_ioctl_info, ARRAY_SIZE(compat_ioctl_info));
> -
> -	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:
> -		ret = -ENOIOCTLCMD;
> -	}
> -
> -	return ret;
> +	return __media_device_ioctl(
> +		filp, cmd, (void __user *)arg,
> +		compat_ioctl_info, ARRAY_SIZE(compat_ioctl_info));
>  }
>  #endif /* CONFIG_COMPAT */

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Wednesday 04 May 2016 14:20:54 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>

> ---
>  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 39fe07f..8aef5b8 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -390,21 +390,26 @@ static long copy_arg_to_user_nop(void __user *uarg,
> void *karg, }
>  #endif
> 
> -#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;
>  	long (*fn)(struct media_device *dev, void *arg);
> +	unsigned short flags;
>  	long (*arg_from_user)(void *karg, void __user *uarg, unsigned int cmd);
>  	long (*arg_to_user)(void __user *uarg, void *karg, unsigned int cmd);
>  };
> @@ -449,9 +454,13 @@ static long __media_device_ioctl(
> 
>  	info->arg_from_user(karg, arg, cmd);
> 
> -	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)
>  		return ret;
> @@ -460,11 +469,11 @@ static long __media_device_ioctl(
>  }
> 
>  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,
> @@ -510,11 +519,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, copy_arg_to_user_nop), -	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,
> copy_arg_to_user_nop), +	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,

-- 
Regards,

Laurent Pinchart

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

[Sakari Ailus]

Hi Laurent,

Many thanks for the review!

Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Wednesday 04 May 2016 16:09:51 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>
>> ---
>> since v2:
>>
>> - Remove function to calculate maximum argument size, replace by a char
>>   array of 256 or kmalloc() if that's too small.
>>
>>  drivers/media/media-device.c | 194 +++++++++++++++++++---------------------
>>  1 file changed, 94 insertions(+), 100 deletions(-)
>>
>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>> index 9b5a88d..0797e4b 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,50 @@ 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,    \
>> +#ifdef CONFIG_COMPAT
>> +/* Only compat IOCTLs need this right now. */
>> +static long copy_arg_to_user_nop(void __user *uarg, void *karg,
>> +				 unsigned int cmd)
>> +{
>> +	return 0;
>> +}
>> +#endif
>> +
>> +#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 +423,7 @@ static long __media_device_ioctl(
>>  	struct media_devnode *devnode = media_devnode_data(filp);
>>  	struct media_device *dev = to_media_device(devnode);
>>  	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 +432,24 @@ 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;
>> +	}
>> +
>> +	info->arg_from_user(karg, arg, cmd);
>> +
>>  	mutex_lock(&dev->graph_mutex);
>> -	ret = info->fn(dev, arg);
>> +	ret = info->fn(dev, karg);
>>  	mutex_unlock(&dev->graph_mutex);
>>
>> +	if (!ret)
> 
> How about if (!ret && info->arg_to_user) instead, and getting rid of 
> copy_arg_to_user_nop() ?

I thought of that, but I decided to optimise the common case ---  which
is that the argument is copied back and forth. Not copying the argument
back is a very special case, we use it for a single compat IOCTL.

That said, we could use it for the proper ENUM_LINKS as well. Still that
does not change what's normal.

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

[PATCH v2.2 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>
---
since v2.1:

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

 drivers/media/media-device.c | 197 +++++++++++++++++++++----------------------
 1 file changed, 97 insertions(+), 100 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 9b5a88d..ef30139 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,50 @@ 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,    \
+#ifdef CONFIG_COMPAT
+/* Only compat IOCTLs need this right now. */
+static long copy_arg_to_user_nop(void __user *uarg, void *karg,
+				 unsigned int cmd)
+{
+	return 0;
+}
+#endif
+
+#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 +423,7 @@ static long __media_device_ioctl(
 	struct media_devnode *devnode = media_devnode_data(filp);
 	struct media_device *dev = to_media_device(devnode);
 	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 +432,27 @@ 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;
+	}
+
+	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)
+		ret = info->arg_to_user(arg, karg, cmd);
+
+out_free:
+	if (karg != __karg)
+		kfree(karg);
+
 	return ret;
 }
 
@@ -485,23 +481,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 +506,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, copy_arg_to_user_nop),
 	MEDIA_IOC(SETUP_LINK, media_device_setup_link),
 	MEDIA_IOC(G_TOPOLOGY, media_device_get_topology),
 };
-- 
1.9.1

Re: [PATCH v2.1 5/5] media: Support variable size IOCTL arguments

[Hans Verkuil]

On 05/05/2016 01:06 AM, Sakari Ailus wrote:
> 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>

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

I think it is important to have exact matches instead of using a min-max range.
Issues related to alignment problems on different architectures (32/64 bits,
how padding in struct is handled, etc.) that could cause a different size should
be caught by the validation check. Any size other than this discrete list of
allowed sizes is an indication that something is seriously wrong on the kernel or
userspace side.

If we get ioctls that have a variable-sized array at the end, then that should
be signaled differently in the media_ioctl_info struct. We'll handle that when
that happens.

Regards,

	Hans

> ---
> since v2:
> 
> - Use a list of supported argument sizes instead of a minimum value.
> 
>  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 e88e6d3..5cfeccf 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -393,32 +393,71 @@ static long copy_arg_to_user_nop(void __user *uarg, void *karg,
>  /* 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;
>  	long (*fn)(struct media_device *dev, void *arg);
>  	unsigned short flags;
> +	const unsigned short *alt_arg_sizes;
>  	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(
> @@ -445,6 +484,9 @@ static long __media_device_ioctl(
>  
>  	info->arg_from_user(karg, arg, cmd);
>  
> +	/* 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);
>  
> 

Re: [PATCH v2.1 5/5] media: Support variable size IOCTL arguments

[Sakari Ailus]

Hi Hans,

On Fri, Jun 17, 2016 at 06:21:05PM +0200, Hans Verkuil wrote:
> On 05/05/2016 01:06 AM, Sakari Ailus wrote:
> > 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>
> 
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> 
> I think it is important to have exact matches instead of using a min-max
> range. Issues related to alignment problems on different architectures
> (32/64 bits, how padding in struct is handled, etc.) that could cause a
> different size should be caught by the validation check. Any size other
> than this discrete list of allowed sizes is an indication that something
> is seriously wrong on the kernel or userspace side.

Depending on how many fields are added to a new version of a struct, it may
end up that the size of a malformed user IOCTL still matches with a
different version of a struct. I don't think you benefit too much from such
a list, yet there will be a growing number of different versions of the
struct available. We could (and should) certainly hide them to another
header file though.

There's a cost, small but still a cost, in going through such a list to find
the right size.

The IOCTL arguments should always be packed, too, and follow sane field
alignment.

I have no strong opinion on the matter but would like to find an agreement
for the purpose of being able to rely on this functionality in the new
request and event IOCTLs.

> 
> If we get ioctls that have a variable-sized array at the end, then that
> should be signaled differently in the media_ioctl_info struct. We'll
> handle that when that happens.

Yes, we can add a flag for handling those when we get the first one.

-- 
Kind regards,

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

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

[Laurent Pinchart]

Hi Sakari,

On Monday 09 May 2016 16:16:26 Sakari Ailus wrote:
> Laurent Pinchart wrote:
> > On Wednesday 04 May 2016 16:09:51 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>
> >> ---
> >> since v2:
> >> 
> >> - Remove function to calculate maximum argument size, replace by a char
> >>   array of 256 or kmalloc() if that's too small.
> >>  
> >>  drivers/media/media-device.c | 194 ++++++++++++++++---------------------
> >>  1 file changed, 94 insertions(+), 100 deletions(-)
> >> 
> >> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> >> index 9b5a88d..0797e4b 100644
> >> --- a/drivers/media/media-device.c
> >> +++ b/drivers/media/media-device.c

[snip]

> >> @@ -453,10 +432,24 @@ 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;
> >> +	}
> >> +
> >> +	info->arg_from_user(karg, arg, cmd);
> >> +
> >>  	mutex_lock(&dev->graph_mutex);
> >> -	ret = info->fn(dev, arg);
> >> +	ret = info->fn(dev, karg);
> >>  	mutex_unlock(&dev->graph_mutex);
> >> 
> >> +	if (!ret)
> > 
> > How about if (!ret && info->arg_to_user) instead, and getting rid of
> > copy_arg_to_user_nop() ?
> 
> I thought of that, but I decided to optimise the common case ---  which
> is that the argument is copied back and forth. Not copying the argument
> back is a very special case, we use it for a single compat IOCTL.
> 
> That said, we could use it for the proper ENUM_LINKS as well. Still that
> does not change what's normal.

We're talking about one comparison and one branching instruction (that will 
not be taken in the common case). Is that micro-optimization really worth it 
in an ioctl path that is not that performance-critical ? If you think it is, 
could you analyse what the impact of the copy_arg_to_user_nop() function on 
cache locality is for the common case ? ;-)

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Wednesday 04 May 2016 14:20:54 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>
> ---
>  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 39fe07f..8aef5b8 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -390,21 +390,26 @@ static long copy_arg_to_user_nop(void __user *uarg,
> void *karg, }
>  #endif
> 
> -#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;
>  	long (*fn)(struct media_device *dev, void *arg);
> +	unsigned short flags;
>  	long (*arg_from_user)(void *karg, void __user *uarg, unsigned int 
cmd);
>  	long (*arg_to_user)(void __user *uarg, void *karg, unsigned int cmd);
>  };
> @@ -449,9 +454,13 @@ static long __media_device_ioctl(
> 
>  	info->arg_from_user(karg, arg, cmd);
> 
> -	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)
>  		return ret;
> @@ -460,11 +469,11 @@ static long __media_device_ioctl(
>  }
> 
>  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),

do we really need to acquire the graph mutex for this ioctl ?

> +	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,
> @@ -510,11 +519,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, copy_arg_to_user_nop), -	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,
> copy_arg_to_user_nop), +	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,

-- 
Regards,

Laurent Pinchart

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

[Sakari Ailus]

Hi Laurent,

On Sat, Jul 09, 2016 at 10:29:03PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Monday 09 May 2016 16:16:26 Sakari Ailus wrote:
> > Laurent Pinchart wrote:
> > > On Wednesday 04 May 2016 16:09:51 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>
> > >> ---
> > >> since v2:
> > >> 
> > >> - Remove function to calculate maximum argument size, replace by a char
> > >>   array of 256 or kmalloc() if that's too small.
> > >>  
> > >>  drivers/media/media-device.c | 194 ++++++++++++++++---------------------
> > >>  1 file changed, 94 insertions(+), 100 deletions(-)
> > >> 
> > >> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > >> index 9b5a88d..0797e4b 100644
> > >> --- a/drivers/media/media-device.c
> > >> +++ b/drivers/media/media-device.c
> 
> [snip]
> 
> > >> @@ -453,10 +432,24 @@ 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;
> > >> +	}
> > >> +
> > >> +	info->arg_from_user(karg, arg, cmd);
> > >> +
> > >>  	mutex_lock(&dev->graph_mutex);
> > >> -	ret = info->fn(dev, arg);
> > >> +	ret = info->fn(dev, karg);
> > >>  	mutex_unlock(&dev->graph_mutex);
> > >> 
> > >> +	if (!ret)
> > > 
> > > How about if (!ret && info->arg_to_user) instead, and getting rid of
> > > copy_arg_to_user_nop() ?
> > 
> > I thought of that, but I decided to optimise the common case ---  which
> > is that the argument is copied back and forth. Not copying the argument
> > back is a very special case, we use it for a single compat IOCTL.
> > 
> > That said, we could use it for the proper ENUM_LINKS as well. Still that
> > does not change what's normal.
> 
> We're talking about one comparison and one branching instruction (that will 
> not be taken in the common case). Is that micro-optimization really worth it 
> in an ioctl path that is not that performance-critical ? If you think it is, 
> could you analyse what the impact of the copy_arg_to_user_nop() function on 
> cache locality is for the common case ? ;-)

I sense a certain amount of insistence in your arguments. Fine, I'll change
it.

You might want to send a patch removing video_device_release_empty() as
well. :-)

-- 
Regards,

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

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

[Sakari Ailus]

Hi Laurent,

On Sat, Jul 09, 2016 at 10:47:27PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Wednesday 04 May 2016 14:20:54 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>
> > ---
> >  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 39fe07f..8aef5b8 100644
> > --- a/drivers/media/media-device.c
> > +++ b/drivers/media/media-device.c
> > @@ -390,21 +390,26 @@ static long copy_arg_to_user_nop(void __user *uarg,
> > void *karg, }
> >  #endif
> > 
> > -#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;
> >  	long (*fn)(struct media_device *dev, void *arg);
> > +	unsigned short flags;
> >  	long (*arg_from_user)(void *karg, void __user *uarg, unsigned int 
> cmd);
> >  	long (*arg_to_user)(void __user *uarg, void *karg, unsigned int cmd);
> >  };
> > @@ -449,9 +454,13 @@ static long __media_device_ioctl(
> > 
> >  	info->arg_from_user(karg, arg, cmd);
> > 
> > -	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)
> >  		return ret;
> > @@ -460,11 +469,11 @@ static long __media_device_ioctl(
> >  }
> > 
> >  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),
> 
> do we really need to acquire the graph mutex for this ioctl ?

Very probably not, but I would prefer not to change how the IOCTLs are
serialised in this patchset.

> 
> > +	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,
> > @@ -510,11 +519,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, copy_arg_to_user_nop), -	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,
> > copy_arg_to_user_nop), +	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,
> 

-- 
Kind regards,

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

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

[Laurent Pinchart]

Hi Sakari,

On Sunday 10 Jul 2016 01:03:09 Sakari Ailus wrote:
> On Sat, Jul 09, 2016 at 10:29:03PM +0300, Laurent Pinchart wrote:
> > On Monday 09 May 2016 16:16:26 Sakari Ailus wrote:
> >> Laurent Pinchart wrote:
> >>> On Wednesday 04 May 2016 16:09:51 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>
> >>>> ---
> >>>> since v2:
> >>>> 
> >>>> - Remove function to calculate maximum argument size, replace by a
> >>>>   char array of 256 or kmalloc() if that's too small.
> >>>>  
> >>>>  drivers/media/media-device.c | 194 ++++++++++++++-------------------
> >>>>  1 file changed, 94 insertions(+), 100 deletions(-)
> >>>> 
> >>>> diff --git a/drivers/media/media-device.c
> >>>> b/drivers/media/media-device.c
> >>>> index 9b5a88d..0797e4b 100644
> >>>> --- a/drivers/media/media-device.c
> >>>> +++ b/drivers/media/media-device.c
> > 
> > [snip]
> > 
> >>>> @@ -453,10 +432,24 @@ 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;
> >>>> +	}
> >>>> +
> >>>> +	info->arg_from_user(karg, arg, cmd);
> >>>> +
> >>>>  	mutex_lock(&dev->graph_mutex);
> >>>> -	ret = info->fn(dev, arg);
> >>>> +	ret = info->fn(dev, karg);
> >>>>  	mutex_unlock(&dev->graph_mutex);
> >>>> 
> >>>> +	if (!ret)
> >>> 
> >>> How about if (!ret && info->arg_to_user) instead, and getting rid of
> >>> copy_arg_to_user_nop() ?
> >> 
> >> I thought of that, but I decided to optimise the common case ---  which
> >> is that the argument is copied back and forth. Not copying the argument
> >> back is a very special case, we use it for a single compat IOCTL.
> >> 
> >> That said, we could use it for the proper ENUM_LINKS as well. Still that
> >> does not change what's normal.
> > 
> > We're talking about one comparison and one branching instruction (that
> > will not be taken in the common case). Is that micro-optimization really
> > worth it in an ioctl path that is not that performance-critical ? If you
> > think it is, could you analyse what the impact of the
> > copy_arg_to_user_nop() function on cache locality is for the common case ?
> > ;-)
> 
> I sense a certain amount of insistence in your arguments. Fine, I'll change
> it.

Thanks. I'll change that in the next version of the request API patches I will 
send out.

> You might want to send a patch removing video_device_release_empty() as
> well. :-)

Actually we should, but for an entirely different reason : most drivers that 
use video_device_release_empty() do so because they believe devm_kzalloc() is 
the best invention since sliced bread, but in reality they will crash at 
unbind time if userspace holds a reference to the video node.

-- 
Regards,

Laurent Pinchart

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

[Sakari Ailus]

On Sun, Jul 10, 2016 at 02:12:24AM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Sunday 10 Jul 2016 01:03:09 Sakari Ailus wrote:
> > On Sat, Jul 09, 2016 at 10:29:03PM +0300, Laurent Pinchart wrote:
> > > On Monday 09 May 2016 16:16:26 Sakari Ailus wrote:
> > >> Laurent Pinchart wrote:
> > >>> On Wednesday 04 May 2016 16:09:51 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>
> > >>>> ---
> > >>>> since v2:
> > >>>> 
> > >>>> - Remove function to calculate maximum argument size, replace by a
> > >>>>   char array of 256 or kmalloc() if that's too small.
> > >>>>  
> > >>>>  drivers/media/media-device.c | 194 ++++++++++++++-------------------
> > >>>>  1 file changed, 94 insertions(+), 100 deletions(-)
> > >>>> 
> > >>>> diff --git a/drivers/media/media-device.c
> > >>>> b/drivers/media/media-device.c
> > >>>> index 9b5a88d..0797e4b 100644
> > >>>> --- a/drivers/media/media-device.c
> > >>>> +++ b/drivers/media/media-device.c
> > > 
> > > [snip]
> > > 
> > >>>> @@ -453,10 +432,24 @@ 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;
> > >>>> +	}
> > >>>> +
> > >>>> +	info->arg_from_user(karg, arg, cmd);
> > >>>> +
> > >>>>  	mutex_lock(&dev->graph_mutex);
> > >>>> -	ret = info->fn(dev, arg);
> > >>>> +	ret = info->fn(dev, karg);
> > >>>>  	mutex_unlock(&dev->graph_mutex);
> > >>>> 
> > >>>> +	if (!ret)
> > >>> 
> > >>> How about if (!ret && info->arg_to_user) instead, and getting rid of
> > >>> copy_arg_to_user_nop() ?
> > >> 
> > >> I thought of that, but I decided to optimise the common case ---  which
> > >> is that the argument is copied back and forth. Not copying the argument
> > >> back is a very special case, we use it for a single compat IOCTL.
> > >> 
> > >> That said, we could use it for the proper ENUM_LINKS as well. Still that
> > >> does not change what's normal.
> > > 
> > > We're talking about one comparison and one branching instruction (that
> > > will not be taken in the common case). Is that micro-optimization really
> > > worth it in an ioctl path that is not that performance-critical ? If you
> > > think it is, could you analyse what the impact of the
> > > copy_arg_to_user_nop() function on cache locality is for the common case ?
> > > ;-)
> > 
> > I sense a certain amount of insistence in your arguments. Fine, I'll change
> > it.
> 
> Thanks. I'll change that in the next version of the request API patches I will 
> send out.

I think we rather should try to decrease the size of the set and get the
preparation patches in first.

I'm ready to send a pull request on these (after testing the rebased
patches), but it's been pending on the minimum arg size vs. list of
supported sizes discussion.

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

[PATCH v2.3 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>
---
since v2.2:

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

 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

[PATCH v2.1 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>
---
since v2:

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

 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