Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 15 May 2018 13:39:23 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH 0/9] linux v4.16 update

On Sat, Apr 28, 2018 at 09:56:56PM +0200, Szabolcs Nagy wrote:
> the ptrace.h change is probably not ok
> (glibc added struct __ptrace_seccomp_metadata
> instead of the kernel struct seccomp_metadata,
> but the same is true for ptrace_peeksiginfo_args
> where musl currently follows linux instead of
> glibc, maybe that should be fixed?).

I'm not sure. The ptrace-related (including ptrace.h, user.h, etc.)
stuff has always been a mess.

> the last patch is an unfinished proposal to add
> some new syscalls glibc already has, but i ran
> into some issues so comments are welcome.
> 
> i ran the patch set through compile testing.
> 
> Szabolcs Nagy (9):
>   siginfo struct change following linux v4.16
>   elf.h: add NT_PPC_PKEY from linux v4.16
>   sys/epoll.h: add EPOLLNVAL from linux v4.16
>   netinet/if_ether.h: add ETH_P_ERSPAN2 from linux v4.16
>   netinet/if_ether.h: add ETH_TLEN from linux v4.16
>   sys/ptrace.h: add PTRACE_SECCOMP_GET_METADATA from linux v4.16
>   aarch64: add HWCAP_ASIMDFHM from linux v4.16
>   powerpc: add pkey syscall numbers from linux v4.16
>   [RFC] add new memory mapping related apis
> 
>  arch/aarch64/bits/hwcap.h        |  1 +
>  arch/powerpc/bits/mman.h         |  4 ++++
>  arch/powerpc/bits/syscall.h.in   |  3 +++
>  arch/powerpc64/bits/mman.h       |  4 ++++
>  arch/powerpc64/bits/syscall.h.in |  3 +++
>  include/elf.h                    |  1 +
>  include/netinet/if_ether.h       |  2 ++
>  include/signal.h                 | 12 ++++++++----
>  include/sys/epoll.h              |  1 +
>  include/sys/mman.h               | 24 +++++++++++++++++++++---
>  include/sys/ptrace.h             |  6 ++++++
>  src/linux/memfd_create.c         | 10 ++++++++++
>  src/linux/mlock2.c               | 15 +++++++++++++++
>  src/linux/pkey_alloc.c           | 15 +++++++++++++++
>  src/linux/pkey_get.c             |  9 +++++++++
>  src/linux/pkey_mprotect.c        | 15 +++++++++++++++
>  src/linux/pkey_set.c             |  9 +++++++++
>  17 files changed, 127 insertions(+), 7 deletions(-)
>  create mode 100644 src/linux/memfd_create.c
>  create mode 100644 src/linux/mlock2.c
>  create mode 100644 src/linux/pkey_alloc.c
>  create mode 100644 src/linux/pkey_get.c
>  create mode 100644 src/linux/pkey_mprotect.c
>  create mode 100644 src/linux/pkey_set.c
> 
> -- 
> 2.16.3
> 

> From f7eb8934396890e39b9e5e2b4fdd1534f1c024cc Mon Sep 17 00:00:00 2001
> From: Szabolcs Nagy <nsz@...t70.net>
> Date: Mon, 16 Apr 2018 22:16:29 +0000
> Subject: [PATCH 1/9] siginfo struct change following linux v4.16
> 
> this is supposed to be a cosmetic change only, not affecting api or abi,
> follows linux commit b68a68d3dcc15ebbf23cbe91af1abf57591bd96b and
> 859d880cf544dbe095ce97534ef04cd88ba2f2b4 with slightly different field
> names to follow musl conventions (which must be different to avoid
> depending on anonymous union support and polluting the namespace).
> ---
>  include/signal.h | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/include/signal.h b/include/signal.h
> index a4f85cca..d68331e8 100644
> --- a/include/signal.h
> +++ b/include/signal.h
> @@ -123,13 +123,17 @@ typedef struct {
>  		} __si_common;
>  		struct {
>  			void *si_addr;
> -			short si_addr_lsb;
>  			union {
> +				short si_addr_lsb;
>  				struct {
> +					void *__dummy_bnd;
>  					void *si_lower;
>  					void *si_upper;
>  				} __addr_bnd;
> -				unsigned si_pkey;
> +				struct {
> +					void *__dummy_pkey;
> +					unsigned si_pkey;
> +				} __addr_pkey;
>  			} __first;
>  		} __sigfault;
>  		struct {
> @@ -150,10 +154,10 @@ typedef struct {
>  #define si_stime   __si_fields.__si_common.__second.__sigchld.si_stime
>  #define si_value   __si_fields.__si_common.__second.si_value
>  #define si_addr    __si_fields.__sigfault.si_addr
> -#define si_addr_lsb __si_fields.__sigfault.si_addr_lsb
> +#define si_addr_lsb __si_fields.__sigfault.__first.si_addr_lsb
>  #define si_lower   __si_fields.__sigfault.__first.__addr_bnd.si_lower
>  #define si_upper   __si_fields.__sigfault.__first.__addr_bnd.si_upper
> -#define si_pkey    __si_fields.__sigfault.__first.si_pkey
> +#define si_pkey    __si_fields.__sigfault.__first.__addr_pkey.si_pkey
>  #define si_band    __si_fields.__sigpoll.si_band
>  #define si_fd      __si_fields.__sigpoll.si_fd
>  #define si_timerid __si_fields.__si_common.__first.__timer.si_timerid

Is there any benefit to making this change?

> From de4ca3284d759563322c795479601e9eee394832 Mon Sep 17 00:00:00 2001
> From: Szabolcs Nagy <nsz@...t70.net>
> Date: Sat, 28 Apr 2018 17:25:41 +0000
> Subject: [PATCH 9/9] [RFC] add new memory mapping related apis
> 
> memfd_create (linux v3.17)
> mlock2 (linux v4.4)
> pkey_alloc (linux v4.9)
> pkey_free (linux v4.9)
> pkey_mprotect (linux v4.9)
> pkey_get (glibc 2.27)
> pkey_set (glibc 2.27)
> 
> notes:
> 
> - pkey_alloc type is inconsistent between the manual and
>   the glibc source (unsigned int vs unsigned long args)

This needs to be resolved, I think.

> - pkey_get / pkey_set are not syscalls (query/update the
>   pkru register on x86), not implemented yet.
> - fallback in mlock2 to mlock and pkey_mprotect to mprotect
>   follows glibc.
> - mlock2 EINVAL return means invalid (or unsupported) flags.
> - moved MLOCK_ONFAULT under _GNU_SOURCE following glibc.
> ---
>  arch/powerpc/bits/mman.h   |  4 ++++
>  arch/powerpc64/bits/mman.h |  4 ++++
>  include/sys/mman.h         | 24 +++++++++++++++++++++---
>  src/linux/memfd_create.c   | 10 ++++++++++
>  src/linux/mlock2.c         | 15 +++++++++++++++
>  src/linux/pkey_alloc.c     | 15 +++++++++++++++
>  src/linux/pkey_get.c       |  9 +++++++++
>  src/linux/pkey_mprotect.c  | 15 +++++++++++++++
>  src/linux/pkey_set.c       |  9 +++++++++
>  9 files changed, 102 insertions(+), 3 deletions(-)

Am I correct in assuming the header & external function interfaces
match how glibc is exposing these functions?

One thing that looks a little bit weird to me is conditioning
existence of some functions on the syscall macro:

> diff --git a/src/linux/pkey_alloc.c b/src/linux/pkey_alloc.c
> new file mode 100644
> index 00000000..0b0770a6
> --- /dev/null
> +++ b/src/linux/pkey_alloc.c
> @@ -0,0 +1,15 @@
> +#include "syscall.h"
> +
> +#ifdef SYS_pkey_alloc
> +#define _GNU_SOURCE 1
> +#include <sys/mman.h>
> +int pkey_alloc(unsigned flags, unsigned access)
> +{
> +	return syscall(SYS_pkey_alloc, flags, access);
> +}
> +
> +int pkey_free(int pkey)
> +{
> +	return syscall(SYS_pkey_free, pkey);
> +}
> +#endif

where others fail with ENOSYS when the arch has no definition. I think
it's also problematic that syscall.h is being included before #define
_GNU_SOURCE. This would break under changes in how the features.h
mechanisms work.

Is there a reason you want to avoid an external functon existing when
the arch lacks the feature?

The other patches in this series look ok I think. I'll commit them
soon if I don't see any further things to ask about.

Rich

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.