Due to commit 5cec93c216db77 (x86-64: Emulate legacy vsyscalls), we get the following warning:
arch/x86/kernel/vsyscall_64.c: In function ‘do_emulate_vsyscall’:
arch/x86/kernel/vsyscall_64.c:111:7: warning: ‘ret’ may be used uninitialized in this function
The uninitialized value of 'ret' maybe gets assigned to regs->ax. So, initialize it with -EINVAL.
Signed-off-by: Rakib Mullick <[email protected]>
---
diff --git a/arch/x86/kernel/vsyscall_64.c b/arch/x86/kernel/vsyscall_64.c
index 10cd8ac..180c56d 100644
--- a/arch/x86/kernel/vsyscall_64.c
+++ b/arch/x86/kernel/vsyscall_64.c
@@ -108,7 +108,7 @@ void dotraplinkage do_emulate_vsyscall(struct pt_regs *regs, long error_code)
struct task_struct *tsk;
unsigned long caller;
int vsyscall_nr;
- long ret;
+ long ret = -EINVAL;
/* Kernel code must never get here. */
BUG_ON(!user_mode(regs));
@@ -163,7 +163,7 @@ void dotraplinkage do_emulate_vsyscall(struct pt_regs *regs, long error_code)
BUG();
}
- if (ret == -EFAULT) {
+ if (ret == -EFAULT || ret == -EINVAL) {
/*
* Bad news -- userspace fed a bad pointer to a vsyscall.
*
On Sat, Jun 11, 2011 at 3:31 AM, Rakib Mullick <[email protected]> wrote:
> Due to commit 5cec93c216db77 (x86-64: Emulate legacy vsyscalls), we get the following warning:
>
> ? arch/x86/kernel/vsyscall_64.c: In function ?do_emulate_vsyscall?:
> ? arch/x86/kernel/vsyscall_64.c:111:7: warning: ?ret? may be used uninitialized in this function
What's the code path that uses ret without initializing it?
> - ? ? ? if (ret == -EFAULT) {
> + ? ? ? if (ret == -EFAULT || ret == -EINVAL) {
> ? ? ? ? ? ? ? ?/*
> ? ? ? ? ? ? ? ? * Bad news -- userspace fed a bad pointer to a vsyscall.
> ? ? ? ? ? ? ? ? *
EINVAL doesn't seem like grounds to fault. (I'm not sure how to get
EINVAL from time, gettimeofday, or getcpu, but in case there is, we
should return it back to userspace.)
--Andy
On Sat, Jun 11, 2011 at 5:01 PM, Andrew Lutomirski <[email protected]> wrote:
> On Sat, Jun 11, 2011 at 3:31 AM, Rakib Mullick <[email protected]> wrote:
>> Due to commit 5cec93c216db77 (x86-64: Emulate legacy vsyscalls), we get the following warning:
>>
>> ? arch/x86/kernel/vsyscall_64.c: In function ?do_emulate_vsyscall?:
>> ? arch/x86/kernel/vsyscall_64.c:111:7: warning: ?ret? may be used uninitialized in this function
>
> What's the code path that uses ret without initializing it?
>
In case of, vsyscall_nr is default it might gets uninitialized. And
current code already treats it as a bug.
>> - ? ? ? if (ret == -EFAULT) {
>> + ? ? ? if (ret == -EFAULT || ret == -EINVAL) {
>> ? ? ? ? ? ? ? ?/*
>> ? ? ? ? ? ? ? ? * Bad news -- userspace fed a bad pointer to a vsyscall.
>> ? ? ? ? ? ? ? ? *
>
> EINVAL doesn't seem like grounds to fault. ?(I'm not sure how to get
> EINVAL from time, gettimeofday, or getcpu, but in case there is, we
> should return it back to userspace.)
>
If ret = EINVAL, then it means vsyscall_nr doesn't any of
gettimeofday, time or getcpu. So, I grounds it into fault. In case of
gettimeofday, EINVAL may gets return. But, maybe not in case of time
or getcpu. So, maybe we need to check EINVAL in case of gettimeofday
and maybe should separate EINVAL and EFAULT.
Thanks,
Rakib
> --Andy
>
On Sun, Jun 12, 2011 at 1:12 AM, Rakib Mullick <[email protected]> wrote:
> On Sat, Jun 11, 2011 at 5:01 PM, Andrew Lutomirski <[email protected]> wrote:
>> On Sat, Jun 11, 2011 at 3:31 AM, Rakib Mullick <[email protected]> wrote:
>>> Due to commit 5cec93c216db77 (x86-64: Emulate legacy vsyscalls), we get the following warning:
>>>
>>> ? arch/x86/kernel/vsyscall_64.c: In function ?do_emulate_vsyscall?:
>>> ? arch/x86/kernel/vsyscall_64.c:111:7: warning: ?ret? may be used uninitialized in this function
>>
>> What's the code path that uses ret without initializing it?
>>
> In case of, vsyscall_nr is default it might gets uninitialized. And
> current code already treats it as a bug.
>
>>> - ? ? ? if (ret == -EFAULT) {
>>> + ? ? ? if (ret == -EFAULT || ret == -EINVAL) {
>>> ? ? ? ? ? ? ? ?/*
>>> ? ? ? ? ? ? ? ? * Bad news -- userspace fed a bad pointer to a vsyscall.
>>> ? ? ? ? ? ? ? ? *
>>
>> EINVAL doesn't seem like grounds to fault. ?(I'm not sure how to get
>> EINVAL from time, gettimeofday, or getcpu, but in case there is, we
>> should return it back to userspace.)
>>
> If ret = EINVAL, then it means vsyscall_nr doesn't any of
> gettimeofday, time or getcpu. So, I grounds it into fault. In case of
> gettimeofday, EINVAL may gets return. But, maybe not in case of time
> or getcpu. So, maybe we need to check EINVAL in case of gettimeofday
> and maybe should separate EINVAL and EFAULT.
I think there are three separate issues here:
1. Can ret be used uninitialized? I say no, even as seen by the
compiler. If vsyscall_nr is 0, 1, or 2, then ret is initialized. If
vsyscall_nr is 3, then the BUG gets hit. BUG is defined as some
assembly magic followed by unreachable(), and the compiler is supposed
to know that code after unreachable() is qunreachable. So how can ret
be used uninitialized?
What version of gcc do you have? gcc (GCC) 4.6.0 20110530 (Red Hat
4.6.0-9) does not produce this warning.
2. Is the BUG correct? I say yes. vsyscall_nr can only be 0, 1, 2,
or 3 (see the function that generates it), and the only way that 3
could happen is if regs->ip == 0xffffffffff600c02. That can't happen
because the instruction at ...601 is int3.
3. Should the test for EFAULT be changed to EINVAL? I can't see why.
We need to preserve userspace ABI, and userspace expects vsyscalls
that fail for reasons other than a fault to return an error, not
segfault the caller.
Note that regs->as *is* the return value, so we're not ignoring errors.
--Andy
On Mon, Jun 13, 2011 at 8:52 AM, Andrew Lutomirski <[email protected]> wrote:
> On Sun, Jun 12, 2011 at 1:12 AM, Rakib Mullick <[email protected]> wrote:
>> On Sat, Jun 11, 2011 at 5:01 PM, Andrew Lutomirski <[email protected]> wrote:
>>> On Sat, Jun 11, 2011 at 3:31 AM, Rakib Mullick <[email protected]> wrote:
>>>> Due to commit 5cec93c216db77 (x86-64: Emulate legacy vsyscalls), we get the following warning:
>>>>
>>>> ? arch/x86/kernel/vsyscall_64.c: In function ?do_emulate_vsyscall?:
>>>> ? arch/x86/kernel/vsyscall_64.c:111:7: warning: ?ret? may be used uninitialized in this function
>>>
>>> What's the code path that uses ret without initializing it?
>>>
>> In case of, vsyscall_nr is default it might gets uninitialized. And
>> current code already treats it as a bug.
>>
>>>> - ? ? ? if (ret == -EFAULT) {
>>>> + ? ? ? if (ret == -EFAULT || ret == -EINVAL) {
>>>> ? ? ? ? ? ? ? ?/*
>>>> ? ? ? ? ? ? ? ? * Bad news -- userspace fed a bad pointer to a vsyscall.
>>>> ? ? ? ? ? ? ? ? *
>>>
>>> EINVAL doesn't seem like grounds to fault. ?(I'm not sure how to get
>>> EINVAL from time, gettimeofday, or getcpu, but in case there is, we
>>> should return it back to userspace.)
>>>
>> If ret = EINVAL, then it means vsyscall_nr doesn't any of
>> gettimeofday, time or getcpu. So, I grounds it into fault. In case of
>> gettimeofday, EINVAL may gets return. But, maybe not in case of time
>> or getcpu. So, maybe we need to check EINVAL in case of gettimeofday
>> and maybe should separate EINVAL and EFAULT.
>
> I think there are three separate issues here:
>
> 1. Can ret be used uninitialized? ?I say no, even as seen by the
> compiler. ?If vsyscall_nr is 0, 1, or 2, then ret is initialized. ?If
> vsyscall_nr is 3, then the BUG gets hit. ?BUG is defined as some
> assembly magic followed by unreachable(), and the compiler is supposed
> to know that code after unreachable() is qunreachable. ?So how can ret
> be used uninitialized?
>
I don't have much knowledge of advance assembly, so I really don't
understand that part - how BUG handles this. If it really makes sure
that, it will handle it properly then I think you can drop this patch.
> What version of gcc do you have? ?gcc (GCC) 4.6.0 20110530 (Red Hat
> 4.6.0-9) does not produce this warning.
>
Currently, I'm replying from outside my home. I'll let you know when
I'm back home.
> 2. Is the BUG correct? ?I say yes. ?vsyscall_nr can only be 0, 1, 2,
> or 3 (see the function that generates it), and the only way that 3
> could happen is if regs->ip == 0xffffffffff600c02. ?That can't happen
> because the instruction at ...601 is int3.
>
Ok, thanks for explaining. What will regs->ax will have if it hits BUG?
> 3. Should the test for EFAULT be changed to EINVAL? ?I can't see why.
> We need to preserve userspace ABI, and userspace expects vsyscalls
> that fail for reasons other than a fault to return an error, not
> segfault the caller.
>
Right. I think, we need to check for both EFAULT and EINVAL rather
than changing test for EFAULT to EINVAL. Since both of them can
happen, maybe it will help preserve userspace ABI properly.
> Note that regs->as *is* the return value, so we're not ignoring errors.
>
Yes, right. This was the worrying factor, what will regs->ax have. We
shouldn't allow anything else other than return value or EINVAL.
Thanks,
Rakib
> --Andy
>
On Mon, Jun 13, 2011 at 10:54 AM, Rakib Mullick <[email protected]> wrote:
> On Mon, Jun 13, 2011 at 8:52 AM, Andrew Lutomirski <[email protected]> wrote:
>> On Sun, Jun 12, 2011 at 1:12 AM, Rakib Mullick <[email protected]> wrote:
>>> On Sat, Jun 11, 2011 at 5:01 PM, Andrew Lutomirski <[email protected]> wrote:
>>>> On Sat, Jun 11, 2011 at 3:31 AM, Rakib Mullick <[email protected]> wrote:
>>
>> I think there are three separate issues here:
>>
>> 1. Can ret be used uninitialized? ?I say no, even as seen by the
>> compiler. ?If vsyscall_nr is 0, 1, or 2, then ret is initialized. ?If
>> vsyscall_nr is 3, then the BUG gets hit. ?BUG is defined as some
>> assembly magic followed by unreachable(), and the compiler is supposed
>> to know that code after unreachable() is qunreachable. ?So how can ret
>> be used uninitialized?
>>
> I don't have much knowledge of advance assembly, so I really don't
> understand that part - how BUG handles this. If it really makes sure
> that, it will handle it properly then I think you can drop this patch.
>
>> What version of gcc do you have? ?gcc (GCC) 4.6.0 20110530 (Red Hat
>> 4.6.0-9) does not produce this warning.
>>
> Currently, I'm replying from outside my home. I'll let you know when
> I'm back home.
>
Here is my GCC version - gcc version 4.5.1 20100924 (Red Hat 4.5.1-4)
(GCC). I'm using Fedora 14.
Thanks,
Rakib
* Andrew Lutomirski <[email protected]> wrote:
> On Sat, Jun 11, 2011 at 3:31 AM, Rakib Mullick <[email protected]> wrote:
> > Due to commit 5cec93c216db77 (x86-64: Emulate legacy vsyscalls), we get the following warning:
> >
> > arch/x86/kernel/vsyscall_64.c: In function ‘do_emulate_vsyscall’:
> > arch/x86/kernel/vsyscall_64.c:111:7: warning: ‘ret’ may be used uninitialized in this function
>
> What's the code path that uses ret without initializing it?
If the code is correct but GCC got confused then please use the
simplest possible patch to help GCC find its way around the code.
Thanks,
Ingo
On Mon, Jun 13, 2011 at 5:29 AM, Ingo Molnar <[email protected]> wrote:
>
> * Andrew Lutomirski <[email protected]> wrote:
>
>> On Sat, Jun 11, 2011 at 3:31 AM, Rakib Mullick <[email protected]> wrote:
>> > Due to commit 5cec93c216db77 (x86-64: Emulate legacy vsyscalls), we get the following warning:
>> >
>> > ? arch/x86/kernel/vsyscall_64.c: In function ?do_emulate_vsyscall?:
>> > ? arch/x86/kernel/vsyscall_64.c:111:7: warning: ?ret? may be used uninitialized in this function
>>
>> What's the code path that uses ret without initializing it?
>
> If the code is correct but GCC got confused then please use the
> simplest possible patch to help GCC find its way around the code.
The simplest patch is to mark ret as uninitialized_var.
Before making that change, though, I'd like to try to reproduce this,
since it's possible that something's wrong with the BUG macro. That's
why I'm waiting to hear what gcc version gets the warning.
--Andy
* Andrew Lutomirski <[email protected]> wrote:
> On Mon, Jun 13, 2011 at 5:29 AM, Ingo Molnar <[email protected]> wrote:
> >
> > * Andrew Lutomirski <[email protected]> wrote:
> >
> >> On Sat, Jun 11, 2011 at 3:31 AM, Rakib Mullick <[email protected]> wrote:
> >> > Due to commit 5cec93c216db77 (x86-64: Emulate legacy vsyscalls), we get the following warning:
> >> >
> >> > arch/x86/kernel/vsyscall_64.c: In function ‘do_emulate_vsyscall’:
> >> > arch/x86/kernel/vsyscall_64.c:111:7: warning: ‘ret’ may be used uninitialized in this function
> >>
> >> What's the code path that uses ret without initializing it?
> >
> > If the code is correct but GCC got confused then please use the
> > simplest possible patch to help GCC find its way around the code.
>
> The simplest patch is to mark ret as uninitialized_var.
No - that primitive really sucks as it might hide *future* debug
warnings and silently break code.
The problem with uninitialized_var() is that such code:
int test(void)
{
int uninitialized_var(ret);
return ret;
}
Builds without a single warning but it is very broken code.
So if we use uninitialized_var() and the code is changed in the
future to have the above broken sequence, we'll have a silent runtime
failure ...
So we try to avoid using uninitialized_var() in arch/x86/ and use
explicit initialization instead.
That way GCC that can see through the flow will optimize away the
superfluous initialization - GCC versions that are older will
generate one more instruction but that's OK.
Thanks,
Ingo
On Mon, Jun 13, 2011 at 10:14 AM, Ingo Molnar <[email protected]> wrote:
>
> * Andrew Lutomirski <[email protected]> wrote:
>
>> On Mon, Jun 13, 2011 at 5:29 AM, Ingo Molnar <[email protected]> wrote:
>> >
>> > * Andrew Lutomirski <[email protected]> wrote:
>> >
>> >> On Sat, Jun 11, 2011 at 3:31 AM, Rakib Mullick <[email protected]> wrote:
>> >> > Due to commit 5cec93c216db77 (x86-64: Emulate legacy vsyscalls), we get the following warning:
>> >> >
>> >> > ? arch/x86/kernel/vsyscall_64.c: In function ?do_emulate_vsyscall?:
>> >> > ? arch/x86/kernel/vsyscall_64.c:111:7: warning: ?ret? may be used uninitialized in this function
>> >>
>> >> What's the code path that uses ret without initializing it?
>> >
>> > If the code is correct but GCC got confused then please use the
>> > simplest possible patch to help GCC find its way around the code.
>>
>> The simplest patch is to mark ret as uninitialized_var.
>
> No - that primitive really sucks as it might hide *future* debug
> warnings and silently break code.
>
> The problem with uninitialized_var() is that such code:
>
> ? ? ? ?int test(void)
> ? ? ? ?{
> ? ? ? ? ? ? ? ?int uninitialized_var(ret);
>
> ? ? ? ? ? ? ? ?return ret;
> ? ? ? ?}
>
> Builds without a single warning but it is very broken code.
>
> So if we use uninitialized_var() and the code is changed in the
> future to have the above broken sequence, we'll have a silent runtime
> failure ...
>
> So we try to avoid using uninitialized_var() in arch/x86/ and use
> explicit initialization instead.
>
> That way GCC that can see through the flow will optimize away the
> superfluous initialization - GCC versions that are older will
> generate one more instruction but that's OK.
Fair enough.
Unfortunately there doesn't seem to be an EKERNELBUG error code, and
initializing to EFAULT seems silly. 0 is probably harmless.
I'll wait awhile longer for that GCC version, since there might be a
better fix. In any case, it would be nice for the changelog entry to
say which version has a warning that's being worked around.
--Andy
On Mon, Jun 13, 2011 at 8:18 PM, Andrew Lutomirski <[email protected]> wrote:
> On Mon, Jun 13, 2011 at 10:14 AM, Ingo Molnar <[email protected]> wrote:
>>
>> * Andrew Lutomirski <[email protected]> wrote:
>>
>>> On Mon, Jun 13, 2011 at 5:29 AM, Ingo Molnar <[email protected]> wrote:
>>> >
>>> > * Andrew Lutomirski <[email protected]> wrote:
>>> >
>>> >> On Sat, Jun 11, 2011 at 3:31 AM, Rakib Mullick <[email protected]> wrote:
>>> >> > Due to commit 5cec93c216db77 (x86-64: Emulate legacy vsyscalls), we get the following warning:
>>> >> >
>>> >> > ? arch/x86/kernel/vsyscall_64.c: In function ?do_emulate_vsyscall?:
>>> >> > ? arch/x86/kernel/vsyscall_64.c:111:7: warning: ?ret? may be used uninitialized in this function
>>> >>
>>> >> What's the code path that uses ret without initializing it?
>>> >
>>> > If the code is correct but GCC got confused then please use the
>>> > simplest possible patch to help GCC find its way around the code.
>>>
>>> The simplest patch is to mark ret as uninitialized_var.
>>
>> No - that primitive really sucks as it might hide *future* debug
>> warnings and silently break code.
>>
>> The problem with uninitialized_var() is that such code:
>>
>> ? ? ? ?int test(void)
>> ? ? ? ?{
>> ? ? ? ? ? ? ? ?int uninitialized_var(ret);
>>
>> ? ? ? ? ? ? ? ?return ret;
>> ? ? ? ?}
>>
>> Builds without a single warning but it is very broken code.
>>
>> So if we use uninitialized_var() and the code is changed in the
>> future to have the above broken sequence, we'll have a silent runtime
>> failure ...
>>
>> So we try to avoid using uninitialized_var() in arch/x86/ and use
>> explicit initialization instead.
>>
>> That way GCC that can see through the flow will optimize away the
>> superfluous initialization - GCC versions that are older will
>> generate one more instruction but that's OK.
>
> Fair enough.
>
> Unfortunately there doesn't seem to be an EKERNELBUG error code, and
> initializing to EFAULT seems silly. ?0 is probably harmless.
>
> I'll wait awhile longer for that GCC version, since there might be a
> better fix. ?In any case, it would be nice for the changelog entry to
> say which version has a warning that's being worked around.
>
Well, I think I already posted the GCC version in this thread. Anyway,
for you're convenience, here is my GCC version: gcc version 4.5.1
20100924 (Red Hat 4.5.1-4). I'm using Fedora Core 14.
Thanks,
Rakib
On Mon, Jun 13, 2011 at 1:05 PM, Rakib Mullick <[email protected]> wrote:
> On Mon, Jun 13, 2011 at 8:18 PM, Andrew Lutomirski <[email protected]> wrote:
>> On Mon, Jun 13, 2011 at 10:14 AM, Ingo Molnar <[email protected]> wrote:
>>>
>>> * Andrew Lutomirski <[email protected]> wrote:
>>>
>>>> On Mon, Jun 13, 2011 at 5:29 AM, Ingo Molnar <[email protected]> wrote:
>>>> >
>>>> > * Andrew Lutomirski <[email protected]> wrote:
>>>> >
>>>> >> On Sat, Jun 11, 2011 at 3:31 AM, Rakib Mullick <[email protected]> wrote:
>>>> >> > Due to commit 5cec93c216db77 (x86-64: Emulate legacy vsyscalls), we get the following warning:
>>>> >> >
>>>> >> > ? arch/x86/kernel/vsyscall_64.c: In function ?do_emulate_vsyscall?:
>>>> >> > ? arch/x86/kernel/vsyscall_64.c:111:7: warning: ?ret? may be used uninitialized in this function
>>>> >>
>>>> >> What's the code path that uses ret without initializing it?
>>>> >
>>>> > If the code is correct but GCC got confused then please use the
>>>> > simplest possible patch to help GCC find its way around the code.
>>>>
>>>> The simplest patch is to mark ret as uninitialized_var.
>>>
>>> No - that primitive really sucks as it might hide *future* debug
>>> warnings and silently break code.
>>>
>>> The problem with uninitialized_var() is that such code:
>>>
>>> ? ? ? ?int test(void)
>>> ? ? ? ?{
>>> ? ? ? ? ? ? ? ?int uninitialized_var(ret);
>>>
>>> ? ? ? ? ? ? ? ?return ret;
>>> ? ? ? ?}
>>>
>>> Builds without a single warning but it is very broken code.
>>>
>>> So if we use uninitialized_var() and the code is changed in the
>>> future to have the above broken sequence, we'll have a silent runtime
>>> failure ...
>>>
>>> So we try to avoid using uninitialized_var() in arch/x86/ and use
>>> explicit initialization instead.
>>>
>>> That way GCC that can see through the flow will optimize away the
>>> superfluous initialization - GCC versions that are older will
>>> generate one more instruction but that's OK.
>>
>> Fair enough.
>>
>> Unfortunately there doesn't seem to be an EKERNELBUG error code, and
>> initializing to EFAULT seems silly. ?0 is probably harmless.
>>
>> I'll wait awhile longer for that GCC version, since there might be a
>> better fix. ?In any case, it would be nice for the changelog entry to
>> say which version has a warning that's being worked around.
>>
> Well, I think I already posted the GCC version in this thread. Anyway,
> for you're convenience, here is my GCC version: gcc version 4.5.1
> 20100924 (Red Hat 4.5.1-4). I'm using Fedora Core 14.
Sorry -- I think I managed to read every email in the thread except
that one. Will test tonight.
--Andy
>
>
> Thanks,
> Rakib
>
On Mon, Jun 13, 2011 at 4:45 AM, Rakib Mullick <[email protected]> wrote:
> On Mon, Jun 13, 2011 at 10:54 AM, Rakib Mullick <[email protected]> wrote:
>> On Mon, Jun 13, 2011 at 8:52 AM, Andrew Lutomirski <[email protected]> wrote:
>>> On Sun, Jun 12, 2011 at 1:12 AM, Rakib Mullick <[email protected]> wrote:
>>>> On Sat, Jun 11, 2011 at 5:01 PM, Andrew Lutomirski <[email protected]> wrote:
>>>>> On Sat, Jun 11, 2011 at 3:31 AM, Rakib Mullick <[email protected]> wrote:
>>>
>>> I think there are three separate issues here:
>>>
>>> 1. Can ret be used uninitialized? ?I say no, even as seen by the
>>> compiler. ?If vsyscall_nr is 0, 1, or 2, then ret is initialized. ?If
>>> vsyscall_nr is 3, then the BUG gets hit. ?BUG is defined as some
>>> assembly magic followed by unreachable(), and the compiler is supposed
>>> to know that code after unreachable() is qunreachable. ?So how can ret
>>> be used uninitialized?
>>>
>> I don't have much knowledge of advance assembly, so I really don't
>> understand that part - how BUG handles this. If it really makes sure
>> that, it will handle it properly then I think you can drop this patch.
>>
>>> What version of gcc do you have? ?gcc (GCC) 4.6.0 20110530 (Red Hat
>>> 4.6.0-9) does not produce this warning.
>>>
>> Currently, I'm replying from outside my home. I'll let you know when
>> I'm back home.
>>
> Here is my GCC version - gcc version 4.5.1 20100924 (Red Hat 4.5.1-4)
> (GCC). I'm using Fedora 14.
I also have gcc (GCC) 4.5.1 20100924 (Red Hat 4.5.1-4) on another box,
and I still can't reproduce this.
Can you tell me which git revision you're building and send me your
.config and the output of:
$ touch arch/x86/kernel/vsyscall_64.o
$ make V=1 arch/x86/kernel/vsyscall_64.o
Thanks,
Andy
On Tue, Jun 14, 2011 at 12:06 AM, Andrew Lutomirski <[email protected]> wrote:
> On Mon, Jun 13, 2011 at 4:45 AM, Rakib Mullick <[email protected]> wrote:
>> On Mon, Jun 13, 2011 at 10:54 AM, Rakib Mullick <[email protected]> wrote:
>>> On Mon, Jun 13, 2011 at 8:52 AM, Andrew Lutomirski <[email protected]> wrote:
>>>> On Sun, Jun 12, 2011 at 1:12 AM, Rakib Mullick <[email protected]> wrote:
>>>>> On Sat, Jun 11, 2011 at 5:01 PM, Andrew Lutomirski <[email protected]> wrote:
>>>>>> On Sat, Jun 11, 2011 at 3:31 AM, Rakib Mullick <[email protected]> wrote:
>>>>
>>>> I think there are three separate issues here:
>>>>
>>>> 1. Can ret be used uninitialized? ?I say no, even as seen by the
>>>> compiler. ?If vsyscall_nr is 0, 1, or 2, then ret is initialized. ?If
>>>> vsyscall_nr is 3, then the BUG gets hit. ?BUG is defined as some
>>>> assembly magic followed by unreachable(), and the compiler is supposed
>>>> to know that code after unreachable() is qunreachable. ?So how can ret
>>>> be used uninitialized?
>>>>
>>> I don't have much knowledge of advance assembly, so I really don't
>>> understand that part - how BUG handles this. If it really makes sure
>>> that, it will handle it properly then I think you can drop this patch.
>>>
>>>> What version of gcc do you have? ?gcc (GCC) 4.6.0 20110530 (Red Hat
>>>> 4.6.0-9) does not produce this warning.
>>>>
>>> Currently, I'm replying from outside my home. I'll let you know when
>>> I'm back home.
>>>
>> Here is my GCC version - gcc version 4.5.1 20100924 (Red Hat 4.5.1-4)
>> (GCC). I'm using Fedora 14.
>
> I also have gcc (GCC) 4.5.1 20100924 (Red Hat 4.5.1-4) on another box,
> and I still can't reproduce this.
>
> Can you tell me which git revision you're building and send me your
> .config and the output of:
>
I'm using 3.0.0-rc2 (lastly I pulled tip tree 3 days ago). I've
attached the .config (config.log).
> $ touch arch/x86/kernel/vsyscall_64.o
> $ make V=1 arch/x86/kernel/vsyscall_64.o
>
Output of the above steps are attached (vsyscall_64.log). Hope that will help.
Thanks,
Rakib
> Thanks,
> Andy
>
On 06/14/2011 01:43 PM, Rakib Mullick wrote:
> On Tue, Jun 14, 2011 at 12:06 AM, Andrew Lutomirski<[email protected]> wrote:
>> On Mon, Jun 13, 2011 at 4:45 AM, Rakib Mullick<[email protected]> wrote:
>>> On Mon, Jun 13, 2011 at 10:54 AM, Rakib Mullick<[email protected]> wrote:
>>>> On Mon, Jun 13, 2011 at 8:52 AM, Andrew Lutomirski<[email protected]> wrote:
>>>>> On Sun, Jun 12, 2011 at 1:12 AM, Rakib Mullick<[email protected]> wrote:
>>>>>> On Sat, Jun 11, 2011 at 5:01 PM, Andrew Lutomirski<[email protected]> wrote:
>>>>>>> On Sat, Jun 11, 2011 at 3:31 AM, Rakib Mullick<[email protected]> wrote:
>>>>>
>>>>> I think there are three separate issues here:
>>>>>
>>>>> 1. Can ret be used uninitialized? I say no, even as seen by the
>>>>> compiler. If vsyscall_nr is 0, 1, or 2, then ret is initialized. If
>>>>> vsyscall_nr is 3, then the BUG gets hit. BUG is defined as some
>>>>> assembly magic followed by unreachable(), and the compiler is supposed
>>>>> to know that code after unreachable() is qunreachable. So how can ret
>>>>> be used uninitialized?
>>>>>
>>>> I don't have much knowledge of advance assembly, so I really don't
>>>> understand that part - how BUG handles this. If it really makes sure
>>>> that, it will handle it properly then I think you can drop this patch.
>>>>
>>>>> What version of gcc do you have? gcc (GCC) 4.6.0 20110530 (Red Hat
>>>>> 4.6.0-9) does not produce this warning.
>>>>>
>>>> Currently, I'm replying from outside my home. I'll let you know when
>>>> I'm back home.
>>>>
>>> Here is my GCC version - gcc version 4.5.1 20100924 (Red Hat 4.5.1-4)
>>> (GCC). I'm using Fedora 14.
>>
>> I also have gcc (GCC) 4.5.1 20100924 (Red Hat 4.5.1-4) on another box,
>> and I still can't reproduce this.
>>
>> Can you tell me which git revision you're building and send me your
>> .config and the output of:
>>
> I'm using 3.0.0-rc2 (lastly I pulled tip tree 3 days ago). I've
> attached the .config (config.log).
>
>> $ touch arch/x86/kernel/vsyscall_64.o
>> $ make V=1 arch/x86/kernel/vsyscall_64.o
>>
> Output of the above steps are attached (vsyscall_64.log). Hope that will help.
Aha! You have CONFIG_BUG=n. I'm not sure that fixing warnings for that
case is worth it (or is even a good idea).
Can you try this patch, though:
Signed-off-by: Andy Lutomirski <[email protected]>
diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index dfb0ec6..f4083f4 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -107,11 +107,11 @@ extern void warn_slowpath_null(const char *file,
const int line);
#else /* !CONFIG_BUG */
#ifndef HAVE_ARCH_BUG
-#define BUG() do {} while(0)
+#define BUG() do { unreachable(); } while(0)
#endif
#ifndef HAVE_ARCH_BUG_ON
-#define BUG_ON(condition) do { if (condition) ; } while(0)
+#define BUG_ON(condition) do { if (condition) unreachable(); } while(0)
#endif
#ifndef HAVE_ARCH_WARN_ON
It may silence a lot of warnings, although it'll come at a cost of
increased code size with CONFIG_BUG=n on older gcc. On newer GCC,
you'll get possibly faster and smaller code.
--Andy
* Andy Lutomirski <[email protected]> wrote:
> Aha! You have CONFIG_BUG=n. I'm not sure that fixing warnings for
> that case is worth it (or is even a good idea).
Especially since the warning is correct, CONFIG_BUG=n is mortally
broken!
A BUG() turned into NOP is 100% evil, full stop.
> Can you try this patch, though:
>
> Signed-off-by: Andy Lutomirski <[email protected]>
>
> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
> index dfb0ec6..f4083f4 100644
> --- a/include/asm-generic/bug.h
> +++ b/include/asm-generic/bug.h
> @@ -107,11 +107,11 @@ extern void warn_slowpath_null(const char
> *file, const int line);
>
> #else /* !CONFIG_BUG */
> #ifndef HAVE_ARCH_BUG
> -#define BUG() do {} while(0)
> +#define BUG() do { unreachable(); } while(0)
> #endif
>
> #ifndef HAVE_ARCH_BUG_ON
> -#define BUG_ON(condition) do { if (condition) ; } while(0)
> +#define BUG_ON(condition) do { if (condition) unreachable(); } while(0)
> #endif
>
> #ifndef HAVE_ARCH_WARN_ON
>
> It may silence a lot of warnings, although it'll come at a cost of
> increased code size with CONFIG_BUG=n on older gcc. On newer GCC,
> you'll get possibly faster and smaller code.
I sent such a patch ages ago but was shouted down by 'this will
increase code size'.
I think correctness trumps code size and turning BUG() and BUG_ON()
into a NOP is just crazy ...
Thanks,
Ingo
On Tue, Jun 14, 2011 at 2:16 PM, Ingo Molnar <[email protected]> wrote:
>
> I think correctness trumps code size and turning BUG() and BUG_ON()
> into a NOP is just crazy ...
Umm. It's even CRAZIER to turn it into a "compiler generates random code".
Which is what "unreachable()" ends up doing (different compilers will
generate different things - ranging from an infinite loop, to a "nop
with random behavior after it because gcc decided that it doesn't need
to pop arguments off the stack and just runs into random code
instead").
So a NOP is a *hell* of a lot better than turning BUG_ON() into
something random.
The only (and I mean *only*) valid use-case for unreachable() is after
an inline asm that really causes the next instruction to be
unreachable(), but the compiler just doesn't understand it. It is
*not* valid for that kind of crazy "if (condition) do-random-thing"
crap.
Seriously. If you want a "do random thing" thing, don't call it
BUG_ON(). Call it "I_M_A_FUCKING_MORON()".
There is no way I will ever accept a moronic patch like that.
Really.
Linus
* Linus Torvalds <[email protected]> wrote:
> On Tue, Jun 14, 2011 at 2:16 PM, Ingo Molnar <[email protected]> wrote:
> >
> > I think correctness trumps code size and turning BUG() and BUG_ON()
> > into a NOP is just crazy ...
>
> Umm. It's even CRAZIER to turn it into a "compiler generates random code".
Sigh, i assumed it got turned into an infinite loop - that is what
i've done in a prior patch.
You are right, unreachable() is bogus and you'd also be right to
suggest that i should not comment on patches after 11pm ;-)
Thanks,
Ingo
On Tue, Jun 14, 2011 at 5:31 PM, Ingo Molnar <[email protected]> wrote:
>
> * Linus Torvalds <[email protected]> wrote:
>
>> On Tue, Jun 14, 2011 at 2:16 PM, Ingo Molnar <[email protected]> wrote:
>> >
>> > I think correctness trumps code size and turning BUG() and BUG_ON()
>> > into a NOP is just crazy ...
>>
>> Umm. It's even CRAZIER to turn it into a "compiler generates random code".
>
> Sigh, i assumed it got turned into an infinite loop - that is what
> i've done in a prior patch.
>
> You are right, unreachable() is bogus and you'd also be right to
> suggest that i should not comment on patches after 11pm ;-)
What we want is a magic GCC trick that says "don't warn about code
paths that go through here but generate the same code as you would
without this annotation." I don't think such a thing exists.
--Andy
On Wed, Jun 15, 2011 at 3:33 AM, Andrew Lutomirski <[email protected]> wrote:
> On Tue, Jun 14, 2011 at 5:31 PM, Ingo Molnar <[email protected]> wrote:
>>
>> * Linus Torvalds <[email protected]> wrote:
>>
>>> On Tue, Jun 14, 2011 at 2:16 PM, Ingo Molnar <[email protected]> wrote:
>>> >
>>> > I think correctness trumps code size and turning BUG() and BUG_ON()
>>> > into a NOP is just crazy ...
>>>
>>> Umm. It's even CRAZIER to turn it into a "compiler generates random code".
>>
>> Sigh, i assumed it got turned into an infinite loop - that is what
>> i've done in a prior patch.
>>
>> You are right, unreachable() is bogus and you'd also be right to
>> suggest that i should not comment on patches after 11pm ;-)
>
> What we want is a magic GCC trick that says "don't warn about code
> paths that go through here but generate the same code as you would
> without this annotation." ?I don't think such a thing exists.
>
No, I don't think we need such kind of thing. I think, we should less
rely on GCC. Here, we need to reconsider the use of BUG. When
vsyscall_nr is default, it hits BUG. Here is the code comment:
" * If we get here, then vsyscall_nr indicates that int 0xcc
* happened at an address in the vsyscall page that doesn't
* contain int 0xcc. That can't happen. "
If that can't happen, I think we can treat it as a FAULT. So, rather
than calling BUG we can ground it into EFAULT. Does it break ABI
compatibility?
Thanks,
Rakib
* Rakib Mullick <[email protected]> wrote:
> On Wed, Jun 15, 2011 at 3:33 AM, Andrew Lutomirski <[email protected]> wrote:
> > On Tue, Jun 14, 2011 at 5:31 PM, Ingo Molnar <[email protected]> wrote:
> >>
> >> * Linus Torvalds <[email protected]> wrote:
> >>
> >>> On Tue, Jun 14, 2011 at 2:16 PM, Ingo Molnar <[email protected]> wrote:
> >>> >
> >>> > I think correctness trumps code size and turning BUG() and BUG_ON()
> >>> > into a NOP is just crazy ...
> >>>
> >>> Umm. It's even CRAZIER to turn it into a "compiler generates random code".
> >>
> >> Sigh, i assumed it got turned into an infinite loop - that is what
> >> i've done in a prior patch.
> >>
> >> You are right, unreachable() is bogus and you'd also be right to
> >> suggest that i should not comment on patches after 11pm ;-)
> >
> > What we want is a magic GCC trick that says "don't warn about code
> > paths that go through here but generate the same code as you would
> > without this annotation." ?I don't think such a thing exists.
> >
> No, I don't think we need such kind of thing. I think, we should less
> rely on GCC. Here, we need to reconsider the use of BUG. When
> vsyscall_nr is default, it hits BUG. Here is the code comment:
>
> " * If we get here, then vsyscall_nr indicates that int 0xcc
> * happened at an address in the vsyscall page that doesn't
> * contain int 0xcc. That can't happen. "
>
> If that can't happen, I think we can treat it as a FAULT. So,
> rather than calling BUG we can ground it into EFAULT. Does it break
> ABI compatibility?
No, that BUG() is a "cannot happen on a correct kernel" so it has no
ABI impact - but it might trigger if the execution environment is
violated:
- hardware failure
- miscompilation
- data corruption by some other kernel bug
- etc.
- or it might trigger in the future if someone changes the code in
a way that breaks the underlying assumption.
I guess we could do a __BUG_ON() that wont be optimized away even on
!CONFIG_BUG kernels but it seems a bit silly.
So can someone tell me what the assumptions of CONFIG_BUG=n are?
If CONFIG_BUG=n means "i trust the kernel, the toolchain, the kernel
and the hardware to be 100% correct [or don't care if any of those
are broken]" then i can only see one solution:
- leave the warning as-is. Whoever builds with CONFIG_BUG=n will
have to live with the consequences of the 'impossible' happening
and will have to accept the more unpredictable kernel behavior
that *will* trigger in various parts of the kernel if BUG() is
turned into a NOP. If any of the above 'impossible' failure modes
triggers then having more undefined behavior in form of an
uninitialized variable will be the least of their worry.
Thanks,
Ingo
On Wed, Jun 15, 2011 at 12:25 AM, Ingo Molnar <[email protected]> wrote:
>
> No, that BUG() is a "cannot happen on a correct kernel" so it has no
> ABI impact - but it might trigger if the execution environment is
> violated:
Well, in genreal, I would seriously suggest that people not use
BUG_ON() as liberally as they tend to be used.
There are *very* few reasons to have a BUG_ON(), and they are all "I
cannot possibly continue from this state".
Anything else should have a WARN_ON() or just return an error, or (if
it's a security issue) just kill the process.
Some kernel developers seem to use BUG_ON() as a "I can't see how this
could ever trigger, so let's kill the machine if it does", and that
really is very wrong.
If you are aware that something should never trigger, I'd suggest you
either say "ok, I'm _sure_ that this cannot trigger" and just remove
the BUG_ON(), or you should ask yourself "are we better off killing
the machine than just returning an error".
In this case, for example, if
> - leave the warning as-is. Whoever builds with CONFIG_BUG=n will
> have to live with the consequences of the 'impossible' happening
> and will have to accept the more unpredictable kernel behavior
> that *will* trigger in various parts of the kernel if BUG() is
> turned into a NOP. If any of the above 'impossible' failure modes
> triggers then having more undefined behavior in form of an
> uninitialized variable will be the least of their worry.
If it's that impossible, I don't see why you have the BUG_ON() in the
first place.
I also don't see why the code isn't just written to be so strict that
the lack of BUG_ON just wouldn't matter. I think that code that
"depends" on a BUG_ON() for correctness (ie assuming that the whole
thing never returns) is buggy by definition. Please just don't write
code like that.
So what's the reason for just not initializing that 'ret' variable to
-EFAULT, and leaving it at that? And/or just removing all the BUG_ON's
entirely? Do they actually _help_ anything?
Linus
On Wed, Jun 15, 2011 at 2:49 PM, Linus Torvalds
<[email protected]> wrote:
> On Wed, Jun 15, 2011 at 12:25 AM, Ingo Molnar <[email protected]> wrote:
>>
>> No, that BUG() is a "cannot happen on a correct kernel" so it has no
>> ABI impact - but it might trigger if the execution environment is
>> violated:
>
> Well, in genreal, I would seriously suggest that people not use
> BUG_ON() as liberally as they tend to be used.
>
> There are *very* few reasons to have a BUG_ON(), and they are all "I
> cannot possibly continue from this state".
>
> Anything else should have a WARN_ON() or just return an error, or (if
> it's a security issue) just kill the process.
>
> Some kernel developers seem to use BUG_ON() as a "I can't see how this
> could ever trigger, so let's kill the machine if it does", and that
> really is very wrong.
>
> If you are aware that something should never trigger, I'd suggest you
> either say "ok, I'm _sure_ that this cannot trigger" and just remove
> the BUG_ON(), or you should ask yourself "are we better off killing
> the machine than just returning an error".
>
> In this case, for example, if
>
>> ?- leave the warning as-is. Whoever builds with CONFIG_BUG=n will
>> ? have to live with the consequences of the 'impossible' happening
>> ? and will have to accept the more unpredictable kernel behavior
>> ? that *will* trigger in various parts of the kernel if BUG() is
>> ? turned into a NOP. If any of the above 'impossible' failure modes
>> ? triggers then having more undefined behavior in form of an
>> ? uninitialized variable will be the least of their worry.
>
> If it's that impossible, I don't see why you have the BUG_ON() in the
> first place.
>
> I also don't see why the code isn't just written to be so strict that
> the lack of BUG_ON just wouldn't matter. I think that code that
> "depends" on a BUG_ON() for correctness (ie assuming that the whole
> thing never returns) is buggy by definition. Please just don't write
> code like that.
>
> So what's the reason for just not initializing that 'ret' variable to
> -EFAULT, and leaving it at that? And/or just removing all the BUG_ON's
> entirely? Do they actually _help_ anything?
Well, let's say that my logic is wrong and this particular BUG can be
hit because some kernel bug allows some user program to trigger it.
Then we can do one of four things:
1. Return some value back to userspace (i.e. not EFAULT). (This value
could be hardcoded or left as uninitialized.) I don't like this: if
this happens, I want to see a bug report so I can fix the root cause.
Also, for so long as the kernel bug exists, then userspace exploits
could try to take advantage of the incorrect kernel behavior to take
over buggy programs.
2. Set ret = -EFAULT. That will hit the EFAULT path which will print
"vsyscall fault (exploit attempt?)" to the kernel log and kill the
process. I have no problem killing the process, but the message is
misleading. We don't have an "exploit attempt?"; we have a kernel bug
that should be fixed. This will confuse people and could result in me
not getting a bug report.
3. Add code to print a more informative message and kill the process.
This seems like needless bloat.
4. BUG(). This will kill the process, print a nice loud message that
will get reported, and should leave the system pretty much usable
because no locks are held and no resources are in a funny state.
So, given the context, I stand by my BUG.
*However*, in this particular case, there is a different fix. I can
rearrange the code to send vsyscall_nr == 3 through the bad RIP (i.e.
"illegal int 0xcc") path. It even removes eight lines of code, and
I'll submit a patch as part of v2 of "remove all remaining code from
the vsyscall page".
--Andy
P.S. I would like to congratulate myself for my
nitpicking-received-per-unit-patch score. :)
P.P.S. Maybe I'm off the hook until you notice the
BUG_ON(!user_mode(regs)) a few lines up. That one I think should stay
since we're pretty much screwed if kernel code hits the int 0xcc
handler. Trying to emulate a ret instruction will throw the caller
into some random address and OOPS just as hard a few nanoseconds
later. Anyway, if int 0xcb will OOPS, then I think int 0xcc should
also.
On Wed, Jun 15, 2011 at 12:24 PM, Andrew Lutomirski <[email protected]> wrote:
>
> Well, let's say that my logic is wrong and this particular BUG can be
> hit because some kernel bug allows some user program to trigger it.
Christ, we already check the particular address. And if a user can
generate the buggy situation, THEN THE BUG_ON() SURE AS HELL ISN'T
HELPING ANYTHING!
Guys, if that BUG_ON can ever be triggered, IT IS A SECURITY HOLE IN
ITSELF! What's so hard to understand about that?
BUG_ON's are not "good ways to figure out something went wrong". They
are an absolute last-case situation. They are NOT "let's fix that
security hole by halting the whole machine" kind of valid.
If you're really worried about it ever triggering, then dammit, HANDLE THE CASE.
Don't add a BUG_ON() for something you're afraid of. That is NEVER the
right thing to do. If you're worried that that situation can trigger,
then do the right code for that situation. Don't throw your hands in
the air and say "that's a bug".
Linus
On Wed, Jun 15, 2011 at 3:32 PM, Linus Torvalds
<[email protected]> wrote:
>
> If you're really worried about it ever triggering, then dammit, HANDLE THE CASE.
Like I said, I'll send a patch to handle the case.
--Andy