Re: kgdb: fix optional arch functions and probe_kernel_*

Re: kgdb: fix optional arch functions and probe_kernel_*

[Andrew Morton]

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.


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()?

Re: kgdb: fix optional arch functions and probe_kernel_*

[Ingo Molnar]

* 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?

	Ingo

Re: kgdb: fix optional arch functions and probe_kernel_*

[Jason Wessel]

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.

Re: kgdb: fix optional arch functions and probe_kernel_*

[Ingo Molnar]

* Jason Wessel <jason.wessel@windriver.com> wrote:

> >> 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.

yeah, i guess so - it's no big issue as this isnt performance critical 
in any way. Do you know which exact codepath/mechanism in ARM relies on 
the set_fs() being there?

	Ingo