linuxsh-dev

[PATCH] pata_platform: Fix NULL pointer dereference

[Magnus D. <mag...@gm...>]

pata_platform: Fix NULL pointer dereference

The platform-specific structures may leave pdev->dev.platform_data as NULL.

Signed-off-by: Magnus Damm <da...@ig...>
---

 Tested on a Solution Engline 7722
 Applies to git linux-2.6 a5fcaa210626a79465321e344c91a6a7dc3881fa

 drivers/ata/pata_platform.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

--- 0001/drivers/ata/pata_platform.c
+++ work/drivers/ata/pata_platform.c	2007-07-17 15:01:09.000000000 +0900
@@ -213,8 +213,9 @@ static int __devinit pata_platform_probe
 	pata_platform_setup_port(&ap->ioaddr, pp_info);
 
 	/* activate */
-	return ata_host_activate(host, platform_get_irq(pdev, 0), ata_interrupt,
-				 pp_info->irq_flags, &pata_platform_sht);
+	return ata_host_activate(host, platform_get_irq(pdev, 0), 
+				 ata_interrupt, pp_info ? pp_info->irq_flags
+				 : 0, &pata_platform_sht);
 }
 
 /**

Re: [PATCH] pata_platform: Fix NULL pointer dereference

[Paul M. <le...@li...>]

On Tue, Jul 17, 2007 at 05:42:52PM +0900, Magnus Damm wrote:
> pata_platform: Fix NULL pointer dereference
> 
> The platform-specific structures may leave pdev->dev.platform_data as NULL.
> 
> Signed-off-by: Magnus Damm <da...@ig...>

Looks like the change that broke this came in the libata merge, more
specifically this change:

commit 5f45bc50976ee1f408f7171af155aec646655a37
Author: Sonic Zhang <son...@gm...>
Date:   Fri Jun 15 17:45:49 2007 +0800

    Add irq_flags to struct pata_platform_info

    On some embedded platforms, such as blackfin, the gpio interrupt for
    IDE interface is designed to be triggered with high voltage. The gpio
    port should be configured properly by set_irq_type() when register
    the irq. This patch enable the generic pata platform driver to
    accept platform irq flags data.

    Signed-off-by: Sonic Zhang <son...@gm...>
    Signed-off-by: Jeff Garzik <je...@ga...>

It would be great if libata people could actually be bothered to CC driver
authors on driver changes so that way these things don't get completely broken
in mainline when simple testing on the platforms that actually _rely_ on this
driver would have shown that this was broken.

I don't intend to poll the various libata branches on the off chance I'll find
something that's hosed my platforms, and I don't think it's an unreasonable
expectation to at least have to see the patch and sign off on it before it gets
merged. Every other subsystem works this way, I wonder why libata feels that
it's special.

This is also not the first time changes to pata_platform have caused fallout,
without finding out about it until after the changes were already merged.

Re: [PATCH] pata_platform: Fix NULL pointer dereference

[Jeff G. <je...@ga...>]

Paul Mundt wrote:
> On Tue, Jul 17, 2007 at 05:42:52PM +0900, Magnus Damm wrote:
>> pata_platform: Fix NULL pointer dereference
>>
>> The platform-specific structures may leave pdev->dev.platform_data as NULL.
>>
>> Signed-off-by: Magnus Damm <da...@ig...>
> 
> Looks like the change that broke this came in the libata merge, more
> specifically this change:
> 
> commit 5f45bc50976ee1f408f7171af155aec646655a37
> Author: Sonic Zhang <son...@gm...>
> Date:   Fri Jun 15 17:45:49 2007 +0800
> 
>     Add irq_flags to struct pata_platform_info
> 
>     On some embedded platforms, such as blackfin, the gpio interrupt for
>     IDE interface is designed to be triggered with high voltage. The gpio
>     port should be configured properly by set_irq_type() when register
>     the irq. This patch enable the generic pata platform driver to
>     accept platform irq flags data.
> 
>     Signed-off-by: Sonic Zhang <son...@gm...>
>     Signed-off-by: Jeff Garzik <je...@ga...>
> 
> It would be great if libata people could actually be bothered to CC driver
> authors on driver changes so that way these things don't get completely broken
> in mainline when simple testing on the platforms that actually _rely_ on this
> driver would have shown that this was broken.
> 
> I don't intend to poll the various libata branches on the off chance I'll find
> something that's hosed my platforms, and I don't think it's an unreasonable
> expectation to at least have to see the patch and sign off on it before it gets
> merged. Every other subsystem works this way, I wonder why libata feels that
> it's special.
> 
> This is also not the first time changes to pata_platform have caused fallout,
> without finding out about it until after the changes were already merged.

The patch lived for weeks in -mm along with tons of other git trees 
Andrew pulls.  If you want a single point to watch for upcoming stuff, 
test the -mm trees.  It's a key part of the development process:  I 
merge a patch, -mm tree pulls my tree and others, people test and 
complain and give feedback, ...

With embedded stuff its tough to judge whether people are knowledgeable 
about the impact of the change, and sometimes I judge wrongly.  My 
apologies.

What is your suggestion for moving linux-2.6.git forward?

	Jeff

Re: [PATCH] pata_platform: Fix NULL pointer dereference

[Magnus D. <mag...@gm...>]

On 7/17/07, Jeff Garzik <je...@ga...> wrote:
> Paul Mundt wrote:
> > On Tue, Jul 17, 2007 at 05:42:52PM +0900, Magnus Damm wrote:
> >> pata_platform: Fix NULL pointer dereference
> >>
> >> The platform-specific structures may leave pdev->dev.platform_data as NULL.
> >>
> >> Signed-off-by: Magnus Damm <da...@ig...>
>
> What is your suggestion for moving linux-2.6.git forward?

This fix solves the problem for me and Paul seems to be happy with it as well.
Jeff, can you please pick up this patch so we can unbreak things for rc1?

Thanks!

/ magnus

Re: [PATCH] pata_platform: Fix NULL pointer dereference

[Paul M. <le...@li...>]

On Tue, Jul 17, 2007 at 05:33:00AM -0400, Jeff Garzik wrote:
> Paul Mundt wrote:
> >It would be great if libata people could actually be bothered to CC
> >driver authors on driver changes so that way these things don't get
> >completely broken in mainline when simple testing on the platforms
> >that actually _rely_ on this driver would have shown that this was
> >broken.
> 
> The patch lived for weeks in -mm along with tons of other git trees 
> Andrew pulls.  If you want a single point to watch for upcoming stuff, 
> test the -mm trees.  It's a key part of the development process:  I 
> merge a patch, -mm tree pulls my tree and others, people test and 
> complain and give feedback, ...
> 
A key part of the development process is making sure that driver authors
are aware of the changes being made to their drivers, so they're able to
ACK/NACK or at least point out something that's obviously wrong. I test
-mm when time permits, and this happened to slip through. If I had
actually been CC'ed on it, it would not have.

Your entire process is fundamentally flawed if you're merging the patch
first and expecting people to only find out if it's broken after the
fact. In the best case it leaves -mm broken for a single release, and in
the other case, it happens to make its way to mainline before anyone
notices.

> With embedded stuff its tough to judge whether people are knowledgeable 
> about the impact of the change, and sometimes I judge wrongly.  My 
> apologies.
> 
Another reason to make sure the authors are CC'ed. I really don't see why
this is so difficult for you, _everyone_ else manages to do this
properly, and shis sort of case illustrates _exactly_ why.

> What is your suggestion for moving linux-2.6.git forward?
> 
The patch from Magnus is fine with me for fixing this particular problem,
so at least getting that merged quickly will get the sh and ppc platforms
fixed again.

To avoid having this happen again in the future, I'd appreciate being
CC'ed on things that impact pata_platform pre-merge, and again, I don't
see this as an extraordinarly unreasonable request. If you can't be
bothered doing that, then just stop applying pata_platform patches, and
I'll merge them myself.

Re: [PATCH] pata_platform: Fix NULL pointer dereference

[Jeff G. <je...@ga...>]

Paul Mundt wrote:
> On Tue, Jul 17, 2007 at 05:33:00AM -0400, Jeff Garzik wrote:
>> Paul Mundt wrote:
>>> It would be great if libata people could actually be bothered to CC
>>> driver authors on driver changes so that way these things don't get
>>> completely broken in mainline when simple testing on the platforms
>>> that actually _rely_ on this driver would have shown that this was
>>> broken.
>> The patch lived for weeks in -mm along with tons of other git trees 
>> Andrew pulls.  If you want a single point to watch for upcoming stuff, 
>> test the -mm trees.  It's a key part of the development process:  I 
>> merge a patch, -mm tree pulls my tree and others, people test and 
>> complain and give feedback, ...
>>
> A key part of the development process is making sure that driver authors
> are aware of the changes being made to their drivers, so they're able to
> ACK/NACK or at least point out something that's obviously wrong. I test
> -mm when time permits, and this happened to slip through. If I had
> actually been CC'ed on it, it would not have.
> 
> Your entire process is fundamentally flawed if you're merging the patch
> first and expecting people to only find out if it's broken after the
> fact. In the best case it leaves -mm broken for a single release, and in
> the other case, it happens to make its way to mainline before anyone
> notices.

It's just reality that there are never enough people to verify every 
patch before it goes in.  We just support way too much hardware for that 
to be remotely feasible.  Sure we -try- to keep the driver maintainer 
CC'd, but alas we are human.

Your thinking is flawed if you think I'm going to hold up every patch 
until it's verified on each of 63 ATA controller drivers, when I make a 
core change, for example.  Not scalable.  We scale in another way:

We have the biggest test lab in the world -- the Internet -- and a 
development process staged so that testing and development occur in 
parallel.  This here is actually an example of the process working: 
despite my forgetting to CC you, the problem was caught long before it 
made it into any release kernel.

	Jeff

Re: [PATCH] pata_platform: Fix NULL pointer dereference

[Paul M. <le...@li...>]

On Tue, Jul 17, 2007 at 06:24:53AM -0400, Jeff Garzik wrote:
> It's just reality that there are never enough people to verify every 
> patch before it goes in.  We just support way too much hardware for that 
> to be remotely feasible.  Sure we -try- to keep the driver maintainer 
> CC'd, but alas we are human.
> 
Try? I've never _once_ received a CC from you regarding a change to
pata_platform. The only time I've been CC'ed at all is when someone
submitting the patch had the common courtesy to do so.

Whereas the number of times I've had to clean up after something merged
behind my back has been higher than the number of times I've been CC'ed
on any of these changes.

> Your thinking is flawed if you think I'm going to hold up every patch 
> until it's verified on each of 63 ATA controller drivers, when I make a 
> core change, for example.  Not scalable.  We scale in another way:
> 
This has nothing to do with core changes that impact every driver, this
has to do with isolated changes to a _single_ driver that you're
obviously not in a position to test. That's a very different situation.

Again, if you can't be bothered to CC people on changes to their drivers
that aren't libata-wide, don't apply them in the first place. I would
much rather play API catch up with my driver than have to backtrack and
figure out what went wrong when suddenly all of my boards stop booting.

Your "merge first and hope someone else notices and fixes it later"
approach is insane. We've never done kernel development like that, libata
isn't special.