Re: [PATCH 3/4] Add 32 bit VDSO time support for 32 bit kernel

[Andy Lutomirski]

On Sat, Feb 1, 2014 at 7:32 AM,  <stefani@xxxxxxxxxxx> wrote:
> From: Stefani Seibold <stefani@xxxxxxxxxxx>
>
> This patch add the time support for 32 bit a VDSO to a 32 bit kernel.
>
> For 32 bit programs running on a 32 bit kernel, the same mechanism is
> used as for 64 bit programs running on a 64 bit kernel.
>
> Signed-off-by: Stefani Seibold <stefani@xxxxxxxxxxx>

I have multiple issues with this patch.

 - It's very hard to review the vclock_gettime.c changes.  You're
doing at least three things in there: reworking the return types of
all the helpers, moving code, and adding 32-bit code.  Can you split
the patch so that the resulting diff is readable?  (e.g. one patch
that purely reorders things, one patch that reworks the return types,
and a third patch with the meat?  if splitting into just two results
in a comprehensible diff, that's fine, too.)

 - There are multiple vvar mappings.  I still don't understand why.
You've stuck the vvar page into the fixmap *and* into a special
mapping.  The former works on native 32 bit only, but you don't seem
to *use* the mapping, which confuses me.  The latter may or may not
confuse things (criu?) that try to parse /proc/self/maps.

  If it is, indeed, okay to use non-fixed maps on 32-bit, it might
also be okay on 64-bit.  If so, it could be useful to implement that,
which would remove a bit of a wart and allow PR_SET_TSC to work
usefully for 64-bit userspace.  (This would remove the need for the
VVAR macro and would allow shorter rip-relative address modes.)

 (Note that those fixmaps are a security problem on native 32-bit if
NX is not available.  We may not care.)

 - The VVAR macro is all screwed up now.  Please either continue to
use it (and fix the definition) or stop using it everywhere and figure
out a different way.  The current approach is just going to fester if
anyone ports the rest of the vdso to work in 32-bit environments.

 - You've hardcoded the address of the counter in the HPET page into
the linker script.  Please don't.  If you really need to have the HPET
mapping in the linker script, please just reference the base address
and add the offset in C code using the appropriate macro.

> ---
>  arch/x86/include/asm/elf.h            |   2 +-
>  arch/x86/include/asm/vdso.h           |   3 +
>  arch/x86/include/asm/vdso32.h         |  10 +++
>  arch/x86/vdso/Makefile                |   7 ++
>  arch/x86/vdso/vclock_gettime.c        | 165 ++++++++++++++++++++++------------
>  arch/x86/vdso/vdso-layout.lds.S       |  25 ++++++
>  arch/x86/vdso/vdso32-setup.c          |  47 ++++++++--
>  arch/x86/vdso/vdso32/vclock_gettime.c |  16 ++++
>  arch/x86/vdso/vdso32/vdso32.lds.S     |  11 ++-
>  9 files changed, 218 insertions(+), 68 deletions(-)
>  create mode 100644 arch/x86/include/asm/vdso32.h
>  create mode 100644 arch/x86/vdso/vdso32/vclock_gettime.c
>
> diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
> index 9c999c1..21bae90 100644
> --- a/arch/x86/include/asm/elf.h
> +++ b/arch/x86/include/asm/elf.h
> @@ -289,7 +289,7 @@ do {                                                                        \
>
>  #else /* CONFIG_X86_32 */
>
> -#define VDSO_HIGH_BASE         0xffffe000U /* CONFIG_COMPAT_VDSO address */
> +#define VDSO_HIGH_BASE         0xffffc000U /* CONFIG_COMPAT_VDSO address */

This is odd.  Can you explain it?


--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/