| Message ID | 20110729025007.7889.19879.stgit@mj.roinet.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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<proski@gnu.org> 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.
Pavel, On Fri, Jul 29, 2011 at 13:02, Pavel Roskin <proski@gnu.org> 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,
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.
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; }
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 <proski@gnu.org> --- 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(-) -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html