Re: user_memcpy()

> "François Revol" <[email protected]> wrote:
> > > François Revol made me recognize a design flaw in our 
> > > user_memcpy()
> > > /
> > > user_strlcpy() API: you have to check the validity of the user 
> > > address yourself, ie. you have to make sure it's in user space.
> > > But we don't have a function for this, just the (kernel private) 
> > > macro IS_USER_ADDRESS().
> > Actually there is more to that...
> [...]
> > So you end up having is_valid_range(), which noone called anyway 
> > (also 
> > because it wasn't in a public header), and to which you didn't know 
> > which flag to give anyway:
> 
> Doing is_valid_range() correctly is quite a bit overhead, and if 
> possible I would like to get away without doing that :-)
> I think we could introduce something like:
> 	bool called_from_kernel(void);

Whatever the name.

> The VFS could set a flag in the current thread structure. For Haiku, 
> we 

Yes that was my idea.

> could also easily change the driver API (which we do anyway for new-
> style drivers), but a common solution would be preferable, I guess.

We already have the new driver model in as we need it for the IDE 
replacement driver.

> > #define B_READ_AREA 1
> > #define B_WRITE_AREA 2 */
> > #define B_USER_READ_AREA    0x00000004  /* user can read */
> > #define B_USER_WRITE_AREA    0x00000008  /* user can write */
> 
> Isn't it the other way around?
> B_READ_AREA already expresses "user can read". What's missing is a 
> B_KERNEL_READ_AREA (meaning: only kernel can read if used alone).
> 

Nono.. actually it's not the same values... the real constants are:
inc/vm.h:#define PROT_KRD       0x00000001      /* kernel read */
inc/vm.h:#define PROT_KWR       0x00000002      /* kernel write */
inc/vm.h:#define        PROT_URD        0x00000004      /* user read */
inc/vm.h:#define        PROT_UWR        0x00000008      /* user write *
/
inc/vm.h:#define        PROT_ALL        (PROT_KRD | PROT_KWR | PROT_URD 
| PROT_UWR)

> > /* why isn't that in KernelExport ??? */
> > extern bool is_valid_range(void *start, size_t len, uint32 prot);
> 
> Probably out of stupidity - it's a bit too much for an oversight.

Yeah probably.

> I don't think it should be always root, even if it could simply 
> switch 
> its access rights if needed (which would only be safe, though, if 
> these 
> rights could be specified by thread).

But they aren't. uid/gid is set for a whole team.
of course getuid() and friends could use called_from_kernel()...
But that would hide the real uid, so if someone really wants it...

> > See:
> > http://lxr.linux.no/source/include/asm-i386/uaccess.h
> 
> I don't really like this, though - the called_from_kernel() is a bit 
> more flexible.

Indeed.
And it hides the implementation, so drivers wo'nt be affected by a 
change.

> 
> > Now, the elf loader in BeOS certainly doesn't care about those 
> > sections, and the exception handler doesn't either. Probably Haiku 
> > has/
> 
> What does this have to do with the ELF loader, anyway?

Because in linux the return address for exception in *_*_user() are 
stored in those .eh_frame section, so the elf loader needs to parse 
them and store the addresses somewhere.

> 
> > work if we boot a new kernel that has say a new 1:3 vm split, or 
> > maybe 
> > a reverse split as it would make it easier for WINE.
> 
> We already did that for Haiku, btw.

Fine.

> > While Haiku's could be something closer to the linux one:
> > status_t copy_to_user(void *to, void *from, size_t len)
> > {
> > 	if (to < current->addr_limit)
> >         return EFAULT;
> >     // set up exception handler for memcpy
> >     return memcpy(to, from, len);
> > }
> > (but there is still the question of copying to an address in user 
> > space 
> > that is valid, but read only...)
> 
> That would still be catched by the Haiku version, at least (ie. it 
> would safely fail).
> BTW to be consistent to strlcpy_to_user() and user_memset(), I think 
> I 
> would like memcpy_to_user() more than copy_to_user().

I guess so.

> 
> > Anyway I'd propose 2 simple calls we could hardcode as macros for 
> > R5:
> > 
> > #if defined(COMPILE_FOR_R5) && !defined(B_ZETA_VERSION_VENTURE)
> > #define check_for_user() false
> > #define check_for_user_addr() false
> > #define check_for_user_fd() true
> > #else
> > // check for valid user addresses 
> > extern bool check_for_user_addr(void);
> > // check for valid user permissions (for fds and such)
> > extern bool check_for_user_fd(void);
> > #endif
> 
> Why FD?

cause it depends on if the call is made on the user fds or the kernel 
ones.

> Anyway, I think we should have those two:
> bool called_from_kernel(void);
> bool is_user_address(void);

I'd go for those... hmm is_ usually means it applies to a passed 
argument.
do_check_user_address maybe ?

> 
> (or also called_from_user() instead)

That one would just be !called_from_kernel(), which is conceptually 
different than the address stuff. (it's basically or R5 because though 
we must supposed we are called from user we must not check for user 
addresses).

> > I think I can stick something in Zeta R1, but the time window is 
> > shrinking, so it would be nice to reach a consensus there...
> 
> Would be nice, yes, although I we will definitely introduce a new 
> driver architecture, anyway, so maybe the pressure to be compatible 
> is 
> not that high (even if we both could benefit from it).
> 

One more ?
Remember it's yT's prerogative to increment B_CUR_DRIVER_API_VERSION 
now :D


François.




-------------------------------------------------------
This SF.Net email is sponsored by: NEC IT Guy Games.
Get your fingers limbered up and give it your best shot. 4 great events, 4
opportunities to win big! Highest score wins.NEC IT Guy Games. Play to
win an NEC 61 plasma display. Visit http://www.necitguy.com/?r