From: Markus Elfring <[email protected]>
Date: Sat, 29 Nov 2014 18:55:40 +0100
The pci_dev_put() function tests whether its argument is NULL
and then returns immediately. Thus the test around the call
is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <[email protected]>
---
drivers/net/ethernet/pasemi/pasemi_mac.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/pasemi/pasemi_mac.c b/drivers/net/ethernet/pasemi/pasemi_mac.c
index 30d934d..44e8d7d 100644
--- a/drivers/net/ethernet/pasemi/pasemi_mac.c
+++ b/drivers/net/ethernet/pasemi/pasemi_mac.c
@@ -1837,10 +1837,8 @@ pasemi_mac_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
return err;
out:
- if (mac->iob_pdev)
- pci_dev_put(mac->iob_pdev);
- if (mac->dma_pdev)
- pci_dev_put(mac->dma_pdev);
+ pci_dev_put(mac->iob_pdev);
+ pci_dev_put(mac->dma_pdev);
free_netdev(dev);
out_disable_device:
--
2.1.3
On 29.11.2014 19:00, SF Markus Elfring wrote:
> out:
> - if (mac->iob_pdev)
> - pci_dev_put(mac->iob_pdev);
> - if (mac->dma_pdev)
> - pci_dev_put(mac->dma_pdev);
> + pci_dev_put(mac->iob_pdev);
> + pci_dev_put(mac->dma_pdev);
>
> free_netdev(dev);
> out_disable_device:
>
Hi,
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.
Quite frankly i would vote for the opposite approach: Never rely on the
callee do to checks for NULL and do it always in the caller. An
exception could be for calls on a fast path. But most of those checks
are done on error paths anyway.
Just my 2 cents.
Regards,
Lino
On Sun, 30 Nov 2014, Lino Sanfilippo wrote:
> On 29.11.2014 19:00, SF Markus Elfring wrote:
>
> > out:
> > - if (mac->iob_pdev)
> > - pci_dev_put(mac->iob_pdev);
> > - if (mac->dma_pdev)
> > - pci_dev_put(mac->dma_pdev);
> > + pci_dev_put(mac->iob_pdev);
> > + pci_dev_put(mac->dma_pdev);
> >
> > free_netdev(dev);
> > out_disable_device:
> >
>
> Hi,
>
> 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.
>
> Quite frankly i would vote for the opposite approach: Never rely on the
> callee do to checks for NULL and do it always in the caller. An
> exception could be for calls on a fast path. But most of those checks
> are done on error paths anyway.
I have heard of at least one case where the problem you are raising
happened in practice...
julia
On 30.11.2014 18:47, Julia Lawall wrote:
>
> I have heard of at least one case where the problem you are raising
> happened in practice...
>
> julia
If one case is known then there are probably a lot more that have not
been discovered yet.
Maybe this topic should be clarified somewhere (e.g. in "CodingStyle")?
On the other hand i always found it obvious that its the callers
responsibility to only pass sane parameters to the called functions...
Regards,
Lino
> Maybe this topic should be clarified somewhere (e.g. in "CodingStyle")?
> On the other hand i always found it obvious that its the callers
> responsibility to only pass sane parameters to the called functions...
Can you imagine that any more source code places which would benefit from
check adjustments because of defensive programming?
Regards,
Markus
On 30.11.2014 21:40, SF Markus Elfring wrote:
>> Maybe this topic should be clarified somewhere (e.g. in "CodingStyle")?
>> On the other hand i always found it obvious that its the callers
>> responsibility to only pass sane parameters to the called functions...
>
> Can you imagine that any more source code places which would benefit from
> check adjustments because of defensive programming?
>
I am not sure if i understand your question correctly. But i would not
call sanity checks for function parameters "defensive programming". I
would rather call it not being totally careless. So to me the question
if those checks should be done or not is different from the question
whether there are code parts that would benefit from an adjustment to
defensive programming.
Regards,
Lino
> 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
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
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
On Sat, Nov 29, 2014 at 10:00 AM, SF Markus Elfring
<[email protected]> wrote:
> From: Markus Elfring <[email protected]>
> Date: Sat, 29 Nov 2014 18:55:40 +0100
>
> The pci_dev_put() function tests whether its argument is NULL
> and then returns immediately. Thus the test around the call
> is not needed.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <[email protected]>
For this particular case:
Acked-by: Olof Johansson <[email protected]>
Note that the "this might cause problems with backports" case is
mostly academic in the scope of _this particular driver_. It's still a
very valid discussion and issue though.
So I'll be happy to give the ack on this driver, but the larger
problem needs consideration still.
-Olof
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
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
On Tue, Dec 2, 2014 at 11:53 AM, Johannes Berg
<[email protected]> 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 <[email protected]>
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
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
From: SF Markus Elfring <[email protected]>
Date: Sat, 29 Nov 2014 19:00:17 +0100
> From: Markus Elfring <[email protected]>
> Date: Sat, 29 Nov 2014 18:55:40 +0100
>
> The pci_dev_put() function tests whether its argument is NULL
> and then returns immediately. Thus the test around the call
> is not needed.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <[email protected]>
Applied.