Messages in this thread | | | Date | Mon, 21 Apr 2008 09:05:02 -0500 | From | Jason Wessel <> | Subject | Re: kgdb: fix optional arch functions and probe_kernel_* |
| |
Ingo Molnar wrote: > * Andrew Morton <akpm@linux-foundation.org> wrote: > > >> On Fri, 18 Apr 2008 17:42:54 GMT >> Linux Kernel Mailing List <linux-kernel@vger.kernel.org> wrote: >> >> >>> --- a/mm/maccess.c >>> +++ b/mm/maccess.c >>> @@ -17,11 +17,14 @@ >>> long probe_kernel_read(void *dst, void *src, size_t size) >>> { >>> long ret; >>> + mm_segment_t old_fs = get_fs(); >>> >>> + set_fs(KERNEL_DS); >>> pagefault_disable(); >>> ret = __copy_from_user_inatomic(dst, >>> (__force const void __user *)src, size); >>> pagefault_enable(); >>> + set_fs(old_fs); >>> >>> return ret ? -EFAULT : 0; >>> } >>> >> Oh. Well that rather invalidates my earlier comments. It looks like >> this change could have been folded, but I understand that this >> sometimes gets wearisome and isn't terribly important if >> >> a) the fix doesn't repair build breakage and >> >> b) the fix doesn't fix runtime breakage and >> >> c) the fix fixes code which the git-bisect user won't have enabled in >> config anyway. >> > > yeah. I mentioned it in the pull request that i kept the fixes apart to > demonstrate the overall fix dynamics of the KGDB tree over a full kernel > cycle. I normally backmerge and create a clean queue - but that creates > a false perception that the tree is 'too fresh' and trust is harder to > be expressed. > > >> Still. Do we need the set_fs() in there? __copy_from_user_inatomic() >> is a "__" uaccess function and hence shouldn't be running access_ok()? >> > > yeah, i guess that's true. Jason? > >
In so far as the testing showed, it worked ok on the X86 arch with and without the set_fs(), but on ARM it is absolutely required. This means we have to decide to make arch specific or leave generic as it stands right now.
Jason.
| |