| Message ID | 20170208134633.5152-9-kernel@kempniu.pl (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
Michael On Mon, Feb 13, 2017 at 10:40:15AM +0800, kernel test robot wrote: > FYI, we noticed the following commit: > > commit: b925ff7dcd1fc45b86baaebd3442f8b484123716 ("platform/x86: fujitsu-laptop: only register backlight device if FUJ02B1 is present") > url: https://github.com/0day-ci/linux/commits/Micha-K-pie/fujitsu-laptop-renames-and-cleanups/20170209-030748 > base: git://git.infradead.org/users/dvhart/linux-platform-drivers-x86.git for-next > > in testcase: boot > > on test machine: qemu-system-i386 -enable-kvm -cpu Haswell,+smep,+smap -m 360M > > caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace): > : > [ 4.656202] fujitsu_laptop: call_fext_func: FUNC interface is not present > [ 4.657478] BUG: unable to handle kernel NULL pointer dereference at 00000008 > [ 4.658433] IP: fujitsu_init+0x137/0x1b7 > [ 4.659208] *pdpt = 0000000000000000 *pde = f000ff53f000ff53 > : I'm away from my Fujitsu hardware right now, and in any case this does not get triggered on it. I note that prior to the bug we get "FUNC interface is not present". Therefore something called call_fext_func() some time before the NULL pointer dereference. As far as I can tell the only such call which could be made prior to or within fujitsu_init() is from fujitsu_init(), but this is conditional on acpi_video_get_backlight_type() returning acpi_backlight_vendor (line 1269). Obviously if this conditional were passed but fujitsu_bl->bl_device were NULL then we would get the NULL dereference. If ACPI_FUJITSU_LAPTOP_HID is not present then presumedly the acpi_bus_register_driver(&acpi_fujitsu_bl_driver) call in fujitsu_init() will fail and nothing further would happen. Therefore this HID must be in the system. However, the acpi_fujitsu_bl_add() callback wouldn't necessarily get run by acpi_bus_register_driver(), would it? I'm not too familiar with the lower level ACPI functions but a quick trip through the source suggested that the add callback isn't called via acpi_bus_register_driver(). This would mean that that fujitsu_bl->bl_device would not yet be initialised when referenced within fujitsu_init() at line 1271 or 1273. If this were the case then I see two options: 1) Don't move the backlight registration out of fujitsu_init(). 2) Move the remaining backlight code (lines 1268-1274) into acpi_fujitsu_bl_add(). Item 1 effectively amounts to dropping this commit. I'm not sure option 2 is workable because of the code's reliance on FUJ02E3; is that guaranteed to be useable by the time acpi_fujitsu_bl_add() is called? The only problem with the above theory is that it doesn't explain why the NULL pointer dereference wasn't triggered on my Fujitsu hardware when this code was tested on it. If the ACPI bus probed/added asynchronously I guess there could be a race whereby acpi_fujitsu_bl_add() may or may not have completed by the time fujitsu_init() referenced fujitsu_bl->bl_device. That doesn't seem right to me though. Regards jonathan
> Michael > > On Mon, Feb 13, 2017 at 10:40:15AM +0800, kernel test robot wrote: > > FYI, we noticed the following commit: > > > > commit: b925ff7dcd1fc45b86baaebd3442f8b484123716 ("platform/x86: fujitsu-laptop: only register backlight device if FUJ02B1 is present") > > url: https://github.com/0day-ci/linux/commits/Micha-K-pie/fujitsu-laptop-renames-and-cleanups/20170209-030748 > > base: git://git.infradead.org/users/dvhart/linux-platform-drivers-x86.git for-next > > > > in testcase: boot > > > > on test machine: qemu-system-i386 -enable-kvm -cpu Haswell,+smep,+smap -m 360M > > > > caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace): > > : > > [ 4.656202] fujitsu_laptop: call_fext_func: FUNC interface is not present > > [ 4.657478] BUG: unable to handle kernel NULL pointer dereference at 00000008 > > [ 4.658433] IP: fujitsu_init+0x137/0x1b7 > > [ 4.659208] *pdpt = 0000000000000000 *pde = f000ff53f000ff53 > > : > > I'm away from my Fujitsu hardware right now, and in any case this does not > get triggered on it. > > I note that prior to the bug we get "FUNC interface is not present". > Therefore something called call_fext_func() some time before the NULL > pointer dereference. As far as I can tell the only such call which could be > made prior to or within fujitsu_init() is from fujitsu_init(), but this is > conditional on acpi_video_get_backlight_type() returning > acpi_backlight_vendor (line 1269). Obviously if this conditional were > passed but fujitsu_bl->bl_device were NULL then we would get the NULL > dereference. Yes, this is exactly what is happening. > > If ACPI_FUJITSU_LAPTOP_HID I think you meant ACPI_FUJITSU_BL_HID. > is not present then presumedly the > > acpi_bus_register_driver(&acpi_fujitsu_bl_driver) > > call in fujitsu_init() will fail and nothing further would happen. > Therefore this HID must be in the system. Not really. acpi_bus_register_driver() is just a wrapper around driver_register(). In other words, whether or not a given HID is present in the firmware does not have any influence on the return code of that function. In fact, the bug only happens when ACPI_FUJITSU_BL_HID is _not_ present. Or, putting it differently, on all Skylake machines (when acpi_backlight=vendor). Once again I am really glad the kernel test robot is there to counter my carelessness... > > However, the acpi_fujitsu_bl_add() callback wouldn't necessarily get run by > acpi_bus_register_driver(), would it? I'm not too familiar with the lower > level ACPI functions but a quick trip through the source suggested that the > add callback isn't called via acpi_bus_register_driver(). This would mean > that that fujitsu_bl->bl_device would not yet be initialised when referenced > within fujitsu_init() at line 1271 or 1273. If this were the case then I > see two options: > > 1) Don't move the backlight registration out of fujitsu_init(). > > 2) Move the remaining backlight code (lines 1268-1274) into > acpi_fujitsu_bl_add(). > > Item 1 effectively amounts to dropping this commit. I'm not sure option 2 > is workable because of the code's reliance on FUJ02E3; is that guaranteed to > be useable by the time acpi_fujitsu_bl_add() is called? To keep things simple, I think we should drop this particular patch for now. Darren, Andy, could you skip it when applying this series? Patches 9 and 10 do not rely on this one being applied. Thanks and sorry for the trouble. v2 of my fujitsu_init() cleanup series will fix this properly. > > The only problem with the above theory is that it doesn't explain why the > NULL pointer dereference wasn't triggered on my Fujitsu hardware when this > code was tested on it. Because the bug is not triggered as long as FUJ02B1 is present. > If the ACPI bus probed/added asynchronously I guess > there could be a race whereby acpi_fujitsu_bl_add() may or may not have > completed by the time fujitsu_init() referenced fujitsu_bl->bl_device. That > doesn't seem right to me though. When acpi_bus_register_driver() is called, the .add callback is "synchronously" called for all ACPI devices handled by the registered driver that are yet unbound to any driver. So if FUJ02B1 is present, acpi_fujitsu_bl_add() is called and bl_device is allocated. However, if that ACPI device is not present (like on Skylakes) and acpi_backlight=vendor, we get a NULL dereference.
On Mon, Feb 13, 2017 at 09:14:40AM +0100, Micha?? K??pie?? wrote: > > On Mon, Feb 13, 2017 at 10:40:15AM +0800, kernel test robot wrote: > > > FYI, we noticed the following commit: > > > > > > commit: b925ff7dcd1fc45b86baaebd3442f8b484123716 ("platform/x86: fujitsu-laptop: only register backlight device if FUJ02B1 is present") > > > url: https://github.com/0day-ci/linux/commits/Micha-K-pie/fujitsu-laptop-renames-and-cleanups/20170209-030748 > > > base: git://git.infradead.org/users/dvhart/linux-platform-drivers-x86.git for-next > > > > > > in testcase: boot > > > > > > on test machine: qemu-system-i386 -enable-kvm -cpu Haswell,+smep,+smap -m 360M > > > > > > caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace): > > > : > > > [ 4.656202] fujitsu_laptop: call_fext_func: FUNC interface is not present > > > [ 4.657478] BUG: unable to handle kernel NULL pointer dereference at 00000008 > > > [ 4.658433] IP: fujitsu_init+0x137/0x1b7 > > > [ 4.659208] *pdpt = 0000000000000000 *pde = f000ff53f000ff53 > > > : > : > > If ACPI_FUJITSU_LAPTOP_HID > > I think you meant ACPI_FUJITSU_BL_HID. I did. Darn cut and paste. > > is not present then presumedly the > > > > acpi_bus_register_driver(&acpi_fujitsu_bl_driver) > > > > call in fujitsu_init() will fail and nothing further would happen. > > Therefore this HID must be in the system. > > Not really. acpi_bus_register_driver() is just a wrapper around > driver_register(). > In other words, whether or not a given HID is present in the firmware does > not have any influence on the return code of that function. Yes, I saw that much but erroneously assumed there was an indication of device presence in the return value. Thanks for putting me right on this. > > However, the acpi_fujitsu_bl_add() callback wouldn't necessarily get run by > > acpi_bus_register_driver(), would it? I'm not too familiar with the lower > > level ACPI functions but a quick trip through the source suggested that the > > add callback isn't called via acpi_bus_register_driver(). This would mean > > that that fujitsu_bl->bl_device would not yet be initialised when referenced > > within fujitsu_init() at line 1271 or 1273. If this were the case then I > > see two options: > > > > 1) Don't move the backlight registration out of fujitsu_init(). > > > > 2) Move the remaining backlight code (lines 1268-1274) into > > acpi_fujitsu_bl_add(). > > > > Item 1 effectively amounts to dropping this commit. I'm not sure option 2 > > is workable because of the code's reliance on FUJ02E3; is that guaranteed to > > be useable by the time acpi_fujitsu_bl_add() is called? > > To keep things simple, I think we should drop this particular patch for > now. Darren, Andy, could you skip it when applying this series? > Patches 9 and 10 do not rely on this one being applied. Thanks and > sorry for the trouble. v2 of my fujitsu_init() cleanup series will fix > this properly. We could just add a test for NULL on fujitsu_bl->bl_device in fujitsu_init(), couldn't we? However, if you are planning to make further structural changes to fujitsu_init() in the upcoming patch series then I agree that messing around with this now is kind of pointless. In this case, skipping this single patch is fine. > > If the ACPI bus probed/added asynchronously I guess > > there could be a race whereby acpi_fujitsu_bl_add() may or may not have > > completed by the time fujitsu_init() referenced fujitsu_bl->bl_device. That > > doesn't seem right to me though. > > When acpi_bus_register_driver() is called, the .add callback is > "synchronously" called for all ACPI devices handled by the registered > driver that are yet unbound to any driver. That's what I first thought would be the case, but I couldn't find how the add method was called by register_driver(). I therefore thought it must be triggered at some later stage (although an async mechanism seemed a bit out there). Clearly there's an indirect mechanism that I missed. > So if FUJ02B1 is present, > acpi_fujitsu_bl_add() is called and bl_device is allocated. However, if > that ACPI device is not present (like on Skylakes) and > acpi_backlight=vendor, we get a NULL dereference. Yes, that makes sense in light of the fact that acpi_bus_register_driver() can cause acpi_fujitsu_bl_add() to be called. Regards jonathan
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c index a5478a011b90..8247abd4658f 100644 --- a/drivers/platform/x86/fujitsu-laptop.c +++ b/drivers/platform/x86/fujitsu-laptop.c @@ -762,6 +762,23 @@ static int acpi_fujitsu_bl_add(struct acpi_device *device) fujitsu_bl->max_brightness = FUJITSU_LCD_N_LEVELS; get_lcd_level(); + if (acpi_video_get_backlight_type() == acpi_backlight_vendor) { + struct backlight_properties props; + + memset(&props, 0, sizeof(struct backlight_properties)); + props.type = BACKLIGHT_PLATFORM; + props.max_brightness = fujitsu_bl->max_brightness - 1; + props.brightness = fujitsu_bl->brightness_level; + fujitsu_bl->bl_device = + backlight_device_register("fujitsu-laptop", NULL, NULL, + &fujitsu_bl_ops, &props); + if (IS_ERR(fujitsu_bl->bl_device)) { + error = PTR_ERR(fujitsu_bl->bl_device); + fujitsu_bl->bl_device = NULL; + goto err_unregister_input_dev; + } + } + return 0; err_unregister_input_dev: @@ -778,6 +795,9 @@ static int acpi_fujitsu_bl_remove(struct acpi_device *device) struct fujitsu_bl *fujitsu_bl = acpi_driver_data(device); struct input_dev *input = fujitsu_bl->input; + if (fujitsu_bl->bl_device) + backlight_device_unregister(fujitsu_bl->bl_device); + input_unregister_device(input); fujitsu_bl->acpi_handle = NULL; @@ -1192,7 +1212,7 @@ MODULE_DEVICE_TABLE(acpi, fujitsu_ids); static int __init fujitsu_init(void) { - int ret, max_brightness; + int ret; if (acpi_disabled) return -ENODEV; @@ -1229,30 +1249,9 @@ static int __init fujitsu_init(void) if (ret) goto fail_platform_device2; - /* Register backlight stuff */ - - if (acpi_video_get_backlight_type() == acpi_backlight_vendor) { - struct backlight_properties props; - - memset(&props, 0, sizeof(struct backlight_properties)); - max_brightness = fujitsu_bl->max_brightness; - props.type = BACKLIGHT_PLATFORM; - props.max_brightness = max_brightness - 1; - fujitsu_bl->bl_device = backlight_device_register("fujitsu-laptop", - NULL, NULL, - &fujitsu_bl_ops, - &props); - if (IS_ERR(fujitsu_bl->bl_device)) { - ret = PTR_ERR(fujitsu_bl->bl_device); - fujitsu_bl->bl_device = NULL; - goto fail_sysfs_group; - } - fujitsu_bl->bl_device->props.brightness = fujitsu_bl->brightness_level; - } - ret = platform_driver_register(&fujitsu_pf_driver); if (ret) - goto fail_backlight; + goto fail_sysfs_group; /* Register laptop driver */ @@ -1282,8 +1281,6 @@ static int __init fujitsu_init(void) kfree(fujitsu_laptop); fail_laptop: platform_driver_unregister(&fujitsu_pf_driver); -fail_backlight: - backlight_device_unregister(fujitsu_bl->bl_device); fail_sysfs_group: sysfs_remove_group(&fujitsu_bl->pf_device->dev.kobj, &fujitsu_pf_attribute_group); @@ -1307,8 +1304,6 @@ static void __exit fujitsu_cleanup(void) platform_driver_unregister(&fujitsu_pf_driver); - backlight_device_unregister(fujitsu_bl->bl_device); - sysfs_remove_group(&fujitsu_bl->pf_device->dev.kobj, &fujitsu_pf_attribute_group);