Re: PATCH: Add x32 arch_prctl support

[Roland McGrath <roland at hack dot frob dot com>]

> 	* sysdeps/unix/sysv/linux/x86_64/x32/Makefile (sysdep_routines):
> 	Add arch_prctl.
> 
> 	* sysdeps/unix/sysv/linux/x86_64/x32/arch_prctl.c: New file.
> 	* sysdeps/unix/sysv/linux/x86_64/x32/ptrace.c: Likewise.

No blank line.

> +/* Since x32 arch_prctl stores 32bit base address of segment register %fs
> +   and %gs as unsigned 64bit value via ARCH_GET_FS and ARCH_GET_GS, we
> +   use a local unsigned 64bit variable to hold the base address and copy
> +   it to ADDR after arch_prctl return.  */

"32-bit", "64-bit".

> +int
> +__arch_prctl (int code, unsigned long *addr)

Never use 'long', always 'long int'.  Here I think it's more useful just to
use uintptr_t.

> +  unsigned long long base_addr;
> +  unsigned long *addr_saved;

This is probably going to get a "might be used uninitialized" warning.

Anyway, I think it's cleaner to pass an 'unsigned long long int' (or
uint64_t) to the syscall, since that's what it is in the kernel.
Might as well just make it:

  uint64_t addr64 = (uintptr_t) addr;

> +  switch (code)
> +    {
> +    case ARCH_GET_FS:
> +    case ARCH_GET_GS:
> +      addr_saved = addr;
> +      addr = (unsigned long *) &base_addr;
> +      break;

Then here it's just:

      addr64 = (uintptr_t) &addr64;

> +    default:
> +      break;

No need for the default case (below too).

> +  if (res == 0)
> +    switch (code)
> +      {
> +      case ARCH_GET_FS:
> +      case ARCH_GET_GS:
> +	*addr_saved = (unsigned long) base_addr;
> +	break;

And here just:

      *addr = addr64;

But I think it would be wise to do some sanity check for a too-large value.
e.g.

      /* Check for a large value that overflows.  */
      if ((uintptr_t) addr64 != addr64)
	{
	  errno = EOVERFLOW;
	  return -1;
	}

> +weak_alias (__arch_prctl, arch_prctl);

No trailing ; after that macro.

> +#include <errno.h>
> +#include <sys/types.h>
> +#define ptrace __redirect_ptrace
> +#include <sys/ptrace.h>
> +#undef ptrace
> +#include <sys/syscall.h>
> +#include <sysdep.h>

If this funny business were warranted, it would need some comments.
But it's not.  Just use stdarg like linux/ptrace.c does.

> +#ifndef PTRACE_ARCH_PRCTL
> +#define PTRACE_ARCH_PRCTL      30
> +#endif

This should never be required.  Just add it to ptrace.h.
If it were, "# define" inside #if.

> +/* Since x32 ptrace stores 32bit base address of segment register %fs
> +   and %gs as unsigned 64bit value via ARCH_GET_FS and ARCH_GET_GS with
> +   PTRACE_ARCH_PRCTL, we use a local unsigned 64bit variable to hold
> +   the base address and copy it to ADDR after ptrace return.  */

"32-bit", "64-bit".

> +long

Always 'long int', never 'long'.

> +  switch ((int) request)
> +    {

There's no need to cast to int when you have a default case anyway.


But actually, PTRACE_ARCH_PRCTL should be entirely obsolete anyway.
You should just drop it from the x32 kernel.  Since many kernel
versions ago, you can just use the fs_base and gs_base fields in
user_regs_struct via the other ptrace calls.


Thanks,
Roland