| Message ID | 1384560238-10708-1-git-send-email-shuah.kh@samsung.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On 11/15/2013 05:21 PM, Rafael J. Wysocki wrote: > On Friday, November 15, 2013 05:03:57 PM Shuah Khan wrote: >> device_wakeup_enable() uses dev_name(dev) as the wakeup source name. >> When it gets called with a device with its name not yet set, ws structure >> with ws->name = NULL gets created. >> >> When kernel is booted with wakeup_source_activate enabled, it will panic >> when the trace point code tries to derefernces ws->name. >> >> Change device_wakeup_enable() to check for dev_name(dev) null condition >> and return -EINVAL to avoid panics when device_wakeup_enable() gets called >> before device is fully initialized with its name. return -EINVAL; > > Can you please use WARN_ON(!dev_name(dev)) here? While I agree that it is a > bad idea to crash the kernel because dev has no name, that indicates a driver > bug that shouldn't be too easy to ignore. > > Thanks! > Right. ok I will re-cut the patch with WARN_ON and send it. fyi I did send fix for the driver (power_supply) as well. http://www.kernelhub.org/?msg=362354&p=2 -- Shuah
On Fri, Nov 15, 2013 at 05:16:31PM -0700, Shuah Khan wrote: > On 11/15/2013 05:21 PM, Rafael J. Wysocki wrote: > > On Friday, November 15, 2013 05:03:57 PM Shuah Khan wrote: > >> device_wakeup_enable() uses dev_name(dev) as the wakeup source name. > >> When it gets called with a device with its name not yet set, ws structure > >> with ws->name = NULL gets created. > >> > >> When kernel is booted with wakeup_source_activate enabled, it will panic > >> when the trace point code tries to derefernces ws->name. > >> > >> Change device_wakeup_enable() to check for dev_name(dev) null condition > >> and return -EINVAL to avoid panics when device_wakeup_enable() gets called > >> before device is fully initialized with its name. > return -EINVAL; > > > > Can you please use WARN_ON(!dev_name(dev)) here? While I agree that it is a > > bad idea to crash the kernel because dev has no name, that indicates a driver > > bug that shouldn't be too easy to ignore. > > > > Thanks! > > > > Right. ok I will re-cut the patch with WARN_ON and send it. fyi I did > send fix for the driver (power_supply) as well. > > http://www.kernelhub.org/?msg=362354&p=2 Why is a driver calling kobject_set_name() instead of device_set_name()? Yes, it's really the same thing deep down, but drivers should never care about a kobject, just 'struct device'. Well, even then it usually should care about it's type of 'struct device' but that's a different issue... Anyway, not saying your patch is wrong at all, just for the future if people are looking for code cleanups... thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, November 15, 2013 05:16:31 PM Shuah Khan wrote: > On 11/15/2013 05:21 PM, Rafael J. Wysocki wrote: > > On Friday, November 15, 2013 05:03:57 PM Shuah Khan wrote: > >> device_wakeup_enable() uses dev_name(dev) as the wakeup source name. > >> When it gets called with a device with its name not yet set, ws structure > >> with ws->name = NULL gets created. > >> > >> When kernel is booted with wakeup_source_activate enabled, it will panic > >> when the trace point code tries to derefernces ws->name. > >> > >> Change device_wakeup_enable() to check for dev_name(dev) null condition > >> and return -EINVAL to avoid panics when device_wakeup_enable() gets called > >> before device is fully initialized with its name. > return -EINVAL; > > > > Can you please use WARN_ON(!dev_name(dev)) here? While I agree that it is a > > bad idea to crash the kernel because dev has no name, that indicates a driver > > bug that shouldn't be too easy to ignore. > > > > Thanks! > > > > Right. ok I will re-cut the patch with WARN_ON and send it. fyi I did > send fix for the driver (power_supply) as well. > > http://www.kernelhub.org/?msg=362354&p=2 That's OK (thanks for taking care of this), but there may be more brilliant stuff like that in the future and people should see right away that something's wrong in those cases. Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/15/2013 05:25 PM, Greg KH wrote: > On Fri, Nov 15, 2013 at 05:16:31PM -0700, Shuah Khan wrote: >> On 11/15/2013 05:21 PM, Rafael J. Wysocki wrote: >>> On Friday, November 15, 2013 05:03:57 PM Shuah Khan wrote: >>>> device_wakeup_enable() uses dev_name(dev) as the wakeup source name. >>>> When it gets called with a device with its name not yet set, ws structure >>>> with ws->name = NULL gets created. >>>> >>>> When kernel is booted with wakeup_source_activate enabled, it will panic >>>> when the trace point code tries to derefernces ws->name. >>>> >>>> Change device_wakeup_enable() to check for dev_name(dev) null condition >>>> and return -EINVAL to avoid panics when device_wakeup_enable() gets called >>>> before device is fully initialized with its name. >> return -EINVAL; >>> >>> Can you please use WARN_ON(!dev_name(dev)) here? While I agree that it is a >>> bad idea to crash the kernel because dev has no name, that indicates a driver >>> bug that shouldn't be too easy to ignore. >>> >>> Thanks! >>> >> >> Right. ok I will re-cut the patch with WARN_ON and send it. fyi I did >> send fix for the driver (power_supply) as well. >> >> http://www.kernelhub.org/?msg=362354&p=2 > > Why is a driver calling kobject_set_name() instead of device_set_name()? > > Yes, it's really the same thing deep down, but drivers should never care > about a kobject, just 'struct device'. Well, even then it usually > should care about it's type of 'struct device' but that's a different > issue... > > Anyway, not saying your patch is wrong at all, just for the future if > people are looking for code cleanups... > Use of kobject_set_name() did look odd to me. I will fix that for this driver by re-doing the patch while I am it. About the power_supply patch, stable needs fixing as well, and I will have to backport once he fix goes into the mainline. http://www.kernelhub.org/?msg=362354&p=2 as is doesn't apply to 3.10 and 3.11 -- Shuah
On 11/15/2013 05:32 PM, Rafael J. Wysocki wrote: > On Friday, November 15, 2013 05:16:31 PM Shuah Khan wrote: >> On 11/15/2013 05:21 PM, Rafael J. Wysocki wrote: >>> On Friday, November 15, 2013 05:03:57 PM Shuah Khan wrote: >>>> device_wakeup_enable() uses dev_name(dev) as the wakeup source name. >>>> When it gets called with a device with its name not yet set, ws structure >>>> with ws->name = NULL gets created. >>>> >>>> When kernel is booted with wakeup_source_activate enabled, it will panic >>>> when the trace point code tries to derefernces ws->name. >>>> >>>> Change device_wakeup_enable() to check for dev_name(dev) null condition >>>> and return -EINVAL to avoid panics when device_wakeup_enable() gets called >>>> before device is fully initialized with its name. >> return -EINVAL; >>> >>> Can you please use WARN_ON(!dev_name(dev)) here? While I agree that it is a >>> bad idea to crash the kernel because dev has no name, that indicates a driver >>> bug that shouldn't be too easy to ignore. >>> >>> Thanks! >>> >> >> Right. ok I will re-cut the patch with WARN_ON and send it. fyi I did >> send fix for the driver (power_supply) as well. >> >> http://www.kernelhub.org/?msg=362354&p=2 > > That's OK (thanks for taking care of this), but there may be more brilliant > stuff like that in the future and people should see right away that something's > wrong in those cases. > Rafael/Anton/David, I added WARN_ON and testing the patch. We have two instance of device_wakeup_enable() calls with null dev_name during boot. Both are from power_supply_register(). One when AC Adapter [ADP1] is added and the second when device: 'BAT1' gets added. [ 4.412959] device_wakeup_enable() called with null device nme [ 4.412967] device: 'ADP1': device_add [ 4.460190] device_wakeup_enable() called with null device nme [ 4.460197] device: 'BAT1': device_add I think adding WARN_ON to device_wakeup_enable() and fix to power_supply_register() should go in as a dependent patch series, to avoid panics during boot. As soon as the device_wakeup_enable() WARN_ON goes in, I suspect most laptops will panic during boot, similar to what I am seeing in my testing. Thoughts? I will send these fixes, maybe these two can be funneled through PM tree with power_supply maintainers ACK for the power_supply_register() fix. thanks, -- Shuah
diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c index 2d56f41..e086f12 100644 --- a/drivers/base/power/wakeup.c +++ b/drivers/base/power/wakeup.c @@ -220,7 +220,7 @@ int device_wakeup_enable(struct device *dev) struct wakeup_source *ws; int ret; - if (!dev || !dev->power.can_wakeup) + if (!dev || !dev->power.can_wakeup || !dev_name(dev)) return -EINVAL; ws = wakeup_source_register(dev_name(dev));
device_wakeup_enable() uses dev_name(dev) as the wakeup source name. When it gets called with a device with its name not yet set, ws structure with ws->name = NULL gets created. When kernel is booted with wakeup_source_activate enabled, it will panic when the trace point code tries to derefernces ws->name. Change device_wakeup_enable() to check for dev_name(dev) null condition and return -EINVAL to avoid panics when device_wakeup_enable() gets called before device is fully initialized with its name. The following panic resulted from power_supply_register() registering wakeup source with a null device name. [ 819.769934] device: 'BAT1': device_add [ 819.770078] PM: Adding info for No Bus:BAT1 [ 819.770235] BUG: unable to handle kernel NULL pointer dereference at (null) [ 819.770435] IP: [<ffffffff813381c0>] skip_spaces+0x30/0x30 [ 819.770572] PGD 3efd90067 PUD 3eff61067 PMD 0 [ 819.770716] Oops: 0000 [#1] SMP [ 819.770829] Modules linked in: arc4 iwldvm mac80211 x86_pkg_temp_thermal coretemp kvm_intel joydev i915 kvm uvcvideo ghash_clmulni_intel videobuf2_vmalloc aesni_intel videobuf2_memops videobuf2_core aes_x86_64 ablk_helper cryptd videodev iwlwifi lrw rfcomm gf128mul glue_helper bnep btusb media bluetooth parport_pc hid_generic ppdev snd_hda_codec_hdmi drm_kms_helper snd_hda_codec_realtek cfg80211 drm tpm_infineon samsung_laptop snd_hda_intel usbhid snd_hda_codec hid snd_hwdep snd_pcm microcode snd_page_alloc snd_timer psmouse i2c_algo_bit lpc_ich tpm_tis video wmi mac_hid serio_raw ext2 lp parport r8169 mii [ 819.771802] CPU: 0 PID: 2167 Comm: bash Not tainted 3.12.0+ #25 [ 819.771876] Hardware name: SAMSUNG ELECTRONICS CO., LTD. 900X3C/900X3D/900X4C/900X4D/SAMSUNG_NP1234567890, BIOS P03AAC 07/12/2012 [ 819.772022] task: ffff88002e6ddcc0 ti: ffff8804015ca000 task.ti: ffff8804015ca000 [ 819.772119] RIP: 0010:[<ffffffff813381c0>] [<ffffffff813381c0>] skip_spaces+0x30/0x30 [ 819.772242] RSP: 0018:ffff8804015cbc70 EFLAGS: 00010046 [ 819.772310] RAX: 0000000000000003 RBX: ffff88040cfd6d40 RCX: 0000000000000018 [ 819.772397] RDX: 0000000000020001 RSI: 0000000000000000 RDI: 0000000000000000 [ 819.772484] RBP: ffff8804015cbcc0 R08: 0000000000000000 R09: ffff8803f0768d40 [ 819.772570] R10: ffffea001033b800 R11: 0000000000000000 R12: ffffffff81c519c0 [ 819.772656] R13: 0000000000020001 R14: 0000000000000000 R15: 0000000000020001 [ 819.772744] FS: 00007ff98309b740(0000) GS:ffff88041f200000(0000) knlGS:0000000000000000 [ 819.772845] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 819.772917] CR2: 0000000000000000 CR3: 00000003f59dc000 CR4: 00000000001407f0 [ 819.773001] Stack: [ 819.773030] ffffffff81114003 ffff8804015cbcb0 0000000000000000 0000000000000046 [ 819.773146] ffff880409757a18 ffff8803f065a160 0000000000000000 0000000000020001 [ 819.773273] 0000000000000000 0000000000000000 ffff8804015cbce8 ffffffff8143e388 [ 819.773387] Call Trace: [ 819.773434] [<ffffffff81114003>] ? ftrace_raw_event_wakeup_source+0x43/0xe0 [ 819.773520] [<ffffffff8143e388>] wakeup_source_report_event+0xb8/0xd0 [ 819.773595] [<ffffffff8143e3cd>] __pm_stay_awake+0x2d/0x50 [ 819.773724] [<ffffffff8153395c>] power_supply_changed+0x3c/0x90 [ 819.773795] [<ffffffff8153407c>] power_supply_register+0x18c/0x250 [ 819.773869] [<ffffffff813d8d18>] sysfs_add_battery+0x61/0x7b [ 819.773935] [<ffffffff813d8d69>] battery_notify+0x37/0x3f [ 819.774001] [<ffffffff816ccb7c>] notifier_call_chain+0x4c/0x70 [ 819.774071] [<ffffffff81073ded>] __blocking_notifier_call_chain+0x4d/0x70 [ 819.774149] [<ffffffff81073e26>] blocking_notifier_call_chain+0x16/0x20 [ 819.774227] [<ffffffff8109397a>] pm_notifier_call_chain+0x1a/0x40 [ 819.774316] [<ffffffff81095b66>] hibernate+0x66/0x1c0 [ 819.774407] [<ffffffff81093931>] state_store+0x71/0xa0 [ 819.774507] [<ffffffff81331d8f>] kobj_attr_store+0xf/0x20 [ 819.774613] [<ffffffff811f8618>] sysfs_write_file+0x128/0x1c0 [ 819.774735] [<ffffffff8118579d>] vfs_write+0xbd/0x1e0 [ 819.774841] [<ffffffff811861d9>] SyS_write+0x49/0xa0 [ 819.774939] [<ffffffff816d1052>] system_call_fastpath+0x16/0x1b [ 819.775055] Code: 89 f8 48 89 e5 f6 82 c0 a6 84 81 20 74 15 0f 1f 44 00 00 48 83 c0 01 0f b6 10 f6 82 c0 a6 84 81 20 75 f0 5d c3 66 0f 1f 44 00 00 <80> 3f 00 55 48 89 e5 74 15 48 89 f8 0f 1f 40 00 48 83 c0 01 80 [ 819.775760] RIP [<ffffffff813381c0>] skip_spaces+0x30/0x30 [ 819.775881] RSP <ffff8804015cbc70> [ 819.775949] CR2: 0000000000000000 [ 819.794175] ---[ end trace c4ef25127039952e ]--- Signed-off-by: Shuah Khan <shuah.kh@samsung.com> Cc: stable@vger.kernel.org --- drivers/base/power/wakeup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)