linux-kernel - RE: [RFC 2/5] iwlwifi: fix request

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 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]

Date:   Sun, 19 Feb 2017 09:47:59 +0000
From:   "Grumbach, Emmanuel" <emmanuel.grumbach@...el.com>
To:     "Luis R. Rodriguez" <mcgrof@...nel.org>,
        "Berg, Johannes" <johannes.berg@...el.com>,
        "Coelho, Luciano" <luciano.coelho@...el.com>,
        "tj@...nel.org" <tj@...nel.org>,
        "arjan@...ux.intel.com" <arjan@...ux.intel.com>,
        "ming.lei@...onical.com" <ming.lei@...onical.com>,
        "zajec5@...il.com" <zajec5@...il.com>
CC:     "jeyu@...hat.com" <jeyu@...hat.com>,
        "rusty@...tcorp.com.au" <rusty@...tcorp.com.au>,
        "pmladek@...e.com" <pmladek@...e.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        linuxwifi <linuxwifi@...el.com>,
        "linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [RFC 2/5] iwlwifi: fix request_module() use

> 
> The return value of request_module() being 0 does not mean that the driver
> which was requested has loaded. To properly check that the driver was
> loaded each driver can use internal mechanisms to vet the driver is now
> present. The helper try_then_request_module() was added to help with
> this, allowing drivers to specify their own validation as the first argument.
> 
> On iwlwifi the use case is a bit more complicated given that the value we
> need to check for is protected with a mutex later used on the
> module_init() of the module we are asking for. If we were to lock and
> request_module() we'd deadlock. iwlwifi needs its own wrapper then so it
> can handle its special locking requirements.
> 
> Signed-off-by: Luis R. Rodriguez <mcgrof@...nel.org>

I don't see the problem with the current code. We don't assume that everything has been
loaded immediately after request_module returns. We just free the intermediate firmware
structures that won't be using anymore. What happens here is that after request_module
returns, we patiently wait until it is loaded, and when that happens, iwl{d,m}vm's init function
will be called. That one is the one that continues the flow by calling:

	ret = iwl_opmode_register("iwlmvm", &iwl_mvm_ops);

(for the iwlmvm case).

Where am I wrong here?


> ---
>  drivers/net/wireless/intel/iwlwifi/iwl-drv.c | 34 ++++++++++++++++++++---
> -----
>  1 file changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
> b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
> index e198d6f5fcea..6beb92d19ea7 100644
> --- a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
> +++ b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
> @@ -1231,6 +1231,29 @@ static void _iwl_op_mode_stop(struct iwl_drv
> *drv)
>  	}
>  }
> 
> +static void iwlwifi_try_load_op(struct iwlwifi_opmode_table *op,
> +				struct iwl_drv *drv)
> +{
> +	int ret = 0;
> +
> +	ret = request_module("%s", op->name);
> +	if (ret)
> +		goto out;
> +
> +	mutex_lock(&iwlwifi_opmode_table_mtx);
> +	if (!op->ops)
> +		ret = -ENOENT;
> +	mutex_unlock(&iwlwifi_opmode_table_mtx);
> +
> +out:
> +#ifdef CONFIG_IWLWIFI_OPMODE_MODULAR
> +	if (ret)
> +		IWL_ERR(drv,
> +			"failed to load module %s (error %d), is dynamic
> loading enabled?\n",
> +			op->name, ret);
> +#endif
> +}
> +
>  /**
>   * iwl_req_fw_callback - callback when firmware was loaded
>   *
> @@ -1471,15 +1494,8 @@ static void iwl_req_fw_callback(const struct
> firmware *ucode_raw, void *context)
>  	 * else from proceeding if the module fails to load
>  	 * or hangs loading.
>  	 */
> -	if (load_module) {
> -		err = request_module("%s", op->name);
> -#ifdef CONFIG_IWLWIFI_OPMODE_MODULAR
> -		if (err)
> -			IWL_ERR(drv,
> -				"failed to load module %s (error %d), is
> dynamic loading enabled?\n",
> -				op->name, err);
> -#endif
> -	}
> +	if (load_module)
> +		iwlwifi_try_load_op(op, drv);
>  	goto free;
> 
>   try_again:
> --
> 2.11.0