[PATCH 2/4 - 2.6.15]net: 32 bit (socket layer) ioctl emulation for 64 bit kernels

[PATCH 2/4 - 2.6.15]net: 32 bit (socket layer) ioctl emulation for 64 bit kernels

[Shaun Pereira]

The second part of this series. 

This routine is needed by the x25 module (32-64 bit patch), as
recommended it has been added to compat.c

diff -uprN -X dontdiff linux-2.6.15-vanilla/include/net/compat.h
linux-2.6.15/include/net/compat.h
--- linux-2.6.15-vanilla/include/net/compat.h	2006-01-03
14:21:10.000000000 +1100
+++ linux-2.6.15/include/net/compat.h	2006-01-12 16:01:09.000000000
+1100
@@ -23,6 +23,8 @@ struct compat_cmsghdr {
 	compat_int_t	cmsg_type;
 };
 
+extern int compat_sock_get_timestamp(struct sock *, struct timeval
__user *);
+
 #else /* defined(CONFIG_COMPAT) */
 #define compat_msghdr	msghdr		/* to avoid compiler warnings */
 #endif /* defined(CONFIG_COMPAT) */
diff -uprN -X dontdiff linux-2.6.15-vanilla/net/compat.c
linux-2.6.15/net/compat.c
--- linux-2.6.15-vanilla/net/compat.c	2006-01-03 14:21:10.000000000
+1100
+++ linux-2.6.15/net/compat.c	2006-01-12 16:01:09.000000000 +1100
@@ -503,6 +503,20 @@ static int do_get_sock_timeout(int fd, i
 	return err;
 }
 
+int compat_sock_get_timestamp(struct sock *sk, struct timeval __user
*userstamp)
+{
+	struct compat_timeval __user *ctv;
+	ctv = (struct compat_timeval __user*) userstamp;
+	if(!sock_flag(sk, SOCK_TIMESTAMP))
+		sock_enable_timestamp(sk);
+	if(sk->sk_stamp.tv_sec == -1)
+		return -ENOENT;
+	if(sk->sk_stamp.tv_sec == 0)
+		do_gettimeofday(&sk->sk_stamp);
+	return copy_to_user(ctv, &sk->sk_stamp, sizeof(struct
compat_timeval)) ?
+			-EFAULT : 0;
+}
+
 asmlinkage long compat_sys_getsockopt(int fd, int level, int optname,
 				char __user *optval, int __user *optlen)
 {
@@ -602,3 +616,5 @@ asmlinkage long compat_sys_socketcall(in
 	}
 	return ret;
 }
+
+EXPORT_SYMBOL(compat_sock_get_timestamp);

Re: [PATCH 2/4 - 2.6.15]net: 32 bit (socket layer) ioctl emulation for 64 bit kernels

[Arnd Bergmann]

On Thursday 12 January 2006 06:02, Shaun Pereira wrote:
> +int compat_sock_get_timestamp(struct sock *sk, struct timeval __user
> *userstamp)
> +{
> +       struct compat_timeval __user *ctv;
> +       ctv = (struct compat_timeval __user*) userstamp;
> +       if(!sock_flag(sk, SOCK_TIMESTAMP))
> +               sock_enable_timestamp(sk);
> +       if(sk->sk_stamp.tv_sec == -1)
> +               return -ENOENT;
> +       if(sk->sk_stamp.tv_sec == 0)
> +               do_gettimeofday(&sk->sk_stamp);
> +       return copy_to_user(ctv, &sk->sk_stamp, sizeof(struct
> compat_timeval)) ?
> +                       -EFAULT : 0;
> +}

This looks wrong, you're not doing any conversion here.
You cannot just copy sk_stamp to ctv, they are not compatible.
See compat_sys_gettimeofday on how to copy a struct timeval
correctly.

The other patches look good.

	Arnd <><

Re: [PATCH 2/4 - 2.6.15]net: 32 bit (socket layer) ioctl emulation for 64 bit kernels

[Shaun Pereira]

Hi Arnd
Thank you for reviewing that bit of code.  
I had a look at compat_sys_gettimeofday and sys32_gettimeofday codes.
They seem to work in a similar way, casting a pointer to the structure
from user space to a compat_timeval type.

But to make sure I have tested the routine by forcing the sk-
>sk_stamp.tv_sec value to 0 in the x25_module ( for testing purposes
only, as it is initialised to -1). Now when
I make a 32 bit userspace SIOCGSTAMP ioctl to the 64 bit kernel I should
get the current time back in user space. This seems to work, the ioctl
returns the system time (just after TEST6:)

So I have left the patch as is for now. However if necessary to use
the element-by-element __put_user routine as in put_tv32, then I can
make the change, just let me know.

Lastly, if anyone following this mail can help with adding this into the
next release, that would be really helpful. We are building a network
management application on linux for a telco and while they have 
the support of the SuSE's and Redhat's, any patches accepted by the
kernel community makes a difference, besides saving us from building
custom patches. (BTW, the application used to run on HP-UX, we are now
porting it to linux).

Many Thanks 
Shaun
------------------------------
ghostview:/home/spereira/x25_32/src/func_tests/server # uname -a
Linux ghostview 2.6.15-smp #9 SMP Fri Jan 13 12:43:27 EST 2006 x86_64
x86_64 x86_64 GNU/Linux
ghostview:/home/spereira/x25_32/src/func_tests/server # file server
server: ELF 32-bit LSB executable, Intel 80386, version 1 (SYSV), for
GNU/Linux 2.2.5, dynamically linked (uses shared libs), not stripped
ghostview:/home/spereira/x25_32/src/func_tests/server # ./server
usage: server <local X.121 address> <interface name>
ghostview:/home/spereira/x25_32/src/func_tests/server # ./server
05052384500000 x25tap0
TEST1: create socket : passed
TEST2: bind socket : passed
TEST3: set subscription: passed
TEST4: set facilities: passed

**************
 Window size in = 2
 Window size out = 2
 Packet size in = 7
 Packet size out = 7
 Reverse flag = 00
 Throughput  = DD
**************
TEST5: set call user data on listen passed
cud[ 0 ] = 02
cud[ 1 ] = 03
TEST6: set matchlength on listen socket handle passed
The time stamp is Fri Jan 13 13:37:17 2006
TEST7: listen on socket: passed
 Waiting for X25 packets
-----------------------------------------------------------





On Thu, 2006-01-12 at 19:24 +0000, Arnd Bergmann wrote:
> On Thursday 12 January 2006 06:02, Shaun Pereira wrote:
> > +int compat_sock_get_timestamp(struct sock *sk, struct timeval __user
> > *userstamp)
> > +{
> > +       struct compat_timeval __user *ctv;
> > +       ctv = (struct compat_timeval __user*) userstamp;
> > +       if(!sock_flag(sk, SOCK_TIMESTAMP))
> > +               sock_enable_timestamp(sk);
> > +       if(sk->sk_stamp.tv_sec == -1)
> > +               return -ENOENT;
> > +       if(sk->sk_stamp.tv_sec == 0)
> > +               do_gettimeofday(&sk->sk_stamp);
> > +       return copy_to_user(ctv, &sk->sk_stamp, sizeof(struct
> > compat_timeval)) ?
> > +                       -EFAULT : 0;
> > +}
> 
> This looks wrong, you're not doing any conversion here.
> You cannot just copy sk_stamp to ctv, they are not compatible.
> See compat_sys_gettimeofday on how to copy a struct timeval
> correctly.
> 
> The other patches look good.
> 
> 	Arnd <><

Re: [PATCH 2/4 - 2.6.15]net: 32 bit (socket layer) ioctl emulation for 64 bit kernels

[Arnd Bergmann]

On Friday 13 January 2006 03:14, Shaun Pereira wrote:
> Thank you for reviewing that bit of code.  
> I had a look at compat_sys_gettimeofday and sys32_gettimeofday codes.
> They seem to work in a similar way, casting a pointer to the structure
> from user space to a compat_timeval type.

The part with the case is ok, except that you don't have to write

struct compat_timeval __user *ctv;
ctv = (struct compat_timeval __user*) userstamp;

Instead,

struct compat_timeval __user *ctv = userstamp;

is the more common way to write it. The result is the same, since
userstamp is a 'void __user *'.

> But to make sure I have tested the routine by forcing the sk-
> >sk_stamp.tv_sec value to 0 in the x25_module ( for testing purposes
> only, as it is initialised to -1). Now when
> I make a 32 bit userspace SIOCGSTAMP ioctl to the 64 bit kernel I should
> get the current time back in user space. This seems to work, the ioctl
> returns the system time (just after TEST6:)
> 
> So I have left the patch as is for now. However if necessary to use
> the element-by-element __put_user routine as in put_tv32, then I can
> make the change, just let me know.

You need to to exactly that, yes. I'm not sure what exactly you have
tested, but the expected result of your code would be that you see
the sk_stamp.tv_sec value in the output, but not the tv_usec value.

On little-endian system like x86_64, that is not much of a difference
(less than a second) that you might miss in a test case, but on
big-endian, it would be fatal.

The layout of the structures on most systems is 

		64 bit LE	64 bit BE	32 bit

bytes 0-3	tv_sec low	tv_sec high	tv_sec low
bytes 4-7	tv_sec high	tv_sec low	tv_usec low
bytes 8-11	tv_usec low	tv_usec high
bytes 12-15	tv_usec high	tv_usec low

You code copies the first eight bytes of the 64 bit data structure
into the 32 bit data structure.

	Arnd <><

Re: 32 bit (socket layer) ioctl emulation for 64 bit kernels

[Shaun Pereira]

Hi Arnd

I have made the changes suggested, and attached it below. I think it
should be good now.  
Just a couple of questions if I may. 
If I understand correctly from your comments (thanks for that, they are
helpful)
copy_to_user acts like a memcopy for an 'array' of bytes and should not
be used to copy the timeval struct to userspace. 
Rather put_user / __put_user macros should be used which allows transfer
of single element values of the structure. 
Does that also mean that copy_to_user should not be used in ioctl
calls? 

I was wondering if this the compat_sock_get_timestamp function is
needed? If I were to remove the SIOCGSTAMP case from the
compat_x25_ioctl function, then a SIOCGSTAMP ioctl system call would
return -ENOIOCTLCMD which could  then be handled by do_siocgstamp
handler in the ioctl32_hash_table? (fs/compat_ioctl.c)
In which case I could remove this patch from the rest of the series.
 
/Shaun


Index: linux-2.6.15/include/net/compat.h
===================================================================
--- linux-2.6.15.orig/include/net/compat.h
+++ linux-2.6.15/include/net/compat.h
@@ -23,6 +23,8 @@ struct compat_cmsghdr {
 	compat_int_t	cmsg_type;
 };
 
+extern int compat_sock_get_timestamp(struct sock *, struct timeval
__user *);
+
 #else /* defined(CONFIG_COMPAT) */
 #define compat_msghdr	msghdr		/* to avoid compiler warnings */
 #endif /* defined(CONFIG_COMPAT) */
Index: linux-2.6.15/net/compat.c
===================================================================
--- linux-2.6.15.orig/net/compat.c
+++ linux-2.6.15/net/compat.c
@@ -503,6 +503,25 @@ static int do_get_sock_timeout(int fd, i
 	return err;
 }
 
+int compat_sock_get_timestamp(struct sock *sk, struct timeval __user
*userstamp)
+{
+	struct compat_timeval __user *ctv
+		= (struct compat_timeval __user*) userstamp;
+	int err = -ENOENT;
+	if(!sock_flag(sk, SOCK_TIMESTAMP))
+		sock_enable_timestamp(sk);
+	if(sk->sk_stamp.tv_sec == -1)
+		return err;
+	if(sk->sk_stamp.tv_sec == 0)
+		do_gettimeofday(&sk->sk_stamp);
+	err = -EFAULT;
+	if(access_ok(VERIFTY_WRITE, ctv, sizeof(*ctv))) {
+		err = __put_user(sk->sk_stamp.tv_sec, &ctv->tv_sec);
+		err != __put_user(sk->sk_stamp.tv_usec, &ctv->tv_usec);
+	}
+	return err;
+}
+
 asmlinkage long compat_sys_getsockopt(int fd, int level, int optname,
 				char __user *optval, int __user *optlen)
 {
@@ -602,3 +621,5 @@ asmlinkage long compat_sys_socketcall(in
 	}
 	return ret;
 }
+
+EXPORT_SYMBOL(compat_sock_get_timestamp);


On Fri, 2006-01-13 at 11:46 +0000, Arnd Bergmann wrote:
> On Friday 13 January 2006 03:14, Shaun Pereira wrote:
> > Thank you for reviewing that bit of code.  
> > I had a look at compat_sys_gettimeofday and sys32_gettimeofday codes.
> > They seem to work in a similar way, casting a pointer to the structure
> > from user space to a compat_timeval type.
> 
> The part with the case is ok, except that you don't have to write
> 
> struct compat_timeval __user *ctv;
> ctv = (struct compat_timeval __user*) userstamp;
> 
> Instead,
> 
> struct compat_timeval __user *ctv = userstamp;
> 
> is the more common way to write it. The result is the same, since
> userstamp is a 'void __user *'.
> 
> > But to make sure I have tested the routine by forcing the sk-
> > >sk_stamp.tv_sec value to 0 in the x25_module ( for testing purposes
> > only, as it is initialised to -1). Now when
> > I make a 32 bit userspace SIOCGSTAMP ioctl to the 64 bit kernel I should
> > get the current time back in user space. This seems to work, the ioctl
> > returns the system time (just after TEST6:)
> > 
> > So I have left the patch as is for now. However if necessary to use
> > the element-by-element __put_user routine as in put_tv32, then I can
> > make the change, just let me know.
> 
> You need to to exactly that, yes. I'm not sure what exactly you have
> tested, but the expected result of your code would be that you see
> the sk_stamp.tv_sec value in the output, but not the tv_usec value.
> 
> On little-endian system like x86_64, that is not much of a difference
> (less than a second) that you might miss in a test case, but on
> big-endian, it would be fatal.
> 
> The layout of the structures on most systems is 
> 
> 		64 bit LE	64 bit BE	32 bit
> 
> bytes 0-3	tv_sec low	tv_sec high	tv_sec low
> bytes 4-7	tv_sec high	tv_sec low	tv_usec low
> bytes 8-11	tv_usec low	tv_usec high
> bytes 12-15	tv_usec high	tv_usec low
> 
> You code copies the first eight bytes of the 64 bit data structure
> into the 32 bit data structure.
> 
> 	Arnd <><

Re: 32 bit (socket layer) ioctl emulation for 64 bit kernels

[YOSHIFUJI Hideaki / 吉藤英明]

In article <1137391160.5588.32.camel@spereira05.tusc.com.au> (at Mon, 16 Jan 2006 16:59:20 +1100), Shaun Pereira <spereira@tusc.com.au> says:

> If I understand correctly from your comments (thanks for that, they are
> helpful)
> copy_to_user acts like a memcopy for an 'array' of bytes and should not
> be used to copy the timeval struct to userspace. 
> Rather put_user / __put_user macros should be used which allows transfer
> of single element values of the structure. 

> +int compat_sock_get_timestamp(struct sock *sk, struct timeval __user
> *userstamp)
> +{
> +	struct compat_timeval __user *ctv
> +		= (struct compat_timeval __user*) userstamp;
> +	int err = -ENOENT;
> +	if(!sock_flag(sk, SOCK_TIMESTAMP))
> +		sock_enable_timestamp(sk);
> +	if(sk->sk_stamp.tv_sec == -1)
> +		return err;
> +	if(sk->sk_stamp.tv_sec == 0)
> +		do_gettimeofday(&sk->sk_stamp);
> +	err = -EFAULT;
> +	if(access_ok(VERIFTY_WRITE, ctv, sizeof(*ctv))) {
> +		err = __put_user(sk->sk_stamp.tv_sec, &ctv->tv_sec);
> +		err != __put_user(sk->sk_stamp.tv_usec, &ctv->tv_usec);
> +	}
> +	return err;
> +}
> +

Hmm, you will copy 32bit of MSB in big-endian.
You should do something like this:

        strtuct compat_timeval tvtmp;
        :
	tvtmp.tv_sec = sk->sk_stamp.tv_sec;
	tvtmp.tv_usec = sk->sk_stemp.tv_usec;
	return copy_to_user(ctv, &tvtmp, sizeof(tvtmp));

Or, am I miss something?

--yoshfuji

[PATCH 1/4 - 2.6.15 ]net : 32 bit (socket layer) ioctl emulation for 64 bit kernels

[Shaun Pereira]

Hi
Here are the corrected patches
/Shaun

diff -uprN -X dontdiff linux-2.6.15-vanilla/include/linux/net.h
linux-2.6.15/include/linux/net.h
--- linux-2.6.15-vanilla/include/linux/net.h	2006-01-03
14:21:10.000000000 +1100
+++ linux-2.6.15/include/linux/net.h	2006-01-17 09:43:56.000000000 +1100
@@ -143,6 +143,8 @@ struct proto_ops {
 				      struct poll_table_struct *wait);
 	int		(*ioctl)     (struct socket *sock, unsigned int cmd,
 				      unsigned long arg);
+	int	 	(*compat_ioctl) (struct socket *sock, unsigned int cmd,
+				      unsigned long arg);
 	int		(*listen)    (struct socket *sock, int len);
 	int		(*shutdown)  (struct socket *sock, int flags);
 	int		(*setsockopt)(struct socket *sock, int level,
@@ -247,6 +249,8 @@ SOCKCALL_UWRAP(name, poll, (struct file 
 	      (file, sock, wait)) \
 SOCKCALL_WRAP(name, ioctl, (struct socket *sock, unsigned int cmd, \
 			 unsigned long arg), (sock, cmd, arg)) \
+SOCKCALL_WRAP(name, compat_ioctl, (struct socket *sock, unsigned int
cmd, \
+			 unsigned long arg), (sock, cmd, arg)) \
 SOCKCALL_WRAP(name, listen, (struct socket *sock, int len), (sock,
len)) \
 SOCKCALL_WRAP(name, shutdown, (struct socket *sock, int flags), (sock,
flags)) \
 SOCKCALL_WRAP(name, setsockopt, (struct socket *sock, int level, int
optname, \
@@ -271,6 +275,7 @@ static struct proto_ops name##_ops = {		
 	.getname	= __lock_##name##_getname,	\
 	.poll		= __lock_##name##_poll,		\
 	.ioctl		= __lock_##name##_ioctl,	\
+	.compat_ioctl	= __lock_##name##_compat_ioctl,	\
 	.listen		= __lock_##name##_listen,	\
 	.shutdown	= __lock_##name##_shutdown,	\
 	.setsockopt	= __lock_##name##_setsockopt,	\
@@ -279,6 +284,7 @@ static struct proto_ops name##_ops = {		
 	.recvmsg	= __lock_##name##_recvmsg,	\
 	.mmap		= __lock_##name##_mmap,		\
 };
+
 #endif
 
 #define MODULE_ALIAS_NETPROTO(proto) \
diff -uprN -X dontdiff linux-2.6.15-vanilla/net/socket.c
linux-2.6.15/net/socket.c
--- linux-2.6.15-vanilla/net/socket.c	2006-01-03 14:21:10.000000000
+1100
+++ linux-2.6.15/net/socket.c	2006-01-17 09:43:56.000000000 +1100
@@ -109,6 +109,10 @@ static unsigned int sock_poll(struct fil
 			      struct poll_table_struct *wait);
 static long sock_ioctl(struct file *file,
 		      unsigned int cmd, unsigned long arg);
+#ifdef CONFIG_COMPAT
+static long compat_sock_ioctl(struct file *file,
+		      unsigned int cmd, unsigned long arg);
+#endif
 static int sock_fasync(int fd, struct file *filp, int on);
 static ssize_t sock_readv(struct file *file, const struct iovec
*vector,
 			  unsigned long count, loff_t *ppos);
@@ -130,6 +134,9 @@ static struct file_operations socket_fil
 	.aio_write =	sock_aio_write,
 	.poll =		sock_poll,
 	.unlocked_ioctl = sock_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl = compat_sock_ioctl,
+#endif
 	.mmap =		sock_mmap,
 	.open =		sock_no_open,	/* special open code to disallow open via /proc
*/
 	.release =	sock_close,
@@ -2084,6 +2091,20 @@ void socket_seq_show(struct seq_file *se
 }
 #endif /* CONFIG_PROC_FS */
 
+#ifdef CONFIG_COMPAT
+static long compat_sock_ioctl(struct file *file, unsigned cmd, unsigned
long arg)
+{
+	struct socket *sock;
+	sock = file->private_data;
+
+	int ret = -ENOIOCTLCMD;
+	if(sock->ops->compat_ioctl)
+		ret = sock->ops->compat_ioctl(sock, cmd, arg);
+
+	return ret;
+}
+#endif
+
 /* ABI emulation layers need these two */
 EXPORT_SYMBOL(move_addr_to_kernel);
 EXPORT_SYMBOL(move_addr_to_user);

[PATCH 2/4 - 2.6.15]net: 32 bit (socket layer) ioctl emulation for 64 bit kernels

[Shaun Pereira]

Provides a 32 bit conversion function for SIOCGSTAMP
diff -uprN -X dontdiff linux-2.6.15-vanilla/include/net/compat.h
linux-2.6.15/include/net/compat.h
--- linux-2.6.15-vanilla/include/net/compat.h	2006-01-03
14:21:10.000000000 +1100
+++ linux-2.6.15/include/net/compat.h	2006-01-17 09:52:50.000000000
+1100
@@ -23,6 +23,8 @@ struct compat_cmsghdr {
 	compat_int_t	cmsg_type;
 };
 
+extern int compat_sock_get_timestamp(struct sock *, struct timeval
__user *);
+
 #else /* defined(CONFIG_COMPAT) */
 #define compat_msghdr	msghdr		/* to avoid compiler warnings */
 #endif /* defined(CONFIG_COMPAT) */
diff -uprN -X dontdiff linux-2.6.15-vanilla/net/compat.c
linux-2.6.15/net/compat.c
--- linux-2.6.15-vanilla/net/compat.c	2006-01-03 14:21:10.000000000
+1100
+++ linux-2.6.15/net/compat.c	2006-01-17 09:52:50.000000000 +1100
@@ -503,6 +503,23 @@ static int do_get_sock_timeout(int fd, i
 	return err;
 }
 
+int compat_sock_get_timestamp(struct sock *sk, struct timeval __user
*userstamp)
+{
+	struct compat_timeval __user *ctv
+		= (struct compat_timeval __user*) userstamp;
+	int err = -ENOENT;
+	if(!sock_flag(sk, SOCK_TIMESTAMP))
+		sock_enable_timestamp(sk);
+	if(sk->sk_stamp.tv_sec == -1)
+		return err;
+	if(sk->sk_stamp.tv_sec == 0)
+		do_gettimeofday(&sk->sk_stamp);
+	if (put_user(sk->sk_stamp.tv_sec, &ctv->tv_sec) |
+			put_user(sk->sk_stamp.tv_usec, &ctv->tv_usec))
+				err = -EFAULT;
+	return err;
+}
+
 asmlinkage long compat_sys_getsockopt(int fd, int level, int optname,
 				char __user *optval, int __user *optlen)
 {
@@ -602,3 +619,5 @@ asmlinkage long compat_sys_socketcall(in
 	}
 	return ret;
 }
+
+EXPORT_SYMBOL(compat_sock_get_timestamp);

[PATCH 3/4 -2.6.15]:x25: 32 bit (socket layer) ioctl emulation for 64 bit kernels

[Shaun Pereira]

Allows x25 to work in 32 bit mode on 64  bit kernel

diff -uprN -X dontdiff linux-2.6.15-vanilla/net/x25/af_x25.c
linux-2.6.15/net/x25/af_x25.c
--- linux-2.6.15-vanilla/net/x25/af_x25.c	2006-01-03 14:21:10.000000000
+1100
+++ linux-2.6.15/net/x25/af_x25.c	2006-01-17 09:56:02.000000000 +1100
@@ -54,6 +54,8 @@
 #include <linux/notifier.h>
 #include <linux/init.h>
 #include <net/x25.h>
+#include <linux/compat.h>
+#include <net/compat.h>
 
 int sysctl_x25_restart_request_timeout = X25_DEFAULT_T20;
 int sysctl_x25_call_request_timeout    = X25_DEFAULT_T21;
@@ -68,6 +70,14 @@ static struct proto_ops x25_proto_ops;
 
 static struct x25_address null_x25_address = {"               "};
 
+#ifdef CONFIG_COMPAT
+struct compat_x25_subscrip_struct {
+    char device[200-sizeof(compat_ulong_t)];
+    compat_ulong_t global_facil_mask;
+    compat_uint_t extended;
+};
+#endif
+
 int x25_addr_ntoa(unsigned char *p, struct x25_address *called_addr,
 		  struct x25_address *calling_addr)
 {
@@ -1391,6 +1401,119 @@ static struct net_proto_family x25_famil
 	.owner	=	THIS_MODULE,
 };
 
+
+#ifdef CONFIG_COMPAT
+static int compat_x25_subscr_ioctl(unsigned int cmd,
+       	struct compat_x25_subscrip_struct __user *x25_subscr32)
+{
+	struct x25_subscrip_struct x25_subscr;
+	struct x25_neigh *nb;
+	struct net_device *dev;
+	int rc = -EINVAL;
+
+	if (cmd != SIOCX25GSUBSCRIP && cmd != SIOCX25SSUBSCRIP)
+   		goto out;
+
+	rc = -EFAULT;
+	if(copy_from_user(&x25_subscr, x25_subscr32, sizeof(*x25_subscr32)))
+		goto out;
+
+	rc = -EINVAL;
+	if ((dev = x25_dev_get(x25_subscr.device)) == NULL)
+		goto out;
+
+	if ((nb = x25_get_neigh(dev)) == NULL)
+		goto out_dev_put;
+
+	dev_put(dev);
+
+	if(cmd == SIOCX25GSUBSCRIP) {
+		x25_subscr.extended             = nb->extended;
+		x25_subscr.global_facil_mask     = nb->global_facil_mask;
+		rc = copy_to_user(x25_subscr32, &x25_subscr,
+	    		sizeof(*x25_subscr32)) ? -EFAULT : 0;
+	} else {
+		rc = -EINVAL;
+		if (!(x25_subscr.extended && x25_subscr.extended != 1)) {
+			 rc = 0;
+			 nb->extended            = x25_subscr.extended;
+			 nb->global_facil_mask   = x25_subscr.global_facil_mask;
+		}
+	}
+	x25_neigh_put(nb);
+out:
+	return rc;
+out_dev_put:
+	dev_put(dev);
+	goto out;
+}
+
+static int compat_x25_ioctl(struct socket *sock, unsigned int cmd,
unsigned long arg)
+{
+	void __user *argp = compat_ptr(arg);
+	struct sock *sk = sock->sk;
+
+	int rc = -ENOIOCTLCMD;
+
+	switch(cmd) {
+		case TIOCOUTQ:
+		case TIOCINQ:
+			rc = x25_ioctl(sock, cmd, (unsigned long)argp);
+			break;
+		case SIOCGSTAMP:
+			rc = -EINVAL;
+			if (sk)
+				rc = compat_sock_get_timestamp(sk,
+						(struct timeval __user*)argp);
+			break;
+		case SIOCGIFADDR:
+		case SIOCSIFADDR:
+		case SIOCGIFDSTADDR:
+		case SIOCSIFDSTADDR:
+		case SIOCGIFBRDADDR:
+		case SIOCSIFBRDADDR:
+		case SIOCGIFNETMASK:
+		case SIOCSIFNETMASK:
+		case SIOCGIFMETRIC:
+		case SIOCSIFMETRIC:
+			rc = -EINVAL;
+			break;
+		case SIOCADDRT:
+		case SIOCDELRT:
+			rc = -EPERM;
+			if(!capable(CAP_NET_ADMIN))
+				break;
+			rc = x25_route_ioctl(cmd, argp);
+			break;
+		case SIOCX25GSUBSCRIP:
+			rc = compat_x25_subscr_ioctl(cmd, argp);
+			break;
+		case SIOCX25SSUBSCRIP:
+			rc = -EPERM;
+			if (!capable(CAP_NET_ADMIN))
+				break;
+			rc = compat_x25_subscr_ioctl(cmd, argp);
+			break;
+		case SIOCX25GFACILITIES:
+		case SIOCX25SFACILITIES:
+		case SIOCX25GCALLUSERDATA:
+		case SIOCX25SCALLUSERDATA:
+		case SIOCX25GCAUSEDIAG:
+		case SIOCX25SCUDMATCHLEN:
+		case SIOCX25CALLACCPTAPPRV:
+		case SIOCX25SENDCALLACCPT:
+			rc = x25_ioctl(sock, cmd, (unsigned long)argp);
+			break;
+		default:
+			rc = -ENOIOCTLCMD;
+			break;
+	}
+
+	return rc;
+}
+
+#endif
+
 static struct proto_ops SOCKOPS_WRAPPED(x25_proto_ops) = {
 	.family =	AF_X25,
 	.owner =	THIS_MODULE,
@@ -1402,6 +1525,9 @@ static struct proto_ops SOCKOPS_WRAPPED(
 	.getname =	x25_getname,
 	.poll =		datagram_poll,
 	.ioctl =	x25_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl = compat_x25_ioctl,
+#endif
 	.listen =	x25_listen,
 	.shutdown =	sock_no_shutdown,
 	.setsockopt =	x25_setsockopt,

Re: [PATCH 3/4 -2.6.15]:x25: 32 bit (socket layer) ioctl emulation for 64 bit kernels

[Arnd Bergmann]

Am Dienstag, 17. Januar 2006 00:12 schrieb Shaun Pereira:
> +static int compat_x25_subscr_ioctl(unsigned int cmd,
> +               struct compat_x25_subscrip_struct __user *x25_subscr32)
> +{
> +       struct x25_subscrip_struct x25_subscr;
> +       struct x25_neigh *nb;
> +       struct net_device *dev;
> +       int rc = -EINVAL;
> +
> +       if (cmd != SIOCX25GSUBSCRIP && cmd != SIOCX25SSUBSCRIP)
> +               goto out;

btw, the above check is not needed here, but that's not my point.

> +
> +       rc = -EFAULT;
> +       if(copy_from_user(&x25_subscr, x25_subscr32, sizeof(*x25_subscr32))) 
> +               goto out;

Unfortunately, I just found another bug in this code, similar to one you 
already fixed in the sock_get_timestamp handler:
You can't do the copy_from_user like this if the arguments have different 
types. Changing the declaration 'struct x25_subscrip_struct x25_subscr;'
to 'struct compat_x25_subscrip_struct x25_subscr;' should fix this problem,
but please verify that it really works with a test case that relies on the 
contents of x25_subscr->extended.

	Arnd <><

[PATCH 3/4 -2.6.15- RESEND]:x25: 32 bit (socket layer) ioctl emulation for 64 bit kernels

[Shaun Pereira]

Hi Arnd
 Here is the corrected version.
Using the x25_subscrip_struct structure (extend value = 1) set 
the following values in the kernel after copy_from_user

 global facility mask is F00000000
 Extended value is 1


Using the 32 bit compat_x25_subscrip_struct
set the following values in the struct after
copying from user.
global facility mask is 0000000F
Extended value is 1

The nb->global_facil_mask was then 
set to 0000000F when extended was 1

Thanks for this,

/Shaun

diff -uprN -X dontdiff linux-2.6.15-vanilla/net/x25/af_x25.c
linux-2.6.15/net/x25/af_x25.c
--- linux-2.6.15-vanilla/net/x25/af_x25.c	2006-01-03 14:21:10.000000000
+1100
+++ linux-2.6.15/net/x25/af_x25.c	2006-01-17 15:08:30.000000000 +1100
@@ -54,6 +54,8 @@
 #include <linux/notifier.h>
 #include <linux/init.h>
 #include <net/x25.h>
+#include <linux/compat.h>
+#include <net/compat.h>
 
 int sysctl_x25_restart_request_timeout = X25_DEFAULT_T20;
 int sysctl_x25_call_request_timeout    = X25_DEFAULT_T21;
@@ -68,6 +70,14 @@ static struct proto_ops x25_proto_ops;
 
 static struct x25_address null_x25_address = {"               "};
 
+#ifdef CONFIG_COMPAT
+struct compat_x25_subscrip_struct {
+	char device[200-sizeof(compat_ulong_t)];
+	compat_ulong_t global_facil_mask;
+	compat_uint_t extended;
+};
+#endif
+
 int x25_addr_ntoa(unsigned char *p, struct x25_address *called_addr,
 		  struct x25_address *calling_addr)
 {
@@ -1391,6 +1401,116 @@ static struct net_proto_family x25_famil
 	.owner	=	THIS_MODULE,
 };
 
+
+#ifdef CONFIG_COMPAT
+static int compat_x25_subscr_ioctl(unsigned int cmd,
+       	struct compat_x25_subscrip_struct __user *x25_subscr32)
+{
+	struct compat_x25_subscrip_struct x25_subscr;
+	struct x25_neigh *nb;
+	struct net_device *dev;
+	int rc = -EINVAL;
+
+	rc = -EFAULT;
+	if(copy_from_user(&x25_subscr, x25_subscr32, sizeof(*x25_subscr32)))
+		goto out;
+
+	rc = -EINVAL;
+	if ((dev = x25_dev_get(x25_subscr.device)) == NULL)
+		goto out;
+
+	if ((nb = x25_get_neigh(dev)) == NULL)
+		goto out_dev_put;
+
+	dev_put(dev);
+
+	if(cmd == SIOCX25GSUBSCRIP) {
+		x25_subscr.extended             = nb->extended;
+		x25_subscr.global_facil_mask     = nb->global_facil_mask;
+		rc = copy_to_user(x25_subscr32, &x25_subscr,
+	    		sizeof(*x25_subscr32)) ? -EFAULT : 0;
+	} else {
+		rc = -EINVAL;
+		if (!(x25_subscr.extended && x25_subscr.extended != 1)) {
+			 rc = 0;
+			 nb->extended            = x25_subscr.extended;
+			 nb->global_facil_mask   = x25_subscr.global_facil_mask;
+		}
+	}
+	x25_neigh_put(nb);
+out:
+	return rc;
+out_dev_put:
+	dev_put(dev);
+	goto out;
+}
+
+static int compat_x25_ioctl(struct socket *sock, unsigned int cmd,
unsigned long arg)
+{
+	void __user *argp = compat_ptr(arg);
+	struct sock *sk = sock->sk;
+
+	int rc = -ENOIOCTLCMD;
+
+	switch(cmd) {
+		case TIOCOUTQ:
+		case TIOCINQ:
+			rc = x25_ioctl(sock, cmd, (unsigned long)argp);
+			break;
+		case SIOCGSTAMP:
+			rc = -EINVAL;
+			if (sk)
+				rc = compat_sock_get_timestamp(sk,
+						(struct timeval __user*)argp);
+			break;
+		case SIOCGIFADDR:
+		case SIOCSIFADDR:
+		case SIOCGIFDSTADDR:
+		case SIOCSIFDSTADDR:
+		case SIOCGIFBRDADDR:
+		case SIOCSIFBRDADDR:
+		case SIOCGIFNETMASK:
+		case SIOCSIFNETMASK:
+		case SIOCGIFMETRIC:
+		case SIOCSIFMETRIC:
+			rc = -EINVAL;
+			break;
+		case SIOCADDRT:
+		case SIOCDELRT:
+			rc = -EPERM;
+			if(!capable(CAP_NET_ADMIN))
+				break;
+			rc = x25_route_ioctl(cmd, argp);
+			break;
+		case SIOCX25GSUBSCRIP:
+			rc = compat_x25_subscr_ioctl(cmd, argp);
+			break;
+		case SIOCX25SSUBSCRIP:
+			rc = -EPERM;
+			if (!capable(CAP_NET_ADMIN))
+				break;
+			rc = compat_x25_subscr_ioctl(cmd, argp);
+			break;
+		case SIOCX25GFACILITIES:
+		case SIOCX25SFACILITIES:
+		case SIOCX25GCALLUSERDATA:
+		case SIOCX25SCALLUSERDATA:
+		case SIOCX25GCAUSEDIAG:
+		case SIOCX25SCUDMATCHLEN:
+		case SIOCX25CALLACCPTAPPRV:
+		case SIOCX25SENDCALLACCPT:
+			rc = x25_ioctl(sock, cmd, (unsigned long)argp);
+			break;
+		default:
+			rc = -ENOIOCTLCMD;
+			break;
+	}
+
+	return rc;
+}
+
+#endif
+
 static struct proto_ops SOCKOPS_WRAPPED(x25_proto_ops) = {
 	.family =	AF_X25,
 	.owner =	THIS_MODULE,
@@ -1402,6 +1522,9 @@ static struct proto_ops SOCKOPS_WRAPPED(
 	.getname =	x25_getname,
 	.poll =		datagram_poll,
 	.ioctl =	x25_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl = compat_x25_ioctl,
+#endif
 	.listen =	x25_listen,
 	.shutdown =	sock_no_shutdown,
 	.setsockopt =	x25_setsockopt,

32 bit (socket layer) ioctl emulation for 64 bit kernels

[Shaun Pereira]

Hi Arnd
 Assuming you are happy with the state of the patches, is there anyway
for me to know if they will become a part of the next release?
Usually submitted/reviewed patches to netdev does not not always
guarantee they will be acccepted/signed-off.
Any advice would be useful
Thanks
Shaun

Re: 32 bit (socket layer) ioctl emulation for 64 bit kernels

[Arnd Bergmann]

Am Wednesday 18 January 2006 07:56 schrieb Shaun Pereira:
>  Assuming you are happy with the state of the patches, is there anyway
> for me to know if they will become a part of the next release?

I don't see any more technical problems with your patches. You still need
to proper patch description and Signed-off-by: line like it is described
in Documentation/SubmittingPatches.
You can add an 'Acked-by: Arnd Bergmann <arnd@arndb.de>' line to the four
patches you posted last if you like.

> Usually submitted/reviewed patches to netdev does not not always
> guarantee they will be acccepted/signed-off.
> Any advice would be useful

I'm not that familiar with the process for non-driver patches for netdev
(nor for device drivers as it seems ;-)), but my understanding is that you 
should address those to Jeff Garzik as well, asking for inclusion in the
netdev-2.6 git tree in your introductory '[PATCH 0/4]' mail.

Since the official merge window for 2.6.16 is now over (2.6.16-rc1 has been
released), it may have to wait for 2.6.17 to become part of the mainline
kernel, that probably depends on Jeffs judgement.

I would think it can still go in since it is a bug fix for the execution of
32 bit programs using x25 ioctls, but it's clearly not my decision ;-).

	Arnd <><

Re: 32 bit (socket layer) ioctl emulation for 64 bit kernels

[David S. Miller]

From: Arnd Bergmann <arnd@arndb.de>
Date: Thu, 19 Jan 2006 01:57:37 +0100

> I'm not that familiar with the process for non-driver patches for
> netdev (nor for device drivers as it seems ;-)), but my
> understanding is that you should address those to Jeff Garzik as
> well, asking for inclusion in the netdev-2.6 git tree in your
> introductory '[PATCH 0/4]' mail.

Those should be CC:'d to me, not Jeff, he has enough to review
and merge :)

[PATCH 4/4]x25: 32 bit (socket layer) ioctl emulation for 64 bit kernels

[Shaun Pereira]

A small fix for the following error, when trying to run a 64 bit x25
server application.

T2 kernel: schedule_timeout: 
 wrong timeout value ffffffffffffffff from ffffffff88164796

diff -uprN -X dontdiff linux-2.6.15-vanilla/net/x25/af_x25.c
linux-2.6.15/net/x25/af_x25.c
--- linux-2.6.15-vanilla/net/x25/af_x25.c	2006-01-17 09:57:38.000000000
+1100
+++ linux-2.6.15/net/x25/af_x25.c	2006-01-17 09:58:04.000000000 +1100
@@ -747,7 +747,7 @@ out:
 	return rc;
 }
 
-static int x25_wait_for_data(struct sock *sk, int timeout)
+static int x25_wait_for_data(struct sock *sk, long timeout)
 {
 	DECLARE_WAITQUEUE(wait, current);
 	int rc = 0;

Re: 32 bit (socket layer) ioctl emulation for 64 bit kernels

[Arnd Bergmann]

On Monday 16 January 2006 06:59, Shaun Pereira wrote:
> 
> I was wondering if this the compat_sock_get_timestamp function is
> needed? If I were to remove the SIOCGSTAMP case from the
> compat_x25_ioctl function, then a SIOCGSTAMP ioctl system call would
> return -ENOIOCTLCMD which could  then be handled by do_siocgstamp
> handler in the ioctl32_hash_table? (fs/compat_ioctl.c)
> In which case I could remove this patch from the rest of the series.

Yes, that would also work, as I already mentioned (or tried to)
in one of my earlier comments. I would prefer to have this patch
though, because in the long term, I think we should migrate more
stuff away from the hash table and having the function there
means that others can use it as well.

> +       err = -EFAULT;
> +       if(access_ok(VERIFTY_WRITE, ctv, sizeof(*ctv))) {
> +               err = __put_user(sk->sk_stamp.tv_sec, &ctv->tv_sec);
> +               err != __put_user(sk->sk_stamp.tv_usec, &ctv->tv_usec);
> +       }
> +       return err;
> +}

This copies the correct data down to user space now, but might result
in returning an invalid error code.
In the second line you now have 'err != __put_user(...);', which is
a comparison, not an assignment!
For readability, I would simply write that as:

	ret = 0;
	if (put_user(sk->sk_stamp.tv_sec, &ctv->tv_sec) |
	    put_user(sk->sk_stamp.tv_usec, &ctv->tv_usec))
		err = -EFAULT;

You can also write it like your code, but with '|' instead of '!', but
that requires the additional knowledge that __put_user can only ever
return '0' or '-EFAULT' itself and that the bitwise or of those is
therefore also one of these two.

	Arnd <><