linux-kernel - Re: [PATCH] usb: typec: ucsi: acpi: Remove notifier before destroying handler

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZBwikZ0wyQ1LGYBc@kuha.fi.intel.com>
Date:   Thu, 23 Mar 2023 11:57:37 +0200
From:   Heikki Krogerus <heikki.krogerus@...ux.intel.com>
To:     Mark Pearson <mpearson-lenovo@...ebb.ca>
Cc:     gregkh@...uxfoundation.org, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] usb: typec: ucsi: acpi: Remove notifier before
 destroying handler

On Tue, Mar 21, 2023 at 03:01:36PM -0400, Mark Pearson wrote:
> Was debugging another issue (since fixed) and noticed that the acpi
> notify_handler should be removed before the ucsi object is destroyed.
> 
> This isn't fixing any issues that I'm aware of - but I assume could
> potentially lead to a race condition if you were really unlucky?
> 
> Signed-off-by: Mark Pearson <mpearson-lenovo@...ebb.ca>
> ---
>  drivers/usb/typec/ucsi/ucsi_acpi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
> index ce0c8ef80c04..be3bf4f996d3 100644
> --- a/drivers/usb/typec/ucsi/ucsi_acpi.c
> +++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
> @@ -176,12 +176,12 @@ static int ucsi_acpi_remove(struct platform_device *pdev)
>  {
>  	struct ucsi_acpi *ua = platform_get_drvdata(pdev);
>  
> -	ucsi_unregister(ua->ucsi);
> -	ucsi_destroy(ua->ucsi);
> -
>  	acpi_remove_notify_handler(ACPI_HANDLE(&pdev->dev), ACPI_DEVICE_NOTIFY,
>  				   ucsi_acpi_notify);
>  
> +	ucsi_unregister(ua->ucsi);
> +	ucsi_destroy(ua->ucsi);
> +
>  	return 0;
>  }

Calling ucsi_desctroy() after removing the notifier makes sense to me,
but do you also need to unregister the instance after that?

You may still be in the middle of init or resume, so I think we need
to accept notifications until we are sure those have finished, i.e.
ucsi_unregister() has finished.

thanks,

-- 
heikki