* Re: [PATCH 10/14] uaccess: remove most CONFIG_SET_FS users
From: Christoph Hellwig @ 2022-02-14 17:06 UTC (permalink / raw)
To: Arnd Bergmann
Cc: mark.rutland, dalias, linux-ia64, linux-sh, peterz, linux-mips,
jcmvbkbc, guoren, sparclinux, linux-riscv, will, ardb, linux-arch,
linux-s390, bcain, linux-hexagon, deller, x86, linux, linux-csky,
Christoph Hellwig, mingo, geert, linux-snps-arc, linux-xtensa,
arnd, hca, linux-um, linuxppc-dev, richard, linux-m68k, openrisc,
green.hu, shorne, linux-arm-kernel, monstr, tsbogend, nickhu,
linux-parisc, linux-mm, linux-api, linux-kernel, dinguyen,
ebiederm, linux-alpha, akpm, Linus Torvalds, davem
In-Reply-To: <20220214163452.1568807-11-arnd@kernel.org>
On Mon, Feb 14, 2022 at 05:34:48PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> On almost all architectures, there are no remaining callers
> of set_fs(), so CONFIG_SET_FS can be disabled, along with
> removing the thread_info field and any references to it.
>
> This turns access_ok() into a cheaper check against TASK_SIZE_MAX.
Wouldn't it make more sense to just merge this into the last patch?
^ permalink raw reply
* Re: [PATCH 11/14] sparc64: remove CONFIG_SET_FS support
From: Christoph Hellwig @ 2022-02-14 17:06 UTC (permalink / raw)
To: Arnd Bergmann
Cc: mark.rutland, dalias, linux-ia64, linux-sh, peterz, linux-mips,
jcmvbkbc, guoren, sparclinux, linux-riscv, will, ardb, linux-arch,
linux-s390, bcain, linux-hexagon, deller, x86, linux, linux-csky,
Christoph Hellwig, mingo, geert, linux-snps-arc, linux-xtensa,
arnd, hca, linux-um, linuxppc-dev, richard, linux-m68k, openrisc,
green.hu, shorne, linux-arm-kernel, monstr, tsbogend, nickhu,
linux-parisc, linux-mm, linux-api, linux-kernel, dinguyen,
ebiederm, linux-alpha, akpm, Linus Torvalds, davem
In-Reply-To: <20220214163452.1568807-12-arnd@kernel.org>
> void prom_world(int enter)
> {
> - if (!enter)
> - set_fs(get_fs());
> -
> __asm__ __volatile__("flushw");
> }
The enter argument is now unused.
^ permalink raw reply
* RE: [PATCH 03/14] nds32: fix access_ok() checks in get/put_user
From: David Laight @ 2022-02-14 17:10 UTC (permalink / raw)
To: 'Christoph Hellwig', Arnd Bergmann
Cc: mark.rutland@arm.com, dalias@libc.org, linux-ia64@vger.kernel.org,
linux-sh@vger.kernel.org, peterz@infradead.org,
linux-mips@vger.kernel.org, jcmvbkbc@gmail.com, guoren@kernel.org,
sparclinux@vger.kernel.org, linux-riscv@lists.infradead.org,
will@kernel.org, ardb@kernel.org, linux-arch@vger.kernel.org,
linux-s390@vger.kernel.org, bcain@codeaurora.org,
linux-hexagon@vger.kernel.org, deller@gmx.de, x86@kernel.org,
linux@armlinux.org.uk, linux-csky@vger.kernel.org,
Christoph Hellwig, mingo@redhat.com, geert@linux-m68k.org,
linux-snps-arc@lists.infradead.org, linux-xtensa@linux-xtensa.org,
arnd@arndb.de, hca@linux.ibm.com, linux-um@lists.infradead.org,
linuxppc-dev@lists.ozlabs.org, richard@nod.at,
linux-m68k@lists.linux-m68k.org, openrisc@lists.librecores.org,
green.hu@gmail.com, shorne@gmail.com,
linux-arm-kernel@lists.infradead.org, monstr@monstr.eu,
tsbogend@alpha.franken.de, nickhu@andestech.com,
linux-parisc@vger.kernel.org, linux-mm@kvack.org,
linux-api@vger.kernel.org, linux-kernel@vger.kernel.org,
stable@vger.kernel.org, dinguyen@kernel.org,
ebiederm@xmission.com, linux-alpha@vger.kernel.org,
akpm@linux-foundation.org, Linus Torvalds, davem@davemloft.net
In-Reply-To: <YgqK1ihlJvRFHJ9h@infradead.org>
From: Christoph Hellwig
> Sent: 14 February 2022 17:01
>
> On Mon, Feb 14, 2022 at 05:34:41PM +0100, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > The get_user()/put_user() functions are meant to check for
> > access_ok(), while the __get_user()/__put_user() functions
> > don't.
> >
> > This broke in 4.19 for nds32, when it gained an extraneous
> > check in __get_user(), but lost the check it needs in
> > __put_user().
>
> Can we follow the lead of MIPS (which this was originally copied
> from I think) and kill the pointless __get/put_user_check wrapper
> that just obsfucate the code?
Is it possible to make all these architectures fall back to
a common definition somewhere?
Maybe they need to define ACCESS_OK_USER_LIMIT - which can be
different from TASK_SIZE.
There'll be a few special cases, but most architectures have
kernel addresses above userspace ones.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH v5 2/6] powerpc/kexec_file: Add KEXEC_SIG support.
From: Mimi Zohar @ 2022-02-14 17:09 UTC (permalink / raw)
To: Michal Suchánek
Cc: Nayna, Sven Schnelle, David Howells, keyrings, Paul Mackerras,
Alexander Gordeev, Rob Herring, Herbert Xu, Baoquan He,
Christian Borntraeger, James Morris, Lakshmi Ramasubramanian,
Christian Borntraeger, Serge E. Hallyn, Vasily Gorbik, linux-s390,
Heiko Carstens, Dmitry Kasatkin, Hari Bathini, Daniel Axtens,
Philipp Rudo, Frank van der Linden, kexec, linux-kernel,
Luis Chamberlain, linux-crypto, linux-security-module, Jessica Yu,
linux-integrity, linuxppc-dev, David S. Miller,
Thiago Jung Bauermann, buendgen
In-Reply-To: <20220214155524.GN3113@kunlun.suse.cz>
On Mon, 2022-02-14 at 16:55 +0100, Michal Suchánek wrote:
> Hello,
>
> On Mon, Feb 14, 2022 at 10:14:16AM -0500, Mimi Zohar wrote:
> > Hi Michal,
> >
> > On Sun, 2022-02-13 at 21:59 -0500, Mimi Zohar wrote:
> >
> > >
> > > On Tue, 2022-01-11 at 12:37 +0100, Michal Suchanek wrote:
> > > > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> > > > index dea74d7717c0..1cde9b6c5987 100644
> > > > --- a/arch/powerpc/Kconfig
> > > > +++ b/arch/powerpc/Kconfig
> > > > @@ -560,6 +560,22 @@ config KEXEC_FILE
> > > > config ARCH_HAS_KEXEC_PURGATORY
> > > > def_bool KEXEC_FILE
> > > >
> > > > +config KEXEC_SIG
> > > > + bool "Verify kernel signature during kexec_file_load() syscall"
> > > > + depends on KEXEC_FILE && MODULE_SIG_FORMAT
> > > > + help
> > > > + This option makes kernel signature verification mandatory for
>
> This is actually wrong. KEXEC_SIG makes it mandatory that any signature
> that is appended is valid and made by a key that is part of the platform
> keyiring (which is also wrong, built-in keys should be also accepted).
> KEXEC_SIG_FORCE or an IMA policy makes it mandatory that the signature
> is present.
I'm aware of MODULE_SIG_FORCE, which isn't normally enabled by distros,
but enabling MODULE_SIG allows MODULE_SIG_FORCE to be enabled on the
boot command line. In the IMA arch policies, if MODULE_SIG is enabled,
it is then enforced, otherwise an IMA "appraise" policy rule is
defined. This rule would prevent the module_load syscall.
I'm not aware of KEXEC_SIG_FORCE. If there is such a Kconfig, then I
assume it could work similarly.
>
> > > > + the kexec_file_load() syscall.
> > >
> > > When KEXEC_SIG is enabled on other architectures, IMA does not define a
> > > kexec 'appraise' policy rule. Refer to the policy rules in
> > > security/ima/ima_efi.c. Similarly the kexec 'appraise' policy rule in
>
> I suppose you mean security/integrity/ima/ima_efi.c
Yes
>
> I also think it's misguided because KEXEC_SIG in itself does not enforce
> the signature. KEXEC_SIG_FORCE does.
Right, which is why the IMA efi policy calls set_module_sig_enforced().
>
> > > arch/powerpc/kernel/ima_policy.c should not be defined.
>
> I suppose you mean arch/powerpc/kernel/ima_arch.c - see above.
Sorry, yes.
>
>
> Thanks for taking the time to reseach and summarize the differences.
>
> > The discussion shouldn't only be about IMA vs. KEXEC_SIG kernel image
> > signature verification. Let's try and reframe the problem a bit.
> >
> > 1. Unify and simply the existing kexec signature verification so
> > verifying the KEXEC kernel image signature works irrespective of
> > signature type - PE, appended signature.
> >
> > solution: enable KEXEC_SIG (This patch set, with the above powerpc IMA
> > policy changes.)
> >
> > 2. Measure and include the kexec kernel image in a log for attestation,
> > if desired.
> >
> > solution: enable IMA_ARCH_POLICY
> > - Powerpc: requires trusted boot to be enabled.
> > - EFI: requires secure boot to be enabled. The IMA efi policy
> > doesn't differentiate between secure and trusted boot.
> >
> > 3. Carry the kexec kernel image measurement across kexec, if desired
> > and supported on the architecture.
> >
> > solution: enable IMA_KEXEC
> >
> > Comparison:
> > - Are there any differences between IMA vs. KEXEC_SIG measuring the
> > kexec kernel image?
> >
> > One of the main differences is "what" is included in the measurement
> > list differs. In both cases, the 'd-ng' field of the IMA measurement
> > list template (e.g. ima-ng, ima-sig, ima-modsig) is the full file hash
> > including the appended signature. With IMA and the 'ima-modsig'
> > template, an additional hash without the appended signature is defined,
> > as well as including the appended signature in the 'sig' field.
> >
> > Including the file hash and appended signature in the measurement list
> > allows an attestation server, for example, to verify the appended
> > signature without having to know the file hash without the signature.
>
> I don't understand this part. Isn't the hash *with* signature always
> included, and the distinguishing part about IMA is the hash *without*
> signature which is the same irrespective of signature type (PE, appended
> xattr) and irrespective of the keyt used for signoing?
Roberto Sassu added support for IMA templates. These are the
definitions of 'ima-sig' and 'ima-modsig'.
{.name = "ima-sig", .fmt = "d-ng|n-ng|sig"},
{.name = "ima-modsig", .fmt = "d-ng|n-ng|sig|d-modsig|modsig"}
d-ng: is the file hash. With the proposed IMA support for fs-verity
digests, the 'd-ng' field may also include the fsverity digest, based
on policy.
n-ng: is the file pathname.
sig: is the file signature stored as a 'security.ima' xattr (may be
NULL).
d-modsig: is the file hash without the appended signature (may be
NULL).
FYI, changing from "module signature" to "appended signature", might
impact the template field and name. :)
modsig: is the appended signature (May be NULL).
I really haven't looked at PE signatures, so I can't comment on them.
>
> > Other differences are already included in the Kconfig KEXEC_SIG "Notes"
> > section.
>
> Which besides what is already described above would be blacklisting
> specific binaries, which is much more effective if you have hashes of
> binaries without signature.
Thanks, Nayna will be happy to hear you approve.
FYI, IMA calculates the file hash once, which is then added to the IMA
measurement list, extended into the TPM (when available), used to
verify signatures, and included in the audit log.
With the KEXEC_SIG support, assuming the IMA arch policy is enabled,
the file hash would be calculated twice - once for verifying the file
signature and again for the measurement.
--
thanks,
Mimi
^ permalink raw reply
* Re: [PATCH v2 11/13] powerpc/ftrace: directly call of function graph tracer by ftrace caller
From: Naveen N. Rao @ 2022-02-14 17:24 UTC (permalink / raw)
To: Christophe Leroy, Jiri Kosina, Joe Lawrence, Josh Poimboeuf,
Miroslav Benes, Ingo Molnar, Petr Mladek, Steven Rostedt
Cc: live-patching@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org
In-Reply-To: <04d196585ff81bde06a000bd9c633a33a5b21130.1640017960.git.christophe.leroy@csgroup.eu>
Christophe Leroy wrote:
> Modify function graph tracer to be handled directly by the standard
> ftrace caller.
>
> This is made possible as powerpc now supports
> CONFIG_DYNAMIC_FTRACE_WITH_ARGS.
>
> This change simplifies the call of function graph ftrace.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/powerpc/include/asm/ftrace.h | 6 ++
> arch/powerpc/kernel/trace/ftrace.c | 11 ++++
> arch/powerpc/kernel/trace/ftrace_32.S | 53 +--------------
> .../powerpc/kernel/trace/ftrace_64_mprofile.S | 64 +------------------
> 4 files changed, 20 insertions(+), 114 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h
> index 45c3d6f11daa..70b457097098 100644
> --- a/arch/powerpc/include/asm/ftrace.h
> +++ b/arch/powerpc/include/asm/ftrace.h
> @@ -38,6 +38,12 @@ static __always_inline void ftrace_instruction_pointer_set(struct ftrace_regs *f
> {
> regs_set_return_ip(&fregs->regs, ip);
> }
> +
> +struct ftrace_ops;
> +
> +#define ftrace_graph_func ftrace_graph_func
> +void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
> + struct ftrace_ops *op, struct ftrace_regs *fregs);
> #endif
> #endif /* __ASSEMBLY__ */
>
> diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
> index ce673764cb69..74a176e394ef 100644
> --- a/arch/powerpc/kernel/trace/ftrace.c
> +++ b/arch/powerpc/kernel/trace/ftrace.c
> @@ -917,6 +917,9 @@ static int ftrace_modify_ftrace_graph_caller(bool enable)
> unsigned long stub = (unsigned long)(&ftrace_graph_stub);
> ppc_inst_t old, new;
>
> + if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_ARGS))
> + return 0;
> +
> old = ftrace_call_replace(ip, enable ? stub : addr, 0);
> new = ftrace_call_replace(ip, enable ? addr : stub, 0);
>
> @@ -955,6 +958,14 @@ unsigned long prepare_ftrace_return(unsigned long parent, unsigned long ip,
> out:
> return parent;
> }
For x86, commit 0c0593b45c9b4e ("x86/ftrace: Make function graph use
ftrace directly") also adds recursion check before the call to
function_graph_enter() in prepare_ftrace_return(). Do we need that on
powerpc as well?
- Naveen
^ permalink raw reply
* Re: [PATCH 07/14] uaccess: generalize access_ok()
From: Al Viro @ 2022-02-14 17:15 UTC (permalink / raw)
To: Arnd Bergmann
Cc: mark.rutland, dalias, linux-ia64, linux-sh, peterz, linux-mips,
linux-mm, guoren, sparclinux, linux-hexagon, linux-riscv, will,
Christoph Hellwig, linux-arch, linux-s390, bcain, deller, x86,
linux, linux-csky, ardb, mingo, geert, linux-snps-arc,
linux-xtensa, arnd, hca, linux-alpha, linux-um, linuxppc-dev,
linux-m68k, openrisc, green.hu, shorne, linux-arm-kernel, monstr,
tsbogend, linux-parisc, nickhu, jcmvbkbc, linux-api, linux-kernel,
dinguyen, ebiederm, richard, akpm, Linus Torvalds, davem
In-Reply-To: <20220214163452.1568807-8-arnd@kernel.org>
On Mon, Feb 14, 2022 at 05:34:45PM +0100, Arnd Bergmann wrote:
> diff --git a/arch/csky/kernel/signal.c b/arch/csky/kernel/signal.c
> index c7b763d2f526..8867ddf3e6c7 100644
> --- a/arch/csky/kernel/signal.c
> +++ b/arch/csky/kernel/signal.c
> @@ -136,7 +136,7 @@ static inline void __user *get_sigframe(struct ksignal *ksig,
> static int
> setup_rt_frame(struct ksignal *ksig, sigset_t *set, struct pt_regs *regs)
> {
> - struct rt_sigframe *frame;
> + struct rt_sigframe __user *frame;
> int err = 0;
>
> frame = get_sigframe(ksig, regs, sizeof(*frame));
Minor nit: might make sense to separate annotations (here, on nios2, etc.) from the rest...
This, OTOH,
> diff --git a/arch/sparc/include/asm/uaccess_64.h b/arch/sparc/include/asm/uaccess_64.h
> index 5c12fb46bc61..000bac67cf31 100644
> --- a/arch/sparc/include/asm/uaccess_64.h
> +++ b/arch/sparc/include/asm/uaccess_64.h
...
> -static inline bool __chk_range_not_ok(unsigned long addr, unsigned long size, unsigned long limit)
> -{
> - if (__builtin_constant_p(size))
> - return addr > limit - size;
> -
> - addr += size;
> - if (addr < size)
> - return true;
> -
> - return addr > limit;
> -}
> -
> -#define __range_not_ok(addr, size, limit) \
> -({ \
> - __chk_user_ptr(addr); \
> - __chk_range_not_ok((unsigned long __force)(addr), size, limit); \
> -})
> -
> -static inline int __access_ok(const void __user * addr, unsigned long size)
> -{
> - return 1;
> -}
> -
> -static inline int access_ok(const void __user * addr, unsigned long size)
> -{
> - return 1;
> -}
> +#define __range_not_ok(addr, size, limit) (!__access_ok(addr, size))
is really wrong. For sparc64, access_ok() should always be true.
This __range_not_ok() thing is used *only* for valid_user_frame() in
arch/sparc/kernel/perf_event.c - it's not a part of normal access_ok()
there.
sparc64 has separate address spaces for kernel and for userland; access_ok()
had never been useful there.
^ permalink raw reply
* Re: [PATCH 00/14] clean up asm/uaccess.h, kill set_fs for good
From: Linus Torvalds @ 2022-02-14 17:35 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Mark Rutland, Rich Felker, linux-ia64, Linux-sh list,
Peter Zijlstra, open list:BROADCOM NVRAM DRIVER, Linux-MM,
Guo Ren, linux-sparc, linux-hexagon, linux-riscv, Will Deacon,
Christoph Hellwig, linux-arch, linux-s390, Brian Cain,
Helge Deller, the arch/x86 maintainers, Russell King - ARM Linux,
linux-csky, Ard Biesheuvel, Ingo Molnar, Geert Uytterhoeven,
open list:SYNOPSYS ARC ARCHITECTURE, linux-xtensa, Arnd Bergmann,
Heiko Carstens, alpha, linux-um, linux-m68k, openrisc,
Greentime Hu, Stafford Horne, Linux ARM, Michal Simek,
Thomas Bogendoerfer, linux-parisc, Nick Hu, Max Filippov,
Linux API, Linux Kernel Mailing List, Dinh Nguyen,
Eric W. Biederman, Richard Weinberger, Andrew Morton,
linuxppc-dev, David Miller
In-Reply-To: <20220214163452.1568807-1-arnd@kernel.org>
On Mon, Feb 14, 2022 at 8:35 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> I did a patch for microblaze at some point, which turned out to be fairly
> generic, and now ported it to most other architectures, using new generic
> implementations of access_ok() and __{get,put}_kernel_nocheck().
Thanks for doing this.
Apart from the sparc64 issue with completely separate address spaces
(so access_ok() should always return true like Al pointed out), this
looks excellent to me.
Somebody should check that there aren't other cases like sparc64, but
let's merge this asap other than that.
Linus
^ permalink raw reply
* [PATCH v1 0/2] mm: enforce pageblock_order < MAX_ORDER
From: David Hildenbrand @ 2022-02-14 17:41 UTC (permalink / raw)
To: linux-kernel
Cc: devicetree, linuxppc-dev, David Hildenbrand, Aneesh Kumar K V,
Michael S. Tsirkin, Robin Murphy, virtualization, linux-mm,
Minchan Kim, iommu, Rob Herring, Paul Mackerras, Zi Yan,
Andrew Morton, Frank Rowand, Christoph Hellwig, Vlastimil Babka,
Marek Szyprowski
Having pageblock_order >= MAX_ORDER seems to be able to happen in corner
cases and some parts of the kernel are not prepared for it.
For example, Aneesh has shown [1] that such kernels can be compiled on
ppc64 with 64k base pages by setting FORCE_MAX_ZONEORDER=8, which will run
into a WARN_ON_ONCE(order >= MAX_ORDER) in comapction code right during
boot.
We can get pageblock_order >= MAX_ORDER when the default hugetlb size is
bigger than the maximum allocation granularity of the buddy, in which case
we are no longer talking about huge pages but instead gigantic pages.
Having pageblock_order >= MAX_ORDER can only make alloc_contig_range() of
such gigantic pages more likely to succeed.
Reliable use of gigantic pages either requires boot time allcoation or CMA,
no need to overcomplicate some places in the kernel to optimize for corner
cases that are broken in other areas of the kernel.
Let's enforce pageblock_order < MAX_ORDER and simplify.
Especially patch #1 can be regarded a cleanup before:
[PATCH v5 0/6] Use pageblock_order for cma and alloc_contig_range
alignment. [2]
[1] https://lkml.kernel.org/r/87r189a2ks.fsf@linux.ibm.com
[2] https://lkml.kernel.org/r/20220211164135.1803616-1-zi.yan@sent.com
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Aneesh Kumar K V <aneesh.kumar@linux.ibm.com>
Cc: Zi Yan <ziy@nvidia.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: devicetree@vger.kernel.org
Cc: virtualization@lists.linux-foundation.org
Cc: iommu@lists.linux-foundation.org
Cc: linux-mm@kvack.org
David Hildenbrand (2):
cma: factor out minimum alignment requirement
mm: enforce pageblock_order < MAX_ORDER
arch/powerpc/include/asm/fadump-internal.h | 5 ----
arch/powerpc/kernel/fadump.c | 2 +-
drivers/of/of_reserved_mem.c | 9 ++----
drivers/virtio/virtio_mem.c | 9 ++----
include/linux/cma.h | 8 ++++++
include/linux/pageblock-flags.h | 7 +++--
kernel/dma/contiguous.c | 4 +--
mm/Kconfig | 3 ++
mm/cma.c | 20 ++++----------
mm/page_alloc.c | 32 ++++++----------------
10 files changed, 37 insertions(+), 62 deletions(-)
base-commit: 754e0b0e35608ed5206d6a67a791563c631cec07
--
2.34.1
^ permalink raw reply
* [PATCH v1 1/2] cma: factor out minimum alignment requirement
From: David Hildenbrand @ 2022-02-14 17:41 UTC (permalink / raw)
To: linux-kernel
Cc: devicetree, linuxppc-dev, David Hildenbrand, Aneesh Kumar K V,
Michael S. Tsirkin, Robin Murphy, virtualization, linux-mm,
Minchan Kim, iommu, Rob Herring, Paul Mackerras, Zi Yan,
Andrew Morton, Frank Rowand, Christoph Hellwig, Vlastimil Babka,
Marek Szyprowski
In-Reply-To: <20220214174132.219303-1-david@redhat.com>
Let's factor out determining the minimum alignment requirement for CMA
and add a helpful comment.
No functional change intended.
Signed-off-by: David Hildenbrand <david@redhat.com>
---
arch/powerpc/include/asm/fadump-internal.h | 5 -----
arch/powerpc/kernel/fadump.c | 2 +-
drivers/of/of_reserved_mem.c | 9 +++------
include/linux/cma.h | 9 +++++++++
kernel/dma/contiguous.c | 4 +---
mm/cma.c | 20 +++++---------------
6 files changed, 19 insertions(+), 30 deletions(-)
diff --git a/arch/powerpc/include/asm/fadump-internal.h b/arch/powerpc/include/asm/fadump-internal.h
index 52189928ec08..81bcb9abb371 100644
--- a/arch/powerpc/include/asm/fadump-internal.h
+++ b/arch/powerpc/include/asm/fadump-internal.h
@@ -19,11 +19,6 @@
#define memblock_num_regions(memblock_type) (memblock.memblock_type.cnt)
-/* Alignment per CMA requirement. */
-#define FADUMP_CMA_ALIGNMENT (PAGE_SIZE << \
- max_t(unsigned long, MAX_ORDER - 1, \
- pageblock_order))
-
/* FAD commands */
#define FADUMP_REGISTER 1
#define FADUMP_UNREGISTER 2
diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
index d03e488cfe9c..7eb67201ea41 100644
--- a/arch/powerpc/kernel/fadump.c
+++ b/arch/powerpc/kernel/fadump.c
@@ -544,7 +544,7 @@ int __init fadump_reserve_mem(void)
if (!fw_dump.nocma) {
fw_dump.boot_memory_size =
ALIGN(fw_dump.boot_memory_size,
- FADUMP_CMA_ALIGNMENT);
+ CMA_MIN_ALIGNMENT_BYTES);
}
#endif
diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 9c0fb962c22b..75caa6f5d36f 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -22,6 +22,7 @@
#include <linux/slab.h>
#include <linux/memblock.h>
#include <linux/kmemleak.h>
+#include <linux/cma.h>
#include "of_private.h"
@@ -116,12 +117,8 @@ static int __init __reserved_mem_alloc_size(unsigned long node,
if (IS_ENABLED(CONFIG_CMA)
&& of_flat_dt_is_compatible(node, "shared-dma-pool")
&& of_get_flat_dt_prop(node, "reusable", NULL)
- && !nomap) {
- unsigned long order =
- max_t(unsigned long, MAX_ORDER - 1, pageblock_order);
-
- align = max(align, (phys_addr_t)PAGE_SIZE << order);
- }
+ && !nomap)
+ align = max_t(phys_addr_t, align, CMA_MIN_ALIGNMENT_BYTES);
prop = of_get_flat_dt_prop(node, "alloc-ranges", &len);
if (prop) {
diff --git a/include/linux/cma.h b/include/linux/cma.h
index bd801023504b..75fe188ec4a1 100644
--- a/include/linux/cma.h
+++ b/include/linux/cma.h
@@ -20,6 +20,15 @@
#define CMA_MAX_NAME 64
+/*
+ * TODO: once the buddy -- especially pageblock merging and alloc_contig_range()
+ * -- can deal with only some pageblocks of a higher-order page being
+ * MIGRATE_CMA, we can use pageblock_nr_pages.
+ */
+#define CMA_MIN_ALIGNMENT_PAGES max_t(phys_addr_t, MAX_ORDER_NR_PAGES, \
+ pageblock_nr_pages)
+#define CMA_MIN_ALIGNMENT_BYTES (PAGE_SIZE * CMA_MIN_ALIGNMENT_PAGES)
+
struct cma;
extern unsigned long totalcma_pages;
diff --git a/kernel/dma/contiguous.c b/kernel/dma/contiguous.c
index 3d63d91cba5c..6ea80ae42622 100644
--- a/kernel/dma/contiguous.c
+++ b/kernel/dma/contiguous.c
@@ -399,8 +399,6 @@ static const struct reserved_mem_ops rmem_cma_ops = {
static int __init rmem_cma_setup(struct reserved_mem *rmem)
{
- phys_addr_t align = PAGE_SIZE << max(MAX_ORDER - 1, pageblock_order);
- phys_addr_t mask = align - 1;
unsigned long node = rmem->fdt_node;
bool default_cma = of_get_flat_dt_prop(node, "linux,cma-default", NULL);
struct cma *cma;
@@ -416,7 +414,7 @@ static int __init rmem_cma_setup(struct reserved_mem *rmem)
of_get_flat_dt_prop(node, "no-map", NULL))
return -EINVAL;
- if ((rmem->base & mask) || (rmem->size & mask)) {
+ if (!IS_ALIGNED(rmem->base | rmem->size, CMA_MIN_ALIGNMENT_BYTES)) {
pr_err("Reserved memory: incorrect alignment of CMA region\n");
return -EINVAL;
}
diff --git a/mm/cma.c b/mm/cma.c
index bc9ca8f3c487..5a2cd5851658 100644
--- a/mm/cma.c
+++ b/mm/cma.c
@@ -168,7 +168,6 @@ int __init cma_init_reserved_mem(phys_addr_t base, phys_addr_t size,
struct cma **res_cma)
{
struct cma *cma;
- phys_addr_t alignment;
/* Sanity checks */
if (cma_area_count == ARRAY_SIZE(cma_areas)) {
@@ -179,15 +178,12 @@ int __init cma_init_reserved_mem(phys_addr_t base, phys_addr_t size,
if (!size || !memblock_is_region_reserved(base, size))
return -EINVAL;
- /* ensure minimal alignment required by mm core */
- alignment = PAGE_SIZE <<
- max_t(unsigned long, MAX_ORDER - 1, pageblock_order);
-
/* alignment should be aligned with order_per_bit */
- if (!IS_ALIGNED(alignment >> PAGE_SHIFT, 1 << order_per_bit))
+ if (!IS_ALIGNED(CMA_MIN_ALIGNMENT_PAGES, 1 << order_per_bit))
return -EINVAL;
- if (ALIGN(base, alignment) != base || ALIGN(size, alignment) != size)
+ /* ensure minimal alignment required by mm core */
+ if (!IS_ALIGNED(base | size, CMA_MIN_ALIGNMENT_BYTES))
return -EINVAL;
/*
@@ -262,14 +258,8 @@ int __init cma_declare_contiguous_nid(phys_addr_t base,
if (alignment && !is_power_of_2(alignment))
return -EINVAL;
- /*
- * Sanitise input arguments.
- * Pages both ends in CMA area could be merged into adjacent unmovable
- * migratetype page by page allocator's buddy algorithm. In the case,
- * you couldn't get a contiguous memory, which is not what we want.
- */
- alignment = max(alignment, (phys_addr_t)PAGE_SIZE <<
- max_t(unsigned long, MAX_ORDER - 1, pageblock_order));
+ /* Sanitise input arguments. */
+ alignment = max_t(phys_addr_t, alignment, CMA_MIN_ALIGNMENT_BYTES);
if (fixed && base & (alignment - 1)) {
ret = -EINVAL;
pr_err("Region at %pa must be aligned to %pa bytes\n",
--
2.34.1
^ permalink raw reply related
* [PATCH v1 2/2] mm: enforce pageblock_order < MAX_ORDER
From: David Hildenbrand @ 2022-02-14 17:41 UTC (permalink / raw)
To: linux-kernel
Cc: devicetree, linuxppc-dev, David Hildenbrand, Aneesh Kumar K V,
Michael S. Tsirkin, Robin Murphy, virtualization, linux-mm,
Minchan Kim, iommu, Rob Herring, Paul Mackerras, Zi Yan,
Andrew Morton, Frank Rowand, Christoph Hellwig, Vlastimil Babka,
Marek Szyprowski
In-Reply-To: <20220214174132.219303-1-david@redhat.com>
Some places in the kernel don't really expect pageblock_order >=
MAX_ORDER, and it looks like this is only possible in corner cases:
1) CONFIG_DEFERRED_STRUCT_PAGE_INIT we'll end up freeing pageblock_order
pages via __free_pages_core(), which cannot possibly work.
2) find_zone_movable_pfns_for_nodes() will roundup the ZONE_MOVABLE
start PFN to MAX_ORDER_NR_PAGES. Consequently with a bigger
pageblock_order, we could have a single pageblock partially managed by
two zones.
3) compaction code runs into __fragmentation_index() with order
>= MAX_ORDER, when checking WARN_ON_ONCE(order >= MAX_ORDER). [1]
4) mm/page_reporting.c won't be reporting any pages with default
page_reporting_order == pageblock_order, as we'll be skipping the
reporting loop inside page_reporting_process_zone().
5) __rmqueue_fallback() will never be able to steal with
ALLOC_NOFRAGMENT.
pageblock_order >= MAX_ORDER is weird either way: it's a pure
optimization for making alloc_contig_range(), as used for allcoation of
gigantic pages, a little more reliable to succeed. However, if there is
demand for somewhat reliable allocation of gigantic pages, affected setups
should be using CMA or boottime allocations instead.
So let's make sure that pageblock_order < MAX_ORDER and simplify.
[1] https://lkml.kernel.org/r/87r189a2ks.fsf@linux.ibm.com
Signed-off-by: David Hildenbrand <david@redhat.com>
---
drivers/virtio/virtio_mem.c | 9 +++------
include/linux/cma.h | 3 +--
include/linux/pageblock-flags.h | 7 +++++--
mm/Kconfig | 3 +++
mm/page_alloc.c | 32 ++++++++------------------------
5 files changed, 20 insertions(+), 34 deletions(-)
diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
index 38becd8d578c..e7d6b679596d 100644
--- a/drivers/virtio/virtio_mem.c
+++ b/drivers/virtio/virtio_mem.c
@@ -2476,13 +2476,10 @@ static int virtio_mem_init_hotplug(struct virtio_mem *vm)
VIRTIO_MEM_DEFAULT_OFFLINE_THRESHOLD);
/*
- * We want subblocks to span at least MAX_ORDER_NR_PAGES and
- * pageblock_nr_pages pages. This:
- * - Is required for now for alloc_contig_range() to work reliably -
- * it doesn't properly handle smaller granularity on ZONE_NORMAL.
+ * TODO: once alloc_contig_range() works reliably with pageblock
+ * granularity on ZONE_NORMAL, use pageblock_nr_pages instead.
*/
- sb_size = max_t(uint64_t, MAX_ORDER_NR_PAGES,
- pageblock_nr_pages) * PAGE_SIZE;
+ sb_size = PAGE_SIZE * MAX_ORDER_NR_PAGES;
sb_size = max_t(uint64_t, vm->device_block_size, sb_size);
if (sb_size < memory_block_size_bytes() && !force_bbm) {
diff --git a/include/linux/cma.h b/include/linux/cma.h
index 75fe188ec4a1..b1ba94f1cc9c 100644
--- a/include/linux/cma.h
+++ b/include/linux/cma.h
@@ -25,8 +25,7 @@
* -- can deal with only some pageblocks of a higher-order page being
* MIGRATE_CMA, we can use pageblock_nr_pages.
*/
-#define CMA_MIN_ALIGNMENT_PAGES max_t(phys_addr_t, MAX_ORDER_NR_PAGES, \
- pageblock_nr_pages)
+#define CMA_MIN_ALIGNMENT_PAGES MAX_ORDER_NR_PAGES
#define CMA_MIN_ALIGNMENT_BYTES (PAGE_SIZE * CMA_MIN_ALIGNMENT_PAGES)
struct cma;
diff --git a/include/linux/pageblock-flags.h b/include/linux/pageblock-flags.h
index 973fd731a520..83c7248053a1 100644
--- a/include/linux/pageblock-flags.h
+++ b/include/linux/pageblock-flags.h
@@ -37,8 +37,11 @@ extern unsigned int pageblock_order;
#else /* CONFIG_HUGETLB_PAGE_SIZE_VARIABLE */
-/* Huge pages are a constant size */
-#define pageblock_order HUGETLB_PAGE_ORDER
+/*
+ * Huge pages are a constant size, but don't exceed the maximum allocation
+ * granularity.
+ */
+#define pageblock_order min_t(unsigned int, HUGETLB_PAGE_ORDER, MAX_ORDER - 1)
#endif /* CONFIG_HUGETLB_PAGE_SIZE_VARIABLE */
diff --git a/mm/Kconfig b/mm/Kconfig
index 3326ee3903f3..4c91b92e7537 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -262,6 +262,9 @@ config HUGETLB_PAGE_SIZE_VARIABLE
HUGETLB_PAGE_ORDER when there are multiple HugeTLB page sizes available
on a platform.
+ Note that the pageblock_order cannot exceed MAX_ORDER - 1 and will be
+ clamped down to MAX_ORDER - 1.
+
config CONTIG_ALLOC
def_bool (MEMORY_ISOLATION && COMPACTION) || CMA
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 3589febc6d31..04cf964b57b5 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1072,14 +1072,12 @@ static inline void __free_one_page(struct page *page,
int migratetype, fpi_t fpi_flags)
{
struct capture_control *capc = task_capc(zone);
+ unsigned int max_order = pageblock_order;
unsigned long buddy_pfn;
unsigned long combined_pfn;
- unsigned int max_order;
struct page *buddy;
bool to_tail;
- max_order = min_t(unsigned int, MAX_ORDER - 1, pageblock_order);
-
VM_BUG_ON(!zone_is_initialized(zone));
VM_BUG_ON_PAGE(page->flags & PAGE_FLAGS_CHECK_AT_PREP, page);
@@ -2260,19 +2258,8 @@ void __init init_cma_reserved_pageblock(struct page *page)
} while (++p, --i);
set_pageblock_migratetype(page, MIGRATE_CMA);
-
- if (pageblock_order >= MAX_ORDER) {
- i = pageblock_nr_pages;
- p = page;
- do {
- set_page_refcounted(p);
- __free_pages(p, MAX_ORDER - 1);
- p += MAX_ORDER_NR_PAGES;
- } while (i -= MAX_ORDER_NR_PAGES);
- } else {
- set_page_refcounted(page);
- __free_pages(page, pageblock_order);
- }
+ set_page_refcounted(page);
+ __free_pages(page, pageblock_order);
adjust_managed_page_count(page, pageblock_nr_pages);
page_zone(page)->cma_pages += pageblock_nr_pages;
@@ -7389,16 +7376,15 @@ static inline void setup_usemap(struct zone *zone) {}
/* Initialise the number of pages represented by NR_PAGEBLOCK_BITS */
void __init set_pageblock_order(void)
{
- unsigned int order;
+ unsigned int order = MAX_ORDER - 1;
/* Check that pageblock_nr_pages has not already been setup */
if (pageblock_order)
return;
- if (HPAGE_SHIFT > PAGE_SHIFT)
+ /* Don't let pageblocks exceed the maximum allocation granularity. */
+ if (HPAGE_SHIFT > PAGE_SHIFT && HUGETLB_PAGE_ORDER < order)
order = HUGETLB_PAGE_ORDER;
- else
- order = MAX_ORDER - 1;
/*
* Assume the largest contiguous order of interest is a huge page.
@@ -8986,14 +8972,12 @@ struct page *has_unmovable_pages(struct zone *zone, struct page *page,
#ifdef CONFIG_CONTIG_ALLOC
static unsigned long pfn_max_align_down(unsigned long pfn)
{
- return pfn & ~(max_t(unsigned long, MAX_ORDER_NR_PAGES,
- pageblock_nr_pages) - 1);
+ return ALIGN_DOWN(pfn, MAX_ORDER_NR_PAGES);
}
static unsigned long pfn_max_align_up(unsigned long pfn)
{
- return ALIGN(pfn, max_t(unsigned long, MAX_ORDER_NR_PAGES,
- pageblock_nr_pages));
+ return ALIGN(pfn, MAX_ORDER_NR_PAGES);
}
#if defined(CONFIG_DYNAMIC_DEBUG) || \
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v2 12/13] powerpc/ftrace: Prepare ftrace_64_mprofile.S for reuse by PPC32
From: Naveen N. Rao @ 2022-02-14 17:51 UTC (permalink / raw)
To: Christophe Leroy, Jiri Kosina, Joe Lawrence, Josh Poimboeuf,
Miroslav Benes, Ingo Molnar, Petr Mladek, Steven Rostedt
Cc: live-patching@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org
In-Reply-To: <82a732915dc71ee766e31809350939331944006d.1640017960.git.christophe.leroy@csgroup.eu>
Christophe Leroy wrote:
> PPC64 mprofile versions and PPC32 are very similar.
>
> Modify PPC64 version so that if can be reused for PPC32.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> .../powerpc/kernel/trace/ftrace_64_mprofile.S | 73 +++++++++++++------
> 1 file changed, 51 insertions(+), 22 deletions(-)
While I agree that ppc32 and -mprofile-kernel ftrace code are very
similar, I think this patch adds way too many #ifdefs. IMHO, this
makes the resultant code quite difficult to follow.
- Naveen
>
> diff --git a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
> index 6071e0122797..56da60e98327 100644
> --- a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
> +++ b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
> @@ -34,13 +34,16 @@
> */
> _GLOBAL(ftrace_regs_caller)
> /* Save the original return address in A's stack frame */
> - std r0,LRSAVE(r1)
> +#ifdef CONFIG_MPROFILE_KERNEL
> + PPC_STL r0,LRSAVE(r1)
> +#endif
>
> /* Create our stack frame + pt_regs */
> - stdu r1,-SWITCH_FRAME_SIZE(r1)
> + PPC_STLU r1,-SWITCH_FRAME_SIZE(r1)
>
> /* Save all gprs to pt_regs */
> SAVE_GPR(0, r1)
> +#ifdef CONFIG_PPC64
> SAVE_GPRS(2, 11, r1)
>
> /* Ok to continue? */
> @@ -49,10 +52,13 @@ _GLOBAL(ftrace_regs_caller)
> beq ftrace_no_trace
>
> SAVE_GPRS(12, 31, r1)
> +#else
> + stmw r2, GPR2(r1)
> +#endif
>
> /* Save previous stack pointer (r1) */
> addi r8, r1, SWITCH_FRAME_SIZE
> - std r8, GPR1(r1)
> + PPC_STL r8, GPR1(r1)
>
> /* Load special regs for save below */
> mfmsr r8
> @@ -63,10 +69,11 @@ _GLOBAL(ftrace_regs_caller)
> /* Get the _mcount() call site out of LR */
> mflr r7
> /* Save it as pt_regs->nip */
> - std r7, _NIP(r1)
> + PPC_STL r7, _NIP(r1)
> /* Save the read LR in pt_regs->link */
> - std r0, _LINK(r1)
> + PPC_STL r0, _LINK(r1)
>
> +#ifdef CONFIG_PPC64
> /* Save callee's TOC in the ABI compliant location */
> std r2, 24(r1)
> ld r2,PACATOC(r13) /* get kernel TOC in r2 */
> @@ -74,8 +81,12 @@ _GLOBAL(ftrace_regs_caller)
> addis r3,r2,function_trace_op@toc@ha
> addi r3,r3,function_trace_op@toc@l
> ld r5,0(r3)
> +#else
> + lis r3,function_trace_op@ha
> + lwz r5,function_trace_op@l(r3)
> +#endif
>
> -#ifdef CONFIG_LIVEPATCH
> +#ifdef CONFIG_LIVEPATCH_64
> mr r14,r7 /* remember old NIP */
> #endif
> /* Calculate ip from nip-4 into r3 for call below */
> @@ -85,10 +96,10 @@ _GLOBAL(ftrace_regs_caller)
> mr r4, r0
>
> /* Save special regs */
> - std r8, _MSR(r1)
> - std r9, _CTR(r1)
> - std r10, _XER(r1)
> - std r11, _CCR(r1)
> + PPC_STL r8, _MSR(r1)
> + PPC_STL r9, _CTR(r1)
> + PPC_STL r10, _XER(r1)
> + PPC_STL r11, _CCR(r1)
>
> /* Load &pt_regs in r6 for call below */
> addi r6, r1 ,STACK_FRAME_OVERHEAD
> @@ -100,27 +111,32 @@ ftrace_regs_call:
> nop
>
> /* Load ctr with the possibly modified NIP */
> - ld r3, _NIP(r1)
> + PPC_LL r3, _NIP(r1)
> mtctr r3
> -#ifdef CONFIG_LIVEPATCH
> +#ifdef CONFIG_LIVEPATCH_64
> cmpd r14, r3 /* has NIP been altered? */
> #endif
>
> /* Restore gprs */
> - REST_GPR(0, r1)
> +#ifdef CONFIG_PPC64
> REST_GPRS(2, 31, r1)
> +#else
> + lmw r2, GPR2(r1)
> +#endif
>
> /* Restore possibly modified LR */
> - ld r0, _LINK(r1)
> + PPC_LL r0, _LINK(r1)
> mtlr r0
>
> +#ifdef CONFIG_PPC64
> /* Restore callee's TOC */
> ld r2, 24(r1)
> +#endif
>
> /* Pop our stack frame */
> addi r1, r1, SWITCH_FRAME_SIZE
>
> -#ifdef CONFIG_LIVEPATCH
> +#ifdef CONFIG_LIVEPATCH_64
> /* Based on the cmpd above, if the NIP was altered handle livepatch */
> bne- livepatch_handler
> #endif
> @@ -129,6 +145,7 @@ ftrace_regs_call:
> _GLOBAL(ftrace_stub)
> blr
>
> +#ifdef CONFIG_PPC64
> ftrace_no_trace:
> mflr r3
> mtctr r3
> @@ -136,25 +153,31 @@ ftrace_no_trace:
> addi r1, r1, SWITCH_FRAME_SIZE
> mtlr r0
> bctr
> +#endif
>
> _GLOBAL(ftrace_caller)
> /* Save the original return address in A's stack frame */
> - std r0, LRSAVE(r1)
> +#ifdef CONFIG_MPROFILE_KERNEL
> + PPC_STL r0, LRSAVE(r1)
> +#endif
>
> /* Create our stack frame + pt_regs */
> - stdu r1, -SWITCH_FRAME_SIZE(r1)
> + PPC_STLU r1, -SWITCH_FRAME_SIZE(r1)
>
> /* Save all gprs to pt_regs */
> SAVE_GPRS(3, 10, r1)
>
> +#ifdef CONFIG_PPC64
> lbz r3, PACA_FTRACE_ENABLED(r13)
> cmpdi r3, 0
> beq ftrace_no_trace
> +#endif
>
> /* Get the _mcount() call site out of LR */
> mflr r7
> - std r7, _NIP(r1)
> + PPC_STL r7, _NIP(r1)
>
> +#ifdef CONFIG_PPC64
> /* Save callee's TOC in the ABI compliant location */
> std r2, 24(r1)
> ld r2, PACATOC(r13) /* get kernel TOC in r2 */
> @@ -162,6 +185,10 @@ _GLOBAL(ftrace_caller)
> addis r3, r2, function_trace_op@toc@ha
> addi r3, r3, function_trace_op@toc@l
> ld r5, 0(r3)
> +#else
> + lis r3,function_trace_op@ha
> + lwz r5,function_trace_op@l(r3)
> +#endif
>
> #ifdef CONFIG_LIVEPATCH_64
> SAVE_GPR(14, r1)
> @@ -171,7 +198,7 @@ _GLOBAL(ftrace_caller)
> subi r3, r7, MCOUNT_INSN_SIZE
>
> /* Put the original return address in r4 as parent_ip */
> - std r0, _LINK(r1)
> + PPC_STL r0, _LINK(r1)
> mr r4, r0
>
> /* Load &pt_regs in r6 for call below */
> @@ -183,7 +210,7 @@ ftrace_call:
> bl ftrace_stub
> nop
>
> - ld r3, _NIP(r1)
> + PPC_LL r3, _NIP(r1)
> mtctr r3
> #ifdef CONFIG_LIVEPATCH_64
> cmpd r14, r3 /* has NIP been altered? */
> @@ -193,11 +220,13 @@ ftrace_call:
> /* Restore gprs */
> REST_GPRS(3, 10, r1)
>
> +#ifdef CONFIG_PPC64
> /* Restore callee's TOC */
> ld r2, 24(r1)
> +#endif
>
> /* Restore possibly modified LR */
> - ld r0, _LINK(r1)
> + PPC_LL r0, _LINK(r1)
> mtlr r0
>
> /* Pop our stack frame */
> @@ -209,7 +238,7 @@ ftrace_call:
> #endif
> bctr /* jump after _mcount site */
>
> -#ifdef CONFIG_LIVEPATCH
> +#ifdef CONFIG_LIVEPATCH_64
> /*
> * This function runs in the mcount context, between two functions. As
> * such it can only clobber registers which are volatile and used in
> --
> 2.33.1
>
^ permalink raw reply
* Re: [PATCH v1 1/2] cma: factor out minimum alignment requirement
From: Zi Yan @ 2022-02-14 17:59 UTC (permalink / raw)
To: David Hildenbrand
Cc: devicetree, Michael S. Tsirkin, Aneesh Kumar K V, linuxppc-dev,
linux-kernel, virtualization, linux-mm, Minchan Kim, iommu,
Rob Herring, Paul Mackerras, Robin Murphy, Andrew Morton,
Frank Rowand, Christoph Hellwig, Vlastimil Babka,
Marek Szyprowski
In-Reply-To: <20220214174132.219303-2-david@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 745 bytes --]
On 14 Feb 2022, at 12:41, David Hildenbrand wrote:
> Let's factor out determining the minimum alignment requirement for CMA
> and add a helpful comment.
>
> No functional change intended.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> arch/powerpc/include/asm/fadump-internal.h | 5 -----
> arch/powerpc/kernel/fadump.c | 2 +-
> drivers/of/of_reserved_mem.c | 9 +++------
> include/linux/cma.h | 9 +++++++++
> kernel/dma/contiguous.c | 4 +---
> mm/cma.c | 20 +++++---------------
> 6 files changed, 19 insertions(+), 30 deletions(-)
LGTM. Thanks. Reviewed-by: Zi Yan <ziy@nvidia.com>
--
Best Regards,
Yan, Zi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]
^ permalink raw reply
* Re: [PATCH v1 2/2] mm: enforce pageblock_order < MAX_ORDER
From: Zi Yan @ 2022-02-14 18:05 UTC (permalink / raw)
To: David Hildenbrand
Cc: devicetree, Michael S. Tsirkin, Aneesh Kumar K V, linuxppc-dev,
linux-kernel, virtualization, linux-mm, Minchan Kim, iommu,
Rob Herring, Paul Mackerras, Robin Murphy, Andrew Morton,
Frank Rowand, Christoph Hellwig, Vlastimil Babka,
Marek Szyprowski
In-Reply-To: <20220214174132.219303-3-david@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 1912 bytes --]
On 14 Feb 2022, at 12:41, David Hildenbrand wrote:
> Some places in the kernel don't really expect pageblock_order >=
> MAX_ORDER, and it looks like this is only possible in corner cases:
>
> 1) CONFIG_DEFERRED_STRUCT_PAGE_INIT we'll end up freeing pageblock_order
> pages via __free_pages_core(), which cannot possibly work.
>
> 2) find_zone_movable_pfns_for_nodes() will roundup the ZONE_MOVABLE
> start PFN to MAX_ORDER_NR_PAGES. Consequently with a bigger
> pageblock_order, we could have a single pageblock partially managed by
> two zones.
>
> 3) compaction code runs into __fragmentation_index() with order
> >= MAX_ORDER, when checking WARN_ON_ONCE(order >= MAX_ORDER). [1]
>
> 4) mm/page_reporting.c won't be reporting any pages with default
> page_reporting_order == pageblock_order, as we'll be skipping the
> reporting loop inside page_reporting_process_zone().
>
> 5) __rmqueue_fallback() will never be able to steal with
> ALLOC_NOFRAGMENT.
>
> pageblock_order >= MAX_ORDER is weird either way: it's a pure
> optimization for making alloc_contig_range(), as used for allcoation of
> gigantic pages, a little more reliable to succeed. However, if there is
> demand for somewhat reliable allocation of gigantic pages, affected setups
> should be using CMA or boottime allocations instead.
>
> So let's make sure that pageblock_order < MAX_ORDER and simplify.
>
> [1] https://lkml.kernel.org/r/87r189a2ks.fsf@linux.ibm.com
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> drivers/virtio/virtio_mem.c | 9 +++------
> include/linux/cma.h | 3 +--
> include/linux/pageblock-flags.h | 7 +++++--
> mm/Kconfig | 3 +++
> mm/page_alloc.c | 32 ++++++++------------------------
> 5 files changed, 20 insertions(+), 34 deletions(-)
LGTM. Thanks. Reviewed-by: Zi Yan <ziy@nvidia.com>
--
Best Regards,
Yan, Zi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]
^ permalink raw reply
* Re: [PATCH] powerpc/boot: Add `otheros-too-big.bld` to .gitignore
From: Geoff Levand @ 2022-02-14 18:27 UTC (permalink / raw)
To: Paul Menzel, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <20220214065543.198992-1-pmenzel@molgen.mpg.de>
Hi Paul,
On 2/13/22 22:55, Paul Menzel wrote:
> Currently, `git status` lists the file as untracked by git, so tell git
> to ignore it.
Thanks for your contribution.
Acked-by: Geoff Levand <geoff@infradead.org>
^ permalink raw reply
* Re: [BUG] mtd: cfi_cmdset_0002: write regression since v4.17-rc1
From: Tokunori Ikegami @ 2022-02-14 18:46 UTC (permalink / raw)
To: Ahmad Fatoum, Thorsten Leemhuis, linux-mtd, Joakim.Tjernlund,
miquel.raynal, vigneshr, richard, regressions@lists.linux.dev
Cc: linuxppc-dev, linux-kernel@vger.kernel.org, marek.vasut,
Chris Packham, Pengutronix Kernel Team, cyrille.pitchen,
Brian Norris, David Woodhouse
In-Reply-To: <579eab10-594c-d6b2-0ddb-ea6ab8e02856@pengutronix.de>
Hi Ahmad-san,
On 2022/02/15 1:22, Ahmad Fatoum wrote:
> Hello Tokunori-san,
>
> On 13.02.22 17:47, Tokunori Ikegami wrote:
>> Hi Ahmad-san,
>>
>> Thanks for your confirmations. Sorry for late to reply.
> No worries. I appreciate you taking the time.
>
>> Could you please try the patch attached to disable the chip_good() change as before?
>> I think this should work for S29GL964N since the chip_ready() is used and works as mentioned.
> yes, this resolves my issue:
> Tested-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
Thanks for your testing. I have just sent the patch to review.
>
>>>>> Doesn't seem to be a buffered write issue here though as the writes
>>>>> did work fine before dfeae1073583. Any other ideas?
>>>> At first I thought the issue is possible to be resolved by using the word write instead of the buffered writes.
>>>> Now I am thinking to disable the changes dfeae1073583 partially with any condition if possible.
>>> What seems to work for me is checking if chip_good or chip_ready
>>> and map_word is equal to 0xFF. I can't justify why this is ok though.
>>> (Worst case bus is floating at this point of time and Hi-Z is read
>>> as 0xff on CPU data lines...)
>> Sorry I am not sure about this.
>> I thought the chip_ready() itself is correct as implemented as the data sheet in the past.
>> But it did not work correctly so changed to use chip_good() instead as it is also correct.
> What exactly in the datasheet makes you believe chip_good is not appropriate?
I just mentioned about the actual issue behaviors as not worked
chip_good() on S29GL964N and not worked chip_ready() on
MX29GL512FHT2I-11G before etc.
Anyway let me recheck the data sheet details as just checked it again
quickly but needed more investigation to understand.
Regards,
Ikegami
>
> Cheers,
> Ahmad
>
>
^ permalink raw reply
* Re: [PATCH v2 11/13] powerpc/ftrace: directly call of function graph tracer by ftrace caller
From: Steven Rostedt @ 2022-02-14 19:03 UTC (permalink / raw)
To: Naveen N. Rao
Cc: Petr Mladek, Joe Lawrence, Jiri Kosina,
linux-kernel@vger.kernel.org, Ingo Molnar, Josh Poimboeuf,
live-patching@vger.kernel.org, Miroslav Benes,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <1644859156.qdgqumennn.naveen@linux.ibm.com>
On Mon, 14 Feb 2022 22:54:23 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> wrote:
> For x86, commit 0c0593b45c9b4e ("x86/ftrace: Make function graph use
> ftrace directly") also adds recursion check before the call to
> function_graph_enter() in prepare_ftrace_return(). Do we need that on
> powerpc as well?
Yes. The function_graph_enter() does not provide any recursion protection,
so if it were to call something that gets function graph traced, it will
crash the machine.
-- Steve
^ permalink raw reply
* Re: [PATCH 07/14] uaccess: generalize access_ok()
From: Arnd Bergmann @ 2022-02-14 19:25 UTC (permalink / raw)
To: Al Viro
Cc: Mark Rutland, Rich Felker, linux-ia64, Linux-sh list,
Peter Zijlstra, open list:BROADCOM NVRAM DRIVER, Linux-MM,
Guo Ren, sparclinux, open list:QUALCOMM HEXAGON..., linux-riscv,
Will Deacon, Christoph Hellwig, linux-arch, linux-s390,
Brian Cain, Helge Deller, the arch/x86 maintainers,
Russell King - ARM Linux, linux-csky, Ard Biesheuvel, Ingo Molnar,
Geert Uytterhoeven, open list:SYNOPSYS ARC ARCHITECTURE,
open list:TENSILICA XTENSA PORT (xtensa), Arnd Bergmann,
Heiko Carstens, alpha, linux-um, linuxppc-dev, linux-m68k,
Openrisc, Greentime Hu, Stafford Horne, Linux ARM, Michal Simek,
Thomas Bogendoerfer, Parisc List, Nick Hu, Max Filippov,
Linux API, Linux Kernel Mailing List, Dinh Nguyen,
Eric W . Biederman, Richard Weinberger, Andrew Morton,
Linus Torvalds, David Miller
In-Reply-To: <YgqOLZbFK7/B2HJT@zeniv-ca.linux.org.uk>
On Mon, Feb 14, 2022 at 6:15 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Mon, Feb 14, 2022 at 05:34:45PM +0100, Arnd Bergmann wrote:
>
> > diff --git a/arch/csky/kernel/signal.c b/arch/csky/kernel/signal.c
> > index c7b763d2f526..8867ddf3e6c7 100644
> > --- a/arch/csky/kernel/signal.c
> > +++ b/arch/csky/kernel/signal.c
> > @@ -136,7 +136,7 @@ static inline void __user *get_sigframe(struct ksignal *ksig,
> > static int
> > setup_rt_frame(struct ksignal *ksig, sigset_t *set, struct pt_regs *regs)
> > {
> > - struct rt_sigframe *frame;
> > + struct rt_sigframe __user *frame;
> > int err = 0;
> >
> > frame = get_sigframe(ksig, regs, sizeof(*frame));
>
> Minor nit: might make sense to separate annotations (here, on nios2, etc.) from the rest...
Done.
> > -}
> > -
> > -static inline int access_ok(const void __user * addr, unsigned long size)
> > -{
> > - return 1;
> > -}
> > +#define __range_not_ok(addr, size, limit) (!__access_ok(addr, size))
>
> is really wrong. For sparc64, access_ok() should always be true.
> This __range_not_ok() thing is used *only* for valid_user_frame() in
> arch/sparc/kernel/perf_event.c - it's not a part of normal access_ok()
> there.
>
> sparc64 has separate address spaces for kernel and for userland; access_ok()
> had never been useful there.
Ok, fixed as well now. I had the access_ok() bit right, the definition just
moved around here so it comes before the #include, but I missed the
bit about __range_not_ok(), which I have now reverted back to the
correct version in my tree.
Arnd
^ permalink raw reply
* Re: [PATCH 10/14] uaccess: remove most CONFIG_SET_FS users
From: Arnd Bergmann @ 2022-02-14 19:40 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Mark Rutland, Rich Felker, linux-ia64, Linux-sh list,
Peter Zijlstra, open list:BROADCOM NVRAM DRIVER, Max Filippov,
Guo Ren, sparclinux, linux-riscv, Will Deacon, Ard Biesheuvel,
linux-arch, linux-s390, Brian Cain, open list:QUALCOMM HEXAGON...,
Helge Deller, the arch/x86 maintainers, Russell King - ARM Linux,
linux-csky, Christoph Hellwig, Ingo Molnar, Geert Uytterhoeven,
open list:SYNOPSYS ARC ARCHITECTURE,
open list:TENSILICA XTENSA PORT (xtensa), Arnd Bergmann,
Heiko Carstens, linux-um, linuxppc-dev, Richard Weinberger,
linux-m68k, Openrisc, Greentime Hu, Stafford Horne, Linux ARM,
Michal Simek, Thomas Bogendoerfer, Nick Hu, Parisc List, Linux-MM,
Linux API, Linux Kernel Mailing List, Dinh Nguyen,
Eric W . Biederman, alpha, Andrew Morton, Linus Torvalds,
David Miller
In-Reply-To: <YgqL/NJ3YHEAhj4i@infradead.org>
On Mon, Feb 14, 2022 at 6:06 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Mon, Feb 14, 2022 at 05:34:48PM +0100, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > On almost all architectures, there are no remaining callers
> > of set_fs(), so CONFIG_SET_FS can be disabled, along with
> > removing the thread_info field and any references to it.
> >
> > This turns access_ok() into a cheaper check against TASK_SIZE_MAX.
>
> Wouldn't it make more sense to just merge this into the last patch?
Yes, sounds good. I wasn't sure at first if there is enough buy-in to get
all architectures cleaned up, and I hadn't done the ia64 patch, so it
seemed more important to do this part early, but now it seems that it
will all go in at the same time, so doing this as part of a big removal
at the end makes sense.
Arnd
^ permalink raw reply
* Re: [PATCH 04/14] x86: use more conventional access_ok() definition
From: Arnd Bergmann @ 2022-02-14 19:45 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Mark Rutland, Rich Felker, linux-ia64, Linux-sh list,
Peter Zijlstra, open list:BROADCOM NVRAM DRIVER, Max Filippov,
Guo Ren, sparclinux, linux-riscv, Will Deacon, Ard Biesheuvel,
linux-arch, linux-s390, Brian Cain, open list:QUALCOMM HEXAGON...,
Helge Deller, the arch/x86 maintainers, Russell King - ARM Linux,
linux-csky, Christoph Hellwig, Ingo Molnar, Geert Uytterhoeven,
open list:SYNOPSYS ARC ARCHITECTURE,
open list:TENSILICA XTENSA PORT (xtensa), Arnd Bergmann,
Heiko Carstens, linux-um, linuxppc-dev, Richard Weinberger,
linux-m68k, Openrisc, Greentime Hu, Stafford Horne, Linux ARM,
Michal Simek, Thomas Bogendoerfer, Nick Hu, Parisc List, Linux-MM,
Linux API, Linux Kernel Mailing List, Dinh Nguyen,
Eric W . Biederman, alpha, Andrew Morton, Linus Torvalds,
David Miller, Al Viro
In-Reply-To: <YgqLFYqIqkIsNC92@infradead.org>
On Mon, Feb 14, 2022 at 6:02 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Mon, Feb 14, 2022 at 05:34:42PM +0100, Arnd Bergmann wrote:
> > +#define __range_not_ok(addr, size, limit) (!__access_ok(addr, size))
> > +#define __chk_range_not_ok(addr, size, limit) (!__access_ok((void __user *)addr, size))
>
> Can we just kill these off insted of letting themm obsfucate the code?
As Al pointed out, they turned out to be necessary on sparc64, but the only
definitions are on sparc64 and x86, so it's possible that they serve a similar
purpose here, in which case changing the limit from TASK_SIZE to
TASK_SIZE_MAX is probably wrong as well.
So either I need to revert the original definition as I did on sparc64, or
they can be removed completely. Hopefully Al or the x86 maintainers
can clarify.
Arnd
^ permalink raw reply
* Re: [PATCH 04/14] x86: use more conventional access_ok() definition
From: Christoph Hellwig @ 2022-02-14 20:00 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Mark Rutland, Rich Felker, linux-ia64, Linux-sh list,
Peter Zijlstra, open list:BROADCOM NVRAM DRIVER, Max Filippov,
Guo Ren, sparclinux, linux-riscv, Will Deacon, Ard Biesheuvel,
linux-arch, linux-s390, Brian Cain, open list:QUALCOMM HEXAGON...,
Helge Deller, the arch/x86 maintainers, Russell King - ARM Linux,
linux-csky, Christoph Hellwig, Christoph Hellwig, Ingo Molnar,
Geert Uytterhoeven, open list:SYNOPSYS ARC ARCHITECTURE,
open list:TENSILICA XTENSA PORT (xtensa), Arnd Bergmann,
Heiko Carstens, linux-um, linuxppc-dev, Richard Weinberger,
linux-m68k, Openrisc, Greentime Hu, Stafford Horne, Linux ARM,
Michal Simek, Thomas Bogendoerfer, Nick Hu, Parisc List, Linux-MM,
Linux API, Linux Kernel Mailing List, Dinh Nguyen,
Eric W . Biederman, alpha, Andrew Morton, Linus Torvalds,
David Miller, Al Viro
In-Reply-To: <CAK8P3a1F3JaYaJPy9bSCG1+YV6EN05PE0DbwpD_GT1qRwFSJ-w@mail.gmail.com>
On Mon, Feb 14, 2022 at 08:45:52PM +0100, Arnd Bergmann wrote:
> As Al pointed out, they turned out to be necessary on sparc64, but the only
> definitions are on sparc64 and x86, so it's possible that they serve a similar
> purpose here, in which case changing the limit from TASK_SIZE to
> TASK_SIZE_MAX is probably wrong as well.
>
> So either I need to revert the original definition as I did on sparc64, or
> they can be removed completely. Hopefully Al or the x86 maintainers
> can clarify.
Looking at the x86 users I think:
- valid_user_frame should go away and the caller should use get_user
instead of __get_user
- the one in copy_code can just go away, as there is another check
in copy_from_user_nmi
- copy_stack_frame should just use access_ok
- as does copy_from_user_nmi
but yes, having someone who actually knows this code look over it
would be very helpful.
^ permalink raw reply
* Re: [PATCH 04/14] x86: use more conventional access_ok() definition
From: Linus Torvalds @ 2022-02-14 20:01 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Mark Rutland, Rich Felker, linux-ia64, Linux-sh list,
Peter Zijlstra, open list:BROADCOM NVRAM DRIVER, Max Filippov,
Guo Ren, sparclinux, linux-riscv, Will Deacon, Ard Biesheuvel,
linux-arch, linux-s390, Brian Cain, open list:QUALCOMM HEXAGON...,
Helge Deller, the arch/x86 maintainers, Russell King - ARM Linux,
linux-csky, Christoph Hellwig, Christoph Hellwig, Ingo Molnar,
Geert Uytterhoeven, open list:SYNOPSYS ARC ARCHITECTURE,
open list:TENSILICA XTENSA PORT (xtensa), Arnd Bergmann,
Heiko Carstens, linux-um, Richard Weinberger, linux-m68k,
Openrisc, Greentime Hu, Stafford Horne, Linux ARM, Michal Simek,
Thomas Bogendoerfer, Nick Hu, Parisc List, Linux-MM, Linux API,
Linux Kernel Mailing List, Dinh Nguyen, Eric W . Biederman, alpha,
Andrew Morton, linuxppc-dev, David Miller, Al Viro
In-Reply-To: <CAK8P3a1F3JaYaJPy9bSCG1+YV6EN05PE0DbwpD_GT1qRwFSJ-w@mail.gmail.com>
On Mon, Feb 14, 2022 at 11:46 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> As Al pointed out, they turned out to be necessary on sparc64, but the only
> definitions are on sparc64 and x86, so it's possible that they serve a similar
> purpose here, in which case changing the limit from TASK_SIZE to
> TASK_SIZE_MAX is probably wrong as well.
x86-64 has always(*) used TASK_SIZE_MAX for access_ok(), and the
get_user() assembler implementation does the same.
I think any __range_not_ok() users that use TASK_SIZE are entirely
historical, and should be just fixed.
Linus
(*) And by "always" I mean "as far back as I bothered to go". In the
2.6.12 git import, we had
#define USER_DS MAKE_MM_SEG(PAGE_OFFSET)
so the user access limit was actually not really TASK_SIZE_MAX at all,
but the beginning of the kernel mapping, which on x86-64 is much much
higher.
^ permalink raw reply
* Re: [PATCH 04/14] x86: use more conventional access_ok() definition
From: Al Viro @ 2022-02-14 20:17 UTC (permalink / raw)
To: Linus Torvalds
Cc: Mark Rutland, Rich Felker, linux-ia64, Linux-sh list,
Peter Zijlstra, open list:BROADCOM NVRAM DRIVER, Max Filippov,
Guo Ren, sparclinux, linux-riscv, Will Deacon, Ard Biesheuvel,
linux-arch, linux-s390, Brian Cain, open list:QUALCOMM HEXAGON...,
Helge Deller, the arch/x86 maintainers, Russell King - ARM Linux,
linux-csky, Christoph Hellwig, Christoph Hellwig, Ingo Molnar,
Geert Uytterhoeven, open list:SYNOPSYS ARC ARCHITECTURE,
open list:TENSILICA XTENSA PORT (xtensa), Arnd Bergmann,
Heiko Carstens, linux-um, Richard Weinberger, linux-m68k,
Openrisc, Greentime Hu, Stafford Horne, Linux ARM, Arnd Bergmann,
Michal Simek, Thomas Bogendoerfer, Nick Hu, Parisc List, Linux-MM,
Linux API, Linux Kernel Mailing List, Dinh Nguyen,
Eric W . Biederman, alpha, Andrew Morton, linuxppc-dev,
David Miller
In-Reply-To: <CAHk-=whq6_Nh3cB3FieP481VcRyCu69X3=wO1yLHGmcZEj69SA@mail.gmail.com>
On Mon, Feb 14, 2022 at 12:01:05PM -0800, Linus Torvalds wrote:
> On Mon, Feb 14, 2022 at 11:46 AM Arnd Bergmann <arnd@kernel.org> wrote:
> >
> > As Al pointed out, they turned out to be necessary on sparc64, but the only
> > definitions are on sparc64 and x86, so it's possible that they serve a similar
> > purpose here, in which case changing the limit from TASK_SIZE to
> > TASK_SIZE_MAX is probably wrong as well.
>
> x86-64 has always(*) used TASK_SIZE_MAX for access_ok(), and the
> get_user() assembler implementation does the same.
>
> I think any __range_not_ok() users that use TASK_SIZE are entirely
> historical, and should be just fixed.
IIRC, that was mostly userland stack trace collection in perf.
I'll try to dig in archives and see what shows up - it's been
a while ago...
^ permalink raw reply
* Re: [PATCH 04/14] x86: use more conventional access_ok() definition
From: Linus Torvalds @ 2022-02-14 20:24 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Mark Rutland, Rich Felker, linux-ia64, Linux-sh list,
Peter Zijlstra, open list:BROADCOM NVRAM DRIVER, Max Filippov,
Guo Ren, sparclinux, linux-riscv, Will Deacon, Ard Biesheuvel,
linux-arch, linux-s390, Brian Cain, open list:QUALCOMM HEXAGON...,
Helge Deller, the arch/x86 maintainers, Russell King - ARM Linux,
linux-csky, Christoph Hellwig, Christoph Hellwig, Ingo Molnar,
Geert Uytterhoeven, open list:SYNOPSYS ARC ARCHITECTURE,
open list:TENSILICA XTENSA PORT (xtensa), Arnd Bergmann,
Heiko Carstens, linux-um, Richard Weinberger, linux-m68k,
Openrisc, Greentime Hu, Stafford Horne, Linux ARM, Michal Simek,
Thomas Bogendoerfer, Nick Hu, Parisc List, Linux-MM, Linux API,
Linux Kernel Mailing List, Dinh Nguyen, Eric W . Biederman, alpha,
Andrew Morton, linuxppc-dev, David Miller, Al Viro
In-Reply-To: <CAHk-=whq6_Nh3cB3FieP481VcRyCu69X3=wO1yLHGmcZEj69SA@mail.gmail.com>
On Mon, Feb 14, 2022 at 12:01 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> x86-64 has always(*) used TASK_SIZE_MAX for access_ok(), and the
> get_user() assembler implementation does the same.
Side note: we could just check the sign bit instead, and avoid big
constants that way.
Right now we actually have this complexity in the x86-64 user access code:
#ifdef CONFIG_X86_5LEVEL
#define LOAD_TASK_SIZE_MINUS_N(n) \
ALTERNATIVE __stringify(mov $((1 << 47) - 4096 - (n)),%rdx), \
__stringify(mov $((1 << 56) - 4096 - (n)),%rdx),
X86_FEATURE_LA57
#else
#define LOAD_TASK_SIZE_MINUS_N(n) \
mov $(TASK_SIZE_MAX - (n)),%_ASM_DX
#endif
just because the code tries to get that TASK_SIZE_MAX boundary just right.
And getting that boundary just right is important on 32-bit x86, but
it's *much* less important on x86-64.
There's still a (weak) reason to do it even for 64-bit code: page
faults outside the valid user space range don't actually cause a #PF
fault - they cause #GP - and then we have the #GP handler warn about
"this address hasn't been checked".
Which is nice and useful for doing syzbot kind of randomization loads
(ie user accesses that didn't go through access_ok() will stand out
nicely), but maybe it's not worth this. syzbot would be fine with only
the "sign bit set" case warning for the same thing.
So on x86-64, we could just check the sign of the address instead, and
simplify and shrink those get/put_user() code sequences (but
array_index_mask_nospec() currently uses the carry flag computation
too, so we'd have to change that part as well, maybe not worth it).
Linus
^ permalink raw reply
* Re: [PATCH 08/14] arm64: simplify access_ok()
From: Robin Murphy @ 2022-02-14 21:06 UTC (permalink / raw)
To: Arnd Bergmann, Linus Torvalds, Christoph Hellwig, linux-arch,
linux-mm, linux-api, arnd, linux-kernel
Cc: mark.rutland, dalias, linux-ia64, linux-sh, peterz, jcmvbkbc,
guoren, sparclinux, linux-hexagon, linux-riscv, will, ardb,
linux-s390, bcain, deller, x86, linux, linux-csky, mingo, geert,
linux-snps-arc, linux-xtensa, hca, linux-alpha, linux-um,
linux-m68k, openrisc, green.hu, shorne, linux-arm-kernel, monstr,
tsbogend, linux-parisc, nickhu, linux-mips, dinguyen, ebiederm,
richard, akpm, linuxppc-dev, davem
In-Reply-To: <20220214163452.1568807-9-arnd@kernel.org>
On 2022-02-14 16:34, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> arm64 has an inline asm implementation of access_ok() that is derived from
> the 32-bit arm version and optimized for the case that both the limit and
> the size are variable. With set_fs() gone, the limit is always constant,
> and the size usually is as well, so just using the default implementation
> reduces the check into a comparison against a constant that can be
> scheduled by the compiler.
Aww, I still vividly remember the birth of this madness, sat with my
phone on a Saturday morning waiting for my bike to be MOT'd, staring at
the 7-instruction sequence that Mark and I had come up with and certain
that it could be shortened still. Kinda sad to see it go, but at the
same time, glad that it can.
Acked-by: Robin Murphy <robin.murphy@arm.com>
> On a defconfig build, this saves over 28KB of .text.
Not to mention saving those "WTF is going on there... oh yeah,
access_ok()" moments when looking through disassembly :)
Cheers,
Robin.
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> arch/arm64/include/asm/uaccess.h | 28 +++++-----------------------
> 1 file changed, 5 insertions(+), 23 deletions(-)
>
> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> index 357f7bd9c981..e8dce0cc5eaa 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -26,6 +26,8 @@
> #include <asm/memory.h>
> #include <asm/extable.h>
>
> +static inline int __access_ok(const void __user *ptr, unsigned long size);
> +
> /*
> * Test whether a block of memory is a valid user space address.
> * Returns 1 if the range is valid, 0 otherwise.
> @@ -33,10 +35,8 @@
> * This is equivalent to the following test:
> * (u65)addr + (u65)size <= (u65)TASK_SIZE_MAX
> */
> -static inline unsigned long __access_ok(const void __user *addr, unsigned long size)
> +static inline int access_ok(const void __user *addr, unsigned long size)
> {
> - unsigned long ret, limit = TASK_SIZE_MAX - 1;
> -
> /*
> * Asynchronous I/O running in a kernel thread does not have the
> * TIF_TAGGED_ADDR flag of the process owning the mm, so always untag
> @@ -46,27 +46,9 @@ static inline unsigned long __access_ok(const void __user *addr, unsigned long s
> (current->flags & PF_KTHREAD || test_thread_flag(TIF_TAGGED_ADDR)))
> addr = untagged_addr(addr);
>
> - __chk_user_ptr(addr);
> - asm volatile(
> - // A + B <= C + 1 for all A,B,C, in four easy steps:
> - // 1: X = A + B; X' = X % 2^64
> - " adds %0, %3, %2\n"
> - // 2: Set C = 0 if X > 2^64, to guarantee X' > C in step 4
> - " csel %1, xzr, %1, hi\n"
> - // 3: Set X' = ~0 if X >= 2^64. For X == 2^64, this decrements X'
> - // to compensate for the carry flag being set in step 4. For
> - // X > 2^64, X' merely has to remain nonzero, which it does.
> - " csinv %0, %0, xzr, cc\n"
> - // 4: For X < 2^64, this gives us X' - C - 1 <= 0, where the -1
> - // comes from the carry in being clear. Otherwise, we are
> - // testing X' - C == 0, subject to the previous adjustments.
> - " sbcs xzr, %0, %1\n"
> - " cset %0, ls\n"
> - : "=&r" (ret), "+r" (limit) : "Ir" (size), "0" (addr) : "cc");
> -
> - return ret;
> + return likely(__access_ok(addr, size));
> }
> -#define __access_ok __access_ok
> +#define access_ok access_ok
>
> #include <asm-generic/access_ok.h>
>
^ permalink raw reply
* RE: [PATCH 04/14] x86: use more conventional access_ok() definition
From: David Laight @ 2022-02-14 22:13 UTC (permalink / raw)
To: 'Linus Torvalds', Arnd Bergmann
Cc: Mark Rutland, Rich Felker, linux-ia64@vger.kernel.org,
Linux-sh list, Peter Zijlstra, Linux Kernel Mailing List,
Max Filippov, Guo Ren, sparclinux, linux-riscv, Will Deacon,
Ard Biesheuvel, linux-arch, linux-s390, Brian Cain,
open list:QUALCOMM HEXAGON..., Helge Deller,
the arch/x86 maintainers, Russell King - ARM Linux,
linux-csky@vger.kernel.org, Christoph Hellwig, Christoph Hellwig,
Ingo Molnar, Geert Uytterhoeven,
open list:SYNOPSYS ARC ARCHITECTURE,
open list:TENSILICA XTENSA PORT (xtensa), Arnd Bergmann,
Heiko Carstens, alpha, linux-um, linux-m68k, Openrisc,
Greentime Hu, Stafford Horne, Linux ARM, Michal Simek,
Thomas Bogendoerfer, Parisc List, Nick Hu, Linux-MM, Linux API,
open list:BROADCOM NVRAM DRIVER, Dinh Nguyen, Eric W . Biederman,
Richard Weinberger, Andrew Morton, linuxppc-dev, David Miller,
Al Viro
In-Reply-To: <CAHk-=wgYu67OwP4LhcrPdDVxv2mOsx-Xsc2DKoVW6GZwKFtOYQ@mail.gmail.com>
From: Linus Torvalds
> Sent: 14 February 2022 20:24
> >
> > x86-64 has always(*) used TASK_SIZE_MAX for access_ok(), and the
> > get_user() assembler implementation does the same.
>
> Side note: we could just check the sign bit instead, and avoid big
> constants that way.
The cheap test for most 64bit is (addr | size) >> 62 != 0.
I did some tests last week and the compilers correctly optimise
out constant size.
Doesn't sparc64 still need a wrap test?
Or is that assumed because there is always an unmapped page
and transfer are 'adequately' done on increasing addresses?
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox