Re: net-PA Semi: Deletion of unnecessary checks before the function call "pci_dev_put"

Re: net-PA Semi: Deletion of unnecessary checks before the function call "pci_dev_put"

[SF Markus Elfring]

> I know there has been some criticism about those kind of "code
> improvements" already but i would like to point out just one more thing:
> 
> Some of those NULL pointer checks on input parameters may have been
> added subsequently to functions. So there may be older kernel versions
> out there in which those checks dont exists in some cases. If some of
> the now "cleaned up" code is backported to such a kernel chances are
> good that those missing checks are overseen. And then neither caller nor
> callee is doing the NULL pointer check.

I guess that the Coccinelle software can also help you in this use case.
How do you think about to shield against "unwanted" or unexpected collateral
evolutions with additional inline functions?

I assume that a few backporters can tell you more about their corresponding
software development experiences.
http://www.do-not-panic.com/2014/04/automatic-linux-kernel-backporting-with-coccinelle.html

Regards,
Markus

Re: net-PA Semi: Deletion of unnecessary checks before the function call "pci_dev_put"

[Johannes Berg]

On Mon, 2014-12-01 at 02:34 +0100, SF Markus Elfring wrote:

> > Some of those NULL pointer checks on input parameters may have been
> > added subsequently to functions. So there may be older kernel versions
> > out there in which those checks dont exists in some cases. If some of
> > the now "cleaned up" code is backported to such a kernel chances are
> > good that those missing checks are overseen. And then neither caller nor
> > callee is doing the NULL pointer check.

> I assume that a few backporters can tell you more about their corresponding
> software development experiences.
> http://www.do-not-panic.com/2014/04/automatic-linux-kernel-backporting-with-coccinelle.html

In such cases we just provide an appropriate wrapper and replace callers
of the original function by callers of the wrapper, typically with a
#define.

So this kind of evolution is no problem for the (automated) backports
using the backports project - although it can be difficult to detect
such a thing is needed.

johannes

Re: net-PA Semi: Deletion of unnecessary checks before the function call "pci_dev_put"

[Julia Lawall]

On Mon, 1 Dec 2014, Johannes Berg wrote:

> On Mon, 2014-12-01 at 02:34 +0100, SF Markus Elfring wrote:
> 
> > > Some of those NULL pointer checks on input parameters may have been
> > > added subsequently to functions. So there may be older kernel versions
> > > out there in which those checks dont exists in some cases. If some of
> > > the now "cleaned up" code is backported to such a kernel chances are
> > > good that those missing checks are overseen. And then neither caller nor
> > > callee is doing the NULL pointer check.
> 
> > I assume that a few backporters can tell you more about their corresponding
> > software development experiences.
> > http://www.do-not-panic.com/2014/04/automatic-linux-kernel-backporting-with-coccinelle.html
> 
> In such cases we just provide an appropriate wrapper and replace callers
> of the original function by callers of the wrapper, typically with a
> #define.
> 
> So this kind of evolution is no problem for the (automated) backports
> using the backports project - although it can be difficult to detect
> such a thing is needed.

That is exactly the problem...

julia

Re: net-PA Semi: Deletion of unnecessary checks before the function call "pci_dev_put"

[Johannes Berg]

On Mon, 2014-12-01 at 21:34 +0100, Julia Lawall wrote:

> > So this kind of evolution is no problem for the (automated) backports
> > using the backports project - although it can be difficult to detect
> > such a thing is needed.
> 
> That is exactly the problem...

I'm not convinced though that it should stop such progress in mainline.

johannes

Re: net-PA Semi: Deletion of unnecessary checks before the function call "pci_dev_put"

[Dan Carpenter]

On Tue, Dec 02, 2014 at 05:53:28PM +0100, Johannes Berg wrote:
> On Mon, 2014-12-01 at 21:34 +0100, Julia Lawall wrote:
> 
> > > So this kind of evolution is no problem for the (automated) backports
> > > using the backports project - although it can be difficult to detect
> > > such a thing is needed.
> > 
> > That is exactly the problem...
> 
> I'm not convinced though that it should stop such progress in mainline.

Is it progress?  These patches match the code look simpler by passing
hiding the NULL check inside a function call.  Calling pci_dev_put(NULL)
doesn't make sense.  Just because a sanity check exists doesn't mean we
should do insane things.

It's easy enough to store which functions have a sanity check in a
database, but to rememember all that as a human being trying to read the
code is impossible.

If we really wanted to make this code cleaner we would introduce more
error labels with better names.

regards,
dan carpenter

Re: net-PA Semi: Deletion of unnecessary checks before the function call "pci_dev_put"

[Luis R. Rodriguez]

On Tue, Dec 02, 2014 at 09:35:09PM +0300, Dan Carpenter wrote:
> On Tue, Dec 02, 2014 at 05:53:28PM +0100, Johannes Berg wrote:
> > On Mon, 2014-12-01 at 21:34 +0100, Julia Lawall wrote:
> > 
> > > > So this kind of evolution is no problem for the (automated) backports
> > > > using the backports project - although it can be difficult to detect
> > > > such a thing is needed.
> > > 
> > > That is exactly the problem...
> > 
> > I'm not convinced though that it should stop such progress in mainline.
> 
> Is it progress? 

I like to think of progress as using tools to help fix code where we know
it can be made simpler with a small ammendment: if you can extend the tools
to also vet for safety for backports to avoid crashes even better.

So its a small evolution but we can do better, which is the point you and
Julia are making.

> These patches match the code look simpler by passing
> hiding the NULL check inside a function call.  Calling pci_dev_put(NULL)
> doesn't make sense.  Just because a sanity check exists doesn't mean we
> should do insane things.

It'd crash the system if the function call didn't have the check in place
but having the code in question call pci_dev_put(NULL) is also ludicrious.
Either way in this case I think we shouldn't go beyond analyzing the
function call and if the error check was present before as it is a real
case that has introduced crashes before which Julia wanted to flag.

> It's easy enough to store which functions have a sanity check in a
> database,

This is easy but it adds complexities which I'd prefer to keep on
some other people's workstations. For the developer I think we should
strive to only have: a) git b) Coccinelle c) smatch.

> but to rememember all that as a human being trying to read the
> code is impossible.

Agreed. The problem statement presented by Julia is part of the effort
of addressing the "how do we evolve faster" problem on Linux kernel development,
what you describe adds to the mix of the complexities, and while Oleg does
note that part of this is academic there are those of us who are making things
which are academic immediately practical and a reality for Linux. This is also
how we evolve faster :)

> If we really wanted to make this code cleaner we would introduce more
> error labels with better names.

Can you describe a bit more what you mean here? If we had a label *in code*
on the caller, perhaps a comment, I can see tool-wise how it'd remove the
requirement for a database for immediate analysis for safety here, ie,
we hunt for a label on the code; but other than that its unclear what
you mean here.

If you folks agree with my simplication tool analsysi for safety can
we devise a tag for whitelisting this check for a series of routines?
Where would we put it, in the kernel or a tools package? If in the kernel
we could end up sharing it, so I think that's be better. Perhaps scripts/safety/ ?
Maybe use a header that describes the safety check that is vetted by the rule
present, followed by a list of routines vetted?

Then the Cocci file can preload this and a rule that wants this paranoid check
can include this db file for safety ?

The safety here would require vetting thirough history in git that the routine
has a check in place throughout the routines's history up to a certain point.
I propose we only care up to what kernels are listed on kernel.org as supported.

 Luis

Re: net-PA Semi: Deletion of unnecessary checks before the function call "pci_dev_put"

[Luis R. Rodriguez]

On Tue, Dec 2, 2014 at 11:53 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2014-12-01 at 21:34 +0100, Julia Lawall wrote:
>
>> > So this kind of evolution is no problem for the (automated) backports
>> > using the backports project - although it can be difficult to detect
>> > such a thing is needed.
>>
>> That is exactly the problem...
>
> I'm not convinced though that it should stop such progress in mainline.

I believe this case requires a bit more information explained as to
why it was explained. The "form" of change this patch has is of the
type that can crash systems if the NULL pointer check on the caller
implementation was only added later. We might be able to grammatically
check for this situation in the future if we had a white list / black
list / kernel revision where the NULL check was added but for now we
don't have that and as such care is just required on the developer in
consideration for backports.

It should be up to the maintainer to appreciate the gains of doing
something differently to make it easier for backporting. I obviously
think its a good thing to consider, its extra work though, so only if
the maintainer has some appreciation for backporting would this make
sense.

In this particular case I've reviewed Julia's concern and I've
determined that the patch is safe up to at least v2.6.12-rc2 (which is
where our git history begins on Linus' tree), this is because the
check for NULL has been there since then:

git show 1da177e drivers/pci/pci-driver.c

+void pci_dev_put(struct pci_dev *dev)
+{
+       if (dev)
+               put_device(&dev->dev);
+}

So this type of wide collateral evolution should not cause panics.
Because of this:

Acked-by: Luis R. Rodriguez <mcgrof@suse.com>

But note -- I still think its only good for us to vet these, if we
can't why not? If the maintainer doesn't give a shit that's different,
but if there are folks out there willing to help with vetting then
well, why not :)

PS. Including something like historical vetting as I did above on the
commit log should help folks.

 Luis