Re: unregister_ioctl32_conversion and modules. ioctl32 revisited.

Re: unregister_ioctl32_conversion and modules. ioctl32 revisited.

[Lee Revell]

On Wed, 2004-12-15 at 19:20 +0100, Takashi Iwai wrote:
> At Wed, 15 Dec 2004 09:46:35 +0200,
> Michael S. Tsirkin wrote:
> > 
> > There were two additional motivations for my patch:
> > 1. Make it possible to avoid the BKL completely by writing
> >    an ioctl with proper internal locking.
> > 2. As noted by  Juergen Kreileder, the compat hash does not work
> >    for ioctls that encode additional information in the command, like this:
> > 
> > #define EVIOCGBIT(ev,len)  _IOC(_IOC_READ, 'E', 0x20 + ev, len)
> 
> I like the idea very well.  Other benifits in addition:
> 

How does this all relate to Ingo's ->unlocked_ioctl stuff which is "an
official way to do BKL-less ioctls"?

http://lkml.org/lkml/2004/12/14/53

Lee

Re: unregister_ioctl32_conversion and modules. ioctl32 revisited.

[Michael S. Tsirkin]

Hello!
Quoting r. Lee Revell (rlrevell@joe-job.com) "Re: unregister_ioctl32_conversion and modules. ioctl32 revisited.":
> On Wed, 2004-12-15 at 19:20 +0100, Takashi Iwai wrote:
> > At Wed, 15 Dec 2004 09:46:35 +0200,
> > Michael S. Tsirkin wrote:
> > > 
> > > There were two additional motivations for my patch:
> > > 1. Make it possible to avoid the BKL completely by writing
> > >    an ioctl with proper internal locking.
> > > 2. As noted by  Juergen Kreileder, the compat hash does not work
> > >    for ioctls that encode additional information in the command, like this:
> > > 
> > > #define EVIOCGBIT(ev,len)  _IOC(_IOC_READ, 'E', 0x20 + ev, len) > > 
> > I like the idea very well.  Other benifits in addition:
> > 
> 
> How does this all relate to Ingo's ->unlocked_ioctl stuff which is "an
> official way to do BKL-less ioctls"?
> 
> http://lkml.org/lkml/2004/12/14/53
> 
> Lee

It conflicts :) When I wrote the original patch for 2.6.8.1
I didnt see the unlocked_ioctl.patch.

unlocked_ioctl is the same as ioctl_native in my patch, except that

1. I added more documentation in several places :)
   (notably Documentation/filesystems/Locking)

2. I thought it a bit silly to name a function for what
   it does not do (does not take a lock), and we
   still need a call for the compat layer.

My patch adds another call to enable special handling
for 32 bit ioctls on 64 bit systems.

I could look at porting to -rc3-mm1, unless
we dont want to back unlocked_ioctl.patch off.
What do people here think?

MST


-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now. 
http://productguide.itmanagersjournal.com/

Re: [discuss] Re: unregister_ioctl32_conversion and modules. ioctl32 revisited.

[Andi Kleen]

On Wed, Dec 15, 2004 at 01:30:59PM -0500, Lee Revell wrote:
> On Wed, 2004-12-15 at 19:20 +0100, Takashi Iwai wrote:
> > At Wed, 15 Dec 2004 09:46:35 +0200,
> > Michael S. Tsirkin wrote:
> > > 
> > > There were two additional motivations for my patch:
> > > 1. Make it possible to avoid the BKL completely by writing
> > >    an ioctl with proper internal locking.
> > > 2. As noted by  Juergen Kreileder, the compat hash does not work
> > >    for ioctls that encode additional information in the command, like this:
> > > 
> > > #define EVIOCGBIT(ev,len)  _IOC(_IOC_READ, 'E', 0x20 + ev, len)
> > 
> > I like the idea very well.  Other benifits in addition:
> > 
> 
> How does this all relate to Ingo's ->unlocked_ioctl stuff which is "an
> official way to do BKL-less ioctls"?

This is another "official" way which is more powerful. I suppose it will
replace Ingo's patch.

-Andi


-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now. 
http://productguide.itmanagersjournal.com/

Re: [discuss] Re: unregister_ioctl32_conversion and modules. ioctl32 revisited.

[Ingo Molnar]

* Andi Kleen <ak@suse.de> wrote:

> > How does this all relate to Ingo's ->unlocked_ioctl stuff which is "an
> > official way to do BKL-less ioctls"?
> 
> This is another "official" way which is more powerful. I suppose it
> will replace Ingo's patch.

the ALSA changes are mine but i'm otherwise building ontop of the 
following patch in -rc3-mm1:

 http://kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.10-rc3/2.6.10-rc3-mm1/broken-out/unlocked_ioctl.patch

whichever approach gets adopted upstream, the various actors ought to
synchronize a bit more - this is the third approach so far in a very
short interval to get rid of the BKL in ioctls :-)

	Ingo

Re: [discuss] Re: unregister_ioctl32_conversion and modules. ioctl32 revisited.

[Andi Kleen]

On Thu, Dec 16, 2004 at 08:53:01AM +0100, Ingo Molnar wrote:
> 
> * Andi Kleen <ak@suse.de> wrote:
> 
> > > How does this all relate to Ingo's ->unlocked_ioctl stuff which is "an
> > > official way to do BKL-less ioctls"?
> > 
> > This is another "official" way which is more powerful. I suppose it
> > will replace Ingo's patch.
> 
> the ALSA changes are mine but i'm otherwise building ontop of the 
> following patch in -rc3-mm1:
> 
>  http://kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.10-rc3/2.6.10-rc3-mm1/broken-out/unlocked_ioctl.patch
> 
> whichever approach gets adopted upstream, the various actors ought to
> synchronize a bit more - this is the third approach so far in a very
> short interval to get rid of the BKL in ioctls :-)

I think Michael's patch is best (but I'm probably biased) because it addresses
the independent problem of a race in unregister_ioctl32_conversion() too
(and some other smaller issues in ioctl 32bit emulation)

Andrew, could we replace unlocked_ioctl.patch with Michael's patch? Adapting
depending code should be very easy, since only the name of the function
vector has changed.

-Andi

Re: [discuss] Re: unregister_ioctl32_conversion and modules. ioctl32 revisited.

[Andrew Morton]

Andi Kleen <ak@suse.de> wrote:
>
> I think Michael's patch is best (but I'm probably biased) because it addresses
>  the independent problem of a race in unregister_ioctl32_conversion() too
>  (and some other smaller issues in ioctl 32bit emulation)

They should be separate patches.

>  Andrew, could we replace unlocked_ioctl.patch with Michael's patch?

Where would one locate Michael's patch?

Re: [discuss] Re: unregister_ioctl32_conversion and modules. ioctl32 revisited.

[Michael S. Tsirkin]

Hello!
Quoting r. Andrew Morton (akpm@osdl.org) "Re: [discuss] Re: unregister_ioctl32_conversion and modules. ioctl32 revisited.":
> Andi Kleen <ak@suse.de> wrote:
> >
> > I think Michael's patch is best (but I'm probably biased) because it addresses
> >  the independent problem of a race in unregister_ioctl32_conversion() too
> >  (and some other smaller issues in ioctl 32bit emulation)
> 
> They should be separate patches.
> 
> >  Andrew, could we replace unlocked_ioctl.patch with Michael's patch?
> 
> Where would one locate Michael's patch?

Here it is for review (against 2.6.10-rc3) http://lkml.org/lkml/2004/12/15/62
I plan to incorporate Arnd Bergmann's comments and repost later today.

MST

Re: [discuss] Re: unregister_ioctl32_conversion and modules. ioctl32 revisited.

[Andi Kleen]

On Thu, Dec 16, 2004 at 12:25:39AM -0800, Andrew Morton wrote:
> Andi Kleen <ak@suse.de> wrote:
> >
> > I think Michael's patch is best (but I'm probably biased) because it addresses
> >  the independent problem of a race in unregister_ioctl32_conversion() too
> >  (and some other smaller issues in ioctl 32bit emulation)
> 
> They should be separate patches.

The two new methods (ioctl_native and ioctl_compat) are in the same patch
because they basically touch the same piece of code and would be hard
to separate. The other stuff (actually replacing register_ioctl32_conversion and 
converting a few obvious users to use the BKL less fast path) will be addon patches.

> 
> >  Andrew, could we replace unlocked_ioctl.patch with Michael's patch?
> 
> Where would one locate Michael's patch?

See his mail.

-Andi

[PATCH] unregister_ioctl32_conversion and modules. ioctl32 revisited.

[Michael S. Tsirkin]

Hello, Andrew!
Since I didnt get any negative comments on this (and some positive)
Andi Kleen suggested I submit the following patch to you.
It boots fine for me, please consider for mainline inclusion.

Dependencies:
The patch below is against 2.6.10-rc3. Please note it replaces
Ingo's ->unlocked_ioctl patch from rc3-mm1, so you have to back that off
before applying mine to mm:

http://kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.10-rc3/2.6.10-rc3-mm1/broken-out/unlocked_ioctl.patch

Please mail me directly with comments since I'm not on the list.

Changes from the last version I posted ( http://lkml.org/lkml/2004/12/15/62 )
- Two whitespace cleanups. I hope its all good now.
- Arnd Bergmann's idea: make it possible to go back from ioctl_compat
  to hash lookup (good for static ioctl tables) by returning -ENOIOCTLCMD
  from ioctl_compat
- Petr Vandrovec's idea: add HAVE_... macros to make it easier for
  out-of-kernel modules to detect the new file_operations.

Description:
The patch introduces two new methods (ioctl_native and ioctl_compat):
ioctl_native is called on native ioctl syscall, without BKL being taken,
and is, in that respect, equivalent to Ingo's unlocked_ioctl (which is
why it conflicts).
ioctl_compat is called on compat (i.e. 32 bit app on 64 bit OS) ioctl,
again without BKL being taken.
If a new call is not defined, default to the old behaviour.
(It should be possible for me to build a patch that applies on top of Ingo's
unlocked_ioctl, if its really needed let me know and I'll look at it the next
week.)


Motivation: Quoting Andi Kleen:
> Hallo,
> 
> There seems to be an unfixable module unload race in the current
> register_ioctl32_conversion support. The problem is that
> there is no way to wait for a conversion handler is blocked
> in a sleeping *_user access before module unloading. The module
> count is also not increase in this case.
> ...

[Snip]

> A better solution would be to switch the few users of 
> register_ioctl32_conversion() over to a new ->ioctl32 method
> in file_operations and do the conversion from there. This would
> avoid the race because the VFS will take care of the module
> count in open/release.
> 
> Michael did a patch for this some time ago for a different motivation -
> he had some benchmarks where the hash table lookup hurt and it was
> noticeable faster to use a O(1) ->ioctl32 lookup from the file_operations
> for his application.
> 
> An useful side effect would be also to the ability to support 
> a per device ioctl name space. While the core kernel doesn't have
> much (any?) ioctls with duplicated numbers this mistake seems
> to be quite common in out of tree drivers and it is hard to 
> fix without breaking compatibility.
> 
> And it would be faster for this case of course too, so even performance
> critical in kernel ioctls could be slowly converted to ioctl32
> I wouldn't do it for all, because the current central tables work
> reasonably well for them and most ioctls are not speed critical
> anyways.
> 
> As for in kernel code it won't affect much code because near 
> all conversion handlers in the main tree are not modular (alsa 
> is one exception, there are a few others e.g. in some raid drivers). 
> I expect it will be a bigger problem in the future though as ioctl 
> emulation becomes more widespread and is done more in individual drivers.
> 
> averell:lsrc/v2.6/linux% gid register_ioctl32_conversion | wc -l
> 75
> averell:lsrc/v2.6/linux% 
> 
> In tree users are alsa, aaraid, fusion, some s390 stuff, sisfb, alsa
> 
> My proposal would be to dust off Michael's patch and convert 
> all users in tree over to ioctl32 and then deprecate and later remove
> (un)register_ioctl32_conversion 

There was an additional motivation for my patch:
As noted by  Juergen Kreileder, the compat hash does not work
for ioctls that encode additional information in the command, like this:

#define EVIOCGBIT(ev,len)  _IOC(_IOC_READ, 'E', 0x20 + ev, len)

Comments?

Thanks,
MST

Signed-off-by: Michael Tsirkin <mst@mellanox.co.il>

diff -rup linux-2.6.10-rc3/Documentation/filesystems/Locking linux-2.6.10-rc3-mstioctl/Documentation/filesystems/Locking
--- linux-2.6.10-rc3/Documentation/filesystems/Locking	2004-12-16 15:20:36.000000000 +0200
+++ linux-2.6.10-rc3-mstioctl/Documentation/filesystems/Locking	2004-12-16 18:25:35.289970464 +0200
@@ -350,6 +350,10 @@ prototypes:
 	unsigned int (*poll) (struct file *, struct poll_table_struct *);
 	int (*ioctl) (struct inode *, struct file *, unsigned int,
 			unsigned long);
+	long (*ioctl_native) (struct inode *, struct file *, unsigned int,
+			unsigned long);
+	long (*ioctl_compat) (struct inode *, struct file *, unsigned int,
+			unsigned long);
 	int (*mmap) (struct file *, struct vm_area_struct *);
 	int (*open) (struct inode *, struct file *);
 	int (*flush) (struct file *);
@@ -383,6 +387,8 @@ aio_write:		no
 readdir: 		no
 poll:			no
 ioctl:			yes	(see below)
+ioctl_native:		no	(see below)
+ioctl_compat:		no	(see below)
 mmap:			no
 open:			maybe	(see below)
 flush:			no
@@ -428,6 +434,9 @@ move ->readdir() to inode_operations and
 anything that resembles union-mount we won't have a struct file for all
 components. And there are other reasons why the current interface is a mess...
 
+->ioctl() on regular files is superceded by the ->ioctl_native() and
+->ioctl_compat() pair. The lock is not taken for these new calls.
+
 ->read on directories probably must go away - we should just enforce -EISDIR
 in sys_read() and friends.
 
diff -rup linux-2.6.10-rc3/include/linux/fs.h linux-2.6.10-rc3-mstioctl/include/linux/fs.h
--- linux-2.6.10-rc3/include/linux/fs.h	2004-12-16 15:20:46.000000000 +0200
+++ linux-2.6.10-rc3-mstioctl/include/linux/fs.h	2004-12-16 18:25:35.291970160 +0200
@@ -900,6 +900,12 @@ typedef struct {
 
 typedef int (*read_actor_t)(read_descriptor_t *, struct page *, unsigned long, unsigned long);
 
+/* These macros are for out of kernel modules to test that
+ * the kernel supports the ioctl_native and ioctl_compat
+ * fields in struct file_operations. */
+#define HAVE_IOCTL_COMPAT 1
+#define HAVE_IOCTL_NATIVE 1
+
 /*
  * NOTE:
  * read, write, poll, fsync, readv, writev can be called
@@ -915,6 +921,24 @@ struct file_operations {
 	int (*readdir) (struct file *, void *, filldir_t);
 	unsigned int (*poll) (struct file *, struct poll_table_struct *);
 	int (*ioctl) (struct inode *, struct file *, unsigned int, unsigned long);
+	/* The two calls ioctl_native and ioctl_compat described below
+	 * can be used as a replacement for the ioctl call above. They
+	 * take precedence over ioctl: thus if they are set, ioctl is
+	 * not used.  Unlike ioctl, BKL is not taken: drivers manage
+	 * their own locking. */
+
+	/* If ioctl_native is set, it is used instead of ioctl for
+	 * native ioctl syscalls.
+	 * Note that the standard glibc ioctl trims the return code to
+	 * type int, so dont try to put a 64 bit value there.
+	 */
+	long (*ioctl_native) (struct inode *, struct file *, unsigned int, unsigned long);
+	/* If ioctl_compat is set, it is used for a 32 bit compatible
+	 * ioctl (i.e. a 32 bit binary running on a 64 bit OS).
+	 * Return -ENOIOCTLCMD if you dont handle it.
+	 * Note that only the low 32 bit of the return code are passed
+	 * to the user-space application. */
+	long (*ioctl_compat) (struct inode *, struct file *, unsigned int, unsigned long);
 	int (*mmap) (struct file *, struct vm_area_struct *);
 	int (*open) (struct inode *, struct file *);
 	int (*flush) (struct file *);
diff -rup linux-2.6.10-rc3/include/linux/ioctl.h linux-2.6.10-rc3-mstioctl/include/linux/ioctl.h
--- linux-2.6.10-rc3/include/linux/ioctl.h	2004-12-16 15:19:34.000000000 +0200
+++ linux-2.6.10-rc3-mstioctl/include/linux/ioctl.h	2004-12-16 18:25:35.291970160 +0200
@@ -3,5 +3,13 @@
 
 #include <asm/ioctl.h>
 
+/* Handles standard ioctl commands, and returns the result in status.
+   Does nothing and returns non-zero if cmd is not one of the standard commands.
+*/
+
+struct file;
+int std_sys_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg,
+		  struct file *filp, long *status);
+
 #endif /* _LINUX_IOCTL_H */
 
diff -rup linux-2.6.10-rc3/fs/ioctl.c linux-2.6.10-rc3-mstioctl/fs/ioctl.c
--- linux-2.6.10-rc3/fs/ioctl.c	2004-12-16 15:20:45.000000000 +0200
+++ linux-2.6.10-rc3-mstioctl/fs/ioctl.c	2004-12-16 18:27:21.000899960 +0200
@@ -36,7 +36,9 @@ static int file_ioctl(struct file *filp,
 			if ((error = get_user(block, p)) != 0)
 				return error;
 
+			lock_kernel();
 			res = mapping->a_ops->bmap(mapping, block);
+			unlock_kernel();
 			return put_user(res, p);
 		}
 		case FIGETBSZ:
@@ -46,29 +48,21 @@ static int file_ioctl(struct file *filp,
 		case FIONREAD:
 			return put_user(i_size_read(inode) - filp->f_pos, p);
 	}
-	if (filp->f_op && filp->f_op->ioctl)
-		return filp->f_op->ioctl(inode, filp, cmd, arg);
 	return -ENOTTY;
 }
 
 
-asmlinkage long sys_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg)
-{	
-	struct file * filp;
+EXPORT_SYMBOL(std_sys_ioctl);
+int std_sys_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg,
+		  struct file *filp, long *status)
+{
 	unsigned int flag;
-	int on, error = -EBADF;
-
-	filp = fget(fd);
-	if (!filp)
-		goto out;
+	int on, error, unknown = 0;
 
 	error = security_file_ioctl(filp, cmd, arg);
-	if (error) {
-                fput(filp);
+	if (error)
                 goto out;
-        }
 
-	lock_kernel();
 	switch (cmd) {
 		case FIOCLEX:
 			set_close_on_exec(fd, 1);
@@ -100,8 +94,11 @@ asmlinkage long sys_ioctl(unsigned int f
 
 			/* Did FASYNC state change ? */
 			if ((flag ^ filp->f_flags) & FASYNC) {
-				if (filp->f_op && filp->f_op->fasync)
+				if (filp->f_op && filp->f_op->fasync) {
+					lock_kernel();
 					error = filp->f_op->fasync(fd, filp, on);
+					unlock_kernel();
+				}
 				else error = -ENOTTY;
 			}
 			if (error != 0)
@@ -125,15 +122,46 @@ asmlinkage long sys_ioctl(unsigned int f
 			break;
 		default:
 			error = -ENOTTY;
-			if (S_ISREG(filp->f_dentry->d_inode->i_mode))
+			if (S_ISREG(filp->f_dentry->d_inode->i_mode)) {
 				error = file_ioctl(filp, cmd, arg);
-			else if (filp->f_op && filp->f_op->ioctl)
-				error = filp->f_op->ioctl(filp->f_dentry->d_inode, filp, cmd, arg);
+			}
+			if (error == -ENOTTY) {
+				unknown = 1;
+				goto out;
+			}
+			break;
+	}
+out:
+	*status = error;
+	return unknown;
+}
+
+asmlinkage long sys_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg)
+{	
+	struct file *filp;
+	long error = -EBADF;
+	int fput_needed;
+
+	filp = fget_light(fd, &fput_needed);
+	if (!filp)
+		goto out2;
+
+	if (!std_sys_ioctl(fd, cmd, arg, filp, &error))
+		goto out;
+
+	if (filp->f_op && filp->f_op->ioctl_native)
+		error = filp->f_op->ioctl_native(filp->f_dentry->d_inode,
+						 filp, cmd, arg);
+	else if (filp->f_op && filp->f_op->ioctl) {
+		lock_kernel();
+		error = filp->f_op->ioctl(filp->f_dentry->d_inode,
+					  filp, cmd, arg);
+		unlock_kernel();
 	}
-	unlock_kernel();
-	fput(filp);
 
 out:
+	fput_light(filp, fput_needed);
+out2:
 	return error;
 }
 
diff -rup linux-2.6.10-rc3/fs/compat.c linux-2.6.10-rc3-mstioctl/fs/compat.c
--- linux-2.6.10-rc3/fs/compat.c	2004-12-16 15:20:45.000000000 +0200
+++ linux-2.6.10-rc3-mstioctl/fs/compat.c	2004-12-16 18:26:23.013715352 +0200
@@ -401,16 +401,21 @@ asmlinkage long compat_sys_ioctl(unsigne
 				unsigned long arg)
 {
 	struct file * filp;
-	int error = -EBADF;
+	long error = -EBADF;
 	struct ioctl_trans *t;
+	int fput_needed;
 
-	filp = fget(fd);
-	if(!filp)
+	filp = fget_light(fd, &fput_needed);
+	if (!filp)
 		goto out2;
 
-	if (!filp->f_op || !filp->f_op->ioctl) {
-		error = sys_ioctl (fd, cmd, arg);
+	if (!std_sys_ioctl(fd, cmd, arg, filp, &error))
 		goto out;
+	else if (filp->f_op && filp->f_op->ioctl_compat) {
+		error = filp->f_op->ioctl_compat(filp->f_dentry->d_inode,
+						 filp, cmd, arg);
+		if (error != -ENOIOCTLCMD)
+			goto out;
 	}
 
 	down_read(&ioctl32_sem);
@@ -425,9 +430,12 @@ asmlinkage long compat_sys_ioctl(unsigne
 			error = t->handler(fd, cmd, arg, filp);
 			unlock_kernel();
 			up_read(&ioctl32_sem);
-		} else {
+		} else if (filp->f_op && filp->f_op->ioctl) {
 			up_read(&ioctl32_sem);
-			error = sys_ioctl(fd, cmd, arg);
+			lock_kernel();
+			error = filp->f_op->ioctl(filp->f_dentry->d_inode,
+						  filp, cmd, arg);
+			unlock_kernel();
 		}
 	} else {
 		up_read(&ioctl32_sem);
@@ -466,7 +474,7 @@ asmlinkage long compat_sys_ioctl(unsigne
 		}
 	}
 out:
-	fput(filp);
+	fput_light(filp, fput_needed);
 out2:
 	return error;
 }

Re: [PATCH] unregister_ioctl32_conversion and modules. ioctl32 revisited.

[Christoph Hellwig]

> +	long (*ioctl_native) (struct inode *, struct file *, unsigned int,
> +			unsigned long);
> +	long (*ioctl_compat) (struct inode *, struct file *, unsigned int,
> +			unsigned long);

Please remove the struct inode * argument, it's easily retrievable
from file->f_dentry->d_inode.  The ioctl prototype is a leftover from
really old days where that wasn't true.

[PATCH] deprecate (un)register_ioctl32_conversion

[Michael S. Tsirkin]

Hello, Andrew, all!

Since in -mm1 we now have a race-free replacement (that being ioctl_compat),
here is a patch to deprecate (un)register_ioctl32_conversion.

Signed-off-by: Michael S. Tsirkin <mst@mellanox.co.il>


diff -ruI -u linux-2.6.10/include/linux/ioctl32.h linux-2.6.10-ioctls/include/linux/ioctl32.h
--- linux-2.6.10/include/linux/ioctl32.h	2004-12-24 23:33:49.000000000 +0200
+++ linux-2.6.10-ioctls/include/linux/ioctl32.h	2005-01-05 20:19:46.716664232 +0200
@@ -23,9 +23,12 @@
  */ 
 
 #ifdef CONFIG_COMPAT
-extern int register_ioctl32_conversion(unsigned int cmd,
+
+/* The following two calls trigger an unfixable module unload race. */
+/* Deprecated in favor of ioctl_compat in struct file_operations. */
+extern int __deprecated register_ioctl32_conversion(unsigned int cmd,
 				ioctl_trans_handler_t handler);
-extern int unregister_ioctl32_conversion(unsigned int cmd);
+extern int __deprecated unregister_ioctl32_conversion(unsigned int cmd);
 
 #else
 

Re: [PATCH] deprecate (un)register_ioctl32_conversion

[Christoph Hellwig]

On Wed, Jan 05, 2005 at 04:40:43PM +0200, Michael S. Tsirkin wrote:
> Hello, Andrew, all!
> 
> Since in -mm1 we now have a race-free replacement (that being ioctl_compat),
> here is a patch to deprecate (un)register_ioctl32_conversion.

Sorry, but this is a lot too early.  Once there's a handfull users left
in _mainline_ you can start deprecating it (or better remove it completely).

So far we have a non-final version of the replacement in -mm and no single
user converted.

Re: [PATCH] deprecate (un)register_ioctl32_conversion

[Michael S. Tsirkin]

Hello!
Quoting r. Christoph Hellwig (hch@infradead.org) "Re: [PATCH] deprecate (un)register_ioctl32_conversion":
> On Wed, Jan 05, 2005 at 04:40:43PM +0200, Michael S. Tsirkin wrote:
> > Hello, Andrew, all!
> > 
> > Since in -mm1 we now have a race-free replacement (that being ioctl_compat),
> > here is a patch to deprecate (un)register_ioctl32_conversion.
> 
> Sorry, but this is a lot too early.  Once there's a handfull users left
> in _mainline_ you can start deprecating it (or better remove it completely).

I dont know. So how will people know they are supposed to convert then?

> So far we have a non-final version of the replacement in -mm and no single
> user converted.

Christoph, I know you want to remove the inode parameter :)

Otherwise I think -mm1 has the final version of the replacement.

MST

Re: [PATCH] deprecate (un)register_ioctl32_conversion

[Christoph Hellwig]

On Wed, Jan 05, 2005 at 05:03:10PM +0200, Michael S. Tsirkin wrote:
> I dont know. So how will people know they are supposed to convert then?

Tell the janitors about it - or do it yourself.  Except for taking care
of the BKL going away it's a trivial conversion.

Re: [PATCH] deprecate (un)register_ioctl32_conversion

[Andrew Morton]

"Michael S. Tsirkin" <mst@mellanox.co.il> wrote:
>
> Quoting r. Christoph Hellwig (hch@infradead.org) "Re: [PATCH] deprecate (un)register_ioctl32_conversion":
>  > On Wed, Jan 05, 2005 at 04:40:43PM +0200, Michael S. Tsirkin wrote:
>  > > Hello, Andrew, all!
>  > > 
>  > > Since in -mm1 we now have a race-free replacement (that being ioctl_compat),
>  > > here is a patch to deprecate (un)register_ioctl32_conversion.
>  > 
>  > Sorry, but this is a lot too early.  Once there's a handfull users left
>  > in _mainline_ you can start deprecating it (or better remove it completely).
> 
>  I dont know. So how will people know they are supposed to convert then?
> 
>  > So far we have a non-final version of the replacement in -mm and no single
>  > user converted.
> 
>  Christoph, I know you want to remove the inode parameter :)
> 
>  Otherwise I think -mm1 has the final version of the replacement.

I merged Christoph's verion of the patch into -mm.



--- 25/Documentation/filesystems/Locking~ioctl-rework-2	Tue Jan  4 15:08:56 2005
+++ 25-akpm/Documentation/filesystems/Locking	Tue Jan  4 15:08:56 2005
@@ -350,6 +350,8 @@ prototypes:
 	unsigned int (*poll) (struct file *, struct poll_table_struct *);
 	int (*ioctl) (struct inode *, struct file *, unsigned int,
 			unsigned long);
+	long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long);
+	long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
 	int (*mmap) (struct file *, struct vm_area_struct *);
 	int (*open) (struct inode *, struct file *);
 	int (*flush) (struct file *);
@@ -383,6 +385,8 @@ aio_write:		no
 readdir: 		no
 poll:			no
 ioctl:			yes	(see below)
+unlocked_ioctl:		no	(see below)
+compat_ioctl:		no
 mmap:			no
 open:			maybe	(see below)
 flush:			no
@@ -428,6 +432,9 @@ move ->readdir() to inode_operations and
 anything that resembles union-mount we won't have a struct file for all
 components. And there are other reasons why the current interface is a mess...
 
+->ioctl() on regular files is superceded by the ->unlocked_ioctl() that
+doesn't take the BKL.
+
 ->read on directories probably must go away - we should just enforce -EISDIR
 in sys_read() and friends.
 
diff -puN fs/compat.c~ioctl-rework-2 fs/compat.c
--- 25/fs/compat.c~ioctl-rework-2	Tue Jan  4 15:08:56 2005
+++ 25-akpm/fs/compat.c	Tue Jan  4 15:08:56 2005
@@ -397,77 +397,87 @@ out:
 }
 EXPORT_SYMBOL(unregister_ioctl32_conversion); 
 
+static void compat_ioctl_error(struct file *filp, unsigned int fd,
+		unsigned int cmd, unsigned long arg)
+{
+	char buf[10];
+	char *fn = "?";
+	char *path;
+
+	/* find the name of the device. */
+	path = (char *)__get_free_page(GFP_KERNEL);
+	if (path) {
+		fn = d_path(filp->f_dentry, filp->f_vfsmnt, path, PAGE_SIZE);
+		if (IS_ERR(fn))
+			fn = "?";
+	}
+
+	sprintf(buf,"'%c'", (cmd>>24) & 0x3f);
+	if (!isprint(buf[1]))
+		sprintf(buf, "%02x", buf[1]);
+	printk("ioctl32(%s:%d): Unknown cmd fd(%d) "
+			"cmd(%08x){%s} arg(%08x) on %s\n",
+			current->comm, current->pid,
+			(int)fd, (unsigned int)cmd, buf,
+			(unsigned int)arg, fn);
+
+	if (path)
+		free_page((unsigned long)path);
+}
+
 asmlinkage long compat_sys_ioctl(unsigned int fd, unsigned int cmd,
 				unsigned long arg)
 {
-	struct file * filp;
+	struct file *filp;
 	int error = -EBADF;
 	struct ioctl_trans *t;
 
 	filp = fget(fd);
-	if(!filp)
-		goto out2;
-
-	if (!filp->f_op || !filp->f_op->ioctl) {
-		error = sys_ioctl (fd, cmd, arg);
+	if (!filp)
 		goto out;
+
+	if (!filp->f_op) {
+		if (!filp->f_op->ioctl)
+			goto do_ioctl;
+	} else if (filp->f_op->compat_ioctl) {
+		error = filp->f_op->compat_ioctl(filp, cmd, arg);
+		goto out_fput;
 	}
 
 	down_read(&ioctl32_sem);
+	for (t = ioctl32_hash_table[ioctl32_hash(cmd)]; t; t = t->next) {
+		if (t->cmd == cmd)
+			goto found_handler;
+	}
+	up_read(&ioctl32_sem);
 
-	t = ioctl32_hash_table[ioctl32_hash (cmd)];
-
-	while (t && t->cmd != cmd)
-		t = t->next;
-	if (t) {
-		if (t->handler) { 
-			lock_kernel();
-			error = t->handler(fd, cmd, arg, filp);
-			unlock_kernel();
-			up_read(&ioctl32_sem);
-		} else {
-			up_read(&ioctl32_sem);
-			error = sys_ioctl(fd, cmd, arg);
-		}
+	if (cmd >= SIOCDEVPRIVATE && cmd <= (SIOCDEVPRIVATE + 15)) {
+		error = siocdevprivate_ioctl(fd, cmd, arg);
 	} else {
+		static int count;
+
+		if (++count <= 50)
+			compat_ioctl_error(filp, fd, cmd, arg);
+		error = -EINVAL;
+	}
+
+	goto out_fput;
+
+ found_handler:
+	if (t->handler) {
+		lock_kernel();
+		error = t->handler(fd, cmd, arg, filp);
+		unlock_kernel();
 		up_read(&ioctl32_sem);
-		if (cmd >= SIOCDEVPRIVATE && cmd <= (SIOCDEVPRIVATE + 15)) {
-			error = siocdevprivate_ioctl(fd, cmd, arg);
-		} else {
-			static int count;
-			if (++count <= 50) {
-				char buf[10];
-				char *fn = "?";
-				char *path;
-
-				path = (char *)__get_free_page(GFP_KERNEL);
-
-				/* find the name of the device. */
-				if (path) {
-			       		fn = d_path(filp->f_dentry,
-						filp->f_vfsmnt, path,
-						PAGE_SIZE);
-					if (IS_ERR(fn))
-						fn = "?";
-				}
-
-				sprintf(buf,"'%c'", (cmd>>24) & 0x3f);
-				if (!isprint(buf[1]))
-				    sprintf(buf, "%02x", buf[1]);
-				printk("ioctl32(%s:%d): Unknown cmd fd(%d) "
-					"cmd(%08x){%s} arg(%08x) on %s\n",
-					current->comm, current->pid,
-					(int)fd, (unsigned int)cmd, buf,
-					(unsigned int)arg, fn);
-				if (path)
-					free_page((unsigned long)path);
-			}
-			error = -EINVAL;
-		}
+		goto out_fput;
 	}
-out:
+
+	up_read(&ioctl32_sem);
+ do_ioctl:
+	error = sys_ioctl(fd, cmd, arg);
+ out_fput:
 	fput(filp);
-out2:
+ out:
 	return error;
 }
 
diff -puN fs/ioctl.c~ioctl-rework-2 fs/ioctl.c
--- 25/fs/ioctl.c~ioctl-rework-2	Tue Jan  4 15:08:56 2005
+++ 25-akpm/fs/ioctl.c	Tue Jan  4 15:08:56 2005
@@ -16,7 +16,29 @@
 #include <asm/uaccess.h>
 #include <asm/ioctls.h>
 
-static int file_ioctl(struct file *filp,unsigned int cmd,unsigned long arg)
+static long do_ioctl(struct file *filp, unsigned int cmd,
+		unsigned long arg)
+{
+	int error = -ENOTTY;
+
+	if (!filp->f_op)
+		goto out;
+
+	if (filp->f_op->unlocked_ioctl) {
+		error = filp->f_op->unlocked_ioctl(filp, cmd, arg);
+	} else if (filp->f_op->ioctl) {
+		lock_kernel();
+		error = filp->f_op->ioctl(filp->f_dentry->d_inode,
+					  filp, cmd, arg);
+		unlock_kernel();
+	}
+
+ out:
+	return error;
+}
+
+static int file_ioctl(struct file *filp, unsigned int cmd,
+		unsigned long arg)
 {
 	int error;
 	int block;
@@ -36,7 +58,9 @@ static int file_ioctl(struct file *filp,
 			if ((error = get_user(block, p)) != 0)
 				return error;
 
+			lock_kernel();
 			res = mapping->a_ops->bmap(mapping, block);
+			unlock_kernel();
 			return put_user(res, p);
 		}
 		case FIGETBSZ:
@@ -46,14 +70,13 @@ static int file_ioctl(struct file *filp,
 		case FIONREAD:
 			return put_user(i_size_read(inode) - filp->f_pos, p);
 	}
-	if (filp->f_op && filp->f_op->ioctl)
-		return filp->f_op->ioctl(inode, filp, cmd, arg);
-	return -ENOTTY;
+
+	return do_ioctl(filp, cmd, arg);
 }
 
 
 asmlinkage long sys_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg)
-{	
+{
 	struct file * filp;
 	unsigned int flag;
 	int on, error = -EBADF;
@@ -63,12 +86,9 @@ asmlinkage long sys_ioctl(unsigned int f
 		goto out;
 
 	error = security_file_ioctl(filp, cmd, arg);
-	if (error) {
-                fput(filp);
-                goto out;
-        }
+	if (error)
+		goto out_fput;
 
-	lock_kernel();
 	switch (cmd) {
 		case FIOCLEX:
 			set_close_on_exec(fd, 1);
@@ -100,8 +120,11 @@ asmlinkage long sys_ioctl(unsigned int f
 
 			/* Did FASYNC state change ? */
 			if ((flag ^ filp->f_flags) & FASYNC) {
-				if (filp->f_op && filp->f_op->fasync)
+				if (filp->f_op && filp->f_op->fasync) {
+					lock_kernel();
 					error = filp->f_op->fasync(fd, filp, on);
+					unlock_kernel();
+				}
 				else error = -ENOTTY;
 			}
 			if (error != 0)
@@ -124,16 +147,15 @@ asmlinkage long sys_ioctl(unsigned int f
 				error = -ENOTTY;
 			break;
 		default:
-			error = -ENOTTY;
 			if (S_ISREG(filp->f_dentry->d_inode->i_mode))
 				error = file_ioctl(filp, cmd, arg);
-			else if (filp->f_op && filp->f_op->ioctl)
-				error = filp->f_op->ioctl(filp->f_dentry->d_inode, filp, cmd, arg);
+			else
+				error = do_ioctl(filp, cmd, arg);
+			break;
 	}
-	unlock_kernel();
+ out_fput:
 	fput(filp);
-
-out:
+ out:
 	return error;
 }
 
diff -puN include/linux/fs.h~ioctl-rework-2 include/linux/fs.h
--- 25/include/linux/fs.h~ioctl-rework-2	Tue Jan  4 15:08:56 2005
+++ 25-akpm/include/linux/fs.h	Tue Jan  4 15:08:56 2005
@@ -907,8 +907,8 @@ typedef int (*read_actor_t)(read_descrip
 
 /*
  * NOTE:
- * read, write, poll, fsync, readv, writev can be called
- *   without the big kernel lock held in all filesystems.
+ * read, write, poll, fsync, readv, writev, unlocked_ioctl and compat_ioctl
+ * can be called without the big kernel lock held in all filesystems.
  */
 struct file_operations {
 	struct module *owner;
@@ -920,6 +920,8 @@ struct file_operations {
 	int (*readdir) (struct file *, void *, filldir_t);
 	unsigned int (*poll) (struct file *, struct poll_table_struct *);
 	int (*ioctl) (struct inode *, struct file *, unsigned int, unsigned long);
+	long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long);
+	long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
 	int (*mmap) (struct file *, struct vm_area_struct *);
 	int (*open) (struct inode *, struct file *);
 	int (*flush) (struct file *);
_

Re: [PATCH] deprecate (un)register_ioctl32_conversion

[Michael S. Tsirkin]

Hello!
Quoting r. Andrew Morton (akpm@osdl.org) "Re: [PATCH] deprecate (un)register_ioctl32_conversion":
> >  Christoph, I know you want to remove the inode parameter :)
> > 
> >  Otherwise I think -mm1 has the final version of the replacement.
> 
> I merged Christoph's verion of the patch into -mm.

I see some more problems with this decision.

> 
> 
>  asmlinkage long compat_sys_ioctl(unsigned int fd, unsigned int cmd,
>  				unsigned long arg)
>  {
> -	struct file * filp;
> +	struct file *filp;
>  	int error = -EBADF;
>  	struct ioctl_trans *t;
>  
>  	filp = fget(fd);
> -	if(!filp)
> -		goto out2;
> -
> -	if (!filp->f_op || !filp->f_op->ioctl) {
> -		error = sys_ioctl (fd, cmd, arg);
> +	if (!filp)
>  		goto out;
> +
> +	if (!filp->f_op) {
> +		if (!filp->f_op->ioctl)
> +			goto do_ioctl;
> +	} else if (filp->f_op->compat_ioctl) {
> +		error = filp->f_op->compat_ioctl(filp, cmd, arg);
> +		goto out_fput;
>  	}


Stare at this as I might, I dont understand why does it make sence.
So if filp->f_op is NULL, you are then checking filp->f_op->ioctl?
Looks like an oops to me.

What should be there:

> +	if (!filp->f_op) {
> +		goto do_ioctl;
> +	} else if (filp->f_op->compat_ioctl) {
> +		error = filp->f_op->compat_ioctl(filp, cmd, arg);
> +		goto out_fput;

Look, was this patch even tested?

MST

Re: [PATCH] deprecate (un)register_ioctl32_conversion

[Christoph Hellwig]

> Stare at this as I might, I dont understand why does it make sence.
> So if filp->f_op is NULL, you are then checking filp->f_op->ioctl?
> Looks like an oops to me.

I doesn't make sense, but fortunately files with NULL filp->f_op don't
happen in practice (need to research whether it can't happen in theory
either so we could lose lots of checks)

> 
> What should be there:
> 
> > +	if (!filp->f_op) {
> > +		goto do_ioctl;
> > +	} else if (filp->f_op->compat_ioctl) {
> > +		error = filp->f_op->compat_ioctl(filp, cmd, arg);
> > +		goto out_fput;

not correct either, see the incremental patch below.

> Look, was this patch even tested?

Yes.

--- linux-2.6.10-mm2.orig/fs/compat.c	2005-01-06 11:40:18.831900000 +0100
+++ linux-2.6.10-mm2/fs/compat.c	2005-01-06 15:50:23.802874672 +0100
@@ -436,10 +436,10 @@
 	if (!filp)
 		goto out;
 
-	if (!filp->f_op) {
-		if (!filp->f_op->ioctl)
-			goto do_ioctl;
-	} else if (filp->f_op->compat_ioctl) {
+	if (!filp->f_op || !filp->f_op->ioctl)
+		goto do_ioctl;
+
+	if (filp->f_op || filp->f_op->compat_ioctl) {
 		error = filp->f_op->compat_ioctl(filp, cmd, arg);
 		goto out_fput;
 	}

Re: [PATCH] deprecate (un)register_ioctl32_conversion

[Michael S. Tsirkin]

Hello!
Quoting r. Christoph Hellwig (hch@infradead.org) "Re: [PATCH] deprecate (un)register_ioctl32_conversion":
> > Stare at this as I might, I dont understand why does it make sence.
> > So if filp->f_op is NULL, you are then checking filp->f_op->ioctl?
> > Looks like an oops to me.
> 
> I doesn't make sense, but fortunately files with NULL filp->f_op don't
> happen in practice (need to research whether it can't happen in theory
> either so we could lose lots of checks)

Interesting. At least for character devices coming out of modules I think you
dont - you need at least fops->owner.

> --- linux-2.6.10-mm2.orig/fs/compat.c	2005-01-06 11:40:18.831900000 +0100
> +++ linux-2.6.10-mm2/fs/compat.c	2005-01-06 15:50:23.802874672 +0100
> @@ -436,10 +436,10 @@
>  	if (!filp)
>  		goto out;
>  
> -	if (!filp->f_op) {
> -		if (!filp->f_op->ioctl)
> -			goto do_ioctl;
> -	} else if (filp->f_op->compat_ioctl) {
> +	if (!filp->f_op || !filp->f_op->ioctl)
> +		goto do_ioctl;
> +
> +	if (filp->f_op || filp->f_op->compat_ioctl) {
>  		error = filp->f_op->compat_ioctl(filp, cmd, arg);
>  		goto out_fput;
>  	}

So now if I dont have ->ioctl the ioctl_compat wont be called.
What if I only have unlocked_ioctl?

MST

Re: [PATCH] deprecate (un)register_ioctl32_conversion

[Christoph Hellwig]

On Thu, Jan 06, 2005 at 05:22:48PM +0200, Michael S. Tsirkin wrote:
> > +	if (!filp->f_op || !filp->f_op->ioctl)
> > +		goto do_ioctl;
> > +
> > +	if (filp->f_op || filp->f_op->compat_ioctl) {
> >  		error = filp->f_op->compat_ioctl(filp, cmd, arg);
> >  		goto out_fput;
> >  	}
> 
> So now if I dont have ->ioctl the ioctl_compat wont be called.
> What if I only have unlocked_ioctl?

Indeed.  In my test setup I didn't have a driver using both.  So let's
think a little more what checks we want.

The original intention (pre-patch) was that without an ioctl entry
we'd skip the hash table lookup and skip right to trying the few standard
ioctls.

So with ->compat_ioctl we should try that one first, then checking
for either ->ioctl or ->unlocked_ioctl beeing there.  Like the patch
below (this time it's actually untested because all my 64bit machines
are in use):


--- linux-2.6.10-mm2.orig/fs/compat.c	2005-01-06 11:40:18.831900000 +0100
+++ linux-2.6.10-mm2/fs/compat.c	2005-01-06 16:36:17.340977664 +0100
@@ -436,14 +436,15 @@
 	if (!filp)
 		goto out;
 
-	if (!filp->f_op) {
-		if (!filp->f_op->ioctl)
-			goto do_ioctl;
-	} else if (filp->f_op->compat_ioctl) {
+	if (filp->f_op && filp->f_op->compat_ioctl) {
 		error = filp->f_op->compat_ioctl(filp, cmd, arg);
 		goto out_fput;
 	}
 
+	if (!filp->f_op ||
+	    (!filp->f_op->ioctl && !filp->f_op->unlocked_ioctl))
+		goto do_ioctl;
+
 	down_read(&ioctl32_sem);
 	for (t = ioctl32_hash_table[ioctl32_hash(cmd)]; t; t = t->next) {
 		if (t->cmd == cmd)

> 
> 
> MST
---end quoted text---

Re: [PATCH] deprecate (un)register_ioctl32_conversion

[Michael S. Tsirkin]

Hello!
Quoting r. Christoph Hellwig (hch@infradead.org) "Re: [PATCH] deprecate (un)register_ioctl32_conversion":
> On Thu, Jan 06, 2005 at 05:22:48PM +0200, Michael S. Tsirkin wrote:
> > > +	if (!filp->f_op || !filp->f_op->ioctl)
> > > +		goto do_ioctl;
> > > +
> > > +	if (filp->f_op || filp->f_op->compat_ioctl) {
> > >  		error = filp->f_op->compat_ioctl(filp, cmd, arg);
> > >  		goto out_fput;
> > >  	}
> > 
> > So now if I dont have ->ioctl the ioctl_compat wont be called.
> > What if I only have unlocked_ioctl?
> 
> Indeed.  In my test setup I didn't have a driver using both.  So let's
> think a little more what checks we want.
> 
> The original intention (pre-patch) was that without an ioctl entry
> we'd skip the hash table lookup and skip right to trying the few standard
> ioctls.
> 
> So with ->compat_ioctl we should try that one first, then checking
> for either ->ioctl or ->unlocked_ioctl beeing there.  Like the patch
> below (this time it's actually untested because all my 64bit machines
> are in use):
> 
> 
> --- linux-2.6.10-mm2.orig/fs/compat.c	2005-01-06 11:40:18.831900000 +0100
> +++ linux-2.6.10-mm2/fs/compat.c	2005-01-06 16:36:17.340977664 +0100
> @@ -436,14 +436,15 @@
>  	if (!filp)
>  		goto out;
>  
> -	if (!filp->f_op) {
> -		if (!filp->f_op->ioctl)
> -			goto do_ioctl;
> -	} else if (filp->f_op->compat_ioctl) {
> +	if (filp->f_op && filp->f_op->compat_ioctl) {
>  		error = filp->f_op->compat_ioctl(filp, cmd, arg);
>  		goto out_fput;
>  	}
>  
> +	if (!filp->f_op ||
> +	    (!filp->f_op->ioctl && !filp->f_op->unlocked_ioctl))
> +		goto do_ioctl;
> +
>  	down_read(&ioctl32_sem);
>  	for (t = ioctl32_hash_table[ioctl32_hash(cmd)]; t; t = t->next) {
>  		if (t->cmd == cmd)
> 
> > 
> > 
> > MST
> ---end quoted text---


Amen to that. Since it conflicts with this change, here again is my patch
to make it possible for the compat_ioctl to drop back on the standard
conversions.
Applied on top of Christoph's last patch (above).

Signed-off-by: Michael S. Tsirkn <mst@mellanox.co.il>

diff -rup linux-2.6.10-mm2/fs/compat.c linux-2.6.10-ioctls/fs/compat.c
--- linux-2.6.10-mm2/fs/compat.c	2005-01-06 21:30:57.485167280 +0200
+++ linux-2.6.10-ioctls/fs/compat.c	2005-01-06 21:33:17.608865240 +0200
@@ -431,9 +431,10 @@ asmlinkage long compat_sys_ioctl(unsigne
 	if (!filp)
 		goto out;
 
 	if (filp->f_op && filp->f_op->compat_ioctl) {
 		error = filp->f_op->compat_ioctl(filp, cmd, arg);
-		goto out_fput;
+		if (error != -ENOIOCTLCMD)
+			goto out_fput;
 	}
 
 	if (!filp->f_op ||
diff -rup linux-2.6.10-mm2/fs/ioctl.c linux-2.6.10-ioctls/fs/ioctl.c
--- linux-2.6.10-mm2/fs/ioctl.c	2005-01-06 17:54:13.000000000 +0200
+++ linux-2.6.10-ioctls/fs/ioctl.c	2005-01-06 20:34:09.329285728 +0200
@@ -26,6 +26,9 @@ static long do_ioctl(struct file *filp, 
 
 	if (filp->f_op->unlocked_ioctl) {
 		error = filp->f_op->unlocked_ioctl(filp, cmd, arg);
+		if (error == -ENOIOCTLCMD)
+			error = -EINVAL;
+		goto out;
 	} else if (filp->f_op->ioctl) {
 		lock_kernel();
 		error = filp->f_op->ioctl(filp->f_dentry->d_inode,

Re: [PATCH] deprecate (un)register_ioctl32_conversion

[Andi Kleen]

Christoph Hellwig <hch@infradead.org> writes:

> On Wed, Jan 05, 2005 at 04:40:43PM +0200, Michael S. Tsirkin wrote:
>> Hello, Andrew, all!
>> 
>> Since in -mm1 we now have a race-free replacement (that being ioctl_compat),
>> here is a patch to deprecate (un)register_ioctl32_conversion.
>
> Sorry, but this is a lot too early.  Once there's a handfull users left
> in _mainline_ you can start deprecating it (or better remove it completely).

There were never more than a handful users of it anyways. So I think
Michael's suggestion to deprecate it early is very reasonable.

> So far we have a non-final version of the replacement in -mm and no single
> user converted.

For me it looks quite final.

-Andi

Re: [PATCH] deprecate (un)register_ioctl32_conversion

[Christoph Hellwig]

On Wed, Jan 05, 2005 at 04:19:16PM +0100, Andi Kleen wrote:
> Christoph Hellwig <hch@infradead.org> writes:
> 
> > On Wed, Jan 05, 2005 at 04:40:43PM +0200, Michael S. Tsirkin wrote:
> >> Hello, Andrew, all!
> >> 
> >> Since in -mm1 we now have a race-free replacement (that being ioctl_compat),
> >> here is a patch to deprecate (un)register_ioctl32_conversion.
> >
> > Sorry, but this is a lot too early.  Once there's a handfull users left
> > in _mainline_ you can start deprecating it (or better remove it completely).
> 
> There were never more than a handful users of it anyways. So I think
> Michael's suggestion to deprecate it early is very reasonable.

It's 72 callers in mainline.  Aka 72 new warnings for an allmodconfig
build.  This isn't reasonable just because the interface got out of
fashion.

Re: [PATCH] deprecate (un)register_ioctl32_conversion

[Takashi Iwai]

At Wed, 5 Jan 2005 16:40:43 +0200,
Michael S. Tsirkin wrote:
> 
> Hello, Andrew, all!
> 
> Since in -mm1 we now have a race-free replacement (that being ioctl_compat),
> here is a patch to deprecate (un)register_ioctl32_conversion.

Good to see that ioctl_native and ioctl_compat ops are already there!

Will it be merged to 2.6.11?  If so, I'll prepare the patch to convert
the ALSA compat32 stuff, too...


Takashi


-------------------------------------------------------
The SF.Net email is sponsored by: Beat the post-holiday blues
Get a FREE limited edition SourceForge.net t-shirt from ThinkGeek.
It's fun and FREE -- well, almost....http://www.thinkgeek.com/sfshirt

Re: [PATCH] deprecate (un)register_ioctl32_conversion

[Andrew Morton]

Takashi Iwai <tiwai@suse.de> wrote:
>
> At Wed, 5 Jan 2005 16:40:43 +0200,
>  Michael S. Tsirkin wrote:
>  > 
>  > Hello, Andrew, all!
>  > 
>  > Since in -mm1 we now have a race-free replacement (that being ioctl_compat),
>  > here is a patch to deprecate (un)register_ioctl32_conversion.
> 
>  Good to see that ioctl_native and ioctl_compat ops are already there!
> 
>  Will it be merged to 2.6.11?

It should be, unless there's some problem.  In maybe a week or so.

[PATCH] macros to detect existance of unlocked_ioctl and ioctl_compat

[Michael S. Tsirkin]

Hello!
Quoting r. Andrew Morton (akpm@osdl.org) "Re: [PATCH] deprecate (un)register_ioctl32_conversion":
> Takashi Iwai <tiwai@suse.de> wrote:
> >
> > At Wed, 5 Jan 2005 16:40:43 +0200,
> >  Michael S. Tsirkin wrote:
> >  > 
> >  > Hello, Andrew, all!
> >  > 
> >  > Since in -mm1 we now have a race-free replacement (that being ioctl_compat),
> >  > here is a patch to deprecate (un)register_ioctl32_conversion.
> > 
> >  Good to see that ioctl_native and ioctl_compat ops are already there!
> > 
> >  Will it be merged to 2.6.11?
> 
> It should be, unless there's some problem.  In maybe a week or so.

To make life bearable for out-of kernel modules, the following patch
adds 2 macros so that existance of unlocked_ioctl and ioctl_compat
can be easily detected.

Signed-off-by: Michael S. Tsirkin <mst@mellanox.co.il>

diff -puN include/linux/fs.h~ioctl-rework include/linux/fs.h
--- 25/include/linux/fs.h~ioctl-rework	Thu Dec 16 15:48:31 2004
+++ 25-akpm/include/linux/fs.h	Thu Dec 16 15:48:31 2004
@@ -907,6 +907,12 @@ typedef struct {
 
 typedef int (*read_actor_t)(read_descriptor_t *, struct page *, unsigned long, unsigned long);
 
+/* These macros are for out of kernel modules to test that
+ * the kernel supports the unlocked_ioctl and ioctl_compat
+ * fields in struct file_operations. */
+#define HAVE_IOCTL_COMPAT 1
+#define HAVE_UNLOCKED_IOCTL 1
+
 /*
  * NOTE:
  * read, write, poll, fsync, readv, writev can be called

Re: [PATCH] macros to detect existance of unlocked_ioctl and ioctl_compat

[Christoph Hellwig]

On Thu, Jan 06, 2005 at 04:06:36PM +0200, Michael S. Tsirkin wrote:
> > It should be, unless there's some problem.  In maybe a week or so.
> 
> To make life bearable for out-of kernel modules, the following patch
> adds 2 macros so that existance of unlocked_ioctl and ioctl_compat
> can be easily detected.

That's not the way we're making additions.  Get your code merged and
there won't be any need to detect the feature.

Re: [PATCH] macros to detect existance of unlocked_ioctl and ioctl_compat

[Andi Kleen]

On Thu, Jan 06, 2005 at 02:53:56PM +0000, Christoph Hellwig wrote:
> On Thu, Jan 06, 2005 at 04:06:36PM +0200, Michael S. Tsirkin wrote:
> > > It should be, unless there's some problem.  In maybe a week or so.
> > 
> > To make life bearable for out-of kernel modules, the following patch
> > adds 2 macros so that existance of unlocked_ioctl and ioctl_compat
> > can be easily detected.
> 
> That's not the way we're making additions.  Get your code merged and
> there won't be any need to detect the feature.

I would agree that it shouldn't be used for in tree code, but for
out of tree code it is rather useful. There are other such feature macros
for major driver interface changes too (e.g. in the network stack).

-Andi

Re: [PATCH] macros to detect existance of unlocked_ioctl and ioctl_compat

[Lee Revell]

On Thu, 2005-01-06 at 15:14 +0000, Christoph Hellwig wrote:
> On Thu, Jan 06, 2005 at 04:09:42PM +0100, Andi Kleen wrote:
> > I would agree that it shouldn't be used for in tree code, but for
> > out of tree code it is rather useful. There are other such feature macros
> > for major driver interface changes too (e.g. in the network stack).
> 
> Which is the only place doing it.  We had this discussion in the past
> (lastone I revolve Greg vetoed it).  We simply shouldn't clutter our
> headers for the sake of out of tree drivers - with LINUX_VERSION_CODE
> we've already implemented a mechanism for out of tree drivers.
> 
> p.s. droppe alsa-devel from Cc because of the braindead moderation policy.
> 

Um, alsa-devel is not moderated, we accept posts from non subscribers.
Where do you think all the spam in our archive comes from?

Lee

new home for alsa-devel archive? (was Re: [PATCH] macros to detect existance of unlocked_ioctl and ioctl_compat)

[Lee Revell]

[re-added alsa-devel to cc:]

On Thu, 2005-01-06 at 15:43 +0000, Christoph Hellwig wrote:
> On Thu, Jan 06, 2005 at 10:38:58AM -0500, Lee Revell wrote:
> > The only working up to date archive I know of is the sourceforge one,
> > not my favorite interface:
> > 
> > http://sourceforge.net/mailarchive/forum.php?forum_id=1752
> 
> Indeed, I'd even say it's horrible.  Maybe Alsa folks could ask whether
> http://marc.theaimsgroup.com/ could host the new alsa lists?  They only
> have the dead really old list currently.
> 

This would be great.  I guess the first step is to find someone with a
local copy of the archive, in mbox format, preferably despammed.  I only
have back to June.  Anyone?

Lee



-------------------------------------------------------
The SF.Net email is sponsored by: Beat the post-holiday blues
Get a FREE limited edition SourceForge.net t-shirt from ThinkGeek.
It's fun and FREE -- well, almost....http://www.thinkgeek.com/sfshirt

Re: new home for alsa-devel archive?

[Måns Rullgård]

Lee Revell <rlrevell@joe-job.com> writes:

> [re-added alsa-devel to cc:]
>
> On Thu, 2005-01-06 at 15:43 +0000, Christoph Hellwig wrote:
>> On Thu, Jan 06, 2005 at 10:38:58AM -0500, Lee Revell wrote:
>> > The only working up to date archive I know of is the sourceforge one,
>> > not my favorite interface:
>> > 
>> > http://sourceforge.net/mailarchive/forum.php?forum_id=1752
>> 
>> Indeed, I'd even say it's horrible.  Maybe Alsa folks could ask whether
>> http://marc.theaimsgroup.com/ could host the new alsa lists?  They only
>> have the dead really old list currently.
>> 
>
> This would be great.  I guess the first step is to find someone with a
> local copy of the archive, in mbox format, preferably despammed.  I only
> have back to June.  Anyone?

There are archives at gmane, too.  Not perfect but infinitely better
than sf.net.

-- 
Måns Rullgård
mru@inprovide.com



-------------------------------------------------------
The SF.Net email is sponsored by: Beat the post-holiday blues
Get a FREE limited edition SourceForge.net t-shirt from ThinkGeek.
It's fun and FREE -- well, almost....http://www.thinkgeek.com/sfshirt

Re: Re: new home for alsa-devel archive?

[Eric Dantan Rzewnicki]

Måns Rullgård wrote:
> Lee Revell <rlrevell@joe-job.com> writes:
>>[re-added alsa-devel to cc:]
>>On Thu, 2005-01-06 at 15:43 +0000, Christoph Hellwig wrote:
>>>On Thu, Jan 06, 2005 at 10:38:58AM -0500, Lee Revell wrote:
>>>>The only working up to date archive I know of is the sourceforge one,
>>>>not my favorite interface:
>>>>http://sourceforge.net/mailarchive/forum.php?forum_id=1752
>>>Indeed, I'd even say it's horrible.  Maybe Alsa folks could ask whether
>>>http://marc.theaimsgroup.com/ could host the new alsa lists?  They only
>>>have the dead really old list currently.
>>This would be great.  I guess the first step is to find someone with a
>>local copy of the archive, in mbox format, preferably despammed.  I only
>>have back to June.  Anyone?
> There are archives at gmane, too.  Not perfect but infinitely better
> than sf.net.

Sorry for jumping in late on this thread. I missed the beginning of it.

I'm not sure if anyone has expressed an opinion about mailman, but if 
the community is open to it I can offer techweb.rfa.org as a host for 
the lists and archives. We already host the portaudio list.

http://techweb.rfa.org/mailman/listinfo

Please CC me on any reply. I rarely have time to read alsa lists lately 
and might miss it otherwise.

-Eric Rz.


-------------------------------------------------------
The SF.Net email is sponsored by: Beat the post-holiday blues
Get a FREE limited edition SourceForge.net t-shirt from ThinkGeek.
It's fun and FREE -- well, almost....http://www.thinkgeek.com/sfshirt

Re: [PATCH] macros to detect existance of unlocked_ioctl and ioctl_compat

[Petr Vandrovec]

On Thu, Jan 06, 2005 at 02:53:56PM +0000, Christoph Hellwig wrote:
> On Thu, Jan 06, 2005 at 04:06:36PM +0200, Michael S. Tsirkin wrote:
> > > It should be, unless there's some problem.  In maybe a week or so.
> > 
> > To make life bearable for out-of kernel modules, the following patch
> > adds 2 macros so that existance of unlocked_ioctl and ioctl_compat
> > can be easily detected.
> 
> That's not the way we're making additions.  Get your code merged and
> there won't be any need to detect the feature.

When Greg made sysfs GPL only, I've asked whether it is possible to merge
vmmon/vmnet in (and changing its license, of course).  Answer on LKML was 
quite clear: no, you are not interested in having vmmon/vmnet in Linux 
kernel as you do not believe that they are usable for anything else than VMware.  

So please tell me what I can do to satisfy you?  You do not want our modules
in kernel tree, and you do not want to allow us to detect kernel interface
easily.  Of course we can use autoconf-like scripts we are using for
example to detect pml4/pgd vs. pgd/pud vs. pgd/pmd/pte vs. pmd/pte only,
but each time you can detect feature by looking at flags and not by trying
to build one source after another detection is simpler and more reliable.

BTW, vmmon will still require register_ioctl32_conversion as we are using
(abusing) it to be able to issue 64bit ioctls from 32bit application.  I
assume that there is no supported way how to issue 64bit ioctls from 32bit
aplication anymore after you disallow system-wide translations to be registered
by modules.

						Best regards,
							Petr Vandrovec

Re: [PATCH] macros to detect existance of unlocked_ioctl and ioctl_compat

[Christoph Hellwig]

On Thu, Jan 06, 2005 at 05:35:59PM +0100, Petr Vandrovec wrote:
> When Greg made sysfs GPL only, I've asked whether it is possible to merge
> vmmon/vmnet in (and changing its license, of course).  Answer on LKML was 
> quite clear: no, you are not interested in having vmmon/vmnet in Linux 
> kernel as you do not believe that they are usable for anything else than VMware.  

While I think there could be users, these users would probably come only
after the code was GPLed, so this is kinda chicken & egg.

> So please tell me what I can do to satisfy you?  You do not want our modules
> in kernel tree, and you do not want to allow us to detect kernel interface
> easily.  Of course we can use autoconf-like scripts we are using for
> example to detect pml4/pgd vs. pgd/pud vs. pgd/pmd/pte vs. pmd/pte only,
> but each time you can detect feature by looking at flags and not by trying
> to build one source after another detection is simpler and more reliable.

I don't care at all for non-opensource users, or small opensource glue for
a big propritary user.

> BTW, vmmon will still require register_ioctl32_conversion as we are using
> (abusing) it to be able to issue 64bit ioctls from 32bit application.  I
> assume that there is no supported way how to issue 64bit ioctls from 32bit
> aplication anymore after you disallow system-wide translations to be registered
> by modules.

Well, bad luck.  You'll have to stop using undocumented hacks.

Re: [PATCH] macros to detect existance of unlocked_ioctl and ioctl_compat

[Andi Kleen]

On Thu, Jan 06, 2005 at 05:35:59PM +0100, Petr Vandrovec wrote:
> BTW, vmmon will still require register_ioctl32_conversion as we are using
> (abusing) it to be able to issue 64bit ioctls from 32bit application.  I
> assume that there is no supported way how to issue 64bit ioctls from 32bit
> aplication anymore after you disallow system-wide translations to be registered
> by modules.

Why are you issuing 64bit ioctls from 32bit applications? 

-Andi

Re: [PATCH] macros to detect existance of unlocked_ioctl and ioctl_compat

[Petr Vandrovec]

On Thu, Jan 06, 2005 at 05:57:15PM +0100, Andi Kleen wrote:
> On Thu, Jan 06, 2005 at 05:35:59PM +0100, Petr Vandrovec wrote:
> > BTW, vmmon will still require register_ioctl32_conversion as we are using
> > (abusing) it to be able to issue 64bit ioctls from 32bit application.  I
> > assume that there is no supported way how to issue 64bit ioctls from 32bit
> > aplication anymore after you disallow system-wide translations to be registered
> > by modules.
> 
> Why are you issuing 64bit ioctls from 32bit applications? 

There are three reasons (main reason is that vmware is 32bit app, but it is how
things are currently laid out; even if there will be 64bit app, 32bit versions are
already in use and people wants to use them on 64bit kernels):

* USB.  usbfs API is written in a way which does not allow you to safely wrap
it in "generic" 32->64 layer, and attempts to do it in non-generic way in usbfs
code itself did not look feasible year ago.  Even on current 64bit kernels it is not
possible to issue raw USB operations from 32bit apps, and I do not believe that
it is going to change - after all, just issuing ioctl through 64bit path from application
which is aware of 64bit kernel is quite simple, much simpler than any attempt to
make kernel dual-interface.

* parport interface, serial port:  not all APIs were implemented in kernel.  Current
kernels do wrap all APIs vmware needs, but older kernels do not, and so we had to find
some solution for older kernels too.  Not so surprisingly, solution we had in place for
USB works for them too...

* floppy:  it is actually different from examples above, as FDRAWCMD command is
supported by 32->64 layer, but it is supported incorrectly.  Due to this all above
started, as we had to make application aware of kernel it runs on, as FDRAWCMD on 32bit
kernel returns 80 byte structure, while 104 byte on 64bit kernel, and you do not now
which one you'll get until you call this ioctl...  And once we had code in place,
it was reused for USB and later for ppdev & serial.

So we added simple wrapper to vmmon which just gets {64bit-ioctl-number, 64bit-arg-argument}
and passes it down to 64bit sys_ioctl:

/* Use weak: not all kernels export sys_ioctl for use by modules */
#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 5, 66)
asmlinkage __attribute__((weak)) long
sys_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg);
#else
asmlinkage __attribute__((weak)) int
sys_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg);
#endif

...

static int
LinuxDriver_Ioctl3264Passthrough(unsigned int fd, unsigned int iocmd,
                                 unsigned long ioarg, struct file * filp)
{
   VMIoctl64 cmd;

   if (copy_from_user(&cmd, (VMIoctl64*)ioarg, sizeof(cmd))) {
      return -EFAULT;
   }
   if (sys_ioctl) {
      int err;

#if LINUX_VERSION_CODE < KERNEL_VERSION(2, 4, 26) || \
   (LINUX_VERSION_CODE >= KERNEL_VERSION(2, 5, 0) && LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 3))
      err = sys_ioctl(fd, cmd.iocmd, cmd.ioarg);
#else
      unlock_kernel();
      err = sys_ioctl(fd, cmd.iocmd, cmd.ioarg);
      lock_kernel();
#endif
      return err;
   }
   return -ENOTTY;
}


We were thinking about creating 64bit thread, and issuing 64bit syscalls from it, but 
it looked more fragile than code above.
								Best regards,
									Petr Vandrovec

Re: [PATCH] macros to detect existance of unlocked_ioctl and ioctl_compat

[Pavel Machek]

Hi!

> > > > It should be, unless there's some problem.  In maybe a week or so.
> > > 
> > > To make life bearable for out-of kernel modules, the following patch
> > > adds 2 macros so that existance of unlocked_ioctl and ioctl_compat
> > > can be easily detected.
> > 
> > That's not the way we're making additions.  Get your code merged and
> > there won't be any need to detect the feature.
> 
> When Greg made sysfs GPL only, I've asked whether it is possible to merge
> vmmon/vmnet in (and changing its license, of course).  Answer on LKML was 
> quite clear: no, you are not interested in having vmmon/vmnet in Linux 
> kernel as you do not believe that they are usable for anything else
> than VMware.  

What about

1) Put vmmon/vmnet under GPL

2) Find some GPL user of vmmon/vmnet

3) Merge should be doable now

								Pavel

-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

[PATCH] fix: macros to detect existance of unlocked_ioctl and compat_ioctl

[Michael S. Tsirkin]

Hi!
I just noticed that my original patch says "ioctl_compat" all over the place
while the actual field name in -mm2 is "compat_ioctl". Doh.
Here's a replacement patch.
PS. Please dont flame me, I do not maintain out of kernel modules, myself :)

To make life bearable for out-of kernel modules, the following patch
adds 2 macros so that existance of unlocked_ioctl and compat_ioctl
can be easily detected.
 
Signed-off-by: Michael S. Tsirkin <mst@mellanox.co.il>

diff -puN include/linux/fs.h~ioctl-rework include/linux/fs.h
--- 25/include/linux/fs.h~ioctl-rework	Thu Dec 16 15:48:31 2004
+++ 25-akpm/include/linux/fs.h	Thu Dec 16 15:48:31 2004
@@ -907,6 +907,12 @@ typedef struct {
 
 typedef int (*read_actor_t)(read_descriptor_t *, struct page *, unsigned long, unsigned long);
 
+/* These macros are for out of kernel modules to test that
+ * the kernel supports the unlocked_ioctl and compat_ioctl
+ * fields in struct file_operations. */
+#define HAVE_COMPAT_IOCTL 1
+#define HAVE_UNLOCKED_IOCTL 1
+
 /*
  * NOTE:
  * read, write, poll, fsync, readv, writev can be called

Re: [PATCH] fix: macros to detect existance of unlocked_ioctl and compat_ioctl

[Greg KH]

On Wed, Jan 12, 2005 at 10:36:06PM +0200, Michael S. Tsirkin wrote:
> To make life bearable for out-of kernel modules, the following patch
> adds 2 macros so that existance of unlocked_ioctl and compat_ioctl
> can be easily detected.
>  
> Signed-off-by: Michael S. Tsirkin <mst@mellanox.co.il>
> 
> diff -puN include/linux/fs.h~ioctl-rework include/linux/fs.h
> --- 25/include/linux/fs.h~ioctl-rework	Thu Dec 16 15:48:31 2004
> +++ 25-akpm/include/linux/fs.h	Thu Dec 16 15:48:31 2004
> @@ -907,6 +907,12 @@ typedef struct {
>  
>  typedef int (*read_actor_t)(read_descriptor_t *, struct page *, unsigned long, unsigned long);
>  
> +/* These macros are for out of kernel modules to test that
> + * the kernel supports the unlocked_ioctl and compat_ioctl
> + * fields in struct file_operations. */
> +#define HAVE_COMPAT_IOCTL 1
> +#define HAVE_UNLOCKED_IOCTL 1

No, we do not do that in the kernel today, and I'm pretty sure we don't
want to start doing it (it would get _huge_ very quickly...)

Please don't apply this.  Remember, out-of-the-tree modules are on their
own.

thanks,

greg k-h

Re: [PATCH] fix: macros to detect existance of unlocked_ioctl and compat_ioctl

[Andi Kleen]

> No, we do not do that in the kernel today, and I'm pretty sure we don't

Actually we do. e.g. take a look at skbuff.h HAVE_*
There are other examples too.

> want to start doing it (it would get _huge_ very quickly...)

I disagree since the alternative is so ugly.


> Please don't apply this.  Remember, out-of-the-tree modules are on their
> own.

I don't know who made this "policy", but actively sabotating or denying 
useful facilities to free out of tree modules doesn't seem to be a 
very wise policy to me.

-Andi

Re: [PATCH] fix: macros to detect existance of unlocked_ioctl and compat_ioctl

[Greg KH]

On Wed, Jan 12, 2005 at 10:43:26PM +0100, Andi Kleen wrote:
> > No, we do not do that in the kernel today, and I'm pretty sure we don't
> 
> Actually we do. e.g. take a look at skbuff.h HAVE_*
> There are other examples too.
> 
> > want to start doing it (it would get _huge_ very quickly...)
> 
> I disagree since the alternative is so ugly.

But the main problem with this is, when do we start deleting the HAVE_
symbols?  After a relativly short period of time, they become useless,
as all kernels of the past year or two have that feature, and why even
test for it?

I agree, that for short term, vendor-patched kernels, it's nice to have
something like that to try to build your out-of-the-tree module.  But
move to get that module into the tree, or provide your favorite vendor
with the properly patched driver (hey, I can dream...)

And is the alternative (using autoconf to build tiny modules testing for
different features) that ugly?  Yeah, I hate autoconf too, but I thought
that work (kernel feature testing in autoconf) has already been done,
right?

> > Please don't apply this.  Remember, out-of-the-tree modules are on their
> > own.
> 
> I don't know who made this "policy", but actively sabotating or denying 
> useful facilities to free out of tree modules doesn't seem to be a 
> very wise policy to me.

I'm not trying to sabotage anything, I just don't want the maintaince of
a zillion HAVE_ macros to slowly overrun us until we drown under the
weight.  All to support some looney, ill-informed vendor that can never
get around to submitting their driver to mainline.

And as for that "policy", it's been stated in public by Andrew and
Linus and me (if I count for anything, doubtful...) a number of
documented times.

thanks,

greg k-h

Re: [PATCH] fix: macros to detect existance of unlocked_ioctl and compat_ioctl

[Andrew Morton]

Greg KH <greg@kroah.com> wrote:
>
> On Wed, Jan 12, 2005 at 10:43:26PM +0100, Andi Kleen wrote:
> > > No, we do not do that in the kernel today, and I'm pretty sure we don't
> > 
> > Actually we do. e.g. take a look at skbuff.h HAVE_*
> > There are other examples too.
> > 
> > > want to start doing it (it would get _huge_ very quickly...)
> > 
> > I disagree since the alternative is so ugly.
> 
> But the main problem with this is, when do we start deleting the HAVE_
> symbols?

This is a self-correcting system.  If the symbols are so offensive, someone
will get offended and will submit a patch to delete them at the appropriate
time.  If they're not so offensive then we've nothing to care about.

> ...
> And as for that "policy", it's been stated in public by Andrew and
> Linus and me (if I count for anything, doubtful...) a number of
> documented times.

not me ;) It's two lines of code and makes things much simpler for the
users of our work.  Seems a no-brainer.

And practically speaking, we don't make such fundamental driver-visible
changes _that_ often - if we end up getting buried under a proliferation of
HAVE_FOO macros, then the presence of the macros is the least of our
problems, yes?

Re: [PATCH] fix: macros to detect existance of unlocked_ioctl and compat_ioctl

[Greg KH]

On Wed, Jan 12, 2005 at 03:10:49PM -0800, Andrew Morton wrote:
> Greg KH <greg@kroah.com> wrote:
> > ...
> > And as for that "policy", it's been stated in public by Andrew and
> > Linus and me (if I count for anything, doubtful...) a number of
> > documented times.
> 
> not me ;) It's two lines of code and makes things much simpler for the
> users of our work.  Seems a no-brainer.

Sorry, the "policy" I was referring to was the "out-of-the-tree drivers
are on their own" statement.  Not the use of the HAVE macros.

> And practically speaking, we don't make such fundamental driver-visible
> changes _that_ often - if we end up getting buried under a proliferation of
> HAVE_FOO macros, then the presence of the macros is the least of our
> problems, yes?

Ok, but can someone add a section in the feature-removal-schedule.txt
file about when these specific macros will be removed?  They must be
created with some specific use in mind, right?

thanks,

greg k-h

security hook missing in compat ioctl in 2.6.11-rc1-mm2

[Michael S. Tsirkin]

Hi!
Security hook seems to be missing before compat_ioctl in mm2.
And, it would be nice to avoid calling it twice on some paths.

Chris Wright's patch addressed this in the most elegant way I think,
by adding vfs_ioctl.

Accordingly, this change:

@@ -468,6 +496,11 @@ asmlinkage long compat_sys_ioctl(unsigne
 
  found_handler:
 	if (t->handler) {
+		/* RED-PEN how should LSM module know it's handling 32bit? */
+		error = security_file_ioctl(filp, cmd, arg);
+		if (error)
+			goto out_fput;
+
 		lock_kernel();
 		error = t->handler(fd, cmd, arg, filp);
 		unlock_kernel();

 from Andy's "some fixes" patch wont be needed.

Chris - are you planning to update your patch to -rc1-mm2?
I'd like to see this addressed, after this I believe logically
we'll get everything right, then I have a couple of small
cosmetic patches, and I believe we'll be set.

-- 
I dont speak for Mellanox.

Re: security hook missing in compat ioctl in 2.6.11-rc1-mm2

[Chris Wright]

* Michael S. Tsirkin (mst@mellanox.co.il) wrote:
> Hi!
> Security hook seems to be missing before compat_ioctl in mm2.
> And, it would be nice to avoid calling it twice on some paths.
> 
> Chris Wright's patch addressed this in the most elegant way I think,
> by adding vfs_ioctl.
> 
> Accordingly, this change:
> 
> @@ -468,6 +496,11 @@ asmlinkage long compat_sys_ioctl(unsigne
>  
>   found_handler:
>  	if (t->handler) {
> +		/* RED-PEN how should LSM module know it's handling 32bit? */
> +		error = security_file_ioctl(filp, cmd, arg);
> +		if (error)
> +			goto out_fput;
> +
>  		lock_kernel();
>  		error = t->handler(fd, cmd, arg, filp);
>  		unlock_kernel();
> 
>  from Andy's "some fixes" patch wont be needed.
> 
> Chris - are you planning to update your patch to -rc1-mm2?
> I'd like to see this addressed, after this I believe logically
> we'll get everything right, then I have a couple of small
> cosmetic patches, and I believe we'll be set.

Yes, Andrew asked me to wait until mm2 came out, so I'll rediff and send
shortly.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

[PATCH] compat ioctl security hook fixup

[Chris Wright]

* Michael S. Tsirkin (mst@mellanox.co.il) wrote:
> Security hook seems to be missing before compat_ioctl in mm2.
> And, it would be nice to avoid calling it twice on some paths.
> 
> Chris Wright's patch addressed this in the most elegant way I think,
> by adding vfs_ioctl.

The patch below is against Linus' tree as per Andrew's request.  It will
conflict with some of the changes in -mm2 (including the some-fixes bit
from Andi, and LTT).  I also have a patch directly against -mm2 if anyone
would like to see that instead.

thanks,
-chris
--

Introduce a simple helper, vfs_ioctl(), so that both sys_ioctl() and
compat_sys_ioctl() call the security hook in all cases and without
duplication.

Signed-off-by: Chris Wright <chrisw@osdl.org>

===== fs/ioctl.c 1.15 vs edited =====
--- 1.15/fs/ioctl.c	2005-01-15 14:31:01 -08:00
+++ edited/fs/ioctl.c	2005-01-18 11:18:33 -08:00
@@ -77,21 +77,10 @@ static int file_ioctl(struct file *filp,
 	return do_ioctl(filp, cmd, arg);
 }
 
-
-asmlinkage long sys_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg)
+int vfs_ioctl(struct file *filp, unsigned int fd, unsigned int cmd, unsigned long arg)
 {
-	struct file * filp;
 	unsigned int flag;
-	int on, error = -EBADF;
-	int fput_needed;
-
-	filp = fget_light(fd, &fput_needed);
-	if (!filp)
-		goto out;
-
-	error = security_file_ioctl(filp, cmd, arg);
-	if (error)
-		goto out_fput;
+	int on, error = 0;
 
 	switch (cmd) {
 		case FIOCLEX:
@@ -157,6 +146,24 @@ asmlinkage long sys_ioctl(unsigned int f
 				error = do_ioctl(filp, cmd, arg);
 			break;
 	}
+	return error;
+}
+
+asmlinkage long sys_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg)
+{
+	struct file * filp;
+	int error = -EBADF;
+	int fput_needed;
+
+	filp = fget_light(fd, &fput_needed);
+	if (!filp)
+		goto out;
+
+	error = security_file_ioctl(filp, cmd, arg);
+	if (error)
+		goto out_fput;
+
+	error = vfs_ioctl(filp, fd, cmd, arg);
  out_fput:
 	fput_light(filp, fput_needed);
  out:
===== fs/compat.c 1.48 vs edited =====
--- 1.48/fs/compat.c	2005-01-15 14:31:01 -08:00
+++ edited/fs/compat.c	2005-01-18 11:07:56 -08:00
@@ -437,6 +437,11 @@ asmlinkage long compat_sys_ioctl(unsigne
 	if (!filp)
 		goto out;
 
+	/* RED-PEN how should LSM module know it's handling 32bit? */
+	error = security_file_ioctl(filp, cmd, arg);
+	if (error)
+		goto out_fput;
+
 	if (filp->f_op && filp->f_op->compat_ioctl) {
 		error = filp->f_op->compat_ioctl(filp, cmd, arg);
 		if (error != -ENOIOCTLCMD)
@@ -477,7 +482,7 @@ asmlinkage long compat_sys_ioctl(unsigne
 
 	up_read(&ioctl32_sem);
  do_ioctl:
-	error = sys_ioctl(fd, cmd, arg);
+	error = vfs_ioctl(filp, fd, cmd, arg);
  out_fput:
 	fput_light(filp, fput_needed);
  out:
===== include/linux/fs.h 1.373 vs edited =====
--- 1.373/include/linux/fs.h	2005-01-15 14:31:01 -08:00
+++ edited/include/linux/fs.h	2005-01-18 11:10:54 -08:00
@@ -1564,6 +1564,8 @@ extern int vfs_stat(char __user *, struc
 extern int vfs_lstat(char __user *, struct kstat *);
 extern int vfs_fstat(unsigned int, struct kstat *);
 
+extern int vfs_ioctl(struct file *, unsigned int, unsigned int, unsigned long);
+
 extern struct file_system_type *get_fs_type(const char *name);
 extern struct super_block *get_super(struct block_device *);
 extern struct super_block *user_get_super(dev_t);

Re: [PATCH] compat ioctl security hook fixup

[Andi Kleen]

On Thu, Jan 20, 2005 at 05:26:56PM -0800, Chris Wright wrote:
> * Michael S. Tsirkin (mst@mellanox.co.il) wrote:
> > Security hook seems to be missing before compat_ioctl in mm2.
> > And, it would be nice to avoid calling it twice on some paths.
> > 
> > Chris Wright's patch addressed this in the most elegant way I think,
> > by adding vfs_ioctl.
> 
> The patch below is against Linus' tree as per Andrew's request.  It will
> conflict with some of the changes in -mm2 (including the some-fixes bit
> from Andi, and LTT).  I also have a patch directly against -mm2 if anyone
> would like to see that instead.

I'm not sure really adding vfs_ioctl is a good idea politically.
I predict we'll see drivers starting to use it, which will cause quite
broken design.

If you add it make at least sure it's not EXPORT_SYMBOL()ed.

-Andi

Re: [PATCH] compat ioctl security hook fixup

[Chris Wright]

* Andi Kleen (ak@suse.de) wrote:
> I'm not sure really adding vfs_ioctl is a good idea politically.
> I predict we'll see drivers starting to use it, which will cause quite
> broken design.

Yes, that'd be quite broken.  I didn't have the same expectation.

> If you add it make at least sure it's not EXPORT_SYMBOL()ed.

It's certainly not, nor intended to be.  Would a comment to that
affect alleviate your concern?

thanks,
-chris

Re: [PATCH] compat ioctl security hook fixup

[Andi Kleen]

On Thu, Jan 20, 2005 at 09:51:03PM -0800, Chris Wright wrote:
> > If you add it make at least sure it's not EXPORT_SYMBOL()ed.
> 
> It's certainly not, nor intended to be.  Would a comment to that
> affect alleviate your concern?

Yes please.

-Andi

[PATCH] compat ioctl security hook fixup (take2)

[Chris Wright]

* Andi Kleen (ak@suse.de) wrote:
> On Thu, Jan 20, 2005 at 09:51:03PM -0800, Chris Wright wrote:
> > > If you add it make at least sure it's not EXPORT_SYMBOL()ed.
> > 
> > It's certainly not, nor intended to be.  Would a comment to that
> > affect alleviate your concern?
> 
> Yes please.

Patch respun, with comment added.

thanks,
-chris
--

Introduce a simple helper, vfs_ioctl(), so that both sys_ioctl() and
compat_sys_ioctl() call the security hook in all cases and without
duplication.

Signed-off-by: Chris Wright <chrisw@osdl.org>

===== fs/ioctl.c 1.15 vs edited =====
--- 1.15/fs/ioctl.c	2005-01-15 14:31:01 -08:00
+++ edited/fs/ioctl.c	2005-01-20 22:27:43 -08:00
@@ -77,21 +77,13 @@ static int file_ioctl(struct file *filp,
 	return do_ioctl(filp, cmd, arg);
 }
 
-
-asmlinkage long sys_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg)
+/* Simple helper for sys_ioctl and compat_sys_ioctl.  Not for drivers'
+ * use, and not intended to be EXPORT_SYMBOL()'d
+ */
+int vfs_ioctl(struct file *filp, unsigned int fd, unsigned int cmd, unsigned long arg)
 {
-	struct file * filp;
 	unsigned int flag;
-	int on, error = -EBADF;
-	int fput_needed;
-
-	filp = fget_light(fd, &fput_needed);
-	if (!filp)
-		goto out;
-
-	error = security_file_ioctl(filp, cmd, arg);
-	if (error)
-		goto out_fput;
+	int on, error = 0;
 
 	switch (cmd) {
 		case FIOCLEX:
@@ -157,6 +149,24 @@ asmlinkage long sys_ioctl(unsigned int f
 				error = do_ioctl(filp, cmd, arg);
 			break;
 	}
+	return error;
+}
+
+asmlinkage long sys_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg)
+{
+	struct file * filp;
+	int error = -EBADF;
+	int fput_needed;
+
+	filp = fget_light(fd, &fput_needed);
+	if (!filp)
+		goto out;
+
+	error = security_file_ioctl(filp, cmd, arg);
+	if (error)
+		goto out_fput;
+
+	error = vfs_ioctl(filp, fd, cmd, arg);
  out_fput:
 	fput_light(filp, fput_needed);
  out:
===== fs/compat.c 1.48 vs edited =====
--- 1.48/fs/compat.c	2005-01-15 14:31:01 -08:00
+++ edited/fs/compat.c	2005-01-20 22:25:33 -08:00
@@ -437,6 +437,11 @@ asmlinkage long compat_sys_ioctl(unsigne
 	if (!filp)
 		goto out;
 
+	/* RED-PEN how should LSM module know it's handling 32bit? */
+	error = security_file_ioctl(filp, cmd, arg);
+	if (error)
+		goto out_fput;
+
 	if (filp->f_op && filp->f_op->compat_ioctl) {
 		error = filp->f_op->compat_ioctl(filp, cmd, arg);
 		if (error != -ENOIOCTLCMD)
@@ -477,7 +482,7 @@ asmlinkage long compat_sys_ioctl(unsigne
 
 	up_read(&ioctl32_sem);
  do_ioctl:
-	error = sys_ioctl(fd, cmd, arg);
+	error = vfs_ioctl(filp, fd, cmd, arg);
  out_fput:
 	fput_light(filp, fput_needed);
  out:
===== include/linux/fs.h 1.373 vs edited =====
--- 1.373/include/linux/fs.h	2005-01-15 14:31:01 -08:00
+++ edited/include/linux/fs.h	2005-01-20 22:25:33 -08:00
@@ -1564,6 +1564,8 @@ extern int vfs_stat(char __user *, struc
 extern int vfs_lstat(char __user *, struct kstat *);
 extern int vfs_fstat(unsigned int, struct kstat *);
 
+extern int vfs_ioctl(struct file *, unsigned int, unsigned int, unsigned long);
+
 extern struct file_system_type *get_fs_type(const char *name);
 extern struct super_block *get_super(struct block_device *);
 extern struct super_block *user_get_super(dev_t);

[PATCH] fget_light/fput_light for ioctls

[Michael S. Tsirkin]

Hello!
With new unlocked_ioctl and ioctl_compat, ioctls can now
be as fast as read/write.
So lets use fget_light/fput_light there, to get some speedup
in common case on SMP.

mst

Signed-off-by: Michael s. Tsirkin <mst@mellanox.co.il>

diff -rup linux-2.6.10/fs/compat.c linux-2.6.10-ioctls/fs/compat.c
--- linux-2.6.10/fs/compat.c	2005-01-06 17:54:13.000000000 +0200
+++ linux-2.6.10-ioctls/fs/compat.c	2005-01-06 20:15:44.407259408 +0200
@@ -431,8 +431,9 @@ asmlinkage long compat_sys_ioctl(unsigne
 	struct file *filp;
 	int error = -EBADF;
 	struct ioctl_trans *t;
+	int fput_needed;
 
-	filp = fget(fd);
+	filp = fget_light(fd, &fput_needed);
 	if (!filp)
 		goto out;
 
@@ -476,7 +479,7 @@ asmlinkage long compat_sys_ioctl(unsigne
  do_ioctl:
 	error = sys_ioctl(fd, cmd, arg);
  out_fput:
-	fput(filp);
+	fput_light(file, fput_needed);
  out:
 	return error;
 }
diff -rup linux-2.6.10/fs/ioctl.c linux-2.6.10-ioctls/fs/ioctl.c
--- linux-2.6.10/fs/ioctl.c	2005-01-06 17:54:13.000000000 +0200
+++ linux-2.6.10-ioctls/fs/ioctl.c	2005-01-06 20:34:09.329285728 +0200
@@ -80,8 +83,9 @@ asmlinkage long sys_ioctl(unsigned int f
 	struct file * filp;
 	unsigned int flag;
 	int on, error = -EBADF;
+	int fput_needed;
 
-	filp = fget(fd);
+	filp = fget_light(fd, &fput_needed);
 	if (!filp)
 		goto out;
 
@@ -154,7 +158,7 @@ asmlinkage long sys_ioctl(unsigned int f
 			break;
 	}
  out_fput:
-	fput(filp);
+	fput_light(filp, fput_needed);
  out:
 	return error;
 }

[PATCH] fget_light/fput_light for ioctls (fixed)

[Michael S. Tsirkin]

Hello!
Sorry, that patch had a typo. Here's an updated version.

>>> Quoting r. Michael S. Tsirkin (mst@mellanox.co.il)

With new unlocked_ioctl and ioctl_compat, ioctls can now
be as fast as read/write.  So lets use fget_light/fput_light there,
to get some speedup in common case on SMP.

mst

Signed-off-by: Michael s. Tsirkin <mst@mellanox.co.il>

diff -rup linux-2.6.10/fs/compat.c linux-2.6.10-ioctls/fs/compat.c
--- linux-2.6.10/fs/compat.c	2005-01-06 17:54:13.000000000 +0200
+++ linux-2.6.10-ioctls/fs/compat.c	2005-01-06 20:15:44.407259408 +0200
@@ -431,8 +431,9 @@ asmlinkage long compat_sys_ioctl(unsigne
 	struct file *filp;
 	int error = -EBADF;
 	struct ioctl_trans *t;
+	int fput_needed;
 
-	filp = fget(fd);
+	filp = fget_light(fd, &fput_needed);
 	if (!filp)
 		goto out;
 
@@ -476,7 +479,7 @@ asmlinkage long compat_sys_ioctl(unsigne
  do_ioctl:
 	error = sys_ioctl(fd, cmd, arg);
  out_fput:
-	fput(filp);
+	fput_light(filp, fput_needed);
  out:
 	return error;
 }
diff -rup linux-2.6.10/fs/ioctl.c linux-2.6.10-ioctls/fs/ioctl.c
--- linux-2.6.10/fs/ioctl.c	2005-01-06 17:54:13.000000000 +0200
+++ linux-2.6.10-ioctls/fs/ioctl.c	2005-01-06 20:34:09.329285728 +0200
@@ -80,8 +83,9 @@ asmlinkage long sys_ioctl(unsigned int f
 	struct file * filp;
 	unsigned int flag;
 	int on, error = -EBADF;
+	int fput_needed;
 
-	filp = fget(fd);
+	filp = fget_light(fd, &fput_needed);
 	if (!filp)
 		goto out;
 
@@ -154,7 +158,7 @@ asmlinkage long sys_ioctl(unsigned int f
 			break;
 	}
  out_fput:
-	fput(filp);
+	fput_light(filp, fput_needed);
  out:
 	return error;
 }