[PATCH 0/2] Compat IOCTL handler for Media controller

[PATCH 0/2] Compat IOCTL handler for Media controller

[Sakari Ailus]

Hi,

I noticed recently that we're missing the compat IOCTL handling in Media
controller API to support 32-bit binaries on 64-bit systems. I guess the
issue hasn't bitten anyone too badly since this hasn't been noticed earlier.

I can think of the reasons, too: Media controller is primarily used on
embedded systems that are still mainly 32-bit and the binaries are
targeted to those very systems specifically.

I noticed this since I'm currently running my 32-bit Debian system on a
64-bit kernel (I like to run 64-bit virtual machines but I'm lazy to
update; waiting for multiarch) so my native media-ctl binary just failed
to report anything about a UVC device Laurent kindly lended me.

The patchset implements separate compat ioctl handling and splits link
enumeration handling into 32-bit and 64-bit portions.

Many thanks to Laurent for testing these patches!

-- 
Kind regards,

Sakari Ailus
sakari.ailus@iki.fi

[PATCH 1/2] media: Add 64--32 bit compat ioctl handler

[Sakari Ailus]

Provide an ioctl handler for 32-bit binaries on 64-bit systems.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/media-devnode.c |   31 ++++++++++++++++++++++++++++---
 include/media/media-devnode.h |    1 +
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
index 023b2a1..fb0f046 100644
--- a/drivers/media/media-devnode.c
+++ b/drivers/media/media-devnode.c
@@ -116,19 +116,41 @@ static unsigned int media_poll(struct file *filp,
 	return mdev->fops->poll(filp, poll);
 }
 
-static long media_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+static long
+__media_ioctl(struct file *filp, unsigned int cmd, unsigned long arg,
+	      long (*ioctl_func)(struct file *filp, unsigned int cmd,
+				 unsigned long arg))
 {
 	struct media_devnode *mdev = media_devnode_data(filp);
 
-	if (!mdev->fops->ioctl)
+	if (!ioctl_func)
 		return -ENOTTY;
 
 	if (!media_devnode_is_registered(mdev))
 		return -EIO;
 
-	return mdev->fops->ioctl(filp, cmd, arg);
+	return ioctl_func(filp, cmd, arg);
+}
+
+static long media_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+	struct media_devnode *mdev = media_devnode_data(filp);
+
+	return __media_ioctl(filp, cmd, arg, mdev->fops->ioctl);
 }
 
+#ifdef CONFIG_COMPAT
+
+static long media_compat_ioctl(struct file *filp, unsigned int cmd,
+			       unsigned long arg)
+{
+	struct media_devnode *mdev = media_devnode_data(filp);
+
+	return __media_ioctl(filp, cmd, arg, mdev->fops->compat_ioctl);
+}
+
+#endif /* CONFIG_COMPAT */
+
 /* Override for the open function */
 static int media_open(struct inode *inode, struct file *filp)
 {
@@ -188,6 +210,9 @@ static const struct file_operations media_devnode_fops = {
 	.write = media_write,
 	.open = media_open,
 	.unlocked_ioctl = media_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl = media_compat_ioctl,
+#endif /* CONFIG_COMPAT */
 	.release = media_release,
 	.poll = media_poll,
 	.llseek = no_llseek,
diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
index f6caafc..3446af2 100644
--- a/include/media/media-devnode.h
+++ b/include/media/media-devnode.h
@@ -46,6 +46,7 @@ struct media_file_operations {
 	ssize_t (*write) (struct file *, const char __user *, size_t, loff_t *);
 	unsigned int (*poll) (struct file *, struct poll_table_struct *);
 	long (*ioctl) (struct file *, unsigned int, unsigned long);
+	long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
 	int (*open) (struct file *);
 	int (*release) (struct file *);
 };
-- 
1.7.10.4

[PATCH 2/2] media: implement 32-on-64 bit compat IOCTL handling

[Sakari Ailus]

Use the same handlers where the structs are the same. Implement a new
handler for link enumeration since struct media_links_enum is different on
32-bit and 64-bit systems.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/media-device.c |  102 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 89 insertions(+), 13 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index d01fcb7..99b80b6 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -20,10 +20,11 @@
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 
-#include <linux/types.h>
+#include <linux/compat.h>
+#include <linux/export.h>
 #include <linux/ioctl.h>
 #include <linux/media.h>
-#include <linux/export.h>
+#include <linux/types.h>
 
 #include <media/media-device.h>
 #include <media/media-devnode.h>
@@ -124,35 +125,31 @@ 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 __user *ulinks)
+static long __media_device_enum_links(struct media_device *mdev,
+				      struct media_links_enum *links)
 {
 	struct media_entity *entity;
-	struct media_links_enum links;
 
-	if (copy_from_user(&links, ulinks, sizeof(links)))
-		return -EFAULT;
-
-	entity = find_entity(mdev, links.entity);
+	entity = find_entity(mdev, links->entity);
 	if (entity == NULL)
 		return -EINVAL;
 
-	if (links.pads) {
+	if (links->pads) {
 		unsigned int p;
 
 		for (p = 0; p < entity->num_pads; p++) {
 			struct media_pad_desc pad;
 			media_device_kpad_to_upad(&entity->pads[p], &pad);
-			if (copy_to_user(&links.pads[p], &pad, sizeof(pad)))
+			if (copy_to_user(&links->pads[p], &pad, sizeof(pad)))
 				return -EFAULT;
 		}
 	}
 
-	if (links.links) {
+	if (links->links) {
 		struct media_link_desc __user *ulink;
 		unsigned int l;
 
-		for (l = 0, ulink = links.links; l < entity->num_links; l++) {
+		for (l = 0, ulink = links->links; l < entity->num_links; l++) {
 			struct media_link_desc link;
 
 			/* Ignore backlinks. */
@@ -169,8 +166,26 @@ static long media_device_enum_links(struct media_device *mdev,
 			ulink++;
 		}
 	}
+
+	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;
 }
 
@@ -251,10 +266,71 @@ static long media_device_ioctl(struct file *filp, unsigned int cmd,
 	return ret;
 }
 
+#ifdef CONFIG_COMPAT
+
+struct media_links_enum32 {
+	__u32 entity;
+	compat_uptr_t pads; /* struct media_pad_desc * */
+	compat_uptr_t links; /* struct media_link_desc * */
+	__u32 reserved[4];
+};
+
+static long media_device_enum_links32(struct media_device *mdev,
+				      struct media_links_enum32 __user *ulinks)
+{
+	struct media_links_enum links;
+	compat_uptr_t pads_ptr, links_ptr;
+
+	memset(&links, 0, sizeof(links));
+
+	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);
+
+	return __media_device_enum_links(mdev, &links);
+}
+
+#define MEDIA_IOC_ENUM_LINKS32		_IOWR('|', 0x02, struct media_links_enum32)
+
+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:
+		return media_device_ioctl(filp, cmd, arg);
+
+	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;
+}
+#endif /* CONFIG_COMPAT */
+
 static const struct media_file_operations media_device_fops = {
 	.owner = THIS_MODULE,
 	.open = media_device_open,
 	.ioctl = media_device_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl = media_device_compat_ioctl,
+#endif /* CONFIG_COMPAT */
 	.release = media_device_close,
 };
 
-- 
1.7.10.4

Re: [PATCH 2/2] media: implement 32-on-64 bit compat IOCTL handling

[Hans Verkuil]

On Tue January 22 2013 17:27:56 Sakari Ailus wrote:
> Use the same handlers where the structs are the same. Implement a new
> handler for link enumeration since struct media_links_enum is different on
> 32-bit and 64-bit systems.

I think I would prefer to have the compat handling split off into a
seperate source. I know it is just a small amount of code at the moment,
but that's the way it is done as well for the V4L2 ioctls and I rather
like that approach.

What do others think about that?

Regards,

	Hans

> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/media-device.c |  102 ++++++++++++++++++++++++++++++++++++------
>  1 file changed, 89 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index d01fcb7..99b80b6 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -20,10 +20,11 @@
>   * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>   */
>  
> -#include <linux/types.h>
> +#include <linux/compat.h>
> +#include <linux/export.h>
>  #include <linux/ioctl.h>
>  #include <linux/media.h>
> -#include <linux/export.h>
> +#include <linux/types.h>
>  
>  #include <media/media-device.h>
>  #include <media/media-devnode.h>
> @@ -124,35 +125,31 @@ 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 __user *ulinks)
> +static long __media_device_enum_links(struct media_device *mdev,
> +				      struct media_links_enum *links)
>  {
>  	struct media_entity *entity;
> -	struct media_links_enum links;
>  
> -	if (copy_from_user(&links, ulinks, sizeof(links)))
> -		return -EFAULT;
> -
> -	entity = find_entity(mdev, links.entity);
> +	entity = find_entity(mdev, links->entity);
>  	if (entity == NULL)
>  		return -EINVAL;
>  
> -	if (links.pads) {
> +	if (links->pads) {
>  		unsigned int p;
>  
>  		for (p = 0; p < entity->num_pads; p++) {
>  			struct media_pad_desc pad;
>  			media_device_kpad_to_upad(&entity->pads[p], &pad);
> -			if (copy_to_user(&links.pads[p], &pad, sizeof(pad)))
> +			if (copy_to_user(&links->pads[p], &pad, sizeof(pad)))
>  				return -EFAULT;
>  		}
>  	}
>  
> -	if (links.links) {
> +	if (links->links) {
>  		struct media_link_desc __user *ulink;
>  		unsigned int l;
>  
> -		for (l = 0, ulink = links.links; l < entity->num_links; l++) {
> +		for (l = 0, ulink = links->links; l < entity->num_links; l++) {
>  			struct media_link_desc link;
>  
>  			/* Ignore backlinks. */
> @@ -169,8 +166,26 @@ static long media_device_enum_links(struct media_device *mdev,
>  			ulink++;
>  		}
>  	}
> +
> +	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;
>  }
>  
> @@ -251,10 +266,71 @@ static long media_device_ioctl(struct file *filp, unsigned int cmd,
>  	return ret;
>  }
>  
> +#ifdef CONFIG_COMPAT
> +
> +struct media_links_enum32 {
> +	__u32 entity;
> +	compat_uptr_t pads; /* struct media_pad_desc * */
> +	compat_uptr_t links; /* struct media_link_desc * */
> +	__u32 reserved[4];
> +};
> +
> +static long media_device_enum_links32(struct media_device *mdev,
> +				      struct media_links_enum32 __user *ulinks)
> +{
> +	struct media_links_enum links;
> +	compat_uptr_t pads_ptr, links_ptr;
> +
> +	memset(&links, 0, sizeof(links));
> +
> +	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);
> +
> +	return __media_device_enum_links(mdev, &links);
> +}
> +
> +#define MEDIA_IOC_ENUM_LINKS32		_IOWR('|', 0x02, struct media_links_enum32)
> +
> +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:
> +		return media_device_ioctl(filp, cmd, arg);
> +
> +	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;
> +}
> +#endif /* CONFIG_COMPAT */
> +
>  static const struct media_file_operations media_device_fops = {
>  	.owner = THIS_MODULE,
>  	.open = media_device_open,
>  	.ioctl = media_device_ioctl,
> +#ifdef CONFIG_COMPAT
> +	.compat_ioctl = media_device_compat_ioctl,
> +#endif /* CONFIG_COMPAT */
>  	.release = media_device_close,
>  };
>  
> 

Re: [PATCH 2/2] media: implement 32-on-64 bit compat IOCTL handling

[Sakari Ailus]

Hi Hans,

Thanks for the comments!

On Tue, Jan 22, 2013 at 08:03:01PM +0100, Hans Verkuil wrote:
> On Tue January 22 2013 17:27:56 Sakari Ailus wrote:
> > Use the same handlers where the structs are the same. Implement a new
> > handler for link enumeration since struct media_links_enum is different on
> > 32-bit and 64-bit systems.
> 
> I think I would prefer to have the compat handling split off into a
> seperate source. I know it is just a small amount of code at the moment,
> but that's the way it is done as well for the V4L2 ioctls and I rather
> like that approach.
> 
> What do others think about that?

I pondered the possibility but thought that as it's not much code so it'd be
fine in the same file. I agree that hiding the compat code out of sight in
V4L2 works really well --- it's mostly uninteresting and only needs to be
touched when the API changes. But there's also much, much more code and
handling to do in V4L2. I doubt the MC will ever be nearly as large unless
the functionality and the problem area of the API changes significantly.

I have nothing against putting this into a separate file, though. :-)

-- 
Cheers,

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