| Message ID | 700dbf296d02e32376329774be35cfbead08041d.1725611321.git.christophe.leroy@csgroup.eu (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [1/2] powerpc/vdso: Fix VDSO data access when running in a non-root time namespace | expand |
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
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.
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
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
"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
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
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(-)