IS_ERR_OR_NULL - please STOP telling people to use this on a whim

IS_ERR_OR_NULL - please STOP telling people to use this on a whim

[Russell King - ARM Linux]

People,

This is not aimed at anyone specifically - but it is aimed at everyone
who reviews patches to make them aware of this issue, and to modify their
behaviour.

I'm geting sick and tired of telling people about this.  I've just
floated the idea of removing IS_ERR_OR_NULL from the kernel tree because
it's one of the most incorrectly used and abused macros we have in the
source tree.

It would be one thing if this was only being done by people who are
submitting new code, but it's far worse than that.  Reviewers who should
know better are telling people to use it _incorrectly_.

Reviewers really need to think about your review comments.  Looking
through the kernel tree today, I see lots of uses of IS_ERR_OR_NULL(),
many of them are *buggy*.

Take a moment to think about this:

int error_value(struct device *dev, void *foo)
{
	if (IS_ERR_OR_NULL(foo))
		return PTR_ERR(foo);
	return 0;
}

Consider the value this function returns for three arguments:

1. an errno encoded pointer
2. a NULL pointer.
3. a valid pointer.

If you can't see the problem, then *do* *not* tell anyone to use
IS_ERR_OR_NULL(), because you do *not* have the understanding necessary
to make that judgement yourself - you're probably telling people to
create buggy code.

Here's the list so far of what looks like buggy uses specific to ARM.
There _are_ others elsewhere in the kernel.

drivers/media/video/s5p-mfc/s5p_mfc.c:  if (IS_ERR_OR_NULL(dev->alloc_ctx[0])) {
drivers/media/video/s5p-mfc/s5p_mfc.c-          ret = PTR_ERR(dev->alloc_ctx[0]);
drivers/media/video/s5p-mfc/s5p_mfc.c-          goto err_res;
drivers/media/video/s5p-mfc/s5p_mfc.c-  }
--
drivers/media/video/s5p-mfc/s5p_mfc.c:  if (IS_ERR_OR_NULL(dev->alloc_ctx[1])) {
drivers/media/video/s5p-mfc/s5p_mfc.c-          ret = PTR_ERR(dev->alloc_ctx[1]);
drivers/media/video/s5p-mfc/s5p_mfc.c-          goto err_mem_init_ctx_1;
drivers/media/video/s5p-mfc/s5p_mfc.c-  }
--
drivers/staging/omapdrm/omap_dmm_tiler.c:       if (IS_ERR_OR_NULL(txn))
drivers/staging/omapdrm/omap_dmm_tiler.c-               return PTR_ERR(txn);
drivers/staging/omapdrm/omap_dmm_tiler.c-
drivers/staging/omapdrm/omap_dmm_tiler.c-       tcm_for_each_slice(slice, *area, area_s) {
--
drivers/staging/omap-thermal/omap-bandgap.c:    if (IS_ERR_OR_NULL(bg_ptr)) {
drivers/staging/omap-thermal/omap-bandgap.c-            dev_err(&pdev->dev, "failed to fetch platform data\n");
drivers/staging/omap-thermal/omap-bandgap.c-            return PTR_ERR(bg_ptr);
drivers/staging/omap-thermal/omap-bandgap.c-    }
--
drivers/staging/omap-thermal/omap-thermal-common.c:     if (IS_ERR_OR_NULL(data->omap_thermal)) {
drivers/staging/omap-thermal/omap-thermal-common.c-             dev_err(bg_ptr->dev, "thermal zone device is NULL\n");
drivers/staging/omap-thermal/omap-thermal-common.c-             return PTR_ERR(data->omap_thermal);
drivers/staging/omap-thermal/omap-thermal-common.c-     }
--
drivers/staging/omap-thermal/omap-thermal-common.c:     if (IS_ERR_OR_NULL(freq_table)) {
drivers/staging/omap-thermal/omap-thermal-common.c-             dev_err(bg_ptr->dev,
drivers/staging/omap-thermal/omap-thermal-common.c-                     "%s: failed to get cpufreq table (%p)\n",
drivers/staging/omap-thermal/omap-thermal-common.c-                     __func__, freq_table);
--
drivers/staging/omap-thermal/omap-thermal-common.c:     if (IS_ERR_OR_NULL(data->cool_dev)) {
drivers/staging/omap-thermal/omap-thermal-common.c-             dev_err(bg_ptr->dev,
drivers/staging/omap-thermal/omap-thermal-common.c-                     "Failed to register cpufreq cooling device\n");
drivers/staging/omap-thermal/omap-thermal-common.c-             return PTR_ERR(data->cool_dev);
--
drivers/gpu/drm/exynos/exynos_drm_dmabuf.c:     if (IS_ERR_OR_NULL(sgt)) {
drivers/gpu/drm/exynos/exynos_drm_dmabuf.c-             ret = PTR_ERR(sgt);
drivers/gpu/drm/exynos/exynos_drm_dmabuf.c-             goto err_buf_detach;
drivers/gpu/drm/exynos/exynos_drm_dmabuf.c-     }
--
drivers/gpu/drm/exynos/exynos_drm_fbdev.c:      if (IS_ERR_OR_NULL(helper->fb)) {
drivers/gpu/drm/exynos/exynos_drm_fbdev.c-              DRM_ERROR("failed to create drm framebuffer.\n");
drivers/gpu/drm/exynos/exynos_drm_fbdev.c-              ret = PTR_ERR(helper->fb);
drivers/gpu/drm/exynos/exynos_drm_fbdev.c-              goto out;
--

Re: IS_ERR_OR_NULL - please STOP telling people to use this on a whim

[Phil Carmody]

On 17/10/12 20:41 +0100, Russell King - ARM Linux wrote:
> People,
> 
> This is not aimed at anyone specifically - but it is aimed at everyone
> who reviews patches to make them aware of this issue, and to modify their
> behaviour.
> 
> I'm geting sick and tired of telling people about this.  I've just
> floated the idea of removing IS_ERR_OR_NULL from the kernel tree because
> it's one of the most incorrectly used and abused macros we have in the
> source tree.

This makes me sad. I was responsible for its introduction, and my motive
was exactly yours in sending the above. 

> It would be one thing if this was only being done by people who are
> submitting new code, but it's far worse than that.  Reviewers who should
> know better are telling people to use it _incorrectly_.
> 
> Reviewers really need to think about your review comments.  Looking
> through the kernel tree today, I see lots of uses of IS_ERR_OR_NULL(),
> many of them are *buggy*.
> 
> Take a moment to think about this:
> 
> int error_value(struct device *dev, void *foo)
> {
> 	if (IS_ERR_OR_NULL(foo))
> 		return PTR_ERR(foo);
> 	return 0;
> }
> 
> Consider the value this function returns for three arguments:
> 
> 1. an errno encoded pointer
> 2. a NULL pointer.
> 3. a valid pointer.
> 
> If you can't see the problem, then *do* *not* tell anyone to use
> IS_ERR_OR_NULL(), because you do *not* have the understanding necessary
> to make that judgement yourself - you're probably telling people to
> create buggy code.

The problem I saw was functions returning -ERRORs or NULL. There were
too many, and there was too much sloppy code inconsistently handling one
or either of the two, and not always both. I did consider trying to fix
some of the core functions that were returning -ERRORs or NULL to the
drivers I was involved in, but it seemed like there were too many, and
that would be too "brave". I imagined that my macro would help catch
that undesirable situation, and permit people to map the error onto
whatever was most appropriate to propagate on.

The idea of them propagating the undesirable problem up further in the call
chain is the exact antithesis of what I intended.

Thank you for highlighting the issue I didn't foresee (neither did my 
colleagues at Nokia, they made good use of it fairly quickly) and in 
such unambiguous terms. Better to nip it in the bud, certainly.

> Here's the list so far of what looks like buggy uses specific to ARM.
> There _are_ others elsewhere in the kernel.
> 
> drivers/media/video/s5p-mfc/s5p_mfc.c:  if (IS_ERR_OR_NULL(dev->alloc_ctx[0])) {
> drivers/media/video/s5p-mfc/s5p_mfc.c-          ret = PTR_ERR(dev->alloc_ctx[0]);
> drivers/media/video/s5p-mfc/s5p_mfc.c-          goto err_res;
> drivers/media/video/s5p-mfc/s5p_mfc.c-  }
...

:-(

So, what to do? It can and has been used sensibly, so I don't think removing
it is the best option.

Phil

"pcarmody@nokia.com" is no more, I'm now "pc+lkml@asdf.org"

Re: IS_ERR_OR_NULL - please STOP telling people to use this on a whim

[Russell King - ARM Linux]

On Wed, Oct 17, 2012 at 11:28:48PM +0300, Phil Carmody wrote:
> So, what to do? It can and has been used sensibly, so I don't think removing
> it is the best option.

Well, the first thing that needs to be done is a full review of every user
and fixes applied.

The second thing is that we need eyes on code _and_ review comments, and
educate those who are telling people to use IS_ERR_OR_NULL() whenever they
see an IS_ERR() to think about the code a little more - that's kind of the
purpose of my email.

Unfortunately, it's going to take time to achieve a change, and if I end
up being the only one who's spotting these errors, I'm going to get
incredibly pissed off at doing so (because it will feel like I'm being
ignored when there's a constant stream of it.)

Another thing would be to work out whether we can get checkpatch to
detect usage of IS_ERR_OR_NULL(p) followed by PTR_ERR(p) without any
explicit NULL checks against p in the same block.  That's probably
going to be interesting to code up in perl.

Re: IS_ERR_OR_NULL - please STOP telling people to use this on a whim

[Nicolas Ferre]

On 10/17/2012 11:33 PM, Russell King - ARM Linux :
> On Wed, Oct 17, 2012 at 11:28:48PM +0300, Phil Carmody wrote:
>> So, what to do? It can and has been used sensibly, so I don't think removing
>> it is the best option.
> 
> Well, the first thing that needs to be done is a full review of every user
> and fixes applied.
> 
> The second thing is that we need eyes on code _and_ review comments, and
> educate those who are telling people to use IS_ERR_OR_NULL() whenever they
> see an IS_ERR() to think about the code a little more - that's kind of the
> purpose of my email.
> 
> Unfortunately, it's going to take time to achieve a change, and if I end
> up being the only one who's spotting these errors, I'm going to get
> incredibly pissed off at doing so (because it will feel like I'm being
> ignored when there's a constant stream of it.)
> 
> Another thing would be to work out whether we can get checkpatch to
> detect usage of IS_ERR_OR_NULL(p) followed by PTR_ERR(p) without any
> explicit NULL checks against p in the same block.  That's probably
> going to be interesting to code up in perl.

True that it would make sense to include in checkpatch to be able to
block code beforehand.
But for sure correction of existing code seems to be a work for Coccinelle.

Best regards,
-- 
Nicolas Ferre