[PATCH -rt] race condition in fs/compat.c with compat_sys_ioctl

[PATCH -rt] race condition in fs/compat.c with compat_sys_ioctl

[Steven Rostedt]

Hi Ingo and friends.

Well I finally got my AMD64 x2 up and running and a cross compiler
working (is there a better way in Debian to get a AMD64 cross compiler
than building one from scratch?).

I got Thomas' kthrt running with no problem (my default kernel in
fact ;-)

Then I felt good and compiled -rt.  After a few problems (mostly
configuration) it booted and I got the gdm login.  "cool" I thought.

Well, as soon as I logged in, the system froze.  Well not really. I had
interrupts working, and the mouse moved around, but all the bash shells
would lock up immediately, as well as all the gnome-terminals ???

Well I solved it, and this might even be a problem with the vanilla
kernel.  Grant you, this is most susceptible with a dual AMD64 on RT.

Here's the problem: 

in fs/compat.c: compat_sys_ioctl

It has this hash table of ioctl commands protected with a ioctl32_sem.
To get an ioctl you must grab the sem and then search for your ioctl in
the hash table.  When you find it, you call your ioctl and then release
the sem. 

That's the problem. I found out that one ioctl might sleep holding the
sem and won't be woken up until another process calls another ioctl to
wake it up. But unfortunately, the one waking up the sleeper will block
on the sem.  (the killer was tty_wait_until_sent)

I don't know how this doesn't lock up the non-rt kernels.  I guess the
ioctls just don't sleep. I haven't looked too deep into why this works
outside of -rt, I just solved it for -rt and will look deeper into this
tomorrow.

Here's the patch:

I created a temporary variable to hold the function pointer when it
finds the ioctl to call, and then release the semaphore.  This is
definitely the solution for -rt, since I can't login without this patch.

-- Steve

PS. There's other bug messages I'm getting (in -rt), but I'll work on
those tomorrow ;-)

Index: linux-2.6.14-rt13/fs/compat.c
===================================================================
--- linux-2.6.14-rt13.orig/fs/compat.c	2005-10-27 20:02:08.000000000 -0400
+++ linux-2.6.14-rt13/fs/compat.c	2005-11-15 23:00:14.000000000 -0500
@@ -347,6 +347,7 @@
 	int error = -EBADF;
 	struct ioctl_trans *t;
 	int fput_needed;
+	ioctl_trans_handler_t handler = NULL;
 
 	filp = fget_light(fd, &fput_needed);
 	if (!filp)
@@ -394,8 +395,11 @@
 	   this lock! -AK */
 	down_read(&ioctl32_sem);
 	for (t = ioctl32_hash_table[ioctl32_hash(cmd)]; t; t = t->next) {
-		if (t->cmd == cmd)
+		if (t->cmd == cmd) {
+			handler = t->handler;
+			up_read(&ioctl32_sem);
 			goto found_handler;
+		}
 	}
 	up_read(&ioctl32_sem);
 
@@ -413,15 +417,13 @@
 	goto out_fput;
 
  found_handler:
-	if (t->handler) {
+	if (handler) {
 		lock_kernel();
-		error = t->handler(fd, cmd, arg, filp);
+		error = handler(fd, cmd, arg, filp);
 		unlock_kernel();
-		up_read(&ioctl32_sem);
 		goto out_fput;
 	}
 
-	up_read(&ioctl32_sem);
  do_ioctl:
 	error = vfs_ioctl(filp, fd, cmd, arg);
  out_fput:

Re: [PATCH -rt] race condition in fs/compat.c with compat_sys_ioctl

[Andi Kleen]

Steven Rostedt <rostedt@goodmis.org> writes:
> 
> That's the problem. I found out that one ioctl might sleep holding the
> sem and won't be woken up until another process calls another ioctl to
> wake it up. But unfortunately, the one waking up the sleeper will block
> on the sem.  (the killer was tty_wait_until_sent)

You should have looked into mainline first. The semaphore is already gone
because it wasn't even needed anymore.

-Andi

Re: [PATCH -rt] race condition in fs/compat.c with compat_sys_ioctl

[Ingo Molnar]

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

> Steven Rostedt <rostedt@goodmis.org> writes:
> > 
> > That's the problem. I found out that one ioctl might sleep holding the
> > sem and won't be woken up until another process calls another ioctl to
> > wake it up. But unfortunately, the one waking up the sleeper will block
> > on the sem.  (the killer was tty_wait_until_sent)
> 
> You should have looked into mainline first. The semaphore is already 
> gone because it wasn't even needed anymore.

well 2.6.14 isnt _that_ old :-) But in any case, it's great that the 
semaphore is gone!

	Ingo

Re: [PATCH -rt] race condition in fs/compat.c with compat_sys_ioctl

[Steven Rostedt]

On Wed, 2005-11-16 at 06:55 +0100, Andi Kleen wrote:
> Steven Rostedt <rostedt@goodmis.org> writes:
> > 
> > That's the problem. I found out that one ioctl might sleep holding the
> > sem and won't be woken up until another process calls another ioctl to
> > wake it up. But unfortunately, the one waking up the sleeper will block
> > on the sem.  (the killer was tty_wait_until_sent)
> 
> You should have looked into mainline first. The semaphore is already gone
> because it wasn't even needed anymore.

It's still there in 2.6.15-rc1-git3 (the sem is the down_read of
ioctl32_sem in fs/compat.c).

No, the problem was unique to the rt patch.  In -rt the default
down_read is just like a down (since it is very hard to do PI on readers
and writer locks).  So the solution in -rt was to convert this back to a
normal RW sem.

-- Steve

Re: [PATCH -rt] race condition in fs/compat.c with compat_sys_ioctl

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> It's still there in 2.6.15-rc1-git3 (the sem is the down_read of 
> ioctl32_sem in fs/compat.c).
> 
> No, the problem was unique to the rt patch.  In -rt the default 
> down_read is just like a down (since it is very hard to do PI on 
> readers and writer locks).  So the solution in -rt was to convert this 
> back to a normal RW sem.

to be precise: rwsems in PREEMPT_RT are "only one reader, but reader 
might self-recurse". I.e. the sequence of:

	down_read();
	...
		down_read();
		...
		up_read();
	...
	up_read();

is still supported.

this very simple rw-semaphore type covers the overwhelming majority of 
uses: out of hundreds of rwsems in the kernel, ioctl32_sem is the first 
one that had to be converted to a 'compat' rwsem.

	Ingo

Re: [PATCH -rt] race condition in fs/compat.c with compat_sys_ioctl

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

>  	down_read(&ioctl32_sem);
>  	for (t = ioctl32_hash_table[ioctl32_hash(cmd)]; t; t = t->next) {
> -		if (t->cmd == cmd)
> +		if (t->cmd == cmd) {
> +			handler = t->handler;
> +			up_read(&ioctl32_sem);
>  			goto found_handler;
> +		}
>  	}
>  	up_read(&ioctl32_sem);

i think this problem only triggers on RT kernels, because the RT kernel 
only allows a single reader within a read-semaphore. This works well in 
99.9% of the cases. You just found the remaining 0.1% :-| The better 
solution within -rt would be to change ioctl32_sem to a compat 
semaphore, via the patch below. Can you confirm that this solves the 
bootup problem too?

	Ingo

Index: linux/fs/compat.c
===================================================================
--- linux.orig/fs/compat.c
+++ linux/fs/compat.c
@@ -268,7 +268,7 @@ out:
 
 #define IOCTL_HASHSIZE 256
 static struct ioctl_trans *ioctl32_hash_table[IOCTL_HASHSIZE];
-static DECLARE_RWSEM(ioctl32_sem);
+static COMPAT_DECLARE_RWSEM(ioctl32_sem);
 
 extern struct ioctl_trans ioctl_start[];
 extern int ioctl_table_size;

Re: [PATCH -rt] race condition in fs/compat.c with compat_sys_ioctl

[Steven Rostedt]

On Wed, 2005-11-16 at 09:32 +0100, Ingo Molnar wrote:
> * Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> >  	down_read(&ioctl32_sem);
> >  	for (t = ioctl32_hash_table[ioctl32_hash(cmd)]; t; t = t->next) {
> > -		if (t->cmd == cmd)
> > +		if (t->cmd == cmd) {
> > +			handler = t->handler;
> > +			up_read(&ioctl32_sem);
> >  			goto found_handler;
> > +		}
> >  	}
> >  	up_read(&ioctl32_sem);
> 
> i think this problem only triggers on RT kernels, because the RT kernel 
> only allows a single reader within a read-semaphore. This works well in 
> 99.9% of the cases. You just found the remaining 0.1% :-| The better 
> solution within -rt would be to change ioctl32_sem to a compat 
> semaphore, via the patch below. Can you confirm that this solves the 
> bootup problem too?
> 

ACK:

Well duh!  OK, I blame lack of sleep on missing the obvious (and getting
up at 4am to go to the bathroom, and then responding to this doesn't
help ;)

I just applied this patch and it works.  I didn't even notice the word
"read" in down_read and up_read, otherwise I would have sent you this
patch.  I was just so frustrated at getting this to work that I just
assumed that this was a normal down and up. Oh well. At least now I
don't have to see why this works in the non-rt case.

Thanks Ingo, yes go ahead and apply your patch. That is definitely a
better solution.

-- Steve