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;
}
On Sat, Oct 15, 2005 at 11:29:12PM +0200, [email protected] 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!
On Sun, Oct 16, 2005 at 12:01:15AM +0200, Mattia Dongili wrote:
> On Sat, Oct 15, 2005 at 11:29:12PM +0200, [email protected] 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!
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:
--- 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.
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);
}
On Sunday 16 October 2005 17:06, [email protected] 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 <[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)
> - 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 :)
On Mon, Oct 17, 2005 at 01:16:35AM -0600, [email protected] 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