Re: Re: [PATCH] staging: media: davinci_vpfe: add error handling on kmalloc failure

2018-03-19 Thread Dan Carpenter 
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

Re: Re: [PATCH] staging: media: davinci_vpfe: add error handling on kmalloc failure

2018-03-19 Thread Dan Carpenter 
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

Re: Re: [PATCH] staging: media: davinci_vpfe: add error handling on kmalloc failure

2018-03-18 Thread Ji-Hun Kim 
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  _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

Re: Re: [PATCH] staging: media: davinci_vpfe: add error handling on kmalloc failure

2018-03-18 Thread Ji-Hun Kim 
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  _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

Re: [PATCH] staging: media: davinci_vpfe: add error handling on kmalloc failure

2018-03-16 Thread Dan Carpenter 
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.

  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  _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

Re: [PATCH] staging: media: davinci_vpfe: add error handling on kmalloc failure

2018-03-16 Thread Dan Carpenter 
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.

  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  _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

[PATCH] staging: media: davinci_vpfe: add error handling on kmalloc failure

2018-03-15 Thread Ji-Hun Kim 
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: media: davinci_vpfe: add error handling on kmalloc failure

2018-03-15 Thread Ji-Hun Kim 
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