While trying to figure out the best way to handle an odd mouse, I found
that we oopsed when doing a rmmod on usbhid while someone had a USB
device open through evdev.
We try to loop through evdev->list, calling kill_fasync on each member,
however by the time we try to get the next pointer, we have already
freed the member and poisoned the next/last.
The fix is fairly simple, and if nobody objects I think we should try
and get this into -stable too.
Signed-off-by: "Zephaniah E. Hull" <[email protected]>
diff -ur linux-test/drivers/input/evdev.c linux-2.6/drivers/input/evdev.c
--- linux-test/drivers/input/evdev.c 2006-07-24 23:36:01.000000000 -0400
+++ linux-2.6/drivers/input/evdev.c 2006-08-07 11:41:13.000000000 -0400
@@ -659,7 +659,7 @@
static void evdev_disconnect(struct input_handle *handle)
{
struct evdev *evdev = handle->private;
- struct evdev_list *list;
+ struct evdev_list *list, *next;
sysfs_remove_link(&input_class.subsys.kset.kobj, evdev->name);
class_device_destroy(&input_class,
@@ -669,7 +669,7 @@
if (evdev->open) {
input_close_device(handle);
wake_up_interruptible(&evdev->wait);
- list_for_each_entry(list, &evdev->list, node)
+ list_for_each_entry_safe(list, next, &evdev->list, node)
kill_fasync(&list->fasync, SIGIO, POLL_HUP);
} else
evdev_free(evdev);
--
1024D/E65A7801 Zephaniah E. Hull <[email protected]>
92ED 94E4 B1E6 3624 226D 5727 4453 008B E65A 7801
CCs of replies from mailing lists are requested.
"Sir," barked one of those useless aristocratic generals to William
Howard Russell, the great Times war correspondent, "I do not like what
you write." "Then, sir," retorted Russell, "I suggest you do not do what
I write about."
Hi,
On 8/7/06, Zephaniah E. Hull <[email protected]> wrote:
> if (evdev->open) {
> input_close_device(handle);
> wake_up_interruptible(&evdev->wait);
> - list_for_each_entry(list, &evdev->list, node)
> + list_for_each_entry_safe(list, next, &evdev->list, node)
> kill_fasync(&list->fasync, SIGIO, POLL_HUP);
NAK. kill_fasync does not affect the list state so using _safe does
not buy us anything.
BTW, [email protected] address is dead, please use
[email protected] or [email protected] or [email protected].
--
Dmitry
On Mon, Aug 07, 2006 at 01:35:50PM -0400, Dmitry Torokhov wrote:
> Hi,
>
> On 8/7/06, Zephaniah E. Hull <[email protected]> wrote:
> > if (evdev->open) {
> > input_close_device(handle);
> > wake_up_interruptible(&evdev->wait);
> >- list_for_each_entry(list, &evdev->list, node)
> >+ list_for_each_entry_safe(list, next, &evdev->list, node)
> > kill_fasync(&list->fasync, SIGIO, POLL_HUP);
>
> NAK. kill_fasync does not affect the list state so using _safe does
> not buy us anything.
Sorry, but you're wrong.
Immediately before the kill_fasync call list->node.next is a valid
pointer, immediately afterwords it is 0x100100, which happens to be
list_poison. kill_fasync is triggering a close somehow, evdev_close
deletes that element of the list, which poisons the next value, which
can make us crash and burn.
I have a 100% reproducible crash case, which is fixed by the change.
If kill_fasync shouldn't be making it close that's another issue, but at
the moment it is and this is a fairly non-invasive change which fixes
it.
>
> BTW, [email protected] address is dead, please use
> [email protected] or [email protected] or [email protected].
Noted, recommend updating the entry in MAINTAINERS. :)
Zephaniah E. Hull.
--
1024D/E65A7801 Zephaniah E. Hull <[email protected]>
92ED 94E4 B1E6 3624 226D 5727 4453 008B E65A 7801
CCs of replies from mailing lists are requested.
> Is there an API or other means to determine what video
> card, namely the chipset, that the user has installed
> on his machine?
On a modern X86 machine use the PCI/AGP bus data. On a PS/2 use the MCA bus
data. On nubus use the nubus probe data. On old style ISA bus PCs done a large
pointy hat and spend several years reading arcane and forbidden scrolls
-- Alan Cox
On 8/7/06, Zephaniah E. Hull <[email protected]> wrote:
> On Mon, Aug 07, 2006 at 01:35:50PM -0400, Dmitry Torokhov wrote:
> > Hi,
> >
> > On 8/7/06, Zephaniah E. Hull <[email protected]> wrote:
> > > if (evdev->open) {
> > > input_close_device(handle);
> > > wake_up_interruptible(&evdev->wait);
> > >- list_for_each_entry(list, &evdev->list, node)
> > >+ list_for_each_entry_safe(list, next, &evdev->list, node)
> > > kill_fasync(&list->fasync, SIGIO, POLL_HUP);
> >
> > NAK. kill_fasync does not affect the list state so using _safe does
> > not buy us anything.
>
> Sorry, but you're wrong.
>
> Immediately before the kill_fasync call list->node.next is a valid
> pointer, immediately afterwords it is 0x100100, which happens to be
> list_poison. kill_fasync is triggering a close somehow, evdev_close
> deletes that element of the list, which poisons the next value, which
> can make us crash and burn.
>
> I have a 100% reproducible crash case, which is fixed by the change.
>
> If kill_fasync shouldn't be making it close that's another issue, but at
> the moment it is and this is a fairly non-invasive change which fixes
> it.
>
Unfortunately it does not really fix the problem, it just papers over
the issue. The crash will still happen if for some reason
evdev_release runs at a bad moment.
> > BTW, [email protected] address is dead, please use
> > [email protected] or [email protected] or [email protected].
>
> Noted, recommend updating the entry in MAINTAINERS. :)
>
Already done ;)
--
Dmitry
On Mon, Aug 07, 2006 at 03:04:36PM -0400, Dmitry Torokhov wrote:
> On 8/7/06, Zephaniah E. Hull <[email protected]> wrote:
> >On Mon, Aug 07, 2006 at 01:35:50PM -0400, Dmitry Torokhov wrote:
> >> Hi,
> >>
> >> On 8/7/06, Zephaniah E. Hull <[email protected]> wrote:
> >> > if (evdev->open) {
> >> > input_close_device(handle);
> >> > wake_up_interruptible(&evdev->wait);
> >> >- list_for_each_entry(list, &evdev->list, node)
> >> >+ list_for_each_entry_safe(list, next, &evdev->list, node)
> >> > kill_fasync(&list->fasync, SIGIO, POLL_HUP);
> >>
> >> NAK. kill_fasync does not affect the list state so using _safe does
> >> not buy us anything.
> >
> >Sorry, but you're wrong.
> >
> >Immediately before the kill_fasync call list->node.next is a valid
> >pointer, immediately afterwords it is 0x100100, which happens to be
> >list_poison. kill_fasync is triggering a close somehow, evdev_close
> >deletes that element of the list, which poisons the next value, which
> >can make us crash and burn.
> >
> >I have a 100% reproducible crash case, which is fixed by the change.
> >
> >If kill_fasync shouldn't be making it close that's another issue, but at
> >the moment it is and this is a fairly non-invasive change which fixes
> >it.
> >
>
> Unfortunately it does not really fix the problem, it just papers over
> the issue. The crash will still happen if for some reason
> evdev_release runs at a bad moment.
Almost agreed, it papers over the lack of locking, however most forms of
locking may need this to avoid a deadlock anyhow. (I don't know the
semantics of kill_fasync, and thus I don't know if attempting to take a
lock in the close that is held here would deadlock, or if execution
would move back to here, but I have a nasty feeling that it's the
former.)
That said, at the moment, we have a 100% guaranteed oops on rmmod usbhid
if something has the device open, which is bad.
Zephaniah E. Hull.
--
1024D/E65A7801 Zephaniah E. Hull <[email protected]>
92ED 94E4 B1E6 3624 226D 5727 4453 008B E65A 7801
CCs of replies from mailing lists are requested.
"I would rather spend 10 hours reading someone else's source code than
10 minutes listening to Musak waiting for technical support which
isn't."
(By Dr. Greg Wettstein, Roger Maris Cancer Center)