* [PATCH] staging: media: davinci_vpfe: add error handling on kmalloc failure
[not found] <CGME20180316045841epcas2p34dc11231c65e2032e88ac7138db2daee@epcas2p3.samsung.com>
2018-03-16 4:58 ` Ji-Hun Kim
@ 2018-03-16 4:58 ` Ji-Hun Kim
0 siblings, 0 replies; 10+ messages in thread
From: Ji-Hun Kim @ 2018-03-16 4:58 UTC (permalink / raw)
To: mchehab
Cc: devel, gregkh, kernel-janitors, linux-kernel, ji_hun.kim,
arvind.yadav.cs, linux-media
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 <ji_hun.kim@samsung.com>
---
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] staging: media: davinci_vpfe: add error handling on kmalloc failure
@ 2018-03-16 4:58 ` Ji-Hun Kim
0 siblings, 0 replies; 10+ messages in thread
From: Ji-Hun Kim @ 2018-03-16 4:58 UTC (permalink / raw)
To: mchehab
Cc: gregkh, arvind.yadav.cs, ji_hun.kim, linux-media, devel,
linux-kernel, kernel-janitors
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 <ji_hun.kim@samsung.com>
---
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] staging: media: davinci_vpfe: add error handling on kmalloc failure
@ 2018-03-16 4:58 ` Ji-Hun Kim
0 siblings, 0 replies; 10+ messages in thread
From: Ji-Hun Kim @ 2018-03-16 4:58 UTC (permalink / raw)
To: mchehab
Cc: gregkh, arvind.yadav.cs, ji_hun.kim, linux-media, devel,
linux-kernel, kernel-janitors
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 <ji_hun.kim@samsung.com>
---
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] staging: media: davinci_vpfe: add error handling on kmalloc failure
2018-03-16 4:58 ` Ji-Hun Kim
@ 2018-03-16 8:32 ` Dan Carpenter
-1 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2018-03-16 8:32 UTC (permalink / raw)
To: Ji-Hun Kim
Cc: mchehab, devel, gregkh, kernel-janitors, linux-kernel,
arvind.yadav.cs, linux-media
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 <ji_hun.kim@samsung.com>
> ---
> 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.
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;
free_params:
kfree(params);
return rval;
1304 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] staging: media: davinci_vpfe: add error handling on kmalloc failure
@ 2018-03-16 8:32 ` Dan Carpenter
0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2018-03-16 8:32 UTC (permalink / raw)
To: Ji-Hun Kim
Cc: mchehab, devel, gregkh, kernel-janitors, linux-kernel,
arvind.yadav.cs, linux-media
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 <ji_hun.kim@samsung.com>
> ---
> 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.
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;
free_params:
kfree(params);
return rval;
1304 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Re: [PATCH] staging: media: davinci_vpfe: add error handling on kmalloc failure
2018-03-16 8:32 ` Dan Carpenter
@ 2018-03-19 4:24 ` Ji-Hun Kim
-1 siblings, 0 replies; 10+ messages in thread
From: Ji-Hun Kim @ 2018-03-19 4:24 UTC (permalink / raw)
To: Dan Carpenter
Cc: mchehab, gregkh, arvind.yadav.cs, ji_hun.kim, linux-media, devel,
linux-kernel, kernel-janitors
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 <ji_hun.kim@samsung.com>
> > ---
> > 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Re: [PATCH] staging: media: davinci_vpfe: add error handling on kmalloc failure
@ 2018-03-19 4:24 ` Ji-Hun Kim
0 siblings, 0 replies; 10+ messages in thread
From: Ji-Hun Kim @ 2018-03-19 4:24 UTC (permalink / raw)
To: Dan Carpenter
Cc: mchehab, gregkh, arvind.yadav.cs, ji_hun.kim, linux-media, devel,
linux-kernel, kernel-janitors
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 <ji_hun.kim@samsung.com>
> > ---
> > 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Re: [PATCH] staging: media: davinci_vpfe: add error handling on kmalloc failure
2018-03-19 4:24 ` Ji-Hun Kim
(?)
@ 2018-03-19 7:14 ` Dan Carpenter
-1 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2018-03-19 7:14 UTC (permalink / raw)
To: Ji-Hun Kim
Cc: devel, gregkh, kernel-janitors, linux-kernel, arvind.yadav.cs,
mchehab, linux-media
On Mon, Mar 19, 2018 at 01:24:57PM +0900, Ji-Hun Kim wrote:
> > 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.
>
In the proposed code, the errors all do a return or a goto so "rval"
would be zero here. Then the error path would look like:
err_free_params:
kfree(params);
return rval;
}
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Re: [PATCH] staging: media: davinci_vpfe: add error handling on kmalloc failure
@ 2018-03-19 7:14 ` Dan Carpenter
0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2018-03-19 7:14 UTC (permalink / raw)
To: Ji-Hun Kim
Cc: mchehab, gregkh, arvind.yadav.cs, linux-media, devel,
linux-kernel, kernel-janitors
On Mon, Mar 19, 2018 at 01:24:57PM +0900, Ji-Hun Kim wrote:
> > 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.
>
In the proposed code, the errors all do a return or a goto so "rval"
would be zero here. Then the error path would look like:
err_free_params:
kfree(params);
return rval;
}
regards,
dan carpenter
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Re: [PATCH] staging: media: davinci_vpfe: add error handling on kmalloc failure
@ 2018-03-19 7:14 ` Dan Carpenter
0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2018-03-19 7:14 UTC (permalink / raw)
To: Ji-Hun Kim
Cc: mchehab, gregkh, arvind.yadav.cs, linux-media, devel,
linux-kernel, kernel-janitors
On Mon, Mar 19, 2018 at 01:24:57PM +0900, Ji-Hun Kim wrote:
> > 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.
>
In the proposed code, the errors all do a return or a goto so "rval"
would be zero here. Then the error path would look like:
err_free_params:
kfree(params);
return rval;
}
regards,
dan carpenter
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-03-19 7:15 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20180316045841epcas2p34dc11231c65e2032e88ac7138db2daee@epcas2p3.samsung.com>
2018-03-16 4:58 ` [PATCH] staging: media: davinci_vpfe: add error handling on kmalloc failure Ji-Hun Kim
2018-03-16 4:58 ` Ji-Hun Kim
2018-03-16 4:58 ` Ji-Hun Kim
2018-03-16 8:32 ` Dan Carpenter
2018-03-16 8:32 ` Dan Carpenter
2018-03-19 4:24 ` Ji-Hun Kim
2018-03-19 4:24 ` Ji-Hun Kim
2018-03-19 7:14 ` Dan Carpenter
2018-03-19 7:14 ` Dan Carpenter
2018-03-19 7:14 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.