[PATCH] platform/chrome: cros_usbpd_notify: Fix error handling in cros_usbpd_notify_init()

[PATCH] platform/chrome: cros_usbpd_notify: Fix error handling in cros_usbpd_notify_init()

[Yuan Can]

The following WARNING message was given when rmmod cros_usbpd_notify:

 Unexpected driver unregister!
 WARNING: CPU: 0 PID: 253 at drivers/base/driver.c:270 driver_unregister+0x8a/0xb0
 Modules linked in: cros_usbpd_notify(-)
 CPU: 0 PID: 253 Comm: rmmod Not tainted 6.1.0-rc3 #24
 ...
 Call Trace:
  <TASK>
  cros_usbpd_notify_exit+0x11/0x1e [cros_usbpd_notify]
  __x64_sys_delete_module+0x3c7/0x570
  ? __ia32_sys_delete_module+0x570/0x570
  ? lock_is_held_type+0xe3/0x140
  ? syscall_enter_from_user_mode+0x17/0x50
  ? rcu_read_lock_sched_held+0xa0/0xd0
  ? syscall_enter_from_user_mode+0x1c/0x50
  do_syscall_64+0x37/0x90
  entry_SYSCALL_64_after_hwframe+0x63/0xcd
 RIP: 0033:0x7f333fe9b1b7

The reason is that the cros_usbpd_notify_init() does not check the return
value of platform_driver_register(), and the cros_usbpd_notify can
install successfully even if platform_driver_register() failed.

Fix by checking the return value of platform_driver_register() and
unregister cros_usbpd_notify_plat_driver when it failed.

Fixes: ec2daf6e33f9 ("platform: chrome: Add cros-usbpd-notify driver")
Signed-off-by: Yuan Can <yuancan@huawei.com>
---
 drivers/platform/chrome/cros_usbpd_notify.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/chrome/cros_usbpd_notify.c b/drivers/platform/chrome/cros_usbpd_notify.c
index 4b5a81c9dc6d..10670b6588e3 100644
--- a/drivers/platform/chrome/cros_usbpd_notify.c
+++ b/drivers/platform/chrome/cros_usbpd_notify.c
@@ -239,7 +239,11 @@ static int __init cros_usbpd_notify_init(void)
 		return ret;
 
 #ifdef CONFIG_ACPI
-	platform_driver_register(&cros_usbpd_notify_acpi_driver);
+	ret = platform_driver_register(&cros_usbpd_notify_acpi_driver);
+	if (ret) {
+		platform_driver_unregister(&cros_usbpd_notify_plat_driver);
+		return ret;
+	}
 #endif
 	return 0;
 }
-- 
2.17.1

Re: [PATCH] platform/chrome: cros_usbpd_notify: Fix error handling in cros_usbpd_notify_init()

[Prashant Malani]

Hi Yuan,

On Thu, Nov 17, 2022 at 12:10 AM Yuan Can <yuancan@huawei.com> wrote:
>
> The following WARNING message was given when rmmod cros_usbpd_notify:
>
>  Unexpected driver unregister!
>  WARNING: CPU: 0 PID: 253 at drivers/base/driver.c:270 driver_unregister+0x8a/0xb0
>  Modules linked in: cros_usbpd_notify(-)
>  CPU: 0 PID: 253 Comm: rmmod Not tainted 6.1.0-rc3 #24
>  ...
>  Call Trace:
>   <TASK>
>   cros_usbpd_notify_exit+0x11/0x1e [cros_usbpd_notify]
>   __x64_sys_delete_module+0x3c7/0x570
>   ? __ia32_sys_delete_module+0x570/0x570
>   ? lock_is_held_type+0xe3/0x140
>   ? syscall_enter_from_user_mode+0x17/0x50
>   ? rcu_read_lock_sched_held+0xa0/0xd0
>   ? syscall_enter_from_user_mode+0x1c/0x50
>   do_syscall_64+0x37/0x90
>   entry_SYSCALL_64_after_hwframe+0x63/0xcd
>  RIP: 0033:0x7f333fe9b1b7
>
> The reason is that the cros_usbpd_notify_init() does not check the return
> value of platform_driver_register(), and the cros_usbpd_notify can
> install successfully even if platform_driver_register() failed.
>
> Fix by checking the return value of platform_driver_register() and

Can you provide details regarding why cros_usbpd_notify_acpi_driver registration
is failing on your system (logs or other details) ?

Thanks,

- Prashant

Re: [PATCH] platform/chrome: cros_usbpd_notify: Fix error handling in cros_usbpd_notify_init()

[Brian Norris]

On Thu, Nov 17, 2022 at 08:08:23AM +0000, Yuan Can wrote:
> The following WARNING message was given when rmmod cros_usbpd_notify:
> 
>  Unexpected driver unregister!
>  WARNING: CPU: 0 PID: 253 at drivers/base/driver.c:270 driver_unregister+0x8a/0xb0
>  Modules linked in: cros_usbpd_notify(-)
>  CPU: 0 PID: 253 Comm: rmmod Not tainted 6.1.0-rc3 #24
>  ...
>  Call Trace:
>   <TASK>
>   cros_usbpd_notify_exit+0x11/0x1e [cros_usbpd_notify]
>   __x64_sys_delete_module+0x3c7/0x570
>   ? __ia32_sys_delete_module+0x570/0x570
>   ? lock_is_held_type+0xe3/0x140
>   ? syscall_enter_from_user_mode+0x17/0x50
>   ? rcu_read_lock_sched_held+0xa0/0xd0
>   ? syscall_enter_from_user_mode+0x1c/0x50
>   do_syscall_64+0x37/0x90
>   entry_SYSCALL_64_after_hwframe+0x63/0xcd
>  RIP: 0033:0x7f333fe9b1b7
> 
> The reason is that the cros_usbpd_notify_init() does not check the return
> value of platform_driver_register(), and the cros_usbpd_notify can
> install successfully even if platform_driver_register() failed.
> 
> Fix by checking the return value of platform_driver_register() and
> unregister cros_usbpd_notify_plat_driver when it failed.

On first glance, the last part looked wrong (if register() fails, we
don't need to call unregister()), but these are two different drivers
getting registered. Looks good to me, then.

> Fixes: ec2daf6e33f9 ("platform: chrome: Add cros-usbpd-notify driver")
> Signed-off-by: Yuan Can <yuancan@huawei.com>

Reviewed-by: Brian Norris <briannorris@chromium.org>

> ---
>  drivers/platform/chrome/cros_usbpd_notify.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/chrome/cros_usbpd_notify.c b/drivers/platform/chrome/cros_usbpd_notify.c
> index 4b5a81c9dc6d..10670b6588e3 100644
> --- a/drivers/platform/chrome/cros_usbpd_notify.c
> +++ b/drivers/platform/chrome/cros_usbpd_notify.c
> @@ -239,7 +239,11 @@ static int __init cros_usbpd_notify_init(void)
>  		return ret;
>  
>  #ifdef CONFIG_ACPI
> -	platform_driver_register(&cros_usbpd_notify_acpi_driver);
> +	ret = platform_driver_register(&cros_usbpd_notify_acpi_driver);
> +	if (ret) {
> +		platform_driver_unregister(&cros_usbpd_notify_plat_driver);
> +		return ret;
> +	}
>  #endif
>  	return 0;
>  }
> -- 
> 2.17.1
> 
> 

Re: [PATCH] platform/chrome: cros_usbpd_notify: Fix error handling in cros_usbpd_notify_init()

[Yuan Can]

在 2022/11/18 3:07, Prashant Malani 写道:
> Hi Yuan,
>
> On Thu, Nov 17, 2022 at 12:10 AM Yuan Can <yuancan@huawei.com> wrote:
>> The following WARNING message was given when rmmod cros_usbpd_notify:
>>
>>   Unexpected driver unregister!
>>   WARNING: CPU: 0 PID: 253 at drivers/base/driver.c:270 driver_unregister+0x8a/0xb0
>>   Modules linked in: cros_usbpd_notify(-)
>>   CPU: 0 PID: 253 Comm: rmmod Not tainted 6.1.0-rc3 #24
>>   ...
>>   Call Trace:
>>    <TASK>
>>    cros_usbpd_notify_exit+0x11/0x1e [cros_usbpd_notify]
>>    __x64_sys_delete_module+0x3c7/0x570
>>    ? __ia32_sys_delete_module+0x570/0x570
>>    ? lock_is_held_type+0xe3/0x140
>>    ? syscall_enter_from_user_mode+0x17/0x50
>>    ? rcu_read_lock_sched_held+0xa0/0xd0
>>    ? syscall_enter_from_user_mode+0x1c/0x50
>>    do_syscall_64+0x37/0x90
>>    entry_SYSCALL_64_after_hwframe+0x63/0xcd
>>   RIP: 0033:0x7f333fe9b1b7
>>
>> The reason is that the cros_usbpd_notify_init() does not check the return
>> value of platform_driver_register(), and the cros_usbpd_notify can
>> install successfully even if platform_driver_register() failed.
>>
>> Fix by checking the return value of platform_driver_register() and
> Can you provide details regarding why cros_usbpd_notify_acpi_driver registration
> is failing on your system (logs or other details) ?

The reason could be an OOM happend when platform_driver_register() tries 
to allocate memory,

a simple call graph:

  platform_driver_register()
    driver_register()
      bus_add_driver()
        dev = kzalloc(...) # OOM happened

-- 
Best regards,
Yuan Can

Re: [PATCH] platform/chrome: cros_usbpd_notify: Fix error handling in cros_usbpd_notify_init()

[Prashant Malani]

On Fri, Nov 18, 2022 at 2:18 AM Yuan Can <yuancan@huawei.com> wrote:
>
>
> 在 2022/11/18 3:07, Prashant Malani 写道:
> > Hi Yuan,
> >
> > On Thu, Nov 17, 2022 at 12:10 AM Yuan Can <yuancan@huawei.com> wrote:
> >> The reason is that the cros_usbpd_notify_init() does not check the return
> >> value of platform_driver_register(), and the cros_usbpd_notify can
> >> install successfully even if platform_driver_register() failed.
> >>
> >> Fix by checking the return value of platform_driver_register() and
> > Can you provide details regarding why cros_usbpd_notify_acpi_driver registration
> > is failing on your system (logs or other details) ?
>
> The reason could be an OOM happend when platform_driver_register() tries
> to allocate memory,
>
> a simple call graph:
>
>   platform_driver_register()
>     driver_register()
>       bus_add_driver()
>         dev = kzalloc(...) # OOM happened
>

Sure, but is that what you're seeing? Can you share logs from your repro case?

I've got no problems with the change itself, but find the
platform_driver_register()
failure to be quite odd here.

Thanks,

Re: [PATCH] platform/chrome: cros_usbpd_notify: Fix error handling in cros_usbpd_notify_init()

[Yuan Can]

在 2022/11/19 4:11, Prashant Malani 写道:
> On Fri, Nov 18, 2022 at 2:18 AM Yuan Can <yuancan@huawei.com> wrote:
>>
>> 在 2022/11/18 3:07, Prashant Malani 写道:
>>> Hi Yuan,
>>>
>>> On Thu, Nov 17, 2022 at 12:10 AM Yuan Can <yuancan@huawei.com> wrote:
>>>> The reason is that the cros_usbpd_notify_init() does not check the return
>>>> value of platform_driver_register(), and the cros_usbpd_notify can
>>>> install successfully even if platform_driver_register() failed.
>>>>
>>>> Fix by checking the return value of platform_driver_register() and
>>> Can you provide details regarding why cros_usbpd_notify_acpi_driver registration
>>> is failing on your system (logs or other details) ?
>> The reason could be an OOM happend when platform_driver_register() tries
>> to allocate memory,
>>
>> a simple call graph:
>>
>>    platform_driver_register()
>>      driver_register()
>>        bus_add_driver()
>>          dev = kzalloc(...) # OOM happened
>>
> Sure, but is that what you're seeing? Can you share logs from your repro case?
Yes, that the reason, this problem is triggered by the OOM 
fault-injection test.

-- 
Best regards,
Yuan Can

Re: [PATCH] platform/chrome: cros_usbpd_notify: Fix error handling in cros_usbpd_notify_init()

[Prashant Malani]

On Wed, Nov 23, 2022 at 12:33 AM Yuan Can <yuancan@huawei.com> wrote:
>
>
> 在 2022/11/19 4:11, Prashant Malani 写道:
> > On Fri, Nov 18, 2022 at 2:18 AM Yuan Can <yuancan@huawei.com> wrote:
> >>
> >> 在 2022/11/18 3:07, Prashant Malani 写道:
> >>> Hi Yuan,
> >>>
> >>> On Thu, Nov 17, 2022 at 12:10 AM Yuan Can <yuancan@huawei.com> wrote:
> >>>> The reason is that the cros_usbpd_notify_init() does not check the return
> >>>> value of platform_driver_register(), and the cros_usbpd_notify can
> >>>> install successfully even if platform_driver_register() failed.
> >>>>
> >>>> Fix by checking the return value of platform_driver_register() and
> >>> Can you provide details regarding why cros_usbpd_notify_acpi_driver registration
> >>> is failing on your system (logs or other details) ?
> >> The reason could be an OOM happend when platform_driver_register() tries
> >> to allocate memory,
> >>
> >> a simple call graph:
> >>
> >>    platform_driver_register()
> >>      driver_register()
> >>        bus_add_driver()
> >>          dev = kzalloc(...) # OOM happened
> >>
> > Sure, but is that what you're seeing? Can you share logs from your repro case?
> Yes, that the reason, this problem is triggered by the OOM
> fault-injection test.

Alright. Thanks for providing that detail.

Best regards,

-Prashant

Re: [PATCH] platform/chrome: cros_usbpd_notify: Fix error handling in cros_usbpd_notify_init()

[patchwork-bot+chrome-platform]

Hello:

This patch was applied to chrome-platform/linux.git (for-kernelci)
by Prashant Malani <pmalani@chromium.org>:

On Thu, 17 Nov 2022 08:08:23 +0000 you wrote:
> The following WARNING message was given when rmmod cros_usbpd_notify:
> 
>  Unexpected driver unregister!
>  WARNING: CPU: 0 PID: 253 at drivers/base/driver.c:270 driver_unregister+0x8a/0xb0
>  Modules linked in: cros_usbpd_notify(-)
>  CPU: 0 PID: 253 Comm: rmmod Not tainted 6.1.0-rc3 #24
>  ...
>  Call Trace:
>   <TASK>
>   cros_usbpd_notify_exit+0x11/0x1e [cros_usbpd_notify]
>   __x64_sys_delete_module+0x3c7/0x570
>   ? __ia32_sys_delete_module+0x570/0x570
>   ? lock_is_held_type+0xe3/0x140
>   ? syscall_enter_from_user_mode+0x17/0x50
>   ? rcu_read_lock_sched_held+0xa0/0xd0
>   ? syscall_enter_from_user_mode+0x1c/0x50
>   do_syscall_64+0x37/0x90
>   entry_SYSCALL_64_after_hwframe+0x63/0xcd
>  RIP: 0033:0x7f333fe9b1b7
> 
> [...]

Here is the summary with links:
  - platform/chrome: cros_usbpd_notify: Fix error handling in cros_usbpd_notify_init()
    https://git.kernel.org/chrome-platform/c/5a2d96623670

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH] platform/chrome: cros_usbpd_notify: Fix error handling in cros_usbpd_notify_init()

[patchwork-bot+chrome-platform]

Hello:

This patch was applied to chrome-platform/linux.git (for-next)
by Prashant Malani <pmalani@chromium.org>:

On Thu, 17 Nov 2022 08:08:23 +0000 you wrote:
> The following WARNING message was given when rmmod cros_usbpd_notify:
> 
>  Unexpected driver unregister!
>  WARNING: CPU: 0 PID: 253 at drivers/base/driver.c:270 driver_unregister+0x8a/0xb0
>  Modules linked in: cros_usbpd_notify(-)
>  CPU: 0 PID: 253 Comm: rmmod Not tainted 6.1.0-rc3 #24
>  ...
>  Call Trace:
>   <TASK>
>   cros_usbpd_notify_exit+0x11/0x1e [cros_usbpd_notify]
>   __x64_sys_delete_module+0x3c7/0x570
>   ? __ia32_sys_delete_module+0x570/0x570
>   ? lock_is_held_type+0xe3/0x140
>   ? syscall_enter_from_user_mode+0x17/0x50
>   ? rcu_read_lock_sched_held+0xa0/0xd0
>   ? syscall_enter_from_user_mode+0x1c/0x50
>   do_syscall_64+0x37/0x90
>   entry_SYSCALL_64_after_hwframe+0x63/0xcd
>  RIP: 0033:0x7f333fe9b1b7
> 
> [...]

Here is the summary with links:
  - platform/chrome: cros_usbpd_notify: Fix error handling in cros_usbpd_notify_init()
    https://git.kernel.org/chrome-platform/c/5a2d96623670

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html