| Message ID | 20190614152908.8101-3-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [01/12] m68k: Remove vDSO support | expand |
Ping. On 14/06/2019 12:28, Adhemerval Zanella wrote: > Although s390 explicit does not enable vDSO for binaries without > (arch/s390/kernel/vdso.c:217), there is no indication in the code > the rationale for disabling it. In fact, I rebuilt a kernel with the > check removed and the vDSO does work for static build for supplied > symbols. > > Checked on s390x-linux-gnu. > > [BZ #19767] > * sysdeps/unix/sysv/linux/s390/init-first.c: Remove #ifdef SHARED. > * sysdeps/unix/sysv/linux/s390/libc-vdso.h: Likewise. > * sysdeps/unix/sysv/linux/s390/s390-32/sysdep.h > (ALWAYS_USE_VSYSCALL): Define. > * sysdeps/unix/sysv/linux/s390/s390-64/sysdep.h > (ALWAYS_USE_VSYSCALL): Likewise. > --- > sysdeps/unix/sysv/linux/s390/init-first.c | 8 ++------ > sysdeps/unix/sysv/linux/s390/libc-vdso.h | 3 --- > sysdeps/unix/sysv/linux/s390/s390-32/sysdep.h | 3 +++ > sysdeps/unix/sysv/linux/s390/s390-64/sysdep.h | 4 ++++ > 4 files changed, 9 insertions(+), 9 deletions(-) > > diff --git a/sysdeps/unix/sysv/linux/s390/init-first.c b/sysdeps/unix/sysv/linux/s390/init-first.c > index 71dd43f738..847977c0d9 100644 > --- a/sysdeps/unix/sysv/linux/s390/init-first.c > +++ b/sysdeps/unix/sysv/linux/s390/init-first.c > @@ -16,9 +16,8 @@ > License along with the GNU C Library; if not, see > <http://www.gnu.org/licenses/>. */ > > -#ifdef SHARED > -# include <dl-vdso.h> > -# include <libc-vdso.h> > +#include <dl-vdso.h> > +#include <libc-vdso.h> > > long int (*VDSO_SYMBOL(gettimeofday)) (struct timeval *, void *) > attribute_hidden; > @@ -54,7 +53,4 @@ _libc_vdso_platform_setup (void) > VDSO_SYMBOL (getcpu) = p; > } > > -# define VDSO_SETUP _libc_vdso_platform_setup > -#endif > - > #include <csu/init-first.c> > diff --git a/sysdeps/unix/sysv/linux/s390/libc-vdso.h b/sysdeps/unix/sysv/linux/s390/libc-vdso.h > index 5e2d1e4b3f..cc97601383 100644 > --- a/sysdeps/unix/sysv/linux/s390/libc-vdso.h > +++ b/sysdeps/unix/sysv/linux/s390/libc-vdso.h > @@ -20,8 +20,6 @@ > #ifndef _LIBC_VDSO_H > #define _LIBC_VDSO_H > > -#ifdef SHARED > - > #include <sysdep-vdso.h> > > extern long int (*VDSO_SYMBOL(gettimeofday)) (struct timeval *, void *) > @@ -33,6 +31,5 @@ extern long int (*VDSO_SYMBOL(clock_getres)) (clockid_t, struct timespec *); > > extern long int (*VDSO_SYMBOL(getcpu)) (unsigned *, unsigned *, void *) > attribute_hidden; > -#endif > > #endif /* _LIBC_VDSO_H */ > diff --git a/sysdeps/unix/sysv/linux/s390/s390-32/sysdep.h b/sysdeps/unix/sysv/linux/s390/s390-32/sysdep.h > index 640fb52de1..47d1c5cdb2 100644 > --- a/sysdeps/unix/sysv/linux/s390/s390-32/sysdep.h > +++ b/sysdeps/unix/sysv/linux/s390/s390-32/sysdep.h > @@ -19,6 +19,9 @@ > #ifndef _LINUX_S390_SYSDEP_H > #define _LINUX_S390_SYSDEP_H > > +/* Always enable vsyscalls on s390-32. */ > +#define ALWAYS_USE_VSYSCALL 1 > + > #include <sysdeps/s390/s390-32/sysdep.h> > #include <sysdeps/unix/sysdep.h> > #include <sysdeps/unix/sysv/linux/sysdep.h> > diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/sysdep.h b/sysdeps/unix/sysv/linux/s390/s390-64/sysdep.h > index 9a9834c750..95829390e2 100644 > --- a/sysdeps/unix/sysv/linux/s390/s390-64/sysdep.h > +++ b/sysdeps/unix/sysv/linux/s390/s390-64/sysdep.h > @@ -20,6 +20,10 @@ > #ifndef _LINUX_S390_SYSDEP_H > #define _LINUX_S390_SYSDEP_H > > +/* Always enable vsyscalls on s390-64. */ > +#define ALWAYS_USE_VSYSCALL 1 > + > + > #include <sysdeps/s390/s390-64/sysdep.h> > #include <sysdeps/unix/sysdep.h> > #include <sysdeps/unix/sysv/linux/sysdep.h> >
On 8/2/19 2:34 PM, Adhemerval Zanella wrote: > Ping. > > On 14/06/2019 12:28, Adhemerval Zanella wrote: >> Although s390 explicit does not enable vDSO for binaries without >> (arch/s390/kernel/vdso.c:217), there is no indication in the code >> the rationale for disabling it. In fact, I rebuilt a kernel with the >> check removed and the vDSO does work for static build for supplied >> symbols. Hi Adhemerval, Sorry, I haven't recognized this patch as it was hidden behind the email-thread starting with "[PATCH 01/12] m68k: Remove vDSO support". Please cc me if I'm not responding for s390 patches. I've just had a look into the kernel arch/s390/kernel/vdso.c file and I'm a little bit confused regarding the line number 217 (see my excerpt below). Just to be sure, have you removed the "if (!uses_interp)" or the "if (!vdso_enabled)"? /* * This is called from binfmt_elf, we create the special vma for the * vDSO and insert it into the mm struct tree */ int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp) { struct mm_struct *mm = current->mm; struct vm_area_struct *vma; unsigned long vdso_pages; unsigned long vdso_base; int rc; 217: if (!vdso_enabled) return 0; /* * Only map the vdso for dynamically linked elf binaries. */ 222: if (!uses_interp) return 0; As far as I know, vdso was disabled for statically linked binaries as glibc had not supported it and thus nothing "useless" was mapped (especially on 31bit). I've also cc'ed Heiko. Perhaps he can also enable it on s390 for statically linked binaries. Nevertheless this patch breaks s390. I've just committed the attached fix which adds the removed VDSO_SETUP macro definition. Bye. Stefan commit cf6ac72fdd2026e91c0672ccf0eefd2e58920bdc Author: Stefan Liebler <stli@linux.ibm.com> Date: Tue Aug 6 14:15:09 2019 +0200 s390: Fix Enable VDSO for static linking The commit 5e855c8954014bca7b0d6f07312ec09553695ffd "s390: Enable VDSO for static linking" removed the definition of VDSO_SETUP which leads to not setup the vdso symbols. Instead it jumps to false addresses. This patch just re adds the removed VDSO_SETUP macro definition. ChangeLog: * sysdeps/unix/sysv/linux/s390/init-first.c (VDSO_SETUP): New define. diff --git a/sysdeps/unix/sysv/linux/s390/init-first.c b/sysdeps/unix/sysv/linux/s390/init-first.c index 8c54d13935..a1ad9458e3 100644 --- a/sysdeps/unix/sysv/linux/s390/init-first.c +++ b/sysdeps/unix/sysv/linux/s390/init-first.c @@ -53,4 +53,6 @@ _libc_vdso_platform_setup (void) VDSO_SYMBOL (getcpu) = p; } +#define VDSO_SETUP _libc_vdso_platform_setup + #include <csu/init-first.c>
On 06/08/2019 11:08, Stefan Liebler wrote: > On 8/2/19 2:34 PM, Adhemerval Zanella wrote: >> Ping. >> >> On 14/06/2019 12:28, Adhemerval Zanella wrote: >>> Although s390 explicit does not enable vDSO for binaries without >>> (arch/s390/kernel/vdso.c:217), there is no indication in the code >>> the rationale for disabling it. In fact, I rebuilt a kernel with the >>> check removed and the vDSO does work for static build for supplied >>> symbols. > > Hi Adhemerval, > > Sorry, I haven't recognized this patch as it was hidden behind the email-thread starting with "[PATCH 01/12] m68k: Remove vDSO support". Please cc me if I'm not responding for s390 patches. > > I've just had a look into the kernel arch/s390/kernel/vdso.c file > and I'm a little bit confused regarding the line number 217 (see my excerpt below). > Just to be sure, have you removed the "if (!uses_interp)" or the "if (!vdso_enabled)"? > > /* > * This is called from binfmt_elf, we create the special vma for the > * vDSO and insert it into the mm struct tree > */ > int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp) > { > struct mm_struct *mm = current->mm; > struct vm_area_struct *vma; > unsigned long vdso_pages; > unsigned long vdso_base; > int rc; > > 217: if (!vdso_enabled) > return 0; > /* > * Only map the vdso for dynamically linked elf binaries. > */ > 222: if (!uses_interp) > return 0; In fact it was the !uses_interp that I commented out to enable the vDSO page on the system emulated environment I used. > > > As far as I know, vdso was disabled for statically linked binaries as glibc had not supported it and thus nothing "useless" was mapped (especially on 31bit). > I've also cc'ed Heiko. Perhaps he can also enable it on s390 for statically linked binaries. Yeah, I noted that s390 is an outlier here where all the other architectures do map the vDSO even for statically linked binaries. Thanks for checking on it. > > Nevertheless this patch breaks s390. I've just committed the attached fix which adds the removed VDSO_SETUP macro definition. Sigh, this line was removed with a cherry-pick, sorry about that.
On Tue, Aug 06, 2019 at 04:08:02PM +0200, Stefan Liebler wrote: > int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp) > { > struct mm_struct *mm = current->mm; > struct vm_area_struct *vma; > unsigned long vdso_pages; > unsigned long vdso_base; > int rc; > > 217: if (!vdso_enabled) > return 0; > /* > * Only map the vdso for dynamically linked elf binaries. > */ > 222: if (!uses_interp) > return 0; > > > As far as I know, vdso was disabled for statically linked binaries as glibc > had not supported it and thus nothing "useless" was mapped (especially on > 31bit). > I've also cc'ed Heiko. Perhaps he can also enable it on s390 for statically > linked binaries. That was introduced in the kernel with commit fc5243d98ac2575ad14a974b3c097e9ba874c03d Author: Martin Schwidefsky <schwidefsky@de.ibm.com> Date: Thu Dec 25 13:38:35 2008 +0100 [S390] arch_setup_additional_pages arguments arch_setup_additional_pages currently gets two arguments, the binary format descripton and an indication if the process uses an executable stack or not. The second argument is not used by anybody, it could be removed without replacement. What actually does make sense is to pass an indication if the process uses the elf interpreter or not. The glibc code will not use anything from the vdso if the process does not use the dynamic linker, so for statically linked binaries the architecture backend can choose not to map the vdso. Acked-by: Ingo Molnar <mingo@elte.hu> Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com> And as it looks like only s390 ever made use of this optimization, which now isn't an optimization anymore. So this can simply be removed.
diff --git a/sysdeps/unix/sysv/linux/s390/init-first.c b/sysdeps/unix/sysv/linux/s390/init-first.c index 71dd43f738..847977c0d9 100644 --- a/sysdeps/unix/sysv/linux/s390/init-first.c +++ b/sysdeps/unix/sysv/linux/s390/init-first.c @@ -16,9 +16,8 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ -#ifdef SHARED -# include <dl-vdso.h> -# include <libc-vdso.h> +#include <dl-vdso.h> +#include <libc-vdso.h> long int (*VDSO_SYMBOL(gettimeofday)) (struct timeval *, void *) attribute_hidden; @@ -54,7 +53,4 @@ _libc_vdso_platform_setup (void) VDSO_SYMBOL (getcpu) = p; } -# define VDSO_SETUP _libc_vdso_platform_setup -#endif - #include <csu/init-first.c> diff --git a/sysdeps/unix/sysv/linux/s390/libc-vdso.h b/sysdeps/unix/sysv/linux/s390/libc-vdso.h index 5e2d1e4b3f..cc97601383 100644 --- a/sysdeps/unix/sysv/linux/s390/libc-vdso.h +++ b/sysdeps/unix/sysv/linux/s390/libc-vdso.h @@ -20,8 +20,6 @@ #ifndef _LIBC_VDSO_H #define _LIBC_VDSO_H -#ifdef SHARED - #include <sysdep-vdso.h> extern long int (*VDSO_SYMBOL(gettimeofday)) (struct timeval *, void *) @@ -33,6 +31,5 @@ extern long int (*VDSO_SYMBOL(clock_getres)) (clockid_t, struct timespec *); extern long int (*VDSO_SYMBOL(getcpu)) (unsigned *, unsigned *, void *) attribute_hidden; -#endif #endif /* _LIBC_VDSO_H */ diff --git a/sysdeps/unix/sysv/linux/s390/s390-32/sysdep.h b/sysdeps/unix/sysv/linux/s390/s390-32/sysdep.h index 640fb52de1..47d1c5cdb2 100644 --- a/sysdeps/unix/sysv/linux/s390/s390-32/sysdep.h +++ b/sysdeps/unix/sysv/linux/s390/s390-32/sysdep.h @@ -19,6 +19,9 @@ #ifndef _LINUX_S390_SYSDEP_H #define _LINUX_S390_SYSDEP_H +/* Always enable vsyscalls on s390-32. */ +#define ALWAYS_USE_VSYSCALL 1 + #include <sysdeps/s390/s390-32/sysdep.h> #include <sysdeps/unix/sysdep.h> #include <sysdeps/unix/sysv/linux/sysdep.h> diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/sysdep.h b/sysdeps/unix/sysv/linux/s390/s390-64/sysdep.h index 9a9834c750..95829390e2 100644 --- a/sysdeps/unix/sysv/linux/s390/s390-64/sysdep.h +++ b/sysdeps/unix/sysv/linux/s390/s390-64/sysdep.h @@ -20,6 +20,10 @@ #ifndef _LINUX_S390_SYSDEP_H #define _LINUX_S390_SYSDEP_H +/* Always enable vsyscalls on s390-64. */ +#define ALWAYS_USE_VSYSCALL 1 + + #include <sysdeps/s390/s390-64/sysdep.h> #include <sysdeps/unix/sysdep.h> #include <sysdeps/unix/sysv/linux/sysdep.h>