From 15803478fdea964e5f76079851fcd13068208d5d Mon Sep 17 00:00:00 2001 From: Stefan Richter Date: Sun, 24 Feb 2008 18:57:23 +0100 Subject: firewire: potentially invalid pointers used in fw_card_bm_work The bus management workqueue job was in danger to dereference NULL pointers. Also, after having temporarily lifted card->lock, a few node pointers and a device pointer may have become invalid. Add NULL pointer checks and get the necessary references. Also, move card->local_node out of fw_card_bm_work's sight during shutdown of the card. Signed-off-by: Stefan Richter Signed-off-by: Jarod Wilson --- drivers/firewire/fw-card.c | 51 ++++++++++++++++++++++++++++++---------------- 1 file changed, 34 insertions(+), 17 deletions(-) (limited to 'drivers/firewire/fw-card.c') diff --git a/drivers/firewire/fw-card.c b/drivers/firewire/fw-card.c index 3e9719948a8..e6395b29850 100644 --- a/drivers/firewire/fw-card.c +++ b/drivers/firewire/fw-card.c @@ -214,17 +214,29 @@ static void fw_card_bm_work(struct work_struct *work) { struct fw_card *card = container_of(work, struct fw_card, work.work); - struct fw_device *root; + struct fw_device *root_device; + struct fw_node *root_node, *local_node; struct bm_data bmd; unsigned long flags; int root_id, new_root_id, irm_id, gap_count, generation, grace; int do_reset = 0; spin_lock_irqsave(&card->lock, flags); + local_node = card->local_node; + root_node = card->root_node; + + if (local_node == NULL) { + spin_unlock_irqrestore(&card->lock, flags); + return; + } + fw_node_get(local_node); + fw_node_get(root_node); generation = card->generation; - root = card->root_node->data; - root_id = card->root_node->node_id; + root_device = root_node->data; + if (root_device) + fw_device_get(root_device); + root_id = root_node->node_id; grace = time_after(jiffies, card->reset_jiffies + DIV_ROUND_UP(HZ, 10)); if (card->bm_generation + 1 == generation || @@ -243,14 +255,14 @@ fw_card_bm_work(struct work_struct *work) irm_id = card->irm_node->node_id; if (!card->irm_node->link_on) { - new_root_id = card->local_node->node_id; + new_root_id = local_node->node_id; fw_notify("IRM has link off, making local node (%02x) root.\n", new_root_id); goto pick_me; } bmd.lock.arg = cpu_to_be32(0x3f); - bmd.lock.data = cpu_to_be32(card->local_node->node_id); + bmd.lock.data = cpu_to_be32(local_node->node_id); spin_unlock_irqrestore(&card->lock, flags); @@ -267,12 +279,12 @@ fw_card_bm_work(struct work_struct *work) * Another bus reset happened. Just return, * the BM work has been rescheduled. */ - return; + goto out; } if (bmd.rcode == RCODE_COMPLETE && bmd.old != 0x3f) /* Somebody else is BM, let them do the work. */ - return; + goto out; spin_lock_irqsave(&card->lock, flags); if (bmd.rcode != RCODE_COMPLETE) { @@ -282,7 +294,7 @@ fw_card_bm_work(struct work_struct *work) * do a bus reset and pick the local node as * root, and thus, IRM. */ - new_root_id = card->local_node->node_id; + new_root_id = local_node->node_id; fw_notify("BM lock failed, making local node (%02x) root.\n", new_root_id); goto pick_me; @@ -295,7 +307,7 @@ fw_card_bm_work(struct work_struct *work) */ spin_unlock_irqrestore(&card->lock, flags); schedule_delayed_work(&card->work, DIV_ROUND_UP(HZ, 10)); - return; + goto out; } /* @@ -305,20 +317,20 @@ fw_card_bm_work(struct work_struct *work) */ card->bm_generation = generation; - if (root == NULL) { + if (root_device == NULL) { /* * Either link_on is false, or we failed to read the * config rom. In either case, pick another root. */ - new_root_id = card->local_node->node_id; - } else if (atomic_read(&root->state) != FW_DEVICE_RUNNING) { + new_root_id = local_node->node_id; + } else if (atomic_read(&root_device->state) != FW_DEVICE_RUNNING) { /* * If we haven't probed this device yet, bail out now * and let's try again once that's done. */ spin_unlock_irqrestore(&card->lock, flags); - return; - } else if (root->config_rom[2] & BIB_CMC) { + goto out; + } else if (root_device->config_rom[2] & BIB_CMC) { /* * FIXME: I suppose we should set the cmstr bit in the * STATE_CLEAR register of this node, as described in @@ -332,7 +344,7 @@ fw_card_bm_work(struct work_struct *work) * successfully read the config rom, but it's not * cycle master capable. */ - new_root_id = card->local_node->node_id; + new_root_id = local_node->node_id; } pick_me: @@ -341,8 +353,8 @@ fw_card_bm_work(struct work_struct *work) * the typically much larger 1394b beta repeater delays though. */ if (!card->beta_repeaters_present && - card->root_node->max_hops < ARRAY_SIZE(gap_count_table)) - gap_count = gap_count_table[card->root_node->max_hops]; + root_node->max_hops < ARRAY_SIZE(gap_count_table)) + gap_count = gap_count_table[root_node->max_hops]; else gap_count = 63; @@ -364,6 +376,11 @@ fw_card_bm_work(struct work_struct *work) fw_send_phy_config(card, new_root_id, generation, gap_count); fw_core_initiate_bus_reset(card, 1); } + out: + if (root_device) + fw_device_put(root_device); + fw_node_put(root_node); + fw_node_put(local_node); } static void -- cgit v1.2.3 From 855c603d61ede7e2810217f15f0d574b4f29c891 Mon Sep 17 00:00:00 2001 From: Stefan Richter Date: Wed, 27 Feb 2008 22:14:27 +0100 Subject: firewire: fix crash in automatic module unloading "modprobe firewire-ohci; sleep .1; modprobe -r firewire-ohci" used to result in crashes like this: BUG: unable to handle kernel paging request at ffffffff8807b455 IP: [] PGD 203067 PUD 207063 PMD 7c170067 PTE 0 Oops: 0010 [1] PREEMPT SMP CPU 0 Modules linked in: i915 drm cpufreq_ondemand acpi_cpufreq freq_table applesmc input_polldev led_class coretemp hwmon eeprom snd_seq_oss snd_seq_midi_event snd_seq snd_seq_device snd_pcm_oss snd_mixer_oss button thermal processor sg snd_hda_intel snd_pcm snd_timer snd snd_page_alloc sky2 i2c_i801 rtc [last unloaded: crc_itu_t] Pid: 9, comm: events/0 Not tainted 2.6.25-rc2 #3 RIP: 0010:[] [] RSP: 0018:ffff81007dcdde88 EFLAGS: 00010246 RAX: ffff81007dc95040 RBX: ffff81007dee5390 RCX: 0000000000005e13 RDX: 0000000000008c8b RSI: 0000000000000001 RDI: ffff81007dee5388 RBP: ffff81007dc5eb40 R08: 0000000000000002 R09: ffffffff8022d05c R10: ffffffff8023b34c R11: ffffffff8041a353 R12: ffff81007dee5388 R13: ffffffff8807b455 R14: ffffffff80593bc0 R15: 0000000000000000 FS: 0000000000000000(0000) GS:ffffffff8055a000(0000) knlGS:0000000000000000 CS: 0010 DS: 0018 ES: 0018 CR0: 000000008005003b CR2: ffffffff8807b455 CR3: 0000000000201000 CR4: 00000000000006e0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400 Process events/0 (pid: 9, threadinfo ffff81007dcdc000, task ffff81007dc95040) Stack: ffffffff8023b396 ffffffff88082524 0000000000000000 ffffffff8807d9ae ffff81007dc5eb40 ffff81007dc9dce0 ffff81007dc5eb40 ffff81007dc5eb80 ffff81007dc9dce0 ffffffffffffffff ffffffff8023be87 0000000000000000 Call Trace: [] ? run_workqueue+0xdf/0x1df [] ? worker_thread+0xd8/0xe3 [] ? autoremove_wake_function+0x0/0x2e [] ? worker_thread+0x0/0xe3 [] ? kthread+0x47/0x74 [] ? trace_hardirqs_on_thunk+0x35/0x3a [] ? child_rip+0xa/0x12 [] ? restore_args+0x0/0x3d [] ? kthreadd+0x14c/0x171 [] ? kthreadd+0x14c/0x171 [] ? kthread+0x0/0x74 [] ? child_rip+0x0/0x12 Code: Bad RIP value. RIP [] RSP CR2: ffffffff8807b455 ---[ end trace c7366c6657fe5bed ]--- Note that this crash happened _after_ firewire-core was unloaded. The shared workqueue tried to run firewire-core's device initialization jobs or similar jobs. The fix makes sure that firewire-ohci and hence firewire-core is not unloaded before all device shutdown jobs have been completed. This is determined by the count of device initializations minus device releases. Also skip useless retries in the node initialization job if the node is to be shut down. Signed-off-by: Stefan Richter Signed-off-by: Jarod Wilson --- drivers/firewire/fw-card.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) (limited to 'drivers/firewire/fw-card.c') diff --git a/drivers/firewire/fw-card.c b/drivers/firewire/fw-card.c index e6395b29850..a03462750b9 100644 --- a/drivers/firewire/fw-card.c +++ b/drivers/firewire/fw-card.c @@ -18,6 +18,7 @@ #include #include +#include #include #include #include @@ -398,6 +399,7 @@ fw_card_initialize(struct fw_card *card, const struct fw_card_driver *driver, static atomic_t index = ATOMIC_INIT(-1); kref_init(&card->kref); + atomic_set(&card->device_count, 0); card->index = atomic_inc_return(&index); card->driver = driver; card->device = device; @@ -528,8 +530,14 @@ fw_core_remove_card(struct fw_card *card) card->driver = &dummy_driver; fw_destroy_nodes(card); - flush_scheduled_work(); + /* + * Wait for all device workqueue jobs to finish. Otherwise the + * firewire-core module could be unloaded before the jobs ran. + */ + while (atomic_read(&card->device_count) > 0) + msleep(100); + cancel_delayed_work_sync(&card->work); fw_flush_transactions(card); del_timer_sync(&card->flush_timer); -- cgit v1.2.3