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);
}
Dmitry Torokhov <[email protected]> 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);
?
On Saturday 23 August 2003 02:19 am, Andrew Morton wrote:
> Dmitry Torokhov <[email protected]> wrote:
> > On Saturday 23 August 2003 02:00 am, Andrew Morton wrote:
> > > Dmitry Torokhov <[email protected]> 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?
Dmitry Torokhov <[email protected]> wrote:
>
> On Saturday 23 August 2003 02:00 am, Andrew Morton wrote:
> > Dmitry Torokhov <[email protected]> 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.
Dmitry Torokhov <[email protected]> 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.
On Sat, 23 Aug 2003, Andrew Morton wrote:
> Dmitry Torokhov <[email protected]> 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:
On Saturday 23 August 2003 02:34 am, Andrew Morton wrote:
> Dmitry Torokhov <[email protected]> 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);