Re: [PATCH 8/8] membarrier: Rewrite sync_core_before_usermode() and improve documentation
Excerpts from Andy Lutomirski's message of June 20, 2021 1:50 am: > > > On Fri, Jun 18, 2021, at 11:02 PM, Nicholas Piggin wrote: >> Excerpts from Mathieu Desnoyers's message of June 19, 2021 6:09 am: >> > - On Jun 18, 2021, at 3:58 PM, Andy Lutomirski l...@kernel.org wrote: >> > >> >> On Fri, Jun 18, 2021, at 9:31 AM, Mathieu Desnoyers wrote: >> >>> - On Jun 17, 2021, at 8:12 PM, Andy Lutomirski l...@kernel.org wrote: >> >>> >> >>> > On 6/17/21 7:47 AM, Mathieu Desnoyers wrote: >> >>> > >> >>> >> Please change back this #ifndef / #else / #endif within function for >> >>> >> >> >>> >> if (!IS_ENABLED(CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE)) { >> >>> >> ... >> >>> >> } else { >> >>> >> ... >> >>> >> } >> >>> >> >> >>> >> I don't think mixing up preprocessor and code logic makes it more >> >>> >> readable. >> >>> > >> >>> > I agree, but I don't know how to make the result work well. >> >>> > membarrier_sync_core_before_usermode() isn't defined in the !IS_ENABLED >> >>> > case, so either I need to fake up a definition or use #ifdef. >> >>> > >> >>> > If I faked up a definition, I would want to assert, at build time, that >> >>> > it isn't called. I don't think we can do: >> >>> > >> >>> > static void membarrier_sync_core_before_usermode() >> >>> > { >> >>> >BUILD_BUG_IF_REACHABLE(); >> >>> > } >> >>> >> >>> Let's look at the context here: >> >>> >> >>> static void ipi_sync_core(void *info) >> >>> { >> >>> [] >> >>> membarrier_sync_core_before_usermode() >> >>> } >> >>> >> >>> ^ this can be within #ifdef / #endif >> >>> >> >>> static int membarrier_private_expedited(int flags, int cpu_id) >> >>> [...] >> >>>if (!IS_ENABLED(CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE)) >> >>> return -EINVAL; >> >>> if (!(atomic_read(&mm->membarrier_state) & >> >>> >> >>> MEMBARRIER_STATE_PRIVATE_EXPEDITED_SYNC_CORE_READY)) >> >>> return -EPERM; >> >>> ipi_func = ipi_sync_core; >> >>> >> >>> All we need to make the line above work is to define an empty >> >>> ipi_sync_core >> >>> function in the #else case after the ipi_sync_core() function definition. >> >>> >> >>> Or am I missing your point ? >> >> >> >> Maybe? >> >> >> >> My objection is that an empty ipi_sync_core is a lie — it doesn’t sync >> >> the core. >> >> I would be fine with that if I could have the compiler statically verify >> >> that >> >> it’s not called, but I’m uncomfortable having it there if the >> >> implementation is >> >> actively incorrect. >> > >> > I see. Another approach would be to implement a "setter" function to >> > populate >> > "ipi_func". That setter function would return -EINVAL in its #ifndef >> > CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE >> > implementation. >> >> I still don't get the problem with my suggestion. Sure the >> ipi is a "lie", but it doesn't get used. That's how a lot of >> ifdef folding works out. E.g., >> >> diff --git a/kernel/sched/membarrier.c b/kernel/sched/membarrier.c >> index b5add64d9698..54cb32d064af 100644 >> --- a/kernel/sched/membarrier.c >> +++ b/kernel/sched/membarrier.c >> @@ -5,6 +5,15 @@ >> * membarrier system call >> */ >> #include "sched.h" >> +#ifdef CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE >> +#include >> +#else >> +static inline void membarrier_sync_core_before_usermode(void) >> +{ >> +compiletime_assert(0, "architecture does not implement >> membarrier_sync_core_before_usermode"); >> +} >> + > > With the assert there, I’m fine with this. Let me see if the result builds. It had better, because compiletime_assert already relies on a similar level of code elimination to work. I think it's fine to use for now, but it may not be quite the the logically correct primitive if we want to be really clean, because a valid compiletime_assert implementation should be able to fire even for code that is never linked. We would want something like to be clean IMO: #ifdef CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE #include #else extern void membarrier_sync_core_before_usermode(void) __compiletime_error("architecture does not implement membarrier_sync_core_before_usermode"); #endif However that does not have the ifdef for optimising compile so AFAIKS it could break with a false positive in some cases. Something like compiletime_assert_not_called("msg") that either compiles to a noop or a __compiletime_error depending on __OPTIMIZE__ would be the way to go IMO. I don't know if anything exists that fits, but it's certainly not a unique thing in the kernel so I may not be looking hard enough. Thanks, Nick
[GIT PULL] Please pull powerpc/linux.git powerpc-5.13-6 tag
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 Hi Linus, Please pull some more powerpc fixes for 5.13: The following changes since commit 59cc84c802eb923805e7bba425976a3df5ce35d8: Revert "powerpc/kernel/iommu: Align size for IOMMU_PAGE_SIZE() to save TCEs" (2021-06-01 11:17:08 +1000) are available in the git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git tags/powerpc-5.13-6 for you to fetch changes up to 60b7ed54a41b550d50caf7f2418db4a7e75b5bdc: powerpc/perf: Fix crash in perf_instruction_pointer() when ppmu is not set (2021-06-18 16:30:36 +1000) - -- powerpc fixes for 5.13 #6 Fix initrd corruption caused by our recent change to use relative jump labels. Fix a crash using perf record on systems without a hardware PMU backend. Rework our 64-bit signal handling slighty to make it more closely match the old behaviour, after the recent change to use unsafe user accessors. Thanks to: Anastasia Kovaleva, Athira Rajeev, Christophe Leroy, Daniel Axtens, Greg Kurz, Roman Bolshakov. - -- Athira Rajeev (1): powerpc/perf: Fix crash in perf_instruction_pointer() when ppmu is not set Christophe Leroy (1): powerpc/mem: Add back missing header to fix 'no previous prototype' error Michael Ellerman (2): powerpc/signal64: Copy siginfo before changing regs->nip powerpc: Fix initrd corruption with relative jump labels arch/powerpc/include/asm/jump_label.h | 2 +- arch/powerpc/kernel/signal_64.c | 9 - arch/powerpc/mm/mem.c | 1 + arch/powerpc/perf/core-book3s.c | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) -BEGIN PGP SIGNATURE- iQIzBAEBCAAdFiEEJFGtCPCthwEv2Y/bUevqPMjhpYAFAmDOf4MACgkQUevqPMjh pYCWZQ/+IFYW1st6xlM1jBDlLSItSn234dqOmvlihIo+9lCR82l72H4E4EWIj7A+ 3GGzMllEdDcGooEF1jg+7+zUlx8i1WNlRCF4RNszGzpipiDWGPlxW2t5FqNeQRQ7 YyooMzrgPYlRxGHnG/KHfGiPJxxLj4ZsyRhWfoS6cY1EbS/YOX8SDX1Xz2qQu/Jr qRzZvSZKkBpVdvcxEYcn7WSauDpqtZ9keWHdP8e6WRd/Bceu4nyxlxOI0z+pBUsr 3IhWzQexznJwCGClQBbaXg/uPmUDtEjx+LzhU0jTmSMLxVI+UFPVDIbco6bMX7AE EevcU35aDLu8tclNd3IAA9Au/EZPUe8kMNUPmBncFAID4ek+gybRJuGO9b9XEJ1r AZFFCb2rRugBvOeNtb5y3u9XNR0Ct0S2lsZygSOkCQ6R3Sf2yoVgP0M49PbFvEEO fSVLnAMEWQDWfaLjYxFXp2S3vddyLw8G36lBzJo74Y/cRuz10g/87oWpIlFq5tqK aMXTroINmErOv1XVALqix1ScrLeBnPlL2nH0gBSZ96W0A196kFjWkKaGsQFwXMEH X2Om1rKYiC3/vKrLXRYcxSZcoRg7/a1es7ftVSv5DQAPGRGWDrplCJh70x5JW2Js kA6IB2K8+Ehf3F0a7O+i5q5t1oTvOvR+wIEl2TpJdx5aMVGkBt0= =CmCH -END PGP SIGNATURE-
Re: [PATCH v3] lockdown, selinux: fix wrong subject in some SELinux lockdown checks
On Wed, Jun 16 2021 at 10:51, Ondrej Mosnacek wrote: > diff --git a/arch/x86/mm/testmmiotrace.c b/arch/x86/mm/testmmiotrace.c > index bda73cb7a044..c43a13241ae8 100644 > --- a/arch/x86/mm/testmmiotrace.c > +++ b/arch/x86/mm/testmmiotrace.c > @@ -116,7 +116,7 @@ static void do_test_bulk_ioremapping(void) > static int __init init(void) > { > unsigned long size = (read_far) ? (8 << 20) : (16 << 10); > - int ret = security_locked_down(LOCKDOWN_MMIOTRACE); > + int ret = security_locked_down(current_cred(), LOCKDOWN_MMIOTRACE); I have no real objection to those patches, but it strikes me odd that out of the 62 changed places 58 have 'current_cred()' and 4 have NULL as argument. I can't see why this would ever end up with anything else than current_cred() or NULL and NULL being the 'special' case. So why not having security_locked_down_no_cred() and make current_cred() implicit for security_locked_down() which avoids most of the churn and just makes the special cases special. I might be missing something though. Thanks, tglx
Re: [PATCH 8/8] membarrier: Rewrite sync_core_before_usermode() and improve documentation
On Fri, Jun 18, 2021, at 11:02 PM, Nicholas Piggin wrote: > Excerpts from Mathieu Desnoyers's message of June 19, 2021 6:09 am: > > - On Jun 18, 2021, at 3:58 PM, Andy Lutomirski l...@kernel.org wrote: > > > >> On Fri, Jun 18, 2021, at 9:31 AM, Mathieu Desnoyers wrote: > >>> - On Jun 17, 2021, at 8:12 PM, Andy Lutomirski l...@kernel.org wrote: > >>> > >>> > On 6/17/21 7:47 AM, Mathieu Desnoyers wrote: > >>> > > >>> >> Please change back this #ifndef / #else / #endif within function for > >>> >> > >>> >> if (!IS_ENABLED(CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE)) { > >>> >> ... > >>> >> } else { > >>> >> ... > >>> >> } > >>> >> > >>> >> I don't think mixing up preprocessor and code logic makes it more > >>> >> readable. > >>> > > >>> > I agree, but I don't know how to make the result work well. > >>> > membarrier_sync_core_before_usermode() isn't defined in the !IS_ENABLED > >>> > case, so either I need to fake up a definition or use #ifdef. > >>> > > >>> > If I faked up a definition, I would want to assert, at build time, that > >>> > it isn't called. I don't think we can do: > >>> > > >>> > static void membarrier_sync_core_before_usermode() > >>> > { > >>> >BUILD_BUG_IF_REACHABLE(); > >>> > } > >>> > >>> Let's look at the context here: > >>> > >>> static void ipi_sync_core(void *info) > >>> { > >>> [] > >>> membarrier_sync_core_before_usermode() > >>> } > >>> > >>> ^ this can be within #ifdef / #endif > >>> > >>> static int membarrier_private_expedited(int flags, int cpu_id) > >>> [...] > >>>if (!IS_ENABLED(CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE)) > >>> return -EINVAL; > >>> if (!(atomic_read(&mm->membarrier_state) & > >>> MEMBARRIER_STATE_PRIVATE_EXPEDITED_SYNC_CORE_READY)) > >>> return -EPERM; > >>> ipi_func = ipi_sync_core; > >>> > >>> All we need to make the line above work is to define an empty > >>> ipi_sync_core > >>> function in the #else case after the ipi_sync_core() function definition. > >>> > >>> Or am I missing your point ? > >> > >> Maybe? > >> > >> My objection is that an empty ipi_sync_core is a lie — it doesn’t sync the > >> core. > >> I would be fine with that if I could have the compiler statically verify > >> that > >> it’s not called, but I’m uncomfortable having it there if the > >> implementation is > >> actively incorrect. > > > > I see. Another approach would be to implement a "setter" function to > > populate > > "ipi_func". That setter function would return -EINVAL in its #ifndef > > CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE > > implementation. > > I still don't get the problem with my suggestion. Sure the > ipi is a "lie", but it doesn't get used. That's how a lot of > ifdef folding works out. E.g., > > diff --git a/kernel/sched/membarrier.c b/kernel/sched/membarrier.c > index b5add64d9698..54cb32d064af 100644 > --- a/kernel/sched/membarrier.c > +++ b/kernel/sched/membarrier.c > @@ -5,6 +5,15 @@ > * membarrier system call > */ > #include "sched.h" > +#ifdef CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE > +#include > +#else > +static inline void membarrier_sync_core_before_usermode(void) > +{ > + compiletime_assert(0, "architecture does not implement > membarrier_sync_core_before_usermode"); > +} > + With the assert there, I’m fine with this. Let me see if the result builds. > +#endif > > /* > * For documentation purposes, here are some membarrier ordering >
Re: [PATCH for 4.16 v7 02/11] powerpc: membarrier: Skip memory barrier in switch_mm()
On Sat, Jun 19, 2021 at 11:35:34AM +0200, Christophe Leroy wrote: > > > Le 18/06/2021 à 19:26, Mathieu Desnoyers a écrit : > >- On Jun 18, 2021, at 1:13 PM, Christophe Leroy > >christophe.le...@csgroup.eu wrote: > >[...] > >> > >>I don't understand all that complexity to just replace a simple > >>'smp_mb__after_unlock_lock()'. > >> > >>#define smp_mb__after_unlock_lock() smp_mb() > >>#define smp_mb()barrier() > >># define barrier() __asm__ __volatile__("": : :"memory") > >> > >> > >>Am I missing some subtility ? > > > >On powerpc CONFIG_SMP, smp_mb() is actually defined as: > > > >#define smp_mb()__smp_mb() > >#define __smp_mb() mb() > >#define mb() __asm__ __volatile__ ("sync" : : : "memory") > > > >So the original motivation here was to skip a "sync" instruction whenever > >switching between threads which are part of the same process. But based on > >recent discussions, I suspect my implementation may be inaccurately doing > >so though. > > > > I see. > > Then, if you think a 'sync' is a concern, shouldn't we try and remove the > forest of 'sync' in the I/O accessors ? > > I can't really understand why we need all those 'sync' and 'isync' and > 'twi' around the accesses whereas I/O memory is usually mapped as 'Guarded' > so memory access ordering is already garantied. > > I'm sure we'll save a lot with that. The point of the twi in the I/O accessors was to make things easier to debug if the accesses fail: for the twi insn to complete the load will have to have completed as well. On a correctly working system you never should need this (until something fails ;-) ) Without the twi you might need to enforce ordering in some cases still. The twi is a very heavy hammer, but some of that that gives us is no doubt actually needed. Segher
Re: [PATCH v2 0/9] powerpc: Add support for Microwatt soft-core
On Fri, Jun 18, 2021 at 01:42:53PM +1000, Paul Mackerras wrote: > This series of patches adds support for the Microwatt soft-core. > Microwatt is an open-source 64-bit Power ISA processor written in VHDL > which targets medium-sized FPGAs such as the Xilinx Artix-7 or the > Lattice ECP5. Microwatt currently implements the scalar fixed plus > floating-point subset of Power ISA v3.0B plus the radix MMU, but not > logical partitioning (i.e. it does not have hypervisor mode or nested > radix translation). For the whole series: Reviewed-by: Segher Boessenkool I didn't see anything in this revision that would prevent it from being included upstream (that HV=1 thing should be fixed sooner rather than later, but that is not a kernel problem). Looks in great state :-) Segher
Re: [PATCH v2 6/9] powerpc/microwatt: Add support for hardware random number generator
On Sat, Jun 19, 2021 at 01:08:51PM +1000, Nicholas Piggin wrote: > Excerpts from Paul Mackerras's message of June 18, 2021 1:47 pm: > > Microwatt's hardware RNG is accessed using the DARN instruction. > > I think we're getting a platforms/book3s soon with the VAS patches, > might be a place to add the get_random_darn function. > > Huh, DARN is unprivileged right? It is, that's the whole point: to make it very very cheap for user software it has to be an unprivileged instruction. Segher
Re: [PATCH v2 2/9] powerpc: Add Microwatt device tree
On Fri, Jun 18, 2021 at 01:44:16PM +1000, Paul Mackerras wrote: > Microwatt currently runs with MSR[HV] = 0, That isn't compliant though? If your implementation does not have LPAR it must set MSR[HV]=1 always. Segher
Re: [PATCH for 4.16 v7 02/11] powerpc: membarrier: Skip memory barrier in switch_mm()
Le 18/06/2021 à 19:26, Mathieu Desnoyers a écrit : - On Jun 18, 2021, at 1:13 PM, Christophe Leroy christophe.le...@csgroup.eu wrote: [...] I don't understand all that complexity to just replace a simple 'smp_mb__after_unlock_lock()'. #define smp_mb__after_unlock_lock() smp_mb() #define smp_mb()barrier() # define barrier() __asm__ __volatile__("": : :"memory") Am I missing some subtility ? On powerpc CONFIG_SMP, smp_mb() is actually defined as: #define smp_mb()__smp_mb() #define __smp_mb() mb() #define mb() __asm__ __volatile__ ("sync" : : : "memory") So the original motivation here was to skip a "sync" instruction whenever switching between threads which are part of the same process. But based on recent discussions, I suspect my implementation may be inaccurately doing so though. I see. Then, if you think a 'sync' is a concern, shouldn't we try and remove the forest of 'sync' in the I/O accessors ? I can't really understand why we need all those 'sync' and 'isync' and 'twi' around the accesses whereas I/O memory is usually mapped as 'Guarded' so memory access ordering is already garantied. I'm sure we'll save a lot with that. Christophe
[Bug 205099] KASAN hit at raid6_pq: BUG: Unable to handle kernel data access at 0x00f0fd0d
https://bugzilla.kernel.org/show_bug.cgi?id=205099 --- Comment #33 from Erhard F. (erhar...@mailbox.org) --- I can tell you in the next few weeks, as soon as my G4 MDD gets its' overhaul with the serviced PSU. -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.