[patch -next] caif: add error handling for allocation

[patch -next] caif: add error handling for allocation

[Dan Carpenter]

The allocation could fail so we should check, or other errors could
happen and we should free the "phyinfo" variable.

Signed-off-by: Dan Carpenter <error27@gmail.com>

diff --git a/net/caif/cfcnfg.c b/net/caif/cfcnfg.c
index f07ab8c..b213b53 100644
--- a/net/caif/cfcnfg.c
+++ b/net/caif/cfcnfg.c
@@ -467,7 +467,7 @@ cfcnfg_add_phy_layer(struct cfcnfg *cnfg, enum cfcnfg_phy_type phy_type,
 {
 	struct cflayer *frml;
 	struct cflayer *phy_driver = NULL;
-	struct cfcnfg_phyinfo *phyinfo;
+	struct cfcnfg_phyinfo *phyinfo = NULL;
 	int i;
 	u8 phyid;
 
@@ -482,23 +482,25 @@ cfcnfg_add_phy_layer(struct cfcnfg *cnfg, enum cfcnfg_phy_type phy_type,
 			goto got_phyid;
 	}
 	pr_warn("Too many CAIF Link Layers (max 6)\n");
-	goto out;
+	goto out_err;
 
 got_phyid:
 	phyinfo = kzalloc(sizeof(struct cfcnfg_phyinfo), GFP_ATOMIC);
+	if (!phyinfo)
+		goto out_err;
 
 	switch (phy_type) {
 	case CFPHYTYPE_FRAG:
 		phy_driver =
 		    cfserl_create(CFPHYTYPE_FRAG, phyid, stx);
 		if (!phy_driver)
-			goto out;
+			goto out_err;
 		break;
 	case CFPHYTYPE_CAIF:
 		phy_driver = NULL;
 		break;
 	default:
-		goto out;
+		goto out_err;
 	}
 	phy_layer->id = phyid;
 	phyinfo->pref = pref;
@@ -512,10 +514,8 @@ got_phyid:
 
 	frml = cffrml_create(phyid, fcs);
 
-	if (!frml) {
-		kfree(phyinfo);
-		goto out;
-	}
+	if (!frml)
+		goto out_err;
 	phyinfo->frm_layer = frml;
 	layer_set_up(frml, cnfg->mux);
 
@@ -531,7 +531,11 @@ got_phyid:
 	}
 
 	list_add_rcu(&phyinfo->node, &cnfg->phys);
-out:
+	mutex_unlock(&cnfg->lock);
+	return;
+
+out_err:
+	kfree(phyinfo);
 	mutex_unlock(&cnfg->lock);
 }
 EXPORT_SYMBOL(cfcnfg_add_phy_layer);

Re: [patch -next] caif: add error handling for allocation

[Sjur Brændeland]

Hi Dan,
...
>        switch (phy_type) {
>        case CFPHYTYPE_FRAG:
>                phy_driver =
>                    cfserl_create(CFPHYTYPE_FRAG, phyid, stx);
>                if (!phy_driver)
> -                       goto out;
> +                       goto out_err;
>                break;
...
> -out:
> +       mutex_unlock(&cnfg->lock);
> +       return;
> +
> +out_err:
> +       kfree(phyinfo);
>        mutex_unlock(&cnfg->lock);

Thank you for your patch.
When reviewing this I found another potential memory leak as well.
If cffrml_create fails, we might be leaking the phy_driver.
So perhaps you could do kfree(phy_driver) in out_err: as well, while
you are at it?

Regards,
Sjur

[PATCH] caif: fix a potential NULL dereference

[Eric Dumazet]

Commit bd30ce4bc0b7 (caif: Use RCU instead of spin-lock in caif_dev.c)
added a potential NULL dereference in case alloc_percpu() fails.

caif_device_alloc() can also use GFP_KERNEL instead of GFP_ATOMIC.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
CC: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 net/caif/caif_dev.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/caif/caif_dev.c b/net/caif/caif_dev.c
index 7c2fa0a..7f9ac07 100644
--- a/net/caif/caif_dev.c
+++ b/net/caif/caif_dev.c
@@ -93,10 +93,14 @@ static struct caif_device_entry *caif_device_alloc(struct net_device *dev)
 	caifdevs = caif_device_list(dev_net(dev));
 	BUG_ON(!caifdevs);
 
-	caifd = kzalloc(sizeof(*caifd), GFP_ATOMIC);
+	caifd = kzalloc(sizeof(*caifd), GFP_KERNEL);
 	if (!caifd)
 		return NULL;
 	caifd->pcpu_refcnt = alloc_percpu(int);
+	if (!caifd->pcpu_refcnt) {
+		kfree(caifd);
+		return NULL;
+	}
 	caifd->netdev = dev;
 	dev_hold(dev);
 	return caifd;

Re: [PATCH] caif: fix a potential NULL dereference

[Sjur Brændeland]

Eric Dumazet wrote:
> Commit bd30ce4bc0b7 (caif: Use RCU instead of spin-lock in caif_dev.c)
> added a potential NULL dereference in case alloc_percpu() fails.
>
> caif_device_alloc() can also use GFP_KERNEL instead of GFP_ATOMIC.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Looks good thanks,
Acked-by: Sjur Brændeland <sjur.brandeland@stericsson.com>

Re: [PATCH] caif: fix a potential NULL dereference

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 02 Sep 2011 14:19:23 +0200

> Commit bd30ce4bc0b7 (caif: Use RCU instead of spin-lock in caif_dev.c)
> added a potential NULL dereference in case alloc_percpu() fails.
> 
> caif_device_alloc() can also use GFP_KERNEL instead of GFP_ATOMIC.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> CC: Sjur Brændeland <sjur.brandeland@stericsson.com>

Applied.

Re: [patch -next] caif: add error handling for allocation

[Dan Carpenter]

On Fri, Sep 02, 2011 at 11:40:23AM +0200, Sjur Brændeland wrote:
> Thank you for your patch.
> When reviewing this I found another potential memory leak as well.
> If cffrml_create fails, we might be leaking the phy_driver.
> So perhaps you could do kfree(phy_driver) in out_err: as well, while
> you are at it?
> 

Good point.  A kfree(phy_driver) would fix the leak.  But why does
cfserl_create() return &this->layer; instead of just "return this;"
Their equivalent now, but if you change the cfserl struct it will
break the kfree().

I'll be travelling for a while, so I may be out of reach until
Wednessday.

regards,
dan carpenter

[patch v2] caif: add error handling for allocation

[Dan Carpenter]

The allocation of "phyinfo" wasn't checked, and also the allocation
wasn't freed on error paths.  Sjur Brændeland pointed out as well
that "phy_driver" should be freed on the error path too.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
V2:  Add a kfree(phy_driver).

diff --git a/net/caif/cfcnfg.c b/net/caif/cfcnfg.c
index f07ab8c..00523ec 100644
--- a/net/caif/cfcnfg.c
+++ b/net/caif/cfcnfg.c
@@ -467,7 +467,7 @@ cfcnfg_add_phy_layer(struct cfcnfg *cnfg, enum cfcnfg_phy_type phy_type,
 {
 	struct cflayer *frml;
 	struct cflayer *phy_driver = NULL;
-	struct cfcnfg_phyinfo *phyinfo;
+	struct cfcnfg_phyinfo *phyinfo = NULL;
 	int i;
 	u8 phyid;
 
@@ -482,23 +482,25 @@ cfcnfg_add_phy_layer(struct cfcnfg *cnfg, enum cfcnfg_phy_type phy_type,
 			goto got_phyid;
 	}
 	pr_warn("Too many CAIF Link Layers (max 6)\n");
-	goto out;
+	goto out_err;
 
 got_phyid:
 	phyinfo = kzalloc(sizeof(struct cfcnfg_phyinfo), GFP_ATOMIC);
+	if (!phyinfo)
+		goto out_err;
 
 	switch (phy_type) {
 	case CFPHYTYPE_FRAG:
 		phy_driver =
 		    cfserl_create(CFPHYTYPE_FRAG, phyid, stx);
 		if (!phy_driver)
-			goto out;
+			goto out_err;
 		break;
 	case CFPHYTYPE_CAIF:
 		phy_driver = NULL;
 		break;
 	default:
-		goto out;
+		goto out_err;
 	}
 	phy_layer->id = phyid;
 	phyinfo->pref = pref;
@@ -512,10 +514,8 @@ got_phyid:
 
 	frml = cffrml_create(phyid, fcs);
 
-	if (!frml) {
-		kfree(phyinfo);
-		goto out;
-	}
+	if (!frml)
+		goto out_err;
 	phyinfo->frm_layer = frml;
 	layer_set_up(frml, cnfg->mux);
 
@@ -531,7 +531,12 @@ got_phyid:
 	}
 
 	list_add_rcu(&phyinfo->node, &cnfg->phys);
-out:
+	mutex_unlock(&cnfg->lock);
+	return;
+
+out_err:
+	kfree(phy_driver);
+	kfree(phyinfo);
 	mutex_unlock(&cnfg->lock);
 }
 EXPORT_SYMBOL(cfcnfg_add_phy_layer);

RE: [patch v2] caif: add error handling for allocation

[Sjur BRENDELAND]

[Dan]: 
> The allocation of "phyinfo" wasn't checked, and also the allocation
> wasn't freed on error paths.  Sjur Brændeland pointed out as well
> that "phy_driver" should be freed on the error path too.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Looks good, thank you Dan.
Acked-by: Sjur Brændeland <sjur.brandeland@stericsson.com>

Re: [patch v2] caif: add error handling for allocation

[David Miller]

From: Sjur BRENDELAND <sjur.brandeland@stericsson.com>
Date: Fri, 23 Sep 2011 12:38:45 +0200

> [Dan]: 
>> The allocation of "phyinfo" wasn't checked, and also the allocation
>> wasn't freed on error paths.  Sjur Brændeland pointed out as well
>> that "phy_driver" should be freed on the error path too.
>> 
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> Looks good, thank you Dan.
> Acked-by: Sjur Brændeland <sjur.brandeland@stericsson.com>

Applied, thanks.