* Fault tolerance/bad patch, [was Re: [PATCH 29/30] [PATCH] PCI Hotplug: fake NULL pointer dereferences in IBM Hot Plug Controller Driver] [not found] ` <11507535123764-git-send-email-greg@kroah.com> @ 2006-06-23 15:04 ` Linas Vepstas 2006-06-23 15:28 ` Alan Cox 2006-06-23 18:33 ` Greg KH 0 siblings, 2 replies; 4+ messages in thread From: Linas Vepstas @ 2006-06-23 15:04 UTC (permalink / raw) To: Greg KH; +Cc: linux-pci, Eric Sesterhenn, Greg Kroah-Hartman, linux-kernel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Fault tolerance/bad patch, [was Re: [PATCH 29/30] [PATCH] PCI Hotplug: fake NULL pointer dereferences in IBM Hot Plug Controller Driver] 2006-06-23 15:04 ` Fault tolerance/bad patch, [was Re: [PATCH 29/30] [PATCH] PCI Hotplug: fake NULL pointer dereferences in IBM Hot Plug Controller Driver] Linas Vepstas @ 2006-06-23 15:28 ` Alan Cox 2006-06-23 15:52 ` Eric Sesterhenn / Snakebyte 2006-06-23 18:33 ` Greg KH 1 sibling, 1 reply; 4+ messages in thread From: Alan Cox @ 2006-06-23 15:28 UTC (permalink / raw) To: Linas Vepstas Cc: Greg KH, linux-pci, Eric Sesterhenn, Greg Kroah-Hartman, linux-kernel 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) ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Fault tolerance/bad patch, [was Re: [PATCH 29/30] [PATCH] PCI Hotplug: fake NULL pointer dereferences in IBM Hot Plug Controller Driver] 2006-06-23 15:28 ` Alan Cox @ 2006-06-23 15:52 ` Eric Sesterhenn / Snakebyte 0 siblings, 0 replies; 4+ messages in thread From: Eric Sesterhenn / Snakebyte @ 2006-06-23 15:52 UTC (permalink / raw) To: Alan Cox Cc: Linas Vepstas, Greg KH, linux-pci, Eric Sesterhenn, Greg Kroah-Hartman, linux-kernel * 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Fault tolerance/bad patch, [was Re: [PATCH 29/30] [PATCH] PCI Hotplug: fake NULL pointer dereferences in IBM Hot Plug Controller Driver] 2006-06-23 15:04 ` Fault tolerance/bad patch, [was Re: [PATCH 29/30] [PATCH] PCI Hotplug: fake NULL pointer dereferences in IBM Hot Plug Controller Driver] Linas Vepstas 2006-06-23 15:28 ` Alan Cox @ 2006-06-23 18:33 ` Greg KH 1 sibling, 0 replies; 4+ messages in thread From: Greg KH @ 2006-06-23 18:33 UTC (permalink / raw) To: Linas Vepstas; +Cc: Greg KH, linux-pci, Eric Sesterhenn, linux-kernel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2006-06-23 18:36 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1150753481625-git-send-email-greg@kroah.com>
[not found] ` <115075348565-git-send-email-greg@kroah.com>
[not found] ` <11507534883521-git-send-email-greg@kroah.com>
[not found] ` <11507534914002-git-send-email-greg@kroah.com>
[not found] ` <11507534953044-git-send-email-greg@kroah.com>
[not found] ` <11507534983982-git-send-email-greg@kroah.com>
[not found] ` <11507535021937-git-send-email-greg@kroah.com>
[not found] ` <11507535054091-git-send-email-greg@kroah.com>
[not found] ` <11507535082418-git-send-email-greg@kroah.com>
[not found] ` <11507535123764-git-send-email-greg@kroah.com>
2006-06-23 15:04 ` Fault tolerance/bad patch, [was Re: [PATCH 29/30] [PATCH] PCI Hotplug: fake NULL pointer dereferences in IBM Hot Plug Controller Driver] Linas Vepstas
2006-06-23 15:28 ` Alan Cox
2006-06-23 15:52 ` Eric Sesterhenn / Snakebyte
2006-06-23 18:33 ` Greg KH
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox