uinput crash and fix

uinput crash and fix

[emard]

HI

During some begginer's fiddling with uinput it
wasn't too difficult to obtain a hard kernel freeze:

CPU:    1
EIP:    0060:[<f90310ff>]    Tainted: P      VLI
EFLAGS: 00210246   (2.6.13.4)
EIP is at uinput_request_done+0x14/0x3e [uinput]
eax: e2d72000   ebx: e2d73ea4   ecx: ea9e7020   edx: c17efa80
esi: dcbf8400   edi: 400c55cb   ebp: dcbf8400   esp: c47bdee0
ds: 007b   es: 007b   ss: 0068
Process ifeel (pid: 10855, threadinfo=c47bc000 task=dcb2e520)
Stack: c4b45980 b7f3c3b4 f9031db7 dcbf8400 e2d73ea4 0000000c 00000001 00000000
       00000000 00000003 00200002 da41e00c 00200202 00000021 00200002 c02ed08d
       00000000 d9bcabec 00200202 c02edf2f da41e00c 00000002 00000000 00000000
Call Trace:
 [<f9031db7>] uinput_ioctl+0x2fa/0x49b [uinput]
 [<c02ed08d>] tty_ldisc_deref+0x48/0x71
 [<c02edf2f>] tty_write+0x1cc/0x21e
 [<c0170688>] do_ioctl+0x78/0x81
 [<c0170813>] vfs_ioctl+0x5a/0x1f1
 [<c01709e6>] sys_ioctl+0x3c/0x5a
 [<c0102e39>] syscall_call+0x7/0xb
Code: 8b 54 24 08 31 c0 83 fa 0f 77 0b 8b 44 24 04 8b 84 90 1c 01 00 00 c3 56 53 8b 74 24 0c 8b 5c 24 10 8d 43 0c e8 26 a7 0e c7 8b 03 <c7> 84 86 1c 01 00 00 00 00 00 00 8d 86 5c 01 00 00 c7 44 24 0c

and I think this patch fixes this:

--- linux-2.6.13.4/drivers/input/misc/uinput.c.orig	2005-10-15 10:09:38.000000000 +0200
+++ linux-2.6.13.4/drivers/input/misc/uinput.c	2005-10-15 10:19:54.000000000 +0200
@@ -517,7 +517,11 @@ static int uinput_ioctl(struct inode *in
 				break;
 			}
 			req = uinput_request_find(udev, ff_up.request_id);
-			if (!(req && req->code == UI_FF_UPLOAD && req->u.effect)) {
+			if (!req) {
+				retval = -EINVAL;
+				break;
+			}
+			if (!(req->code == UI_FF_UPLOAD && req->u.effect)) {
 				retval = -EINVAL;
 				break;
 			}
@@ -535,7 +539,11 @@ static int uinput_ioctl(struct inode *in
 				break;
 			}
 			req = uinput_request_find(udev, ff_erase.request_id);
-			if (!(req && req->code == UI_FF_ERASE)) {
+			if (!req) {
+				retval = -EINVAL;
+				break;
+			}
+			if (!(req->code == UI_FF_ERASE)) {
 				retval = -EINVAL;
 				break;
 			}
@@ -553,7 +561,11 @@ static int uinput_ioctl(struct inode *in
 				break;
 			}
 			req = uinput_request_find(udev, ff_up.request_id);
-			if (!(req && req->code == UI_FF_UPLOAD && req->u.effect)) {
+			if (!req) {
+				retval = -EINVAL;
+				break;
+			}
+			if (!(req->code == UI_FF_UPLOAD && req->u.effect)) {
 				retval = -EINVAL;
 				break;
 			}
@@ -568,7 +580,11 @@ static int uinput_ioctl(struct inode *in
 				break;
 			}
 			req = uinput_request_find(udev, ff_erase.request_id);
-			if (!(req && req->code == UI_FF_ERASE)) {
+			if (!req) {
+				retval = -EINVAL;
+				break;
+			}
+			if (!(req->code == UI_FF_ERASE)) {
 				retval = -EINVAL;
 				break;
 			}

Re: uinput crash and fix

[Mattia Dongili]

On Sat, Oct 15, 2005 at 11:29:12PM +0200, emard@softhome.net wrote:
> HI
[...]
>  			req = uinput_request_find(udev, ff_up.request_id);
> -			if (!(req && req->code == UI_FF_UPLOAD && req->u.effect)) {
> +			if (!req) {

out of curiosity, instead of adding a whole if block wouldn't be easier
to just write 

	if (!req || !(req->code == UI_FF_UPLOAD && req->u.effect)) {

in order to evaulate !req first and eventually dereference it?
-- 
mattia
:wq!

Re: uinput crash and fix

[Mattia Dongili]

On Sun, Oct 16, 2005 at 12:01:15AM +0200, Mattia Dongili wrote:
> On Sat, Oct 15, 2005 at 11:29:12PM +0200, emard@softhome.net wrote:
> > HI
> [...]
> >  			req = uinput_request_find(udev, ff_up.request_id);
> > -			if (!(req && req->code == UI_FF_UPLOAD && req->u.effect)) {
> > +			if (!req) {
> 
> out of curiosity, instead of adding a whole if block wouldn't be easier
> to just write 
> 
> 	if (!req || !(req->code == UI_FF_UPLOAD && req->u.effect)) {
> 
> in order to evaulate !req first and eventually dereference it?

hmmm... no. it's simply the same ( (A && B) == (!A || !B)). So your
patch seems wrong too.

goodnight :P
-- 
mattia
:wq!

Re: uinput crash and fix

[emard]

[-- Attachment #1: Type: text/plain, Size: 246 bytes --]

It doesn't fix the crash, it happened again.
Maybe it comes from SMP race condition

Here's the source, if you have one of those
force feedback mice you can feel it, but it
can also be tried without force feedback, the
problem is the same

Emard

[-- Attachment #2: ifeel038.tar.bz2 --]
[-- Type: application/octet-stream, Size: 17012 bytes --]

Re: uinput crash and NO FIX YET

[emard]

HI

I tested uinput that crashes on my SMP PREEMPT 2.6.13.4 (pentium 4 HT)
on a 1-CPU 2.6.12.5 (pentium 3) and there is no such crash.

Something evil is happening with uinput on SMP machines

Re: uinput crash and NO FIX YET

[emard]

Seems the crash is coming from bad locking

I have located exact place where the problem is manifested and
did this patch:

--- drivers/input/misc/uinput.c.orig	2005-10-15 10:09:38.000000000 +0200
+++ drivers/input/misc/uinput.c	2005-10-16 22:19:20.000000000 +0200
@@ -93,7 +93,8 @@ static void uinput_request_done(struct u
 	complete(&request->done);
 
 	/* Mark slot as available */
-	udev->requests[request->id] = NULL;
+	if(request->id >= 0 && request->id < UINPUT_NUM_REQUESTS)
+		udev->requests[request->id] = NULL;
 	wake_up_interruptible(&udev->requests_waitq);
 }
 
This checks wether request id has some sane value.
This way I have avoided crashes but now I'm running
of request[ ] slots, sometimes they tend to leak and
when all the slots are exhausted no more effects can
be accepted.

[PATCH] uinput crash maybe this is the FIX

[emard]

HI

THanks for all the hints.

Here's my fix for the situation in the uinput.
It generally breaks when Force feedback is removed,
sometimes request id slot is not freed correctly and
it can lead to crash and/or running out of slots.

Without knowing much what I'm doing, I added one
lock and it seems to fix the problem.

Please check is this the right approach on how to do
this locks....

--- linux-2.6.13.4/drivers/input/misc/uinput.c.orig	2005-10-15 10:09:38.000000000 +0200
+++ linux-2.6.13.4/drivers/input/misc/uinput.c	2005-10-16 23:54:20.000000000 +0200
@@ -90,10 +90,16 @@ static inline int uinput_request_reserve
 
 static void uinput_request_done(struct uinput_device *udev, struct uinput_request *request)
 {
+	int id;
+	
+	spin_lock(&udev->requests_lock);
+	id = request->id;
+	spin_unlock(&udev->requests_lock);
 	complete(&request->done);
 
 	/* Mark slot as available */
-	udev->requests[request->id] = NULL;
+	if(id >= 0 && id < UINPUT_NUM_REQUESTS)
+		udev->requests[id] = NULL;
 	wake_up_interruptible(&udev->requests_waitq);
 }
 

Re: [PATCH] uinput crash maybe this is the FIX

[Dmitry Torokhov]

On Sunday 16 October 2005 17:06, emard@softhome.net wrote:
> HI
> 
> THanks for all the hints.
> 
> Here's my fix for the situation in the uinput.
> It generally breaks when Force feedback is removed,
> sometimes request id slot is not freed correctly and
> it can lead to crash and/or running out of slots.
> 
> Without knowing much what I'm doing, I added one
> lock and it seems to fix the problem.
>

The lock is not really needed, please try the patch below.

-- 
Dmitry

Input: uniput - fix crash on SMP

Only signal completion after marking request slot as free,
otherwise other processor can free request structure before
we finish using it.

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/input/misc/uinput.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Index: work/drivers/input/misc/uinput.c
===================================================================
--- work.orig/drivers/input/misc/uinput.c
+++ work/drivers/input/misc/uinput.c
@@ -90,11 +90,11 @@ static inline int uinput_request_reserve
 
 static void uinput_request_done(struct uinput_device *udev, struct uinput_request *request)
 {
-	complete(&request->done);
-
 	/* Mark slot as available */
 	udev->requests[request->id] = NULL;
 	wake_up_interruptible(&udev->requests_waitq);
+
+	complete(&request->done);
 }
 
 static int uinput_request_submit(struct input_dev *dev, struct uinput_request *request)

Re: uinput crash maybe this is the FIX

[emard]

> -	complete(&request->done);
> -
>  	/* Mark slot as available */
>  	udev->requests[request->id] = NULL;
>  	wake_up_interruptible(&udev->requests_waitq);
> +
> +	complete(&request->done);

Man, I think that's the way to go!
I will continue testing with this
patch, so far it looks good :) 

Let this uinput patch go to 2.6.14

[emard]

On Mon, Oct 17, 2005 at 01:16:35AM -0600, emard@softhome.net wrote:
> >-	complete(&request->done);
> >-
> > 	/* Mark slot as available */
> > 	udev->requests[request->id] = NULL;
> > 	wake_up_interruptible(&udev->requests_waitq);
> >+
> >+	complete(&request->done);

I have tested this quite thoroughly, 
no crash no leakage in effect slots,
I'd like to have this in 2.6.14