Possible MTD bug in 2.6.15

Possible MTD bug in 2.6.15

[Jim Ramsay]

[-- Attachment #1: Type: text/plain, Size: 1678 bytes --]

We have an interesting problem with MTD and a flash chip on an
embedded board.  The problem stems from the fact that due to hardware
constraints we can only access up to 32M of address space on an
attached flash device.  However, the actual part attached to the board
is 64M.  Yes, I know this is not likely to happen, but it points at a
kernel bug which will happen if you ever specify a MTD map->size which
is less than the actual size of the CFI flash chip.

When we specify the map->size as 32M (0x02000000) and do the CFI
probe, the chip is properly detected, but then in gen_probe.c the
following happens:

- genprobe_ident_chips is run
  - It sets cfi.chipshift based on the cfi.cfiq->DevSize, which gets
properly set to 0x1a (64M flash chip).
  - It then sets the local "max_chips" variable by shifting down
map->size by this chipshift, which shifts our size (0x02000000 = 32M)
down all the way to 0.
  - Since 'max_chips' is zero, no memory is allocated for this chip,
and the waitqueue is not initialized.  The will cause a kernel panic
later, if you ever try to read from this chip.

The routine completes and you are left with a seemingly valid MTD
device.  However, if you ever try to read or write this device, the
waitqueue is uninitialized, which causes a nasty kernel panic.

My proposed fix is attached (a patch against 2.6.15).  After shifting
the map->size down by cfi.chipshift, I just ensure that max_chips is
at least one.  Does this seem like a reasonable fix?

Note: Please CC my email address in reply, as I am not currently
subscribed to the linux-kernel list.

--
Jim Ramsay
"Me fail English?  That's unpossible!"

[-- Attachment #2: mtd_wrong_size_bug.patch --]
[-- Type: application/octet-stream, Size: 833 bytes --]

Index: drivers/mtd/chips/gen_probe.c
===================================================================
RCS file: /cvs/PM35_35_14_01/linux_2_6/drivers/mtd/chips/gen_probe.c,v
retrieving revision 1.1.1.3
diff -u -u -r1.1.1.3 gen_probe.c
--- drivers/mtd/chips/gen_probe.c	10 Jan 2006 00:44:25 -0000	1.1.1.3
+++ drivers/mtd/chips/gen_probe.c	19 Apr 2006 16:32:10 -0000
@@ -100,6 +100,13 @@
 	 * Align bitmap storage size to full byte.
 	 */
 	max_chips = map->size >> cfi.chipshift;
+	// If we shift down to 0, assume there is at least one chip here
+	if( max_chips == 0 )
+	{
+		printk( KERN_WARNING "%s: map->size as specified is less than "
+				     "actual chip size\n", map->name );
+		max_chips = 1;
+	}
 	mapsize = (max_chips / 8) + ((max_chips % 8) ? 1 : 0);
 	chip_map = kmalloc(mapsize, GFP_KERNEL);
 	if (!chip_map) {

Possible MTD bug in 2.6.15

[Thiago Galesi]

Ok, a couple of comments/questions

1 - Wouldn't it be better to map all flash, and leave the unneeded
part as read only?

2 - Please follow  Documentation/SubmittingPatches format for sending
patches (especially the signed-off part and sending patches inline)

3 - No C++ style comments, please

Thiago


On 4/19/06, Jim Ramsay <kernel@jimramsay.com> wrote:
> We have an interesting problem with MTD and a flash chip on an
> embedded board.  The problem stems from the fact that due to hardware
> constraints we can only access up to 32M of address space on an
> attached flash device.  However, the actual part attached to the board
> is 64M.  Yes, I know this is not likely to happen, but it points at a
> kernel bug which will happen if you ever specify a MTD map->size which
> is less than the actual size of the CFI flash chip.
>
> When we specify the map->size as 32M (0x02000000) and do the CFI
> probe, the chip is properly detected, but then in gen_probe.c the
> following happens:
>
> - genprobe_ident_chips is run
>   - It sets cfi.chipshift based on the cfi.cfiq->DevSize, which gets
> properly set to 0x1a (64M flash chip).
>   - It then sets the local "max_chips" variable by shifting down
> map->size by this chipshift, which shifts our size (0x02000000 = 32M)
> down all the way to 0.
>   - Since 'max_chips' is zero, no memory is allocated for this chip,
> and the waitqueue is not initialized.  The will cause a kernel panic
> later, if you ever try to read from this chip.
>
> The routine completes and you are left with a seemingly valid MTD
> device.  However, if you ever try to read or write this device, the
> waitqueue is uninitialized, which causes a nasty kernel panic.
>
> My proposed fix is attached (a patch against 2.6.15).  After shifting
> the map->size down by cfi.chipshift, I just ensure that max_chips is
> at least one.  Does this seem like a reasonable fix?
>
> Note: Please CC my email address in reply, as I am not currently
> subscribed to the linux-kernel list.
>
> --
> Jim Ramsay
> "Me fail English?  That's unpossible!"
>
>
>

Re: Possible MTD bug in 2.6.15

[Pekka Enberg]

On 4/19/06, Thiago Galesi <thiagogalesi@gmail.com> wrote:
> Ok, a couple of comments/questions
>
> 1 - Wouldn't it be better to map all flash, and leave the unneeded
> part as read only?
>
> 2 - Please follow  Documentation/SubmittingPatches format for sending
> patches (especially the signed-off part and sending patches inline)
>
> 3 - No C++ style comments, please

4 - Read Documentation/CodingStyle before resubmitting the patch.

                                     Pekka

Re: Possible MTD bug in 2.6.15

[David Woodhouse]

On Wed, 2006-04-19 at 10:49 -0600, Jim Ramsay wrote:
> We have an interesting problem with MTD and a flash chip on an
> embedded board.  The problem stems from the fact that due to hardware
> constraints we can only access up to 32M of address space on an
> attached flash device.  However, the actual part attached to the board
> is 64M.  Yes, I know this is not likely to happen, but it points at a
> kernel bug which will happen if you ever specify a MTD map->size which
> is less than the actual size of the CFI flash chip. 

Please could you confirm this is fixed in the current MTD git tree (and
in the current -mm kernel). 

-- 
dwmw2

Re: Possible MTD bug in 2.6.15

[Jörn Engel]

On Sat, 22 April 2006 21:48:28 -0600, Jim Ramsay wrote:
> 
> And really, it's not a big stretch from what the code currently does
> to what my patch changes.  At this point in the code, we know for a
> fact that we already have at least one flash chip.  The math that's
> going on here with the 'max_chips' variable is to check if there is
> actually more than one physical chip implementing the entire reported
> size.  The only mistake is that the math goes too far, shifting the
> count down to zero if the reported size is too small.  This
> 'max_chips' should never be allowed to be lower than 1, because we
> really do know that there is at least one flash chip.

In a setup like yours, someone should go and educate hardware
developers.

Anyway, if you add a CONFIG_BROKEN_HARDWARE of some sorts to your
patch and send it to the proper list (linux-mtd@lists.infradead.org),
I don't have a problem with your patch.

Jörn

-- 
To announce that there must be no criticism of the President, or that we
are to stand by the President, right or wrong, is not only unpatriotic
and servile, but is morally treasonable to the American public.
-- Theodore Roosevelt, Kansas City Star, 1918

Re: Possible MTD bug in 2.6.15

[Jim Ramsay]

On 22/04/06, Thiago Galesi <thiagogalesi@gmail.com> wrote:
> > > > Ok, a couple of comments/questions
> > > >
> > > > 1 - Wouldn't it be better to map all flash, and leave the unneeded
> > > > part as read only?
> >
> > In general, yes.  But this should either be enforced somewhere nicer
> > (ie, die gracefully) so the kernel doesn't panic later, or be allowed
> > as in my patch.
>
> The fundamental problem there seems to be a mismatch between what is
> set by the user and what is read from the flash chip. As you mention
> in your first message, (what it came across is that) you don't have
> (physical / electrical) access to all the flash; not something I would
> recommend (that is, having limited electrical connection to the flash)

Yes, that is exactly what is going on - we have only have electrical
access to 32M of addresses regardless of the size of the actual chip
installed, and unfortunately I can't change that.  However, it's not
really a problem from the electrical side of things - We can still
access the lowest 32M on the chip, just the highest address pin isn't
connected to anything.

> As for the options you propose - enforce and die gracefully (that is,
> if there is a size mismatch, warning and purposely not working) seems
> more correct than the second option.

I don't see why - The size mismatch doesn't prevent the flash chip
from functioning.  Isn't it better to help things work in more
circumstances rather than in less?

And really, it's not a big stretch from what the code currently does
to what my patch changes.  At this point in the code, we know for a
fact that we already have at least one flash chip.  The math that's
going on here with the 'max_chips' variable is to check if there is
actually more than one physical chip implementing the entire reported
size.  The only mistake is that the math goes too far, shifting the
count down to zero if the reported size is too small.  This
'max_chips' should never be allowed to be lower than 1, because we
really do know that there is at least one flash chip.

I agree with you that it's probably "more correct" to actually specify
the real size of the chip in the 'map' struct before the CFI probe,
but I can't think of any reason why specifying a smaller size should
die (even gracefully) when it still works just fine.

Of course, maybe there are some models of flash chips where this
wouldn't work and a graceful prevention of this would be important. 
But with our CFI-compliant chip, we don't see any problem with my
patch applied.

--
Jim Ramsay
"Me fail English?  That's unpossible!"

Possible MTD bug in 2.6.15

[Thiago Galesi]

> > > Ok, a couple of comments/questions
> > >
> > > 1 - Wouldn't it be better to map all flash, and leave the unneeded
> > > part as read only?
>
> In general, yes.  But this should either be enforced somewhere nicer
> (ie, die gracefully) so the kernel doesn't panic later, or be allowed
> as in my patch.

The fundamental problem there seems to be a mismatch between what is
set by the user and what is read from the flash chip. As you mention
in your first message, (what it came across is that) you don't have
(physical / electrical) access to all the flash; not something I would
recommend (that is, having limited electrical connection to the flash)

As for the options you propose - enforce and die gracefully (that is,
if there is a size mismatch, warning and purposely not working) seems
more correct than the second option.