[PATCH 0/6] fix a common error of while loop condition in error path

[PATCH 0/6] fix a common error of while loop condition in error path

[Chengguang Xu]

There is a common error of while loop condition which misses
the case '(--i) == 0' in error path. This patch series just
tries to fix it in several driver's code.

Note: I'm not specialist of specific drivers so just compile tested
for the fixes.

Chengguang Xu (6):
  netlink: fix missing destruction of rhash table in error case
  staging: vt6655: fix missing resource cleanup in error cases
  scsi: ipr: fix missing/incorrect resource cleanup in error case
  drm/exynos: fix missing resource cleanup in error case
  scsi: pmcraid: fix missing resource cleanup in error case
  media: platform: fix missing/incorrect resource cleanup in error case

 drivers/gpu/drm/exynos/exynos_drm_gsc.c             | 2 +-
 drivers/media/platform/samsung/s5p-mfc/s5p_mfc_pm.c | 3 +--
 drivers/scsi/ipr.c                                  | 4 ++--
 drivers/scsi/pmcraid.c                              | 2 +-
 drivers/staging/vt6655/device_main.c                | 8 ++++----
 net/netlink/af_netlink.c                            | 2 +-
 6 files changed, 10 insertions(+), 11 deletions(-)

-- 
2.27.0

[PATCH 1/6] netlink: fix missing destruction of rhash table in error case

[Chengguang Xu]

Fix missing destruction(when '(--i) == 0') for error case in
netlink_proto_init().

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 net/netlink/af_netlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 0cd91f813a3b..bd0b090a378b 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2887,7 +2887,7 @@ static int __init netlink_proto_init(void)
 	for (i = 0; i < MAX_LINKS; i++) {
 		if (rhashtable_init(&nl_table[i].hash,
 				    &netlink_rhashtable_params) < 0) {
-			while (--i > 0)
+			while (--i >= 0)
 				rhashtable_destroy(&nl_table[i].hash);
 			kfree(nl_table);
 			goto panic;
-- 
2.27.0

[PATCH 2/6] staging: vt6655: fix missing resource cleanup in error cases

[Chengguang Xu]

Fix missing resource cleanup(when '(--i) == 0') for error cases

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 drivers/staging/vt6655/device_main.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c
index 897d70cf32b8..cf8d92e785e6 100644
--- a/drivers/staging/vt6655/device_main.c
+++ b/drivers/staging/vt6655/device_main.c
@@ -565,7 +565,7 @@ static int device_init_rd0_ring(struct vnt_private *priv)
 	kfree(desc->rd_info);
 
 err_free_desc:
-	while (--i) {
+	while (--i >= 0) {
 		desc = &priv->aRD0Ring[i];
 		device_free_rx_buf(priv, desc);
 		kfree(desc->rd_info);
@@ -611,7 +611,7 @@ static int device_init_rd1_ring(struct vnt_private *priv)
 	kfree(desc->rd_info);
 
 err_free_desc:
-	while (--i) {
+	while (--i >= 0) {
 		desc = &priv->aRD1Ring[i];
 		device_free_rx_buf(priv, desc);
 		kfree(desc->rd_info);
@@ -676,7 +676,7 @@ static int device_init_td0_ring(struct vnt_private *priv)
 	return 0;
 
 err_free_desc:
-	while (--i) {
+	while (--i >= 0) {
 		desc = &priv->apTD0Rings[i];
 		kfree(desc->td_info);
 	}
@@ -716,7 +716,7 @@ static int device_init_td1_ring(struct vnt_private *priv)
 	return 0;
 
 err_free_desc:
-	while (--i) {
+	while (--i >= 0) {
 		desc = &priv->apTD1Rings[i];
 		kfree(desc->td_info);
 	}
-- 
2.27.0

[PATCH 3/6] scsi: ipr: fix missing/incorrect resource cleanup in error case

[Chengguang Xu]

Fix missing resource cleanup(when '(--i) == 0') for
error case in ipr_alloc_mem() and skip incorrect resource
cleanup(when '(--i) == 0') for error case in
ipr_request_other_msi_irqs() because variable i started from 1.

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 drivers/scsi/ipr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index 256ec6d08c16..9d01a3e3c26a 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -9795,7 +9795,7 @@ static int ipr_alloc_mem(struct ipr_ioa_cfg *ioa_cfg)
 					GFP_KERNEL);
 
 		if (!ioa_cfg->hrrq[i].host_rrq)  {
-			while (--i > 0)
+			while (--i >= 0)
 				dma_free_coherent(&pdev->dev,
 					sizeof(u32) * ioa_cfg->hrrq[i].size,
 					ioa_cfg->hrrq[i].host_rrq,
@@ -10068,7 +10068,7 @@ static int ipr_request_other_msi_irqs(struct ipr_ioa_cfg *ioa_cfg,
 			ioa_cfg->vectors_info[i].desc,
 			&ioa_cfg->hrrq[i]);
 		if (rc) {
-			while (--i >= 0)
+			while (--i > 0)
 				free_irq(pci_irq_vector(pdev, i),
 					&ioa_cfg->hrrq[i]);
 			return rc;
-- 
2.27.0

[PATCH 4/6] drm/exynos: fix missing resource cleanup in error case

[Chengguang Xu]

Fix missing resource cleanup(when '(--i) == 0') for error case
in gsc_runtime_resume().

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 drivers/gpu/drm/exynos/exynos_drm_gsc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_gsc.c b/drivers/gpu/drm/exynos/exynos_drm_gsc.c
index 964dceb28c1e..68ea92742b06 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gsc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_gsc.c
@@ -1342,7 +1342,7 @@ static int __maybe_unused gsc_runtime_resume(struct device *dev)
 	for (i = 0; i < ctx->num_clocks; i++) {
 		ret = clk_prepare_enable(ctx->clocks[i]);
 		if (ret) {
-			while (--i > 0)
+			while (--i >= 0)
 				clk_disable_unprepare(ctx->clocks[i]);
 			return ret;
 		}
-- 
2.27.0

[PATCH 5/6] scsi: pmcraid: fix missing resource cleanup in error case

[Chengguang Xu]

Fix missing resource cleanup(when '(--i) == 0') for error case in
pmcraid_register_interrupt_handler().

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 drivers/scsi/pmcraid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/pmcraid.c b/drivers/scsi/pmcraid.c
index 3d5cd337a2a6..0007a8a993a1 100644
--- a/drivers/scsi/pmcraid.c
+++ b/drivers/scsi/pmcraid.c
@@ -4031,7 +4031,7 @@ pmcraid_register_interrupt_handler(struct pmcraid_instance *pinstance)
 	return 0;
 
 out_unwind:
-	while (--i > 0)
+	while (--i >= 0)
 		free_irq(pci_irq_vector(pdev, i), &pinstance->hrrq_vector[i]);
 	pci_free_irq_vectors(pdev);
 	return rc;
-- 
2.27.0

[PATCH 6/6] media: platform: fix missing/incorrect resource cleanup in error case

[Chengguang Xu]

In error case of s5p_mfc_power_on() we should call
clk_disable_unprepare() for the
clocks(from pm->clocks[0] to pm->clocks[i-1]).

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 drivers/media/platform/samsung/s5p-mfc/s5p_mfc_pm.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_pm.c b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_pm.c
index 72a901e99450..187849841a28 100644
--- a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_pm.c
+++ b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_pm.c
@@ -88,7 +88,6 @@ int s5p_mfc_power_on(void)
 		if (ret < 0) {
 			mfc_err("clock prepare failed for clock: %s\n",
 				pm->clk_names[i]);
-			i++;
 			goto err;
 		}
 	}
@@ -98,7 +97,7 @@ int s5p_mfc_power_on(void)
 
 	return 0;
 err:
-	while (--i > 0)
+	while (--i >= 0)
 		clk_disable_unprepare(pm->clocks[i]);
 	pm_runtime_put(pm->device);
 	return ret;
-- 
2.27.0

Re: [PATCH 0/6] fix a common error of while loop condition in error path

[Dan Carpenter]

On Sun, May 29, 2022 at 11:34:50PM +0800, Chengguang Xu wrote:
> There is a common error of while loop condition which misses
> the case '(--i) == 0' in error path. This patch series just
> tries to fix it in several driver's code.
> 
> Note: I'm not specialist of specific drivers so just compile tested
> for the fixes.

These are all correct.  Thank you.

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter

Re: [PATCH 1/6] netlink: fix missing destruction of rhash table in error case

[Paolo Abeni]

Hello,

On Sun, 2022-05-29 at 23:34 +0800, Chengguang Xu wrote:
> Fix missing destruction(when '(--i) == 0') for error case in
> netlink_proto_init().
> 
> Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
> ---
>  net/netlink/af_netlink.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index 0cd91f813a3b..bd0b090a378b 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -2887,7 +2887,7 @@ static int __init netlink_proto_init(void)
>  	for (i = 0; i < MAX_LINKS; i++) {
>  		if (rhashtable_init(&nl_table[i].hash,
>  				    &netlink_rhashtable_params) < 0) {
> -			while (--i > 0)
> +			while (--i >= 0)
>  				rhashtable_destroy(&nl_table[i].hash);
>  			kfree(nl_table);
>  			goto panic;

The patch looks correct to me, but it looks like each patch in this
series is targeting a different tree. I suggest to re-send, splitting
the series into individual patches, and sending each of them to the
appropriate tree. You can retain Dan's Review tag.

Thanks,

Paolo

Re: [PATCH 1/6] netlink: fix missing destruction of rhash table in error case

[Dan Carpenter]

On Tue, May 31, 2022 at 10:43:09AM +0200, Paolo Abeni wrote:
> Hello,
> 
> On Sun, 2022-05-29 at 23:34 +0800, Chengguang Xu wrote:
> > Fix missing destruction(when '(--i) == 0') for error case in
> > netlink_proto_init().
> > 
> > Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
> > ---
> >  net/netlink/af_netlink.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> > index 0cd91f813a3b..bd0b090a378b 100644
> > --- a/net/netlink/af_netlink.c
> > +++ b/net/netlink/af_netlink.c
> > @@ -2887,7 +2887,7 @@ static int __init netlink_proto_init(void)
> >  	for (i = 0; i < MAX_LINKS; i++) {
> >  		if (rhashtable_init(&nl_table[i].hash,
> >  				    &netlink_rhashtable_params) < 0) {
> > -			while (--i > 0)
> > +			while (--i >= 0)
> >  				rhashtable_destroy(&nl_table[i].hash);
> >  			kfree(nl_table);
> >  			goto panic;
> 
> The patch looks correct to me, but it looks like each patch in this
> series is targeting a different tree. I suggest to re-send, splitting
> the series into individual patches, and sending each of them to the
> appropriate tree. You can retain Dan's Review tag.

Since it looks like you're going to be resending these then could you
add Fixes tags?  Please keep my Review tag.

regards,
dan carpenter

Re: [PATCH 1/6] netlink: fix missing destruction of rhash table in error case

[Chengguang Xu]

在 2022/5/31 19:25, Dan Carpenter 写道:
> On Tue, May 31, 2022 at 10:43:09AM +0200, Paolo Abeni wrote:
>> Hello,
>>
>> On Sun, 2022-05-29 at 23:34 +0800, Chengguang Xu wrote:
>>> Fix missing destruction(when '(--i) == 0') for error case in
>>> netlink_proto_init().
>>>
>>> Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
>>> ---
>>>   net/netlink/af_netlink.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
>>> index 0cd91f813a3b..bd0b090a378b 100644
>>> --- a/net/netlink/af_netlink.c
>>> +++ b/net/netlink/af_netlink.c
>>> @@ -2887,7 +2887,7 @@ static int __init netlink_proto_init(void)
>>>   	for (i = 0; i < MAX_LINKS; i++) {
>>>   		if (rhashtable_init(&nl_table[i].hash,
>>>   				    &netlink_rhashtable_params) < 0) {
>>> -			while (--i > 0)
>>> +			while (--i >= 0)
>>>   				rhashtable_destroy(&nl_table[i].hash);
>>>   			kfree(nl_table);
>>>   			goto panic;
>> The patch looks correct to me, but it looks like each patch in this
>> series is targeting a different tree. I suggest to re-send, splitting
>> the series into individual patches, and sending each of them to the
>> appropriate tree. You can retain Dan's Review tag.
> Since it looks like you're going to be resending these then could you
> add Fixes tags?  Please keep my Review tag.
>

OK, no problem.

Thanks,
Chengguang

Re: [PATCH 3/6] scsi: ipr: fix missing/incorrect resource cleanup in error case

[Brian King]

Acked-by: Brian King <brking@linux.vnet.ibm.com>


-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

Re: [PATCH 0/6] fix a common error of while loop condition in error path

[Martin K. Petersen]

On Sun, 29 May 2022 23:34:50 +0800, Chengguang Xu wrote:

> There is a common error of while loop condition which misses
> the case '(--i) == 0' in error path. This patch series just
> tries to fix it in several driver's code.
> 
> Note: I'm not specialist of specific drivers so just compile tested
> for the fixes.
> 
> [...]

Applied to 5.19/scsi-fixes, thanks!

[3/6] scsi: ipr: fix missing/incorrect resource cleanup in error case
      https://git.kernel.org/mkp/scsi/c/d64c49191132
[5/6] scsi: pmcraid: fix missing resource cleanup in error case
      https://git.kernel.org/mkp/scsi/c/ec1e8adcbdf6

-- 
Martin K. Petersen	Oracle Linux Engineering