compat_ioctl question

compat_ioctl question

[Paul Fulghum]

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?

Thanks,
Paul


-- 
Paul Fulghum
Microgate Systems, Ltd

Re: compat_ioctl question

[Arnd Bergmann]

On Thursday 26 April 2007, Paul Fulghum wrote:
> 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?

	Arnd <><

Re: compat_ioctl question

[Paul Fulghum]

Arnd Bergmann wrote:
> 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

--
Paul

Re: compat_ioctl question

[Arnd Bergmann]

On Friday 27 April 2007, Paul Fulghum wrote:
> Arnd Bergmann wrote:
> > 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.

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.

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

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

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

	Arnd <><

Re: compat_ioctl question

[Paul Fulghum]

Arnd Bergmann wrote:
> 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.

I missed that approach, thanks.

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

I'll look at that.

You have given me precisely the information I need.

I just wanted to be sure I did not pursue a dead end
and have people go 'ewwww... why did you do it that way?'

Thanks,
Paul

[PATCH] tty add compat_ioctl method

[Paul Fulghum]

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.

Based on patch by Arnd Bergmann:
http://www.uwsg.iu.edu/hypermail/linux/kernel/0511.0/1732.html

This patch does not remove tty ioctl entries in compat_ioctl.[ch]
That will be implemented in a separate patch.

Signed-off-by: Paul Fulghum <paulkf@microgate.com>
CC: Arnd Bergmann <arnd@arndb.de>

--- a/drivers/char/n_tty.c	2007-04-30 15:13:19.000000000 -0500
+++ b/drivers/char/n_tty.c	2007-04-30 15:17:28.000000000 -0500
@@ -1545,21 +1545,18 @@ static unsigned int normal_poll(struct t
 }
 
 struct tty_ldisc tty_ldisc_N_TTY = {
-	TTY_LDISC_MAGIC,	/* magic */
-	"n_tty",		/* name */
-	0,			/* num */
-	0,			/* flags */
-	n_tty_open,		/* open */
-	n_tty_close,		/* close */
-	n_tty_flush_buffer,	/* flush_buffer */
-	n_tty_chars_in_buffer,	/* chars_in_buffer */
-	read_chan,		/* read */
-	write_chan,		/* write */
-	n_tty_ioctl,		/* ioctl */
-	n_tty_set_termios,	/* set_termios */
-	normal_poll,		/* poll */
-	NULL,			/* hangup */
-	n_tty_receive_buf,	/* receive_buf */
-	n_tty_write_wakeup	/* write_wakeup */
+	.magic           = TTY_LDISC_MAGIC,
+	.name            = "n_tty",
+	.open            = n_tty_open,
+	.close           = n_tty_close,
+	.flush_buffer    = n_tty_flush_buffer,
+	.chars_in_buffer = n_tty_chars_in_buffer,
+	.read            = read_chan,
+	.write           = write_chan,
+	.ioctl           = n_tty_ioctl,
+	.set_termios     = n_tty_set_termios,
+	.poll            = normal_poll,
+	.receive_buf     = n_tty_receive_buf,
+	.write_wakeup    = n_tty_write_wakeup
 };
 
--- a/include/linux/tty_driver.h	2006-11-29 15:57:37.000000000 -0600
+++ b/include/linux/tty_driver.h	2007-04-30 14:05:02.000000000 -0500
@@ -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);
@@ -193,6 +197,10 @@ struct tty_driver {
 	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);
--- a/drivers/char/tty_io.c	2006-11-29 15:57:37.000000000 -0600
+++ b/drivers/char/tty_io.c	2007-04-30 14:51:01.000000000 -0500
@@ -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);
+#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);
 
@@ -1153,8 +1156,8 @@ static unsigned int hung_up_tty_poll(str
 	return POLLIN | POLLOUT | POLLERR | POLLHUP | POLLRDNORM | POLLWRNORM;
 }
 
-static int hung_up_tty_ioctl(struct inode * inode, struct file * file,
-			     unsigned int cmd, unsigned long arg)
+static long hung_up_tty_ioctl(struct file * file,
+			      unsigned int cmd, unsigned long arg)
 {
 	return cmd == TIOCSPGRP ? -ENOTTY : -EIO;
 }
@@ -1165,6 +1168,9 @@ static const struct file_operations tty_
 	.write		= tty_write,
 	.poll		= tty_poll,
 	.ioctl		= tty_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl	= tty_compat_ioctl,
+#endif
 	.open		= tty_open,
 	.release	= tty_release,
 	.fasync		= tty_fasync,
@@ -1177,6 +1183,9 @@ static const struct file_operations ptmx
 	.write		= tty_write,
 	.poll		= tty_poll,
 	.ioctl		= tty_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl	= tty_compat_ioctl,
+#endif
 	.open		= ptmx_open,
 	.release	= tty_release,
 	.fasync		= tty_fasync,
@@ -1189,6 +1198,9 @@ static const struct file_operations cons
 	.write		= redirected_tty_write,
 	.poll		= tty_poll,
 	.ioctl		= tty_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl	= tty_compat_ioctl,
+#endif
 	.open		= tty_open,
 	.release	= tty_release,
 	.fasync		= tty_fasync,
@@ -1199,7 +1211,10 @@ static const struct file_operations hung
 	.read		= hung_up_tty_read,
 	.write		= hung_up_tty_write,
 	.poll		= hung_up_tty_poll,
-	.ioctl		= hung_up_tty_ioctl,
+	.unlocked_ioctl = hung_up_tty_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl	= hung_up_tty_ioctl,
+#endif
 	.release	= tty_release,
 };
 
@@ -3284,6 +3299,32 @@ int tty_ioctl(struct inode * inode, stru
 	return retval;
 }
 
+#ifdef CONFIG_COMPAT
+long tty_compat_ioctl(struct file * file, unsigned int cmd, unsigned long arg)
+{
+	struct inode *inode = file->f_dentry->d_inode;
+	struct tty_struct *tty;
+	struct tty_ldisc *ld;
+	int retval = -ENOIOCTLCMD;
+
+	tty = (struct tty_struct *)file->private_data;
+	if (tty_paranoia_check(tty, inode, "tty_ioctl"))
+		return -EINVAL;
+
+	if (tty->driver->compat_ioctl) {
+		retval = (tty->driver->compat_ioctl)(tty, file, cmd, arg);
+		if (retval != -ENOIOCTLCMD)
+			return retval;
+	}
+
+	ld = tty_ldisc_ref_wait(tty);
+	if (ld->compat_ioctl)
+		retval = ld->compat_ioctl(tty, file, cmd, arg);
+	tty_ldisc_deref(ld);
+
+	return retval;
+}
+#endif
 
 /*
  * This implements the "Secure Attention Key" ---  the idea is to
@@ -3691,6 +3732,9 @@ void tty_set_operations(struct tty_drive
 	driver->write_room = op->write_room;
 	driver->chars_in_buffer = op->chars_in_buffer;
 	driver->ioctl = op->ioctl;
+#ifdef CONFIG_COMPAT
+	driver->compat_ioctl = op->compat_ioctl;
+#endif
 	driver->set_termios = op->set_termios;
 	driver->throttle = op->throttle;
 	driver->unthrottle = op->unthrottle;
--- a/include/linux/tty_ldisc.h	2006-11-29 15:57:37.000000000 -0600
+++ b/include/linux/tty_ldisc.h	2007-04-30 14:06:19.000000000 -0500
@@ -59,6 +59,11 @@
  * 	low-level driver can "grab" an ioctl request before the line
  * 	discpline has a chance to see it.
  * 
+ * long	(*compat_ioctl)(struct tty_struct * tty, struct file * file,
+ * 		        unsigned int cmd, unsigned long arg);
+ *
+ *      Process ioctl calls from 32-bit process on 64-bit system
+ *
  * void	(*set_termios)(struct tty_struct *tty, struct termios * old);
  *
  * 	This function notifies the line discpline that a change has
@@ -118,6 +123,10 @@ struct tty_ldisc {
 			 const unsigned char * buf, size_t nr);	
 	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);
 	unsigned int (*poll)(struct tty_struct *, struct file *,
 			     struct poll_table_struct *);

Re: [PATCH] tty add compat_ioctl method

[Arnd Bergmann]

On Wednesday 02 May 2007, Paul Fulghum wrote:
> 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.

Looks ok mostly. Just some details:

> --- a/include/linux/tty_driver.h	2006-11-29 15:57:37.000000000 -0600
> +++ b/include/linux/tty_driver.h	2007-04-30 14:05:02.000000000 -0500
> @@ -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);
> @@ -193,6 +197,10 @@ struct tty_driver {
>  	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);

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

> --- a/drivers/char/tty_io.c	2006-11-29 15:57:37.000000000 -0600
> +++ b/drivers/char/tty_io.c	2007-04-30 14:51:01.000000000 -0500
> @@ -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);
> +#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.

> @@ -3284,6 +3299,32 @@ int tty_ioctl(struct inode * inode, stru
>  	return retval;
>  }
>  
> +#ifdef CONFIG_COMPAT
> +long tty_compat_ioctl(struct file * file, unsigned int cmd, unsigned long arg)
> +{
> +	struct inode *inode = file->f_dentry->d_inode;
> +	struct tty_struct *tty;
> +	struct tty_ldisc *ld;
> +	int retval = -ENOIOCTLCMD;
> +
> +	tty = (struct tty_struct *)file->private_data;

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

	Arnd <><

Re: [PATCH] tty add compat_ioctl method

[Paul Fulghum]

Arnd Bergmann wrote:
> 
> Looks ok mostly. Just some details:
> ...
>> +#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

OK

>> --- a/drivers/char/tty_io.c	2006-11-29 15:57:37.000000000 -0600
>> +++ b/drivers/char/tty_io.c	2007-04-30 14:51:01.000000000 -0500
>> @@ -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);
>> +#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

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

> #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.

OK

>> +	tty = (struct tty_struct *)file->private_data;
> 
> no need for the cast, ->private_data is void*.

Yes, an easy fix.

--
Paul

Re: [PATCH] tty add compat_ioctl method

[Arnd Bergmann]

On Thursday 03 May 2007, Paul Fulghum wrote:
> 
> > 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?

You only get a warning for static declarations without an implementation,
not for globals.

	Arnd <><

Re: compat_ioctl question

[Andi Kleen]

Paul Fulghum <paulkf@microgate.com> writes:
> 
> 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.

(1) is really the recommended solution for all new code

I would recommend you modify the tty driver to pass compat_ioctl
through through. Should not do very hard.

-Andi