Re: [PATCH] resource release cleanup in net/

From: Jesper Juhl (juhl-lkml_at_dif.dk)
Date: 05/01/05

  • Next message: Randy.Dunlap: "Re: 2.6.12-rc3-mm1" 
    Date:	Sun, 1 May 2005 01:13:28 +0200 (CEST)
To: Alexey Dobriyan <adobriyan@mail.ru>

    On Sun, 1 May 2005, Alexey Dobriyan wrote:

    > On Sat, Apr 30, 2005 at 10:36:00PM +0200, Jesper Juhl wrote:
    > > Since Andrew merged the patch that makes calling crypto_free_tfm() with a
    > > NULL pointer safe into 2.6.12-rc3-mm1, I made a patch to remove checks for
    > > NULL before calling that function
    > > drivers/net/wireless/hostap/hostap_crypt_ccmp.c | 5 -
    > > drivers/net/wireless/hostap/hostap_crypt_tkip.c | 10 +-
    > > drivers/net/wireless/hostap/hostap_crypt_wep.c | 5 -
    > > net/ieee80211/ieee80211_crypt_ccmp.c | 5 -
    > > net/ieee80211/ieee80211_crypt_tkip.c | 10 +-
    > > net/ieee80211/ieee80211_crypt_wep.c | 5 -
    > I think I have a better one for these.
    >
    > --- linux-2.6.12-rc3-mm1/drivers/net/wireless/hostap/hostap_crypt_ccmp.c 2005-05-01 01:53:50.000000000 +0000
    > +++ linux-2.6.12-rc3-mm1-hostap/drivers/net/wireless/hostap/hostap_crypt_ccmp.c 2005-05-01 02:21:10.000000000 +0000
    > @@ -102,17 +102,14 @@ static void * hostap_ccmp_init(int key_i
    > if (priv->tfm == NULL) {
    > printk(KERN_DEBUG "hostap_crypt_ccmp: could not allocate "
    > "crypto API aes\n");
    > - goto fail;
    > + goto fail_free;
    > }
    >
    > return priv;
    >
    > +fail_free:
    > + kfree(priv);
    > fail:
    > - if (priv) {
    > - if (priv->tfm)
    > - crypto_free_tfm(priv->tfm);
    > - kfree(priv);
    > - }
    > module_put(THIS_MODULE);
    > return NULL;
    > }

    Personally I find the code a lot simpler when there's just a single label
    "fail" that takes care of all the cleanup. But, let's hear what David has
    to say on that.

    > @@ -121,8 +118,7 @@ fail:
    > static void hostap_ccmp_deinit(void *priv)
    > {
    > struct hostap_ccmp_data *_priv = priv;
    > - if (_priv && _priv->tfm)
    > - crypto_free_tfm(_priv->tfm);
    > + crypto_free_tfm(_priv->tfm);
    > kfree(priv);
    > module_put(THIS_MODULE);
    > }

    This will Oops if _priv is NULL. That's why my patch did

    if (_priv)
            crypto_free_tfm(_priv->tfm);

    > --- linux-2.6.12-rc3-mm1/drivers/net/wireless/hostap/hostap_crypt_tkip.c 2005-05-01 01:53:50.000000000 +0000
    > +++ linux-2.6.12-rc3-mm1-hostap/drivers/net/wireless/hostap/hostap_crypt_tkip.c 2005-05-01 02:27:18.000000000 +0000
    > @@ -88,26 +88,23 @@ static void * hostap_tkip_init(int key_i
    > if (priv->tfm_arc4 == NULL) {
    > printk(KERN_DEBUG "hostap_crypt_tkip: could not allocate "
    > "crypto API arc4\n");
    > - goto fail;
    > + goto fail_arc4;
    > }
    >
    > priv->tfm_michael = crypto_alloc_tfm("michael_mic", 0);
    > if (priv->tfm_michael == NULL) {
    > printk(KERN_DEBUG "hostap_crypt_tkip: could not allocate "
    > "crypto API michael_mic\n");
    > - goto fail;
    > + goto fail_michael;
    > }
    >
    > return priv;
    >
    > +fail_michael:
    > + crypto_free_tfm(priv->tfm_arc4);
    > +fail_arc4:
    > + kfree(priv);
    > fail:
    > - if (priv) {
    > - if (priv->tfm_michael)
    > - crypto_free_tfm(priv->tfm_michael);
    > - if (priv->tfm_arc4)
    > - crypto_free_tfm(priv->tfm_arc4);
    > - kfree(priv);
    > - }
    > module_put(THIS_MODULE);
    > return NULL;
    > }

    Again, I find the single 'fail' approach simpler.

    > @@ -116,10 +113,8 @@ fail:
    > static void hostap_tkip_deinit(void *priv)
    > {
    > struct hostap_tkip_data *_priv = priv;
    > - if (_priv && _priv->tfm_michael)
    > - crypto_free_tfm(_priv->tfm_michael);
    > - if (_priv && _priv->tfm_arc4)
    > - crypto_free_tfm(_priv->tfm_arc4);
    > + crypto_free_tfm(_priv->tfm_michael);
    > + crypto_free_tfm(_priv->tfm_arc4);
    > kfree(priv);
    > module_put(THIS_MODULE);
    > }

    Oops if _priv == NULL.

    > --- linux-2.6.12-rc3-mm1/drivers/net/wireless/hostap/hostap_crypt_wep.c 2005-05-01 01:53:50.000000000 +0000
    > +++ linux-2.6.12-rc3-mm1-hostap/drivers/net/wireless/hostap/hostap_crypt_wep.c 2005-05-01 02:30:08.000000000 +0000
    > @@ -59,7 +59,7 @@ static void * prism2_wep_init(int keyidx
    > if (priv->tfm == NULL) {
    > printk(KERN_DEBUG "hostap_crypt_wep: could not allocate "
    > "crypto API arc4\n");
    > - goto fail;
    > + goto fail_tfm;
    > }
    >
    > /* start WEP IV from a random value */
    > @@ -67,12 +67,9 @@ static void * prism2_wep_init(int keyidx
    >
    > return priv;
    >
    > +fail_tfm:
    > + kfree(priv);
    > fail:
    > - if (priv) {
    > - if (priv->tfm)
    > - crypto_free_tfm(priv->tfm);
    > - kfree(priv);
    > - }
    > module_put(THIS_MODULE);
    > return NULL;
    > }

    Again my preference is for the original, same for the rest of these.

    > @@ -81,8 +78,7 @@ fail:
    > static void prism2_wep_deinit(void *priv)
    > {
    > struct prism2_wep_data *_priv = priv;
    > - if (_priv && _priv->tfm)
    > - crypto_free_tfm(_priv->tfm);
    > + crypto_free_tfm(_priv->tfm);
    > kfree(priv);
    > module_put(THIS_MODULE);
    > }

    Oops if _priv == NULL.

    > --- linux-2.6.12-rc3-mm1/net/ieee80211/ieee80211_crypt_ccmp.c 2005-05-01 01:53:57.000000000 +0000
    > +++ linux-2.6.12-rc3-mm1-hostap/net/ieee80211/ieee80211_crypt_ccmp.c 2005-05-01 02:31:22.000000000 +0000
    > @@ -89,18 +89,14 @@ static void * ieee80211_ccmp_init(int ke
    > if (priv->tfm == NULL) {
    > printk(KERN_DEBUG "ieee80211_crypt_ccmp: could not allocate "
    > "crypto API aes\n");
    > - goto fail;
    > + goto fail_tfm;
    > }
    >
    > return priv;
    >
    > +fail_tfm:
    > + kfree(priv);
    > fail:
    > - if (priv) {
    > - if (priv->tfm)
    > - crypto_free_tfm(priv->tfm);
    > - kfree(priv);
    > - }
    > -
    > return NULL;
    > }
    >
    > @@ -108,8 +104,7 @@ fail:
    > static void ieee80211_ccmp_deinit(void *priv)
    > {
    > struct ieee80211_ccmp_data *_priv = priv;
    > - if (_priv && _priv->tfm)
    > - crypto_free_tfm(_priv->tfm);
    > + crypto_free_tfm(_priv->tfm);
    > kfree(priv);
    > }
    >
    if _priv is NULL this blows up.

    > --- linux-2.6.12-rc3-mm1/net/ieee80211/ieee80211_crypt_tkip.c 2005-05-01 01:53:57.000000000 +0000
    > +++ linux-2.6.12-rc3-mm1-hostap/net/ieee80211/ieee80211_crypt_tkip.c 2005-05-01 02:34:04.000000000 +0000
    > @@ -76,27 +76,23 @@ static void * ieee80211_tkip_init(int ke
    > if (priv->tfm_arc4 == NULL) {
    > printk(KERN_DEBUG "ieee80211_crypt_tkip: could not allocate "
    > "crypto API arc4\n");
    > - goto fail;
    > + goto fail_arc4;
    > }
    >
    > priv->tfm_michael = crypto_alloc_tfm("michael_mic", 0);
    > if (priv->tfm_michael == NULL) {
    > printk(KERN_DEBUG "ieee80211_crypt_tkip: could not allocate "
    > "crypto API michael_mic\n");
    > - goto fail;
    > + goto fail_michael;
    > }
    >
    > return priv;
    >
    > +fail_michael:
    > + crypto_free_tfm(priv->tfm_arc4);
    > +fail_arc4:
    > + kfree(priv);
    > fail:
    > - if (priv) {
    > - if (priv->tfm_michael)
    > - crypto_free_tfm(priv->tfm_michael);
    > - if (priv->tfm_arc4)
    > - crypto_free_tfm(priv->tfm_arc4);
    > - kfree(priv);
    > - }
    > -
    > return NULL;
    > }
    >
    > @@ -104,10 +100,8 @@ fail:
    > static void ieee80211_tkip_deinit(void *priv)
    > {
    > struct ieee80211_tkip_data *_priv = priv;
    > - if (_priv && _priv->tfm_michael)
    > - crypto_free_tfm(_priv->tfm_michael);
    > - if (_priv && _priv->tfm_arc4)
    > - crypto_free_tfm(_priv->tfm_arc4);
    > + crypto_free_tfm(_priv->tfm_michael);
    > + crypto_free_tfm(_priv->tfm_arc4);
    > kfree(priv);
    > }
    >
    Again we'll blow up if _priv is NULL.

    > --- linux-2.6.12-rc3-mm1/net/ieee80211/ieee80211_crypt_wep.c 2005-05-01 01:53:57.000000000 +0000
    > +++ linux-2.6.12-rc3-mm1-hostap/net/ieee80211/ieee80211_crypt_wep.c 2005-05-01 02:35:18.000000000 +0000
    > @@ -54,7 +54,7 @@ static void * prism2_wep_init(int keyidx
    > if (priv->tfm == NULL) {
    > printk(KERN_DEBUG "ieee80211_crypt_wep: could not allocate "
    > "crypto API arc4\n");
    > - goto fail;
    > + goto fail_tfm;
    > }
    >
    > /* start WEP IV from a random value */
    > @@ -62,12 +62,9 @@ static void * prism2_wep_init(int keyidx
    >
    > return priv;
    >
    > +fail_tfm:
    > + kfree(priv);
    > fail:
    > - if (priv) {
    > - if (priv->tfm)
    > - crypto_free_tfm(priv->tfm);
    > - kfree(priv);
    > - }
    > return NULL;
    > }
    >
    > @@ -75,8 +72,7 @@ fail:
    > static void prism2_wep_deinit(void *priv)
    > {
    > struct prism2_wep_data *_priv = priv;
    > - if (_priv && _priv->tfm)
    > - crypto_free_tfm(_priv->tfm);
    > + crypto_free_tfm(_priv->tfm);
    > kfree(priv);
    > }
    >
    _priv == NULL == Oops.

    I think my version of the patch is better.

    Ohh and would you mind submitting patches inline? Makes it a lot easier to
    quote and comment. 

    -- 
Jesper 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
  • Next message: Randy.Dunlap: "Re: 2.6.12-rc3-mm1"

    Relevant Pages