* [PATCH 2.6] 2/3 Serio: possible race in handle_events
@ 2003-08-23 6:31 Dmitry Torokhov
2003-08-23 7:00 ` Andrew Morton
0 siblings, 1 reply; 7+ messages in thread
From: Dmitry Torokhov @ 2003-08-23 6:31 UTC (permalink / raw)
To: linux-kernel; +Cc: Vojtech Pavlik
The event handling in serio is racy because serio_handle_events does not
check whether serio in event list is still alive or has already been freed.
One possible solution is to check whether serio in question is still
registered in serio_list and skip events referring to unknown serios.
Dmitry
diff -urN --exclude-from=/usr/src/exclude 2.6.0-test4/drivers/input/serio/serio.c linux-2.6.0-test4/drivers/input/serio/serio.c
--- 2.6.0-test4/drivers/input/serio/serio.c 2003-08-22 23:26:50.000000000 -0500
+++ linux-2.6.0-test4/drivers/input/serio/serio.c 2003-08-22 23:25:20.000000000 -0500
@@ -85,6 +85,16 @@
static DECLARE_WAIT_QUEUE_HEAD(serio_wait);
static DECLARE_COMPLETION(serio_exited);
+static int is_known_serio(struct serio *serio)
+{
+ struct serio *s;
+
+ list_for_each_entry(s, &serio_list, node)
+ if (s == serio)
+ return 1;
+ return 0;
+}
+
void serio_handle_events(void)
{
struct list_head *node, *next;
@@ -93,17 +103,21 @@
list_for_each_safe(node, next, &serio_event_list) {
event = container_of(node, struct serio_event, node);
+ down(&serio_sem);
+ if (!is_known_serio(event->serio))
+ goto event_done;
+
switch (event->type) {
case SERIO_RESCAN :
- down(&serio_sem);
if (event->serio->dev && event->serio->dev->disconnect)
event->serio->dev->disconnect(event->serio);
serio_find_dev(event->serio);
- up(&serio_sem);
break;
default:
break;
}
+event_done:
+ up(&serio_sem);
list_del_init(node);
kfree(event);
}
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 2.6] 2/3 Serio: possible race in handle_events 2003-08-23 6:31 [PATCH 2.6] 2/3 Serio: possible race in handle_events Dmitry Torokhov @ 2003-08-23 7:00 ` Andrew Morton [not found] ` <200308230206.25142.dtor_core@ameritech.net> 0 siblings, 1 reply; 7+ messages in thread From: Andrew Morton @ 2003-08-23 7:00 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: linux-kernel, vojtech Dmitry Torokhov <dtor_core@ameritech.net> wrote: > > +static int is_known_serio(struct serio *serio) > +{ > + struct serio *s; > + > + list_for_each_entry(s, &serio_list, node) > + if (s == serio) > + return 1; > + return 0; > +} Could this just be return !list_empty(&serio->node); ? ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <200308230206.25142.dtor_core@ameritech.net>]
* Re: [PATCH 2.6] 2/3 Serio: possible race in handle_events [not found] ` <200308230206.25142.dtor_core@ameritech.net> @ 2003-08-23 7:19 ` Andrew Morton 2003-08-23 7:25 ` Dmitry Torokhov 0 siblings, 1 reply; 7+ messages in thread From: Andrew Morton @ 2003-08-23 7:19 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: linux-kernel, vojtech Dmitry Torokhov <dtor_core@ameritech.net> wrote: > > On Saturday 23 August 2003 02:00 am, Andrew Morton wrote: > > Dmitry Torokhov <dtor_core@ameritech.net> wrote: > > > +static int is_known_serio(struct serio *serio) > > > +{ > > > + struct serio *s; > > > + > > > + list_for_each_entry(s, &serio_list, node) > > > + if (s == serio) > > > + return 1; > > > + return 0; > > > +} > > > > Could this just be > > > > return !list_empty(&serio->node); > > > > ? > > The serio could be free()d, I dont think we want to call list_empty with > a dangling pointer. Or am I missing something? > Well if we're playing around with a freed pointer then something is seriously wrong. Like, someone could have allocated a new one and got the same address. If event->serio can point at freed memory and there's any doubt over it then we should be nulling out event->serio to indicate that. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2.6] 2/3 Serio: possible race in handle_events 2003-08-23 7:19 ` Andrew Morton @ 2003-08-23 7:25 ` Dmitry Torokhov 2003-08-23 7:34 ` Andrew Morton 0 siblings, 1 reply; 7+ messages in thread From: Dmitry Torokhov @ 2003-08-23 7:25 UTC (permalink / raw) To: Andrew Morton; +Cc: linux-kernel, vojtech On Saturday 23 August 2003 02:19 am, Andrew Morton wrote: > Dmitry Torokhov <dtor_core@ameritech.net> wrote: > > On Saturday 23 August 2003 02:00 am, Andrew Morton wrote: > > > Dmitry Torokhov <dtor_core@ameritech.net> wrote: > > > > +static int is_known_serio(struct serio *serio) > > > > +{ > > > > + struct serio *s; > > > > + > > > > + list_for_each_entry(s, &serio_list, node) > > > > + if (s == serio) > > > > + return 1; > > > > + return 0; > > > > +} > > > > > > Could this just be > > > > > > return !list_empty(&serio->node); > > > > > > ? > > > > The serio could be free()d, I dont think we want to call list_empty with > > a dangling pointer. Or am I missing something? > > Well if we're playing around with a freed pointer then something is > seriously wrong. Like, someone could have allocated a new one and got the > same address. > > If event->serio can point at freed memory and there's any doubt over it > then we should be nulling out event->serio to indicate that. Right now we can't as events are queued in an event list and are processed by other thread; serio does not know that it's queued and even existence of the event list is not known outside of serio.c module. Do you think we should introduce allocate_serio/free_serio pair for dynamically allocated serios; free_serio would scan event_list and invalidate events that refer to freed serio? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2.6] 2/3 Serio: possible race in handle_events 2003-08-23 7:25 ` Dmitry Torokhov @ 2003-08-23 7:34 ` Andrew Morton 2003-08-23 11:41 ` Zwane Mwaikambo 2003-08-23 21:42 ` Dmitry Torokhov 0 siblings, 2 replies; 7+ messages in thread From: Andrew Morton @ 2003-08-23 7:34 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: linux-kernel, vojtech Dmitry Torokhov <dtor_core@ameritech.net> wrote: > > Do you think we should introduce allocate_serio/free_serio pair for dynamically > allocated serios; free_serio would scan event_list and invalidate events that > refer to freed serio? I don't understand the subsystem well enough to say. But if we are receiving events which refer to an already-freed serio then something is very broken. We should be draining all those events before allowing the original object to be freed up. Let's wait for Vojtech's comments. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2.6] 2/3 Serio: possible race in handle_events 2003-08-23 7:34 ` Andrew Morton @ 2003-08-23 11:41 ` Zwane Mwaikambo 2003-08-23 21:42 ` Dmitry Torokhov 1 sibling, 0 replies; 7+ messages in thread From: Zwane Mwaikambo @ 2003-08-23 11:41 UTC (permalink / raw) To: Andrew Morton; +Cc: Dmitry Torokhov, linux-kernel, vojtech On Sat, 23 Aug 2003, Andrew Morton wrote: > Dmitry Torokhov <dtor_core@ameritech.net> wrote: > > > > Do you think we should introduce allocate_serio/free_serio pair for dynamically > > allocated serios; free_serio would scan event_list and invalidate events that > > refer to freed serio? > > I don't understand the subsystem well enough to say. But if we are > receiving events which refer to an already-freed serio then something is > very broken. We should be draining all those events before allowing the > original object to be freed up. > > Let's wait for Vojtech's comments. This sounds somewhat like; http://bugzilla.kernel.org/show_bug.cgi?id=973 Jul 25 04:54:24 lap kernel: slab error in cache_free_debugcheck(): cache `size-32': double free, or memory before object was overwritten Jul 25 04:54:24 lap kernel: Call Trace: Jul 25 04:54:24 lap kernel: [<c014f130>] kfree+0xf0/0x310 Jul 25 04:54:24 lap kernel: [<c02540fc>] psmouse_disconnect+0x2c/0x40 Jul 25 04:54:24 lap kernel: [<c02540fc>] psmouse_disconnect+0x2c/0x40 Jul 25 04:54:24 lap kernel: [<c025560d>] serio_handle_events+0xad/0xc0 Jul 25 04:54:24 lap kernel: [<c0255620>] serio_thread+0x0/0x100 Jul 25 04:54:24 lap kernel: [<c0255665>] serio_thread+0x45/0x100 Jul 25 04:54:24 lap kernel: [<c010a126>] work_resched+0x5/0x16 Jul 25 04:54:24 lap kernel: [<c0255620>] serio_thread+0x0/0x100 Jul 25 04:54:24 lap kernel: [<c0255620>] serio_thread+0x0/0x100 Jul 25 04:54:24 lap kernel: [<c01073b9>] kernel_thread_helper+0x5/0xc Jul 25 04:54:24 lap kernel: Jul 25 04:54:24 lap kernel: slab error in cache_free_debugcheck(): cache `size-32': double free, or memory after object was overwritten Jul 25 04:54:24 lap kernel: Call Trace: Jul 25 04:54:24 lap kernel: [<c014f15e>] kfree+0x11e/0x310 Jul 25 04:54:24 lap kernel: [<c02540fc>] psmouse_disconnect+0x2c/0x40 Jul 25 04:54:24 lap kernel: [<c02540fc>] psmouse_disconnect+0x2c/0x40 Jul 25 04:54:24 lap kernel: [<c025560d>] serio_handle_events+0xad/0xc0 Jul 25 04:54:24 lap kernel: [<c0255620>] serio_thread+0x0/0x100 Jul 25 04:54:24 lap kernel: [<c0255665>] serio_thread+0x45/0x100 Jul 25 04:54:24 lap kernel: [<c010a126>] work_resched+0x5/0x16 Jul 25 04:54:24 lap kernel: [<c0255620>] serio_thread+0x0/0x100 Jul 25 04:54:24 lap kernel: [<c0255620>] serio_thread+0x0/0x100 Jul 25 04:54:24 lap kernel: [<c01073b9>] kernel_thread_helper+0x5/0xc Jul 25 04:54:24 lap kernel: ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2.6] 2/3 Serio: possible race in handle_events 2003-08-23 7:34 ` Andrew Morton 2003-08-23 11:41 ` Zwane Mwaikambo @ 2003-08-23 21:42 ` Dmitry Torokhov 1 sibling, 0 replies; 7+ messages in thread From: Dmitry Torokhov @ 2003-08-23 21:42 UTC (permalink / raw) To: Andrew Morton; +Cc: linux-kernel, vojtech On Saturday 23 August 2003 02:34 am, Andrew Morton wrote: > Dmitry Torokhov <dtor_core@ameritech.net> wrote: > > Do you think we should introduce allocate_serio/free_serio pair for > > dynamically allocated serios; free_serio would scan event_list and > > invalidate events that refer to freed serio? > > I don't understand the subsystem well enough to say. But if we are > receiving events which refer to an already-freed serio then something is > very broken. We should be draining all those events before allowing the > original object to be freed up. > > Let's wait for Vojtech's comments. I think the patch below should work. We invalidate serio events at the same time serio itself is unregistered. The patch is on top of the 3 previous ones + synaptics, but if needed I can rediff against vanilla kernel. diff -urN --exclude-from=/usr/src/exclude 2.6.0-test4/drivers/input/serio/serio.c linux-2.6.0-test4/drivers/input/serio/serio.c --- 2.6.0-test4/drivers/input/serio/serio.c 2003-08-23 00:38:10.000000000 -0500 +++ linux-2.6.0-test4/drivers/input/serio/serio.c 2003-08-23 16:28:28.000000000 -0500 @@ -89,14 +89,13 @@ static DECLARE_WAIT_QUEUE_HEAD(serio_wait); static DECLARE_COMPLETION(serio_exited); -static int is_known_serio(struct serio *serio) +static void serio_invalidate_pending_events(struct serio *serio) { - struct serio *s; + struct serio_event *event; - list_for_each_entry(s, &serio_list, node) - if (s == serio) - return 1; - return 0; + list_for_each_entry(event, &serio_event_list, node) + if (event->serio == serio) + event->serio = NULL; } void serio_handle_events(void) @@ -108,7 +107,7 @@ event = container_of(node, struct serio_event, node); down(&serio_sem); - if (!is_known_serio(event->serio)) + if (event->serio == NULL) goto event_done; switch (event->type) { @@ -213,6 +212,7 @@ void serio_unregister_port(struct serio *serio) { down(&serio_sem); + serio_invalidate_pending_events(serio); list_del_init(&serio->node); if (serio->dev && serio->dev->disconnect) serio->dev->disconnect(serio); @@ -226,6 +226,7 @@ */ void serio_unregister_slave_port(struct serio *serio) { + serio_invalidate_pending_events(serio); list_del_init(&serio->node); if (serio->dev && serio->dev->disconnect) serio->dev->disconnect(serio); ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2003-08-23 21:43 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2003-08-23 6:31 [PATCH 2.6] 2/3 Serio: possible race in handle_events Dmitry Torokhov
2003-08-23 7:00 ` Andrew Morton
[not found] ` <200308230206.25142.dtor_core@ameritech.net>
2003-08-23 7:19 ` Andrew Morton
2003-08-23 7:25 ` Dmitry Torokhov
2003-08-23 7:34 ` Andrew Morton
2003-08-23 11:41 ` Zwane Mwaikambo
2003-08-23 21:42 ` Dmitry Torokhov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).