[patch] [media] uvcvideo: freeing an error pointer

[patch] [media] uvcvideo: freeing an error pointer

[Dan Carpenter]

A recent cleanup introduced a potential dereference of -EFAULT when we
call kfree(map->menu_info).

Fixes: 4cc5bed1caeb ("[media] uvcvideo: Use memdup_user() rather than duplicating its implementation")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index a7e12fd..3e7e283 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -66,14 +66,14 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain *chain,
 		if (xmap->menu_count == 0 ||
 		    xmap->menu_count > UVC_MAX_CONTROL_MENU_ENTRIES) {
 			ret = -EINVAL;
-			goto done;
+			goto free_map;
 		}
 
 		size = xmap->menu_count * sizeof(*map->menu_info);
 		map->menu_info = memdup_user(xmap->menu_info, size);
 		if (IS_ERR(map->menu_info)) {
 			ret = PTR_ERR(map->menu_info);
-			goto done;
+			goto free_map;
 		}
 
 		map->menu_count = xmap->menu_count;
@@ -83,13 +83,13 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain *chain,
 		uvc_trace(UVC_TRACE_CONTROL, "Unsupported V4L2 control type "
 			  "%u.\n", xmap->v4l2_type);
 		ret = -ENOTTY;
-		goto done;
+		goto free_map;
 	}
 
 	ret = uvc_ctrl_add_mapping(chain, map);
 
-done:
 	kfree(map->menu_info);
+free_map:
 	kfree(map);
 
 	return ret;

[patch] [media] uvcvideo: freeing an error pointer

[Dan Carpenter]

A recent cleanup introduced a potential dereference of -EFAULT when we
call kfree(map->menu_info).

Fixes: 4cc5bed1caeb ("[media] uvcvideo: Use memdup_user() rather than duplicating its implementation")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index a7e12fd..3e7e283 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -66,14 +66,14 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain *chain,
 		if (xmap->menu_count = 0 ||
 		    xmap->menu_count > UVC_MAX_CONTROL_MENU_ENTRIES) {
 			ret = -EINVAL;
-			goto done;
+			goto free_map;
 		}
 
 		size = xmap->menu_count * sizeof(*map->menu_info);
 		map->menu_info = memdup_user(xmap->menu_info, size);
 		if (IS_ERR(map->menu_info)) {
 			ret = PTR_ERR(map->menu_info);
-			goto done;
+			goto free_map;
 		}
 
 		map->menu_count = xmap->menu_count;
@@ -83,13 +83,13 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain *chain,
 		uvc_trace(UVC_TRACE_CONTROL, "Unsupported V4L2 control type "
 			  "%u.\n", xmap->v4l2_type);
 		ret = -ENOTTY;
-		goto done;
+		goto free_map;
 	}
 
 	ret = uvc_ctrl_add_mapping(chain, map);
 
-done:
 	kfree(map->menu_info);
+free_map:
 	kfree(map);
 
 	return ret;

Re: [patch] [media] uvcvideo: freeing an error pointer

[SF Markus Elfring]

> A recent cleanup introduced a potential dereference of -EFAULT when we
> call kfree(map->menu_info).
> 
> Fixes: 4cc5bed1caeb ("[media] uvcvideo: Use memdup_user() rather than duplicating its implementation")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Thanks for your information.


> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index a7e12fd..3e7e283 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -66,14 +66,14 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain *chain,
>  		if (xmap->menu_count == 0 ||
>  		    xmap->menu_count > UVC_MAX_CONTROL_MENU_ENTRIES) {
>  			ret = -EINVAL;
> -			goto done;
> +			goto free_map;
>  		}
>  
>  		size = xmap->menu_count * sizeof(*map->menu_info);
>  		map->menu_info = memdup_user(xmap->menu_info, size);
>  		if (IS_ERR(map->menu_info)) {
>  			ret = PTR_ERR(map->menu_info);
> -			goto done;
> +			goto free_map;
>  		}
>  
>  		map->menu_count = xmap->menu_count;
> @@ -83,13 +83,13 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain *chain,
>  		uvc_trace(UVC_TRACE_CONTROL, "Unsupported V4L2 control type "
>  			  "%u.\n", xmap->v4l2_type);
>  		ret = -ENOTTY;
> -		goto done;
> +		goto free_map;
>  	}
>  
>  	ret = uvc_ctrl_add_mapping(chain, map);
>  
> -done:
>  	kfree(map->menu_info);
> +free_map:
>  	kfree(map);
>  
>  	return ret;
> 

Did your update suggestion become also relevant just because the corresponding
update step “[PATCH 2/2] uvc_v4l2: One function call less in uvc_ioctl_ctrl_map()
after error detection” which I offered as another change possibility on 2016-08-19
was rejected on 2016-11-22?

https://patchwork.linuxtv.org/patch/36528/
https://patchwork.kernel.org/patch/9289897/
https://lkml.kernel.org/r/<8f89ec37-1556-4c09-f0b7-df87b4169320@users.sourceforge.net>

Regards,
Markus

Re: [patch] [media] uvcvideo: freeing an error pointer

[SF Markus Elfring]

> A recent cleanup introduced a potential dereference of -EFAULT when we
> call kfree(map->menu_info).
> 
> Fixes: 4cc5bed1caeb ("[media] uvcvideo: Use memdup_user() rather than duplicating its implementation")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Thanks for your information.


> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index a7e12fd..3e7e283 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -66,14 +66,14 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain *chain,
>  		if (xmap->menu_count = 0 ||
>  		    xmap->menu_count > UVC_MAX_CONTROL_MENU_ENTRIES) {
>  			ret = -EINVAL;
> -			goto done;
> +			goto free_map;
>  		}
>  
>  		size = xmap->menu_count * sizeof(*map->menu_info);
>  		map->menu_info = memdup_user(xmap->menu_info, size);
>  		if (IS_ERR(map->menu_info)) {
>  			ret = PTR_ERR(map->menu_info);
> -			goto done;
> +			goto free_map;
>  		}
>  
>  		map->menu_count = xmap->menu_count;
> @@ -83,13 +83,13 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain *chain,
>  		uvc_trace(UVC_TRACE_CONTROL, "Unsupported V4L2 control type "
>  			  "%u.\n", xmap->v4l2_type);
>  		ret = -ENOTTY;
> -		goto done;
> +		goto free_map;
>  	}
>  
>  	ret = uvc_ctrl_add_mapping(chain, map);
>  
> -done:
>  	kfree(map->menu_info);
> +free_map:
>  	kfree(map);
>  
>  	return ret;
> 

Did your update suggestion become also relevant just because the corresponding
update step “[PATCH 2/2] uvc_v4l2: One function call less in uvc_ioctl_ctrl_map()
after error detection” which I offered as another change possibility on 2016-08-19
was rejected on 2016-11-22?

https://patchwork.linuxtv.org/patch/36528/
https://patchwork.kernel.org/patch/9289897/
https://lkml.kernel.org/r/<8f89ec37-1556-4c09-f0b7-df87b4169320@users.sourceforge.net>

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] [media] uvcvideo: freeing an error pointer

[Laurent Pinchart]

Hi Dan,

Thank you for the patch.

On Friday 25 Nov 2016 13:28:35 Dan Carpenter wrote:
> A recent cleanup introduced a potential dereference of -EFAULT when we
> call kfree(map->menu_info).

I should have caught that, my apologies :-(

Thinking a bit more about this class of problems, would the following patch 
make sense ?

commit 034b71306510643f9f059249a0c14418099eb436
Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Date:   Fri Nov 25 15:54:22 2016 +0200

    mm/slab: WARN_ON error pointers passed to kfree()
    
    Passing an error pointer to kfree() is invalid and can lead to crashes
    or memory corruption. Reject those pointers and WARN in order to catch
    the problems and fix them in the callers.
    
    Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

diff --git a/mm/slab.c b/mm/slab.c
index 0b0550ca85b4..a7eb830c6684 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -3819,6 +3819,8 @@ void kfree(const void *objp)
 
 	if (unlikely(ZERO_OR_NULL_PTR(objp)))
 		return;
+	if (WARN_ON(IS_ERR(objp)))
+		return;
 	local_irq_save(flags);
 	kfree_debugcheck(objp);
 	c = virt_to_cache(objp);

> Fixes: 4cc5bed1caeb ("[media] uvcvideo: Use memdup_user() rather than
> duplicating its implementation")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Mauro, the bug is present in your branch only at the moment and queued for 
v4.10. Could you please pick this patch up as well ?

> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> b/drivers/media/usb/uvc/uvc_v4l2.c index a7e12fd..3e7e283 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -66,14 +66,14 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain
> *chain, if (xmap->menu_count == 0 ||
>  		    xmap->menu_count > UVC_MAX_CONTROL_MENU_ENTRIES) {
>  			ret = -EINVAL;
> -			goto done;
> +			goto free_map;
>  		}
> 
>  		size = xmap->menu_count * sizeof(*map->menu_info);
>  		map->menu_info = memdup_user(xmap->menu_info, size);
>  		if (IS_ERR(map->menu_info)) {
>  			ret = PTR_ERR(map->menu_info);
> -			goto done;
> +			goto free_map;
>  		}
> 
>  		map->menu_count = xmap->menu_count;
> @@ -83,13 +83,13 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain
> *chain, uvc_trace(UVC_TRACE_CONTROL, "Unsupported V4L2 control type "
>  			  "%u.\n", xmap->v4l2_type);
>  		ret = -ENOTTY;
> -		goto done;
> +		goto free_map;
>  	}
> 
>  	ret = uvc_ctrl_add_mapping(chain, map);
> 
> -done:
>  	kfree(map->menu_info);
> +free_map:
>  	kfree(map);
> 
>  	return ret;

-- 
Regards,

Laurent Pinchart

Re: [patch] [media] uvcvideo: freeing an error pointer

[Laurent Pinchart]

Hi Dan,

Thank you for the patch.

On Friday 25 Nov 2016 13:28:35 Dan Carpenter wrote:
> A recent cleanup introduced a potential dereference of -EFAULT when we
> call kfree(map->menu_info).

I should have caught that, my apologies :-(

Thinking a bit more about this class of problems, would the following patch 
make sense ?

commit 034b71306510643f9f059249a0c14418099eb436
Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Date:   Fri Nov 25 15:54:22 2016 +0200

    mm/slab: WARN_ON error pointers passed to kfree()
    
    Passing an error pointer to kfree() is invalid and can lead to crashes
    or memory corruption. Reject those pointers and WARN in order to catch
    the problems and fix them in the callers.
    
    Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

diff --git a/mm/slab.c b/mm/slab.c
index 0b0550ca85b4..a7eb830c6684 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -3819,6 +3819,8 @@ void kfree(const void *objp)
 
 	if (unlikely(ZERO_OR_NULL_PTR(objp)))
 		return;
+	if (WARN_ON(IS_ERR(objp)))
+		return;
 	local_irq_save(flags);
 	kfree_debugcheck(objp);
 	c = virt_to_cache(objp);

> Fixes: 4cc5bed1caeb ("[media] uvcvideo: Use memdup_user() rather than
> duplicating its implementation")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Mauro, the bug is present in your branch only at the moment and queued for 
v4.10. Could you please pick this patch up as well ?

> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> b/drivers/media/usb/uvc/uvc_v4l2.c index a7e12fd..3e7e283 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -66,14 +66,14 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain
> *chain, if (xmap->menu_count = 0 ||
>  		    xmap->menu_count > UVC_MAX_CONTROL_MENU_ENTRIES) {
>  			ret = -EINVAL;
> -			goto done;
> +			goto free_map;
>  		}
> 
>  		size = xmap->menu_count * sizeof(*map->menu_info);
>  		map->menu_info = memdup_user(xmap->menu_info, size);
>  		if (IS_ERR(map->menu_info)) {
>  			ret = PTR_ERR(map->menu_info);
> -			goto done;
> +			goto free_map;
>  		}
> 
>  		map->menu_count = xmap->menu_count;
> @@ -83,13 +83,13 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain
> *chain, uvc_trace(UVC_TRACE_CONTROL, "Unsupported V4L2 control type "
>  			  "%u.\n", xmap->v4l2_type);
>  		ret = -ENOTTY;
> -		goto done;
> +		goto free_map;
>  	}
> 
>  	ret = uvc_ctrl_add_mapping(chain, map);
> 
> -done:
>  	kfree(map->menu_info);
> +free_map:
>  	kfree(map);
> 
>  	return ret;

-- 
Regards,

Laurent Pinchart

Re: [patch] [media] uvcvideo: freeing an error pointer

[walter harms]

Am 25.11.2016 14:57, schrieb Laurent Pinchart:
> Hi Dan,
> 
> Thank you for the patch.
> 
> On Friday 25 Nov 2016 13:28:35 Dan Carpenter wrote:
>> A recent cleanup introduced a potential dereference of -EFAULT when we
>> call kfree(map->menu_info).
> 
> I should have caught that, my apologies :-(
> 
> Thinking a bit more about this class of problems, would the following patch 
> make sense ?
> 
> commit 034b71306510643f9f059249a0c14418099eb436
> Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Date:   Fri Nov 25 15:54:22 2016 +0200
> 
>     mm/slab: WARN_ON error pointers passed to kfree()
>     
>     Passing an error pointer to kfree() is invalid and can lead to crashes
>     or memory corruption. Reject those pointers and WARN in order to catch
>     the problems and fix them in the callers.
>     
>     Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> diff --git a/mm/slab.c b/mm/slab.c
> index 0b0550ca85b4..a7eb830c6684 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -3819,6 +3819,8 @@ void kfree(const void *objp)
>  
>  	if (unlikely(ZERO_OR_NULL_PTR(objp)))
>  		return;
> +	if (WARN_ON(IS_ERR(objp)))
> +		return;
>  	local_irq_save(flags);
>  	kfree_debugcheck(objp);
>  	c = virt_to_cache(objp);


I this is better in kfree_debugcheck().
1. it has the right name
2. is contains already a check

static void kfree_debugcheck(const void *objp)
 {
         if (!virt_addr_valid(objp)) {
                 pr_err("kfree_debugcheck: out of range ptr %lxh\n",
                        (unsigned long)objp);
                 BUG();
         }
  }

btw: should this read %p ?

re,
 wh



>> Fixes: 4cc5bed1caeb ("[media] uvcvideo: Use memdup_user() rather than
>> duplicating its implementation")
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Mauro, the bug is present in your branch only at the moment and queued for 
> v4.10. Could you please pick this patch up as well ?
> 
>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
>> b/drivers/media/usb/uvc/uvc_v4l2.c index a7e12fd..3e7e283 100644
>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
>> @@ -66,14 +66,14 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain
>> *chain, if (xmap->menu_count == 0 ||
>>  		    xmap->menu_count > UVC_MAX_CONTROL_MENU_ENTRIES) {
>>  			ret = -EINVAL;
>> -			goto done;
>> +			goto free_map;
>>  		}
>>
>>  		size = xmap->menu_count * sizeof(*map->menu_info);
>>  		map->menu_info = memdup_user(xmap->menu_info, size);
>>  		if (IS_ERR(map->menu_info)) {
>>  			ret = PTR_ERR(map->menu_info);
>> -			goto done;
>> +			goto free_map;
>>  		}
>>
>>  		map->menu_count = xmap->menu_count;
>> @@ -83,13 +83,13 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain
>> *chain, uvc_trace(UVC_TRACE_CONTROL, "Unsupported V4L2 control type "
>>  			  "%u.\n", xmap->v4l2_type);
>>  		ret = -ENOTTY;
>> -		goto done;
>> +		goto free_map;
>>  	}
>>
>>  	ret = uvc_ctrl_add_mapping(chain, map);
>>
>> -done:
>>  	kfree(map->menu_info);
>> +free_map:
>>  	kfree(map);
>>
>>  	return ret;
> 

Re: [patch] [media] uvcvideo: freeing an error pointer

[walter harms]

Am 25.11.2016 14:57, schrieb Laurent Pinchart:
> Hi Dan,
> 
> Thank you for the patch.
> 
> On Friday 25 Nov 2016 13:28:35 Dan Carpenter wrote:
>> A recent cleanup introduced a potential dereference of -EFAULT when we
>> call kfree(map->menu_info).
> 
> I should have caught that, my apologies :-(
> 
> Thinking a bit more about this class of problems, would the following patch 
> make sense ?
> 
> commit 034b71306510643f9f059249a0c14418099eb436
> Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Date:   Fri Nov 25 15:54:22 2016 +0200
> 
>     mm/slab: WARN_ON error pointers passed to kfree()
>     
>     Passing an error pointer to kfree() is invalid and can lead to crashes
>     or memory corruption. Reject those pointers and WARN in order to catch
>     the problems and fix them in the callers.
>     
>     Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> diff --git a/mm/slab.c b/mm/slab.c
> index 0b0550ca85b4..a7eb830c6684 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -3819,6 +3819,8 @@ void kfree(const void *objp)
>  
>  	if (unlikely(ZERO_OR_NULL_PTR(objp)))
>  		return;
> +	if (WARN_ON(IS_ERR(objp)))
> +		return;
>  	local_irq_save(flags);
>  	kfree_debugcheck(objp);
>  	c = virt_to_cache(objp);


I this is better in kfree_debugcheck().
1. it has the right name
2. is contains already a check

static void kfree_debugcheck(const void *objp)
 {
         if (!virt_addr_valid(objp)) {
                 pr_err("kfree_debugcheck: out of range ptr %lxh\n",
                        (unsigned long)objp);
                 BUG();
         }
  }

btw: should this read %p ?

re,
 wh



>> Fixes: 4cc5bed1caeb ("[media] uvcvideo: Use memdup_user() rather than
>> duplicating its implementation")
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Mauro, the bug is present in your branch only at the moment and queued for 
> v4.10. Could you please pick this patch up as well ?
> 
>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
>> b/drivers/media/usb/uvc/uvc_v4l2.c index a7e12fd..3e7e283 100644
>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
>> @@ -66,14 +66,14 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain
>> *chain, if (xmap->menu_count = 0 ||
>>  		    xmap->menu_count > UVC_MAX_CONTROL_MENU_ENTRIES) {
>>  			ret = -EINVAL;
>> -			goto done;
>> +			goto free_map;
>>  		}
>>
>>  		size = xmap->menu_count * sizeof(*map->menu_info);
>>  		map->menu_info = memdup_user(xmap->menu_info, size);
>>  		if (IS_ERR(map->menu_info)) {
>>  			ret = PTR_ERR(map->menu_info);
>> -			goto done;
>> +			goto free_map;
>>  		}
>>
>>  		map->menu_count = xmap->menu_count;
>> @@ -83,13 +83,13 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain
>> *chain, uvc_trace(UVC_TRACE_CONTROL, "Unsupported V4L2 control type "
>>  			  "%u.\n", xmap->v4l2_type);
>>  		ret = -ENOTTY;
>> -		goto done;
>> +		goto free_map;
>>  	}
>>
>>  	ret = uvc_ctrl_add_mapping(chain, map);
>>
>> -done:
>>  	kfree(map->menu_info);
>> +free_map:
>>  	kfree(map);
>>
>>  	return ret;
> 

Re: [patch] [media] uvcvideo: freeing an error pointer

[Laurent Pinchart]

Hi Walter,

On Friday 25 Nov 2016 15:47:49 walter harms wrote:
> Am 25.11.2016 14:57, schrieb Laurent Pinchart:
> > On Friday 25 Nov 2016 13:28:35 Dan Carpenter wrote:
> >> A recent cleanup introduced a potential dereference of -EFAULT when we
> >> call kfree(map->menu_info).
> > 
> > I should have caught that, my apologies :-(
> > 
> > Thinking a bit more about this class of problems, would the following
> > patch make sense ?
> > 
> > commit 034b71306510643f9f059249a0c14418099eb436
> > Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Date:   Fri Nov 25 15:54:22 2016 +0200
> > 
> >     mm/slab: WARN_ON error pointers passed to kfree()
> >     
> >     Passing an error pointer to kfree() is invalid and can lead to crashes
> >     or memory corruption. Reject those pointers and WARN in order to catch
> >     the problems and fix them in the callers.
> >     
> >     Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > diff --git a/mm/slab.c b/mm/slab.c
> > index 0b0550ca85b4..a7eb830c6684 100644
> > --- a/mm/slab.c
> > +++ b/mm/slab.c
> > @@ -3819,6 +3819,8 @@ void kfree(const void *objp)
> >  	if (unlikely(ZERO_OR_NULL_PTR(objp)))
> >  		return;
> > 
> > +	if (WARN_ON(IS_ERR(objp)))
> > +		return;
> >  	local_irq_save(flags);
> >  	kfree_debugcheck(objp);
> >  	c = virt_to_cache(objp);
> 
> I this is better in kfree_debugcheck().
> 1. it has the right name
> 2. is contains already a check

Sakari Ailus (CC'ed) has expressed the opinion that we might want to go one 
step further and treat error pointers the same way we treat NULL or ZERO 
pointers today, by just returning without logging anything. The reasoning is 
that accepting a NULL pointer in kfree() was decided before we made extensive 
use of allocation APIs returning error pointers, so it could be time to update 
kfree() based on the current allocation usage patterns.

> static void kfree_debugcheck(const void *objp)
>  {
>          if (!virt_addr_valid(objp)) {
>                  pr_err("kfree_debugcheck: out of range ptr %lxh\n",
>                         (unsigned long)objp);
>                  BUG();
>          }
>   }
> 
> btw: should this read %p ?

-- 
Regards,

Laurent Pinchart

Re: [patch] [media] uvcvideo: freeing an error pointer

[Laurent Pinchart]

Hi Walter,

On Friday 25 Nov 2016 15:47:49 walter harms wrote:
> Am 25.11.2016 14:57, schrieb Laurent Pinchart:
> > On Friday 25 Nov 2016 13:28:35 Dan Carpenter wrote:
> >> A recent cleanup introduced a potential dereference of -EFAULT when we
> >> call kfree(map->menu_info).
> > 
> > I should have caught that, my apologies :-(
> > 
> > Thinking a bit more about this class of problems, would the following
> > patch make sense ?
> > 
> > commit 034b71306510643f9f059249a0c14418099eb436
> > Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Date:   Fri Nov 25 15:54:22 2016 +0200
> > 
> >     mm/slab: WARN_ON error pointers passed to kfree()
> >     
> >     Passing an error pointer to kfree() is invalid and can lead to crashes
> >     or memory corruption. Reject those pointers and WARN in order to catch
> >     the problems and fix them in the callers.
> >     
> >     Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > diff --git a/mm/slab.c b/mm/slab.c
> > index 0b0550ca85b4..a7eb830c6684 100644
> > --- a/mm/slab.c
> > +++ b/mm/slab.c
> > @@ -3819,6 +3819,8 @@ void kfree(const void *objp)
> >  	if (unlikely(ZERO_OR_NULL_PTR(objp)))
> >  		return;
> > 
> > +	if (WARN_ON(IS_ERR(objp)))
> > +		return;
> >  	local_irq_save(flags);
> >  	kfree_debugcheck(objp);
> >  	c = virt_to_cache(objp);
> 
> I this is better in kfree_debugcheck().
> 1. it has the right name
> 2. is contains already a check

Sakari Ailus (CC'ed) has expressed the opinion that we might want to go one 
step further and treat error pointers the same way we treat NULL or ZERO 
pointers today, by just returning without logging anything. The reasoning is 
that accepting a NULL pointer in kfree() was decided before we made extensive 
use of allocation APIs returning error pointers, so it could be time to update 
kfree() based on the current allocation usage patterns.

> static void kfree_debugcheck(const void *objp)
>  {
>          if (!virt_addr_valid(objp)) {
>                  pr_err("kfree_debugcheck: out of range ptr %lxh\n",
>                         (unsigned long)objp);
>                  BUG();
>          }
>   }
> 
> btw: should this read %p ?

-- 
Regards,

Laurent Pinchart

Re: [patch] [media] uvcvideo: freeing an error pointer

[Dan Carpenter]

On Fri, Nov 25, 2016 at 03:57:51PM +0200, Laurent Pinchart wrote:
> diff --git a/mm/slab.c b/mm/slab.c
> index 0b0550ca85b4..a7eb830c6684 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -3819,6 +3819,8 @@ void kfree(const void *objp)
>  
>  	if (unlikely(ZERO_OR_NULL_PTR(objp)))
>  		return;
> +	if (WARN_ON(IS_ERR(objp)))
> +		return;
>  	local_irq_save(flags);
>  	kfree_debugcheck(objp);
>  	c = virt_to_cache(objp);
> 

A bunch of people have proposed that.  You're the first person to
actually write up a patch.  Feel free to send it.  ;)

regards,
dan carpenter

Re: [patch] [media] uvcvideo: freeing an error pointer

[Dan Carpenter]

On Fri, Nov 25, 2016 at 03:57:51PM +0200, Laurent Pinchart wrote:
> diff --git a/mm/slab.c b/mm/slab.c
> index 0b0550ca85b4..a7eb830c6684 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -3819,6 +3819,8 @@ void kfree(const void *objp)
>  
>  	if (unlikely(ZERO_OR_NULL_PTR(objp)))
>  		return;
> +	if (WARN_ON(IS_ERR(objp)))
> +		return;
>  	local_irq_save(flags);
>  	kfree_debugcheck(objp);
>  	c = virt_to_cache(objp);
> 

A bunch of people have proposed that.  You're the first person to
actually write up a patch.  Feel free to send it.  ;)

regards,
dan carpenter

Re: [patch] [media] uvcvideo: freeing an error pointer

[Dan Carpenter]

On Fri, Nov 25, 2016 at 06:02:45PM +0200, Laurent Pinchart wrote:
> Sakari Ailus (CC'ed) has expressed the opinion that we might want to go one 
> step further and treat error pointers the same way we treat NULL or ZERO 
> pointers today, by just returning without logging anything. The reasoning is 
> that accepting a NULL pointer in kfree() was decided before we made extensive 
> use of allocation APIs returning error pointers, so it could be time to update 
> kfree() based on the current allocation usage patterns.

Just don't free things that haven't been allocated.  That honestly seems
like a simple rule to me, whenever I touch error handling code it feels
better and simpler after I fix the bugs.  Error handling doesn't have to
be complicated if you just follow the rules.

regards,
dan carpenter

Re: [patch] [media] uvcvideo: freeing an error pointer

[Dan Carpenter]

On Fri, Nov 25, 2016 at 06:02:45PM +0200, Laurent Pinchart wrote:
> Sakari Ailus (CC'ed) has expressed the opinion that we might want to go one 
> step further and treat error pointers the same way we treat NULL or ZERO 
> pointers today, by just returning without logging anything. The reasoning is 
> that accepting a NULL pointer in kfree() was decided before we made extensive 
> use of allocation APIs returning error pointers, so it could be time to update 
> kfree() based on the current allocation usage patterns.

Just don't free things that haven't been allocated.  That honestly seems
like a simple rule to me, whenever I touch error handling code it feels
better and simpler after I fix the bugs.  Error handling doesn't have to
be complicated if you just follow the rules.

regards,
dan carpenter

Re: [patch] [media] uvcvideo: freeing an error pointer

[Sakari Alius]

Hi Dan,

On Fri, Nov 25, 2016 at 10:20:24PM +0300, Dan Carpenter wrote:
> On Fri, Nov 25, 2016 at 06:02:45PM +0200, Laurent Pinchart wrote:
> > Sakari Ailus (CC'ed) has expressed the opinion that we might want to go one 
> > step further and treat error pointers the same way we treat NULL or ZERO 
> > pointers today, by just returning without logging anything. The reasoning is 
> > that accepting a NULL pointer in kfree() was decided before we made extensive 
> > use of allocation APIs returning error pointers, so it could be time to update 
> > kfree() based on the current allocation usage patterns.
> 
> Just don't free things that haven't been allocated.  That honestly seems
> like a simple rule to me, whenever I touch error handling code it feels
> better and simpler after I fix the bugs.  Error handling doesn't have to
> be complicated if you just follow the rules.

kfree() explicitly allows passing a NULL pointer to it; drivers often call
kfree() on objects possibly allocated using kmalloc() and friends. This
makes error handling easier in drivers which in turn decreases the
probability of bugs, the other side of which we've already seen in form of
the bug this patch fixes.

Previously interfaces that allocated memory tended to either allocate that
memory or in failing to do so, returned error in form of a NULL pointer.
memdup_user() breaks that assumption by returning a negative error value as
a pointer instead.

I suppose one of the motivations of memdup_user() has been to reduce
complexity of driver code as well as framework code dealing with
implementing IOCTLs but at least in this case the end result was an
introduction of a bug. This would not have happened in the first place if
the API of functions dealing with releasing memory had been updated as well.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [patch] [media] uvcvideo: freeing an error pointer

[Sakari Alius]

Hi Dan,

On Fri, Nov 25, 2016 at 10:20:24PM +0300, Dan Carpenter wrote:
> On Fri, Nov 25, 2016 at 06:02:45PM +0200, Laurent Pinchart wrote:
> > Sakari Ailus (CC'ed) has expressed the opinion that we might want to go one 
> > step further and treat error pointers the same way we treat NULL or ZERO 
> > pointers today, by just returning without logging anything. The reasoning is 
> > that accepting a NULL pointer in kfree() was decided before we made extensive 
> > use of allocation APIs returning error pointers, so it could be time to update 
> > kfree() based on the current allocation usage patterns.
> 
> Just don't free things that haven't been allocated.  That honestly seems
> like a simple rule to me, whenever I touch error handling code it feels
> better and simpler after I fix the bugs.  Error handling doesn't have to
> be complicated if you just follow the rules.

kfree() explicitly allows passing a NULL pointer to it; drivers often call
kfree() on objects possibly allocated using kmalloc() and friends. This
makes error handling easier in drivers which in turn decreases the
probability of bugs, the other side of which we've already seen in form of
the bug this patch fixes.

Previously interfaces that allocated memory tended to either allocate that
memory or in failing to do so, returned error in form of a NULL pointer.
memdup_user() breaks that assumption by returning a negative error value as
a pointer instead.

I suppose one of the motivations of memdup_user() has been to reduce
complexity of driver code as well as framework code dealing with
implementing IOCTLs but at least in this case the end result was an
introduction of a bug. This would not have happened in the first place if
the API of functions dealing with releasing memory had been updated as well.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [patch] [media] uvcvideo: freeing an error pointer

[Dan Carpenter]

I understand the comparison, but I just think it's better if people
always keep track of what has been allocated and what has not.  I tried
so hard to get Markus to stop sending those hundreds of patches where
he's like "this function has a sanity check so we can pass pointers
that weren't allocated"...  It's garbage code.

But I understand that other people don't agree.

regards,
dan carpenter

Re: [patch] [media] uvcvideo: freeing an error pointer

[Dan Carpenter]

I understand the comparison, but I just think it's better if people
always keep track of what has been allocated and what has not.  I tried
so hard to get Markus to stop sending those hundreds of patches where
he's like "this function has a sanity check so we can pass pointers
that weren't allocated"...  It's garbage code.

But I understand that other people don't agree.

regards,
dan carpenter

Re: [patch] [media] uvcvideo: freeing an error pointer

[Julia Lawall]

On Mon, 28 Nov 2016, Dan Carpenter wrote:

> I understand the comparison, but I just think it's better if people
> always keep track of what has been allocated and what has not.  I tried
> so hard to get Markus to stop sending those hundreds of patches where
> he's like "this function has a sanity check so we can pass pointers
> that weren't allocated"...  It's garbage code.
>
> But I understand that other people don't agree.

In my opinion, it is good for code understanding to only do what is useful
to do.  It's not a hard and fast rule, but I think it is something to take
into account.

julia

>
> regards,
> dan carpenter
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [patch] [media] uvcvideo: freeing an error pointer

[Julia Lawall]

On Mon, 28 Nov 2016, Dan Carpenter wrote:

> I understand the comparison, but I just think it's better if people
> always keep track of what has been allocated and what has not.  I tried
> so hard to get Markus to stop sending those hundreds of patches where
> he's like "this function has a sanity check so we can pass pointers
> that weren't allocated"...  It's garbage code.
>
> But I understand that other people don't agree.

In my opinion, it is good for code understanding to only do what is useful
to do.  It's not a hard and fast rule, but I think it is something to take
into account.

julia

>
> regards,
> dan carpenter
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [patch] [media] uvcvideo: freeing an error pointer

[Laurent Pinchart]

Hi Julia and Dan,

On Monday 28 Nov 2016 14:54:58 Julia Lawall wrote:
> On Mon, 28 Nov 2016, Dan Carpenter wrote:
> > I understand the comparison, but I just think it's better if people
> > always keep track of what has been allocated and what has not.  I tried
> > so hard to get Markus to stop sending those hundreds of patches where
> > he's like "this function has a sanity check so we can pass pointers
> > that weren't allocated"...  It's garbage code.
> > 
> > But I understand that other people don't agree.
> 
> In my opinion, it is good for code understanding to only do what is useful
> to do.  It's not a hard and fast rule, but I think it is something to take
> into account.

On the other hand it complicates the error handling code by increasing the 
number of goto labels, and it then becomes pretty easy when reworking code to 
goto the wrong label. This is even more of an issue when the rework doesn't 
touch the error handling code, in which case the reviewers can easily miss the 
issue if they don't open the original source file to check the goto labels.

-- 
Regards,

Laurent Pinchart

Re: [patch] [media] uvcvideo: freeing an error pointer

[Laurent Pinchart]

Hi Julia and Dan,

On Monday 28 Nov 2016 14:54:58 Julia Lawall wrote:
> On Mon, 28 Nov 2016, Dan Carpenter wrote:
> > I understand the comparison, but I just think it's better if people
> > always keep track of what has been allocated and what has not.  I tried
> > so hard to get Markus to stop sending those hundreds of patches where
> > he's like "this function has a sanity check so we can pass pointers
> > that weren't allocated"...  It's garbage code.
> > 
> > But I understand that other people don't agree.
> 
> In my opinion, it is good for code understanding to only do what is useful
> to do.  It's not a hard and fast rule, but I think it is something to take
> into account.

On the other hand it complicates the error handling code by increasing the 
number of goto labels, and it then becomes pretty easy when reworking code to 
goto the wrong label. This is even more of an issue when the rework doesn't 
touch the error handling code, in which case the reviewers can easily miss the 
issue if they don't open the original source file to check the goto labels.

-- 
Regards,

Laurent Pinchart

Re: [patch] [media] uvcvideo: freeing an error pointer

[Julia Lawall]

To put it another way, tools can figure out what is missing if they have
access to good exmples of what should be there.

To be clear, I can see both points of view.

julia

Re: [patch] [media] uvcvideo: freeing an error pointer

[Julia Lawall]

To put it another way, tools can figure out what is missing if they have
access to good exmples of what should be there.

To be clear, I can see both points of view.

julia

Re: [patch] [media] uvcvideo: freeing an error pointer

[Dan Carpenter]

On Mon, Nov 28, 2016 at 04:49:36PM +0200, Laurent Pinchart wrote:
> Hi Julia and Dan,
> 
> On Monday 28 Nov 2016 14:54:58 Julia Lawall wrote:
> > On Mon, 28 Nov 2016, Dan Carpenter wrote:
> > > I understand the comparison, but I just think it's better if people
> > > always keep track of what has been allocated and what has not.  I tried
> > > so hard to get Markus to stop sending those hundreds of patches where
> > > he's like "this function has a sanity check so we can pass pointers
> > > that weren't allocated"...  It's garbage code.
> > > 
> > > But I understand that other people don't agree.
> > 
> > In my opinion, it is good for code understanding to only do what is useful
> > to do.  It's not a hard and fast rule, but I think it is something to take
> > into account.
> 
> On the other hand it complicates the error handling code by increasing the 
> number of goto labels, and it then becomes pretty easy when reworking code to 
> goto the wrong label. This is even more of an issue when the rework doesn't 
> touch the error handling code, in which case the reviewers can easily miss the 
> issue if they don't open the original source file to check the goto labels.
> 

It's really not.  I've looked at a lot of error handling in the past
five years and sent hundreds of fixes for error paths, more than any
other kernel developer during that time.  Although it seems obvious in
retrospect, it took me years to realize this but the canonical way of
doing error handling is the least error prone.

Counting the labels is the wrong way to measure complexity.  That's like
counting the number of functions.  Code with lots of functions is not
necessarily more complicated than if it's just one big function.

Part of the key to unwinding correctly is using good label names.  It
should say what the label does.  Some people use come-from labels names
like "goto kmalloc_failed".  Those are totally useless.  It's like
naming your functions "called_from_foo()".  If there is only one goto
then come-from label names are useless and if there are more than one
goto which go to the same label then it's useless *and* misleading.

Functions should be written so you can read them from top to bottom
without scrolling back and forth.

	a = alloc();
	if (!a)
		return -ENOMEM;

	b = alloc();
	if (!b) {
		ret = -ENOMEM;
		goto free_a;
	}

That code tells a complete story without any scrolling.  It's future
proof too.  You can tell the goto is correct just from the name.  But
when it's:

	a = alloc();
	if (!a)
		goto out;
	b = alloc();
		goto out;

That code doesn't have enough information to be understandable on it's
own.  It's way more bug prone than the first sample.

regards,
dan carpenter

Re: [patch] [media] uvcvideo: freeing an error pointer

[Dan Carpenter]

On Mon, Nov 28, 2016 at 04:49:36PM +0200, Laurent Pinchart wrote:
> Hi Julia and Dan,
> 
> On Monday 28 Nov 2016 14:54:58 Julia Lawall wrote:
> > On Mon, 28 Nov 2016, Dan Carpenter wrote:
> > > I understand the comparison, but I just think it's better if people
> > > always keep track of what has been allocated and what has not.  I tried
> > > so hard to get Markus to stop sending those hundreds of patches where
> > > he's like "this function has a sanity check so we can pass pointers
> > > that weren't allocated"...  It's garbage code.
> > > 
> > > But I understand that other people don't agree.
> > 
> > In my opinion, it is good for code understanding to only do what is useful
> > to do.  It's not a hard and fast rule, but I think it is something to take
> > into account.
> 
> On the other hand it complicates the error handling code by increasing the 
> number of goto labels, and it then becomes pretty easy when reworking code to 
> goto the wrong label. This is even more of an issue when the rework doesn't 
> touch the error handling code, in which case the reviewers can easily miss the 
> issue if they don't open the original source file to check the goto labels.
> 

It's really not.  I've looked at a lot of error handling in the past
five years and sent hundreds of fixes for error paths, more than any
other kernel developer during that time.  Although it seems obvious in
retrospect, it took me years to realize this but the canonical way of
doing error handling is the least error prone.

Counting the labels is the wrong way to measure complexity.  That's like
counting the number of functions.  Code with lots of functions is not
necessarily more complicated than if it's just one big function.

Part of the key to unwinding correctly is using good label names.  It
should say what the label does.  Some people use come-from labels names
like "goto kmalloc_failed".  Those are totally useless.  It's like
naming your functions "called_from_foo()".  If there is only one goto
then come-from label names are useless and if there are more than one
goto which go to the same label then it's useless *and* misleading.

Functions should be written so you can read them from top to bottom
without scrolling back and forth.

	a = alloc();
	if (!a)
		return -ENOMEM;

	b = alloc();
	if (!b) {
		ret = -ENOMEM;
		goto free_a;
	}

That code tells a complete story without any scrolling.  It's future
proof too.  You can tell the goto is correct just from the name.  But
when it's:

	a = alloc();
	if (!a)
		goto out;
	b = alloc();
		goto out;

That code doesn't have enough information to be understandable on it's
own.  It's way more bug prone than the first sample.

regards,
dan carpenter

Re: [patch] [media] uvcvideo: freeing an error pointer

[Laurent Pinchart]

Hi Dan,

On Wednesday 30 Nov 2016 15:33:26 Dan Carpenter wrote:
> On Mon, Nov 28, 2016 at 04:49:36PM +0200, Laurent Pinchart wrote:
> > On Monday 28 Nov 2016 14:54:58 Julia Lawall wrote:
> >> On Mon, 28 Nov 2016, Dan Carpenter wrote:
> >>> I understand the comparison, but I just think it's better if people
> >>> always keep track of what has been allocated and what has not.  I
> >>> tried so hard to get Markus to stop sending those hundreds of patches
> >>> where he's like "this function has a sanity check so we can pass
> >>> pointers that weren't allocated"...  It's garbage code.
> >>> 
> >>> But I understand that other people don't agree.
> >> 
> >> In my opinion, it is good for code understanding to only do what is
> >> useful to do.  It's not a hard and fast rule, but I think it is
> >> something to take into account.
> > 
> > On the other hand it complicates the error handling code by increasing the
> > number of goto labels, and it then becomes pretty easy when reworking code
> > to goto the wrong label. This is even more of an issue when the rework
> > doesn't touch the error handling code, in which case the reviewers can
> > easily miss the issue if they don't open the original source file to
> > check the goto labels.
>
> It's really not.  I've looked at a lot of error handling in the past
> five years and sent hundreds of fixes for error paths, more than any
> other kernel developer during that time.  Although it seems obvious in
> retrospect, it took me years to realize this but the canonical way of
> doing error handling is the least error prone.
> 
> Counting the labels is the wrong way to measure complexity.  That's like
> counting the number of functions.  Code with lots of functions is not
> necessarily more complicated than if it's just one big function.
> 
> Part of the key to unwinding correctly is using good label names.  It
> should say what the label does.  Some people use come-from labels names
> like "goto kmalloc_failed".  Those are totally useless.  It's like
> naming your functions "called_from_foo()".  If there is only one goto
> then come-from label names are useless and if there are more than one
> goto which go to the same label then it's useless *and* misleading.

Yes, label naming is (or at least should be) largely agreed upon, they should 
name the unwinding action, not the cause of the failure.

> Functions should be written so you can read them from top to bottom
> without scrolling back and forth.
> 
> 	a = alloc();
> 	if (!a)
> 		return -ENOMEM;
> 
> 	b = alloc();
> 	if (!b) {
> 		ret = -ENOMEM;
> 		goto free_a;
> 	}

But then you get the following patch (which, apart from being totally made up, 
probably shows what I've watched yesterday evening).

@@ ... @@
 		return -ENOMEM;
 	}
 
+	ret = check_time_vortex();
+	if (ret < 0)
+		goto power_off_tardis;
+
	matt_smith = alloc_regeneration();
	if (math_smith->status != OK) {
		ret = -E_NEEDS_FISH_FINGERS_AND_CUSTARD;

>From that code only you can't tell whether the jump label is the right one. If 
a single jump label is used with an unwinding code block that supports non-
allocated resources, you don't have to ask yourself any question.

> That code tells a complete story without any scrolling.  It's future
> proof too.  You can tell the goto is correct just from the name.  But
> when it's:
> 
> 	a = alloc();
> 	if (!a)
> 		goto out;
> 	b = alloc();
> 		goto out;
> 
> That code doesn't have enough information to be understandable on it's
> own.  It's way more bug prone than the first sample.

-- 
Regards,

Laurent Pinchart

Re: [patch] [media] uvcvideo: freeing an error pointer

[Laurent Pinchart]

Hi Dan,

On Wednesday 30 Nov 2016 15:33:26 Dan Carpenter wrote:
> On Mon, Nov 28, 2016 at 04:49:36PM +0200, Laurent Pinchart wrote:
> > On Monday 28 Nov 2016 14:54:58 Julia Lawall wrote:
> >> On Mon, 28 Nov 2016, Dan Carpenter wrote:
> >>> I understand the comparison, but I just think it's better if people
> >>> always keep track of what has been allocated and what has not.  I
> >>> tried so hard to get Markus to stop sending those hundreds of patches
> >>> where he's like "this function has a sanity check so we can pass
> >>> pointers that weren't allocated"...  It's garbage code.
> >>> 
> >>> But I understand that other people don't agree.
> >> 
> >> In my opinion, it is good for code understanding to only do what is
> >> useful to do.  It's not a hard and fast rule, but I think it is
> >> something to take into account.
> > 
> > On the other hand it complicates the error handling code by increasing the
> > number of goto labels, and it then becomes pretty easy when reworking code
> > to goto the wrong label. This is even more of an issue when the rework
> > doesn't touch the error handling code, in which case the reviewers can
> > easily miss the issue if they don't open the original source file to
> > check the goto labels.
>
> It's really not.  I've looked at a lot of error handling in the past
> five years and sent hundreds of fixes for error paths, more than any
> other kernel developer during that time.  Although it seems obvious in
> retrospect, it took me years to realize this but the canonical way of
> doing error handling is the least error prone.
> 
> Counting the labels is the wrong way to measure complexity.  That's like
> counting the number of functions.  Code with lots of functions is not
> necessarily more complicated than if it's just one big function.
> 
> Part of the key to unwinding correctly is using good label names.  It
> should say what the label does.  Some people use come-from labels names
> like "goto kmalloc_failed".  Those are totally useless.  It's like
> naming your functions "called_from_foo()".  If there is only one goto
> then come-from label names are useless and if there are more than one
> goto which go to the same label then it's useless *and* misleading.

Yes, label naming is (or at least should be) largely agreed upon, they should 
name the unwinding action, not the cause of the failure.

> Functions should be written so you can read them from top to bottom
> without scrolling back and forth.
> 
> 	a = alloc();
> 	if (!a)
> 		return -ENOMEM;
> 
> 	b = alloc();
> 	if (!b) {
> 		ret = -ENOMEM;
> 		goto free_a;
> 	}

But then you get the following patch (which, apart from being totally made up, 
probably shows what I've watched yesterday evening).

@@ ... @@
 		return -ENOMEM;
 	}
 
+	ret = check_time_vortex();
+	if (ret < 0)
+		goto power_off_tardis;
+
	matt_smith = alloc_regeneration();
	if (math_smith->status != OK) {
		ret = -E_NEEDS_FISH_FINGERS_AND_CUSTARD;

From that code only you can't tell whether the jump label is the right one. If 
a single jump label is used with an unwinding code block that supports non-
allocated resources, you don't have to ask yourself any question.

> That code tells a complete story without any scrolling.  It's future
> proof too.  You can tell the goto is correct just from the name.  But
> when it's:
> 
> 	a = alloc();
> 	if (!a)
> 		goto out;
> 	b = alloc();
> 		goto out;
> 
> That code doesn't have enough information to be understandable on it's
> own.  It's way more bug prone than the first sample.

-- 
Regards,

Laurent Pinchart

Re: [patch] [media] uvcvideo: freeing an error pointer

[Dan Carpenter]

On Wed, Nov 30, 2016 at 03:53:03PM +0200, Laurent Pinchart wrote:
> But then you get the following patch (which, apart from being totally made up, 
> probably shows what I've watched yesterday evening).
> 
> @@ ... @@
>  		return -ENOMEM;
>  	}
>  
> +	ret = check_time_vortex();
> +	if (ret < 0)
> +		goto power_off_tardis;
> +
> 	matt_smith = alloc_regeneration();
> 	if (math_smith->status != OK) {
> 		ret = -E_NEEDS_FISH_FINGERS_AND_CUSTARD;
> 


I don't get it.  Did we power on the tardis on the lines before?  That's
all the state that you need to keep in your head is just the most
recently allocated thing.

> >From that code only you can't tell whether the jump label is the right one. If 
> a single jump label is used with an unwinding code block that supports non-
> allocated resources, you don't have to ask yourself any question.
> 

You absolutely do have to ask that question, you just can't answer it
without jumping back and forth.  Doing everything at once is logically
more complicated than doing them one thing at a time, and empirically
just from looking at which code has the most bugs, then single exit
labels are the most buggy.

regards,
dan carpenter

Re: [patch] [media] uvcvideo: freeing an error pointer

[Dan Carpenter]

On Wed, Nov 30, 2016 at 03:53:03PM +0200, Laurent Pinchart wrote:
> But then you get the following patch (which, apart from being totally made up, 
> probably shows what I've watched yesterday evening).
> 
> @@ ... @@
>  		return -ENOMEM;
>  	}
>  
> +	ret = check_time_vortex();
> +	if (ret < 0)
> +		goto power_off_tardis;
> +
> 	matt_smith = alloc_regeneration();
> 	if (math_smith->status != OK) {
> 		ret = -E_NEEDS_FISH_FINGERS_AND_CUSTARD;
> 


I don't get it.  Did we power on the tardis on the lines before?  That's
all the state that you need to keep in your head is just the most
recently allocated thing.

> >From that code only you can't tell whether the jump label is the right one. If 
> a single jump label is used with an unwinding code block that supports non-
> allocated resources, you don't have to ask yourself any question.
> 

You absolutely do have to ask that question, you just can't answer it
without jumping back and forth.  Doing everything at once is logically
more complicated than doing them one thing at a time, and empirically
just from looking at which code has the most bugs, then single exit
labels are the most buggy.

regards,
dan carpenter