Re: mmap wrapping around to 0 revisited

Re: mmap wrapping around to 0 revisited

[David Ashley]

>It's not a bug that causes a system failure, just a feature that doesn't
>work correctly.  The only people I knew that were affected by this (which
>included me and why I would fix it) were the ones that had a user application
>flash programmer, and had to access the upper address space on PowerPCs.
>Since MTD is now working well, it is easier to just program/read flash with
>that "device", so I never need to do an mmap() like that anymore.
>
>The use of mmap() to physical addresses via user applications is a dangerous
>programming practice anyway.  It's a convenient hack for some quick testing,
>so if it mostly works in a controlled environment that's OK.  It's not the
>way you should be designing production systems (or the way I would do it).

I originally tried using the mtd and it worked once to program the flash
in the box. Then it wouldn't work anymore. We already had a flash utility
from another OS, so it was easy getting that to work reliably, using
mmap of /dev/mem.

XFree86 happily makes use of mmap of /dev/mem to do all its buffer
manipulation. I've seen drivers provided directly from the manufacturer
of IC's do the bulk of their coding within the user space program, and
just the interrupt handler in the kernel code. The case I'm thinking of
didn't do mmap of /dev/mem, but the kernel part did exactly what /dev/mem
does when the mmap function is called.

For testing mmap of /dev/mem has been invaluable in getting drivers going.
I can get something working in user state with little risk of crashing, then
when it is stable roll it into a kernel module.

>
>> .... Where does the fault lie? Who do I need to pass that fix on in
>> order to get it into the kernel? Who is the gatekeeper here keeping
>> the bugs in place?
>
>Relax :-).  This is supposed to be fun.  Everyone contributes, mistakes are
>made, others fix them, it's the beauty of working like this.  In the end
>we end up with something substantially better than any of could have on our
>own.  If you are trying to make a product, you need to understand the history
>of the development, take a snapshot that best meets your requirements,
>stabilize it, use it, and test it (or pay someone to do that).

It is fun, for example just the past few days I was able to write an audio
driver for our chip from the ground up. It is nice doing original coding again
for a while, rather than fixing the bugs that have already been fixed. There
is nothing fun about that, and when you finally figure out the bug after
days of concentrated effort and you find out someone says "Oh, I knew that,"
well, you kind of think something is broken in the system. And you're left with
a pretty hollow feeling when you hoped to have a good feeling about finding
a bug, and wanting to contribute your fix back to the linux community.

-Dave

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: mmap wrapping around to 0 revisited

[David Ashley]

>Wrong fix.  sys_mmap on ppc should really be using do_mmap which already
>includes the cast to unsigned long and checks for overflow.  Arguably,
>it could well check for -'ve offsets and reject them, but traditionally
>Linux has accepted up to 4GB offsets with its 32 bit APIs and changing
>this would break a few things like X.
>
>                -ben


In older versions (like 2.4.2-hhl) the sys_mmap did go through do_mmap, but
for some reason that was changed. The do_mmap itself is broken, the check
for overflow is like this:
	if ((offset + PAGE_ALIGN(len)) < offset)
		goto out;

It should be:
	if ((offset + PAGE_ALIGN(len)-1) < offset)
		goto out;

So: changing sys_mmap to go through do_mmap won't fix the problem unless
the above fix is done to do_mmap.

do_mmap appears to be defunct, and the new method seems to be more standard
across architectures. The problem was as I stated, and the fix I presented
is the best one.

-Dave

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: mmap wrapping around to 0 revisited

[Dan Malek]

David Ashley wrote:

> .... The do_mmap itself is broken, the check
> for overflow is like this:
> 	if ((offset + PAGE_ALIGN(len)) < offset)
> 		goto out;
>
> It should be:
> 	if ((offset + PAGE_ALIGN(len)-1) < offset)
> 		goto out;


This has been fixed and broken over and over more times than
anything else in the kernel.  Anytime someone makes a generic VM
change you can bet this will be broken again.  The only people that
seem to care are those that want to map the last 4K page at the top
of the physical address space from a user application using mmap().
There aren't many of those people :-).


	-- Dan


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

mmap wrapping around to 0 revisited

[David Ashley]

2.4.2-hhl had a problem with mmap right at the end of memory not working
because in a function do_mmap there was a check if
offset + PAGE_ALIGN(len) < offset
then fail.

I fixed it in that version by changing the comparison to
offset + PAGE_ALIGN(len)-1 < offset

The problem was offset + PAGE_ALIGN(len) ended up being zero due to wrapping
of the 32 bit value.

2.4.17 and probably lots of other kernels have the same mmap problem but
for a different reason. The new policy in the mmap is to not deal with the
byte offset but instead deal with the page offset, so the byte offset is
shifted right by usually 12 bits. This works fine on x86. On ppc the problem
is the offset is an off_t type, which isn't unsigned. So shifting it right
12 bits maintains the sign bit.

The fix is in
arch/ppc/kernel/syscalls.c
in the sys_mmap function, change this line:
	err = do_mmap2(addr, len, prot, flags, fd, offset >> PAGE_SHIFT);
to
	err = do_mmap2(addr, len, prot, flags, fd, (unsigned long)offset >> PAGE_SHIFT);

Possibly it would be better to have the argument as an unsigned long instead
of an off_t.

In our box there is a flashrom that goes right to the end of memory. We could
never mmap that last page properly to flash it.

-Dave

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: mmap wrapping around to 0 revisited

[Benjamin LaHaise]

On Mon, Mar 04, 2002 at 08:05:29AM -0800, David Ashley wrote:
> The fix is in
> arch/ppc/kernel/syscalls.c
> in the sys_mmap function, change this line:
> 	err = do_mmap2(addr, len, prot, flags, fd, offset >> PAGE_SHIFT);
> to
> 	err = do_mmap2(addr, len, prot, flags, fd, (unsigned long)offset >> PAGE_SHIFT);
>
> Possibly it would be better to have the argument as an unsigned long instead
> of an off_t.

Wrong fix.  sys_mmap on ppc should really be using do_mmap which already
includes the cast to unsigned long and checks for overflow.  Arguably,
it could well check for -'ve offsets and reject them, but traditionally
Linux has accepted up to 4GB offsets with its 32 bit APIs and changing
this would break a few things like X.

		-ben

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/