Re: compat ioctl32 for /dev/snapshot?

[Arnd Bergmann]

On Monday 04 May 2009, Michael Tokarev wrote:
> Is the following patch ok?  I just pulled all the SNAPSHOT_* stuff from
> include/linux/suspend_ioctls.h and added them into fs/compat_ioctl.c.
> The ioctls are:
>   o argument-less (most of them are)

Some of the ones that do not take a pointer argument actually do
take an integer argument, in particular SNAPSHOT_PREF_IMAGE_SIZE and
SNAPSHOT_PLATFORM_SUPPORT, which you need to list as ULONG_IOCTL
rather than COMPAT_IOCTL if you want to add them to compat_ioctl.c.

>   o have single loff_t argument (other ioctls with the same argument are
>     marked as COMPAT_IOCTL
>   o have single int argument - they's also marked as COMPAT_IOCTL,

right.

>   o and one othem, SNAPSHOT_SET_SWAP_AREA, has argument pointing to
>     the following structure (include/linux/suspend_ioctls.h):
>      struct resume_swap_area {
>          loff_t offset;
>          u_int32_t dev;
>      } __attribute__((packed));
>     so I think it also does not need any translation layer.


this one would be compat_ioctl as well, because the
__attribute__((packed)) turns it into a well-defined size of
12 bytes on any architecture (without the attribute, it would
be 16 bytes on most architectures, but not i386. The recommended
way to do this for new architectures would be explicit padding
rather than packed attribute.).

You are however missing support for deprecated ioctls in your
code: SNAPSHOT_SET_SWAP_FILE and SNAPSHOT_PMOPS are trivial
(COMPATIBLE_IOCTL), but you also need to add support for
these

#define SNAPSHOT_ATOMIC_SNAPSHOT32 _IOW(SNAPSHOT_IOC_MAGIC, 3, compat_uptr_t)
#define SNAPSHOT_SET_IMAGE_SIZE32 _IOW(SNAPSHOT_IOC_MAGIC, 6, compat_ulong)
#define SNAPSHOT_AVAIL_SWAP32 _IOR(SNAPSHOT_IOC_MAGIC, 7, compat_uptr_t)
#define SNAPSHOT_GET_SWAP_PAGE32 _IOR(SNAPSHOT_IOC_MAGIC, 8, compat_uptr_t)

> I can't test it so far, because uswsusp tools are broken in mixed
> 32/64bit case in other places.  But at least it compiles fine and
> does not complain anymore.

I try to reduce the size of compat_ioctl.c. A better implementation
would be to add a ->compat_ioctl() operation to the file_operations
and list the compatible ioctl numbers as well.

Please try this patch instead:

suspend: Add compat_ioctl for snapshot device

All of the ioctl numbers in here are compatible, but for some of
the deprecated numbers, we need to add aliases.

Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>

--- a/kernel/power/user.c
+++ b/kernel/power/user.c
@@ -51,6 +51,10 @@
 #define SNAPSHOT_SET_IMAGE_SIZE		_IOW(SNAPSHOT_IOC_MAGIC, 6, unsigned long)
 #define SNAPSHOT_AVAIL_SWAP		_IOR(SNAPSHOT_IOC_MAGIC, 7, void *)
 #define SNAPSHOT_GET_SWAP_PAGE		_IOR(SNAPSHOT_IOC_MAGIC, 8, void *)
+#define SNAPSHOT_ATOMIC_SNAPSHOT32	_IOW(SNAPSHOT_IOC_MAGIC, 3, compat_uptr_t)
+#define SNAPSHOT_SET_IMAGE_SIZE32	_IOW(SNAPSHOT_IOC_MAGIC, 6, compat_ulong)
+#define SNAPSHOT_AVAIL_SWAP32		_IOR(SNAPSHOT_IOC_MAGIC, 7, compat_uptr_t)
+#define SNAPSHOT_GET_SWAP_PAGE32	_IOR(SNAPSHOT_IOC_MAGIC, 8, compat_uptr_t)
 
 
 #define SNAPSHOT_MINOR	231
@@ -249,6 +253,9 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
 
 	case SNAPSHOT_CREATE_IMAGE:
 	case SNAPSHOT_ATOMIC_SNAPSHOT:
+#ifdef CONFIG_COMPAT
+	case SNAPSHOT_ATOMIC_SNAPSHOT32:
+#endif
 		if (data->mode != O_RDONLY || !data->frozen  || data->ready) {
 			error = -EPERM;
 			break;
@@ -278,6 +285,9 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
 
 	case SNAPSHOT_PREF_IMAGE_SIZE:
 	case SNAPSHOT_SET_IMAGE_SIZE:
+#ifdef CONFIG_COMPAT
+	case SNAPSHOT_SET_IMAGE_SIZE32:
+#endif
 		image_size = arg;
 		break;
 
@@ -293,6 +303,9 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
 
 	case SNAPSHOT_AVAIL_SWAP_SIZE:
 	case SNAPSHOT_AVAIL_SWAP:
+#ifdef CONFIG_COMPAT
+	case SNAPSHOT_AVAIL_SWAP32:
+#endif
 		size = count_swap_pages(data->swap, 1);
 		size <<= PAGE_SHIFT;
 		error = put_user(size, (loff_t __user *)arg);
@@ -300,6 +313,9 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
 
 	case SNAPSHOT_ALLOC_SWAP_PAGE:
 	case SNAPSHOT_GET_SWAP_PAGE:
+#ifdef CONFIG_COMPAT
+	case SNAPSHOT_GET_SWAP_PAGE32:
+#endif
 		if (data->swap < 0 || data->swap >= MAX_SWAPFILES) {
 			error = -ENODEV;
 			break;
@@ -436,6 +452,9 @@ static const struct file_operations snapshot_fops = {
 	.write = snapshot_write,
 	.llseek = no_llseek,
 	.unlocked_ioctl = snapshot_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl = snapshot_ioctl,
+#endif
 };
 
 static struct miscdevice snapshot_device = {
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/