| Message ID | 20200313154345.56760-19-vincenzo.frascino@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce common headers for vDSO | expand |
On Fri, Mar 13, 2020 at 03:43:37PM +0000, Vincenzo Frascino wrote: > --- /dev/null > +++ b/arch/arm64/include/asm/vdso/processor.h > @@ -0,0 +1,31 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright (C) 2020 ARM Ltd. > + */ > +#ifndef __ASM_VDSO_PROCESSOR_H > +#define __ASM_VDSO_PROCESSOR_H > + > +#ifndef __ASSEMBLY__ > + > +#include <asm/page-def.h> > + > +#ifdef CONFIG_COMPAT > +#if defined(CONFIG_ARM64_64K_PAGES) && defined(CONFIG_KUSER_HELPERS) > +/* > + * With CONFIG_ARM64_64K_PAGES enabled, the last page is occupied > + * by the compat vectors page. > + */ > +#define TASK_SIZE_32 UL(0x100000000) > +#else > +#define TASK_SIZE_32 (UL(0x100000000) - PAGE_SIZE) > +#endif /* CONFIG_ARM64_64K_PAGES */ > +#endif /* CONFIG_COMPAT */ Just curious, what's TASK_SIZE_32 used for in the vDSO code? You don't seem to move TASK_SIZE.
Hi Catalin, you should not really work on Sunday ;-) Joking, thanks for reviewing my patches. On 3/15/20 6:30 PM, Catalin Marinas wrote: > On Fri, Mar 13, 2020 at 03:43:37PM +0000, Vincenzo Frascino wrote: >> --- /dev/null >> +++ b/arch/arm64/include/asm/vdso/processor.h >> @@ -0,0 +1,31 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> +/* >> + * Copyright (C) 2020 ARM Ltd. >> + */ >> +#ifndef __ASM_VDSO_PROCESSOR_H >> +#define __ASM_VDSO_PROCESSOR_H >> + >> +#ifndef __ASSEMBLY__ >> + >> +#include <asm/page-def.h> >> + >> +#ifdef CONFIG_COMPAT >> +#if defined(CONFIG_ARM64_64K_PAGES) && defined(CONFIG_KUSER_HELPERS) >> +/* >> + * With CONFIG_ARM64_64K_PAGES enabled, the last page is occupied >> + * by the compat vectors page. >> + */ >> +#define TASK_SIZE_32 UL(0x100000000) >> +#else >> +#define TASK_SIZE_32 (UL(0x100000000) - PAGE_SIZE) >> +#endif /* CONFIG_ARM64_64K_PAGES */ >> +#endif /* CONFIG_COMPAT */ > > Just curious, what's TASK_SIZE_32 used for in the vDSO code? You don't > seem to move TASK_SIZE. > I tried to fine grain the headers as much as I could in order to avoid unneeded/unwanted inclusions: * TASK_SIZE_32 is used to verify ABI consistency on vdso32 (please refer to arch/arm64/kernel/vdso32/vgettimeofday.c). * TASK_SIZE is not required by the vdso library hence it is not present in these headers.
Hi Vincenzo, On Mon, Mar 16, 2020 at 09:42:32AM +0000, Vincenzo Frascino wrote: > On 3/15/20 6:30 PM, Catalin Marinas wrote: > > On Fri, Mar 13, 2020 at 03:43:37PM +0000, Vincenzo Frascino wrote: > >> --- /dev/null > >> +++ b/arch/arm64/include/asm/vdso/processor.h > >> @@ -0,0 +1,31 @@ > >> +/* SPDX-License-Identifier: GPL-2.0-only */ > >> +/* > >> + * Copyright (C) 2020 ARM Ltd. > >> + */ > >> +#ifndef __ASM_VDSO_PROCESSOR_H > >> +#define __ASM_VDSO_PROCESSOR_H > >> + > >> +#ifndef __ASSEMBLY__ > >> + > >> +#include <asm/page-def.h> > >> + > >> +#ifdef CONFIG_COMPAT > >> +#if defined(CONFIG_ARM64_64K_PAGES) && defined(CONFIG_KUSER_HELPERS) > >> +/* > >> + * With CONFIG_ARM64_64K_PAGES enabled, the last page is occupied > >> + * by the compat vectors page. > >> + */ > >> +#define TASK_SIZE_32 UL(0x100000000) > >> +#else > >> +#define TASK_SIZE_32 (UL(0x100000000) - PAGE_SIZE) > >> +#endif /* CONFIG_ARM64_64K_PAGES */ > >> +#endif /* CONFIG_COMPAT */ > > > > Just curious, what's TASK_SIZE_32 used for in the vDSO code? You don't > > seem to move TASK_SIZE. > > > > I tried to fine grain the headers as much as I could in order to avoid > unneeded/unwanted inclusions: > * TASK_SIZE_32 is used to verify ABI consistency on vdso32 (please refer to > arch/arm64/kernel/vdso32/vgettimeofday.c). > * TASK_SIZE is not required by the vdso library hence it is not present in > these headers. It would be worth noting the former point in the commit message, since it can be surprising. I also think it's worth keeping the definitions together if that's easy, as it makes it easier to navigate the codebase, even if TASK_SIZE isn't necessary for the VDSO itself. Thanks, Mark.
On Mon, Mar 16, 2020 at 10:22:15AM +0000, Mark Rutland wrote: > On Mon, Mar 16, 2020 at 09:42:32AM +0000, Vincenzo Frascino wrote: > > On 3/15/20 6:30 PM, Catalin Marinas wrote: > > > On Fri, Mar 13, 2020 at 03:43:37PM +0000, Vincenzo Frascino wrote: > > >> --- /dev/null > > >> +++ b/arch/arm64/include/asm/vdso/processor.h > > >> @@ -0,0 +1,31 @@ > > >> +/* SPDX-License-Identifier: GPL-2.0-only */ > > >> +/* > > >> + * Copyright (C) 2020 ARM Ltd. > > >> + */ > > >> +#ifndef __ASM_VDSO_PROCESSOR_H > > >> +#define __ASM_VDSO_PROCESSOR_H > > >> + > > >> +#ifndef __ASSEMBLY__ > > >> + > > >> +#include <asm/page-def.h> > > >> + > > >> +#ifdef CONFIG_COMPAT > > >> +#if defined(CONFIG_ARM64_64K_PAGES) && defined(CONFIG_KUSER_HELPERS) > > >> +/* > > >> + * With CONFIG_ARM64_64K_PAGES enabled, the last page is occupied > > >> + * by the compat vectors page. > > >> + */ > > >> +#define TASK_SIZE_32 UL(0x100000000) > > >> +#else > > >> +#define TASK_SIZE_32 (UL(0x100000000) - PAGE_SIZE) > > >> +#endif /* CONFIG_ARM64_64K_PAGES */ > > >> +#endif /* CONFIG_COMPAT */ > > > > > > Just curious, what's TASK_SIZE_32 used for in the vDSO code? You don't > > > seem to move TASK_SIZE. > > > > > > > I tried to fine grain the headers as much as I could in order to avoid > > unneeded/unwanted inclusions: > > * TASK_SIZE_32 is used to verify ABI consistency on vdso32 (please refer to > > arch/arm64/kernel/vdso32/vgettimeofday.c). > > * TASK_SIZE is not required by the vdso library hence it is not present in > > these headers. > > It would be worth noting the former point in the commit message, since > it can be surprising. > > I also think it's worth keeping the definitions together if that's easy, > as it makes it easier to navigate the codebase, even if TASK_SIZE isn't > necessary for the VDSO itself. It won't work as TASK_SIZE requires (on arm64) test_thread_flag() which can't be made available to the vDSO.
On 3/16/20 10:22 AM, Mark Rutland wrote: > Hi Vincenzo, > > On Mon, Mar 16, 2020 at 09:42:32AM +0000, Vincenzo Frascino wrote: >> On 3/15/20 6:30 PM, Catalin Marinas wrote: >>> On Fri, Mar 13, 2020 at 03:43:37PM +0000, Vincenzo Frascino wrote: >>>> --- /dev/null >>>> +++ b/arch/arm64/include/asm/vdso/processor.h >>>> @@ -0,0 +1,31 @@ >>>> +/* SPDX-License-Identifier: GPL-2.0-only */ >>>> +/* >>>> + * Copyright (C) 2020 ARM Ltd. >>>> + */ >>>> +#ifndef __ASM_VDSO_PROCESSOR_H >>>> +#define __ASM_VDSO_PROCESSOR_H >>>> + >>>> +#ifndef __ASSEMBLY__ >>>> + >>>> +#include <asm/page-def.h> >>>> + >>>> +#ifdef CONFIG_COMPAT >>>> +#if defined(CONFIG_ARM64_64K_PAGES) && defined(CONFIG_KUSER_HELPERS) >>>> +/* >>>> + * With CONFIG_ARM64_64K_PAGES enabled, the last page is occupied >>>> + * by the compat vectors page. >>>> + */ >>>> +#define TASK_SIZE_32 UL(0x100000000) >>>> +#else >>>> +#define TASK_SIZE_32 (UL(0x100000000) - PAGE_SIZE) >>>> +#endif /* CONFIG_ARM64_64K_PAGES */ >>>> +#endif /* CONFIG_COMPAT */ >>> >>> Just curious, what's TASK_SIZE_32 used for in the vDSO code? You don't >>> seem to move TASK_SIZE. >>> >> >> I tried to fine grain the headers as much as I could in order to avoid >> unneeded/unwanted inclusions: >> * TASK_SIZE_32 is used to verify ABI consistency on vdso32 (please refer to >> arch/arm64/kernel/vdso32/vgettimeofday.c). >> * TASK_SIZE is not required by the vdso library hence it is not present in >> these headers. > > It would be worth noting the former point in the commit message, since > it can be surprising. > Sure it is a good point, I will add this to the commit message. > I also think it's worth keeping the definitions together if that's easy, > as it makes it easier to navigate the codebase, even if TASK_SIZE isn't > necessary for the VDSO itself. This can't be done because TASK_SIZE on arm64 requires test_thread_flag() with is not suited for vDSO. In other words can cause the same problem we are trying to solve. > > Thanks, > Mark. >
On Mon, Mar 16, 2020 at 10:26:22AM +0000, Catalin Marinas wrote: > On Mon, Mar 16, 2020 at 10:22:15AM +0000, Mark Rutland wrote: > > On Mon, Mar 16, 2020 at 09:42:32AM +0000, Vincenzo Frascino wrote: > > > On 3/15/20 6:30 PM, Catalin Marinas wrote: > > > > On Fri, Mar 13, 2020 at 03:43:37PM +0000, Vincenzo Frascino wrote: > > > >> +#ifdef CONFIG_COMPAT > > > >> +#if defined(CONFIG_ARM64_64K_PAGES) && defined(CONFIG_KUSER_HELPERS) > > > >> +/* > > > >> + * With CONFIG_ARM64_64K_PAGES enabled, the last page is occupied > > > >> + * by the compat vectors page. > > > >> + */ > > > >> +#define TASK_SIZE_32 UL(0x100000000) > > > >> +#else > > > >> +#define TASK_SIZE_32 (UL(0x100000000) - PAGE_SIZE) > > > >> +#endif /* CONFIG_ARM64_64K_PAGES */ > > > >> +#endif /* CONFIG_COMPAT */ > > > > > > > > Just curious, what's TASK_SIZE_32 used for in the vDSO code? You don't > > > > seem to move TASK_SIZE. > > > > > > I tried to fine grain the headers as much as I could in order to avoid > > > unneeded/unwanted inclusions: > > > * TASK_SIZE_32 is used to verify ABI consistency on vdso32 (please refer to > > > arch/arm64/kernel/vdso32/vgettimeofday.c). > > > * TASK_SIZE is not required by the vdso library hence it is not present in > > > these headers. > > > > It would be worth noting the former point in the commit message, since > > it can be surprising. > > > > I also think it's worth keeping the definitions together if that's easy, > > as it makes it easier to navigate the codebase, even if TASK_SIZE isn't > > necessary for the VDSO itself. > > It won't work as TASK_SIZE requires (on arm64) test_thread_flag() which > can't be made available to the vDSO. Ah; fair enough. Thanks, Mark.
On 3/16/20 10:26 AM, Catalin Marinas wrote: > On Mon, Mar 16, 2020 at 10:22:15AM +0000, Mark Rutland wrote: >> On Mon, Mar 16, 2020 at 09:42:32AM +0000, Vincenzo Frascino wrote: >>> On 3/15/20 6:30 PM, Catalin Marinas wrote: >>>> On Fri, Mar 13, 2020 at 03:43:37PM +0000, Vincenzo Frascino wrote: >>>>> --- /dev/null >>>>> +++ b/arch/arm64/include/asm/vdso/processor.h >>>>> @@ -0,0 +1,31 @@ >>>>> +/* SPDX-License-Identifier: GPL-2.0-only */ >>>>> +/* >>>>> + * Copyright (C) 2020 ARM Ltd. >>>>> + */ >>>>> +#ifndef __ASM_VDSO_PROCESSOR_H >>>>> +#define __ASM_VDSO_PROCESSOR_H >>>>> + >>>>> +#ifndef __ASSEMBLY__ >>>>> + >>>>> +#include <asm/page-def.h> >>>>> + >>>>> +#ifdef CONFIG_COMPAT >>>>> +#if defined(CONFIG_ARM64_64K_PAGES) && defined(CONFIG_KUSER_HELPERS) >>>>> +/* >>>>> + * With CONFIG_ARM64_64K_PAGES enabled, the last page is occupied >>>>> + * by the compat vectors page. >>>>> + */ >>>>> +#define TASK_SIZE_32 UL(0x100000000) >>>>> +#else >>>>> +#define TASK_SIZE_32 (UL(0x100000000) - PAGE_SIZE) >>>>> +#endif /* CONFIG_ARM64_64K_PAGES */ >>>>> +#endif /* CONFIG_COMPAT */ >>>> >>>> Just curious, what's TASK_SIZE_32 used for in the vDSO code? You don't >>>> seem to move TASK_SIZE. >>>> >>> >>> I tried to fine grain the headers as much as I could in order to avoid >>> unneeded/unwanted inclusions: >>> * TASK_SIZE_32 is used to verify ABI consistency on vdso32 (please refer to >>> arch/arm64/kernel/vdso32/vgettimeofday.c). >>> * TASK_SIZE is not required by the vdso library hence it is not present in >>> these headers. >> >> It would be worth noting the former point in the commit message, since >> it can be surprising. >> >> I also think it's worth keeping the definitions together if that's easy, >> as it makes it easier to navigate the codebase, even if TASK_SIZE isn't >> necessary for the VDSO itself. > > It won't work as TASK_SIZE requires (on arm64) test_thread_flag() which > can't be made available to the vDSO. > Ups, I did not see this :) luckily I agree ;)
On Mon, Mar 16, 2020 at 09:42:32AM +0000, Vincenzo Frascino wrote: > you should not really work on Sunday ;-) I was getting bored ;). > On 3/15/20 6:30 PM, Catalin Marinas wrote: > > On Fri, Mar 13, 2020 at 03:43:37PM +0000, Vincenzo Frascino wrote: > >> --- /dev/null > >> +++ b/arch/arm64/include/asm/vdso/processor.h > >> @@ -0,0 +1,31 @@ > >> +/* SPDX-License-Identifier: GPL-2.0-only */ > >> +/* > >> + * Copyright (C) 2020 ARM Ltd. > >> + */ > >> +#ifndef __ASM_VDSO_PROCESSOR_H > >> +#define __ASM_VDSO_PROCESSOR_H > >> + > >> +#ifndef __ASSEMBLY__ > >> + > >> +#include <asm/page-def.h> > >> + > >> +#ifdef CONFIG_COMPAT > >> +#if defined(CONFIG_ARM64_64K_PAGES) && defined(CONFIG_KUSER_HELPERS) > >> +/* > >> + * With CONFIG_ARM64_64K_PAGES enabled, the last page is occupied > >> + * by the compat vectors page. > >> + */ > >> +#define TASK_SIZE_32 UL(0x100000000) > >> +#else > >> +#define TASK_SIZE_32 (UL(0x100000000) - PAGE_SIZE) > >> +#endif /* CONFIG_ARM64_64K_PAGES */ > >> +#endif /* CONFIG_COMPAT */ > > > > Just curious, what's TASK_SIZE_32 used for in the vDSO code? You don't > > seem to move TASK_SIZE. > > > > I tried to fine grain the headers as much as I could in order to avoid > unneeded/unwanted inclusions: > * TASK_SIZE_32 is used to verify ABI consistency on vdso32 (please refer to > arch/arm64/kernel/vdso32/vgettimeofday.c). I see. But the test is probably useless. With 4K pages, TASK_SIZE_32 is 1UL << 32, so you can't have a u32 greater than this. So I'd argue that the ABI compatibility here doesn't matter. With 16K or 64K pages, TASK_SIZE_32 is slightly smaller but arm32 never supported it. What's the side-effect of dropping this check altogether?
Hi Catalin, On 3/16/20 10:34 AM, Catalin Marinas wrote: [...] >> >> I tried to fine grain the headers as much as I could in order to avoid >> unneeded/unwanted inclusions: >> * TASK_SIZE_32 is used to verify ABI consistency on vdso32 (please refer to >> arch/arm64/kernel/vdso32/vgettimeofday.c). > > I see. But the test is probably useless. With 4K pages, TASK_SIZE_32 is > 1UL << 32, so you can't have a u32 greater than this. So I'd argue that > the ABI compatibility here doesn't matter. > > With 16K or 64K pages, TASK_SIZE_32 is slightly smaller but arm32 never > supported it. > > What's the side-effect of dropping this check altogether? > The main side-effect is that arm32 and arm64 compat have a different behavior, that it is what we want to avoid. The vdsotest [1] I am using, verifies all the side conditions with respect to the ABI, which we are now compatible with. Removing those checks would break this condition. [1] https://github.com/nlynch-mentor/vdsotest
On Mon, Mar 16, 2020 at 10:55:00AM +0000, Vincenzo Frascino wrote: > On 3/16/20 10:34 AM, Catalin Marinas wrote: > >> I tried to fine grain the headers as much as I could in order to avoid > >> unneeded/unwanted inclusions: > >> * TASK_SIZE_32 is used to verify ABI consistency on vdso32 (please refer to > >> arch/arm64/kernel/vdso32/vgettimeofday.c). > > > > I see. But the test is probably useless. With 4K pages, TASK_SIZE_32 is > > 1UL << 32, so you can't have a u32 greater than this. So I'd argue that > > the ABI compatibility here doesn't matter. > > > > With 16K or 64K pages, TASK_SIZE_32 is slightly smaller but arm32 never > > supported it. > > > > What's the side-effect of dropping this check altogether? > > The main side-effect is that arm32 and arm64 compat have a different behavior, > that it is what we want to avoid. > > The vdsotest [1] I am using, verifies all the side conditions with respect to > the ABI, which we are now compatible with. Removing those checks would break > this condition. As I said above, I don't see how removing 'if ((u32)ts >= (1UL << 32))' makes any difference. This check was likely removed by the compiler already. Also, userspace doesn't have a trivial way to figure out TASK_SIZE and I can't see anything that tests this in the vdsotest (though I haven't spent that much time looking). If it's hard-coded, note that arm32 TASK_SIZE is different from TASK_SIZE_32 on arm64. Can you tell what actually is failing in vdsotest if you remove the TASK_SIZE_32 checks in the arm64 compat vdso?
Hi Catalin, On 3/16/20 11:22 AM, Catalin Marinas wrote: > On Mon, Mar 16, 2020 at 10:55:00AM +0000, Vincenzo Frascino wrote: >> On 3/16/20 10:34 AM, Catalin Marinas wrote: [...] > > As I said above, I don't see how removing 'if ((u32)ts >= (1UL << 32))' > makes any difference. This check was likely removed by the compiler > already. > > Also, userspace doesn't have a trivial way to figure out TASK_SIZE and I > can't see anything that tests this in the vdsotest (though I haven't > spent that much time looking). If it's hard-coded, note that arm32 > TASK_SIZE is different from TASK_SIZE_32 on arm64. > > Can you tell what actually is failing in vdsotest if you remove the > TASK_SIZE_32 checks in the arm64 compat vdso? > To me does not seem optimized out. Which version of the compiler are you using? Please find below the list of errors for clock_gettime (similar for the other): passing UINTPTR_MAX to clock_gettime (VDSO): terminated by unexpected signal 7 clock-gettime-monotonic/abi: 1 failures/inconsistencies encountered passing UINTPTR_MAX to clock_gettime (VDSO): terminated by unexpected signal 7 clock-gettime-monotonic-coarse/abi: 1 failures/inconsistencies encountered passing UINTPTR_MAX to clock_gettime (VDSO): terminated by unexpected signal 7 clock-gettime-realtime/abi: 1 failures/inconsistencies encountered passing UINTPTR_MAX to clock_gettime (VDSO): terminated by unexpected signal 7 clock-gettime-realtime-coarse/abi: 1 failures/inconsistencies encountered Please refer to [1] for more details on the test. [1] https://github.com/nlynch-mentor/vdsotest/blob/master/src/clock_gettime_template.c
On Mon, Mar 16, 2020 at 01:35:17PM +0000, Vincenzo Frascino wrote: > On 3/16/20 11:22 AM, Catalin Marinas wrote: > > As I said above, I don't see how removing 'if ((u32)ts >= (1UL << 32))' > > makes any difference. This check was likely removed by the compiler > > already. > > > > Also, userspace doesn't have a trivial way to figure out TASK_SIZE and I > > can't see anything that tests this in the vdsotest (though I haven't > > spent that much time looking). If it's hard-coded, note that arm32 > > TASK_SIZE is different from TASK_SIZE_32 on arm64. > > > > Can you tell what actually is failing in vdsotest if you remove the > > TASK_SIZE_32 checks in the arm64 compat vdso? > > To me does not seem optimized out. Which version of the compiler are you using? I misread the #ifdef'ery in asm/processor.h. So with 4K pages, TASK_SIZE_32 is (1UL<<32)-PAGE_SIZE. However, with 64K pages _and_ CONFIG_KUSER_HELPERS, TASK_SIZE_32 is 1UL<<32 and the check is removed by the compiler. With the 4K build, __vdso_clock_gettime starts as: 00000194 <__vdso_clock_gettime>: 194: f511 5f80 cmn.w r1, #4096 ; 0x1000 198: d214 bcs.n 1c4 <__vdso_clock_gettime+0x30> 19a: b5b0 push {r4, r5, r7, lr} ... 1c4: f06f 000d mvn.w r0, #13 1c8: 4770 bx lr With 64K pages: 00000194 <__vdso_clock_gettime>: 194: b5b0 push {r4, r5, r7, lr} ... 1be: bdb0 pop {r4, r5, r7, pc} I haven't tried but it's likely that the vdsotest fails with 64K pages and compat enabled (requires EXPERT). > Please find below the list of errors for clock_gettime (similar for the other): > > passing UINTPTR_MAX to clock_gettime (VDSO): terminated by unexpected signal 7 > clock-gettime-monotonic/abi: 1 failures/inconsistencies encountered Ah, so it uses UINTPTR_MAX in the test. Fair enough but I don't think the arm64 check is entirely useful. On arm32, the check was meant to return -EFAULT for addresses beyond TASK_SIZE that may enter into the kernel or module space. On arm64 compat, the kernel space is well above the reach of the 32-bit code. If you want to preserve some compatibility for this specific test, what about checking for wrapping around 0, I think it would make more sense. Something like: if ((u32)ts > UINTPTR_MAX - sizeof(*ts) + 1)
On 3/16/20 2:43 PM, Catalin Marinas wrote[...] >> To me does not seem optimized out. Which version of the compiler are you using? > > I misread the #ifdef'ery in asm/processor.h. So with 4K pages, > TASK_SIZE_32 is (1UL<<32)-PAGE_SIZE. However, with 64K pages _and_ > CONFIG_KUSER_HELPERS, TASK_SIZE_32 is 1UL<<32 and the check is removed > by the compiler. > > With the 4K build, __vdso_clock_gettime starts as: > > 00000194 <__vdso_clock_gettime>: > 194: f511 5f80 cmn.w r1, #4096 ; 0x1000 > 198: d214 bcs.n 1c4 <__vdso_clock_gettime+0x30> > 19a: b5b0 push {r4, r5, r7, lr} > ... > 1c4: f06f 000d mvn.w r0, #13 > 1c8: 4770 bx lr > > With 64K pages: > > 00000194 <__vdso_clock_gettime>: > 194: b5b0 push {r4, r5, r7, lr} > ... > 1be: bdb0 pop {r4, r5, r7, pc} > > I haven't tried but it's likely that the vdsotest fails with 64K pages > and compat enabled (requires EXPERT). > This makes more sense. Thanks for the clarification. I agree on the behavior of 64K pages and I think as well that the "compatibility" issue is still there. However as you correctly stated in your first email arm32 never supported 16K or 64K pages, hence I think we should not be concerned about compatibility in this cases. To make it more explicit we could make COMPAT_VDSO on arm64 depend on ARM64_4K_PAGES. What do you think? >> Please find below the list of errors for clock_gettime (similar for the other): >> >> passing UINTPTR_MAX to clock_gettime (VDSO): terminated by unexpected signal 7 >> clock-gettime-monotonic/abi: 1 failures/inconsistencies encountered > > Ah, so it uses UINTPTR_MAX in the test. Fair enough but I don't think > the arm64 check is entirely useful. On arm32, the check was meant to > return -EFAULT for addresses beyond TASK_SIZE that may enter into the > kernel or module space. On arm64 compat, the kernel space is well above > the reach of the 32-bit code. > > If you want to preserve some compatibility for this specific test, what > about checking for wrapping around 0, I think it would make more sense. > Something like: > > if ((u32)ts > UINTPTR_MAX - sizeof(*ts) + 1) > Ok, sounds good to me. But it is something that this patch series inherited, hence I would prefer to send a separate patch that introduces what you are proposing and removes TASK_SIZE_32 from the headers. How does it sound?
On Mon, Mar 16, 2020 at 03:33:30PM +0000, Vincenzo Frascino wrote: > On 3/16/20 2:43 PM, Catalin Marinas wrote[...] > >> To me does not seem optimized out. Which version of the compiler are you using? > > > > I misread the #ifdef'ery in asm/processor.h. So with 4K pages, > > TASK_SIZE_32 is (1UL<<32)-PAGE_SIZE. However, with 64K pages _and_ > > CONFIG_KUSER_HELPERS, TASK_SIZE_32 is 1UL<<32 and the check is removed > > by the compiler. > > > > With the 4K build, __vdso_clock_gettime starts as: > > > > 00000194 <__vdso_clock_gettime>: > > 194: f511 5f80 cmn.w r1, #4096 ; 0x1000 > > 198: d214 bcs.n 1c4 <__vdso_clock_gettime+0x30> > > 19a: b5b0 push {r4, r5, r7, lr} > > ... > > 1c4: f06f 000d mvn.w r0, #13 > > 1c8: 4770 bx lr > > > > With 64K pages: > > > > 00000194 <__vdso_clock_gettime>: > > 194: b5b0 push {r4, r5, r7, lr} > > ... > > 1be: bdb0 pop {r4, r5, r7, pc} > > > > I haven't tried but it's likely that the vdsotest fails with 64K pages > > and compat enabled (requires EXPERT). > > This makes more sense. Thanks for the clarification. > > I agree on the behavior of 64K pages and I think as well that the > "compatibility" issue is still there. However as you correctly stated in your > first email arm32 never supported 16K or 64K pages, hence I think we should not > be concerned about compatibility in this cases. My point is that even with 4K pages it's not really compatibility. The test uses UINTPTR_MAX but on arm32 it would also fail with 0xc0000000. On arm64 compat, however, this value would pass just fine. > To make it more explicit we could make COMPAT_VDSO on arm64 depend on > ARM64_4K_PAGES. What do you think? No, I don't see why we should add this limitation. > >> Please find below the list of errors for clock_gettime (similar for the other): > >> > >> passing UINTPTR_MAX to clock_gettime (VDSO): terminated by unexpected signal 7 > >> clock-gettime-monotonic/abi: 1 failures/inconsistencies encountered > > > > Ah, so it uses UINTPTR_MAX in the test. Fair enough but I don't think > > the arm64 check is entirely useful. On arm32, the check was meant to > > return -EFAULT for addresses beyond TASK_SIZE that may enter into the > > kernel or module space. On arm64 compat, the kernel space is well above > > the reach of the 32-bit code. > > > > If you want to preserve some compatibility for this specific test, what > > about checking for wrapping around 0, I think it would make more sense. > > Something like: > > > > if ((u32)ts > UINTPTR_MAX - sizeof(*ts) + 1) > > Ok, sounds good to me. But it is something that this patch series inherited, > hence I would prefer to send a separate patch that introduces what you are > proposing and removes TASK_SIZE_32 from the headers. How does it sound? I'd rather avoid moving TASK_SIZE_32 unnecessarily. Just add a preparatory patch to your series for arm64 compat vdso and follow with the rest without moving TASK_SIZE_32 around.
On 3/16/20 3:49 PM, Catalin Marinas wrote: > On Mon, Mar 16, 2020 at 03:33:30PM +0000, Vincenzo Frascino wrote: >> On 3/16/20 2:43 PM, Catalin Marinas wrote[...] [...] > >> To make it more explicit we could make COMPAT_VDSO on arm64 depend on >> ARM64_4K_PAGES. What do you think? > > No, I don't see why we should add this limitation. > Fine by me. >>>> Please find below the list of errors for clock_gettime (similar for the other): >>>> >>>> passing UINTPTR_MAX to clock_gettime (VDSO): terminated by unexpected signal 7 >>>> clock-gettime-monotonic/abi: 1 failures/inconsistencies encountered >>> >>> Ah, so it uses UINTPTR_MAX in the test. Fair enough but I don't think >>> the arm64 check is entirely useful. On arm32, the check was meant to >>> return -EFAULT for addresses beyond TASK_SIZE that may enter into the >>> kernel or module space. On arm64 compat, the kernel space is well above >>> the reach of the 32-bit code. >>> >>> If you want to preserve some compatibility for this specific test, what >>> about checking for wrapping around 0, I think it would make more sense. >>> Something like: >>> >>> if ((u32)ts > UINTPTR_MAX - sizeof(*ts) + 1) >> >> Ok, sounds good to me. But it is something that this patch series inherited, >> hence I would prefer to send a separate patch that introduces what you are >> proposing and removes TASK_SIZE_32 from the headers. How does it sound? > > I'd rather avoid moving TASK_SIZE_32 unnecessarily. Just add a > preparatory patch to your series for arm64 compat vdso and follow with > the rest without moving TASK_SIZE_32 around. > Ok, sounds good. I will test it and repost.
diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h index 5ba63204d078..89ba2c5be504 100644 --- a/arch/arm64/include/asm/processor.h +++ b/arch/arm64/include/asm/processor.h @@ -28,6 +28,8 @@ #include <linux/string.h> #include <linux/thread_info.h> +#include <vdso/processor.h> + #include <asm/alternative.h> #include <asm/cpufeature.h> #include <asm/hw_breakpoint.h> @@ -47,15 +49,6 @@ #define TASK_SIZE_64 (UL(1) << vabits_actual) #ifdef CONFIG_COMPAT -#if defined(CONFIG_ARM64_64K_PAGES) && defined(CONFIG_KUSER_HELPERS) -/* - * With CONFIG_ARM64_64K_PAGES enabled, the last page is occupied - * by the compat vectors page. - */ -#define TASK_SIZE_32 UL(0x100000000) -#else -#define TASK_SIZE_32 (UL(0x100000000) - PAGE_SIZE) -#endif /* CONFIG_ARM64_64K_PAGES */ #define TASK_SIZE (test_thread_flag(TIF_32BIT) ? \ TASK_SIZE_32 : TASK_SIZE_64) #define TASK_SIZE_OF(tsk) (test_tsk_thread_flag(tsk, TIF_32BIT) ? \ @@ -256,11 +249,6 @@ extern void release_thread(struct task_struct *); unsigned long get_wchan(struct task_struct *p); -static inline void cpu_relax(void) -{ - asm volatile("yield" ::: "memory"); -} - /* Thread switching */ extern struct task_struct *cpu_switch_to(struct task_struct *prev, struct task_struct *next); diff --git a/arch/arm64/include/asm/vdso/processor.h b/arch/arm64/include/asm/vdso/processor.h new file mode 100644 index 000000000000..fb4883212a2d --- /dev/null +++ b/arch/arm64/include/asm/vdso/processor.h @@ -0,0 +1,31 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright (C) 2020 ARM Ltd. + */ +#ifndef __ASM_VDSO_PROCESSOR_H +#define __ASM_VDSO_PROCESSOR_H + +#ifndef __ASSEMBLY__ + +#include <asm/page-def.h> + +#ifdef CONFIG_COMPAT +#if defined(CONFIG_ARM64_64K_PAGES) && defined(CONFIG_KUSER_HELPERS) +/* + * With CONFIG_ARM64_64K_PAGES enabled, the last page is occupied + * by the compat vectors page. + */ +#define TASK_SIZE_32 UL(0x100000000) +#else +#define TASK_SIZE_32 (UL(0x100000000) - PAGE_SIZE) +#endif /* CONFIG_ARM64_64K_PAGES */ +#endif /* CONFIG_COMPAT */ + +static inline void cpu_relax(void) +{ + asm volatile("yield" ::: "memory"); +} + +#endif /* __ASSEMBLY__ */ + +#endif /* __ASM_VDSO_PROCESSOR_H */
The vDSO library should only include the necessary headers required for a userspace library (UAPI and a minimal set of kernel headers). To make this possible it is necessary to isolate from the kernel headers the common parts that are strictly necessary to build the library. Introduce asm/vdso/processor.h to contain all the arm64 specific functions that are suitable for vDSO inclusion. Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will@kernel.org> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com> --- arch/arm64/include/asm/processor.h | 16 ++----------- arch/arm64/include/asm/vdso/processor.h | 31 +++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 14 deletions(-) create mode 100644 arch/arm64/include/asm/vdso/processor.h