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

[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 <ji_hun.kim@xxxxxxxxxxx>
> > ---
> >  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