Fault tolerance/bad patch, [was Re: [PATCH 29/30] [PATCH] PCI Hotplug: fake NULL pointer dereferences in IBM Hot Plug Controller Driver]

Fault tolerance/bad patch, [was Re: [PATCH 29/30] [PATCH] PCI Hotplug: fake NULL pointer dereferences in IBM Hot Plug Controller Driver]

[Linas Vepstas]

Hi,

On Mon, Jun 19, 2006 at 02:43:35PM -0700, Greg KH wrote:
> From: Eric Sesterhenn <snakebyte@gmx.de>
> 
> Remove checks for value, since the hotplug core always provides
> a valid value.
> 
> -	if (hotplug_slot && value) {
> +	if (hotplug_slot) {

This may be the wrong place to bring up a philosphical issue,
but this example was just too good to pass up.  This patch violates
the general dictates of high-reliability/fault-tolerant programming.

If someone in the future changes the hotplug core so that it 
sometimes returns a null value, this code will potentially crash
and/or do other bad things (corrupt, invalid state, etc.)
This means that this routine will no longer be "robust" in the face of
changes in other parts of the kernel. 

I can hear the objections:
-- Performance. B.S. This routine is not performance critical, it will
   get called once a week, once a month or less often; a few extra
   cycles are utterly irrelevant.

-- Many eyes/shallow bugs. A patch that breaks things would be rejected.
   B.S. Patches that break things get into the kernel all the time.

-- If its broken, testing will find it and fix it. B.S.  The hotplug 
   routines are lightly tested, infrequently tested.  There's not much 
   hardware that does hotplug, and its expensive. Home enthsiasts 
   won't find this.

   Worse, the null-value condition could be a rare corner case that won't 
   show up during "normal" operation, i.e. during "normal" hotplug testing.
   It may only get triggered in some obscuare case e.g. bad pci card
   or sysadmin error, or due to error in a user-land tool.

Hotplug ops are rare; they typically are not done until they are needed 
(e.g. because the card failed), and that is a really bad time to discover 
that you've crashed the kernel.  The whole point of hotplug is to *not* 
have to reboot. 

(If I may blather and pompously pontificate some more: This is also 
why Linux could never be used in life-critical/safety-critical apps, 
like health care, machine control, automoitive, aviation, satellites,
or the Mars rover.  Code that controls life-critical apps has zillions 
of safety checks for situations that "can't possibly happen". But
every insider knows "can't happen" does happen: take the Hyabusa 
asteroid mission as a recent example.  My goal in pontificating is not 
to reject this one patch, but to change the cowboy attitude that makes 
people think that patches like this are OK.)

Thus, I can't begin to imagine why anyone would want to remove
robustness with a patch like this, and gain absolutely nothing in
return.


--linas

Re: Fault tolerance/bad patch, [was Re: [PATCH 29/30] [PATCH] PCI Hotplug: fake NULL pointer dereferences in IBM Hot Plug Controller Driver]

[Alan Cox]

Ar Gwe, 2006-06-23 am 10:04 -0500, ysgrifennodd Linas Vepstas:
> If someone in the future changes the hotplug core so that it 
> sometimes returns a null value, this code will potentially crash
> and/or do other bad things (corrupt, invalid state, etc.)
> This means that this routine will no longer be "robust" in the face of
> changes in other parts of the kernel. 

"Potentially".

But if you replaced it with

BUG_ON(value == NULL);

you'd both clean up the if and improve the reliability even more

> I can hear the objections:
> -- Performance. B.S. This routine is not performance critical, it will
>    get called once a week, once a month or less often; a few extra
>    cycles are utterly irrelevant.

(and half the time gcc eliminates the test itself)

Re: Fault tolerance/bad patch, [was Re: [PATCH 29/30] [PATCH] PCI Hotplug: fake NULL pointer dereferences in IBM Hot Plug Controller Driver]

[Eric Sesterhenn / Snakebyte]

* Alan Cox (alan@lxorguk.ukuu.org.uk) wrote:
> Ar Gwe, 2006-06-23 am 10:04 -0500, ysgrifennodd Linas Vepstas:
> > If someone in the future changes the hotplug core so that it 
> > sometimes returns a null value, this code will potentially crash
> > and/or do other bad things (corrupt, invalid state, etc.)
> > This means that this routine will no longer be "robust" in the face of
> > changes in other parts of the kernel. 
> 
> "Potentially".
> 
> But if you replaced it with
> 
> BUG_ON(value == NULL);
> 
> you'd both clean up the if and improve the reliability even more
> 
> > I can hear the objections:
> > -- Performance. B.S. This routine is not performance critical, it will
> >    get called once a week, once a month or less often; a few extra
> >    cycles are utterly irrelevant.
> 
> (and half the time gcc eliminates the test itself)
> 

I guess the BUG_ON makes more sense than keeping the
check, the reason coverity stumbled across this,
is the debug("get_attention_status - Exit rc[%d] value[%x]\n", rc,
*value); call some lines later, which uses the pointer.
If we just keep the check, we should also put one
around the debug statement

Greetings, Eric

Re: Fault tolerance/bad patch, [was Re: [PATCH 29/30] [PATCH] PCI Hotplug: fake NULL pointer dereferences in IBM Hot Plug Controller Driver]

[Greg KH]

On Fri, Jun 23, 2006 at 10:04:43AM -0500, Linas Vepstas wrote:
> Hi,
> 
> On Mon, Jun 19, 2006 at 02:43:35PM -0700, Greg KH wrote:
> > From: Eric Sesterhenn <snakebyte@gmx.de>
> > 
> > Remove checks for value, since the hotplug core always provides
> > a valid value.
> > 
> > -	if (hotplug_slot && value) {
> > +	if (hotplug_slot) {
> 
> This may be the wrong place to bring up a philosphical issue,

You are right, it is the wrong place for it, please take stuff like this
elsewhere.

value can not be a null value here, it's an impossiblity as that is how
this interface works.

thanks,

greg k-h