[PATCH 1/2] powerpc/vdso: Fix VDSO data access when running in a non-root time namespace

[PATCH 1/2] powerpc/vdso: Fix VDSO data access when running in a non-root time namespace

[Christophe Leroy]

When running in a non-root time namespace, the global VDSO data page
is replaced by a dedicated namespace data page and the global data
page is mapped next to it. Detailed explanations can be found at
commit 660fd04f9317 ("lib/vdso: Prepare for time namespace support").

When it happens, __kernel_get_syscall_map and __kernel_get_tbfreq
and __kernel_sync_dicache don't work anymore because they read 0
instead of the data they need.

To address that, clock_mode has to be read. When it is set to
VDSO_CLOCKMODE_TIMENS, it means it is a dedicated namespace data page
and the global data is located on the following page.

Add a macro called get_realdatapage which reads clock_mode and add
PAGE_SIZE to the pointer provided by get_datapage macro when
clock_mode is equal to VDSO_CLOCKMODE_TIMENS. Use this new macro
instead of get_datapage macro except for time functions as they handle
it internally.

Fixes: 74205b3fc2ef ("powerpc/vdso: Add support for time namespaces")
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/include/asm/vdso_datapage.h | 15 +++++++++++++++
 arch/powerpc/kernel/asm-offsets.c        |  2 ++
 arch/powerpc/kernel/vdso/cacheflush.S    |  2 +-
 arch/powerpc/kernel/vdso/datapage.S      |  4 ++--
 4 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/include/asm/vdso_datapage.h b/arch/powerpc/include/asm/vdso_datapage.h
index e17500c5237e..248dee138f7b 100644
--- a/arch/powerpc/include/asm/vdso_datapage.h
+++ b/arch/powerpc/include/asm/vdso_datapage.h
@@ -113,6 +113,21 @@ extern struct vdso_arch_data *vdso_data;
 	addi	\ptr, \ptr, (_vdso_datapage - 999b)@l
 .endm
 
+#include <asm/asm-offsets.h>
+#include <asm/page.h>
+
+.macro get_realdatapage ptr scratch
+	get_datapage \ptr
+#ifdef CONFIG_TIME_NS
+	lwz	\scratch, VDSO_CLOCKMODE_OFFSET(\ptr)
+	xoris	\scratch, \scratch, VDSO_CLOCKMODE_TIMENS@h
+	xori	\scratch, \scratch, VDSO_CLOCKMODE_TIMENS@l
+	cntlzw	\scratch, \scratch
+	rlwinm	\scratch, \scratch, PAGE_SHIFT - 5, 1 << PAGE_SHIFT
+	add	\ptr, \ptr, \scratch
+#endif
+.endm
+
 #endif /* __ASSEMBLY__ */
 
 #endif /* __KERNEL__ */
diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
index eedb2e04c785..131a8cc10dbe 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -347,6 +347,8 @@ int main(void)
 #else
 	OFFSET(CFG_SYSCALL_MAP32, vdso_arch_data, syscall_map);
 #endif
+	OFFSET(VDSO_CLOCKMODE_OFFSET, vdso_arch_data, data[0].clock_mode);
+	DEFINE(VDSO_CLOCKMODE_TIMENS, VDSO_CLOCKMODE_TIMENS);
 
 #ifdef CONFIG_BUG
 	DEFINE(BUG_ENTRY_SIZE, sizeof(struct bug_entry));
diff --git a/arch/powerpc/kernel/vdso/cacheflush.S b/arch/powerpc/kernel/vdso/cacheflush.S
index 0085ae464dac..3b2479bd2f9a 100644
--- a/arch/powerpc/kernel/vdso/cacheflush.S
+++ b/arch/powerpc/kernel/vdso/cacheflush.S
@@ -30,7 +30,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
 #ifdef CONFIG_PPC64
 	mflr	r12
   .cfi_register lr,r12
-	get_datapage	r10
+	get_realdatapage	r10, r11
 	mtlr	r12
   .cfi_restore	lr
 #endif
diff --git a/arch/powerpc/kernel/vdso/datapage.S b/arch/powerpc/kernel/vdso/datapage.S
index db8e167f0166..2b19b6201a33 100644
--- a/arch/powerpc/kernel/vdso/datapage.S
+++ b/arch/powerpc/kernel/vdso/datapage.S
@@ -28,7 +28,7 @@ V_FUNCTION_BEGIN(__kernel_get_syscall_map)
 	mflr	r12
   .cfi_register lr,r12
 	mr.	r4,r3
-	get_datapage	r3
+	get_realdatapage	r3, r11
 	mtlr	r12
 #ifdef __powerpc64__
 	addi	r3,r3,CFG_SYSCALL_MAP64
@@ -52,7 +52,7 @@ V_FUNCTION_BEGIN(__kernel_get_tbfreq)
   .cfi_startproc
 	mflr	r12
   .cfi_register lr,r12
-	get_datapage	r3
+	get_realdatapage	r3, r11
 #ifndef __powerpc64__
 	lwz	r4,(CFG_TB_TICKS_PER_SEC + 4)(r3)
 #endif
-- 
2.44.0

[PATCH 2/2] Fixup for 3279be36b671 ("powerpc/vdso: Wire up getrandom() vDSO implementation on VDSO32")

[Christophe Leroy]

Use the new get_realdatapage macro instead of get_datapage

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/kernel/vdso/getrandom.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/vdso/getrandom.S b/arch/powerpc/kernel/vdso/getrandom.S
index a957cd2b2b03..f3bbf931931c 100644
--- a/arch/powerpc/kernel/vdso/getrandom.S
+++ b/arch/powerpc/kernel/vdso/getrandom.S
@@ -31,7 +31,7 @@
 	PPC_STL		r2, PPC_MIN_STKFRM + STK_GOT(r1)
   .cfi_rel_offset r2, PPC_MIN_STKFRM + STK_GOT
 #endif
-	get_datapage	r8
+	get_realdatapage	r8, r11
 	addi		r8, r8, VDSO_RNG_DATA_OFFSET
 	bl		CFUNC(DOTSYM(\funct))
 	PPC_LL		r0, PPC_MIN_STKFRM + PPC_LR_STKOFF(r1)
-- 
2.44.0

Re: [PATCH 2/2] Fixup for 3279be36b671 ("powerpc/vdso: Wire up getrandom() vDSO implementation on VDSO32")

[Jason A. Donenfeld]

On Fri, Sep 06, 2024 at 10:33:44AM +0200, Christophe Leroy wrote:
> Use the new get_realdatapage macro instead of get_datapage
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  arch/powerpc/kernel/vdso/getrandom.S | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/kernel/vdso/getrandom.S b/arch/powerpc/kernel/vdso/getrandom.S
> index a957cd2b2b03..f3bbf931931c 100644
> --- a/arch/powerpc/kernel/vdso/getrandom.S
> +++ b/arch/powerpc/kernel/vdso/getrandom.S
> @@ -31,7 +31,7 @@
>  	PPC_STL		r2, PPC_MIN_STKFRM + STK_GOT(r1)
>    .cfi_rel_offset r2, PPC_MIN_STKFRM + STK_GOT
>  #endif
> -	get_datapage	r8
> +	get_realdatapage	r8, r11
>  	addi		r8, r8, VDSO_RNG_DATA_OFFSET
>  	bl		CFUNC(DOTSYM(\funct))
>  	PPC_LL		r0, PPC_MIN_STKFRM + PPC_LR_STKOFF(r1)

I tested that this is working as intended on powerpc, powerpc64, and
powerpc64le. Thanks for writing the patch so quickly.

Jason

Re: [PATCH 2/2] Fixup for 3279be36b671 ("powerpc/vdso: Wire up getrandom() vDSO implementation on VDSO32")

[Christophe Leroy]

Le 06/09/2024 à 16:07, Jason A. Donenfeld a écrit :
> On Fri, Sep 06, 2024 at 10:33:44AM +0200, Christophe Leroy wrote:
>> Use the new get_realdatapage macro instead of get_datapage
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>>   arch/powerpc/kernel/vdso/getrandom.S | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/kernel/vdso/getrandom.S b/arch/powerpc/kernel/vdso/getrandom.S
>> index a957cd2b2b03..f3bbf931931c 100644
>> --- a/arch/powerpc/kernel/vdso/getrandom.S
>> +++ b/arch/powerpc/kernel/vdso/getrandom.S
>> @@ -31,7 +31,7 @@
>>   	PPC_STL		r2, PPC_MIN_STKFRM + STK_GOT(r1)
>>     .cfi_rel_offset r2, PPC_MIN_STKFRM + STK_GOT
>>   #endif
>> -	get_datapage	r8
>> +	get_realdatapage	r8, r11
>>   	addi		r8, r8, VDSO_RNG_DATA_OFFSET
>>   	bl		CFUNC(DOTSYM(\funct))
>>   	PPC_LL		r0, PPC_MIN_STKFRM + PPC_LR_STKOFF(r1)
> 
> I tested that this is working as intended on powerpc, powerpc64, and
> powerpc64le. Thanks for writing the patch so quickly.

You are welcome.

And thanks for playing up with it while I was sleeping and getting ideas 
too.

Did you learn powerpc assembly during the night or did you know it already ?

At the end I ended up with something which I think is simple enough for 
a backport to stable.

On the long run I wonder if we should try to find a more generic 
solution for getrandom instead of requiring each architecture to handle 
it. On gettimeofday the selection of the right page is embeded in the 
generic part, see for instance :

static __maybe_unused __kernel_old_time_t
__cvdso_time_data(const struct vdso_data *vd, __kernel_old_time_t *time)
{
	__kernel_old_time_t t;

	if (IS_ENABLED(CONFIG_TIME_NS) &&
	    vd->clock_mode == VDSO_CLOCKMODE_TIMENS)
		vd = __arch_get_timens_vdso_data(vd);

	t = READ_ONCE(vd[CS_HRES_COARSE].basetime[CLOCK_REALTIME].sec);

	if (time)
		*time = t;

	return t;
}

and powerpc just provides:

static __always_inline
const struct vdso_data *__arch_get_timens_vdso_data(const struct 
vdso_data *vd)
{
	return (void *)vd + (1U << CONFIG_PAGE_SHIFT);
}


I know it may not be that simple for getrandom but its probably worth 
trying.

Or another solution could be to put random data in a third page that is 
always at the same place regardless of timens ?

Christophe

Re: [PATCH 2/2] Fixup for 3279be36b671 ("powerpc/vdso: Wire up getrandom() vDSO implementation on VDSO32")

[Jason A. Donenfeld]

On Fri, Sep 06, 2024 at 04:26:32PM +0200, Christophe Leroy wrote:
> And thanks for playing up with it while I was sleeping and getting ideas 
> too.
> 
> Did you learn powerpc assembly during the night or did you know it already ?

I don't really know ppc assembly. I had perused the tree over the last
week and gotten some feel for it when reviewing patches, but I don't
have anything memorized (except, perhaps, the eieio instruction [1,2]).
Last night after sending the first broken patch I went out to play (I
play jazz guitar ~every night these days), and the whole time I kept
thinking about the problem. So first thing I did when I got home was try
to fake my way through some ppc asm. A fun mini project for me.

[1] https://lore.kernel.org/lkml/Pine.LNX.4.33.0110120919130.31677-100000@penguin.transmeta.com/
[2] https://lore.kernel.org/lkml/alpine.LFD.2.00.0904141006170.18124@localhost.localdomain/

> At the end I ended up with something which I think is simple enough for 
> a backport to stable.

It seems like a good patch indeed, and hopefully small enough that
Michael will let me carry in my tree for 6.12, per the plan.

> On the long run I wonder if we should try to find a more generic 
> solution for getrandom instead of requiring each architecture to handle 
> it. On gettimeofday the selection of the right page is embeded in the 
> generic part, see for instance :
> 
> static __maybe_unused __kernel_old_time_t
> __cvdso_time_data(const struct vdso_data *vd, __kernel_old_time_t *time)
> {
> 	__kernel_old_time_t t;
> 
> 	if (IS_ENABLED(CONFIG_TIME_NS) &&
> 	    vd->clock_mode == VDSO_CLOCKMODE_TIMENS)
> 		vd = __arch_get_timens_vdso_data(vd);
> 
> 	t = READ_ONCE(vd[CS_HRES_COARSE].basetime[CLOCK_REALTIME].sec);
> 
> 	if (time)
> 		*time = t;
> 
> 	return t;
> }
> 
> and powerpc just provides:
> 
> static __always_inline
> const struct vdso_data *__arch_get_timens_vdso_data(const struct 
> vdso_data *vd)
> {
> 	return (void *)vd + (1U << CONFIG_PAGE_SHIFT);
> }

It's tempting, but maybe a bit tricky. LoongArch, for example, doesn't
have this problem at all, because the layout of their vvars doesn't
require it. So the vd->clock_mode access is unnecessary.

> Or another solution could be to put random data in a third page that is 
> always at the same place regardless of timens ?

Maybe that's the easier way, yea. Potentially wasteful, though.

Jason

Re: [PATCH 2/2] Fixup for 3279be36b671 ("powerpc/vdso: Wire up getrandom() vDSO implementation on VDSO32")

[Christophe Leroy]

Le 06/09/2024 à 16:46, Jason A. Donenfeld a écrit :
> On Fri, Sep 06, 2024 at 04:26:32PM +0200, Christophe Leroy wrote:
> 
>> On the long run I wonder if we should try to find a more generic
>> solution for getrandom instead of requiring each architecture to handle
>> it. On gettimeofday the selection of the right page is embeded in the
>> generic part, see for instance :
>>
>> static __maybe_unused __kernel_old_time_t
>> __cvdso_time_data(const struct vdso_data *vd, __kernel_old_time_t *time)
>> {
>> 	__kernel_old_time_t t;
>>
>> 	if (IS_ENABLED(CONFIG_TIME_NS) &&
>> 	    vd->clock_mode == VDSO_CLOCKMODE_TIMENS)
>> 		vd = __arch_get_timens_vdso_data(vd);
>>
>> 	t = READ_ONCE(vd[CS_HRES_COARSE].basetime[CLOCK_REALTIME].sec);
>>
>> 	if (time)
>> 		*time = t;
>>
>> 	return t;
>> }
>>
>> and powerpc just provides:
>>
>> static __always_inline
>> const struct vdso_data *__arch_get_timens_vdso_data(const struct
>> vdso_data *vd)
>> {
>> 	return (void *)vd + (1U << CONFIG_PAGE_SHIFT);
>> }
> 
> It's tempting, but maybe a bit tricky. LoongArch, for example, doesn't
> have this problem at all, because the layout of their vvars doesn't
> require it. So the vd->clock_mode access is unnecessary.
> 
>> Or another solution could be to put random data in a third page that is
>> always at the same place regardless of timens ?
> 
> Maybe that's the easier way, yea. Potentially wasteful, though.
> 

Indeed I just looked at Loongarch and that's exactly what they do: they 
have a third page after the two pages dedicated to TIME for arch 
specific data, and they have added getrandom data there.

The third page is common to every process so it won't waste more than a 
few bytes. It doesn't worry me even on the older boards that only have 
32 Mbytes of RAM.

So yes, I may have a look at that in the future, what we have at the 
moment is good enough to move forward.

Christophe

Re: [PATCH 2/2] Fixup for 3279be36b671 ("powerpc/vdso: Wire up getrandom() vDSO implementation on VDSO32")

[Jason A. Donenfeld]

On Fri, Sep 06, 2024 at 05:14:43PM +0200, Christophe Leroy wrote:
> 
> 
> Le 06/09/2024 à 16:46, Jason A. Donenfeld a écrit :
> > On Fri, Sep 06, 2024 at 04:26:32PM +0200, Christophe Leroy wrote:
> > 
> >> On the long run I wonder if we should try to find a more generic
> >> solution for getrandom instead of requiring each architecture to handle
> >> it. On gettimeofday the selection of the right page is embeded in the
> >> generic part, see for instance :
> >>
> >> static __maybe_unused __kernel_old_time_t
> >> __cvdso_time_data(const struct vdso_data *vd, __kernel_old_time_t *time)
> >> {
> >> 	__kernel_old_time_t t;
> >>
> >> 	if (IS_ENABLED(CONFIG_TIME_NS) &&
> >> 	    vd->clock_mode == VDSO_CLOCKMODE_TIMENS)
> >> 		vd = __arch_get_timens_vdso_data(vd);
> >>
> >> 	t = READ_ONCE(vd[CS_HRES_COARSE].basetime[CLOCK_REALTIME].sec);
> >>
> >> 	if (time)
> >> 		*time = t;
> >>
> >> 	return t;
> >> }
> >>
> >> and powerpc just provides:
> >>
> >> static __always_inline
> >> const struct vdso_data *__arch_get_timens_vdso_data(const struct
> >> vdso_data *vd)
> >> {
> >> 	return (void *)vd + (1U << CONFIG_PAGE_SHIFT);
> >> }
> > 
> > It's tempting, but maybe a bit tricky. LoongArch, for example, doesn't
> > have this problem at all, because the layout of their vvars doesn't
> > require it. So the vd->clock_mode access is unnecessary.
> > 
> >> Or another solution could be to put random data in a third page that is
> >> always at the same place regardless of timens ?
> > 
> > Maybe that's the easier way, yea. Potentially wasteful, though.
> > 
> 
> Indeed I just looked at Loongarch and that's exactly what they do: they 
> have a third page after the two pages dedicated to TIME for arch 
> specific data, and they have added getrandom data there.
> 
> The third page is common to every process so it won't waste more than a 
> few bytes. It doesn't worry me even on the older boards that only have 
> 32 Mbytes of RAM.
> 
> So yes, I may have a look at that in the future, what we have at the 
> moment is good enough to move forward.

My x86 code is kind of icky for this:

static __always_inline const struct vdso_rng_data *__arch_get_vdso_rng_data(void)
{
        if (IS_ENABLED(CONFIG_TIME_NS) && __vdso_data->clock_mode == VDSO_CLOCKMODE_TIMENS)
                return (void *)&__vdso_rng_data + ((void *)&__timens_vdso_data - (void *)&__vdso_data);
        return &__vdso_rng_data;
}

Doing the subtraction like that means that this is more clearly correct.
But it also makes the compiler insert two jumps for the branch, and then
reads the addresses of those variables and such.

If I change it to:

static __always_inline const struct vdso_rng_data *__arch_get_vdso_rng_data(void)
{
        if (IS_ENABLED(CONFIG_TIME_NS) && __vdso_data->clock_mode == VDSO_CLOCKMODE_TIMENS)
                return (void *)&__vdso_rng_data + (3UL << CONFIG_PAGE_SHIFT);
        return &__vdso_rng_data;
}

Then there's a much nicer single `cmov` with no branching.

But if I want to do that for real, I'll have to figure out what set of
nice compile-time constants I can use. I haven't looked into this yet.

Jason

Re: [PATCH 2/2] Fixup for 3279be36b671 ("powerpc/vdso: Wire up getrandom() vDSO implementation on VDSO32")

[Jason A. Donenfeld]

On Fri, Sep 06, 2024 at 08:54:49PM +0200, Jason A. Donenfeld wrote:
> On Fri, Sep 06, 2024 at 05:14:43PM +0200, Christophe Leroy wrote:
> > 
> > 
> > Le 06/09/2024 à 16:46, Jason A. Donenfeld a écrit :
> > > On Fri, Sep 06, 2024 at 04:26:32PM +0200, Christophe Leroy wrote:
> > > 
> > >> On the long run I wonder if we should try to find a more generic
> > >> solution for getrandom instead of requiring each architecture to handle
> > >> it. On gettimeofday the selection of the right page is embeded in the
> > >> generic part, see for instance :
> > >>
> > >> static __maybe_unused __kernel_old_time_t
> > >> __cvdso_time_data(const struct vdso_data *vd, __kernel_old_time_t *time)
> > >> {
> > >> 	__kernel_old_time_t t;
> > >>
> > >> 	if (IS_ENABLED(CONFIG_TIME_NS) &&
> > >> 	    vd->clock_mode == VDSO_CLOCKMODE_TIMENS)
> > >> 		vd = __arch_get_timens_vdso_data(vd);
> > >>
> > >> 	t = READ_ONCE(vd[CS_HRES_COARSE].basetime[CLOCK_REALTIME].sec);
> > >>
> > >> 	if (time)
> > >> 		*time = t;
> > >>
> > >> 	return t;
> > >> }
> > >>
> > >> and powerpc just provides:
> > >>
> > >> static __always_inline
> > >> const struct vdso_data *__arch_get_timens_vdso_data(const struct
> > >> vdso_data *vd)
> > >> {
> > >> 	return (void *)vd + (1U << CONFIG_PAGE_SHIFT);
> > >> }
> > > 
> > > It's tempting, but maybe a bit tricky. LoongArch, for example, doesn't
> > > have this problem at all, because the layout of their vvars doesn't
> > > require it. So the vd->clock_mode access is unnecessary.
> > > 
> > >> Or another solution could be to put random data in a third page that is
> > >> always at the same place regardless of timens ?
> > > 
> > > Maybe that's the easier way, yea. Potentially wasteful, though.
> > > 
> > 
> > Indeed I just looked at Loongarch and that's exactly what they do: they 
> > have a third page after the two pages dedicated to TIME for arch 
> > specific data, and they have added getrandom data there.
> > 
> > The third page is common to every process so it won't waste more than a 
> > few bytes. It doesn't worry me even on the older boards that only have 
> > 32 Mbytes of RAM.
> > 
> > So yes, I may have a look at that in the future, what we have at the 
> > moment is good enough to move forward.
> 
> My x86 code is kind of icky for this:
> 
> static __always_inline const struct vdso_rng_data *__arch_get_vdso_rng_data(void)
> {
>         if (IS_ENABLED(CONFIG_TIME_NS) && __vdso_data->clock_mode == VDSO_CLOCKMODE_TIMENS)
>                 return (void *)&__vdso_rng_data + ((void *)&__timens_vdso_data - (void *)&__vdso_data);
>         return &__vdso_rng_data;
> }
> 
> Doing the subtraction like that means that this is more clearly correct.
> But it also makes the compiler insert two jumps for the branch, and then
> reads the addresses of those variables and such.
> 
> If I change it to:
> 
> static __always_inline const struct vdso_rng_data *__arch_get_vdso_rng_data(void)
> {
>         if (IS_ENABLED(CONFIG_TIME_NS) && __vdso_data->clock_mode == VDSO_CLOCKMODE_TIMENS)
>                 return (void *)&__vdso_rng_data + (3UL << CONFIG_PAGE_SHIFT);
>         return &__vdso_rng_data;
> }
> 
> Then there's a much nicer single `cmov` with no branching.
> 
> But if I want to do that for real, I'll have to figure out what set of
> nice compile-time constants I can use. I haven't looked into this yet.

https://lore.kernel.org/all/20240906190655.2777023-1-Jason@zx2c4.com/

Re: [PATCH 2/2] Fixup for 3279be36b671 ("powerpc/vdso: Wire up getrandom() vDSO implementation on VDSO32")

[Christophe Leroy]

Le 07/09/2024 à 16:35, Jason A. Donenfeld a écrit :
> On Fri, Sep 06, 2024 at 08:54:49PM +0200, Jason A. Donenfeld wrote:
>> On Fri, Sep 06, 2024 at 05:14:43PM +0200, Christophe Leroy wrote:
>>>
>>>
>>> Le 06/09/2024 à 16:46, Jason A. Donenfeld a écrit :
>>>> On Fri, Sep 06, 2024 at 04:26:32PM +0200, Christophe Leroy wrote:
>>>>
>>>>> On the long run I wonder if we should try to find a more generic
>>>>> solution for getrandom instead of requiring each architecture to handle
>>>>> it. On gettimeofday the selection of the right page is embeded in the
>>>>> generic part, see for instance :
>>>>>
>>>>> static __maybe_unused __kernel_old_time_t
>>>>> __cvdso_time_data(const struct vdso_data *vd, __kernel_old_time_t *time)
>>>>> {
>>>>> 	__kernel_old_time_t t;
>>>>>
>>>>> 	if (IS_ENABLED(CONFIG_TIME_NS) &&
>>>>> 	    vd->clock_mode == VDSO_CLOCKMODE_TIMENS)
>>>>> 		vd = __arch_get_timens_vdso_data(vd);
>>>>>
>>>>> 	t = READ_ONCE(vd[CS_HRES_COARSE].basetime[CLOCK_REALTIME].sec);
>>>>>
>>>>> 	if (time)
>>>>> 		*time = t;
>>>>>
>>>>> 	return t;
>>>>> }
>>>>>
>>>>> and powerpc just provides:
>>>>>
>>>>> static __always_inline
>>>>> const struct vdso_data *__arch_get_timens_vdso_data(const struct
>>>>> vdso_data *vd)
>>>>> {
>>>>> 	return (void *)vd + (1U << CONFIG_PAGE_SHIFT);
>>>>> }
>>>>
>>>> It's tempting, but maybe a bit tricky. LoongArch, for example, doesn't
>>>> have this problem at all, because the layout of their vvars doesn't
>>>> require it. So the vd->clock_mode access is unnecessary.
>>>>
>>>>> Or another solution could be to put random data in a third page that is
>>>>> always at the same place regardless of timens ?
>>>>
>>>> Maybe that's the easier way, yea. Potentially wasteful, though.
>>>>
>>>
>>> Indeed I just looked at Loongarch and that's exactly what they do: they
>>> have a third page after the two pages dedicated to TIME for arch
>>> specific data, and they have added getrandom data there.
>>>
>>> The third page is common to every process so it won't waste more than a
>>> few bytes. It doesn't worry me even on the older boards that only have
>>> 32 Mbytes of RAM.
>>>
>>> So yes, I may have a look at that in the future, what we have at the
>>> moment is good enough to move forward.
>>
>> My x86 code is kind of icky for this:
>>
>> static __always_inline const struct vdso_rng_data *__arch_get_vdso_rng_data(void)
>> {
>>          if (IS_ENABLED(CONFIG_TIME_NS) && __vdso_data->clock_mode == VDSO_CLOCKMODE_TIMENS)
>>                  return (void *)&__vdso_rng_data + ((void *)&__timens_vdso_data - (void *)&__vdso_data);
>>          return &__vdso_rng_data;
>> }
>>
>> Doing the subtraction like that means that this is more clearly correct.
>> But it also makes the compiler insert two jumps for the branch, and then
>> reads the addresses of those variables and such.
>>
>> If I change it to:
>>
>> static __always_inline const struct vdso_rng_data *__arch_get_vdso_rng_data(void)
>> {
>>          if (IS_ENABLED(CONFIG_TIME_NS) && __vdso_data->clock_mode == VDSO_CLOCKMODE_TIMENS)
>>                  return (void *)&__vdso_rng_data + (3UL << CONFIG_PAGE_SHIFT);
>>          return &__vdso_rng_data;
>> }
>>
>> Then there's a much nicer single `cmov` with no branching.
>>
>> But if I want to do that for real, I'll have to figure out what set of
>> nice compile-time constants I can use. I haven't looked into this yet.
> 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2F20240906190655.2777023-1-Jason%40zx2c4.com%2F&data=05%7C02%7Cchristophe.leroy%40csgroup.eu%7C3ee8b35fe848434e72fd08dccf4a67ff%7C8b87af7d86474dc78df45f69a2011bb5%7C0%7C0%7C638613165688600378%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=g4zcMonjJNYhwrUWeCDoL5Ri7Mbg5hVQJyZNU2zH4Pc%3D&reserved=0

Looks good.

Allthough other architectures don't use defines but enums for that:

arch/arm64/kernel/vdso.c-36-
arch/arm64/kernel/vdso.c-37-enum vvar_pages {
arch/arm64/kernel/vdso.c:38:	VVAR_DATA_PAGE_OFFSET,
arch/arm64/kernel/vdso.c:39:	VVAR_TIMENS_PAGE_OFFSET,
arch/arm64/kernel/vdso.c-40-	VVAR_NR_PAGES,
arch/arm64/kernel/vdso.c-41-};
--
arch/loongarch/include/asm/vdso/vdso.h-36-
arch/loongarch/include/asm/vdso/vdso.h-37-enum vvar_pages {
arch/loongarch/include/asm/vdso/vdso.h:38:	VVAR_GENERIC_PAGE_OFFSET,
arch/loongarch/include/asm/vdso/vdso.h:39:	VVAR_TIMENS_PAGE_OFFSET,
arch/loongarch/include/asm/vdso/vdso.h-40-	VVAR_LOONGARCH_PAGES_START,
arch/loongarch/include/asm/vdso/vdso.h-41-	VVAR_LOONGARCH_PAGES_END = 
VVAR_LOONGARCH_PAGES_START + LOONGARCH_VDSO_DATA_PAGES - 1,
--
arch/powerpc/kernel/vdso.c-54-
arch/powerpc/kernel/vdso.c-55-enum vvar_pages {
arch/powerpc/kernel/vdso.c:56:	VVAR_DATA_PAGE_OFFSET,
arch/powerpc/kernel/vdso.c:57:	VVAR_TIMENS_PAGE_OFFSET,
arch/powerpc/kernel/vdso.c-58-	VVAR_NR_PAGES,
arch/powerpc/kernel/vdso.c-59-};
--
arch/riscv/kernel/vdso.c-19-
arch/riscv/kernel/vdso.c-20-enum vvar_pages {
arch/riscv/kernel/vdso.c:21:	VVAR_DATA_PAGE_OFFSET,
arch/riscv/kernel/vdso.c:22:	VVAR_TIMENS_PAGE_OFFSET,
arch/riscv/kernel/vdso.c-23-	VVAR_NR_PAGES,
arch/riscv/kernel/vdso.c-24-};
--
arch/s390/kernel/vdso.c-31-
arch/s390/kernel/vdso.c-32-enum vvar_pages {
arch/s390/kernel/vdso.c:33:	VVAR_DATA_PAGE_OFFSET,
arch/s390/kernel/vdso.c:34:	VVAR_TIMENS_PAGE_OFFSET,
arch/s390/kernel/vdso.c-35-	VVAR_NR_PAGES,
arch/s390/kernel/vdso.c-36-};

Christophe

Re: [PATCH 1/2] powerpc/vdso: Fix VDSO data access when running in a non-root time namespace

[Michael Ellerman]

Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> When running in a non-root time namespace, the global VDSO data page
> is replaced by a dedicated namespace data page and the global data
> page is mapped next to it. Detailed explanations can be found at
> commit 660fd04f9317 ("lib/vdso: Prepare for time namespace support").
>
> When it happens, __kernel_get_syscall_map and __kernel_get_tbfreq
> and __kernel_sync_dicache don't work anymore because they read 0
> instead of the data they need.
>
> To address that, clock_mode has to be read. When it is set to
> VDSO_CLOCKMODE_TIMENS, it means it is a dedicated namespace data page
> and the global data is located on the following page.
>
> Add a macro called get_realdatapage which reads clock_mode and add
> PAGE_SIZE to the pointer provided by get_datapage macro when
> clock_mode is equal to VDSO_CLOCKMODE_TIMENS. Use this new macro
> instead of get_datapage macro except for time functions as they handle
> it internally.
>
> Fixes: 74205b3fc2ef ("powerpc/vdso: Add support for time namespaces")
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
 
Oops.

I guess it should also have:

  Cc: stable@vger.kernel.org # v5.13+
  Reported-by: Jason A. Donenfeld <Jason@zx2c4.com>
  Closes: https://lore.kernel.org/all/ZtnYqZI-nrsNslwy@zx2c4.com/

Jason how do you want to handle this?

I can put patch 1 in a topic branch that we both merge? Then you can
apply patch 2 on top of that merge in your tree.

Or we could both apply patch 1 to our trees, it might lead to a conflict
but it wouldn't be anything drastic.

cheers

Re: [PATCH 1/2] powerpc/vdso: Fix VDSO data access when running in a non-root time namespace

[Christophe Leroy]

Le 06/09/2024 à 14:23, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>> When running in a non-root time namespace, the global VDSO data page
>> is replaced by a dedicated namespace data page and the global data
>> page is mapped next to it. Detailed explanations can be found at
>> commit 660fd04f9317 ("lib/vdso: Prepare for time namespace support").
>>
>> When it happens, __kernel_get_syscall_map and __kernel_get_tbfreq
>> and __kernel_sync_dicache don't work anymore because they read 0
>> instead of the data they need.
>>
>> To address that, clock_mode has to be read. When it is set to
>> VDSO_CLOCKMODE_TIMENS, it means it is a dedicated namespace data page
>> and the global data is located on the following page.
>>
>> Add a macro called get_realdatapage which reads clock_mode and add
>> PAGE_SIZE to the pointer provided by get_datapage macro when
>> clock_mode is equal to VDSO_CLOCKMODE_TIMENS. Use this new macro
>> instead of get_datapage macro except for time functions as they handle
>> it internally.
>>
>> Fixes: 74205b3fc2ef ("powerpc/vdso: Add support for time namespaces")
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>   
> Oops.
> 
> I guess it should also have:
> 
>    Cc: stable@vger.kernel.org # v5.13+
>    Reported-by: Jason A. Donenfeld <Jason@zx2c4.com>
>    Closes: https://lore.kernel.org/all/ZtnYqZI-nrsNslwy@zx2c4.com/

Jason only reported a problem with getrandom, the other three are 
"cherry on the cake".

The bug has been there for 3 years, I'm sure it can stay 3-4 more weeks, 
I'm not sure there is a need to apply it in both trees.

As far as I understood Jason was about to squash the fix into his tree 
so I was expecting him to apply patch 1 before "vDSO getrandom 
implementation for powerpc" patches and then squash patch 2 in place.

> 
> Jason how do you want to handle this?
> 
> I can put patch 1 in a topic branch that we both merge? Then you can
> apply patch 2 on top of that merge in your tree.
> 
> Or we could both apply patch 1 to our trees, it might lead to a conflict
> but it wouldn't be anything drastic.

Re: [PATCH 1/2] powerpc/vdso: Fix VDSO data access when running in a non-root time namespace

[Jason A. Donenfeld]

On Fri, Sep 06, 2024 at 10:23:08PM +1000, Michael Ellerman wrote:
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> > When running in a non-root time namespace, the global VDSO data page
> > is replaced by a dedicated namespace data page and the global data
> > page is mapped next to it. Detailed explanations can be found at
> > commit 660fd04f9317 ("lib/vdso: Prepare for time namespace support").
> >
> > When it happens, __kernel_get_syscall_map and __kernel_get_tbfreq
> > and __kernel_sync_dicache don't work anymore because they read 0
> > instead of the data they need.
> >
> > To address that, clock_mode has to be read. When it is set to
> > VDSO_CLOCKMODE_TIMENS, it means it is a dedicated namespace data page
> > and the global data is located on the following page.
> >
> > Add a macro called get_realdatapage which reads clock_mode and add
> > PAGE_SIZE to the pointer provided by get_datapage macro when
> > clock_mode is equal to VDSO_CLOCKMODE_TIMENS. Use this new macro
> > instead of get_datapage macro except for time functions as they handle
> > it internally.
> >
> > Fixes: 74205b3fc2ef ("powerpc/vdso: Add support for time namespaces")
> > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>  
> Oops.
> 
> I guess it should also have:
> 
>   Cc: stable@vger.kernel.org # v5.13+
>   Reported-by: Jason A. Donenfeld <Jason@zx2c4.com>
>   Closes: https://lore.kernel.org/all/ZtnYqZI-nrsNslwy@zx2c4.com/
> 
> Jason how do you want to handle this?
> 
> I can put patch 1 in a topic branch that we both merge? Then you can
> apply patch 2 on top of that merge in your tree.
> 
> Or we could both apply patch 1 to our trees, it might lead to a conflict
> but it wouldn't be anything drastic.

The merge window for 6.12 is pretty soon. Why don't I just take this in
my random.git tree (with your ack) as a prereq to the ppc vDSO work.
It'll slide in _before_ Christophe's other commits, and then the
separate vgetrandom fixup will be squashed in the right place there.

And then it'll hit stable when that's submitted for 6.12. It's an old
bug that nobody noticed, and time namespaces are kind of obscure, so I
think waiting a week and a half for the merge window to open is probably
fine.

Jason

Re: [PATCH 1/2] powerpc/vdso: Fix VDSO data access when running in a non-root time namespace

[Jason A. Donenfeld]

On Fri, Sep 06, 2024 at 03:43:17PM +0200, Jason A. Donenfeld wrote:
> On Fri, Sep 06, 2024 at 10:23:08PM +1000, Michael Ellerman wrote:
> > Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> > > When running in a non-root time namespace, the global VDSO data page
> > > is replaced by a dedicated namespace data page and the global data
> > > page is mapped next to it. Detailed explanations can be found at
> > > commit 660fd04f9317 ("lib/vdso: Prepare for time namespace support").
> > >
> > > When it happens, __kernel_get_syscall_map and __kernel_get_tbfreq
> > > and __kernel_sync_dicache don't work anymore because they read 0
> > > instead of the data they need.
> > >
> > > To address that, clock_mode has to be read. When it is set to
> > > VDSO_CLOCKMODE_TIMENS, it means it is a dedicated namespace data page
> > > and the global data is located on the following page.
> > >
> > > Add a macro called get_realdatapage which reads clock_mode and add
> > > PAGE_SIZE to the pointer provided by get_datapage macro when
> > > clock_mode is equal to VDSO_CLOCKMODE_TIMENS. Use this new macro
> > > instead of get_datapage macro except for time functions as they handle
> > > it internally.
> > >
> > > Fixes: 74205b3fc2ef ("powerpc/vdso: Add support for time namespaces")
> > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> >  
> > Oops.
> > 
> > I guess it should also have:
> > 
> >   Cc: stable@vger.kernel.org # v5.13+
> >   Reported-by: Jason A. Donenfeld <Jason@zx2c4.com>
> >   Closes: https://lore.kernel.org/all/ZtnYqZI-nrsNslwy@zx2c4.com/
> > 
> > Jason how do you want to handle this?
> > 
> > I can put patch 1 in a topic branch that we both merge? Then you can
> > apply patch 2 on top of that merge in your tree.
> > 
> > Or we could both apply patch 1 to our trees, it might lead to a conflict
> > but it wouldn't be anything drastic.
> 
> The merge window for 6.12 is pretty soon. Why don't I just take this in
> my random.git tree (with your ack) as a prereq to the ppc vDSO work.
> It'll slide in _before_ Christophe's other commits, and then the
> separate vgetrandom fixup will be squashed in the right place there.
> 
> And then it'll hit stable when that's submitted for 6.12. It's an old
> bug that nobody noticed, and time namespaces are kind of obscure, so I
> think waiting a week and a half for the merge window to open is probably
> fine.

So I've just done this (preliminarily, pending Michael's approval), and
it comes out decently clean and everything works fine. The commit
sequence becomes:

...
c206cd11e7f2 selftests: vDSO: ensure vgetrandom works in a time namespace
...
e59cc170924c powerpc/vdso: Fix VDSO data access when running in a non-root time namespace
887e7a77dc99 mm: Define VM_DROPPABLE for powerpc/32
f2ee39ec52c2 powerpc/vdso32: Add crtsavres
994148e87080 powerpc/vdso: Refactor CFLAGS for CVDSO build
c49ec121a6dd powerpc/vdso: Wire up getrandom() vDSO implementation on VDSO32 <-- fixed up
a8a5e16cde32 powerpc/vdso: Wire up getrandom() vDSO implementation on VDSO64

So I'm happy with this.

Jason

Re: [PATCH 1/2] powerpc/vdso: Fix VDSO data access when running in a non-root time namespace

[Michael Ellerman]

"Jason A. Donenfeld" <Jason@zx2c4.com> writes:
> On Fri, Sep 06, 2024 at 03:43:17PM +0200, Jason A. Donenfeld wrote:
>> On Fri, Sep 06, 2024 at 10:23:08PM +1000, Michael Ellerman wrote:
>> > Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>> > > When running in a non-root time namespace, the global VDSO data page
>> > > is replaced by a dedicated namespace data page and the global data
>> > > page is mapped next to it. Detailed explanations can be found at
>> > > commit 660fd04f9317 ("lib/vdso: Prepare for time namespace support").
>> > >
>> > > When it happens, __kernel_get_syscall_map and __kernel_get_tbfreq
>> > > and __kernel_sync_dicache don't work anymore because they read 0
>> > > instead of the data they need.
>> > >
>> > > To address that, clock_mode has to be read. When it is set to
>> > > VDSO_CLOCKMODE_TIMENS, it means it is a dedicated namespace data page
>> > > and the global data is located on the following page.
>> > >
>> > > Add a macro called get_realdatapage which reads clock_mode and add
>> > > PAGE_SIZE to the pointer provided by get_datapage macro when
>> > > clock_mode is equal to VDSO_CLOCKMODE_TIMENS. Use this new macro
>> > > instead of get_datapage macro except for time functions as they handle
>> > > it internally.
>> > >
>> > > Fixes: 74205b3fc2ef ("powerpc/vdso: Add support for time namespaces")
>> > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> >  
>> > Oops.
>> > 
>> > I guess it should also have:
>> > 
>> >   Cc: stable@vger.kernel.org # v5.13+
>> >   Reported-by: Jason A. Donenfeld <Jason@zx2c4.com>
>> >   Closes: https://lore.kernel.org/all/ZtnYqZI-nrsNslwy@zx2c4.com/
>> > 
>> > Jason how do you want to handle this?
>> > 
>> > I can put patch 1 in a topic branch that we both merge? Then you can
>> > apply patch 2 on top of that merge in your tree.
>> > 
>> > Or we could both apply patch 1 to our trees, it might lead to a conflict
>> > but it wouldn't be anything drastic.
>> 
>> The merge window for 6.12 is pretty soon. Why don't I just take this in
>> my random.git tree (with your ack) as a prereq to the ppc vDSO work.
>> It'll slide in _before_ Christophe's other commits, and then the
>> separate vgetrandom fixup will be squashed in the right place there.
>> 
>> And then it'll hit stable when that's submitted for 6.12. It's an old
>> bug that nobody noticed, and time namespaces are kind of obscure, so I
>> think waiting a week and a half for the merge window to open is probably
>> fine.
>
> So I've just done this (preliminarily, pending Michael's approval), and
> it comes out decently clean and everything works fine.

Sorry, didn't check email over the weekend.

Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)

> The commit
> sequence becomes:
>
> ...
> c206cd11e7f2 selftests: vDSO: ensure vgetrandom works in a time namespace
> ...
> e59cc170924c powerpc/vdso: Fix VDSO data access when running in a non-root time namespace
> 887e7a77dc99 mm: Define VM_DROPPABLE for powerpc/32
> f2ee39ec52c2 powerpc/vdso32: Add crtsavres
> 994148e87080 powerpc/vdso: Refactor CFLAGS for CVDSO build
> c49ec121a6dd powerpc/vdso: Wire up getrandom() vDSO implementation on VDSO32 <-- fixed up
> a8a5e16cde32 powerpc/vdso: Wire up getrandom() vDSO implementation on VDSO64
>
> So I'm happy with this.

Thanks.

cheers