Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Pull network and Patch Acceptance Consistency

[Dan Carpenter <dan.carpenter@oracle.com>]

On Thu, Jun 13, 2019 at 11:03:53AM -0400, Martin K. Petersen wrote:
> 
> James,
> 
> > It depends: every patch you do to an old driver comes with a risk of
> > breakage.  What we've found is even apparently sane patches cause
> > breakage which isn't discovered until months later when someone with
> > the hardware actually tests.
> 
> My pet peeve is with the constant stream of seemingly innocuous
> helper-interface-of-the-week changes. Such as "Use kzfoobar() instead of
> kfoobar() + memset()". And then a year later somebody decides kzfoobar()
> had a subtle adverse side-effect and now we all need to switch to
> kpzfoobar().
> 
> I appreciate that some of these helpers may have merit in terms of
> facilitating static code checkers, etc. But other than that, I really
> fail to see the value of this constant churn.
> 
> The devil is always in the details. It's almost inevitably these obvious
> five-liners that cause regressions down the line.
> 
> So why do we keep doing this?
> 

You haven't provided any specifics so it's hard to discuss this...  The
only example I can think of are the memdup_user() conversions where
the function uses "goto out;" error handling.

out:
	kfree(ptr);
	return ret;

The problem is that in the original code if kmalloc() fails and we goto
out then kfree(NULL) is a no-op but in the new code if memdup_user()
fails "ptr" is an error pointer and kfree(ERR_PTR(-EFAULT)) is an Oops.

As a reviewer, any time I see a "goto out;" that raises a red flag for
me because this style of error handling tends to be more buggy.  Choose
a better label name and don't free stuff that wasn't allocated.

The good news is that static analysis will find these bugs so it's
probably been a couple years since one of these have made it to a
released kernel.

regards,
dan carpenter