compat_ioctl question

I need to add ioctl translations for my driver to
allow 32 bit access on 64 bit systems.

After digging through the kernel code there seems to be 3 methods of doing this:

1. define compat_ioctl() file operation for device and implement translation code in individual driver
2. add COMPATIBLE_IOCTL entry to include/linux/compat_ioctl.h to mark an ioctl code as the same in any environment
3. add HANDLE_IOCTL entry to fs/compat_ioctl.c with translation code implemented in the same file

There is no way to implement #1 for a tty driver without modifying the kernel tty code to allow registration of a compat_ioctl() handler.

#3 would put a lot of driver specific stuff in a common kernel file. This method also seems to break if there is an ioctl code collision.

All of these methods involve changes to code outside of my driver.

--

Before I spend a lot of time on this I need to know what the officially sanctioned method is. I haven't found any definitive documentation and a review of mailing list archives does not suggest a prevailing opinion.

Does anyone have pointers on which way would be most likely to be accepted as a patch?

It depends a lot on what your specific driver does in the ioctl handler, but normally you should define a compat_ioctl() function. What driver are you talking about?

Quoted 3 lines · Arnd Bergmann

It depends a lot on what your specific driver does in the ioctl handler, but normally you should define a compat_ioctl() function. What driver are you talking about?

drivers/char/synclink.c
drivers/char/synclinkmp.c
drivers/char/synclink_gt.c
drivers/char/pcmcia/synclink_cs.c

All use the same set of ioctl() codes that are peculiar to the synclink drivers.

Defining compat_ioctl() seems to be the best way, but that will require modifying the base tty code to allow the individual tty drivers to register compat_ioctl().

Currently the tty file ops do not include that and tty_io.c does not register a compat_ioctl(), instead relying on compat_ioctl.h and compat_ioctl.c

So you are interested in the MGSL_* set of ioctls, right? AFAICS, they are all compatible, with the exception of MGSL_IOCGPARAMS and MGSL_IOCSPARAMS.

Fortunately, these two have different ioctl numbers on 64 bit, so you can define a new

#define MGSL_IOCSPARAMS32 _IOR(MGSL_MAGIC_IOC,0,struct _MGSL_PARAMS32) #define MGSL_IOCGPARAMS32 _IOR(MGSL_MAGIC_IOC,1,struct _MGSL_PARAMS32)

and handle both versions in the ioctl function.

Yes, that would be the right solution. I've started this some time ago, but never finished it:
http://www.uwsg.iu.edu/hypermail/linux/kernel/0511.0/1732.html

Quoted 3 lines

Currently the tty file ops do not include that and tty_io.c does not register a compat_ioctl(), instead relying on compat_ioctl.h and compat_ioctl.c

Just adding the hook in tty_io.c should be trivial, please do that. If you like, you can also move the vt ioctls in order to reduce the size of fs/compat_ioctl.c.

Add compat_ioctl method for tty code to allow processing of 32 bit ioctl calls on 64 bit systems by tty core, tty drivers, and line disciplines.

include/linux/tty_driver.h +4 -0

@@ -132,6 +132,10 @@ struct tty_operations {
 	int  (*chars_in_buffer)(struct tty_struct *tty);
 	int  (*ioctl)(struct tty_struct *tty, struct file * file,
 		    unsigned int cmd, unsigned long arg);
+#ifdef CONFIG_COMPAT
+	long (*compat_ioctl)(struct tty_struct *tty, struct file * file,
+			     unsigned int cmd, unsigned long arg);
+#endif
 	void (*set_termios)(struct tty_struct *tty, struct termios * old);
 	void (*throttle)(struct tty_struct * tty);
 	void (*unthrottle)(struct tty_struct * tty);

Looks ok mostly. Just some details:
...

include/linux/tty_driver.h struct tty_operations { · line 138

Quoted 4 lines

include/linux/tty_driver.h +4 -0

+#ifdef CONFIG_COMPAT
+	long (*compat_ioctl)(struct tty_struct *tty, struct file * file,
+			     unsigned int cmd, unsigned long arg);
+#endif

I wouldn't hide this inside of an #ifdef. The structures are all static and therefore a single field per driver doesn't add much bload, but being able to always assign the .compat_ioctl pointer makes the code somewhat nicer

Quoted 6 lines

drivers/char/tty_io.c +0 -0

@@ -151,6 +151,9 @@ static int tty_open(struct inode *, stru
 static int tty_release(struct inode *, struct file *);
 int tty_ioctl(struct inode * inode, struct file * file,
 	      unsigned int cmd, unsigned long arg);

drivers/char/tty_io.c static int tty_open(struct inode *, stru · line 158

Quoted 5 lines

drivers/char/tty_io.c +3 -0

+#ifdef CONFIG_COMPAT
+long tty_compat_ioctl(struct file * file, unsigned int cmd, unsigned long arg);
+#endif
 static int tty_fasync(int fd, struct file * filp, int on);
 static void release_mem(struct tty_struct *tty, int idx);

declarations should never be hidden inside of an #ifdef. If you want to be extra clever here, you can do

#ifdef CONFIG_COMPAT
long tty_compat_ioctl(struct file * file, unsigned int cmd, unsigned long arg); #else
#define tty_compat_ioctl NULL
#endif

then you don't need an #ifdef in the code setting the function pointers.

drivers/char/tty_io.c int tty_ioctl(struct inode * inode, stru · line 3310

Quoted 1 line

drivers/char/tty_io.c +1 -0

+	tty = (struct tty_struct *)file->private_data;

no need for the cast, ->private_data is void*.

Quoted 3 lines

declarations should never be hidden inside of an #ifdef. If you want to be extra clever here, you can do

OK, I have no problem with that.
A declaration without implementation won't generate a warning?

1. define compat_ioctl() file operation for device and implement translation code in individual driver
2. add COMPATIBLE_IOCTL entry to include/linux/compat_ioctl.h to mark an ioctl code as the same in any environment
3. add HANDLE_IOCTL entry to fs/compat_ioctl.c with translation code implemented in the same file

There is no way to implement #1 for a tty driver without modifying the kernel tty code to allow registration of a compat_ioctl() handler.