netdev - Re: DM9000 issue with mem resource management

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20080607213504.GA31817@trinity.fluff.org>
Date:	Sat, 7 Jun 2008 22:35:04 +0100
From:	Ben Dooks <ben-linux@...ff.org>, g@...nity.fluff.org
To:	Laurent Pinchart <laurentp@...-semaphore.com>
Cc:	Ben Dooks <ben-linux@...ff.org>, netdev@...r.kernel.org
Subject: Re: DM9000 issue with mem resource management

On Fri, Jun 06, 2008 at 11:01:42AM +0200, Laurent Pinchart wrote:
> Hi Ben,
> 
> On Thursday 05 June 2008 19:40, Ben Dooks wrote:
> > On Thu, Jun 05, 2008 at 05:42:58PM +0200, Laurent Pinchart wrote:
> > > Hi everybody,
> > > 
> > > I ran into a resource-related bug in the DM9000 driver.
> > > 
> > > When the platform device has only 2 resources, dm9000_probe() doesn't set 
> > > db->irq_res, which results in a segfault when the pointer gets
> > > dereferenced in dm9000_open().
> > > 
> > > I tried to fix the issue, and found out that the resource management code
> > > is quite broken.
> > 
> > Personally, I'm thinking about just removing the case for 2, and making it
> > three resources only. The following in-kernel machines use the following
> > resources:
> > 
> > arch/arm/mach-at91/board-sam9261ek.c			3
> > arch/arm/mach-pxa/cm-x270.c				3
> > arch/arm/mach-pxa/em-x270.c				3
> > arch/arm/mach-pxa/trizeps4.c				3
> > arch/arm/mach-pxa/colibri.c				3
> > arch/arm/mach-s3c2410/mach-bast.c			3
> > arch/arm/mach-s3c2410/mach-vr1000.c			3
> > arch/blackfin/mach-bf527/boards/ezkit.c			2
> > arch/blackfin/mach-bf533/boards/H8606.c			2
> > arch/blackfin/mach-bf533/boards/ip0x.c			3
> > arch/blackfin/mach-bf537/boards/generic_board.c		2
> > arch/blackfin/mach-bf537/boards/stamp.c			2
> > 
> > As you can see, the #3 outweigh the #2. Unless anyone else objects, I
> > will add a patch to reduce this to the case where the driver expects
> > 3 resources, and ask the users of #2 to submit changes for their
> > bots.
> >  
> > > If I understand things correctly, specifying 3 resources makes the DM9000 
> > > driver ioremap() the memory, while specifying 2 resources implies that the 
> > > platform code already ioremap()ed the memory. Is that right ?
> > > 
> > > If so, why does dm9000_probe() call request_mem_region() on ioremap()ed 
> > > memory ?
> > 
> > Hmm, dunno. I really have no idea why this happened. I don't think this
> > was my fault!
> >  
> > > Wouldn't it also be simpler to use release_mem_region() in 
> > > dm9000_release_board() instead of release_resource() + kfree() ?
> > 
> > If you already have the resource, this is a reasonably simple way of
> > ensuring you're disposing of the right resource.
> >  
> > > I'd be grateful if someone could confirm my assumptions. I'll then submit
> > > a patch to fix those issues.
> > 
> > My recommendation is that we remove the 2 case completely, so I'd not bother
> > trying to fix this.
> 
> I'm not too sure on that one.

Well, having thought about it some more, the 2 address case is also an
abuse of the IORESOURCE mechanism with platform devices, as the core
driver code uses the .start and .end to register with the relevant
iomem reservation code. I'm going more strongly on removing the #2 case
unless anyone can come up with a really compelling reason to keep it. 
 
> I have DM9000 chips on a custom bus. The bus code remaps the whole address 
> range (32 bytes per card x 32 slots) in one go and initializes the resource 
> with a subset of the remapped range.
>
> Converting that to per-slot ioremaps would create many more mappings. Is that 
> an issue (performance-wise, or in term of a maximum number of mappings if 
> there is a limit) ? I will still need a global mapping, as the bus code needs 
> to access configuration registers on the individual cards, so I won't be able 
> to request_mem_region both the global mapping and the individual mappings. 
> I'm not sure if that's an issue.

That sounds like a problem with the bus code mapping too much of the memory
and thus causing a problem for any subsequent drivers. Having an extra
mapping in is only a minor problem compared with the other travesty the
2 region case is currently causing.

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html