* [PATCH v4] powerpc: use probe_user_read() and probe_user_write()
From: Christophe Leroy @ 2020-01-23 17:30 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
Instead of opencoding, use probe_user_read() to failessly read
a user location and probe_user_write() for writing to user.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
Link to v3: https://patchwork.ozlabs.org/patch/1026042/
v4: Reviving this patch after one year. Now probe_user_read/write() is in the kernel so patch 1 is gone.
v3: No change
v2: Using probe_user_read() instead of probe_user_address()
---
arch/powerpc/kernel/process.c | 12 +-----------
arch/powerpc/kvm/book3s_64_mmu_radix.c | 6 ++----
arch/powerpc/mm/fault.c | 6 +-----
arch/powerpc/oprofile/backtrace.c | 14 ++------------
arch/powerpc/perf/callchain.c | 20 +++-----------------
arch/powerpc/perf/core-book3s.c | 8 +-------
arch/powerpc/sysdev/fsl_pci.c | 10 ++++------
7 files changed, 14 insertions(+), 62 deletions(-)
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 4df94b6e2f32..79f2cb6ecf87 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1264,16 +1264,6 @@ void show_user_instructions(struct pt_regs *regs)
pc = regs->nip - (NR_INSN_TO_PRINT * 3 / 4 * sizeof(int));
- /*
- * Make sure the NIP points at userspace, not kernel text/data or
- * elsewhere.
- */
- if (!__access_ok(pc, NR_INSN_TO_PRINT * sizeof(int), USER_DS)) {
- pr_info("%s[%d]: Bad NIP, not dumping instructions.\n",
- current->comm, current->pid);
- return;
- }
-
seq_buf_init(&s, buf, sizeof(buf));
while (n) {
@@ -1284,7 +1274,7 @@ void show_user_instructions(struct pt_regs *regs)
for (i = 0; i < 8 && n; i++, n--, pc += sizeof(int)) {
int instr;
- if (probe_kernel_address((const void *)pc, instr)) {
+ if (probe_user_read(&instr, (void __user *)pc, sizeof(instr))) {
seq_buf_printf(&s, "XXXXXXXX ");
continue;
}
diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c b/arch/powerpc/kvm/book3s_64_mmu_radix.c
index da857c8ba6e4..231410dc9db4 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_radix.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c
@@ -63,12 +63,10 @@ unsigned long __kvmhv_copy_tofrom_guest_radix(int lpid, int pid,
}
isync();
- pagefault_disable();
if (is_load)
- ret = raw_copy_from_user(to, from, n);
+ ret = probe_user_read(to, (const void __user *)from, n);
else
- ret = raw_copy_to_user(to, from, n);
- pagefault_enable();
+ ret = probe_user_write((void __user *)to, from, n);
/* switch the pid first to avoid running host with unallocated pid */
if (quadrant == 1 && pid != old_pid)
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index b5047f9b5dec..9e119f98a725 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -279,12 +279,8 @@ static bool bad_stack_expansion(struct pt_regs *regs, unsigned long address,
if ((flags & FAULT_FLAG_WRITE) && (flags & FAULT_FLAG_USER) &&
access_ok(nip, sizeof(*nip))) {
unsigned int inst;
- int res;
- pagefault_disable();
- res = __get_user_inatomic(inst, nip);
- pagefault_enable();
- if (!res)
+ if (!probe_user_read(&inst, nip, sizeof(inst)))
return !store_updates_sp(inst);
*must_retry = true;
}
diff --git a/arch/powerpc/oprofile/backtrace.c b/arch/powerpc/oprofile/backtrace.c
index 43245f4a9bcb..2799b922f780 100644
--- a/arch/powerpc/oprofile/backtrace.c
+++ b/arch/powerpc/oprofile/backtrace.c
@@ -28,15 +28,12 @@ static unsigned int user_getsp32(unsigned int sp, int is_first)
unsigned int stack_frame[2];
void __user *p = compat_ptr(sp);
- if (!access_ok(p, sizeof(stack_frame)))
- return 0;
-
/*
* The most likely reason for this is that we returned -EFAULT,
* which means that we've done all that we can do from
* interrupt context.
*/
- if (__copy_from_user_inatomic(stack_frame, p, sizeof(stack_frame)))
+ if (probe_user_read(stack_frame, (void __user *)p, sizeof(stack_frame)))
return 0;
if (!is_first)
@@ -54,11 +51,7 @@ static unsigned long user_getsp64(unsigned long sp, int is_first)
{
unsigned long stack_frame[3];
- if (!access_ok((void __user *)sp, sizeof(stack_frame)))
- return 0;
-
- if (__copy_from_user_inatomic(stack_frame, (void __user *)sp,
- sizeof(stack_frame)))
+ if (probe_user_read(stack_frame, (void __user *)sp, sizeof(stack_frame)))
return 0;
if (!is_first)
@@ -103,7 +96,6 @@ void op_powerpc_backtrace(struct pt_regs * const regs, unsigned int depth)
first_frame = 0;
}
} else {
- pagefault_disable();
#ifdef CONFIG_PPC64
if (!is_32bit_task()) {
while (depth--) {
@@ -112,7 +104,6 @@ void op_powerpc_backtrace(struct pt_regs * const regs, unsigned int depth)
break;
first_frame = 0;
}
- pagefault_enable();
return;
}
#endif
@@ -123,6 +114,5 @@ void op_powerpc_backtrace(struct pt_regs * const regs, unsigned int depth)
break;
first_frame = 0;
}
- pagefault_enable();
}
}
diff --git a/arch/powerpc/perf/callchain.c b/arch/powerpc/perf/callchain.c
index 35d542515faf..cbc251981209 100644
--- a/arch/powerpc/perf/callchain.c
+++ b/arch/powerpc/perf/callchain.c
@@ -155,12 +155,8 @@ static int read_user_stack_64(unsigned long __user *ptr, unsigned long *ret)
((unsigned long)ptr & 7))
return -EFAULT;
- pagefault_disable();
- if (!__get_user_inatomic(*ret, ptr)) {
- pagefault_enable();
+ if (!probe_user_read(ret, ptr, sizeof(*ret)))
return 0;
- }
- pagefault_enable();
return read_user_stack_slow(ptr, ret, 8);
}
@@ -171,12 +167,8 @@ static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret)
((unsigned long)ptr & 3))
return -EFAULT;
- pagefault_disable();
- if (!__get_user_inatomic(*ret, ptr)) {
- pagefault_enable();
+ if (!probe_user_read(ret, ptr, sizeof(*ret)))
return 0;
- }
- pagefault_enable();
return read_user_stack_slow(ptr, ret, 4);
}
@@ -293,17 +285,11 @@ static void perf_callchain_user_64(struct perf_callchain_entry_ctx *entry,
*/
static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret)
{
- int rc;
-
if ((unsigned long)ptr > TASK_SIZE - sizeof(unsigned int) ||
((unsigned long)ptr & 3))
return -EFAULT;
- pagefault_disable();
- rc = __get_user_inatomic(*ret, ptr);
- pagefault_enable();
-
- return rc;
+ return probe_user_read(ret, ptr, sizeof(*ret));
}
static inline void perf_callchain_user_64(struct perf_callchain_entry_ctx *entry,
diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index 48604625ab31..3086055bf681 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -415,7 +415,6 @@ static void power_pmu_sched_task(struct perf_event_context *ctx, bool sched_in)
static __u64 power_pmu_bhrb_to(u64 addr)
{
unsigned int instr;
- int ret;
__u64 target;
if (is_kernel_addr(addr)) {
@@ -426,13 +425,8 @@ static __u64 power_pmu_bhrb_to(u64 addr)
}
/* Userspace: need copy instruction here then translate it */
- pagefault_disable();
- ret = __get_user_inatomic(instr, (unsigned int __user *)addr);
- if (ret) {
- pagefault_enable();
+ if (probe_user_read(&instr, (unsigned int __user *)addr, sizeof(instr)))
return 0;
- }
- pagefault_enable();
target = branch_target(&instr);
if ((!target) || (instr & BRANCH_ABSOLUTE))
diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
index 617a443d673d..4a8874bc1057 100644
--- a/arch/powerpc/sysdev/fsl_pci.c
+++ b/arch/powerpc/sysdev/fsl_pci.c
@@ -1065,13 +1065,11 @@ int fsl_pci_mcheck_exception(struct pt_regs *regs)
addr += mfspr(SPRN_MCAR);
if (is_in_pci_mem_space(addr)) {
- if (user_mode(regs)) {
- pagefault_disable();
- ret = get_user(inst, (__u32 __user *)regs->nip);
- pagefault_enable();
- } else {
+ if (user_mode(regs))
+ ret = probe_user_read(&inst, (void __user *)regs->nip,
+ sizeof(inst));
+ else
ret = probe_kernel_address((void *)regs->nip, inst);
- }
if (!ret && mcheck_handle_load(regs, inst)) {
regs->nip += 4;
--
2.25.0
^ permalink raw reply related
* Re: [PATCH v3 2/7] uaccess: Tell user_access_begin() if it's for a write or not
From: Linus Torvalds @ 2020-01-23 18:02 UTC (permalink / raw)
To: Christophe Leroy
Cc: the arch/x86 maintainers, dri-devel, H. Peter Anvin,
Linux Kernel Mailing List, Jani Nikula, Linux-MM, Ingo Molnar,
Paul Mackerras, Alexander Viro, linux-fsdevel, Borislav Petkov,
Andrew Morton, linuxppc-dev, Thomas Gleixner
In-Reply-To: <e11a8f0670251267f87e3114e0bdbacb1eb72980.1579783936.git.christophe.leroy@c-s.fr>
On Thu, Jan 23, 2020 at 4:59 AM Christophe Leroy
<christophe.leroy@c-s.fr> wrote:
>
> On 32 bits powerPC (book3s/32), only write accesses to user are
> protected and there is no point spending time on unlocking for reads.
Honestly, I'm starting to think that 32-bit ppc just needs to look
more like everybody else, than make these changes.
We used to have a read/write argument to the old "verify_area()" and
"access_ok()" model, and it was a mistake. It was due to odd i386 user
access issues. We got rid of it. I'm not convinced this is any better
- it looks very similar and for odd ppc access issues.
But if we really do want to do this, then:
> Add an argument to user_access_begin() to tell when it's for write and
> return an opaque key that will be used by user_access_end() to know
> what was done by user_access_begin().
You should make it more opaque than "unsigned long".
Also, it shouldn't be a "is this a write". What if it's a read _and_ a
write? Only a write? Only a read?
Linus
^ permalink raw reply
* Re: [PATCH v2 1/6] fs/readdir: Fix filldir() and filldir64() use of user_access_begin()
From: Linus Torvalds @ 2020-01-23 18:38 UTC (permalink / raw)
To: Michael Ellerman
Cc: Linux Kernel Mailing List, Linux-MM, Paul Mackerras,
Alexander Viro, linux-fsdevel, Andrew Morton, linuxppc-dev
In-Reply-To: <87k15iidrq.fsf@mpe.ellerman.id.au>
On Thu, Jan 23, 2020 at 4:00 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> So I guess I'll wait and see what happens with patch 1.
I've committed my fixes to filldir[64]() directly - they really were
fixing me being lazy about the range, and the name length checking
really is a theoretical "access wrong user space pointer" issue with
corrupted filesystems regardless (even though I suspect it's entirely
theoretical - even a corrupt filesystem hopefully won't be passing in
negative directory entry lengths or something like that).
The "pass in read/write" part I'm not entirely convinced about.
Honestly, if this is just for ppc32 and nobody else really needs it,
make the ppc32s thing always just enable both user space reads and
writes. That's the semantics for x86 and arm as is, I'm not convinced
that we should complicate this for a legacy platform.
Linus
^ permalink raw reply
* Re: [PATCH] lib: Reduce user_access_begin() boundaries in strncpy_from_user() and strnlen_user()
From: Linus Torvalds @ 2020-01-23 18:47 UTC (permalink / raw)
To: Christophe Leroy, linux-arch
Cc: Linux-MM, Andrew Morton, linuxppc-dev, Linux Kernel Mailing List
In-Reply-To: <70f99f7474826883877e84f93224e937d9c974de.1579767339.git.christophe.leroy@c-s.fr>
On Thu, Jan 23, 2020 at 12:34 AM Christophe Leroy
<christophe.leroy@c-s.fr> wrote:
>
> The range passed to user_access_begin() by strncpy_from_user() and
> strnlen_user() starts at 'src' and goes up to the limit of userspace
> allthough reads will be limited by the 'count' param.
>
> On 32 bits powerpc (book3s/32) access has to be granted for each 256Mbytes
> segment and the cost increases with the number of segments to unlock.
>
> Limit the range with 'count' param.
Ack. I'm tempted to take this for 5.5 too, just so that the
unquestionably trivial fixes are in that baseline, and the
infrastructure is ready for any architecture that has issues like
this.
Adding 'linux-arch' to the participants, to see if other architectures
are at all looking at actually implementing the whole
user_access_begin/end() dance too..
Linus
^ permalink raw reply
* Re: [PATCH v3 2/7] uaccess: Tell user_access_begin() if it's for a write or not
From: christophe leroy @ 2020-01-23 19:47 UTC (permalink / raw)
To: Linus Torvalds
Cc: the arch/x86 maintainers, dri-devel, H. Peter Anvin,
Linux Kernel Mailing List, Jani Nikula, Linux-MM, Ingo Molnar,
Paul Mackerras, Alexander Viro, linux-fsdevel, Borislav Petkov,
Andrew Morton, linuxppc-dev, Thomas Gleixner
In-Reply-To: <CAHk-=wg4HEABOZdjxMzbembNmxs1zYfrNAEc2L+JS9FBSnM8JA@mail.gmail.com>
Le 23/01/2020 à 19:02, Linus Torvalds a écrit :
> On Thu, Jan 23, 2020 at 4:59 AM Christophe Leroy
> <christophe.leroy@c-s.fr> wrote:
>>
>> On 32 bits powerPC (book3s/32), only write accesses to user are
>> protected and there is no point spending time on unlocking for reads.
>
> Honestly, I'm starting to think that 32-bit ppc just needs to look
> more like everybody else, than make these changes.
Well, beside ppc32, I was also seen it as an opportunity for the modern
ppc64. On it, you can unlock either read or write or both. And this is
what is done for get_user() / put_user() and friends: unlock only reads
for get_user() and only writes for put_user().
Could also be a compromise between performance and security: keeping
reads allowed at all time and only protect against writes on modern
architectures which support it like ppc64.
>
> We used to have a read/write argument to the old "verify_area()" and
> "access_ok()" model, and it was a mistake. It was due to odd i386 user
> access issues. We got rid of it. I'm not convinced this is any better
> - it looks very similar and for odd ppc access issues.
I'm going to leave it aside, at least for the time being, and do it as a
second step later after evaluating the real performance impact. I'll
respin tomorrow in that way.
>
> But if we really do want to do this, then:
Indeed I took the idea from a discussion in last Octobre (Subject:
"book3s/32 KUAP (was Re: [PATCH] Convert filldir[64]() from __put_user()
to unsafe_put_user())" )
https://lore.kernel.org/lkml/87h84avffi.fsf@mpe.ellerman.id.au/
>
>> Add an argument to user_access_begin() to tell when it's for write and
>> return an opaque key that will be used by user_access_end() to know
>> what was done by user_access_begin().
>
> You should make it more opaque than "unsigned long".
>
> Also, it shouldn't be a "is this a write". What if it's a read _and_ a
> write? Only a write? Only a read?
Indeed that was more: does it includes a write. It's either RO or RW
Christophe
^ permalink raw reply
* Re: [PATCH v3 2/7] uaccess: Tell user_access_begin() if it's for a write or not
From: Linus Torvalds @ 2020-01-23 19:57 UTC (permalink / raw)
To: christophe leroy
Cc: the arch/x86 maintainers, dri-devel, H. Peter Anvin,
Linux Kernel Mailing List, Jani Nikula, Linux-MM, Ingo Molnar,
Paul Mackerras, Alexander Viro, linux-fsdevel, Borislav Petkov,
Andrew Morton, linuxppc-dev, Thomas Gleixner
In-Reply-To: <fc5c94a2-5a25-0715-5240-5ba3cbe0f2b2@c-s.fr>
On Thu, Jan 23, 2020 at 11:47 AM christophe leroy
<christophe.leroy@c-s.fr> wrote:
>
> I'm going to leave it aside, at least for the time being, and do it as a
> second step later after evaluating the real performance impact. I'll
> respin tomorrow in that way.
Ok, good.
From a "narrow the access window type" standpoint it does seem to be a
good idea to specify what kind of user accesses will be done, so I
don't hate the idea, it's more that I'm not convinced it matters
enough.
On x86, we have made the rule that user_access_begin/end() can contain
_very_ few operations, and objtool really does enforce that. With
objtool and KASAN, you really end up with very small ranges of
user_access_begin/end().
And since we actually verify it statically on x86-64, I would say that
the added benefit of narrowing by access type is fairly small. We're
not going to have complicated code in that user access region, at
least in generic code.
> > Also, it shouldn't be a "is this a write". What if it's a read _and_ a
> > write? Only a write? Only a read?
>
> Indeed that was more: does it includes a write. It's either RO or RW
I would expect that most actual users would be RO or WO, so it's a bit
odd to have those choices.
Of course, often writing ends up requiring read permissions anyway if
the architecture has problems with alignment handling or similar, but
still... The real RW case does exist conceptually (we have
"copy_in_user()", after all), but still feels like it shouldn't be
seen as the only _interface_ choice.
IOW, an architecture may decide to turn WO into RW because of
architecture limitations (or, like x86 and arm, ignore the whole
RO/RW/WO _entirely_ because there's just a single "allow user space
accesses" flag), but on an interface layer if we add this flag, I
really think it should be an explicit "read or write or both".
So thus my "let's try to avoid doing it in the first place, but if we
_do_ do this, then do it right" plea.
Linus
^ permalink raw reply
* Re: [PATCH v2 02/27] nvdimm: remove prototypes for nonexistent functions
From: Dan Williams @ 2020-01-23 21:49 UTC (permalink / raw)
To: Alastair D'Silva
Cc: Madhavan Srinivasan, Alexey Kardashevskiy, Keith Busch,
Masahiro Yamada, Paul Mackerras, Mauro Carvalho Chehab, Ira Weiny,
Rob Herring, Dave Jiang, linux-nvdimm, Vishal Verma,
Mahesh Salgaonkar, Krzysztof Kozlowski, Anju T Sudhakar,
Aneesh Kumar K.V, alastair, Andrew Donnellan, Arnd Bergmann,
Greg Kurz, Nicholas Piggin, Cédric Le Goater,
Thomas Gleixner, Hari Bathini, Linux MM, Greg Kroah-Hartman,
Linux Kernel Mailing List, Frederic Barrat, Andrew Morton,
linuxppc-dev, David S. Miller
In-Reply-To: <CAPcyv4hhK1dyqqe=CwnGfd=hRdUJn0pL6scCUgCz2R+bijZvYg@mail.gmail.com>
[ add Aneesh ]
On Tue, Dec 3, 2019 at 4:10 PM Dan Williams <dan.j.williams@intel.com> wrote:
>
> On Mon, Dec 2, 2019 at 7:48 PM Alastair D'Silva <alastair@au1.ibm.com> wrote:
> >
> > From: Alastair D'Silva <alastair@d-silva.org>
> >
> > These functions don't exist, so remove the prototypes for them.
> >
> > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> > Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>
> > ---
>
> This was already merged as commit:
>
> cda93d6965a1 libnvdimm: Remove prototypes for nonexistent functions
>
> You should have received a notification from the patchwork bot that it
> was already accepted.
>
> What baseline did you use for this submission?
I never got an answer to this, and I have not seen any updates. Can I
ask you to get an initial review from Aneesh who has been doing good
work in the nvdimm core, and then we can look to get this in the
pipeline for the v5.7 kernel?
^ permalink raw reply
* Re: [PATCH v3 2/7] uaccess: Tell user_access_begin() if it's for a write or not
From: hpa @ 2020-01-24 2:03 UTC (permalink / raw)
To: Linus Torvalds, christophe leroy
Cc: dri-devel, the arch/x86 maintainers, Linux Kernel Mailing List,
Jani Nikula, Linux-MM, Ingo Molnar, Paul Mackerras,
Alexander Viro, linux-fsdevel, Borislav Petkov, Andrew Morton,
linuxppc-dev, Thomas Gleixner
In-Reply-To: <CAHk-=wi8FvaeRv6PpisQ+L_Cv52yE6jCxZzUHQPZ_K7HzFkaBQ@mail.gmail.com>
On January 23, 2020 11:57:57 AM PST, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>On Thu, Jan 23, 2020 at 11:47 AM christophe leroy
><christophe.leroy@c-s.fr> wrote:
>>
>> I'm going to leave it aside, at least for the time being, and do it
>as a
>> second step later after evaluating the real performance impact. I'll
>> respin tomorrow in that way.
>
>Ok, good.
>
>From a "narrow the access window type" standpoint it does seem to be a
>good idea to specify what kind of user accesses will be done, so I
>don't hate the idea, it's more that I'm not convinced it matters
>enough.
>
>On x86, we have made the rule that user_access_begin/end() can contain
>_very_ few operations, and objtool really does enforce that. With
>objtool and KASAN, you really end up with very small ranges of
>user_access_begin/end().
>
>And since we actually verify it statically on x86-64, I would say that
>the added benefit of narrowing by access type is fairly small. We're
>not going to have complicated code in that user access region, at
>least in generic code.
>
>> > Also, it shouldn't be a "is this a write". What if it's a read
>_and_ a
>> > write? Only a write? Only a read?
>>
>> Indeed that was more: does it includes a write. It's either RO or RW
>
>I would expect that most actual users would be RO or WO, so it's a bit
>odd to have those choices.
>
>Of course, often writing ends up requiring read permissions anyway if
>the architecture has problems with alignment handling or similar, but
>still... The real RW case does exist conceptually (we have
>"copy_in_user()", after all), but still feels like it shouldn't be
>seen as the only _interface_ choice.
>
>IOW, an architecture may decide to turn WO into RW because of
>architecture limitations (or, like x86 and arm, ignore the whole
>RO/RW/WO _entirely_ because there's just a single "allow user space
>accesses" flag), but on an interface layer if we add this flag, I
>really think it should be an explicit "read or write or both".
>
>So thus my "let's try to avoid doing it in the first place, but if we
>_do_ do this, then do it right" plea.
>
> Linus
I'm wondering if we should make it a static part of the API instead of a variable.
I have *deep* concern with carrying state in a "key" variable: it's a direct attack vector for a crowbar attack, especially since it is by definition live inside a user access region.
One major reason x86 restricts the regions like this is to minimize the amount of unconstrained state: we don't save and restore the state around, but enter and exit unconditionally, which means that a leaked state will end up having a limited lifespan. Nor is there any state inside the user access region which could be corrupted to leave the region open.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* Re: [PATCH 1/2] powerpc/irq: don't use current_stack_pointer() in check_stack_overflow()
From: Michael Ellerman @ 2020-01-24 5:46 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <bae3e75a0c7f9037e4012ee547842c04cd527931.1575871613.git.christophe.leroy@c-s.fr>
Christophe Leroy <christophe.leroy@c-s.fr> writes:
> current_stack_pointer() doesn't return the stack pointer, but the
> caller's stack frame. See commit bfe9a2cfe91a ("powerpc: Reimplement
> __get_SP() as a function not a define") and commit acf620ecf56c
> ("powerpc: Rename __get_SP() to current_stack_pointer()") for details.
>
> The purpose of check_stack_overflow() is to verify that the stack has
> not overflowed.
>
> To really know whether the stack pointer is still within boundaries,
> the check must be done directly on the value of r1.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> arch/powerpc/kernel/irq.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
> index bb34005ff9d2..4d468d835558 100644
> --- a/arch/powerpc/kernel/irq.c
> +++ b/arch/powerpc/kernel/irq.c
> @@ -599,9 +599,8 @@ u64 arch_irq_stat_cpu(unsigned int cpu)
> static inline void check_stack_overflow(void)
> {
> #ifdef CONFIG_DEBUG_STACKOVERFLOW
> - long sp;
> -
> - sp = current_stack_pointer() & (THREAD_SIZE-1);
> + register unsigned long r1 asm("r1");
> + long sp = r1 & (THREAD_SIZE - 1);
This appears to work but seems to be "unsupported" by GCC, and clang
actually complains about it:
/linux/arch/powerpc/kernel/irq.c:603:12: error: variable 'r1' is uninitialized when used here [-Werror,-Wuninitialized]
long sp = r1 & (THREAD_SIZE - 1);
^~
The GCC docs say:
The only supported use for this feature is to specify registers for
input and output operands when calling Extended asm (see Extended
Asm).
https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gcc/Local-Register-Variables.html#Local-Register-Variables
If I do this it seems to work, but feels a little dicey:
asm ("" : "=r" (r1));
sp = r1 & (THREAD_SIZE - 1);
Generated code looks OK ish:
clang:
sp = r1 & (THREAD_SIZE - 1);
22e0: a0 04 24 78 clrldi r4,r1,50
if (unlikely(sp < 2048)) {
22e4: ff 07 24 28 cmpldi r4,2047
22e8: 58 00 81 40 ble 2340 <do_IRQ+0xe0>
gcc:
if (unlikely(sp < 2048)) {
2eb4: 00 38 28 70 andi. r8,r1,14336
...
2ecc: 24 00 82 40 bne c000000000002ef0 <do_IRQ+0xa0>
cheers
^ permalink raw reply
* Re: [PATCH 1/2] powerpc/irq: don't use current_stack_pointer() in check_stack_overflow()
From: Christophe Leroy @ 2020-01-24 6:19 UTC (permalink / raw)
To: Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <87d0b9iez3.fsf@mpe.ellerman.id.au>
Le 24/01/2020 à 06:46, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@c-s.fr> writes:
>
>> current_stack_pointer() doesn't return the stack pointer, but the
>> caller's stack frame. See commit bfe9a2cfe91a ("powerpc: Reimplement
>> __get_SP() as a function not a define") and commit acf620ecf56c
>> ("powerpc: Rename __get_SP() to current_stack_pointer()") for details.
>>
>> The purpose of check_stack_overflow() is to verify that the stack has
>> not overflowed.
>>
>> To really know whether the stack pointer is still within boundaries,
>> the check must be done directly on the value of r1.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> ---
>> arch/powerpc/kernel/irq.c | 5 ++---
>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
>> index bb34005ff9d2..4d468d835558 100644
>> --- a/arch/powerpc/kernel/irq.c
>> +++ b/arch/powerpc/kernel/irq.c
>> @@ -599,9 +599,8 @@ u64 arch_irq_stat_cpu(unsigned int cpu)
>> static inline void check_stack_overflow(void)
>> {
>> #ifdef CONFIG_DEBUG_STACKOVERFLOW
>> - long sp;
>> -
>> - sp = current_stack_pointer() & (THREAD_SIZE-1);
>> + register unsigned long r1 asm("r1");
>> + long sp = r1 & (THREAD_SIZE - 1);
>
> This appears to work but seems to be "unsupported" by GCC, and clang
> actually complains about it:
>
> /linux/arch/powerpc/kernel/irq.c:603:12: error: variable 'r1' is uninitialized when used here [-Werror,-Wuninitialized]
> long sp = r1 & (THREAD_SIZE - 1);
> ^~
>
> The GCC docs say:
>
> The only supported use for this feature is to specify registers for
> input and output operands when calling Extended asm (see Extended
> Asm).
>
> https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gcc/Local-Register-Variables.html#Local-Register-Variables
>
>
> If I do this it seems to work, but feels a little dicey:
>
> asm ("" : "=r" (r1));
> sp = r1 & (THREAD_SIZE - 1);
Or we could do add in asm/reg.h what we have in boot/reg.h:
register void *__stack_pointer asm("r1");
#define get_sp() (__stack_pointer)
And use get_sp()
I'll try it.
Christophe
^ permalink raw reply
* Re: [PATCH] lib: Reduce user_access_begin() boundaries in strncpy_from_user() and strnlen_user()
From: Christophe Leroy @ 2020-01-24 6:25 UTC (permalink / raw)
To: Linus Torvalds, linux-arch
Cc: Linux-MM, Andrew Morton, linuxppc-dev, Linux Kernel Mailing List
In-Reply-To: <CAHk-=wgcm5JzyacekDGQ4Ocoe-F5it-7-sbgU8oPnhwnSH3KAA@mail.gmail.com>
Le 23/01/2020 à 19:47, Linus Torvalds a écrit :
> On Thu, Jan 23, 2020 at 12:34 AM Christophe Leroy
> <christophe.leroy@c-s.fr> wrote:
>>
>> The range passed to user_access_begin() by strncpy_from_user() and
>> strnlen_user() starts at 'src' and goes up to the limit of userspace
>> allthough reads will be limited by the 'count' param.
>>
>> On 32 bits powerpc (book3s/32) access has to be granted for each 256Mbytes
>> segment and the cost increases with the number of segments to unlock.
>>
>> Limit the range with 'count' param.
>
> Ack. I'm tempted to take this for 5.5 too, just so that the
> unquestionably trivial fixes are in that baseline, and the
> infrastructure is ready for any architecture that has issues like
> this.
It would be nice, then the user_access_begin stuff for powerpc could go
for 5.6 without worring about.
Thanks
Christophe
^ permalink raw reply
* Re: [PATCH 1/2] powerpc/irq: don't use current_stack_pointer() in check_stack_overflow()
From: Christophe Leroy @ 2020-01-24 7:03 UTC (permalink / raw)
To: Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <f4196f83-82ac-4df0-8c15-267a2c6c07ba@c-s.fr>
On 01/24/2020 06:19 AM, Christophe Leroy wrote:
>
>
> Le 24/01/2020 à 06:46, Michael Ellerman a écrit :
>>
>> If I do this it seems to work, but feels a little dicey:
>>
>> asm ("" : "=r" (r1));
>> sp = r1 & (THREAD_SIZE - 1);
>
>
> Or we could do add in asm/reg.h what we have in boot/reg.h:
>
> register void *__stack_pointer asm("r1");
> #define get_sp() (__stack_pointer)
>
> And use get_sp()
>
It works, and I guess doing it this way is acceptable as it's exactly
what's done for current in asm/current.h with register r2.
Now I (still) get:
sp = get_sp() & (THREAD_SIZE - 1);
b9c: 54 24 04 fe clrlwi r4,r1,19
if (unlikely(sp < 2048)) {
ba4: 2f 84 07 ff cmpwi cr7,r4,2047
Allthough GCC 8.1 what doing exactly the same with the form CLANG don't
like:
register unsigned long r1 asm("r1");
long sp = r1 & (THREAD_SIZE - 1);
b84: 54 24 04 fe clrlwi r4,r1,19
if (unlikely(sp < 2048)) {
b8c: 2f 84 07 ff cmpwi cr7,r4,2047
Christophe
^ permalink raw reply
* [PATCH v2 1/3] powerpc/irq: don't use current_stack_pointer() in check_stack_overflow()
From: Christophe Leroy @ 2020-01-24 7:07 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
current_stack_pointer() doesn't return the stack pointer, but the
caller's stack frame. See commit bfe9a2cfe91a ("powerpc: Reimplement
__get_SP() as a function not a define") and commit acf620ecf56c
("powerpc: Rename __get_SP() to current_stack_pointer()") for details.
The purpose of check_stack_overflow() is to verify that the stack has
not overflowed.
To really know whether the stack pointer is still within boundaries,
the check must be done directly on the value of r1.
Define a get_sp() macro to get the value of r1 directly. (Adapted from
arch/powerpc/boot/reg.h)
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
v2: Define a get_sp() macro instead of using asm("r1") locally.
---
arch/powerpc/include/asm/reg.h | 3 +++
arch/powerpc/kernel/irq.c | 2 +-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index 1aa46dff0957..d585f2566338 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -1450,6 +1450,9 @@ static inline void mtsrin(u32 val, u32 idx)
extern unsigned long current_stack_pointer(void);
+register unsigned long __stack_pointer asm("r1");
+#define get_sp() (__stack_pointer)
+
extern unsigned long scom970_read(unsigned int address);
extern void scom970_write(unsigned int address, unsigned long value);
diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
index add67498c126..cd29c2eb2d8e 100644
--- a/arch/powerpc/kernel/irq.c
+++ b/arch/powerpc/kernel/irq.c
@@ -601,7 +601,7 @@ static inline void check_stack_overflow(void)
#ifdef CONFIG_DEBUG_STACKOVERFLOW
long sp;
- sp = current_stack_pointer() & (THREAD_SIZE-1);
+ sp = get_sp() & (THREAD_SIZE - 1);
/* check for stack overflow: is there less than 2KB free? */
if (unlikely(sp < 2048)) {
--
2.25.0
^ permalink raw reply related
* [PATCH v2 2/3] powerpc/irq: use IS_ENABLED() in check_stack_overflow()
From: Christophe Leroy @ 2020-01-24 7:07 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <435e0030e942507766cbef5bc95f906262d2ccf2.1579849665.git.christophe.leroy@c-s.fr>
Instead of #ifdef, use IS_ENABLED(CONFIG_DEBUG_STACKOVERFLOW).
This enable GCC to check for code validity even when the option
is not selected.
---
v2: rebased
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/kernel/irq.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
index cd29c2eb2d8e..9333e115418f 100644
--- a/arch/powerpc/kernel/irq.c
+++ b/arch/powerpc/kernel/irq.c
@@ -598,9 +598,11 @@ u64 arch_irq_stat_cpu(unsigned int cpu)
static inline void check_stack_overflow(void)
{
-#ifdef CONFIG_DEBUG_STACKOVERFLOW
long sp;
+ if (!IS_ENABLED(CONFIG_DEBUG_STACKOVERFLOW))
+ return;
+
sp = get_sp() & (THREAD_SIZE - 1);
/* check for stack overflow: is there less than 2KB free? */
@@ -608,7 +610,6 @@ static inline void check_stack_overflow(void)
pr_err("do_IRQ: stack overflow: %ld\n", sp);
dump_stack();
}
-#endif
}
void __do_irq(struct pt_regs *regs)
--
2.25.0
^ permalink raw reply related
* [PATCH v2 3/3] powerpc/irq: don't use current_stack_pointer() in do_IRQ()
From: Christophe Leroy @ 2020-01-24 7:07 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <435e0030e942507766cbef5bc95f906262d2ccf2.1579849665.git.christophe.leroy@c-s.fr>
Until commit 7306e83ccf5c ("powerpc: Don't use CURRENT_THREAD_INFO to
find the stack"), the current stack base address was obtained by
calling current_thread_info(). That inline function was simply masking
out the value of r1.
In that commit, it was changed to using current_stack_pointer(), which
is an heavier function as it is an outline assembly function which
cannot be inlined and which reads the content of the stack at 0(r1)
Revert to just using get_sp() for geting r1 and masking out its value
to obtain the base address of the stack pointer as before.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
Fixes: 7306e83ccf5c ("powerpc: Don't use CURRENT_THREAD_INFO to find the stack")
---
v2: New in this series. Using get_sp()
---
arch/powerpc/kernel/irq.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
index 9333e115418f..193c2b64ce37 100644
--- a/arch/powerpc/kernel/irq.c
+++ b/arch/powerpc/kernel/irq.c
@@ -647,7 +647,7 @@ void do_IRQ(struct pt_regs *regs)
void *cursp, *irqsp, *sirqsp;
/* Switch to the irq stack to handle this */
- cursp = (void *)(current_stack_pointer() & ~(THREAD_SIZE - 1));
+ cursp = (void *)(get_sp() & ~(THREAD_SIZE - 1));
irqsp = hardirq_ctx[raw_smp_processor_id()];
sirqsp = softirq_ctx[raw_smp_processor_id()];
--
2.25.0
^ permalink raw reply related
* Re: [PATCH 1/2] powerpc/irq: don't use current_stack_pointer() in check_stack_overflow()
From: Segher Boessenkool @ 2020-01-24 8:44 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, Paul Mackerras, linux-kernel
In-Reply-To: <87d0b9iez3.fsf@mpe.ellerman.id.au>
Hi!
On Fri, Jan 24, 2020 at 04:46:24PM +1100, Michael Ellerman wrote:
> Christophe Leroy <christophe.leroy@c-s.fr> writes:
> > static inline void check_stack_overflow(void)
> > {
> > #ifdef CONFIG_DEBUG_STACKOVERFLOW
> > - long sp;
> > -
> > - sp = current_stack_pointer() & (THREAD_SIZE-1);
> > + register unsigned long r1 asm("r1");
> > + long sp = r1 & (THREAD_SIZE - 1);
>
> This appears to work but seems to be "unsupported" by GCC, and clang
> actually complains about it:
>
> /linux/arch/powerpc/kernel/irq.c:603:12: error: variable 'r1' is uninitialized when used here [-Werror,-Wuninitialized]
> long sp = r1 & (THREAD_SIZE - 1);
> ^~
>
> The GCC docs say:
>
> The only supported use for this feature is to specify registers for
> input and output operands when calling Extended asm (see Extended
> Asm).
>
> https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gcc/Local-Register-Variables.html#Local-Register-Variables
Yes. Don't use local register variables any other way. It *will* break.
> If I do this it seems to work, but feels a little dicey:
>
> asm ("" : "=r" (r1));
> sp = r1 & (THREAD_SIZE - 1);
The only thing dicey about that is that you are writing to r1. Heh.
Well that certainly is bad enough, the compiler does not know how to
handle that at all... Of course you aren't *actually* changing
anything, so it might just work.
Segher
^ permalink raw reply
* Re: [PATCH 1/2] powerpc/irq: don't use current_stack_pointer() in check_stack_overflow()
From: Segher Boessenkool @ 2020-01-24 8:58 UTC (permalink / raw)
To: Christophe Leroy; +Cc: Paul Mackerras, linuxppc-dev, linux-kernel
In-Reply-To: <74cb4227-1a24-6fe1-2df4-3d4b069453c4@c-s.fr>
On Fri, Jan 24, 2020 at 07:03:36AM +0000, Christophe Leroy wrote:
> >Le 24/01/2020 à 06:46, Michael Ellerman a écrit :
> >>
> >>If I do this it seems to work, but feels a little dicey:
> >>
> >> asm ("" : "=r" (r1));
> >> sp = r1 & (THREAD_SIZE - 1);
> >
> >
> >Or we could do add in asm/reg.h what we have in boot/reg.h:
> >
> >register void *__stack_pointer asm("r1");
> >#define get_sp() (__stack_pointer)
> >
> >And use get_sp()
> >
>
> It works, and I guess doing it this way is acceptable as it's exactly
> what's done for current in asm/current.h with register r2.
That is a *global* register variable. That works. We still need to
document a bit better what it does exactly, but this is the expected
use case, so that will work.
> Now I (still) get:
>
> sp = get_sp() & (THREAD_SIZE - 1);
> b9c: 54 24 04 fe clrlwi r4,r1,19
> if (unlikely(sp < 2048)) {
> ba4: 2f 84 07 ff cmpwi cr7,r4,2047
>
> Allthough GCC 8.1 what doing exactly the same with the form CLANG don't
> like:
>
> register unsigned long r1 asm("r1");
> long sp = r1 & (THREAD_SIZE - 1);
> b84: 54 24 04 fe clrlwi r4,r1,19
> if (unlikely(sp < 2048)) {
> b8c: 2f 84 07 ff cmpwi cr7,r4,2047
Sure, if it did what you expected, things will usually work out fine ;-)
(Pity that the compiler didn't come up with
rlwinm. r4,r1,0,19,20
bne bad
Or are the low bits of r4 used later again?)
Segher
^ permalink raw reply
* [PATCH 4.14 243/343] perf/ioctl: Add check for the sample_period value
From: Greg Kroah-Hartman @ 2020-01-24 9:31 UTC (permalink / raw)
To: linux-kernel
Cc: Sasha Levin, Ravi Bangoria, maddy, acme, Vince Weaver,
Peter Zijlstra (Intel), Greg Kroah-Hartman, Linus Torvalds,
linuxppc-dev, stable, Arnaldo Carvalho de Melo,
Alexander Shishkin, Stephane Eranian, Thomas Gleixner, Jiri Olsa,
Ingo Molnar
In-Reply-To: <20200124092919.490687572@linuxfoundation.org>
From: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
[ Upstream commit 913a90bc5a3a06b1f04c337320e9aeee2328dd77 ]
perf_event_open() limits the sample_period to 63 bits. See:
0819b2e30ccb ("perf: Limit perf_event_attr::sample_period to 63 bits")
Make ioctl() consistent with it.
Also on PowerPC, negative sample_period could cause a recursive
PMIs leading to a hang (reported when running perf-fuzzer).
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vince Weaver <vincent.weaver@maine.edu>
Cc: acme@kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: maddy@linux.vnet.ibm.com
Cc: mpe@ellerman.id.au
Fixes: 0819b2e30ccb ("perf: Limit perf_event_attr::sample_period to 63 bits")
Link: https://lkml.kernel.org/r/20190604042953.914-1-ravi.bangoria@linux.ibm.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
kernel/events/core.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index ea4f3f7a0c6f3..2ac73b4cb8a93 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4762,6 +4762,9 @@ static int perf_event_period(struct perf_event *event, u64 __user *arg)
if (perf_event_check_period(event, value))
return -EINVAL;
+ if (!event->attr.freq && (value & (1ULL << 63)))
+ return -EINVAL;
+
event_function_call(event, __perf_event_period, &value);
return 0;
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v2 1/6] fs/readdir: Fix filldir() and filldir64() use of user_access_begin()
From: Michael Ellerman @ 2020-01-24 10:42 UTC (permalink / raw)
To: Linus Torvalds
Cc: Linux Kernel Mailing List, Linux-MM, Paul Mackerras,
Alexander Viro, linux-fsdevel, Andrew Morton, linuxppc-dev
In-Reply-To: <CAHk-=whCk8z2_kggSCoAGMne8PNSvcT2T4bBH62ngoFrsTyV6w@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Thu, Jan 23, 2020 at 4:00 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
>>
>> So I guess I'll wait and see what happens with patch 1.
>
> I've committed my fixes to filldir[64]() directly - they really were
> fixing me being lazy about the range, and the name length checking
> really is a theoretical "access wrong user space pointer" issue with
> corrupted filesystems regardless (even though I suspect it's entirely
> theoretical - even a corrupt filesystem hopefully won't be passing in
> negative directory entry lengths or something like that).
Great, thanks.
> The "pass in read/write" part I'm not entirely convinced about.
> Honestly, if this is just for ppc32 and nobody else really needs it,
> make the ppc32s thing always just enable both user space reads and
> writes. That's the semantics for x86 and arm as is, I'm not convinced
> that we should complicate this for a legacy platform.
We can use the read/write info on Power9 too. That's a niche platform
but hopefully not legacy status yet :P
But it's entirely optional, as you say we can just enable read/write if
we aren't passed the read/write info from the upper-level API.
I think our priority should be getting objtool going on powerpc to check
our user access regions are well contained. Once we have that working
maybe then we can look at plumbing the direction through
user_access_begin() etc.
cheers
^ permalink raw reply
* Re: vmlinux ELF header sometimes corrupt
From: Michael Ellerman @ 2020-01-24 10:50 UTC (permalink / raw)
To: Rasmus Villemoes, LKML
Cc: linuxppc-dev@lists.ozlabs.org, Linux Kbuild mailing list
In-Reply-To: <71aa76d0-a3b8-b4f3-a7c3-766cfb75412f@rasmusvillemoes.dk>
Rasmus Villemoes <linux@rasmusvillemoes.dk> writes:
> I'm building for a ppc32 (mpc8309) target using Yocto, and I'm hitting a
> very hard to debug problem that maybe someone else has encountered. This
> doesn't happen always, perhaps 1 in 8 times or something like that.
>
> The issue is that when the build gets to do "${CROSS}objcopy -O binary
> ... vmlinux", vmlinux is not (no longer) a proper ELF file, so naturally
> that fails with
>
> powerpc-oe-linux-objcopy:vmlinux: file format not recognized
>
> So I hacked link-vmlinux.sh to stash copies of vmlinux before and after
> sortextable vmlinux. Both of those are proper ELF files, and comparing
> the corrupted vmlinux to vmlinux.after_sort they are identical after the
> first 52 bytes; in vmlinux, those first 52 bytes are all 0.
>
> I also saved stat(1) info to see if vmlinux is being replaced or
> modified in-place.
>
> $ cat vmlinux.stat.after_sort
> File: 'vmlinux'
> Size: 8608456 Blocks: 16696 IO Block: 4096 regular file
> Device: 811h/2065d Inode: 21919132 Links: 1
> Access: (0755/-rwxr-xr-x) Uid: ( 1000/ user) Gid: ( 1001/ user)
> Access: 2020-01-22 10:52:38.946703081 +0000
> Modify: 2020-01-22 10:52:38.954703105 +0000
> Change: 2020-01-22 10:52:38.954703105 +0000
>
> $ stat vmlinux
> File: 'vmlinux'
> Size: 8608456 Blocks: 16688 IO Block: 4096 regular file
> Device: 811h/2065d Inode: 21919132 Links: 1
> Access: (0755/-rwxr-xr-x) Uid: ( 1000/ user) Gid: ( 1001/ user)
> Access: 2020-01-22 17:20:00.650379057 +0000
> Modify: 2020-01-22 10:52:38.954703105 +0000
> Change: 2020-01-22 10:52:38.954703105 +0000
>
> So the inode number and mtime/ctime are exactly the same, but for some
> reason Blocks: has changed? This is on an ext4 filesystem, but I don't
> suspect the filesystem to be broken, because it's always just vmlinux
> that ends up corrupt, and always in exactly this way with the first 52
> bytes having been wiped.
>
> Any ideas?
Not really sorry. Haven't seen or heard of that before.
Are you doing a parallel make? If so does -j 1 fix it?
If it seems like sortextable is at fault then strace'ing it would be my
next step.
cheers
^ permalink raw reply
* Re: vmlinux ELF header sometimes corrupt
From: Rasmus Villemoes @ 2020-01-24 11:16 UTC (permalink / raw)
To: Michael Ellerman, LKML
Cc: linuxppc-dev@lists.ozlabs.org, Linux Kbuild mailing list
In-Reply-To: <875zh1i0wj.fsf@mpe.ellerman.id.au>
On 24/01/2020 11.50, Michael Ellerman wrote:
> Rasmus Villemoes <linux@rasmusvillemoes.dk> writes:
>> I'm building for a ppc32 (mpc8309) target using Yocto, and I'm hitting a
>> very hard to debug problem that maybe someone else has encountered. This
>> doesn't happen always, perhaps 1 in 8 times or something like that.
>>
>> The issue is that when the build gets to do "${CROSS}objcopy -O binary
>> ... vmlinux", vmlinux is not (no longer) a proper ELF file, so naturally
>> that fails with
>>
>> powerpc-oe-linux-objcopy:vmlinux: file format not recognized
>>
>>
>> Any ideas?
>
> Not really sorry. Haven't seen or heard of that before.
>
> Are you doing a parallel make? If so does -j 1 fix it?
Hard to say, I'll have to try that a number of times to see if it can be
reproduced with that setting.
> If it seems like sortextable is at fault then strace'ing it would be my
> next step.
I don't think sortextable is at fault, that was just my first "I know
that at least pokes around in the ELF file". I do "cp vmlinux
vmlinux.before_sort" and "cp vmlinux vmlinux.after_sort", and both of
those copies are proper ELF files - and the .after_sort is identical to
the corrupt vmlinux apart from vmlinux ending up with its ELF header wiped.
So it's something that happens during some later build step (Yocto has a
lot of steps), perhaps "make modules" or "make modules_install" or
something ends up somehow deciding "hey, vmlinux isn't quite uptodate,
let's nuke it". I'm not even sure it's a Kbuild problem, but I've seen
the same thing happen using another meta-build system called oe-lite,
which is why I'm not primarily suspecting the Yocto logic.
Rasmus
^ permalink raw reply
* [PATCH 4.19 447/639] perf/ioctl: Add check for the sample_period value
From: Greg Kroah-Hartman @ 2020-01-24 9:30 UTC (permalink / raw)
To: linux-kernel
Cc: Sasha Levin, Ravi Bangoria, maddy, acme, Vince Weaver,
Peter Zijlstra (Intel), Greg Kroah-Hartman, Linus Torvalds,
linuxppc-dev, stable, Arnaldo Carvalho de Melo,
Alexander Shishkin, Stephane Eranian, Thomas Gleixner, Jiri Olsa,
Ingo Molnar
In-Reply-To: <20200124093047.008739095@linuxfoundation.org>
From: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
[ Upstream commit 913a90bc5a3a06b1f04c337320e9aeee2328dd77 ]
perf_event_open() limits the sample_period to 63 bits. See:
0819b2e30ccb ("perf: Limit perf_event_attr::sample_period to 63 bits")
Make ioctl() consistent with it.
Also on PowerPC, negative sample_period could cause a recursive
PMIs leading to a hang (reported when running perf-fuzzer).
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vince Weaver <vincent.weaver@maine.edu>
Cc: acme@kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: maddy@linux.vnet.ibm.com
Cc: mpe@ellerman.id.au
Fixes: 0819b2e30ccb ("perf: Limit perf_event_attr::sample_period to 63 bits")
Link: https://lkml.kernel.org/r/20190604042953.914-1-ravi.bangoria@linux.ibm.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
kernel/events/core.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 751888cbed5c0..16af86ab24c42 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -5012,6 +5012,9 @@ static int perf_event_period(struct perf_event *event, u64 __user *arg)
if (perf_event_check_period(event, value))
return -EINVAL;
+ if (!event->attr.freq && (value & (1ULL << 63)))
+ return -EINVAL;
+
event_function_call(event, __perf_event_period, &value);
return 0;
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v2 6/6] powerpc: Implement user_access_begin and friends
From: Christophe Leroy @ 2020-01-24 11:40 UTC (permalink / raw)
To: Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras
Cc: linux-fsdevel, linux-mm, linuxppc-dev, linux-kernel
In-Reply-To: <87ftg6icc8.fsf@mpe.ellerman.id.au>
Le 23/01/2020 à 13:31, Michael Ellerman a écrit :
> Michael Ellerman <mpe@ellerman.id.au> writes:
>> Christophe Leroy <christophe.leroy@c-s.fr> writes:
>>> Today, when a function like strncpy_from_user() is called,
>>> the userspace access protection is de-activated and re-activated
>>> for every word read.
>>>
>>> By implementing user_access_begin and friends, the protection
>>> is de-activated at the beginning of the copy and re-activated at the
>>> end.
>>>
>>> Implement user_access_begin(), user_access_end() and
>>> unsafe_get_user(), unsafe_put_user() and unsafe_copy_to_user()
>>>
>>> For the time being, we keep user_access_save() and
>>> user_access_restore() as nops.
>>
>> That means we will run with user access enabled in a few more places, but
>> it's only used sparingly AFAICS:
>>
>> kernel/trace/trace_branch.c: unsigned long flags = user_access_save();
>> lib/ubsan.c: unsigned long flags = user_access_save();
>> lib/ubsan.c: unsigned long ua_flags = user_access_save();
>> mm/kasan/common.c: unsigned long flags = user_access_save();
>>
>> And we don't have objtool checking that user access enablement isn't
>> leaking in the first place, so I guess it's OK for us not to implement
>> these to begin with?
>
> It looks like we can implement them on on all three KUAP
> implementations.
>
> For radix and 8xx we just return/set the relevant SPR.
>
> For book3s/32/kup.h I think we'd just need to add a KUAP_CURRENT case to
> allow_user_access()?
Can't do that, we don't want to keep the info in current->thread.kuap
after user_access_save(), otherwise we might unexpectedly re-open access
through an interrupt.
And if we use KUAP_CURRENT case of prevent_user_access(), it means we'll
read current->thread.kuap twice.
So, just regenerate addr and end from the flags, and use
allow_user_access() and prevent_user_access() as usual.
I'll have it in v4
Christophe
^ permalink raw reply
* Re: [FSL P5020 P5040 PPC] Onboard SD card doesn't work anymore after the 'mmc-v5.4-2' updates
From: Michael Ellerman @ 2020-01-24 11:42 UTC (permalink / raw)
To: Ulf Hansson, Christian Zigotzky
Cc: Darren Stevens, mad skateman, linux-mmc@vger.kernel.org,
Julian Margetson, Christoph Hellwig, Rob Herring,
contact@a-eon.com, R.T.Dickinson, linuxppc-dev
In-Reply-To: <CAPDyKFoydOuSE=Eaq168=2_Ycouo7hzyw+RZXhGq7q4D4Qe8TQ@mail.gmail.com>
Ulf Hansson <ulf.hansson@linaro.org> writes:
> On Thu, 16 Jan 2020 at 12:18, Christian Zigotzky <chzigotzky@xenosoft.de> wrote:
>>
>> Hi All,
>>
>> We still need the attached patch for our onboard SD card interface
>> [1,2]. Could you please add this patch to the tree?
>
> No, because according to previous discussion that isn't the correct
> solution and more importantly it will break other archs (if I recall
> correctly).
>
> Looks like someone from the ppc community needs to pick up the ball.
That's a pretty small community these days :) :/
Christian can you test this please? I think I got the polarity of all
the tests right, but it's Friday night so maybe I'm wrong :)
cheers
From 975ba6e8b52d6f5358e93c1f5a47adc4a0b5fb70 Mon Sep 17 00:00:00 2001
From: Michael Ellerman <mpe@ellerman.id.au>
Date: Fri, 24 Jan 2020 22:26:59 +1100
Subject: [PATCH] of: Add OF_DMA_DEFAULT_COHERENT & select it on powerpc
There's an OF helper called of_dma_is_coherent(), which checks if a
device has a "dma-coherent" property to see if the device is coherent
for DMA.
But on some platforms devices are coherent by default, and on some
platforms it's not possible to update existing device trees to add the
"dma-coherent" property.
So add a Kconfig symbol to allow arch code to tell
of_dma_is_coherent() that devices are coherent by default, regardless
of the presence of the property.
Select that symbol on powerpc when NOT_COHERENT_CACHE is not set, ie.
when the system has a coherent cache.
Fixes: 92ea637edea3 ("of: introduce of_dma_is_coherent() helper")
Cc: stable@vger.kernel.org # v3.16+
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/Kconfig | 1 +
drivers/of/Kconfig | 4 ++++
drivers/of/address.c | 6 +++++-
3 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 62752c3bfabf..460678ab2375 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -235,6 +235,7 @@ config PPC
select NEED_DMA_MAP_STATE if PPC64 || NOT_COHERENT_CACHE
select NEED_SG_DMA_LENGTH
select OF
+ select OF_DMA_DEFAULT_COHERENT if !NOT_COHERENT_CACHE
select OF_EARLY_FLATTREE
select OLD_SIGACTION if PPC32
select OLD_SIGSUSPEND
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index 37c2ccbefecd..d91618641be6 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -103,4 +103,8 @@ config OF_OVERLAY
config OF_NUMA
bool
+config OF_DMA_DEFAULT_COHERENT
+ # arches should select this if DMA is coherent by default for OF devices
+ bool
+
endif # OF
diff --git a/drivers/of/address.c b/drivers/of/address.c
index 99c1b8058559..e8a39c3ec4d4 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -995,12 +995,16 @@ int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *siz
* @np: device node
*
* It returns true if "dma-coherent" property was found
- * for this device in DT.
+ * for this device in the DT, or if DMA is coherent by
+ * default for OF devices on the current platform.
*/
bool of_dma_is_coherent(struct device_node *np)
{
struct device_node *node = of_node_get(np);
+ if (IS_ENABLED(CONFIG_OF_DMA_DEFAULT_COHERENT))
+ return true;
+
while (node) {
if (of_property_read_bool(node, "dma-coherent")) {
of_node_put(node);
--
2.21.1
^ permalink raw reply related
* [PATCH v4 1/7] readdir: make user_access_begin() use the real access range
From: Christophe Leroy @ 2020-01-24 11:54 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linux-mm, linuxppc-dev, linux-kernel
From: Linus Torvalds <torvalds@linux-foundation.org>
In commit 9f79b78ef744 ("Convert filldir[64]() from __put_user() to
unsafe_put_user()") I changed filldir to not do individual __put_user()
accesses, but instead use unsafe_put_user() surrounded by the proper
user_access_begin/end() pair.
That make them enormously faster on modern x86, where the STAC/CLAC
games make individual user accesses fairly heavy-weight.
However, the user_access_begin() range was not really the exact right
one, since filldir() has the unfortunate problem that it needs to not
only fill out the new directory entry, it also needs to fix up the
previous one to contain the proper file offset.
It's unfortunate, but the "d_off" field in "struct dirent" is _not_ the
file offset of the directory entry itself - it's the offset of the next
one. So we end up backfilling the offset in the previous entry as we
walk along.
But since x86 didn't really care about the exact range, and used to be
the only architecture that did anything fancy in user_access_begin() to
begin with, the filldir[64]() changes did something lazy, and even
commented on it:
/*
* Note! This range-checks 'previous' (which may be NULL).
* The real range was checked in getdents
*/
if (!user_access_begin(dirent, sizeof(*dirent)))
goto efault;
and it all worked fine.
But now 32-bit ppc is starting to also implement user_access_begin(),
and the fact that we faked the range to only be the (possibly not even
valid) previous directory entry becomes a problem, because ppc32 will
actually be using the range that is passed in for more than just "check
that it's user space".
This is a complete rewrite of Christophe's original patch.
By saving off the record length of the previous entry instead of a
pointer to it in the filldir data structures, we can simplify the range
check and the writing of the previous entry d_off field. No need for
any conditionals in the user accesses themselves, although we retain the
conditional EINTR checking for the "was this the first directory entry"
signal handling latency logic.
Fixes: 9f79b78ef744 ("Convert filldir[64]() from __put_user() to unsafe_put_user()")
Link: https://lore.kernel.org/lkml/a02d3426f93f7eb04960a4d9140902d278cab0bb.1579697910.git.christophe.leroy@c-s.fr/
Link: https://lore.kernel.org/lkml/408c90c4068b00ea8f1c41cca45b84ec23d4946b.1579783936.git.christophe.leroy@c-s.fr/
Reported-and-tested-by: Christophe Leroy <christophe.leroy@c-s.fr>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
v4: taken from Linus' tree
---
fs/readdir.c | 73 +++++++++++++++++++++++++---------------------------
1 file changed, 35 insertions(+), 38 deletions(-)
diff --git a/fs/readdir.c b/fs/readdir.c
index d26d5ea4de7b..d5ee72280c82 100644
--- a/fs/readdir.c
+++ b/fs/readdir.c
@@ -206,7 +206,7 @@ struct linux_dirent {
struct getdents_callback {
struct dir_context ctx;
struct linux_dirent __user * current_dir;
- struct linux_dirent __user * previous;
+ int prev_reclen;
int count;
int error;
};
@@ -214,12 +214,13 @@ struct getdents_callback {
static int filldir(struct dir_context *ctx, const char *name, int namlen,
loff_t offset, u64 ino, unsigned int d_type)
{
- struct linux_dirent __user * dirent;
+ struct linux_dirent __user *dirent, *prev;
struct getdents_callback *buf =
container_of(ctx, struct getdents_callback, ctx);
unsigned long d_ino;
int reclen = ALIGN(offsetof(struct linux_dirent, d_name) + namlen + 2,
sizeof(long));
+ int prev_reclen;
buf->error = verify_dirent_name(name, namlen);
if (unlikely(buf->error))
@@ -232,28 +233,24 @@ static int filldir(struct dir_context *ctx, const char *name, int namlen,
buf->error = -EOVERFLOW;
return -EOVERFLOW;
}
- dirent = buf->previous;
- if (dirent && signal_pending(current))
+ prev_reclen = buf->prev_reclen;
+ if (prev_reclen && signal_pending(current))
return -EINTR;
-
- /*
- * Note! This range-checks 'previous' (which may be NULL).
- * The real range was checked in getdents
- */
- if (!user_access_begin(dirent, sizeof(*dirent)))
- goto efault;
- if (dirent)
- unsafe_put_user(offset, &dirent->d_off, efault_end);
dirent = buf->current_dir;
+ prev = (void __user *) dirent - prev_reclen;
+ if (!user_access_begin(prev, reclen + prev_reclen))
+ goto efault;
+
+ /* This might be 'dirent->d_off', but if so it will get overwritten */
+ unsafe_put_user(offset, &prev->d_off, efault_end);
unsafe_put_user(d_ino, &dirent->d_ino, efault_end);
unsafe_put_user(reclen, &dirent->d_reclen, efault_end);
unsafe_put_user(d_type, (char __user *) dirent + reclen - 1, efault_end);
unsafe_copy_dirent_name(dirent->d_name, name, namlen, efault_end);
user_access_end();
- buf->previous = dirent;
- dirent = (void __user *)dirent + reclen;
- buf->current_dir = dirent;
+ buf->current_dir = (void __user *)dirent + reclen;
+ buf->prev_reclen = reclen;
buf->count -= reclen;
return 0;
efault_end:
@@ -267,7 +264,6 @@ SYSCALL_DEFINE3(getdents, unsigned int, fd,
struct linux_dirent __user *, dirent, unsigned int, count)
{
struct fd f;
- struct linux_dirent __user * lastdirent;
struct getdents_callback buf = {
.ctx.actor = filldir,
.count = count,
@@ -285,8 +281,10 @@ SYSCALL_DEFINE3(getdents, unsigned int, fd,
error = iterate_dir(f.file, &buf.ctx);
if (error >= 0)
error = buf.error;
- lastdirent = buf.previous;
- if (lastdirent) {
+ if (buf.prev_reclen) {
+ struct linux_dirent __user * lastdirent;
+ lastdirent = (void __user *)buf.current_dir - buf.prev_reclen;
+
if (put_user(buf.ctx.pos, &lastdirent->d_off))
error = -EFAULT;
else
@@ -299,7 +297,7 @@ SYSCALL_DEFINE3(getdents, unsigned int, fd,
struct getdents_callback64 {
struct dir_context ctx;
struct linux_dirent64 __user * current_dir;
- struct linux_dirent64 __user * previous;
+ int prev_reclen;
int count;
int error;
};
@@ -307,11 +305,12 @@ struct getdents_callback64 {
static int filldir64(struct dir_context *ctx, const char *name, int namlen,
loff_t offset, u64 ino, unsigned int d_type)
{
- struct linux_dirent64 __user *dirent;
+ struct linux_dirent64 __user *dirent, *prev;
struct getdents_callback64 *buf =
container_of(ctx, struct getdents_callback64, ctx);
int reclen = ALIGN(offsetof(struct linux_dirent64, d_name) + namlen + 1,
sizeof(u64));
+ int prev_reclen;
buf->error = verify_dirent_name(name, namlen);
if (unlikely(buf->error))
@@ -319,30 +318,27 @@ static int filldir64(struct dir_context *ctx, const char *name, int namlen,
buf->error = -EINVAL; /* only used if we fail.. */
if (reclen > buf->count)
return -EINVAL;
- dirent = buf->previous;
- if (dirent && signal_pending(current))
+ prev_reclen = buf->prev_reclen;
+ if (prev_reclen && signal_pending(current))
return -EINTR;
-
- /*
- * Note! This range-checks 'previous' (which may be NULL).
- * The real range was checked in getdents
- */
- if (!user_access_begin(dirent, sizeof(*dirent)))
- goto efault;
- if (dirent)
- unsafe_put_user(offset, &dirent->d_off, efault_end);
dirent = buf->current_dir;
+ prev = (void __user *)dirent - prev_reclen;
+ if (!user_access_begin(prev, reclen + prev_reclen))
+ goto efault;
+
+ /* This might be 'dirent->d_off', but if so it will get overwritten */
+ unsafe_put_user(offset, &prev->d_off, efault_end);
unsafe_put_user(ino, &dirent->d_ino, efault_end);
unsafe_put_user(reclen, &dirent->d_reclen, efault_end);
unsafe_put_user(d_type, &dirent->d_type, efault_end);
unsafe_copy_dirent_name(dirent->d_name, name, namlen, efault_end);
user_access_end();
- buf->previous = dirent;
- dirent = (void __user *)dirent + reclen;
- buf->current_dir = dirent;
+ buf->prev_reclen = reclen;
+ buf->current_dir = (void __user *)dirent + reclen;
buf->count -= reclen;
return 0;
+
efault_end:
user_access_end();
efault:
@@ -354,7 +350,6 @@ int ksys_getdents64(unsigned int fd, struct linux_dirent64 __user *dirent,
unsigned int count)
{
struct fd f;
- struct linux_dirent64 __user * lastdirent;
struct getdents_callback64 buf = {
.ctx.actor = filldir64,
.count = count,
@@ -372,9 +367,11 @@ int ksys_getdents64(unsigned int fd, struct linux_dirent64 __user *dirent,
error = iterate_dir(f.file, &buf.ctx);
if (error >= 0)
error = buf.error;
- lastdirent = buf.previous;
- if (lastdirent) {
+ if (buf.prev_reclen) {
+ struct linux_dirent64 __user * lastdirent;
typeof(lastdirent->d_off) d_off = buf.ctx.pos;
+
+ lastdirent = (void __user *) buf.current_dir - buf.prev_reclen;
if (__put_user(d_off, &lastdirent->d_off))
error = -EFAULT;
else
--
2.25.0
^ permalink raw reply related
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