* Re: [PATCH 2/4] ocxl: Access interrupt trigger page from xive directly
From: Andrew Donnellan @ 2020-04-03 6:36 UTC (permalink / raw)
To: Frederic Barrat, linuxppc-dev, clg, christophe_lombard, ukrishn,
mrochs
Cc: haren, groug
In-Reply-To: <20200402154352.586166-3-fbarrat@linux.ibm.com>
On 3/4/20 2:43 am, Frederic Barrat wrote:
> We can access the trigger page through standard APIs so let's use it
> and avoid saving it when allocating the interrupt. It will also allow
> to simplify allocation in a later patch.
>
> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
I don't see any obvious issues.
Acked-by: Andrew Donnellan <ajd@linux.ibm.com>
--
Andrew Donnellan OzLabs, ADL Canberra
ajd@linux.ibm.com IBM Australia Limited
^ permalink raw reply
* Re: [PATCH 4/4] ocxl: Remove custom service to allocate interrupts
From: Cédric Le Goater @ 2020-04-03 5:56 UTC (permalink / raw)
To: Frederic Barrat, linuxppc-dev, christophe_lombard, ajd, ukrishn,
mrochs
Cc: haren, groug
In-Reply-To: <20200402154352.586166-5-fbarrat@linux.ibm.com>
On 4/2/20 5:43 PM, Frederic Barrat wrote:
> We now allocate interrupts through xive directly.
>
> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
> ---
> arch/powerpc/include/asm/pnv-ocxl.h | 3 ---
> arch/powerpc/platforms/powernv/ocxl.c | 30 ---------------------------
> 2 files changed, 33 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/pnv-ocxl.h b/arch/powerpc/include/asm/pnv-ocxl.h
> index 7de82647e761..e90650328c9c 100644
> --- a/arch/powerpc/include/asm/pnv-ocxl.h
> +++ b/arch/powerpc/include/asm/pnv-ocxl.h
> @@ -30,7 +30,4 @@ extern int pnv_ocxl_spa_setup(struct pci_dev *dev, void *spa_mem, int PE_mask,
> extern void pnv_ocxl_spa_release(void *platform_data);
> extern int pnv_ocxl_spa_remove_pe_from_cache(void *platform_data, int pe_handle);
>
> -extern int pnv_ocxl_alloc_xive_irq(u32 *irq, u64 *trigger_addr);
> -extern void pnv_ocxl_free_xive_irq(u32 irq);
> -
> #endif /* _ASM_PNV_OCXL_H */
> diff --git a/arch/powerpc/platforms/powernv/ocxl.c b/arch/powerpc/platforms/powernv/ocxl.c
> index 8c65aacda9c8..ecdad219d704 100644
> --- a/arch/powerpc/platforms/powernv/ocxl.c
> +++ b/arch/powerpc/platforms/powernv/ocxl.c
> @@ -2,7 +2,6 @@
> // Copyright 2017 IBM Corp.
> #include <asm/pnv-ocxl.h>
> #include <asm/opal.h>
> -#include <asm/xive.h>
> #include <misc/ocxl-config.h>
> #include "pci.h"
>
> @@ -484,32 +483,3 @@ int pnv_ocxl_spa_remove_pe_from_cache(void *platform_data, int pe_handle)
> return rc;
> }
> EXPORT_SYMBOL_GPL(pnv_ocxl_spa_remove_pe_from_cache);
> -
> -int pnv_ocxl_alloc_xive_irq(u32 *irq, u64 *trigger_addr)
> -{
> - __be64 flags, trigger_page;
> - s64 rc;
> - u32 hwirq;
> -
> - hwirq = xive_native_alloc_irq();
> - if (!hwirq)
> - return -ENOENT;
> -
> - rc = opal_xive_get_irq_info(hwirq, &flags, NULL, &trigger_page, NULL,
> - NULL);
> - if (rc || !trigger_page) {
> - xive_native_free_irq(hwirq);
> - return -ENOENT;
> - }
> - *irq = hwirq;
> - *trigger_addr = be64_to_cpu(trigger_page);
> - return 0;
> -
> -}
> -EXPORT_SYMBOL_GPL(pnv_ocxl_alloc_xive_irq);
> -
> -void pnv_ocxl_free_xive_irq(u32 irq)
> -{
> - xive_native_free_irq(irq);
> -}
> -EXPORT_SYMBOL_GPL(pnv_ocxl_free_xive_irq);
>
^ permalink raw reply
* Re: [PATCH v4 6/6] pseries/sysfs: Minimise IPI noise while reading [idle_][s]purr
From: Gautham R Shenoy @ 2020-04-03 6:28 UTC (permalink / raw)
To: Naveen N. Rao
Cc: ego, Tyrel Datwyler, linux-kernel, Kamalesh Babulal,
Vaidyanathan Srinivasan, linuxppc-dev
In-Reply-To: <1585811157.uig8s95yst.naveen@linux.ibm.com>
Hi Naveen,
On Thu, Apr 02, 2020 at 01:04:34PM +0530, Naveen N. Rao wrote:
[..snip..]
> >
> >It does reduce it to 10ms window. I am not sure if anyone samples PURR
> >etc faster than that rate.
> >
> >I measured how much time it takes to read the purr, spurr, idle_purr,
> >idle_spurr files back-to-back. It takes not more than 150us. From
> >lparstat will these values be read back-to-back ? If so, we can reduce
> >the staleness_tolerance to something like 500us and still avoid extra
> >IPIs. If not, what is the maximum delay between the first sysfs file
> >read and the last sysfs file read ?
>
> Oh, for lparstat usage, this is perfectly fine.
>
> I meant that there could be other users of [s]purr who might care. I don't
> know of one, but since this is an existing sysfs interface, I wanted to
> point out that the behavior might change.
Fair point. Perhaps this should be documented in the Documentation, if
we are going to continue with this patch.
>
> >
> >>
> >>I wonder if we should introduce a sysctl interface to control thresholding.
> >>It can default to 0, which disables thresholding so that the existing
> >>behavior continues. Applications (lparstat) can optionally set it to suit
> >>their use.
> >
> >We would be introducing 3 new sysfs interfaces that way instead of
> >two.
> >
> >/sys/devices/system/cpu/purr_spurr_staleness
> >/sys/devices/system/cpu/cpuX/idle_purr
> >/sys/devices/system/cpu/cpuX/idle_spurr
> >
> >I don't have a problem with this. Nathan, Michael, thoughts on this?
> >
> >
> >The alternative is to have a procfs interface, something like
> >/proc/powerpc/resource_util_stats
> >
> >which gives a listing similar to /proc/stat, i.e
> >
> > CPUX <purr> <idle_purr> <spurr> <idle_spurr>
> >
> >Even in this case, the values can be obtained in one-shot with a
> >single IPI and be printed in the row corresponding to the CPU.
>
> Right -- and that would be optimal requiring a single system call, at the
> cost of using a legacy interface.
>
> The other option would be to drop this patch and to just go with patches 1-5
> introducing the new sysfs interfaces for idle_[s]purr. It isn't entirely
> clear how often this would be used, or its actual impact. We can perhaps
> consider this optimization if and when this causes problems...
I am ok with that. We can revisit the problem if IPI noise becomes
noticable. However, if Nathan or Michael feel that this problem is
better solved now, than leaving it for the future, we will have to
take a call on what the interface is going to be.
>
>
> Thanks,
> Naveen
>
--
Thanks and Regards
gautham.
^ permalink raw reply
* Re: [PATCH v3 1/1] ppc/crash: Reset spinlocks during crash
From: Nicholas Piggin @ 2020-04-03 6:41 UTC (permalink / raw)
To: Alexios Zavras, Benjamin Herrenschmidt, Christophe Leroy,
Greg Kroah-Hartman, Enrico, Weigelt, Leonardo Bras,
Michael Ellerman, Paul Mackerras, peterz, Thomas, Gleixner
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <02e74be19534ab1db2f16a0c89ecb164e380c12a.camel@linux.ibm.com>
Leonardo Bras's on April 3, 2020 10:37 am:
> On Thu, 2020-04-02 at 22:28 +1100, Michael Ellerman wrote:
>> Leonardo Bras <leonardo@linux.ibm.com> writes:
>> > During a crash, there is chance that the cpus that handle the NMI IPI
>> > are holding a spin_lock. If this spin_lock is needed by crashing_cpu it
>> > will cause a deadlock. (rtas.lock and printk logbuf_lock as of today)
>> >
>> > This is a problem if the system has kdump set up, given if it crashes
>> > for any reason kdump may not be saved for crash analysis.
>> >
>> > After NMI IPI is sent to all other cpus, force unlock all spinlocks
>> > needed for finishing crash routine.
>>
>> I'm not convinced this is the right approach.
>
> Me neither. I think it's a very hacky solution, but I couldn't think of
> anything better at the time.
>
>> Busting locks is risky, it could easily cause a crash if data structures
>> are left in some inconsistent state.
>>
>> I think we need to make this code more careful about what it's doing.
>> There's a clue at the top of default_machine_crash_shutdown(), which
>> calls crash_kexec_prepare_cpus():
>>
>> * This function is only called after the system
>> * has panicked or is otherwise in a critical state.
>> * The minimum amount of code to allow a kexec'd kernel
>> * to run successfully needs to happen here.
>>
>>
>> You said the "IPI complete" message was the cause of one lockup:
>>
>> #0 arch_spin_lock
>> #1 do_raw_spin_lock
>> #2 __raw_spin_lock
>> #3 _raw_spin_lock
>> #4 vprintk_emit
>> #5 vprintk_func
>> #7 crash_kexec_prepare_cpus
>> #8 default_machine_crash_shutdown
>> #9 machine_crash_shutdown
>> #10 __crash_kexec
>> #11 crash_kexec
>> #12 oops_end
>>
>> TBH I think we could just drop that printk() entirely.
>>
>> Or we could tell printk() that we're in NMI context so that it uses the
>> percpu buffers.
>>
>> We should probably do the latter anyway, in case there's any other code
>> we call that inadvertently calls printk().
>>
>
> I was not aware of using per-cpu buffers in printk. It may be a nice
> solution.
>
> There is another printk call there:
> printk("kexec: Starting switchover sequence.\n");
> in default_machine_kexec().
>
> Two printk and one rtas call: it's all I could see using a spinlock
> after IPI Complete.
>
>>
>> The RTAS trace you sent was:
>>
>> #0 arch_spin_lock
>> #1 lock_rtas ()
>> #2 rtas_call (token=8204, nargs=1, nret=1, outputs=0x0)
>> #3 ics_rtas_mask_real_irq (hw_irq=4100)
>> #4 machine_kexec_mask_interrupts
>> #5 default_machine_crash_shutdown
>> #6 machine_crash_shutdown
>> #7 __crash_kexec
>> #8 crash_kexec
>> #9 oops_end
>>
>>
>> Which doesn't make it clear who holds the RTAS lock. We really shouldn't
>> be crashing while holding the RTAS lock, but I guess it could happen.
>> Can you get a full backtrace?
>>
>
> Oh, all traces are from the thread that called the crash, by writing
> 'c' to sysrq. That is what I am using to reproduce.
>
> #10 bad_page_fault
> #11 handle_page_fault
> #12 __handle_sysrq (key=99, check_mask=false)
> #13 write_sysrq_trigger
> #14 proc_reg_write
> #15 __vfs_write
> #16 vfs_write
> #17 SYSC_write
> #18 SyS_write
> #19 system_call
>
>>
>> PAPR says we are not allowed to have multiple CPUs calling RTAS at once,
>> except for a very small list of RTAS calls. So if we bust the RTAS lock
>> there's a risk we violate that part of PAPR and crash even harder.
>
> Interesting, I was not aware.
>
>>
>> Also it's not specific to kdump, we can't even get through a normal
>> reboot if we crash with the RTAS lock held.
>>
>> Anyway here's a patch with some ideas. That allows me to get from a
>> crash with the RTAS lock held through kdump into the 2nd kernel. But it
>> only works if it's the crashing CPU that holds the RTAS lock.
>>
>
> Nice idea.
> But my test environment is just triggering a crash from sysrq, so I
> think it would not improve the result, given that this thread is
> probably not holding the lock by the time.
Crash paths should not take that RTAS lock, it's a massive pain. I'm
fixing it for machine check, for other crashes I think it can be removed
too, it just needs to be unpicked. The good thing with crashing is that
you can reasonably *know* that you're single threaded, so you can
usually reason through situations like above.
> I noticed that when rtas is locked, irqs and preemption are also
> disabled.
>
> Should the IPI send by crash be able to interrupt a thread with
> disabled irqs?
Yes. It's been a bit painful, but in the long term it means that a CPU
which hangs with interrupts off can be debugged, and it means we can
take it offline to crash without risking that it will be clobbering what
we're doing.
Arguably what I should have done is try a regular IPI first, wait a few
seconds, then NMI IPI.
A couple of problems with that. Firstly it probably avoids this issue
you hit almost all the time, so it won't get fixed. So when we really
need the NMI IPI in the field, it'll still be riddled with deadlocks.
Secondly, sending the IPI first in theory can be more intrusive to the
state that we want to debug. It uses the currently running stack, paca
save areas, ec. NMI IPI uses its own stack and save regions so it's a
little more isolated. Maybe this is only a small advantage but I'd like
to have it if we can.
Thanks,
Nick
^ permalink raw reply
* [PATCH v2] powerpc/mm: ptdump: Add missing include <asm/hvcall.h>
From: YueHaibing @ 2020-04-03 6:47 UTC (permalink / raw)
To: mpe, benh, paulus, gregkh, christophe.leroy, allison, armijn,
tglx, aneesh.kumar, yuehaibing
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <20200402135828.37308-1-yuehaibing@huawei.com>
while PPC_PSERIES is not set, gcc build fails:
arch/powerpc/mm/ptdump/hashpagetable.c: In function ‘pseries_find’:
arch/powerpc/mm/ptdump/hashpagetable.c:262:18: error: ‘H_SUCCESS’ undeclared (first use in this function); did you mean ‘FL_ACCESS’?
if (lpar_rc != H_SUCCESS)
^~~~~~~~~
FL_ACCESS
If PPC_PSERIES is not set, <asm/plpar_wrappers.h> does not
include <asm/hvcall.h>, which leads to this failure.
Add missing include file <asm/hvcall.h> in hashpagetable.c to fix this.
Reported-by: Hulk Robot <hulkci@huawei.com>
Fixes: 65e701b2d2a8 ("powerpc/ptdump: drop non vital #ifdefs")
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
v2: Add hvcall.h instead of vio.h, also rework patch log
---
arch/powerpc/mm/ptdump/hashpagetable.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/mm/ptdump/hashpagetable.c b/arch/powerpc/mm/ptdump/hashpagetable.c
index b6ed9578382f..3e7e6206688c 100644
--- a/arch/powerpc/mm/ptdump/hashpagetable.c
+++ b/arch/powerpc/mm/ptdump/hashpagetable.c
@@ -20,6 +20,7 @@
#include <asm/page.h>
#include <asm/pgalloc.h>
#include <asm/plpar_wrappers.h>
+#include <asm/hvcall.h>
#include <linux/memblock.h>
#include <asm/firmware.h>
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v3 0/5] mm: Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA
From: Baoquan He @ 2020-04-03 7:09 UTC (permalink / raw)
To: Hoan Tran
Cc: mmorana, Catalin Marinas, Heiko Carstens, Michal Hocko,
open list:MEMORY MANAGEMENT, Paul Mackerras, H. Peter Anvin,
sparclinux, Alexander Duyck, linux-s390, x86, Mike Rapoport,
Christian Borntraeger, Ingo Molnar, Vlastimil Babka,
Pavel Tatashin, lho, Vasily Gorbik, Will Deacon, Borislav Petkov,
Thomas Gleixner, linux-arm-kernel, Oscar Salvador, linux-kernel,
Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <cc717f09-2b18-b242-e438-3ec730c5dac0@os.amperecomputing.com>
On 04/02/20 at 09:46pm, Hoan Tran wrote:
> Hi All,
>
> On 3/31/20 7:31 AM, Baoquan He wrote:
> > On 03/31/20 at 04:21pm, Michal Hocko wrote:
> > > On Tue 31-03-20 22:03:32, Baoquan He wrote:
> > > > Hi Michal,
> > > >
> > > > On 03/31/20 at 10:55am, Michal Hocko wrote:
> > > > > On Tue 31-03-20 11:14:23, Mike Rapoport wrote:
> > > > > > Maybe I mis-read the code, but I don't see how this could happen. In the
> > > > > > HAVE_MEMBLOCK_NODE_MAP=y case, free_area_init_node() calls
> > > > > > calculate_node_totalpages() that ensures that node->node_zones are entirely
> > > > > > within the node because this is checked in zone_spanned_pages_in_node().
> > > > >
> > > > > zone_spanned_pages_in_node does chech the zone boundaries are within the
> > > > > node boundaries. But that doesn't really tell anything about other
> > > > > potential zones interleaving with the physical memory range.
> > > > > zone->spanned_pages simply gives the physical range for the zone
> > > > > including holes. Interleaving nodes are essentially a hole
> > > > > (__absent_pages_in_range is going to skip those).
> > > > >
> > > > > That means that when free_area_init_core simply goes over the whole
> > > > > physical zone range including holes and that is why we need to check
> > > > > both for physical and logical holes (aka other nodes).
> > > > >
> > > > > The life would be so much easier if the whole thing would simply iterate
> > > > > over memblocks...
> > > >
> > > > The memblock iterating sounds a great idea. I tried with putting the
> > > > memblock iterating in the upper layer, memmap_init(), which is used for
> > > > boot mem only anyway. Do you think it's doable and OK? It yes, I can
> > > > work out a formal patch to make this simpler as you said. The draft code
> > > > is as below. Like this it uses the existing code and involves little change.
> > >
> > > Doing this would be a step in the right direction! I haven't checked the
> > > code very closely though. The below sounds way too simple to be truth I
> > > am afraid. First for_each_mem_pfn_range is available only for
> > > CONFIG_HAVE_MEMBLOCK_NODE_MAP (which is one of the reasons why I keep
> > > saying that I really hate that being conditional). Also I haven't really
> > > checked the deferred initialization path - I have a very vague
> > > recollection that it has been converted to the memblock api but I have
> > > happilly dropped all that memory.
> >
> > Thanks for your quick response and pointing out the rest suspect aspects,
> > I will investigate what you mentioned, see if they impact.
>
> I would like to check if we still move on with my patch to remove
> CONFIG_NODES_SPAN_OTHER_NODES and have another patch on top it?
I think we would like to replace CONFIG_NODES_SPAN_OTHER_NODES with
CONFIG_NUMA, and just let UMA return 0 as node id, as Michal replied in
another mail. Anyway, your patch 2~5 are still needed to sit on top of
the change of this new plan.
^ permalink raw reply
* Re: [PATCH v11 3/8] powerpc/perf: consolidate read_user_stack_32
From: Nicholas Piggin @ 2020-04-03 7:13 UTC (permalink / raw)
To: Michal Suchánek
Cc: Mark Rutland, Gustavo Luiz Duarte, Alexander Shishkin,
Sebastian Andrzej Siewior, linux-kernel, Paul Mackerras,
Jiri Olsa, Rob Herring, Michael Neuling, Eric Richter,
Masahiro Yamada, Nayna Jain, Peter Zijlstra, Ingo Molnar,
Hari Bathini, Jordan Niethe, Valentin Schneider, Arnd Bergmann,
Arnaldo Carvalho de Melo, Alexander Viro, Jonathan Cameron,
Namhyung Kim, Thomas Gleixner, Andy Shevchenko, Allison Randal,
Greg Kroah-Hartman, Claudio Carvalho, Mauro Carvalho Chehab,
Eric W. Biederman, linux-fsdevel, linuxppc-dev, David S. Miller,
Thiago Jung Bauermann
In-Reply-To: <20200324193833.GH25468@kitsune.suse.cz>
Michal Suchánek's on March 25, 2020 5:38 am:
> On Tue, Mar 24, 2020 at 06:48:20PM +1000, Nicholas Piggin wrote:
>> Michal Suchanek's on March 19, 2020 10:19 pm:
>> > There are two almost identical copies for 32bit and 64bit.
>> >
>> > The function is used only in 32bit code which will be split out in next
>> > patch so consolidate to one function.
>> >
>> > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
>> > Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> > ---
>> > v6: new patch
>> > v8: move the consolidated function out of the ifdef block.
>> > v11: rebase on top of def0bfdbd603
>> > ---
>> > arch/powerpc/perf/callchain.c | 48 +++++++++++++++++------------------
>> > 1 file changed, 24 insertions(+), 24 deletions(-)
>> >
>> > diff --git a/arch/powerpc/perf/callchain.c b/arch/powerpc/perf/callchain.c
>> > index cbc251981209..c9a78c6e4361 100644
>> > --- a/arch/powerpc/perf/callchain.c
>> > +++ b/arch/powerpc/perf/callchain.c
>> > @@ -161,18 +161,6 @@ static int read_user_stack_64(unsigned long __user *ptr, unsigned long *ret)
>> > return read_user_stack_slow(ptr, ret, 8);
>> > }
>> >
>> > -static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret)
>> > -{
>> > - if ((unsigned long)ptr > TASK_SIZE - sizeof(unsigned int) ||
>> > - ((unsigned long)ptr & 3))
>> > - return -EFAULT;
>> > -
>> > - if (!probe_user_read(ret, ptr, sizeof(*ret)))
>> > - return 0;
>> > -
>> > - return read_user_stack_slow(ptr, ret, 4);
>> > -}
>> > -
>> > static inline int valid_user_sp(unsigned long sp, int is_64)
>> > {
>> > if (!sp || (sp & 7) || sp > (is_64 ? TASK_SIZE : 0x100000000UL) - 32)
>> > @@ -277,19 +265,9 @@ static void perf_callchain_user_64(struct perf_callchain_entry_ctx *entry,
>> > }
>> >
>> > #else /* CONFIG_PPC64 */
>> > -/*
>> > - * On 32-bit we just access the address and let hash_page create a
>> > - * HPTE if necessary, so there is no need to fall back to reading
>> > - * the page tables. Since this is called at interrupt level,
>> > - * do_page_fault() won't treat a DSI as a page fault.
>> > - */
>> > -static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret)
>> > +static int read_user_stack_slow(void __user *ptr, void *buf, int nb)
>> > {
>> > - if ((unsigned long)ptr > TASK_SIZE - sizeof(unsigned int) ||
>> > - ((unsigned long)ptr & 3))
>> > - return -EFAULT;
>> > -
>> > - return probe_user_read(ret, ptr, sizeof(*ret));
>> > + return 0;
>> > }
>> >
>> > static inline void perf_callchain_user_64(struct perf_callchain_entry_ctx *entry,
>> > @@ -312,6 +290,28 @@ static inline int valid_user_sp(unsigned long sp, int is_64)
>> >
>> > #endif /* CONFIG_PPC64 */
>> >
>> > +/*
>> > + * On 32-bit we just access the address and let hash_page create a
>> > + * HPTE if necessary, so there is no need to fall back to reading
>> > + * the page tables. Since this is called at interrupt level,
>> > + * do_page_fault() won't treat a DSI as a page fault.
>> > + */
>>
>> The comment is actually probably better to stay in the 32-bit
>> read_user_stack_slow implementation. Is that function defined
>> on 32-bit purely so that you can use IS_ENABLED()? In that case
> It documents the IS_ENABLED() and that's where it is. The 32bit
> definition is only a technical detail.
Sorry for the late reply, busy trying to fix bugs in the C rewrite
series. I don't think it is the right place, it should be in the
ppc32 implementation detail. ppc64 has an equivalent comment at the
top of its read_user_stack functions.
>> I would prefer to put a BUG() there which makes it self documenting.
> Which will cause checkpatch complaints about introducing new BUG() which
> is frowned on.
It's fine in this case, that warning is about not introducing
runtime bugs, but this wouldn't be.
But... I actually don't like adding read_user_stack_slow on 32-bit
and especially not just to make IS_ENABLED work.
IMO this would be better if you really want to consolidate it
---
diff --git a/arch/powerpc/perf/callchain.c b/arch/powerpc/perf/callchain.c
index cbc251981209..ca3a599b3f54 100644
--- a/arch/powerpc/perf/callchain.c
+++ b/arch/powerpc/perf/callchain.c
@@ -108,7 +108,7 @@ perf_callchain_kernel(struct perf_callchain_entry_ctx *entry, struct pt_regs *re
* interrupt context, so if the access faults, we read the page tables
* to find which page (if any) is mapped and access it directly.
*/
-static int read_user_stack_slow(void __user *ptr, void *buf, int nb)
+static int read_user_stack_slow(const void __user *ptr, void *buf, int nb)
{
int ret = -EFAULT;
pgd_t *pgdir;
@@ -149,28 +149,21 @@ static int read_user_stack_slow(void __user *ptr, void *buf, int nb)
return ret;
}
-static int read_user_stack_64(unsigned long __user *ptr, unsigned long *ret)
+static int __read_user_stack(const void __user *ptr, void *ret, size_t size)
{
- if ((unsigned long)ptr > TASK_SIZE - sizeof(unsigned long) ||
- ((unsigned long)ptr & 7))
+ if ((unsigned long)ptr > TASK_SIZE - size ||
+ ((unsigned long)ptr & (size - 1)))
return -EFAULT;
- if (!probe_user_read(ret, ptr, sizeof(*ret)))
+ if (!probe_user_read(ret, ptr, size))
return 0;
- return read_user_stack_slow(ptr, ret, 8);
+ return read_user_stack_slow(ptr, ret, size);
}
-static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret)
+static int read_user_stack_64(unsigned long __user *ptr, unsigned long *ret)
{
- if ((unsigned long)ptr > TASK_SIZE - sizeof(unsigned int) ||
- ((unsigned long)ptr & 3))
- return -EFAULT;
-
- if (!probe_user_read(ret, ptr, sizeof(*ret)))
- return 0;
-
- return read_user_stack_slow(ptr, ret, 4);
+ return __read_user_stack(ptr, ret, sizeof(*ret));
}
static inline int valid_user_sp(unsigned long sp, int is_64)
@@ -283,13 +276,13 @@ static void perf_callchain_user_64(struct perf_callchain_entry_ctx *entry,
* the page tables. Since this is called at interrupt level,
* do_page_fault() won't treat a DSI as a page fault.
*/
-static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret)
+static int __read_user_stack(const void __user *ptr, void *ret, size_t size)
{
- if ((unsigned long)ptr > TASK_SIZE - sizeof(unsigned int) ||
- ((unsigned long)ptr & 3))
+ if ((unsigned long)ptr > TASK_SIZE - size ||
+ ((unsigned long)ptr & (size - 1)))
return -EFAULT;
- return probe_user_read(ret, ptr, sizeof(*ret));
+ return probe_user_read(ret, ptr, size);
}
static inline void perf_callchain_user_64(struct perf_callchain_entry_ctx *entry,
@@ -312,6 +305,11 @@ static inline int valid_user_sp(unsigned long sp, int is_64)
#endif /* CONFIG_PPC64 */
+static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret)
+{
+ return __read_user_stack(ptr, ret, sizeof(*ret));
+}
+
/*
* Layout for non-RT signal frames
*/
^ permalink raw reply related
* [PATCH] tty: hvc: remove hvcs_driver_string
From: Jason Yan @ 2020-04-03 7:13 UTC (permalink / raw)
To: gregkh, jslaby, linuxppc-dev; +Cc: Jason Yan
No users of hvcs_driver_string, remove it. This fixes the following gcc
warning:
drivers/tty/hvc/hvcs.c:199:19: warning: ‘hvcs_driver_string’ defined but
not used [-Wunused-const-variable=]
static const char hvcs_driver_string[]
^~~~~~~~~~~~~~~~~~
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
drivers/tty/hvc/hvcs.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/tty/hvc/hvcs.c b/drivers/tty/hvc/hvcs.c
index ee0604cd9c6b..55105ac38f89 100644
--- a/drivers/tty/hvc/hvcs.c
+++ b/drivers/tty/hvc/hvcs.c
@@ -196,8 +196,6 @@ module_param(hvcs_parm_num_devs, int, 0);
static const char hvcs_driver_name[] = "hvcs";
static const char hvcs_device_node[] = "hvcs";
-static const char hvcs_driver_string[]
- = "IBM hvcs (Hypervisor Virtual Console Server) Driver";
/* Status of partner info rescan triggered via sysfs. */
static int hvcs_rescan_status;
--
2.17.2
^ permalink raw reply related
* Re: [RFC PATCH 3/4] powerpc ppc-opcode: move ppc instuction encoding from test_emulate_step
From: Balamuruhan S @ 2020-04-03 7:14 UTC (permalink / raw)
To: Naveen N. Rao, Michael Ellerman
Cc: jniethe5, linuxppc-dev, sandipan, paulus, ravi.bangoria
In-Reply-To: <1585810752.gtbei2f2gy.naveen@linux.ibm.com>
On Thu, 2020-04-02 at 12:34 +0530, Naveen N. Rao wrote:
> Michael Ellerman wrote:
> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
> > > Balamuruhan S wrote:
> > > > Few ppc instructions are encoded in test_emulate_step.c, consolidate
> > > > them to
> > > > ppc-opcode.h, fix redefintion errors in bpf_jit caused due to this
> > > > consolidation.
> > > > Reuse the macros from ppc-opcode.h
> > ...
> > > > diff --git a/arch/powerpc/net/bpf_jit32.h
> > > > b/arch/powerpc/net/bpf_jit32.h
> > > > index 4ec2a9f14f84..8a9f16a7262e 100644
> > > > --- a/arch/powerpc/net/bpf_jit32.h
> > > > +++ b/arch/powerpc/net/bpf_jit32.h
> > > > @@ -76,13 +76,13 @@ DECLARE_LOAD_FUNC(sk_load_byte_msh);
> > > > else { PPC_ADDIS(r, base, IMM_HA(i));
> > > > \
> > > > PPC_LBZ(r, r, IMM_L(i)); } } while(0)
> > > >
> > > > -#define PPC_LD_OFFS(r, base, i) do { if ((i) < 32768) PPC_LD(r, base,
> > > > i); \
> > > > +#define _OFFS(r, base, i) do { if ((i) < 32768) EMIT(PPC_ENCODE_LD(r,
> > > > base, i)); \
> > > ^^^^^
> > > Should be PPC_LD_OFFS. For the next version, please also build ppc32 and
> > > booke codebase to confirm that your changes in those areas are fine.
> > >
> > > PPC_ENCODE_* also looks quite verbose, so perhaps PPC_ENC_* might be
> > > better. Otherwise, this patchset looks good to me and should help reuse
> > > some of those macros, especially from the eBPF codebase.
> > >
> > > Michael,
> > > Can you let us know if this looks ok to you? Based on your feedback, we
> > > will also update the eBPF codebase.
> >
> > I didn't really like the first patch which does the mass renaming. It
> > creates a huge amount of churn.
sorry for that.
> >
> > I think I'd be happier if this series just did what it needs, and then
> > maybe at the end there's a patch to update all the existing names, which
> > I may or may not take.
>
> Ok.
I will work on it.
>
> > As far as the naming, currently we have:
> >
> > PPC_INST_FOO - just the opcode
> >
> > PPC_FOO(x) - macro to encode the opcode with x and (usually) also emit a
> > .long and stringify.
> >
> > And you need an in-between that gives you the full instruction but
> > without the .long and stringify, right?
>
> Yes.
>
> > So how about PPC_RAW_FOO() for just the numeric value, without the .long
> > and stringify.
>
> Sure, thanks for the feedback -- that makes sense.
Thanks for the feedback.
>
> > We also seem to have a lot of PPC_INST_FOO's that are only ever used in
> > the PPC_INST macro. I'm inclined to fold those into the PPC_INST macro,
> > to avoid people accidentally using the PPC_INST version when they don't
> > mean to. But that's a separate issue.
>
> Good point -- I do see many uses of PPC_INST_FOO that can be replaced
> with PPC_RAW_FOO once we introduce that. We will take a stab at doing
> this cleanup as a separate patch at the end.
Will make the changes as suggested.
-- Bala
>
>
> Thanks,
> Naveen
>
^ permalink raw reply
* Re: [PATCH v11 5/8] powerpc/64: make buildable without CONFIG_COMPAT
From: Nicholas Piggin @ 2020-04-03 7:16 UTC (permalink / raw)
To: Michal Suchánek
Cc: Mark Rutland, Gustavo Luiz Duarte, Alexander Shishkin,
Jordan Niethe, Sebastian Andrzej Siewior, linux-kernel,
Paul Mackerras, Jiri Olsa, Rob Herring, Michael Neuling,
Eric Richter, Masahiro Yamada, Nayna Jain, Peter Zijlstra,
Ingo Molnar, Hari Bathini, Valentin Schneider, Arnd Bergmann,
Arnaldo Carvalho de Melo, Alexander Viro, Jonathan Cameron,
Namhyung Kim, Thomas Gleixner, Andy Shevchenko, Allison Randal,
Greg Kroah-Hartman, Claudio Carvalho, Mauro Carvalho Chehab,
Eric W. Biederman, linux-fsdevel, linuxppc-dev, David S. Miller,
Thiago Jung Bauermann
In-Reply-To: <20200324193055.GG25468@kitsune.suse.cz>
Michal Suchánek's on March 25, 2020 5:30 am:
> On Tue, Mar 24, 2020 at 06:54:20PM +1000, Nicholas Piggin wrote:
>> Michal Suchanek's on March 19, 2020 10:19 pm:
>> > diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
>> > index 4b0152108f61..a264989626fd 100644
>> > --- a/arch/powerpc/kernel/signal.c
>> > +++ b/arch/powerpc/kernel/signal.c
>> > @@ -247,7 +247,6 @@ static void do_signal(struct task_struct *tsk)
>> > sigset_t *oldset = sigmask_to_save();
>> > struct ksignal ksig = { .sig = 0 };
>> > int ret;
>> > - int is32 = is_32bit_task();
>> >
>> > BUG_ON(tsk != current);
>> >
>> > @@ -277,7 +276,7 @@ static void do_signal(struct task_struct *tsk)
>> >
>> > rseq_signal_deliver(&ksig, tsk->thread.regs);
>> >
>> > - if (is32) {
>> > + if (is_32bit_task()) {
>> > if (ksig.ka.sa.sa_flags & SA_SIGINFO)
>> > ret = handle_rt_signal32(&ksig, oldset, tsk);
>> > else
>>
>> Unnecessary?
>>
>> > diff --git a/arch/powerpc/kernel/syscall_64.c b/arch/powerpc/kernel/syscall_64.c
>> > index 87d95b455b83..2dcbfe38f5ac 100644
>> > --- a/arch/powerpc/kernel/syscall_64.c
>> > +++ b/arch/powerpc/kernel/syscall_64.c
>> > @@ -24,7 +24,6 @@ notrace long system_call_exception(long r3, long r4, long r5,
>> > long r6, long r7, long r8,
>> > unsigned long r0, struct pt_regs *regs)
>> > {
>> > - unsigned long ti_flags;
>> > syscall_fn f;
>> >
>> > if (IS_ENABLED(CONFIG_PPC_IRQ_SOFT_MASK_DEBUG))
>> > @@ -68,8 +67,7 @@ notrace long system_call_exception(long r3, long r4, long r5,
>> >
>> > local_irq_enable();
>> >
>> > - ti_flags = current_thread_info()->flags;
>> > - if (unlikely(ti_flags & _TIF_SYSCALL_DOTRACE)) {
>> > + if (unlikely(current_thread_info()->flags & _TIF_SYSCALL_DOTRACE)) {
>> > /*
>> > * We use the return value of do_syscall_trace_enter() as the
>> > * syscall number. If the syscall was rejected for any reason
>> > @@ -94,7 +92,7 @@ notrace long system_call_exception(long r3, long r4, long r5,
>> > /* May be faster to do array_index_nospec? */
>> > barrier_nospec();
>> >
>> > - if (unlikely(ti_flags & _TIF_32BIT)) {
>> > + if (unlikely(is_32bit_task())) {
>>
>> Problem is, does this allow the load of ti_flags to be used for both
>> tests, or does test_bit make it re-load?
>>
>> This could maybe be fixed by testing if(IS_ENABLED(CONFIG_COMPAT) &&
> Both points already discussed here:
Agh, I'm hopeless.
I don't think it really resolves this issue. But probably don't have time
to look at generated asm, and might never because it won't really hit
LE unless we add a 32-bit ABI. It's pretty minor though either way.
Sorry for being difficult, I really do like your patches :)
Thanks,
Nick
^ permalink raw reply
* [PATCH v2 2/5] uaccess: Selectively open read or write user access
From: Christophe Leroy @ 2020-04-03 7:20 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, airlied,
daniel, torvalds, viro, akpm, keescook, hpa
Cc: linux-arch, linux-mm, intel-gfx, linuxppc-dev, linux-kernel
In-Reply-To: <36e43241c7f043a24b5069e78c6a7edd11043be5.1585898438.git.christophe.leroy@c-s.fr>
When opening user access to only perform reads, only open read access.
When opening user access to only perform writes, only open write
access.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
Reviewed-by: Kees Cook <keescook@chromium.org>
---
v2: Fixed a mismatched use of _read_ and _write_ in compat_get_bitmap()
and compat_put_bitmap()
---
fs/readdir.c | 12 ++++++------
kernel/compat.c | 12 ++++++------
kernel/exit.c | 12 ++++++------
lib/strncpy_from_user.c | 4 ++--
lib/strnlen_user.c | 4 ++--
lib/usercopy.c | 6 +++---
6 files changed, 25 insertions(+), 25 deletions(-)
diff --git a/fs/readdir.c b/fs/readdir.c
index de2eceffdee8..ed6aaad451aa 100644
--- a/fs/readdir.c
+++ b/fs/readdir.c
@@ -242,7 +242,7 @@ static int filldir(struct dir_context *ctx, const char *name, int namlen,
return -EINTR;
dirent = buf->current_dir;
prev = (void __user *) dirent - prev_reclen;
- if (!user_access_begin(prev, reclen + prev_reclen))
+ if (!user_write_access_begin(prev, reclen + prev_reclen))
goto efault;
/* This might be 'dirent->d_off', but if so it will get overwritten */
@@ -251,14 +251,14 @@ static int filldir(struct dir_context *ctx, const char *name, int namlen,
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();
+ user_write_access_end();
buf->current_dir = (void __user *)dirent + reclen;
buf->prev_reclen = reclen;
buf->count -= reclen;
return 0;
efault_end:
- user_access_end();
+ user_write_access_end();
efault:
buf->error = -EFAULT;
return -EFAULT;
@@ -327,7 +327,7 @@ static int filldir64(struct dir_context *ctx, const char *name, int namlen,
return -EINTR;
dirent = buf->current_dir;
prev = (void __user *)dirent - prev_reclen;
- if (!user_access_begin(prev, reclen + prev_reclen))
+ if (!user_write_access_begin(prev, reclen + prev_reclen))
goto efault;
/* This might be 'dirent->d_off', but if so it will get overwritten */
@@ -336,7 +336,7 @@ static int filldir64(struct dir_context *ctx, const char *name, int namlen,
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();
+ user_write_access_end();
buf->prev_reclen = reclen;
buf->current_dir = (void __user *)dirent + reclen;
@@ -344,7 +344,7 @@ static int filldir64(struct dir_context *ctx, const char *name, int namlen,
return 0;
efault_end:
- user_access_end();
+ user_write_access_end();
efault:
buf->error = -EFAULT;
return -EFAULT;
diff --git a/kernel/compat.c b/kernel/compat.c
index 843dd17e6078..b8d2800bb4b7 100644
--- a/kernel/compat.c
+++ b/kernel/compat.c
@@ -199,7 +199,7 @@ long compat_get_bitmap(unsigned long *mask, const compat_ulong_t __user *umask,
bitmap_size = ALIGN(bitmap_size, BITS_PER_COMPAT_LONG);
nr_compat_longs = BITS_TO_COMPAT_LONGS(bitmap_size);
- if (!user_access_begin(umask, bitmap_size / 8))
+ if (!user_read_access_begin(umask, bitmap_size / 8))
return -EFAULT;
while (nr_compat_longs > 1) {
@@ -211,11 +211,11 @@ long compat_get_bitmap(unsigned long *mask, const compat_ulong_t __user *umask,
}
if (nr_compat_longs)
unsafe_get_user(*mask, umask++, Efault);
- user_access_end();
+ user_read_access_end();
return 0;
Efault:
- user_access_end();
+ user_read_access_end();
return -EFAULT;
}
@@ -228,7 +228,7 @@ long compat_put_bitmap(compat_ulong_t __user *umask, unsigned long *mask,
bitmap_size = ALIGN(bitmap_size, BITS_PER_COMPAT_LONG);
nr_compat_longs = BITS_TO_COMPAT_LONGS(bitmap_size);
- if (!user_access_begin(umask, bitmap_size / 8))
+ if (!user_write_access_begin(umask, bitmap_size / 8))
return -EFAULT;
while (nr_compat_longs > 1) {
@@ -239,10 +239,10 @@ long compat_put_bitmap(compat_ulong_t __user *umask, unsigned long *mask,
}
if (nr_compat_longs)
unsafe_put_user((compat_ulong_t)*mask, umask++, Efault);
- user_access_end();
+ user_write_access_end();
return 0;
Efault:
- user_access_end();
+ user_write_access_end();
return -EFAULT;
}
diff --git a/kernel/exit.c b/kernel/exit.c
index d70d47159640..61b2f7a85079 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1555,7 +1555,7 @@ SYSCALL_DEFINE5(waitid, int, which, pid_t, upid, struct siginfo __user *,
if (!infop)
return err;
- if (!user_access_begin(infop, sizeof(*infop)))
+ if (!user_write_access_begin(infop, sizeof(*infop)))
return -EFAULT;
unsafe_put_user(signo, &infop->si_signo, Efault);
@@ -1564,10 +1564,10 @@ SYSCALL_DEFINE5(waitid, int, which, pid_t, upid, struct siginfo __user *,
unsafe_put_user(info.pid, &infop->si_pid, Efault);
unsafe_put_user(info.uid, &infop->si_uid, Efault);
unsafe_put_user(info.status, &infop->si_status, Efault);
- user_access_end();
+ user_write_access_end();
return err;
Efault:
- user_access_end();
+ user_write_access_end();
return -EFAULT;
}
@@ -1682,7 +1682,7 @@ COMPAT_SYSCALL_DEFINE5(waitid,
if (!infop)
return err;
- if (!user_access_begin(infop, sizeof(*infop)))
+ if (!user_write_access_begin(infop, sizeof(*infop)))
return -EFAULT;
unsafe_put_user(signo, &infop->si_signo, Efault);
@@ -1691,10 +1691,10 @@ COMPAT_SYSCALL_DEFINE5(waitid,
unsafe_put_user(info.pid, &infop->si_pid, Efault);
unsafe_put_user(info.uid, &infop->si_uid, Efault);
unsafe_put_user(info.status, &infop->si_status, Efault);
- user_access_end();
+ user_write_access_end();
return err;
Efault:
- user_access_end();
+ user_write_access_end();
return -EFAULT;
}
#endif
diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c
index 706020b06617..b90ec550183a 100644
--- a/lib/strncpy_from_user.c
+++ b/lib/strncpy_from_user.c
@@ -116,9 +116,9 @@ long strncpy_from_user(char *dst, const char __user *src, long count)
kasan_check_write(dst, count);
check_object_size(dst, count, false);
- if (user_access_begin(src, max)) {
+ if (user_read_access_begin(src, max)) {
retval = do_strncpy_from_user(dst, src, count, max);
- user_access_end();
+ user_read_access_end();
return retval;
}
}
diff --git a/lib/strnlen_user.c b/lib/strnlen_user.c
index 41670d4a5816..1616710b8a82 100644
--- a/lib/strnlen_user.c
+++ b/lib/strnlen_user.c
@@ -109,9 +109,9 @@ long strnlen_user(const char __user *str, long count)
if (max > count)
max = count;
- if (user_access_begin(str, max)) {
+ if (user_read_access_begin(str, max)) {
retval = do_strnlen_user(str, count, max);
- user_access_end();
+ user_read_access_end();
return retval;
}
}
diff --git a/lib/usercopy.c b/lib/usercopy.c
index cbb4d9ec00f2..ca2a697a2061 100644
--- a/lib/usercopy.c
+++ b/lib/usercopy.c
@@ -58,7 +58,7 @@ int check_zeroed_user(const void __user *from, size_t size)
from -= align;
size += align;
- if (!user_access_begin(from, size))
+ if (!user_read_access_begin(from, size))
return -EFAULT;
unsafe_get_user(val, (unsigned long __user *) from, err_fault);
@@ -79,10 +79,10 @@ int check_zeroed_user(const void __user *from, size_t size)
val &= aligned_byte_mask(size);
done:
- user_access_end();
+ user_read_access_end();
return (val == 0);
err_fault:
- user_access_end();
+ user_read_access_end();
return -EFAULT;
}
EXPORT_SYMBOL(check_zeroed_user);
--
2.25.0
^ permalink raw reply related
* [PATCH v2 4/5] powerpc/uaccess: Implement user_read_access_begin and user_write_access_begin
From: Christophe Leroy @ 2020-04-03 7:20 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, airlied,
daniel, torvalds, viro, akpm, keescook, hpa
Cc: linux-arch, linux-mm, intel-gfx, linuxppc-dev, linux-kernel
In-Reply-To: <36e43241c7f043a24b5069e78c6a7edd11043be5.1585898438.git.christophe.leroy@c-s.fr>
Add support for selective read or write user access with
user_read_access_begin/end and user_write_access_begin/end.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
Reviewed-by: Kees Cook <keescook@chromium.org>
---
v2: no change
---
arch/powerpc/include/asm/book3s/32/kup.h | 4 ++--
arch/powerpc/include/asm/kup.h | 14 +++++++++++++-
arch/powerpc/include/asm/uaccess.h | 22 ++++++++++++++++++++++
3 files changed, 37 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/32/kup.h b/arch/powerpc/include/asm/book3s/32/kup.h
index 3c0ba22dc360..1617e73bee30 100644
--- a/arch/powerpc/include/asm/book3s/32/kup.h
+++ b/arch/powerpc/include/asm/book3s/32/kup.h
@@ -108,7 +108,7 @@ static __always_inline void allow_user_access(void __user *to, const void __user
u32 addr, end;
BUILD_BUG_ON(!__builtin_constant_p(dir));
- BUILD_BUG_ON(dir == KUAP_CURRENT);
+ BUILD_BUG_ON(dir & ~KUAP_READ_WRITE);
if (!(dir & KUAP_WRITE))
return;
@@ -131,7 +131,7 @@ static __always_inline void prevent_user_access(void __user *to, const void __us
BUILD_BUG_ON(!__builtin_constant_p(dir));
- if (dir == KUAP_CURRENT) {
+ if (dir & KUAP_CURRENT_WRITE) {
u32 kuap = current->thread.kuap;
if (unlikely(!kuap))
diff --git a/arch/powerpc/include/asm/kup.h b/arch/powerpc/include/asm/kup.h
index 92bcd1a26d73..c745ee41ad66 100644
--- a/arch/powerpc/include/asm/kup.h
+++ b/arch/powerpc/include/asm/kup.h
@@ -10,7 +10,9 @@
* Use the current saved situation instead of the to/from/size params.
* Used on book3s/32
*/
-#define KUAP_CURRENT 4
+#define KUAP_CURRENT_READ 4
+#define KUAP_CURRENT_WRITE 8
+#define KUAP_CURRENT (KUAP_CURRENT_READ | KUAP_CURRENT_WRITE)
#ifdef CONFIG_PPC64
#include <asm/book3s/64/kup-radix.h>
@@ -101,6 +103,16 @@ static inline void prevent_current_access_user(void)
prevent_user_access(NULL, NULL, ~0UL, KUAP_CURRENT);
}
+static inline void prevent_current_read_from_user(void)
+{
+ prevent_user_access(NULL, NULL, ~0UL, KUAP_CURRENT_READ);
+}
+
+static inline void prevent_current_write_to_user(void)
+{
+ prevent_user_access(NULL, NULL, ~0UL, KUAP_CURRENT_WRITE);
+}
+
#endif /* !__ASSEMBLY__ */
#endif /* _ASM_POWERPC_KUAP_H_ */
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 2f500debae21..4427d419eb1d 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -468,6 +468,28 @@ static __must_check inline bool user_access_begin(const void __user *ptr, size_t
#define user_access_save prevent_user_access_return
#define user_access_restore restore_user_access
+static __must_check inline bool
+user_read_access_begin(const void __user *ptr, size_t len)
+{
+ if (unlikely(!access_ok(ptr, len)))
+ return false;
+ allow_read_from_user(ptr, len);
+ return true;
+}
+#define user_read_access_begin user_read_access_begin
+#define user_read_access_end prevent_current_read_from_user
+
+static __must_check inline bool
+user_write_access_begin(const void __user *ptr, size_t len)
+{
+ if (unlikely(!access_ok(ptr, len)))
+ return false;
+ allow_write_to_user((void __user *)ptr, len);
+ return true;
+}
+#define user_write_access_begin user_write_access_begin
+#define user_write_access_end prevent_current_write_to_user
+
#define unsafe_op_wrap(op, err) do { if (unlikely(op)) goto err; } while (0)
#define unsafe_get_user(x, p, e) unsafe_op_wrap(__get_user_allowed(x, p), e)
#define unsafe_put_user(x, p, e) unsafe_op_wrap(__put_user_allowed(x, p), e)
--
2.25.0
^ permalink raw reply related
* [PATCH v2 3/5] drm/i915/gem: Replace user_access_begin by user_write_access_begin
From: Christophe Leroy @ 2020-04-03 7:20 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, airlied,
daniel, torvalds, viro, akpm, keescook, hpa
Cc: linux-arch, linux-mm, intel-gfx, linuxppc-dev, linux-kernel
In-Reply-To: <36e43241c7f043a24b5069e78c6a7edd11043be5.1585898438.git.christophe.leroy@c-s.fr>
When i915_gem_execbuffer2_ioctl() is using user_access_begin(),
that's only to perform unsafe_put_user() so use
user_write_access_begin() in order to only open write access.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
Reviewed-by: Kees Cook <keescook@chromium.org>
---
v2: Rebased (one part of the patch flies away)
---
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 36d069504836..b4c903308590 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -2794,7 +2794,8 @@ i915_gem_execbuffer2_ioctl(struct drm_device *dev, void *data,
* And this range already got effectively checked earlier
* when we did the "copy_from_user()" above.
*/
- if (!user_access_begin(user_exec_list, count * sizeof(*user_exec_list)))
+ if (!user_write_access_begin(user_exec_list,
+ count * sizeof(*user_exec_list)))
goto end;
for (i = 0; i < args->buffer_count; i++) {
@@ -2808,7 +2809,7 @@ i915_gem_execbuffer2_ioctl(struct drm_device *dev, void *data,
end_user);
}
end_user:
- user_access_end();
+ user_write_access_end();
end:;
}
--
2.25.0
^ permalink raw reply related
* [PATCH v2 1/5] uaccess: Add user_read_access_begin/end and user_write_access_begin/end
From: Christophe Leroy @ 2020-04-03 7:20 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, airlied,
daniel, torvalds, viro, akpm, keescook, hpa
Cc: linux-arch, linux-mm, intel-gfx, linuxppc-dev, linux-kernel
Some architectures like powerpc64 have the capability to separate
read access and write access protection.
For get_user() and copy_from_user(), powerpc64 only open read access.
For put_user() and copy_to_user(), powerpc64 only open write access.
But when using unsafe_get_user() or unsafe_put_user(),
user_access_begin open both read and write.
Other architectures like powerpc book3s 32 bits only allow write
access protection. And on this architecture protection is an heavy
operation as it requires locking/unlocking per segment of 256Mbytes.
On those architecture it is therefore desirable to do the unlocking
only for write access. (Note that book3s/32 ranges from very old
powermac from the 90's with powerpc 601 processor, till modern
ADSL boxes with PowerQuicc II processors for instance so it
is still worth considering.)
In order to avoid any risk based of hacking some variable parameters
passed to user_access_begin/end that would allow hacking and
leaving user access open or opening too much, it is preferable to
use dedicated static functions that can't be overridden.
Add a user_read_access_begin and user_read_access_end to only open
read access.
Add a user_write_access_begin and user_write_access_end to only open
write access.
By default, when undefined, those new access helpers default on the
existing user_access_begin and user_access_end.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
Reviewed-by: Kees Cook <keescook@chromium.org>
---
v2: no change in this patch. See each patch for related changes.
v1 at https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=168174
This series is based on the discussion we had in January, see
https://patchwork.ozlabs.org/patch/1227926/ . I tried to
take into account all remarks, especially @hpa 's remark to use
a fixed API on not base the relocking on a magic id returned at
unlocking.
This series is awaited for implementing selective lkdtm test to
test powerpc64 independant read and write protection, see
https://patchwork.ozlabs.org/patch/1231765/
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
include/linux/uaccess.h | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index 67f016010aad..9861c89f93be 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -378,6 +378,14 @@ extern long strnlen_unsafe_user(const void __user *unsafe_addr, long count);
static inline unsigned long user_access_save(void) { return 0UL; }
static inline void user_access_restore(unsigned long flags) { }
#endif
+#ifndef user_write_access_begin
+#define user_write_access_begin user_access_begin
+#define user_write_access_end user_access_end
+#endif
+#ifndef user_read_access_begin
+#define user_read_access_begin user_access_begin
+#define user_read_access_end user_access_end
+#endif
#ifdef CONFIG_HARDENED_USERCOPY
void usercopy_warn(const char *name, const char *detail, bool to_user,
--
2.25.0
^ permalink raw reply related
* [PATCH v2 5/5] uaccess: Rename user_access_begin/end() to user_full_access_begin/end()
From: Christophe Leroy @ 2020-04-03 7:20 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, airlied,
daniel, torvalds, viro, akpm, keescook, hpa
Cc: linux-arch, linux-mm, intel-gfx, linuxppc-dev, linux-kernel
In-Reply-To: <36e43241c7f043a24b5069e78c6a7edd11043be5.1585898438.git.christophe.leroy@c-s.fr>
Now we have user_read_access_begin() and user_write_access_begin()
in addition to user_access_begin().
Make it explicit that user_access_begin() provides both read and
write by renaming it user_full_access_begin(). And the same for
user_access_end() which becomes user_full_access_end().
Done with following command, then hand splitted two too long lines.
sed -i s/user_access_begin/user_full_access_begin/g `git grep -l user_access_begin`
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
v2: New, based on remark from Al Viro.
---
arch/powerpc/include/asm/uaccess.h | 5 +++--
arch/x86/include/asm/futex.h | 4 ++--
arch/x86/include/asm/uaccess.h | 7 ++++---
include/linux/uaccess.h | 8 ++++----
4 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 4427d419eb1d..7fe799e081f2 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -456,14 +456,15 @@ extern long __copy_from_user_flushcache(void *dst, const void __user *src,
extern void memcpy_page_flushcache(char *to, struct page *page, size_t offset,
size_t len);
-static __must_check inline bool user_access_begin(const void __user *ptr, size_t len)
+static __must_check inline bool
+user_full_access_begin(const void __user *ptr, size_t len)
{
if (unlikely(!access_ok(ptr, len)))
return false;
allow_read_write_user((void __user *)ptr, ptr, len);
return true;
}
-#define user_access_begin user_access_begin
+#define user_full_access_begin user_full_access_begin
#define user_access_end prevent_current_access_user
#define user_access_save prevent_user_access_return
#define user_access_restore restore_user_access
diff --git a/arch/x86/include/asm/futex.h b/arch/x86/include/asm/futex.h
index f9c00110a69a..9eefea374bd4 100644
--- a/arch/x86/include/asm/futex.h
+++ b/arch/x86/include/asm/futex.h
@@ -56,7 +56,7 @@ do { \
static __always_inline int arch_futex_atomic_op_inuser(int op, int oparg, int *oval,
u32 __user *uaddr)
{
- if (!user_access_begin(uaddr, sizeof(u32)))
+ if (!user_full_access_begin(uaddr, sizeof(u32)))
return -EFAULT;
switch (op) {
@@ -92,7 +92,7 @@ static inline int futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
{
int ret = 0;
- if (!user_access_begin(uaddr, sizeof(u32)))
+ if (!user_full_access_begin(uaddr, sizeof(u32)))
return -EFAULT;
asm volatile("\n"
"1:\t" LOCK_PREFIX "cmpxchgl %4, %2\n"
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index d8f283b9a569..8776e815f215 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -473,16 +473,17 @@ extern struct movsl_mask {
* The "unsafe" user accesses aren't really "unsafe", but the naming
* is a big fat warning: you have to not only do the access_ok()
* checking before using them, but you have to surround them with the
- * user_access_begin/end() pair.
+ * user_full_access_begin/end() pair.
*/
-static __must_check __always_inline bool user_access_begin(const void __user *ptr, size_t len)
+static __must_check __always_inline bool
+user_full_access_begin(const void __user *ptr, size_t len)
{
if (unlikely(!access_ok(ptr,len)))
return 0;
__uaccess_begin_nospec();
return 1;
}
-#define user_access_begin(a,b) user_access_begin(a,b)
+#define user_full_access_begin(a,b) user_full_access_begin(a,b)
#define user_access_end() __uaccess_end()
#define user_access_save() smap_save()
diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index 9861c89f93be..5be9bc930342 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -368,8 +368,8 @@ extern long strnlen_unsafe_user(const void __user *unsafe_addr, long count);
#define probe_kernel_address(addr, retval) \
probe_kernel_read(&retval, addr, sizeof(retval))
-#ifndef user_access_begin
-#define user_access_begin(ptr,len) access_ok(ptr, len)
+#ifndef user_full_access_begin
+#define user_full_access_begin(ptr,len) access_ok(ptr, len)
#define user_access_end() do { } while (0)
#define unsafe_op_wrap(op, err) do { if (unlikely(op)) goto err; } while (0)
#define unsafe_get_user(x,p,e) unsafe_op_wrap(__get_user(x,p),e)
@@ -379,11 +379,11 @@ static inline unsigned long user_access_save(void) { return 0UL; }
static inline void user_access_restore(unsigned long flags) { }
#endif
#ifndef user_write_access_begin
-#define user_write_access_begin user_access_begin
+#define user_write_access_begin user_full_access_begin
#define user_write_access_end user_access_end
#endif
#ifndef user_read_access_begin
-#define user_read_access_begin user_access_begin
+#define user_read_access_begin user_full_access_begin
#define user_read_access_end user_access_end
#endif
--
2.25.0
^ permalink raw reply related
* Re: [PATCH v11 0/8] Disable compat cruft on ppc64le v11
From: Nicholas Piggin @ 2020-04-03 7:25 UTC (permalink / raw)
To: linuxppc-dev, Michal Suchanek
Cc: Mark Rutland, Gustavo Luiz Duarte, Alexander Shishkin,
Sebastian Andrzej Siewior, linux-kernel, Paul Mackerras,
Jiri Olsa, Rob Herring, Michael Neuling, Eric Richter,
Masahiro Yamada, Nayna Jain, Peter Zijlstra, Ingo Molnar,
Hari Bathini, Jordan Niethe, Valentin Schneider, Arnd Bergmann,
Arnaldo Carvalho de Melo, Alexander Viro, Jonathan Cameron,
Namhyung Kim, Thomas Gleixner, Andy Shevchenko, Allison Randal,
Greg Kroah-Hartman, Claudio Carvalho, Mauro Carvalho Chehab,
Eric W. Biederman, linux-fsdevel, David S. Miller,
Thiago Jung Bauermann
In-Reply-To: <cover.1584620202.git.msuchanek@suse.de>
Michal Suchanek's on March 19, 2020 10:19 pm:
> Less code means less bugs so add a knob to skip the compat stuff.
>
> Changes in v2: saner CONFIG_COMPAT ifdefs
> Changes in v3:
> - change llseek to 32bit instead of builing it unconditionally in fs
> - clanup the makefile conditionals
> - remove some ifdefs or convert to IS_DEFINED where possible
> Changes in v4:
> - cleanup is_32bit_task and current_is_64bit
> - more makefile cleanup
> Changes in v5:
> - more current_is_64bit cleanup
> - split off callchain.c 32bit and 64bit parts
> Changes in v6:
> - cleanup makefile after split
> - consolidate read_user_stack_32
> - fix some checkpatch warnings
> Changes in v7:
> - add back __ARCH_WANT_SYS_LLSEEK to fix build with llseek
> - remove leftover hunk
> - add review tags
> Changes in v8:
> - consolidate valid_user_sp to fix it in the split callchain.c
> - fix build errors/warnings with PPC64 !COMPAT and PPC32
> Changes in v9:
> - remove current_is_64bit()
> Chanegs in v10:
> - rebase, sent together with the syscall cleanup
> Changes in v11:
> - rebase
> - add MAINTAINERS pattern for ppc perf
These all look good to me. I had some minor comment about one patch but
not really a big deal and there were more cleanups on top of it, so I
don't mind if it's merged as is.
Actually I think we have a bit of stack reading fixes for 64s radix now
(not a bug fix as such, but we don't need the hash fault logic in radix),
so if I get around to that I can propose the changes in that series.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v11 0/8] Disable compat cruft on ppc64le v11
From: Christophe Leroy @ 2020-04-03 7:26 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev, Michal Suchanek
Cc: Mark Rutland, Gustavo Luiz Duarte, Alexander Shishkin,
Sebastian Andrzej Siewior, linux-kernel, Paul Mackerras,
Jiri Olsa, Rob Herring, Michael Neuling, Eric Richter,
Masahiro Yamada, Nayna Jain, Peter Zijlstra, Ingo Molnar,
Hari Bathini, Jordan Niethe, Valentin Schneider, Arnd Bergmann,
Arnaldo Carvalho de Melo, Alexander Viro, Jonathan Cameron,
Namhyung Kim, Thomas Gleixner, Andy Shevchenko, Allison Randal,
Greg Kroah-Hartman, Claudio Carvalho, Mauro Carvalho Chehab,
Eric W. Biederman, linux-fsdevel, David S. Miller,
Thiago Jung Bauermann
In-Reply-To: <1585898335.tckaz04a6x.astroid@bobo.none>
Le 03/04/2020 à 09:25, Nicholas Piggin a écrit :
> Michal Suchanek's on March 19, 2020 10:19 pm:
>> Less code means less bugs so add a knob to skip the compat stuff.
>>
>> Changes in v2: saner CONFIG_COMPAT ifdefs
>> Changes in v3:
>> - change llseek to 32bit instead of builing it unconditionally in fs
>> - clanup the makefile conditionals
>> - remove some ifdefs or convert to IS_DEFINED where possible
>> Changes in v4:
>> - cleanup is_32bit_task and current_is_64bit
>> - more makefile cleanup
>> Changes in v5:
>> - more current_is_64bit cleanup
>> - split off callchain.c 32bit and 64bit parts
>> Changes in v6:
>> - cleanup makefile after split
>> - consolidate read_user_stack_32
>> - fix some checkpatch warnings
>> Changes in v7:
>> - add back __ARCH_WANT_SYS_LLSEEK to fix build with llseek
>> - remove leftover hunk
>> - add review tags
>> Changes in v8:
>> - consolidate valid_user_sp to fix it in the split callchain.c
>> - fix build errors/warnings with PPC64 !COMPAT and PPC32
>> Changes in v9:
>> - remove current_is_64bit()
>> Chanegs in v10:
>> - rebase, sent together with the syscall cleanup
>> Changes in v11:
>> - rebase
>> - add MAINTAINERS pattern for ppc perf
>
> These all look good to me. I had some minor comment about one patch but
> not really a big deal and there were more cleanups on top of it, so I
> don't mind if it's merged as is.
>
> Actually I think we have a bit of stack reading fixes for 64s radix now
> (not a bug fix as such, but we don't need the hash fault logic in radix),
> so if I get around to that I can propose the changes in that series.
>
As far as I can see, there is a v12
Christophe
^ permalink raw reply
* Re: [PATCH] tty: hvc: remove hvcs_driver_string
From: Jiri Slaby @ 2020-04-03 7:30 UTC (permalink / raw)
To: Jason Yan, gregkh, linuxppc-dev
In-Reply-To: <20200403071325.3721-1-yanaijie@huawei.com>
On 03. 04. 20, 9:13, Jason Yan wrote:
> No users of hvcs_driver_string, remove it. This fixes the following gcc
> warning:
>
> drivers/tty/hvc/hvcs.c:199:19: warning: ‘hvcs_driver_string’ defined but
> not used [-Wunused-const-variable=]
> static const char hvcs_driver_string[]
> ^~~~~~~~~~~~~~~~~~
>
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
Acked-by: Jiri Slaby <jslaby@suse.cz>
It fixes a commit from 2011:
commit c7704d352d45de47333f2d9f10aead820b49044c
Author: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Sun Feb 6 18:26:25 2011 +0000
powerpc/pseries: Reduce HVCS driver insanity
!
> ---
> drivers/tty/hvc/hvcs.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/tty/hvc/hvcs.c b/drivers/tty/hvc/hvcs.c
> index ee0604cd9c6b..55105ac38f89 100644
> --- a/drivers/tty/hvc/hvcs.c
> +++ b/drivers/tty/hvc/hvcs.c
> @@ -196,8 +196,6 @@ module_param(hvcs_parm_num_devs, int, 0);
>
> static const char hvcs_driver_name[] = "hvcs";
> static const char hvcs_device_node[] = "hvcs";
> -static const char hvcs_driver_string[]
> - = "IBM hvcs (Hypervisor Virtual Console Server) Driver";
>
> /* Status of partner info rescan triggered via sysfs. */
> static int hvcs_rescan_status;
>
--
js
suse labs
^ permalink raw reply
* Re: [RFC WIP PATCH] powerpc/32: system call implement entry/exit logic in C
From: Nicholas Piggin @ 2020-04-03 7:33 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Christophe Leroy, Michael Ellerman,
msuchanek, Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <059c1abd-6be2-25ea-83e0-dcd411b7951b@c-s.fr>
Christophe Leroy's on April 1, 2020 9:48 pm:
>
>
> Le 31/03/2020 à 17:22, Christophe Leroy a écrit :
>> That's first try to port PPC64 syscall entry/exit logic in C to PPC32.
>> I've do the minimum to get it work. I have not reworked calls
>> to sys_fork() and friends for instance.
>>
>> For the time being, it seems to work more or less but:
>> - ping reports EINVAL on recvfrom
>> - strace shows NULL instead of strings in call like open() for instance.
>
> For the two above problems, that's because system_call_exception()
> doesn't set orig_gpr3 whereas DoSycall() does in entry_32.S . Is that
> only done on PPC32 ?
>
> With the following line at the begining of system_call_exception(), it
> works perfectly:
>
> regs->orig_gpr3 = r3;
Oh great, nice work. We should be able to make some simple helpers or
move some things a bit to reduce the amount of ifdefs in the C code.
It doesn't look too bad though.
> I will now focus on performance to see if we can do something about it.
What's the performance difference between current asm code just with
always saving NVGPRS vs C?
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v8 2/7] powerpc/kprobes: Mark newly allocated probes as RO
From: Russell Currey @ 2020-04-03 7:59 UTC (permalink / raw)
To: Naveen N. Rao, linuxppc-dev; +Cc: kernel-hardening, ajd, npiggin, dja
In-Reply-To: <1585852977.oiikywo1jz.naveen@linux.ibm.com>
On Fri, 2020-04-03 at 00:18 +0530, Naveen N. Rao wrote:
> Naveen N. Rao wrote:
> > Russell Currey wrote:
> > > With CONFIG_STRICT_KERNEL_RWX=y and CONFIG_KPROBES=y, there will
> > > be one
> > > W+X page at boot by default. This can be tested with
> > > CONFIG_PPC_PTDUMP=y and CONFIG_PPC_DEBUG_WX=y set, and checking
> > > the
> > > kernel log during boot.
> > >
> > > powerpc doesn't implement its own alloc() for kprobes like other
> > > architectures do, but we couldn't immediately mark RO anyway
> > > since we do
> > > a memcpy to the page we allocate later. After that, nothing
> > > should be
> > > allowed to modify the page, and write permissions are removed
> > > well
> > > before the kprobe is armed.
> > >
> > > The memcpy() would fail if >1 probes were allocated, so use
> > > patch_instruction() instead which is safe for RO.
> > >
> > > Reviewed-by: Daniel Axtens <dja@axtens.net>
> > > Signed-off-by: Russell Currey <ruscur@russell.cc>
> > > Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> > > ---
> > > arch/powerpc/kernel/kprobes.c | 17 +++++++++++++----
> > > 1 file changed, 13 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/arch/powerpc/kernel/kprobes.c
> > > b/arch/powerpc/kernel/kprobes.c
> > > index 81efb605113e..fa4502b4de35 100644
> > > --- a/arch/powerpc/kernel/kprobes.c
> > > +++ b/arch/powerpc/kernel/kprobes.c
> > > @@ -24,6 +24,8 @@
> > > #include <asm/sstep.h>
> > > #include <asm/sections.h>
> > > #include <linux/uaccess.h>
> > > +#include <linux/set_memory.h>
> > > +#include <linux/vmalloc.h>
> > >
> > > DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
> > > DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
> > > @@ -102,6 +104,16 @@ kprobe_opcode_t *kprobe_lookup_name(const
> > > char *name, unsigned int offset)
> > > return addr;
> > > }
> > >
> > > +void *alloc_insn_page(void)
> > > +{
> > > + void *page = vmalloc_exec(PAGE_SIZE);
> > > +
> > > + if (page)
> > > + set_memory_ro((unsigned long)page, 1);
> > > +
> > > + return page;
> > > +}
> > > +
> >
> > This crashes for me with KPROBES_SANITY_TEST during the kretprobe
> > test.
>
> That isn't needed to reproduce this. After bootup, disabling
> optprobes
> also shows the crash with kretprobes:
> sysctl debug.kprobes-optimization=0
>
> The problem happens to be with patch_instruction() in
> arch_prepare_kprobe(). During boot, on kprobe init, we register a
> probe
> on kretprobe_trampoline for use with kretprobes (see
> arch_init_kprobes()). This results in an instruction slot being
> allocated, and arch_prepare_kprobe() to be called for copying the
> instruction (nop) at kretprobe_trampoline. patch_instruction() is
> failing resulting in corrupt instruction which we try to
> emulate/single
> step causing the crash.
OK I think I've fixed it, KPROBES_SANITY_TEST passes too. I'd
appreciate it if you could test v9, and thanks again for finding this -
very embarrassing bug on my side.
- Russell
>
>
> - Naveen
>
^ permalink raw reply
* Re: [PATCH kernel] powerpc/pseries/ddw: Extend upper limit for huge DMA window for persistent memory
From: Alexey Kardashevskiy @ 2020-04-03 8:06 UTC (permalink / raw)
To: Wen Xiong; +Cc: Brian J King, oohall, aneesh.kumar, linuxppc-dev, david
In-Reply-To: <OFAFE8A91E.912BDABA-ON0025853E.00788BCA-0025853E.0079C2C8@notes.na.collabserv.com>
Hi Wen,
Can you please try this? It contains 3 patches from Christoph Hellwig
plus my patch on top, this should improve performance when DMA-ing not
to/from persistent memory:
https://github.com/aik/linux/commits/dma-bypass.3
I am looking for any benchmarks not related to persistent memory. Thanks,
On 03/04/2020 09:09, Wen Xiong wrote:
> I applied the patch on top of the latest upstream kernel. I ran HTX over
> pmem nodes for several hours and it works.
>
> Tested-by: Wen Xiong<wenxiong@linux.vnet.ibm.com>
>
> Thanks,
> Wendy
>
> ----- Original message -----
> From: Alexey Kardashevskiy <aik@ozlabs.ru>
> To: linuxppc-dev@lists.ozlabs.org
> Cc: Alexey Kardashevskiy <aik@ozlabs.ru>, David Gibson
> <david@gibson.dropbear.id.au>, Michael Ellerman
> <mpe@ellerman.id.au>, Oliver O'Halloran <oohall@gmail.com>, "Aneesh
> Kumar K . V" <aneesh.kumar@linux.ibm.com>, Wen Xiong
> <wenxiong@us.ibm.com>, Brian J King <bjking1@us.ibm.com>
> Subject: [EXTERNAL] [PATCH kernel] powerpc/pseries/ddw: Extend upper
> limit for huge DMA window for persistent memory
> Date: Mon, Mar 30, 2020 8:23 PM
>
> Unlike normal memory ("memory" compatible type in the FDT),
> the persistent memory ("ibm,pmemory" in the FDT) can be mapped anywhere
> in the guest physical space and it can be used for DMA.
>
> In order to maintain 1:1 mapping via the huge DMA window, we need to
> know the maximum physical address at the time of the window setup.
> So far we've been looking at "memory" nodes but "ibm,pmemory" does not
> have fixed addresses and the persistent memory may be mapped afterwards.
>
> Since the persistent memory is still backed with page structs,
> use MAX_PHYSMEM_BITS as the upper limit.
>
> This effectively disables huge DMA window in LPAR under pHyp if
> persistent memory is present but this is the best we can do.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> arch/powerpc/platforms/pseries/iommu.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/arch/powerpc/platforms/pseries/iommu.c
> b/arch/powerpc/platforms/pseries/iommu.c
> index 2e0a8eab5588..6d47b4a3ce39 100644
> --- a/arch/powerpc/platforms/pseries/iommu.c
> +++ b/arch/powerpc/platforms/pseries/iommu.c
> @@ -945,6 +945,15 @@ static phys_addr_t ddw_memory_hotplug_max(void)
> phys_addr_t max_addr = memory_hotplug_max();
> struct device_node *memory;
>
> + /*
> + * The "ibm,pmemory" can appear anywhere in the address space.
> + * Assuming it is still backed by page structs, set the upper limit
> + * for the huge DMA window as MAX_PHYSMEM_BITS.
> + */
> + if (of_find_node_by_type(NULL, "ibm,pmemory"))
> + return (sizeof(phys_addr_t) * 8 <= MAX_PHYSMEM_BITS) ?
> + (phys_addr_t) -1 : (1ULL << MAX_PHYSMEM_BITS);
> +
> for_each_node_by_type(memory, "memory") {
> unsigned long start, size;
> int n_mem_addr_cells, n_mem_size_cells, len;
> --
> 2.17.1
>
>
>
>
--
Alexey
^ permalink raw reply
* Re: [PATCH v10 00/14] powerpc/vas: Page fault handling for user space NX requests
From: Cédric Le Goater @ 2020-04-03 8:19 UTC (permalink / raw)
To: Haren Myneni, mpe
Cc: mikey, srikar, frederic.barrat, ajd, linux-kernel, npiggin, hch,
oohall, sukadev, linuxppc-dev, herbert
In-Reply-To: <1585810846.2275.23.camel@hbabu-laptop>
On 4/2/20 9:00 AM, Haren Myneni wrote:
>
> On power9, Virtual Accelerator Switchboard (VAS) allows user space or
> kernel to communicate with Nest Accelerator (NX) directly using COPY/PASTE
> instructions. NX provides various functionalities such as compression,
> encryption and etc. But only compression (842 and GZIP formats) is
> supported in Linux kernel on power9.
>
> 842 compression driver (drivers/crypto/nx/nx-842-powernv.c)
> is already included in Linux. Only GZIP support will be available from
> user space.
>
> Applications can issue GZIP compression / decompression requests to NX with
> COPY/PASTE instructions. When NX is processing these requests, can hit
> fault on the request buffer (not in memory). It issues an interrupt and
> pastes fault CRB in fault FIFO. Expects kernel to handle this fault and
> return credits for both send and fault windows after processing.
How complex would it be to provide a QEMU PowerNV model for the NX and
VAS units ? We can stage the different level of support. First would be
a simple HW init. I am interested for the XIVE modeling of course.
Thanks,
C.
>
> This patch series adds IRQ and fault window setup, and NX fault handling:
> - Alloc IRQ and trigger port address, and configure IRQ per VAS instance.
> - Set port# for each window to generate an interrupt when noticed fault.
> - Set fault window and FIFO on which NX paste fault CRB.
> - Setup IRQ thread fault handler per VAS instance.
> - When receiving an interrupt, Read CRBs from fault FIFO and update
> coprocessor_status_block (CSB) in the corresponding CRB with translation
> failure (CSB_CC_TRANSLATION). After issuing NX requests, process polls
> on CSB address. When it sees translation error, can touch the request
> buffer to bring the page in to memory and reissue NX request.
> - If copy_to_user fails on user space CSB address, OS sends SEGV signal.
>
> Tested these patches with NX-GZIP enable patches and posted them as separate
> patch series.
>
> Patch 1: Define alloc IRQ per chip which is needed to alloc IRQ per VAS
> instance.
> Patch 2: Define nx_fault_stamp on which NX writes fault status for the fault
> CRB
> Patch 3: Alloc and setup IRQ and trigger port address for each VAS instance
> Patches 4 & 5: Setup fault window and register NX per each VAS instance. This
> window is used for NX to paste fault CRB in FIFO.
> Patch 6: Reference to pid and mm so that pid is not used until window closed.
> Needed for multi thread application where child can open a window
> and can be used by parent it later.
> Patch 7: Setup threaded IRQ handler per VAS
> Patch 8: Process CRBs from fault FIFO and notify tasks by updating CSB or
> through signals.
> Patches 9 & 11: Return credits for send and fault windows after handling
> faults.
> Patches 10 & 12: Dump FIFO / CRB data and messages for error conditions
> Patch 13: Fix closing send window after all credits are returned. This issue
> happens only for user space requests. No page faults on kernel
> request buffer.
> Patch 14: For each process / thread, use mm_context->vas_windows counter to
> clear foreign address mapping and disable it.
>
> Changelog:
>
> V2:
> - Use threaded IRQ instead of own kernel thread handler
> - Use pswid instead of user space CSB address to find valid CRB
> - Removed unused macros and other changes as suggested by Christoph Hellwig
>
> V3:
> - Rebased to 5.5-rc2
> - Use struct pid * instead of pid_t for vas_window tgid
> - Code cleanup as suggested by Christoph Hellwig
>
> V4:
> - Define xive alloc and get IRQ info based on chip ID and use these
> functions for IRQ setup per VAS instance. It eliminates skiboot
> dependency as suggested by Oliver.
>
> V5:
> - Do not update CSB if the process is exiting (patch8)
>
> V6:
> - Add interrupt handler instead of default one and return IRQ_HANDLED
> if the fault handling thread is already in progress. (Patch7)
> - Use platform send window ID and CCW[0] bit to find valid CRB in
> fault FIFO (Patch7).
> - Return fault address to user space in BE and other changes as
> suggested by Michael Neuling. (patch8)
> - Rebased to 5.6-rc4
>
> V7:
> - Fixed sparse warnings (patches 4, 9 and 10)
>
> V8:
> - Moved mm_context_remove_copro() before mmdrop() (patch6)
> - Moved barrier before csb.flags store and add WARN_ON_ONCE() checks (patch8)
>
> V9:
> - Rebased to 5.6
> - Changes based on Cedric's comments
> - Removed "Define xive_native_alloc_get_irq_info()" patch and used
> irq_get_handler_data() (patch3)
> - Changes based on comments from Nicholas Piggin
> - Moved "Taking PID reference" patch before setting VAS fault handler
> patch
> - Removed mutex_lock/unlock (patch7)
> - Other cleanup changes
>
> V10:
> - Include patch to enable and disable CP_ABORT execution using
> mm_context->vas_windows counter.
> - Remove 'if (txwin)' line which is covered with 'else' before (patch6)
>
> Haren Myneni (14):
> powerpc/xive: Define xive_native_alloc_irq_on_chip()
> powerpc/vas: Define nx_fault_stamp in coprocessor_request_block
> powerpc/vas: Alloc and setup IRQ and trigger port address
> powerpc/vas: Setup fault window per VAS instance
> powerpc/vas: Register NX with fault window ID and IRQ port value
> powerpc/vas: Take reference to PID and mm for user space windows
> powerpc/vas: Setup thread IRQ handler per VAS instance
> powerpc/vas: Update CSB and notify process for fault CRBs
> powerpc/vas: Return credits after handling fault
> powerpc/vas: Print CRB and FIFO values
> powerpc/vas: Do not use default credits for receive window
> powerpc/vas: Display process stuck message
> powerpc/vas: Free send window in VAS instance after credits returned
> powerpc: Use mm_context vas_windows counter to issue CP_ABORT
>
> arch/powerpc/include/asm/book3s/64/mmu.h | 3 +
> arch/powerpc/include/asm/icswx.h | 20 +-
> arch/powerpc/include/asm/mmu_context.h | 22 ++
> arch/powerpc/include/asm/processor.h | 1 -
> arch/powerpc/include/asm/xive.h | 9 +-
> arch/powerpc/kernel/process.c | 8 +-
> arch/powerpc/platforms/powernv/Makefile | 2 +-
> arch/powerpc/platforms/powernv/vas-debug.c | 2 +-
> arch/powerpc/platforms/powernv/vas-fault.c | 382 ++++++++++++++++++++++++++++
> arch/powerpc/platforms/powernv/vas-window.c | 202 ++++++++++++++-
> arch/powerpc/platforms/powernv/vas.c | 85 ++++++-
> arch/powerpc/platforms/powernv/vas.h | 57 ++++-
> arch/powerpc/sysdev/xive/native.c | 6 +-
> 13 files changed, 767 insertions(+), 32 deletions(-)
> create mode 100644 arch/powerpc/platforms/powernv/vas-fault.c
>
^ permalink raw reply
* Re: [PATCH 1/2] dma-mapping: add a dma_ops_bypass flag to struct device
From: Alexey Kardashevskiy @ 2020-04-03 8:38 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Aneesh Kumar K.V, Joerg Roedel, Robin Murphy, linux-kernel, iommu,
Greg Kroah-Hartman, linuxppc-dev, Lu Baolu
In-Reply-To: <a705afc5-779d-baf4-e5d2-e2da04c82743@ozlabs.ru>
On 26/03/2020 12:26, Alexey Kardashevskiy wrote:
>
>
> On 25/03/2020 19:37, Christoph Hellwig wrote:
>> On Wed, Mar 25, 2020 at 03:51:36PM +1100, Alexey Kardashevskiy wrote:
>>>>> This is for persistent memory which you can DMA to/from but yet it does
>>>>> not appear in the system as a normal memory and therefore requires
>>>>> special handling anyway (O_DIRECT or DAX, I do not know the exact
>>>>> mechanics). All other devices in the system should just run as usual,
>>>>> i.e. use 1:1 mapping if possible.
>>>>
>>>> On other systems (x86 and arm) pmem as long as it is page backed does
>>>> not require any special handling. This must be some weird way powerpc
>>>> fucked up again, and I suspect you'll have to suffer from it.
>>>
>>>
>>> It does not matter if it is backed by pages or not, the problem may also
>>> appear if we wanted for example p2p PCI via IOMMU (between PHBs) and
>>> MMIO might be mapped way too high in the system address space and make
>>> 1:1 impossible.
>>
>> How can it be mapped too high for a direct mapping with a 64-bit DMA
>> mask?
>
> The window size is limited and often it is not even sparse. It requires
> an 8 byte entry per an IOMMU page (which is most commonly is 64k max) so
> 1TB limit (a guest RAM size) is a quite real thing. MMIO is mapped to
> guest physical address space outside of this 1TB (on PPC).
>
>
I am trying now this approach on top of yours "dma-bypass.3" (it is
"wip", needs an upper boundary check):
https://github.com/aik/linux/commit/49d73c7771e3f6054804f6cfa80b4e320111662d
Do you see any serious problem with this approach? Thanks!
--
Alexey
^ permalink raw reply
* Re: [PATCH 3/4] powerpc/uaccess: evaluate macro arguments once, before user access is allowed
From: Nicholas Piggin @ 2020-04-03 8:58 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev
In-Reply-To: <633fbf5b-8227-1640-f056-7e5d203895dd@c-s.fr>
Christophe Leroy's on March 27, 2020 5:21 pm:
>
>
> Le 27/03/2020 à 08:02, Nicholas Piggin a écrit :
>> get/put_user can be called with nontrivial arguments. fs/proc/page.c
>> has a good example:
>>
>> if (put_user(stable_page_flags(ppage), out)) {
>>
>> stable_page_flags is quite a lot of code, including spin locks in the
>> page allocator.
>>
>> Ensure these arguments are evaluated before user access is allowed.
>> This improves security by reducing code with access to userspace, but
>> it also fixes a PREEMPT bug with KUAP on powerpc/64s:
>> stable_page_flags is currently called with AMR set to allow writes,
>> it ends up calling spin_unlock(), which can call preempt_schedule. But
>> the task switch code can not be called with AMR set (it relies on
>> interrupts saving the register), so this blows up.
>>
>> It's fine if the code inside allow_user_access is preemptible, because
>> a timer or IPI will save the AMR, but it's not okay to explicitly
>> cause a reschedule.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> arch/powerpc/include/asm/uaccess.h | 97 ++++++++++++++++++------------
>> 1 file changed, 59 insertions(+), 38 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
>> index 670910df3cc7..1cf8595aeef1 100644
>> --- a/arch/powerpc/include/asm/uaccess.h
>> +++ b/arch/powerpc/include/asm/uaccess.h
>> @@ -162,36 +162,48 @@ do { \
>> prevent_write_to_user(ptr, size); \
>> } while (0)
>>
>> -#define __put_user_nocheck(x, ptr, size, do_allow) \
>> +#define __put_user_nocheck(x, ptr, size, do_allow) \
>
> No need to touch this line. Anyway at the end, you still have several \
> which are not aligned.
>
>> ({ \
>> long __pu_err; \
>> __typeof__(*(ptr)) __user *__pu_addr = (ptr); \
>> + __typeof__(*(ptr)) __pu_val = (x); \
>> + __typeof__(size) __pu_size = (size); \
>> + \
>> if (!is_kernel_addr((unsigned long)__pu_addr)) \
>> might_fault(); \
>> - __chk_user_ptr(ptr); \
>> - if (do_allow) \
>
> No need to touch that line
>
>> - __put_user_size((x), __pu_addr, (size), __pu_err); \
>> - else \
>
> No need to touch that line
Okay fair point I can redo the patch.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH 1/4] powerpc/64s: implement probe_kernel_read/write without touching AMR
From: Nicholas Piggin @ 2020-04-03 8:59 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev
In-Reply-To: <d619b378-3a88-f941-b2d5-42b79574e2ab@c-s.fr>
Christophe Leroy's on March 27, 2020 5:13 pm:
>
>
> Le 27/03/2020 à 08:02, Nicholas Piggin a écrit :
>> There is no need to allow user accesses when probing kernel addresses.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> arch/powerpc/include/asm/uaccess.h | 25 ++++++++++-----
>> arch/powerpc/lib/Makefile | 2 +-
>> arch/powerpc/lib/uaccess.c | 50 ++++++++++++++++++++++++++++++
>> 3 files changed, 68 insertions(+), 9 deletions(-)
>> create mode 100644 arch/powerpc/lib/uaccess.c
>>
>
> [...]
>
>> diff --git a/arch/powerpc/lib/Makefile b/arch/powerpc/lib/Makefile
>> index b8de3be10eb4..a15060b5008e 100644
>> --- a/arch/powerpc/lib/Makefile
>> +++ b/arch/powerpc/lib/Makefile
>> @@ -36,7 +36,7 @@ extra-$(CONFIG_PPC64) += crtsavres.o
>> endif
>>
>> obj-$(CONFIG_PPC_BOOK3S_64) += copyuser_power7.o copypage_power7.o \
>> - memcpy_power7.o
>> + memcpy_power7.o uaccess.o
>
> Why only book3s/64 ? It applies to the 8xx and book3s/32 as well, I
> think it should just be for all powerpc.
Okay I can do that.
>>
>> obj64-y += copypage_64.o copyuser_64.o mem_64.o hweight_64.o \
>> memcpy_64.o memcpy_mcsafe_64.o
>> diff --git a/arch/powerpc/lib/uaccess.c b/arch/powerpc/lib/uaccess.c
>> new file mode 100644
>> index 000000000000..0057ab52d6fe
>> --- /dev/null
>> +++ b/arch/powerpc/lib/uaccess.c
>> @@ -0,0 +1,50 @@
>> +#include <linux/mm.h>
>> +#include <linux/uaccess.h>
>> +
>> +static __always_inline long
>> +probe_read_common(void *dst, const void __user *src, size_t size)
>> +{
>> + long ret;
>> +
>> + pagefault_disable();
>> + ret = raw_copy_from_user_allowed(dst, src, size);
>> + pagefault_enable();
>> +
>> + return ret ? -EFAULT : 0;
>> +}
>> +
>> +static __always_inline long
>> +probe_write_common(void __user *dst, const void *src, size_t size)
>> +{
>> + long ret;
>> +
>> + pagefault_disable();
>> + ret = raw_copy_to_user_allowed(dst, src, size);
>> + pagefault_enable();
>> +
>> + return ret ? -EFAULT : 0;
>> +}
>> +
>> +long probe_kernel_read(void *dst, const void *src, size_t size)
>> +{
>> + long ret;
>> + mm_segment_t old_fs = get_fs();
>> +
>> + set_fs(KERNEL_DS);
>> + ret = probe_read_common(dst, (__force const void __user *)src, size);
>
> I think you should squash probe_read_common() here, having it separated
> is a lot of lines for no added value. It also may make people believe it
> overwrites the generic probe_read_common()
Yeah I'll see how that looks.
Thanks,
Nick
^ 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