[PATCH v3] staging: vt6655: check for memory allocation failures

[PATCH v3] staging: vt6655: check for memory allocation failures

[Ji-Hun Kim]

There are no null pointer checking on rd_info and td_info values which
are allocated by kzalloc. It has potential null pointer dereferencing
issues. Implement error handling code on device_init_rd*, device_init_td*
and vnt_start for the allocation failures.

Signed-off-by: Ji-Hun Kim <ji_hun.kim@samsung.com>
---
Changes v2:

- Delete WARN_ON which can makes crashes on some machines.
- Instead of return directly, goto freeing function for freeing previously
  allocated memory in the for loop after kzalloc() failed.
- In the freeing function, add if statement for freeing to only allocated
  values.

Changes v3:

- Modify return type of device_init_rd*, device_init_td*. Then add returns
  error code at those functions and vnt_start as well.

 drivers/staging/vt6655/device_main.c | 114 +++++++++++++++++++++++++----------
 1 file changed, 82 insertions(+), 32 deletions(-)

diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c
index fbc4bc6..0d55f34 100644
--- a/drivers/staging/vt6655/device_main.c
+++ b/drivers/staging/vt6655/device_main.c
@@ -124,10 +124,10 @@
 static void device_free_info(struct vnt_private *priv);
 static void device_print_info(struct vnt_private *priv);
 
-static void device_init_rd0_ring(struct vnt_private *priv);
-static void device_init_rd1_ring(struct vnt_private *priv);
-static void device_init_td0_ring(struct vnt_private *priv);
-static void device_init_td1_ring(struct vnt_private *priv);
+static int device_init_rd0_ring(struct vnt_private *priv);
+static int device_init_rd1_ring(struct vnt_private *priv);
+static int device_init_td0_ring(struct vnt_private *priv);
+static int device_init_td1_ring(struct vnt_private *priv);
 
 static int  device_rx_srv(struct vnt_private *priv, unsigned int idx);
 static int  device_tx_srv(struct vnt_private *priv, unsigned int idx);
@@ -528,18 +528,22 @@ static void device_free_rings(struct vnt_private *priv)
 				  priv->tx0_bufs, priv->tx_bufs_dma0);
 }
 
-static void device_init_rd0_ring(struct vnt_private *priv)
+static int device_init_rd0_ring(struct vnt_private *priv)
 {
 	int i;
 	dma_addr_t      curr = priv->rd0_pool_dma;
 	struct vnt_rx_desc *desc;
+	int ret = 0;
 
 	/* Init the RD0 ring entries */
 	for (i = 0; i < priv->opts.rx_descs0;
 	     i ++, curr += sizeof(struct vnt_rx_desc)) {
 		desc = &priv->aRD0Ring[i];
 		desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
-
+		if (!desc->rd_info) {
+			ret = -ENOMEM;
+			goto error;
+		}
 		if (!device_alloc_rx_buf(priv, desc))
 			dev_err(&priv->pcid->dev, "can not alloc rx bufs\n");
 
@@ -550,20 +554,29 @@ static void device_init_rd0_ring(struct vnt_private *priv)
 	if (i > 0)
 		priv->aRD0Ring[i-1].next_desc = cpu_to_le32(priv->rd0_pool_dma);
 	priv->pCurrRD[0] = &priv->aRD0Ring[0];
+
+	return 0;
+error:
+	device_free_rd0_ring(priv);
+	return ret;
 }
 
-static void device_init_rd1_ring(struct vnt_private *priv)
+static int device_init_rd1_ring(struct vnt_private *priv)
 {
 	int i;
 	dma_addr_t      curr = priv->rd1_pool_dma;
 	struct vnt_rx_desc *desc;
+	int ret = 0;
 
 	/* Init the RD1 ring entries */
 	for (i = 0; i < priv->opts.rx_descs1;
 	     i ++, curr += sizeof(struct vnt_rx_desc)) {
 		desc = &priv->aRD1Ring[i];
 		desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
-
+		if (!desc->rd_info) {
+			ret = -ENOMEM;
+			goto error;
+		}
 		if (!device_alloc_rx_buf(priv, desc))
 			dev_err(&priv->pcid->dev, "can not alloc rx bufs\n");
 
@@ -574,6 +587,11 @@ static void device_init_rd1_ring(struct vnt_private *priv)
 	if (i > 0)
 		priv->aRD1Ring[i-1].next_desc = cpu_to_le32(priv->rd1_pool_dma);
 	priv->pCurrRD[1] = &priv->aRD1Ring[0];
+
+	return 0;
+error:
+	device_free_rd1_ring(priv);
+	return ret;
 }
 
 static void device_free_rd0_ring(struct vnt_private *priv)
@@ -584,12 +602,12 @@ static void device_free_rd0_ring(struct vnt_private *priv)
 		struct vnt_rx_desc *desc = &priv->aRD0Ring[i];
 		struct vnt_rd_info *rd_info = desc->rd_info;
 
-		dma_unmap_single(&priv->pcid->dev, rd_info->skb_dma,
-				 priv->rx_buf_sz, DMA_FROM_DEVICE);
-
-		dev_kfree_skb(rd_info->skb);
-
-		kfree(desc->rd_info);
+		if (rd_info) {
+			dma_unmap_single(&priv->pcid->dev, rd_info->skb_dma,
+					priv->rx_buf_sz, DMA_FROM_DEVICE);
+			dev_kfree_skb(rd_info->skb);
+			kfree(desc->rd_info);
+		}
 	}
 }
 
@@ -601,27 +619,31 @@ static void device_free_rd1_ring(struct vnt_private *priv)
 		struct vnt_rx_desc *desc = &priv->aRD1Ring[i];
 		struct vnt_rd_info *rd_info = desc->rd_info;
 
-		dma_unmap_single(&priv->pcid->dev, rd_info->skb_dma,
-				 priv->rx_buf_sz, DMA_FROM_DEVICE);
-
-		dev_kfree_skb(rd_info->skb);
-
-		kfree(desc->rd_info);
+		if (rd_info) {
+			dma_unmap_single(&priv->pcid->dev, rd_info->skb_dma,
+					priv->rx_buf_sz, DMA_FROM_DEVICE);
+			dev_kfree_skb(rd_info->skb);
+			kfree(desc->rd_info);
+		}
 	}
 }
 
-static void device_init_td0_ring(struct vnt_private *priv)
+static int device_init_td0_ring(struct vnt_private *priv)
 {
 	int i;
 	dma_addr_t  curr;
 	struct vnt_tx_desc *desc;
+	int ret = 0;
 
 	curr = priv->td0_pool_dma;
 	for (i = 0; i < priv->opts.tx_descs[0];
 	     i++, curr += sizeof(struct vnt_tx_desc)) {
 		desc = &priv->apTD0Rings[i];
 		desc->td_info = kzalloc(sizeof(*desc->td_info), GFP_KERNEL);
-
+		if (!desc->td_info) {
+			ret = -ENOMEM;
+			goto error;
+		}
 		desc->td_info->buf = priv->tx0_bufs + i * PKT_BUF_SZ;
 		desc->td_info->buf_dma = priv->tx_bufs_dma0 + i * PKT_BUF_SZ;
 
@@ -632,13 +654,19 @@ static void device_init_td0_ring(struct vnt_private *priv)
 	if (i > 0)
 		priv->apTD0Rings[i-1].next_desc = cpu_to_le32(priv->td0_pool_dma);
 	priv->apTailTD[0] = priv->apCurrTD[0] = &priv->apTD0Rings[0];
+
+	return 0;
+error:
+	device_free_td0_ring(priv);
+	return ret;
 }
 
-static void device_init_td1_ring(struct vnt_private *priv)
+static int device_init_td1_ring(struct vnt_private *priv)
 {
 	int i;
 	dma_addr_t  curr;
 	struct vnt_tx_desc *desc;
+	int ret = 0;
 
 	/* Init the TD ring entries */
 	curr = priv->td1_pool_dma;
@@ -646,7 +674,10 @@ static void device_init_td1_ring(struct vnt_private *priv)
 	     i++, curr += sizeof(struct vnt_tx_desc)) {
 		desc = &priv->apTD1Rings[i];
 		desc->td_info = kzalloc(sizeof(*desc->td_info), GFP_KERNEL);
-
+		if (!desc->td_info) {
+			ret = -ENOMEM;
+			goto error;
+		}
 		desc->td_info->buf = priv->tx1_bufs + i * PKT_BUF_SZ;
 		desc->td_info->buf_dma = priv->tx_bufs_dma1 + i * PKT_BUF_SZ;
 
@@ -657,6 +688,11 @@ static void device_init_td1_ring(struct vnt_private *priv)
 	if (i > 0)
 		priv->apTD1Rings[i-1].next_desc = cpu_to_le32(priv->td1_pool_dma);
 	priv->apTailTD[1] = priv->apCurrTD[1] = &priv->apTD1Rings[0];
+
+	return 0;
+error:
+	device_free_td1_ring(priv);
+	return ret;
 }
 
 static void device_free_td0_ring(struct vnt_private *priv)
@@ -667,8 +703,10 @@ static void device_free_td0_ring(struct vnt_private *priv)
 		struct vnt_tx_desc *desc = &priv->apTD0Rings[i];
 		struct vnt_td_info *td_info = desc->td_info;
 
-		dev_kfree_skb(td_info->skb);
-		kfree(desc->td_info);
+		if (td_info) {
+			dev_kfree_skb(td_info->skb);
+			kfree(desc->td_info);
+		}
 	}
 }
 
@@ -680,8 +718,10 @@ static void device_free_td1_ring(struct vnt_private *priv)
 		struct vnt_tx_desc *desc = &priv->apTD1Rings[i];
 		struct vnt_td_info *td_info = desc->td_info;
 
-		dev_kfree_skb(td_info->skb);
-		kfree(desc->td_info);
+		if (td_info) {
+			dev_kfree_skb(td_info->skb);
+			kfree(desc->td_info);
+		}
 	}
 }
 
@@ -1165,10 +1205,18 @@ static int vnt_start(struct ieee80211_hw *hw)
 	}
 
 	dev_dbg(&priv->pcid->dev, "call device init rd0 ring\n");
-	device_init_rd0_ring(priv);
-	device_init_rd1_ring(priv);
-	device_init_td0_ring(priv);
-	device_init_td1_ring(priv);
+	ret = device_init_rd0_ring(priv);
+	if (ret)
+		goto error;
+	ret = device_init_rd1_ring(priv);
+	if (ret)
+		goto error;
+	ret = device_init_td0_ring(priv);
+	if (ret)
+		goto error;
+	ret = device_init_td1_ring(priv);
+	if (ret)
+		goto error;
 
 	device_init_registers(priv);
 
@@ -1178,6 +1226,8 @@ static int vnt_start(struct ieee80211_hw *hw)
 	ieee80211_wake_queues(hw);
 
 	return 0;
+error:
+	return ret;
 }
 
 static void vnt_stop(struct ieee80211_hw *hw)
-- 
1.9.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3] staging: vt6655: check for memory allocation failures

[Jia-Ju Bai]

On 2018/3/30 10:44, Ji-Hun Kim wrote:
> @@ -1165,10 +1205,18 @@ static int vnt_start(struct ieee80211_hw *hw)
>   	}
>   
>   	dev_dbg(&priv->pcid->dev, "call device init rd0 ring\n");
> -	device_init_rd0_ring(priv);
> -	device_init_rd1_ring(priv);
> -	device_init_td0_ring(priv);
> -	device_init_td1_ring(priv);
> +	ret = device_init_rd0_ring(priv);
> +	if (ret)
> +		goto error;
> +	ret = device_init_rd1_ring(priv);
> +	if (ret)
> +		goto error;
> +	ret = device_init_td0_ring(priv);
> +	if (ret)
> +		goto error;
> +	ret = device_init_td1_ring(priv);
> +	if (ret)
> +		goto error;
>   
>   	device_init_registers(priv);
>   
> @@ -1178,6 +1226,8 @@ static int vnt_start(struct ieee80211_hw *hw)
>   	ieee80211_wake_queues(hw);
>   
>   	return 0;
> +error:
> +	return ret;
>   }

This code will lead to memory leaks when device_init_rd1_ring() fails, 
because the memory allocated by device_init_rd0_ring() is not freed.

I think this one will be better:
     ret = device_init_rd0_ring(priv);
     if (ret)
         goto error_init_rd0_ring;
     ret = device_init_rd1_ring(priv);
     if (ret)
         goto error_init_rd1_ring;
     ret = device_init_td0_ring(priv);
     if (ret)
         goto error_init_td0_ring;
     ret = device_init_td1_ring(priv);
     if (ret)
         goto error_init_td1_ring;
     ......
error_init_td1_ring:
     device_free_td0_ring(priv);
error_init_td0_ring:
     device_free_rd1_ring(priv);
error_init_rd1_ring:
     device_free_rd0_ring(priv);
error_init_rd0_ring:
     return ret;


Best wishes,
Jia-Ju Bai
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3] staging: vt6655: check for memory allocation failures

[Ji-Hun Kim]

On Fri, Mar 30, 2018 at 11:15:03AM +0800, Jia-Ju Bai wrote:
> 
> 
> On 2018/3/30 10:44, Ji-Hun Kim wrote:
> >@@ -1165,10 +1205,18 @@ static int vnt_start(struct ieee80211_hw *hw)
> >  	}
> >  	dev_dbg(&priv->pcid->dev, "call device init rd0 ring\n");
> >-	device_init_rd0_ring(priv);
> >-	device_init_rd1_ring(priv);
> >-	device_init_td0_ring(priv);
> >-	device_init_td1_ring(priv);
> >+	ret = device_init_rd0_ring(priv);
> >+	if (ret)
> >+		goto error;
> >+	ret = device_init_rd1_ring(priv);
> >+	if (ret)
> >+		goto error;
> >+	ret = device_init_td0_ring(priv);
> >+	if (ret)
> >+		goto error;
> >+	ret = device_init_td1_ring(priv);
> >+	if (ret)
> >+		goto error;
> >  	device_init_registers(priv);
> >@@ -1178,6 +1226,8 @@ static int vnt_start(struct ieee80211_hw *hw)
> >  	ieee80211_wake_queues(hw);
> >  	return 0;
> >+error:
> >+	return ret;
> >  }
> 
> This code will lead to memory leaks when device_init_rd1_ring()
> fails, because the memory allocated by device_init_rd0_ring() is not
> freed.
> 
> I think this one will be better:
>     ret = device_init_rd0_ring(priv);
>     if (ret)
>         goto error_init_rd0_ring;
>     ret = device_init_rd1_ring(priv);
>     if (ret)
>         goto error_init_rd1_ring;
>     ret = device_init_td0_ring(priv);
>     if (ret)
>         goto error_init_td0_ring;
>     ret = device_init_td1_ring(priv);
>     if (ret)
>         goto error_init_td1_ring;
>     ......
> error_init_td1_ring:
>     device_free_td0_ring(priv);
> error_init_td0_ring:
>     device_free_rd1_ring(priv);
> error_init_rd1_ring:
>     device_free_rd0_ring(priv);
> error_init_rd0_ring:
>     return ret;
> 
> 
> Best wishes,
> Jia-Ju Bai
> 
> 
But, those freeing function are already placed in the each device_init
functions for allocation fail like below. 

@@ -550,20 +554,29 @@ static void device_init_rd0_ring(struct vnt_private *priv)
+       return 0;
+error:
+       device_free_rd0_ring(priv);
+       return ret;

Would freeing in the vnt_start() be better instead of freeing in
device_init_rd0_ring function?

Best regards,
Ji-Hun

Re: [PATCH v3] staging: vt6655: check for memory allocation failures

[Jia-Ju Bai]

On 2018/3/30 11:39, Ji-Hun Kim wrote:
> On Fri, Mar 30, 2018 at 11:15:03AM +0800, Jia-Ju Bai wrote:
>>
>> On 2018/3/30 10:44, Ji-Hun Kim wrote:
>>> @@ -1165,10 +1205,18 @@ static int vnt_start(struct ieee80211_hw *hw)
>>>   	}
>>>   	dev_dbg(&priv->pcid->dev, "call device init rd0 ring\n");
>>> -	device_init_rd0_ring(priv);
>>> -	device_init_rd1_ring(priv);
>>> -	device_init_td0_ring(priv);
>>> -	device_init_td1_ring(priv);
>>> +	ret = device_init_rd0_ring(priv);
>>> +	if (ret)
>>> +		goto error;
>>> +	ret = device_init_rd1_ring(priv);
>>> +	if (ret)
>>> +		goto error;
>>> +	ret = device_init_td0_ring(priv);
>>> +	if (ret)
>>> +		goto error;
>>> +	ret = device_init_td1_ring(priv);
>>> +	if (ret)
>>> +		goto error;
>>>   	device_init_registers(priv);
>>> @@ -1178,6 +1226,8 @@ static int vnt_start(struct ieee80211_hw *hw)
>>>   	ieee80211_wake_queues(hw);
>>>   	return 0;
>>> +error:
>>> +	return ret;
>>>   }
>> This code will lead to memory leaks when device_init_rd1_ring()
>> fails, because the memory allocated by device_init_rd0_ring() is not
>> freed.
>>
>> I think this one will be better:
>>      ret = device_init_rd0_ring(priv);
>>      if (ret)
>>          goto error_init_rd0_ring;
>>      ret = device_init_rd1_ring(priv);
>>      if (ret)
>>          goto error_init_rd1_ring;
>>      ret = device_init_td0_ring(priv);
>>      if (ret)
>>          goto error_init_td0_ring;
>>      ret = device_init_td1_ring(priv);
>>      if (ret)
>>          goto error_init_td1_ring;
>>      ......
>> error_init_td1_ring:
>>      device_free_td0_ring(priv);
>> error_init_td0_ring:
>>      device_free_rd1_ring(priv);
>> error_init_rd1_ring:
>>      device_free_rd0_ring(priv);
>> error_init_rd0_ring:
>>      return ret;
>>
>>
>> Best wishes,
>> Jia-Ju Bai
>>
>>
> But, those freeing function are already placed in the each device_init
> functions for allocation fail like below.

I think it is okay.
I suppose that, for each device_init function, you only free the 
resources allocated in this function, and do not handle the resources in 
other functions.

> @@ -550,20 +554,29 @@ static void device_init_rd0_ring(struct vnt_private *priv)
> +       return 0;
> +error:
> +       device_free_rd0_ring(priv);
> +       return ret;

This code is needed to free the resources allocated in this function.


Best wishes,
Jia-Ju Bai
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3] staging: vt6655: check for memory allocation failures

[Ji-Hun Kim]

On Fri, Mar 30, 2018 at 11:15:03AM +0800, Jia-Ju Bai wrote:
> 
> 
> On 2018/3/30 10:44, Ji-Hun Kim wrote:
> >@@ -1165,10 +1205,18 @@ static int vnt_start(struct ieee80211_hw *hw)
> >  	}
> >  	dev_dbg(&priv->pcid->dev, "call device init rd0 ring\n");
> >-	device_init_rd0_ring(priv);
> >-	device_init_rd1_ring(priv);
> >-	device_init_td0_ring(priv);
> >-	device_init_td1_ring(priv);
> >+	ret = device_init_rd0_ring(priv);
> >+	if (ret)
> >+		goto error;
> >+	ret = device_init_rd1_ring(priv);
> >+	if (ret)
> >+		goto error;
> >+	ret = device_init_td0_ring(priv);
> >+	if (ret)
> >+		goto error;
> >+	ret = device_init_td1_ring(priv);
> >+	if (ret)
> >+		goto error;
> >  	device_init_registers(priv);
> >@@ -1178,6 +1226,8 @@ static int vnt_start(struct ieee80211_hw *hw)
> >  	ieee80211_wake_queues(hw);
> >  	return 0;
> >+error:
> >+	return ret;
> >  }
> 
> This code will lead to memory leaks when device_init_rd1_ring()
> fails, because the memory allocated by device_init_rd0_ring() is not
> freed.
> 
> I think this one will be better:
>     ret = device_init_rd0_ring(priv);
>     if (ret)
>         goto error_init_rd0_ring;
>     ret = device_init_rd1_ring(priv);
>     if (ret)
>         goto error_init_rd1_ring;
>     ret = device_init_td0_ring(priv);
>     if (ret)
>         goto error_init_td0_ring;
>     ret = device_init_td1_ring(priv);
>     if (ret)
>         goto error_init_td1_ring;
>     ......
> error_init_td1_ring:
>     device_free_td0_ring(priv);
> error_init_td0_ring:
>     device_free_rd1_ring(priv);
> error_init_rd1_ring:
>     device_free_rd0_ring(priv);
> error_init_rd0_ring:
>     return ret;
> 
> 
> Best wishes,
> Jia-Ju Bai
> 
> 
Oh, sorry, I got what you said. Yes, you are right. I am going to make patch v4. Thanks!

Best regards,
Ji-Hun

Re: [PATCH v3] staging: vt6655: check for memory allocation failures

[Dan Carpenter]

On Fri, Mar 30, 2018 at 11:44:04AM +0900, Ji-Hun Kim wrote:
> @@ -528,18 +528,22 @@ static void device_free_rings(struct vnt_private *priv)
>  				  priv->tx0_bufs, priv->tx_bufs_dma0);
>  }
>  
> -static void device_init_rd0_ring(struct vnt_private *priv)
> +static int device_init_rd0_ring(struct vnt_private *priv)
>  {
>  	int i;
>  	dma_addr_t      curr = priv->rd0_pool_dma;
>  	struct vnt_rx_desc *desc;
> +	int ret = 0;

Don't initialize "ret".  When you do that it disables static analysis to
find uninitialized variable warnings.

>  
>  	/* Init the RD0 ring entries */
>  	for (i = 0; i < priv->opts.rx_descs0;
>  	     i ++, curr += sizeof(struct vnt_rx_desc)) {
>  		desc = &priv->aRD0Ring[i];
>  		desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
> -
> +		if (!desc->rd_info) {
> +			ret = -ENOMEM;
> +			goto error;
> +		}
>  		if (!device_alloc_rx_buf(priv, desc))
>  			dev_err(&priv->pcid->dev, "can not alloc rx bufs\n");
>  

We need to handle the case where device_alloc_rx_buf() fails as well...

Some years back, I wrote a post about error handling that might be
helpful:
https://plus.google.com/106378716002406849458/posts/dnanfhQ4mHQ

You are using "one err" and "do nothing" style error handling which are
described in the post.


> @@ -550,20 +554,29 @@ static void device_init_rd0_ring(struct vnt_private *priv)
>  	if (i > 0)
>  		priv->aRD0Ring[i-1].next_desc = cpu_to_le32(priv->rd0_pool_dma);
>  	priv->pCurrRD[0] = &priv->aRD0Ring[0];
> +
> +	return 0;
> +error:
> +	device_free_rd0_ring(priv);
> +	return ret;
>  }

Of course, Jia-Ju Bai is correct to say that this is a layering
violation.  Each function should only clean up after its self.

Also, this is a very typical "one err" style bug which I explain about
in my g+ post.  The rule that applies here is that you should only free
things which have been allocated.  Since we only partially allocated the
rd0 ring, device_free_rd0_ring() will crash when we do:

		dma_unmap_single(&priv->pcid->dev, rd_info->skb_dma,
				 priv->rx_buf_sz, DMA_FROM_DEVICE);

"rd_info" is NULL so rd_info->skb_dma is a NULL dereference.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3] staging: vt6655: check for memory allocation failures

[Ji-Hun Kim]

On Tue, Apr 03, 2018 at 01:40:52PM +0300, Dan Carpenter wrote:
> >  		desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
> > -
> > +		if (!desc->rd_info) {
> > +			ret = -ENOMEM;
> > +			goto error;
> > +		}
> >  		if (!device_alloc_rx_buf(priv, desc))
> >  			dev_err(&priv->pcid->dev, "can not alloc rx bufs\n");
> >  
> 
> We need to handle the case where device_alloc_rx_buf() fails as well...

Hi Dan, thanks for your comments! I will write a patch v5 including this fix.

> Some years back, I wrote a post about error handling that might be
> helpful:
> https://plus.google.com/106378716002406849458/posts/dnanfhQ4mHQ

This post is very helpful to me, Thank you.

> You are using "one err" and "do nothing" style error handling which are
> described in the post.

Sorry, I didn't fully understand "one err" and "do nothing" style error
handling of this code. The reason using goto instead of returns directly
was that for freeing previously allocated "rd_info"s in the for loop.
Please see below comment.


> > @@ -550,20 +554,29 @@ static void device_init_rd0_ring(struct vnt_private *priv)
> >  	if (i > 0)
> >  		priv->aRD0Ring[i-1].next_desc = cpu_to_le32(priv->rd0_pool_dma);
> >  	priv->pCurrRD[0] = &priv->aRD0Ring[0];
> > +
> > +	return 0;
> > +error:
> > +	device_free_rd0_ring(priv);
> > +	return ret;
> >  }
> 
> Of course, Jia-Ju Bai is correct to say that this is a layering
> violation.  Each function should only clean up after its self.
> 
> Also, this is a very typical "one err" style bug which I explain about
> in my g+ post.  The rule that applies here is that you should only free
> things which have been allocated.  Since we only partially allocated the
> rd0 ring, device_free_rd0_ring() will crash when we do:
> 
> 		dma_unmap_single(&priv->pcid->dev, rd_info->skb_dma,
> 				 priv->rx_buf_sz, DMA_FROM_DEVICE);
> 
> "rd_info" is NULL so rd_info->skb_dma is a NULL dereference.

For dealing with this problem, I added below code on patch v3. I think it
would not make Null dereferencing issues, because it will not enter 
dma_unmap_single(), if "rd_info" is Null.

+               if (rd_info) {
+                       dma_unmap_single(&priv->pcid->dev, rd_info->skb_dma,
+                                       priv->rx_buf_sz, DMA_FROM_DEVICE);
+                       dev_kfree_skb(rd_info->skb);
+                       kfree(desc->rd_info);
+               }

I would appreciate for your opinions what would be better way for freeing
allocated "rd_info"s in the loop previously.

Best regards,
Ji-Hun
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3] staging: vt6655: check for memory allocation failures

[Dan Carpenter]

On Wed, Apr 04, 2018 at 04:24:10PM +0900, Ji-Hun Kim wrote:
> > Since we only partially allocated the
> > rd0 ring, device_free_rd0_ring() will crash when we do:
> > 
> > 		dma_unmap_single(&priv->pcid->dev, rd_info->skb_dma,
> > 				 priv->rx_buf_sz, DMA_FROM_DEVICE);
> > 
> > "rd_info" is NULL so rd_info->skb_dma is a NULL dereference.
> 
> For dealing with this problem, I added below code on patch v3. I think it
> would not make Null dereferencing issues, because it will not enter 
> dma_unmap_single(), if "rd_info" is Null.
> 
> +               if (rd_info) {
> +                       dma_unmap_single(&priv->pcid->dev, rd_info->skb_dma,
> +                                       priv->rx_buf_sz, DMA_FROM_DEVICE);
> +                       dev_kfree_skb(rd_info->skb);
> +                       kfree(desc->rd_info);
> +               }
> 
> I would appreciate for your opinions what would be better way for freeing
> allocated "rd_info"s in the loop previously.

Of course, that works for this particular bug but I'm not a fan of that
approach...

When I say "do nothing" gotos, I mean.

err:
	return ret;

And what I mean by "one err" is this:

err:
	free(a);
	free(b)
	free(c);

	return ret;

There is only one error label even though we are freeing three things.
The problem with that is that you might be freeing something this hasn't
been allocated like "rd_info->skb_dma" but "rd_info" wasn't allocated.
It's a very normally problem to have with this style of error handling.

Most kernel error handling is very simple.

err_free_c:
	free(c);
err_free_b:
	free(b);
err_free_a:
	free(a);

	return ret;

Side note:  Part of the problem here is that device_alloc_rx_buf() does
not have a corresponding free function.  Every alloc should have a
matching free function so that if we ever change the alloc function, we
just have to update one free function.  And it's just cleaner.

static void device_free_rx_buf(struct vnt_private *priv,
			       struct vnt_rx_desc *rd)
{
	struct vnt_rd_info *rd_info = rd->rd_info;

	dma_unmap_single(&priv->pcid->dev, rd_info->skb_dma, priv->rx_buf_sz,
			 DMA_FROM_DEVICE);
	dev_kfree_skb(rd_info->skb);
}

Maybe send a patch that adds that and changes device_free_rd0_ring() to
use the new free function instead of open coding it.  [PATCH 1/2].

These particular functions are slighttly complicated because they have
loops and the error handling for loops is tricky and bug prone.  Here is
what I would like the error handling to look like:

diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c
index 0dc902022a91..e12a959fe080 100644
--- a/drivers/staging/vt6655/device_main.c
+++ b/drivers/staging/vt6655/device_main.c
@@ -533,15 +533,23 @@ static void device_init_rd0_ring(struct vnt_private *priv)
 	int i;
 	dma_addr_t      curr = priv->rd0_pool_dma;
 	struct vnt_rx_desc *desc;
+	int ret;
 
 	/* Init the RD0 ring entries */
 	for (i = 0; i < priv->opts.rx_descs0;
 	     i ++, curr += sizeof(struct vnt_rx_desc)) {
 		desc = &priv->aRD0Ring[i];
 		desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
+		if (!desc->rd_info) {
+			ret = -ENOMEM;
+			goto err_free_descs;
+		}
 
-		if (!device_alloc_rx_buf(priv, desc))
+		if (!device_alloc_rx_buf(priv, desc)) {
 			dev_err(&priv->pcid->dev, "can not alloc rx bufs\n");
+			ret = -ENOMEM;
+			goto err_free_rd;
+		}
 
 		desc->next = &(priv->aRD0Ring[(i + 1) % priv->opts.rx_descs0]);
 		desc->next_desc = cpu_to_le32(curr + sizeof(struct vnt_rx_desc));
@@ -550,6 +558,20 @@ static void device_init_rd0_ring(struct vnt_private *priv)
 	if (i > 0)
 		priv->aRD0Ring[i-1].next_desc = cpu_to_le32(priv->rd0_pool_dma);
 	priv->pCurrRD[0] = &priv->aRD0Ring[0];
+
+	return 0;
+
+err_free_rd:
+	kfree(desc->rd_info);
+
+err_free_descs:
+	while (--i) {
+		desc = &priv->aRD0Ring[i];
+		device_free_rx_buf(priv, desc);
+		kfree(desc->rd_info);
+	}
+
+	return ret;
 }
 
 static void device_init_rd1_ring(struct vnt_private *priv)

It's more work to do it this way.  It's also more lines of code, but
it's easier to read and review because you can look at the function and
see that it's correct without jumping to other functions to verify that
they check if rd_info is NULL or not.

regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel