Doing it by the caller is racy. Some callers neglected to do so. Fix
callers not to call try_module_get() after lib80211_get_crypto_ops().
When ops is copied, move lib80211_crypt_delayed_deinit() after
try_module_get() to avoid the risk that the module would be unloaded
between those calls.
Signed-off-by: Pavel Roskin <[email protected]>
---
drivers/net/wireless/hostap/hostap_ioctl.c | 5 ++---
drivers/net/wireless/ipw2x00/libipw_wx.c | 6 +++---
net/wireless/lib80211.c | 3 +++
3 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/hostap/hostap_ioctl.c b/drivers/net/wireless/hostap/hostap_ioctl.c
index 12de464..af0516c 100644
--- a/drivers/net/wireless/hostap/hostap_ioctl.c
+++ b/drivers/net/wireless/hostap/hostap_ioctl.c
@@ -166,7 +166,7 @@ static int prism2_ioctl_siwencode(struct net_device *dev,
request_module("lib80211_crypt_wep");
new_crypt->ops = lib80211_get_crypto_ops("WEP");
}
- if (new_crypt->ops && try_module_get(new_crypt->ops->owner))
+ if (new_crypt->ops)
new_crypt->priv = new_crypt->ops->init(i);
if (!new_crypt->ops || !new_crypt->priv) {
kfree(new_crypt);
@@ -3293,8 +3293,6 @@ static int prism2_ioctl_siwencodeext(struct net_device *dev,
if (*crypt == NULL || (*crypt)->ops != ops) {
struct lib80211_crypt_data *new_crypt;
- lib80211_crypt_delayed_deinit(&local->crypt_info, crypt);
-
new_crypt = kzalloc(sizeof(struct lib80211_crypt_data),
GFP_KERNEL);
if (new_crypt == NULL) {
@@ -3310,6 +3308,7 @@ static int prism2_ioctl_siwencodeext(struct net_device *dev,
goto done;
}
+ lib80211_crypt_delayed_deinit(&local->crypt_info, crypt);
*crypt = new_crypt;
}
diff --git a/drivers/net/wireless/ipw2x00/libipw_wx.c b/drivers/net/wireless/ipw2x00/libipw_wx.c
index d7bd6cf0..04c4a60 100644
--- a/drivers/net/wireless/ipw2x00/libipw_wx.c
+++ b/drivers/net/wireless/ipw2x00/libipw_wx.c
@@ -395,7 +395,7 @@ int libipw_wx_set_encode(struct libipw_device *ieee,
new_crypt->ops = lib80211_get_crypto_ops("WEP");
}
- if (new_crypt->ops && try_module_get(new_crypt->ops->owner))
+ if (new_crypt->ops)
new_crypt->priv = new_crypt->ops->init(key);
if (!new_crypt->ops || !new_crypt->priv) {
@@ -629,8 +629,6 @@ int libipw_wx_set_encodeext(struct libipw_device *ieee,
if (*crypt == NULL || (*crypt)->ops != ops) {
struct lib80211_crypt_data *new_crypt;
- lib80211_crypt_delayed_deinit(&ieee->crypt_info, crypt);
-
new_crypt = kzalloc(sizeof(*new_crypt), GFP_KERNEL);
if (new_crypt == NULL) {
ret = -ENOMEM;
@@ -644,6 +642,8 @@ int libipw_wx_set_encodeext(struct libipw_device *ieee,
ret = -EINVAL;
goto done;
}
+
+ lib80211_crypt_delayed_deinit(&ieee->crypt_info, crypt);
*crypt = new_crypt;
}
diff --git a/net/wireless/lib80211.c b/net/wireless/lib80211.c
index a55c27b..123fa19 100644
--- a/net/wireless/lib80211.c
+++ b/net/wireless/lib80211.c
@@ -242,6 +242,7 @@ struct lib80211_crypto_ops *lib80211_get_crypto_ops(const char *name)
{
struct lib80211_crypto_alg *alg;
unsigned long flags;
+ struct lib80211_crypto_ops *ret = NULL;
spin_lock_irqsave(&lib80211_crypto_lock, flags);
list_for_each_entry(alg, &lib80211_crypto_algs, list) {
@@ -252,6 +253,8 @@ struct lib80211_crypto_ops *lib80211_get_crypto_ops(const char *name)
return NULL;
found:
+ if (try_module_get(alg->ops->owner))
+ ret = alg->ops;
spin_unlock_irqrestore(&lib80211_crypto_lock, flags);
return alg->ops;
}
On 07/28/2011 10:50 PM, Pavel Roskin wrote:
> Doing it by the caller is racy. Some callers neglected to do so. Fix
> callers not to call try_module_get() after lib80211_get_crypto_ops().
>
> When ops is copied, move lib80211_crypt_delayed_deinit() after
> try_module_get() to avoid the risk that the module would be unloaded
> between those calls.
>
> Signed-off-by: Pavel Roskin<[email protected]>
Sorry, please ignore this patch! I didn't mean to send it. It's not
complete, and I don't think I'll have time to fix it :(
lib80211 has a terrible API, and the module referencing is split between
lib80211 and the callers. Everything is too complicated. Keys may be
freed asynchronously. Module referencing is tied to the keys and not to
the ops. I've seen the reference count for lib80211_crypt_ccmp to
underflow and become 4294967295 or something.
Considering that lib80211 is only used by old modules, I'm even thinking
of making lib80211_crypt_* modules not unloadable. It's too much work
to fix.
What I actually intended to send is "lib80211: remove exports for
functions not called by other modules". That "simplifies" the API a
little bit.
--
Regards,
Pavel Roskin
On 07/28/2011 11:27 PM, Julian Calaby wrote:
> If I recall correctly, lib80211 is the last remainder of the
> pre-mac80211 wireless stack. It should be annihilated with extreme
> prejudice, but the intel centrino 2xxx drivers are so interwoven with
> it that extracting them would be a pain in the ass - and given that
> nobody who has those parts really cares, this hasn't happened.
Basically, lib80211 has no consistent API. The callers (hostap and
ipw2x00) get all the internals and do way too much with them.
Module refcounting is (presumably) done by the "key objects", i.e. the
structures consisting of the keys and the operations that apply to them.
Something is done wrong, as there are more module puts than module gets.
But the caller also gets the ops, that is the set of encryption
functions from a particular encryption module. There is no locking for
the ops. There is no lib80211_put_crypto_ops(), that is, the caller
never says it's not using the ops anymore.
Possible fixes are:
1) Rewrite the whole API. Hide ops from the callers. Enforce
refcounting by the keys. The keys should hold the ops, the ops should
hold the modules. That may be a lot of work, but the result will be
nice. Maybe even mac80211 could use it.
2) Fix key object based refcounting. Ignore the fact that a crypto
module can be unloaded at a wrong time while the caller is using the
ops. That may be a simple fix, but it won't be complete. At least no
new bugs would be introduced.
3) Same as above, but add ops-based refcounting to protect against
crypto module unloading. The problem is that it's hard to find the
places where the callers stop using the ops. The result would work
correctly, and the changes won't be too radical. But it's hard to get
right.
4) Merge lib80211 and lib80211_crypt_* into one module. That solves the
problem completely. Some memory would be wasted for those who use only
one algorithm.
5) Make lib80211_crypt_* modules permanent (not unloadable). Some
sysadmins may be unhappy that the modules cannot be replaced without reboot.
--
Regards,
Pavel Roskin
Pavel,
On Fri, Jul 29, 2011 at 13:02, Pavel Roskin <[email protected]> wrote:
> lib80211 has a terrible API, and the module referencing is split between
> lib80211 and the callers. ?Everything is too complicated. ?Keys may be freed
> asynchronously. ?Module referencing is tied to the keys and not to the ops.
> ?I've seen the reference count for lib80211_crypt_ccmp to underflow and
> become 4294967295 or something.
If I recall correctly, lib80211 is the last remainder of the
pre-mac80211 wireless stack. It should be annihilated with extreme
prejudice, but the intel centrino 2xxx drivers are so interwoven with
it that extracting them would be a pain in the ass - and given that
nobody who has those parts really cares, this hasn't happened.
That said, bugs are bugs and they should be fixed.
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/