Messages in this thread | | | Date | Mon, 13 Jun 2011 10:54:04 +0600 | Subject | Re: [PATCH] x86, vsyscall: Fix build warning in vsyscall_64.c | From | Rakib Mullick <> |
| |
On Mon, Jun 13, 2011 at 8:52 AM, Andrew Lutomirski <luto@mit.edu> wrote: > On Sun, Jun 12, 2011 at 1:12 AM, Rakib Mullick <rakib.mullick@gmail.com> wrote: >> On Sat, Jun 11, 2011 at 5:01 PM, Andrew Lutomirski <luto@mit.edu> wrote: >>> On Sat, Jun 11, 2011 at 3:31 AM, Rakib Mullick <rakib.mullick@gmail.com> 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |