From: Jesper Juhl <juhl-lkml@dif.dk>
To: Alexey Dobriyan <adobriyan@mail.ru>
Cc: "David S. Miller" <davem@davemloft.net>,
Jouni Malinen <jkmaline@cc.hut.fi>,
netdev@oss.sgi.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] resource release cleanup in net/
Date: Sun, 1 May 2005 01:13:28 +0200 (CEST) [thread overview]
Message-ID: <Pine.LNX.4.62.0505010101580.2094@dragon.hyggekrogen.localhost> (raw)
In-Reply-To: <20050501025349.GA9243@mipter.zuzino.mipt.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
next prev parent reply other threads:[~2005-04-30 23:11 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2005-04-30 20:36 Jesper Juhl
2005-04-30 21:26 ` Arnaldo Carvalho de Melo
2005-05-01 1:46 ` [PATCH 0/4] resource release cleanup in net/ (take 2) Jesper Juhl
2005-05-01 1:48 ` [PATCH 1/4] " Jesper Juhl
2005-05-01 1:50 ` [PATCH 2/4] " Jesper Juhl
2005-05-01 1:52 ` [PATCH 3/4] " Jesper Juhl
2005-05-01 1:55 ` [PATCH 4/4] " Jesper Juhl
2005-05-01 2:09 ` YOSHIFUJI Hideaki / 吉藤英明
2005-05-01 2:53 ` [PATCH] resource release cleanup in net/ Alexey Dobriyan
2005-04-30 23:13 ` Jesper Juhl [this message]
2005-05-01 12:32 ` Alexey Dobriyan
2005-05-01 23:08 ` Herbert Xu
2005-05-01 23:21 ` Jesper Juhl
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Pine.LNX.4.62.0505010101580.2094@dragon.hyggekrogen.localhost \
--to=juhl-lkml@dif.dk \
--cc=adobriyan@mail.ru \
--cc=davem@davemloft.net \
--cc=jkmaline@cc.hut.fi \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@oss.sgi.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®