do_mmap

do_mmap

[Thomas 'Dent' Mirlacher]

hi list,

what about the do_mmap/do_mmap_pgoff implementation?
reurn-type: _unsigned_ long	(which should be ok cause we've to return
				 an adress if len == 0)
on error: -ERR_*

and the checks in various places are really strange. - well some
places check for:
	o != NULL
	o > -1024UL
	o ...

guess this nedds some cleanup.

is it possible to have 0 as a valid address? - if not, this should
be the return on errors.

any comments?

	tm

-- 
in some way i do, and in some way i don't.

Re: do_mmap

[Alan Cox]

On Fri, 2002-05-31 at 14:00, Thomas 'Dent' Mirlacher wrote:
> and the checks in various places are really strange. - well some
> places check for:
> 	o != NULL
> 	o > -1024UL

"Not an error". Its relying as some other bits of code do actually that
the top mappable user address is never in the top 1K of the address
space

> is it possible to have 0 as a valid address? - if not, this should
> be the return on errors.

SuS explicitly says that 0 is not a valid mmap return address.

Re: do_mmap

[Thomas 'Dent' Mirlacher]

alan,

> On Fri, 2002-05-31 at 14:00, Thomas 'Dent' Mirlacher wrote:
> > and the checks in various places are really strange. - well some
> > places check for:
> > 	o != NULL
> > 	o > -1024UL
> 
> "Not an error". Its relying as some other bits of code do actually that
> the top mappable user address is never in the top 1K of the address
> space

ok, that explain the -1024UL

> > is it possible to have 0 as a valid address? - if not, this should
> > be the return on errors.
> 
> SuS explicitly says that 0 is not a valid mmap return address.

ok.

so it seems the code itself is correct. it's just a little bit odd
to read over the code, returning an unsigned int, and then find
no comments on this "not so common usage" ;)

nevertheless, functions which just check for != NULL for the return
type needs fixing. - plus s hort comment containing your explaination
above could help other people ...

	tm

-- 
in some way i do, and in some way i don't.

Re: do_mmap

[Linus Torvalds]

In article <1022855243.12888.410.camel@irongate.swansea.linux.org.uk>,
Alan Cox  <alan@lxorguk.ukuu.org.uk> wrote:
>On Fri, 2002-05-31 at 14:00, Thomas 'Dent' Mirlacher wrote:
>> and the checks in various places are really strange. - well some
>> places check for:
>> 	o != NULL
>> 	o > -1024UL
>
>"Not an error". Its relying as some other bits of code do actually that
>the top mappable user address is never in the top 1K of the address
>space
>
>> is it possible to have 0 as a valid address? - if not, this should
>> be the return on errors.
>
>SuS explicitly says that 0 is not a valid mmap return address.

But if so, SuS is _very_ _very_ wrong.

The fact is, if you use something like vm86 mode, you absolutely _need_
to be able to explicitly mmap at address 0. 

So it is correct (and in fact there is no other sane way to do it) to
say

	addr = mmap(NULL, 1024*1024,
		PROT_READ | PROT_WRITE ,
		MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED,
		-1, 0);

and if SuS says that mmap must not return NULL for this case, then SuS
is so full of sh*t that it's not worth worrying about.

In short, under Linux 0 _is_, and will always be (at least on x86) a
perfectly valid return address from mmap() and friends. It's only going
to be returned when you explicitly ask for it with MAP_FIXED, but it
absolutely is a valid return.

		Linus
		Linus

Re: do_mmap

[Thomas 'Dent' Mirlacher]

--snip/snip

> >> is it possible to have 0 as a valid address? - if not, this should
> >> be the return on errors.
> >
> >SuS explicitly says that 0 is not a valid mmap return address.
> 
> But if so, SuS is _very_ _very_ wrong.
> 
> The fact is, if you use something like vm86 mode, you absolutely _need_
> to be able to explicitly mmap at address 0. 
> 
> So it is correct (and in fact there is no other sane way to do it) to
> say
> 
> 	addr = mmap(NULL, 1024*1024,
> 		PROT_READ | PROT_WRITE ,
> 		MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED,
> 		-1, 0);
> 
> and if SuS says that mmap must not return NULL for this case, then SuS
> is so full of sh*t that it's not worth worrying about.
> 
> In short, under Linux 0 _is_, and will always be (at least on x86) a
> perfectly valid return address from mmap() and friends. It's only going
> to be returned when you explicitly ask for it with MAP_FIXED, but it
> absolutely is a valid return.

ok. so 0 or (NULL) is not an option, and also unnneccessary once someone
know how the error retun is used. - wouldn't it be much more cleaner
to convert this _ugly_ unsigned long vals into void * wherever these vals
are carrying an address? (well at least for do_mmap*) and use ERR_PTR
for returning, and IS_ERR for checking for an error?

btw, is err should (according to alans explaination be):

	return (unsigned long)ptr > (unsigned long)-1024UL;

	tm

-- 
in some way i do, and in some way i don't.

Re: do_mmap

[Linus Torvalds]

On Fri, 31 May 2002, Thomas 'Dent' Mirlacher wrote:
> 
> ok. so 0 or (NULL) is not an option, and also unnneccessary once someone
> know how the error retun is used. - wouldn't it be much more cleaner
> to convert this _ugly_ unsigned long vals into void * wherever these vals
> are carrying an address? (well at least for do_mmap*) and use ERR_PTR
> for returning, and IS_ERR for checking for an error?

Using ERR_PTR/PTR_ERR/IS_ERR is probably the correct thing to do, but 
that codebase predates the "error pointer" macros by about 9 years ;)

> btw, is err should (according to alans explaination be):
> 
> 	return (unsigned long)ptr > (unsigned long)-1024UL;

The 1000 vs 1024 thing is just another "random number". I don't know what
the right number is myself, it may be worth trying to pick one that is
uniformly easy to test against, for example. Many architectures are better
at some constants than others.

		Linus

Re: do_mmap

[Richard B. Johnson]

On Fri, 31 May 2002, Thomas 'Dent' Mirlacher wrote:

> --snip/snip
> 
> > >> is it possible to have 0 as a valid address? - if not, this should
> > >> be the return on errors.
> > >
> > >SuS explicitly says that 0 is not a valid mmap return address.
> > 
> > But if so, SuS is _very_ _very_ wrong.
> > 
> > The fact is, if you use something like vm86 mode, you absolutely _need_
> > to be able to explicitly mmap at address 0. 
> > 
> > So it is correct (and in fact there is no other sane way to do it) to
> > say
> > 
> > 	addr = mmap(NULL, 1024*1024,
> > 		PROT_READ | PROT_WRITE ,
> > 		MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED,
> > 		-1, 0);
> > 
> > and if SuS says that mmap must not return NULL for this case, then SuS
> > is so full of sh*t that it's not worth worrying about.
> > 
> > In short, under Linux 0 _is_, and will always be (at least on x86) a
> > perfectly valid return address from mmap() and friends. It's only going
> > to be returned when you explicitly ask for it with MAP_FIXED, but it
> > absolutely is a valid return.
> 
> ok. so 0 or (NULL) is not an option, and also unnneccessary once someone
> know how the error retun is used. - wouldn't it be much more cleaner
> to convert this _ugly_ unsigned long vals into void * wherever these vals
> are carrying an address? (well at least for do_mmap*) and use ERR_PTR
> for returning, and IS_ERR for checking for an error?
> 
> btw, is err should (according to alans explaination be):
> 
> 	return (unsigned long)ptr > (unsigned long)-1024UL;
> 
> 	tm
> 

At the user-mode API, we get to (void *) -1, defined in sys/mman.h
(actually (__ptr_t) -1); so whatever you do, the 'C' runtime library
has to 'know' about your return values if this propagates to
sys-calls.


Cheers,
Dick Johnson

Penguin : Linux version 2.4.18 on an i686 machine (797.90 BogoMips).

                 Windows-2000/Professional isn't.

Re: do_mmap

[Thomas 'Dent' Mirlacher]

Dick,

--snip/snip


> > btw, is err should (according to alans explaination be):
> > 
> > 	return (unsigned long)ptr > (unsigned long)-1024UL;
> > 
> > 	tm
> > 
> 
> At the user-mode API, we get to (void *) -1, defined in sys/mman.h
> (actually (__ptr_t) -1); so whatever you do, the 'C' runtime library
> has to 'know' about your return values if this propagates to
> sys-calls.

the code right now, will pass all the errors through to the user
space in any case (beside a handful internal kernel-functions).

by changing unsigned long to void * everything should stay the same
(at least for todays architectures) - well if i'm wrong, please
enlighten me :)

also using IS_ERR is essentially the same as the other approaches
to check for errors (beside the check for == 0).

this means by "cleaning up" the internal functions, _nothing_ should
me impacted, even if the changes are step by step, function by function,
beside some gcc warnings (the well known: "assignment makes pointer from
integer without a cast").

cheers,

	tm

-- 
in some way i do, and in some way i don't.

Re: do_mmap

[Richard B. Johnson]

On Fri, 31 May 2002, Thomas 'Dent' Mirlacher wrote:

> Dick,
> 
> --snip/snip
> 
> 
> > > btw, is err should (according to alans explaination be):
> > > 
> > > 	return (unsigned long)ptr > (unsigned long)-1024UL;
> > > 
> > > 	tm
> > > 
> > 
> > At the user-mode API, we get to (void *) -1, defined in sys/mman.h
> > (actually (__ptr_t) -1); so whatever you do, the 'C' runtime library
> > has to 'know' about your return values if this propagates to
> > sys-calls.
> 
> the code right now, will pass all the errors through to the user
> space in any case (beside a handful internal kernel-functions).
> 
> by changing unsigned long to void * everything should stay the same
> (at least for todays architectures) - well if i'm wrong, please
> enlighten me :)
> 
> also using IS_ERR is essentially the same as the other approaches
> to check for errors (beside the check for == 0).
> 
> this means by "cleaning up" the internal functions, _nothing_ should
> me impacted, even if the changes are step by step, function by function,
> beside some gcc warnings (the well known: "assignment makes pointer from
> integer without a cast").
> 
> cheers,
> 
> 	tm
> 

Good. It was just a 'sanity-check' as these things caught my
eye. Because I have to fix a lot of junk code that others have
written (here at work), as they become Peter-principled to
higher-level positions, I get sensitized to these things.
No complaint -- I like fixing junk code!
The previously line was written for Network Security Administrators
(Hello Thor).

Cheers,
Dick Johnson

Penguin : Linux version 2.4.18 on an i686 machine (797.90 BogoMips).

                 Windows-2000/Professional isn't.

Re: do_mmap

[Alan Cox]

On Fri, 2002-05-31 at 18:30, Linus Torvalds wrote:
> >> is it possible to have 0 as a valid address? - if not, this should
> >> be the return on errors.
> >
> >SuS explicitly says that 0 is not a valid mmap return address.
> 
> But if so, SuS is _very_ _very_ wrong.
> 
> The fact is, if you use something like vm86 mode, you absolutely _need_
> to be able to explicitly mmap at address 0. 
> 
> So it is correct (and in fact there is no other sane way to do it) to
> say
> 
> 	addr = mmap(NULL, 1024*1024,
> 		PROT_READ | PROT_WRITE ,
> 		MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED,
> 		-1, 0);
> 
> and if SuS says that mmap must not return NULL for this case, then SuS
> is so full of sh*t that it's not worth worrying about.

SuS doesn't have this requirement in the case of MAP_FIXED.

For normal maps it says

"When the implementation selects a value for pa, it never places a mapping 
at address 0, nor does it replace any extant mapping."

For MAP_FIXED it says the return value shal be that of pa (first
argument) exactly

Re: do_mmap

[Kai Henningsen]

torvalds@transmeta.com (Linus Torvalds)  wrote on 31.05.02 in <ad8bvv$3tr$1@penguin.transmeta.com>:

> In article <1022855243.12888.410.camel@irongate.swansea.linux.org.uk>,
> Alan Cox  <alan@lxorguk.ukuu.org.uk> wrote:
> >On Fri, 2002-05-31 at 14:00, Thomas 'Dent' Mirlacher wrote:
> >> and the checks in various places are really strange. - well some
> >> places check for:
> >> 	o != NULL
> >> 	o > -1024UL
> >
> >"Not an error". Its relying as some other bits of code do actually that
> >the top mappable user address is never in the top 1K of the address
> >space
> >
> >> is it possible to have 0 as a valid address? - if not, this should
> >> be the return on errors.
> >
> >SuS explicitly says that 0 is not a valid mmap return address.
>
> But if so, SuS is _very_ _very_ wrong.

[...]

> and if SuS says that mmap must not return NULL for this case, then SuS
> is so full of sh*t that it's not worth worrying about.

Actually, at least SuSv3 does not say any such thing. *What* it says is  
that the return is either MAP_FAILED or the correct address, and that if  
it is called *without* MAP_FIXED, then the argument 0 has special meaning,  
and it won't map something at 0 (and thus return 0) - which, AFAICT, is  
exactly what we want it to say. Specifically, NULL is *never* an error  
return value for mmap.

At least, unless we define MAP_FAILED to be NULL - traditionally, it's  
(void *)-1 probably exactly so it is possible to return an address of 0.

See <http://www.opengroup.org/onlinepubs/007904975/functions/mmap.html>.

MfG Kai

Re: do_mmap

[DervishD]

    Hello Alan & Thomas :)

>> is it possible to have 0 as a valid address? - if not, this should
>> be the return on errors.
>SuS explicitly says that 0 is not a valid mmap return address.

    Not in my copy of the SuS, version 2, but yes in my man page
about mmap(). Anyway, Linus told me that he doesn't like to change
the semantics for mmap, so it can return 0.

    I don't like mmap() returning 0, but I don't like too breaking
the old behaviour of the kernel. Moreover, 0 is returned only if the
length requested is 0 and the address passed as hint is 0 too... I
don't see any problem about it.

    Raúl

Re: do_mmap

[DervishD]

    Hello Thomas :)

>is it possible to have 0 as a valid address? - if not, this should
>be the return on errors.

    It is possible. I've sent a patch about it but really returning 0
sometimes is the desired behaviour ;)

    Raúl

Re: do_mmap

[Pavel Machek]

Hi!

> what about the do_mmap/do_mmap_pgoff implementation?
> reurn-type: _unsigned_ long	(which should be ok cause we've to return
> 				 an adress if len == 0)
> on error: -ERR_*
> 
> and the checks in various places are really strange. - well some
> places check for:
> 	o != NULL
> 	o > -1024UL
> 	o ...
> 
> guess this nedds some cleanup.

While you are at it... fs/binfmt_elf does mmaps but does not check for errors.
And errors actually do happen there :-(

-- 
Philips Velo 1: 1"x4"x8", 300gram, 60, 12MB, 40bogomips, linux, mutt,
details at http://atrey.karlin.mff.cuni.cz/~pavel/velo/index.html.

Re: do_mmap

[Thomas 'Dent' Mirlacher]

pavel,

> > what about the do_mmap/do_mmap_pgoff implementation?
> > reurn-type: _unsigned_ long	(which should be ok cause we've to return
> > 				 an adress if len == 0)
> > on error: -ERR_*
> > 
> > and the checks in various places are really strange. - well some
> > places check for:
> > 	o != NULL
> > 	o > -1024UL
> > 	o ...
> > 
> > guess this nedds some cleanup.
> 
> While you are at it... fs/binfmt_elf does mmaps but does not check for errors.
> And errors actually do happen there :-(

sure, was tripping over that anyways ... since the compiler spits out tons
of "conversion without a cast" warnings (seems to be a different way to 
use grep :)

i just came across one problem: when converting the unsigned longs to
        void * pointers, everything works fine, beside the "advanced"
        pointer arithmetic. (like aligning the address to a certain 
        boundary - using address & MASK) ... for that case i need to
        cast to an unsigned long in any case :( 
                is there another way to do it without explicit casting?
		should we introduce ptr_addr_t?
		can we be sure that an unsigned long is capable to hold
			and address (also in the future)
		can we be sure about gcc void * pointer arithmetic (like
			++ incremets the address by one)

	tm

-- 
in some way i do, and in some way i don't.

Re: do_mmap

[Thomas 'Dent' Mirlacher]

pavel,

--snip/snip

> While you are at it... fs/binfmt_elf does mmaps but does not check for errors.
> And errors actually do happen there :-(

ok, had a second look.
it does check for errors, using BAD_ADDR which is: if (addr > TASK_SIZE)
and TASK_SIZE on the other hand is PAGE_OFFSET == 0xC0000000 (for x86)

hmm, which means even if we correctly map an area > PAGE_OFFSET, we will 
never free the area (well another question is, if we ever try to map
an area > PAGE_OFFSET) - but nevertheless, the check should be before
trying to mmap, and checking for IS_ERR after mmap.

nyone explain what to do if we cannot map the page for MMAP_PAGE_ZERO?
- we just ignore it for now ...

also another instance: if (addr != req_address) - which means if we have
	some alignment with mmap - load_elf_library just bails out.
	guess we need to munmap also (if no error occured during mmap)

do_brk() is _never_ checked for return (at least in binfm_elf) ... oh
well ...

any comments?

	tm

-- 
in some way i do, and in some way i don't.

Re: do_mmap

[Pavel Machek]

Hi!

> do_brk() is _never_ checked for return (at least in binfm_elf) ... oh
> well ...

That's the bug I had in my mind. It actually bitten me.

								Pavel
-- 
Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.