linux-kernel - Re: [PATCH v1] driver core: Ignore driver_async

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 for Android: free password hash cracker in your pocket

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

Message-ID: <YrQtd5q+Q8i21gSF@kroah.com>
Date:   Thu, 23 Jun 2022 11:08:07 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Saravana Kannan <saravanak@...gle.com>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>, kernel-team@...roid.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] driver core: Ignore driver_async_probe cmdline param
 for module drivers

On Wed, Jun 22, 2022 at 09:21:50PM -0700, Saravana Kannan wrote:
> If the module's async_probe option isn't set, the module loading code
> will do a async_synchronize_full() before returning to userspace. This
> effectively negates any benefits of driver_async_probe listing the
> module's driver.
> 
> If the module's async_probe is set, then we already do async probe for
> that module's driver even if driver_async_probe does not list that
> driver. So, again, the driver_async_probe's value doesn't matter for a
> module's driver.
> 
> So this change ignores the driver_async_probe for module drivers to
> avoid useless async probes.
> 
> In addition, if driver_async_probe lists a module's driver and the
> driver's async probe ends up calling request_module() in the async
> thread context, that's going to trigger a spurious WARNING stack dump
> without any real benefits of async probing. So this will avoid that
> unnecessary warning in that situation.
> 
> Signed-off-by: Saravana Kannan <saravanak@...gle.com>

Does this "fix" an older commit?  Does this need to be backported
anywhere?  Or is this just a new feature to help make things work
properly going forward?

> ---
>  drivers/base/dd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index e600dd2afc35..f1ac28a4ce62 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -823,7 +823,7 @@ bool driver_allows_async_probing(struct device_driver *drv)
>  		return false;
>  
>  	default:
> -		if (cmdline_requested_async_probing(drv->name))
> +		if (cmdline_requested_async_probing(drv->name) && drv->owner == NULL)

This feels odd, the module owner shouldn't be used for anything other
then the reference for the module being properly handled.  Not doing
different logic for built-in vs. not-built-in feels wrong, especially
asa some module drivers might not set the owner field for various
reasons (hint, networking drivers do not deal with the owner field as
they have different ways of handling the module reference logic.)

So I don't think this is ok, sorry.  Did you test this with networking
drivers?

thanks,

greg k-h