uinput crash and fix

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:

@@ -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;
 			}

[...]

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

mattia
:wq!

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

goodnight :P

mattia
:wq!

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

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

Seems the crash is coming from bad locking

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

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

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

@@ -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);
 }
 

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 <[email protected]>
---

 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)

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