On Wed, 2007-11-07 at 20:18 +0100, Fabrice Bellard wrote: > Hi, > > Regarding the user memory access, here is my suggestion which should > minimize the changes:
The virtue of making the minimum changes is that there are likely fewer errors. Other than that, it's more important to me to make the *changes*. > - Keep __put_user() and __get_user() as you did. good. > - Remove put_user(), get_user(), copy_from_user() and copy_to_user() I'd prefer to keep put_user(), get_user(), copy_from_user() and copy_to_user() and have them internally perform lock_user()/unlock_user(). I like how lock_user()/unlock_user() minimizes copying - I think that's good. I also like keeping functions that work in similar ways as the kernel counterparts - this is important for maintaining functions that emulate kernel behavior so that code is more comparable to the kernel. This is also very near the way my current patches are written and is the minimum work for me. > - Modify the signal.c code so that it uses __put_user, __get_user and > lock/unlock_user. good. > - Modify lock_user() so that it automatically does access_ok() and > returns NULL if access_ok() fails. Yes - this is good. This is something that I missed with my first set of patches and it's significant. > - Test lock_user/lock_user_struct/... return value explicitely at every > call. yep. > - Fix page_check_range() so that it handles writes to pages containing > code by calling page_unprotect when necessary (the current code can fail > in this case !). Good. > - Suppress no longer needed page_unprotect_range() call in syscall.c. sure. > - Suppress or fix tput/tget macros so that they do access_ok(). I think it's better to use get_user()/put_user() for these which would handle assigning to different sized types and with proper sign-extension when the target and host aren't the same sized word.