[PATCH 1/2] tpm: replace kmalloc() + memcpy() with kmemdup()
Use kmemdup rather than duplicating its implementation. Signed-off-by: Ji-Hun Kim --- drivers/char/tpm/eventlog/of.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c index b7cac47..bba5fba 100644 --- a/drivers/char/tpm/eventlog/of.c +++ b/drivers/char/tpm/eventlog/of.c @@ -69,14 +69,12 @@ int tpm_read_log_of(struct tpm_chip *chip) return -EIO; } - log->bios_event_log = kmalloc(size, GFP_KERNEL); + log->bios_event_log = kmemdup(__va(base), size, GFP_KERNEL); if (!log->bios_event_log) return -ENOMEM; log->bios_event_log_end = log->bios_event_log + size; - memcpy(log->bios_event_log, __va(base), size); - if (chip->flags & TPM_CHIP_FLAG_TPM2) return EFI_TCG2_EVENT_LOG_FORMAT_TCG_2; return EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2; -- 1.9.1
[PATCH 2/2] tpm: replace kmalloc() + memcpy() with kmemdup()
Use kmemdup rather than duplicating its implementation. Signed-off-by: Ji-Hun Kim --- drivers/char/tpm/eventlog/efi.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/char/tpm/eventlog/efi.c b/drivers/char/tpm/eventlog/efi.c index 68f1e7e..3e673ab 100644 --- a/drivers/char/tpm/eventlog/efi.c +++ b/drivers/char/tpm/eventlog/efi.c @@ -51,10 +51,9 @@ int tpm_read_log_efi(struct tpm_chip *chip) } /* malloc EventLog space */ - log->bios_event_log = kmalloc(log_size, GFP_KERNEL); + log->bios_event_log = kmemdup(log_tbl->log, log_size, GFP_KERNEL); if (!log->bios_event_log) goto err_memunmap; - memcpy(log->bios_event_log, log_tbl->log, log_size); log->bios_event_log_end = log->bios_event_log + log_size; tpm_log_version = log_tbl->version; -- 1.9.1
[PATCH] staging: ks7010: replace kmalloc() + memcpy() with kmemdup()
Use kmemdup rather than duplicating its implementation. Signed-off-by: Ji-Hun Kim --- drivers/staging/ks7010/ks7010_sdio.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/staging/ks7010/ks7010_sdio.c b/drivers/staging/ks7010/ks7010_sdio.c index b8f55a1..c8eb55b 100644 --- a/drivers/staging/ks7010/ks7010_sdio.c +++ b/drivers/staging/ks7010/ks7010_sdio.c @@ -589,11 +589,10 @@ static int ks7010_sdio_update_index(struct ks_wlan_private *priv, u32 index) int ret; unsigned char *data_buf; - data_buf = kmalloc(sizeof(u32), GFP_KERNEL); + data_buf = kmemdup(&index, sizeof(u32), GFP_KERNEL); if (!data_buf) return -ENOMEM; - memcpy(data_buf, &index, sizeof(index)); ret = ks7010_sdio_write(priv, WRITE_INDEX, data_buf, sizeof(index)); if (ret) goto err_free_data_buf; -- 1.9.1
[PATCH v5 2/2] staging: vt6655: add handling memory leak on vnt_start()
There was no code for handling memory leaks of device_init_rings() and request_irq(). It needs to free allocated memory in the device_init_rings() , when request_irq() would be failed. Add freeing sequences of irq and device init rings. Signed-off-by: Ji-Hun Kim --- It's additional memory leak handling patch from the [PATCH v5 1/2] staging: vt6655: check for memory allocation failures Changes v2: - Change label names following coding-style conventions. - Remove uneccessary goto, just return error number like original code. drivers/staging/vt6655/device_main.c | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c index 700c03c..1ab0e85 100644 --- a/drivers/staging/vt6655/device_main.c +++ b/drivers/staging/vt6655/device_main.c @@ -1237,13 +1237,13 @@ static int vnt_start(struct ieee80211_hw *hw) IRQF_SHARED, "vt6655", priv); if (ret) { dev_dbg(&priv->pcid->dev, "failed to start irq\n"); - return ret; + goto err_free_rings; } dev_dbg(&priv->pcid->dev, "call device init rd0 ring\n"); ret = device_init_rd0_ring(priv); if (ret) - return ret; + goto err_free_irq; ret = device_init_rd1_ring(priv); if (ret) goto err_free_rd0_ring; @@ -1269,6 +1269,10 @@ static int vnt_start(struct ieee80211_hw *hw) device_free_rd1_ring(priv); err_free_rd0_ring: device_free_rd0_ring(priv); +err_free_irq: + free_irq(priv->pcid->irq, priv); +err_free_rings: + device_free_rings(priv); return ret; } -- 1.9.1
[PATCH v5 1/2] staging: vt6655: check for memory allocation failures
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 --- Changes v5: - Add error handling case for device_alloc_rx_buf() failures. - Add device_free_rx_buf() which is corresponding free function of device_allocated_rx_buf(). And change duplicated codes by this function. - Modify error handling code about freeing allocated value in the loops to more proper ways. - Change goto label names following coding-style conventions. Changes v4: - Fix potential memory leaks from error handling code from device init functions in vnt_start(). 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. 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. drivers/staging/vt6655/device_main.c | 144 --- 1 file changed, 118 insertions(+), 26 deletions(-) diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c index fbc4bc6..700c03c 100644 --- a/drivers/staging/vt6655/device_main.c +++ b/drivers/staging/vt6655/device_main.c @@ -19,6 +19,7 @@ * device_print_info - print out resource * device_rx_srv - rx service function * device_alloc_rx_buf - rx buffer pre-allocated function + * device_free_rx_buf - free rx buffer function * device_free_tx_buf - free tx buffer function * device_init_rd0_ring- initial rd dma0 ring * device_init_rd1_ring- initial rd dma1 ring @@ -124,14 +125,15 @@ 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); static bool device_alloc_rx_buf(struct vnt_private *, struct vnt_rx_desc *); +static void device_free_rx_buf(struct vnt_private *priv, struct vnt_rx_desc *rd); static void device_init_registers(struct vnt_private *priv); static void device_free_tx_buf(struct vnt_private *, struct vnt_tx_desc *); static void device_free_td0_ring(struct vnt_private *priv); @@ -528,20 +530,28 @@ 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; /* 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_desc; + } - 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,22 +560,44 @@ 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_desc: + 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) +static int device_init_rd1_ring(struct vnt
Re: [PATCH v3] staging: vt6655: check for memory allocation failures
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
[PATCH v4 2/2] staging: vt6655: add handling memory leak on vnt_start()
There was no code for handling memory leaks of device_init_rings() and request_irq(). It needs to free allocated memory in the device_init_rings() , when request_irq() is failed. Add freeing sequences of irq and device init rings. Signed-off-by: Ji-Hun Kim --- It's additional memory leak handling patch from [PATCH v4 1/2] staging: vt6655: check for memory allocation failures drivers/staging/vt6655/device_main.c | 13 ++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c index c9752df..3604f2d 100644 --- a/drivers/staging/vt6655/device_main.c +++ b/drivers/staging/vt6655/device_main.c @@ -1194,14 +1194,17 @@ static int vnt_start(struct ieee80211_hw *hw) int ret; priv->rx_buf_sz = PKT_BUF_SZ; - if (!device_init_rings(priv)) - return -ENOMEM; + ret = (int)device_init_rings(priv); + if (!ret) { + ret = -ENOMEM; + goto err_init_rings; + } ret = request_irq(priv->pcid->irq, vnt_interrupt, IRQF_SHARED, "vt6655", priv); if (ret) { dev_dbg(&priv->pcid->dev, "failed to start irq\n"); - return ret; + goto err_irq; } dev_dbg(&priv->pcid->dev, "call device init rd0 ring\n"); @@ -1234,6 +1237,10 @@ static int vnt_start(struct ieee80211_hw *hw) err_init_rd1_ring: device_free_rd0_ring(priv); err_init_rd0_ring: + free_irq(priv->pcid->irq, priv); +err_irq: + device_free_rings(priv); +err_init_rings: return ret; } -- 1.9.1
[PATCH v4 1/2] staging: vt6655: check for memory allocation failures
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 --- 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. Changes v4: - Fix potential memory leaks from error handling code of device init functions in vnt_start(). drivers/staging/vt6655/device_main.c | 121 ++- 1 file changed, 89 insertions(+), 32 deletions(-) diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c index fbc4bc6..c9752df 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_unm
Re: [PATCH v3] staging: vt6655: check for memory allocation failures
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
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
[PATCH v3] staging: vt6655: check for memory allocation failures
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 --- 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-&
Re: [PATCH v2] staging: vt6655: check for memory allocation failures
2018-03-29 17:00 GMT+09:00 Jia-Ju Bai : > > > > On 2018/3/29 15:22, Ji-Hun Kim wrote: >> >> 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. Add return when allocation is failed. >> >> Signed-off-by: Ji-Hun Kim >> --- >> >> Change: since v1: >> >> - 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, if td_info and rd_info are not allocated, no >>needs to free. >> >> drivers/staging/vt6655/device_main.c | 64 >> +--- >> 1 file changed, 44 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/staging/vt6655/device_main.c >> b/drivers/staging/vt6655/device_main.c >> index fbc4bc6..ecbba43 100644 >> --- a/drivers/staging/vt6655/device_main.c >> +++ b/drivers/staging/vt6655/device_main.c >> @@ -539,7 +539,8 @@ static void device_init_rd0_ring(struct vnt_private >> *priv) >> 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) >> + goto error; >> if (!device_alloc_rx_buf(priv, desc)) >> dev_err(&priv->pcid->dev, "can not alloc rx bufs\n"); >> @@ -550,6 +551,10 @@ 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; >> +error: >> + device_free_rd0_ring(priv); >> } >> > > > I think you should return an error number here, because > device_init_rd0_ring() is called by vnt_start(). > You should also implement error handling code in vnt_start(), and let > vnt_start() returns an error number too. > The same for device_init_rd1_ring(), device_init_td0_ring() and > device_init_td1_ring(). > Hi Jia-Ju, Thanks for the feedback. All right, those function looks that needs returns an error number. I will implement error handling code and send a patch v3 tomorrow Best regards, Ji-Hun
[PATCH v2] staging: vt6655: check for memory allocation failures
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. Add return when allocation is failed. Signed-off-by: Ji-Hun Kim --- Change: since v1: - 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, if td_info and rd_info are not allocated, no needs to free. drivers/staging/vt6655/device_main.c | 64 +--- 1 file changed, 44 insertions(+), 20 deletions(-) diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c index fbc4bc6..ecbba43 100644 --- a/drivers/staging/vt6655/device_main.c +++ b/drivers/staging/vt6655/device_main.c @@ -539,7 +539,8 @@ static void device_init_rd0_ring(struct vnt_private *priv) 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) + goto error; if (!device_alloc_rx_buf(priv, desc)) dev_err(&priv->pcid->dev, "can not alloc rx bufs\n"); @@ -550,6 +551,10 @@ 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; +error: + device_free_rd0_ring(priv); } static void device_init_rd1_ring(struct vnt_private *priv) @@ -563,7 +568,8 @@ static void device_init_rd1_ring(struct vnt_private *priv) 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) + goto error; if (!device_alloc_rx_buf(priv, desc)) dev_err(&priv->pcid->dev, "can not alloc rx bufs\n"); @@ -574,6 +580,10 @@ 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; +error: + device_free_rd1_ring(priv); } static void device_free_rd0_ring(struct vnt_private *priv) @@ -584,12 +594,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,12 +611,12 @@ 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); + } } } @@ -621,7 +631,8 @@ static void device_init_td0_ring(struct vnt_private *priv) 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) + 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,6 +643,10 @@ 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-&g
Re: [PATCH] staging: vt6655: check for memory allocation failures
On Wed, Mar 28, 2018 at 05:55:57PM +0800, Jia-Ju Bai wrote: > >@@ -646,7 +649,8 @@ 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 (WARN_ON(!desc->td_info)) > >+return; > > desc->td_info->buf = priv->tx1_bufs + i * PKT_BUF_SZ; > > desc->td_info->buf_dma = priv->tx_bufs_dma1 + i * PKT_BUF_SZ; > > I think the bugs you found are right. > But your patch is not correct, because it is dangerous to return directly. > I think you should return an error and then implement error handling > code for these functions. > Yes, it needs to free previous allocated values in the for loop. Directly return could make memory leaks. I am going to make patch v2. - Delete WARN_ON which could make crashes on some machines. - Add freeing sequences for previously allocated memory when kzalloc() failed instead of returning directly. Does these changes would be fine on this bug?
[PATCH] staging: vt6655: check for memory allocation failures
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. Add return when allocation is failed. Signed-off-by: Ji-Hun Kim --- drivers/staging/vt6655/device_main.c | 12 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c index fbc4bc6..5d0ba94 100644 --- a/drivers/staging/vt6655/device_main.c +++ b/drivers/staging/vt6655/device_main.c @@ -539,7 +539,8 @@ static void device_init_rd0_ring(struct vnt_private *priv) i ++, curr += sizeof(struct vnt_rx_desc)) { desc = &priv->aRD0Ring[i]; desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL); - + if (WARN_ON(!desc->rd_info)) + return; if (!device_alloc_rx_buf(priv, desc)) dev_err(&priv->pcid->dev, "can not alloc rx bufs\n"); @@ -563,7 +564,8 @@ static void device_init_rd1_ring(struct vnt_private *priv) i ++, curr += sizeof(struct vnt_rx_desc)) { desc = &priv->aRD1Ring[i]; desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL); - + if (WARN_ON(!desc->rd_info)) + return; if (!device_alloc_rx_buf(priv, desc)) dev_err(&priv->pcid->dev, "can not alloc rx bufs\n"); @@ -621,7 +623,8 @@ static void device_init_td0_ring(struct vnt_private *priv) i++, curr += sizeof(struct vnt_tx_desc)) { desc = &priv->apTD0Rings[i]; desc->td_info = kzalloc(sizeof(*desc->td_info), GFP_KERNEL); - + if (WARN_ON(!desc->td_info)) + return; desc->td_info->buf = priv->tx0_bufs + i * PKT_BUF_SZ; desc->td_info->buf_dma = priv->tx_bufs_dma0 + i * PKT_BUF_SZ; @@ -646,7 +649,8 @@ 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 (WARN_ON(!desc->td_info)) + return; desc->td_info->buf = priv->tx1_bufs + i * PKT_BUF_SZ; desc->td_info->buf_dma = priv->tx_bufs_dma1 + i * PKT_BUF_SZ; -- 1.9.1
Re: [PATCH v3 1/2] staging: media: davinci_vpfe: add error handling on kmalloc failure
On Wed, Mar 21, 2018 at 01:39:09PM +0900, Ji-Hun Kim wrote: > There is no failure checking on the param value which will be allocated > memory by kmalloc. Add a null pointer checking statement. Then goto error: > and return -ENOMEM error code when kmalloc is failed. > > Signed-off-by: Ji-Hun Kim > --- > Changes since v1: > - Return with -ENOMEM directly, instead of goto error: then return. > - [Patch v3 1/2] is same with [patch v2] > > drivers/staging/media/davinci_vpfe/dm365_ipipe.c | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c > b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c > index 6a3434c..ffcd86d 100644 > --- a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c > +++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c > @@ -1280,6 +1280,9 @@ static int ipipe_s_config(struct v4l2_subdev *sd, > struct vpfe_ipipe_config *cfg) > > params = kmalloc(sizeof(struct ipipe_module_params), >GFP_KERNEL); > + if (!params) > + return -ENOMEM; > + > to = (void *)params + module_if->param_offset; > size = module_if->param_size; > > @@ -1323,6 +1326,9 @@ static int ipipe_g_config(struct v4l2_subdev *sd, > struct vpfe_ipipe_config *cfg) > > params = kmalloc(sizeof(struct ipipe_module_params), > GFP_KERNEL); > + if (!params) > + return -ENOMEM; > + > from = (void *)params + module_if->param_offset; > size = module_if->param_size; > > -- > 1.9.1 > > Are there any opinions? I'd like to know how this patch is going. Best regards, Ji-Hun
[PATCH v3 2/2] staging: media: davinci_vpfe: add kfree() on goto err statement
It needs to free of allocated params value in the goto error statement. Signed-off-by: Ji-Hun Kim --- Changes since v2: - add kfree(params) on the error case of the function - rename unclear goto statement name - declare the params value at start of the function, so it can be free end of the function drivers/staging/media/davinci_vpfe/dm365_ipipe.c | 24 +++- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c index ffcd86d..735d8b5 100644 --- a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c +++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c @@ -1263,6 +1263,7 @@ static int ipipe_get_cgs_params(struct vpfe_ipipe_device *ipipe, void *param) static int ipipe_s_config(struct v4l2_subdev *sd, struct vpfe_ipipe_config *cfg) { struct vpfe_ipipe_device *ipipe = v4l2_get_subdevdata(sd); + struct ipipe_module_params *params; unsigned int i; int rval = 0; @@ -1272,7 +1273,6 @@ static int ipipe_s_config(struct v4l2_subdev *sd, struct vpfe_ipipe_config *cfg) if (cfg->flag & bit) { const struct ipipe_module_if *module_if = &ipipe_modules[i]; - struct ipipe_module_params *params; void __user *from = *(void * __user *) ((void *)cfg + module_if->config_offset); size_t size; @@ -1289,26 +1289,30 @@ static int ipipe_s_config(struct v4l2_subdev *sd, struct vpfe_ipipe_config *cfg) if (to && from && size) { if (copy_from_user(to, from, size)) { rval = -EFAULT; - break; + goto err_free_params; } rval = module_if->set(ipipe, to); if (rval) - goto error; + goto err_free_params; } else if (to && !from && size) { rval = module_if->set(ipipe, NULL); if (rval) - goto error; + goto err_free_params; } kfree(params); } } -error: + return 0; + +err_free_params: + kfree(params); return rval; } static int ipipe_g_config(struct v4l2_subdev *sd, struct vpfe_ipipe_config *cfg) { struct vpfe_ipipe_device *ipipe = v4l2_get_subdevdata(sd); + struct ipipe_module_params *params; unsigned int i; int rval = 0; @@ -1318,7 +1322,6 @@ static int ipipe_g_config(struct v4l2_subdev *sd, struct vpfe_ipipe_config *cfg) if (cfg->flag & bit) { const struct ipipe_module_if *module_if = &ipipe_modules[i]; - struct ipipe_module_params *params; void __user *to = *(void * __user *) ((void *)cfg + module_if->config_offset); size_t size; @@ -1335,16 +1338,19 @@ static int ipipe_g_config(struct v4l2_subdev *sd, struct vpfe_ipipe_config *cfg) if (to && from && size) { rval = module_if->get(ipipe, from); if (rval) - goto error; + goto err_free_params; if (copy_to_user(to, from, size)) { rval = -EFAULT; - break; + goto err_free_params; } } kfree(params); } } -error: + return 0; + +err_free_params: + kfree(params); return rval; } -- 1.9.1
[PATCH v3 1/2] staging: media: davinci_vpfe: add error handling on kmalloc failure
There is no failure checking on the param value which will be allocated memory by kmalloc. Add a null pointer checking statement. Then goto error: and return -ENOMEM error code when kmalloc is failed. Signed-off-by: Ji-Hun Kim --- Changes since v1: - Return with -ENOMEM directly, instead of goto error: then return. - [Patch v3 1/2] is same with [patch v2] drivers/staging/media/davinci_vpfe/dm365_ipipe.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c index 6a3434c..ffcd86d 100644 --- a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c +++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c @@ -1280,6 +1280,9 @@ static int ipipe_s_config(struct v4l2_subdev *sd, struct vpfe_ipipe_config *cfg) params = kmalloc(sizeof(struct ipipe_module_params), GFP_KERNEL); + if (!params) + return -ENOMEM; + to = (void *)params + module_if->param_offset; size = module_if->param_size; @@ -1323,6 +1326,9 @@ static int ipipe_g_config(struct v4l2_subdev *sd, struct vpfe_ipipe_config *cfg) params = kmalloc(sizeof(struct ipipe_module_params), GFP_KERNEL); + if (!params) + return -ENOMEM; + from = (void *)params + module_if->param_offset; size = module_if->param_size; -- 1.9.1
[PATCH v2] staging: media: davinci_vpfe: add error handling on kmalloc failure
There is no failure checking on the param value which will be allocated memory by kmalloc. Add a null pointer checking statement. Then goto error: and return -ENOMEM error code when kmalloc is failed. Signed-off-by: Ji-Hun Kim --- Changes since v1: - Return with -ENOMEM directly, instead of goto error: then return. drivers/staging/media/davinci_vpfe/dm365_ipipe.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c index 6a3434c..ffcd86d 100644 --- a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c +++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c @@ -1280,6 +1280,9 @@ static int ipipe_s_config(struct v4l2_subdev *sd, struct vpfe_ipipe_config *cfg) params = kmalloc(sizeof(struct ipipe_module_params), GFP_KERNEL); + if (!params) + return -ENOMEM; + to = (void *)params + module_if->param_offset; size = module_if->param_size; @@ -1323,6 +1326,9 @@ static int ipipe_g_config(struct v4l2_subdev *sd, struct vpfe_ipipe_config *cfg) params = kmalloc(sizeof(struct ipipe_module_params), GFP_KERNEL); + if (!params) + return -ENOMEM; + from = (void *)params + module_if->param_offset; size = module_if->param_size; -- 1.9.1
Re: Re: [PATCH] staging: media: davinci_vpfe: add error handling on kmalloc failure
On Fri, Mar 16, 2018 at 11:32:34AM +0300, Dan Carpenter wrote: > On Fri, Mar 16, 2018 at 01:58:23PM +0900, Ji-Hun Kim wrote: > > There is no failure checking on the param value which will be allocated > > memory by kmalloc. Add a null pointer checking statement. Then goto error: > > and return -ENOMEM error code when kmalloc is failed. > > > > Signed-off-by: Ji-Hun Kim > > --- > > drivers/staging/media/davinci_vpfe/dm365_ipipe.c | 8 > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c > > b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c > > index 6a3434c..55a922c 100644 > > --- a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c > > +++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c > > @@ -1280,6 +1280,10 @@ static int ipipe_s_config(struct v4l2_subdev *sd, > > struct vpfe_ipipe_config *cfg) > > > > params = kmalloc(sizeof(struct ipipe_module_params), > > GFP_KERNEL); > > + if (!params) { > > + rval = -ENOMEM; > > + goto error; > ^^ > > What does "goto error" do, do you think? It's not clear from the name. > When you have an unclear goto like this it often means the error > handling is going to be buggy. > > In this case, it does nothing so a direct "return -ENOMEM;" would be > more clear. But the rest of the error handling is buggy. Hi Dan, I appreciate for your specific feedbacks. It looks more clear. And I'd like you to see my question below. I will send the patch v2. > > 1263 static int ipipe_s_config(struct v4l2_subdev *sd, struct > vpfe_ipipe_config *cfg) > 1264 { > 1265 struct vpfe_ipipe_device *ipipe = v4l2_get_subdevdata(sd); > 1266 unsigned int i; > 1267 int rval = 0; > 1268 > 1269 for (i = 0; i < ARRAY_SIZE(ipipe_modules); i++) { > 1270 unsigned int bit = 1 << i; > 1271 > 1272 if (cfg->flag & bit) { > 1273 const struct ipipe_module_if *module_if = > 1274 &ipipe_modules[i]; > 1275 struct ipipe_module_params *params; > 1276 void __user *from = *(void * __user *) > 1277 ((void *)cfg + > module_if->config_offset); > 1278 size_t size; > 1279 void *to; > 1280 > 1281 params = kmalloc(sizeof(struct > ipipe_module_params), > 1282 GFP_KERNEL); > > Do a direct return: > > if (!params) > return -ENOMEM; > > 1283 to = (void *)params + module_if->param_offset; > 1284 size = module_if->param_size; > 1285 > 1286 if (to && from && size) { > 1287 if (copy_from_user(to, from, size)) { > 1288 rval = -EFAULT; > 1289 break; > > The most recent thing we allocated is "params" so lets do a > "goto free_params;". We'll have to declare "params" at the start of the > function instead inside this block. > > 1290 } > 1291 rval = module_if->set(ipipe, to); > 1292 if (rval) > 1293 goto error; > > goto free_params again since params is still the most recent thing we > allocated. > > 1294 } else if (to && !from && size) { > 1295 rval = module_if->set(ipipe, NULL); > 1296 if (rval) > 1297 goto error; > > And here again goto free_params. > > 1298 } > 1299 kfree(params); > 1300 } > 1301 } > 1302 error: > 1303 return rval; > > > Change this to: > > return 0; Instead of returning rval, returning 0 would be fine? It looks that should return rval in normal case. > > free_params: > kfree(params); > return rval; > > 1304 } > > regards, > dan carpenter > > Thanks, Ji-Hun
[PATCH] staging: media: davinci_vpfe: add error handling on kmalloc failure
There is no failure checking on the param value which will be allocated memory by kmalloc. Add a null pointer checking statement. Then goto error: and return -ENOMEM error code when kmalloc is failed. Signed-off-by: Ji-Hun Kim --- drivers/staging/media/davinci_vpfe/dm365_ipipe.c | 8 1 file changed, 8 insertions(+) diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c index 6a3434c..55a922c 100644 --- a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c +++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c @@ -1280,6 +1280,10 @@ static int ipipe_s_config(struct v4l2_subdev *sd, struct vpfe_ipipe_config *cfg) params = kmalloc(sizeof(struct ipipe_module_params), GFP_KERNEL); + if (!params) { + rval = -ENOMEM; + goto error; + } to = (void *)params + module_if->param_offset; size = module_if->param_size; @@ -1323,6 +1327,10 @@ static int ipipe_g_config(struct v4l2_subdev *sd, struct vpfe_ipipe_config *cfg) params = kmalloc(sizeof(struct ipipe_module_params), GFP_KERNEL); + if (!params) { + rval = -ENOMEM; + goto error; + } from = (void *)params + module_if->param_offset; size = module_if->param_size; -- 1.9.1
[PATCH] staging: rtl8723bs: core: rtw_cmd: remove unnecessary initialization
Clean up checkpatch error: ERROR: do not initialise globals to 0 Signed-off-by: Ji-Hun Kim --- drivers/staging/rtl8723bs/core/rtw_cmd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c b/drivers/staging/rtl8723bs/core/rtw_cmd.c index af0a9e0..9e132f9 100644 --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c @@ -1742,7 +1742,7 @@ u8 rtw_ps_cmd(struct adapter *padapter) return res; } -u32 g_wait_hiq_empty = 0; +u32 g_wait_hiq_empty; static void rtw_chk_hi_queue_hdl(struct adapter *padapter) { -- 1.9.1
[PATCH 2/2] staging: iio: add spaces around '-' operator
Clean up checkpatch warning: CHECK: spaces preferred around that '-' (ctx:VxV) Signed-off-by: Ji-Hun Kim --- drivers/staging/iio/adc/ad7192.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c index f015955..c4eff71 100644 --- a/drivers/staging/iio/adc/ad7192.c +++ b/drivers/staging/iio/adc/ad7192.c @@ -340,7 +340,7 @@ ad7192_show_scale_available(struct device *dev, } static IIO_DEVICE_ATTR_NAMED(in_v_m_v_scale_available, -in_voltage-voltage_scale_available, +in_voltage - voltage_scale_available, 0444, ad7192_show_scale_available, NULL, 0); static IIO_DEVICE_ATTR(in_voltage_scale_available, 0444, -- 2.10.1 (Apple Git-78)
[PATCH 1/2] staging: iio: remove unnecessary parentheses
Clean up checkpatch warning: CHECK: Unnecessary parentheses around 'st->devid != ID_AD7195' Signed-off-by: Ji-Hun Kim --- drivers/staging/iio/adc/ad7192.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c index cadfb96..f015955 100644 --- a/drivers/staging/iio/adc/ad7192.c +++ b/drivers/staging/iio/adc/ad7192.c @@ -271,7 +271,7 @@ static int ad7192_setup(struct ad7192_state *st, if (pdata->sinc3_en) st->mode |= AD7192_MODE_SINC3; - if (pdata->refin2_en && (st->devid != ID_AD7195)) + if (pdata->refin2_en && st->devid != ID_AD7195) st->conf |= AD7192_CONF_REFSEL; if (pdata->chop_en) { -- 2.10.1 (Apple Git-78)
[PATCH 3/3] staging: irda: separate multiple assignments
Clean up checkpatch warning: CHECK: multiple assignments should be avoided Signed-off-by: JI-HUN KIM --- drivers/staging/irda/drivers/esi-sir.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/staging/irda/drivers/esi-sir.c b/drivers/staging/irda/drivers/esi-sir.c index 00866a3..01097f1 100644 --- a/drivers/staging/irda/drivers/esi-sir.c +++ b/drivers/staging/irda/drivers/esi-sir.c @@ -104,7 +104,8 @@ static int esi_change_speed(struct sir_dev *dev, unsigned int speed) rts = FALSE; break; case 115200: - dtr = rts = TRUE; + dtr = TRUE; + rts = TRUE; break; default: ret = -EINVAL; -- 2.10.1 (Apple Git-78)
[PATCH 2/3] staging: irda: add spaces around '|' operator
Clean up checkpatch warning: CHECK: spaces preferred around that '|' (ctx:VxV) Signed-off-by: JI-HUN KIM --- drivers/staging/irda/drivers/esi-sir.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/irda/drivers/esi-sir.c b/drivers/staging/irda/drivers/esi-sir.c index a12cf55..00866a3 100644 --- a/drivers/staging/irda/drivers/esi-sir.c +++ b/drivers/staging/irda/drivers/esi-sir.c @@ -69,7 +69,7 @@ static int esi_open(struct sir_dev *dev) /* Power up and set dongle to 9600 baud */ sirdev_set_dtr_rts(dev, FALSE, TRUE); - qos->baud_rate.bits &= IR_9600|IR_19200|IR_115200; + qos->baud_rate.bits &= IR_9600 | IR_19200 | IR_115200; qos->min_turn_time.bits = 0x01; /* Needs at least 10 ms */ irda_qos_bits_to_value(qos); -- 2.10.1 (Apple Git-78)
[PATCH 1/3] staging: irda: fix type from "unsigned" to "unsigned int"
Clean up checkpatch warning: WARNING: Prefer 'unsigned int' to bare use of 'unsigned' Signed-off-by: JI-HUN KIM --- drivers/staging/irda/drivers/esi-sir.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/staging/irda/drivers/esi-sir.c b/drivers/staging/irda/drivers/esi-sir.c index eb7aa64..a12cf55 100644 --- a/drivers/staging/irda/drivers/esi-sir.c +++ b/drivers/staging/irda/drivers/esi-sir.c @@ -39,7 +39,7 @@ static int esi_open(struct sir_dev *); static int esi_close(struct sir_dev *); -static int esi_change_speed(struct sir_dev *, unsigned); +static int esi_change_speed(struct sir_dev *, unsigned int); static int esi_reset(struct sir_dev *); static struct dongle_driver esi = { @@ -93,7 +93,7 @@ static int esi_close(struct sir_dev *dev) * Apparently (see old esi-driver) no delays are needed here... * */ -static int esi_change_speed(struct sir_dev *dev, unsigned speed) +static int esi_change_speed(struct sir_dev *dev, unsigned int speed) { int ret = 0; int dtr, rts; -- 2.10.1 (Apple Git-78)