* Re: [PATCH] powerpc/kprobes: Fix trap address when trap happened in real mode
From: Masami Hiramatsu @ 2020-02-18 0:44 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-kernel, Anil S Keshavamurthy, Paul Mackerras, stable,
Naveen N. Rao, linuxppc-dev, David S. Miller, Larry Finger
In-Reply-To: <c6257b49-bf02-d30a-1e2e-99abba5955e6@c-s.fr>
On Mon, 17 Feb 2020 16:38:50 +0100
Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>
>
> Le 17/02/2020 à 11:27, Masami Hiramatsu a écrit :
> > On Mon, 17 Feb 2020 10:03:22 +0100
> > Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> >
> >>
> >>
> >> Le 16/02/2020 à 13:34, Masami Hiramatsu a écrit :
> >>> On Sat, 15 Feb 2020 11:28:49 +0100
> >>> Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> >>>
> >>>> Hi,
> >>>>
> >>>> Le 14/02/2020 à 14:54, Masami Hiramatsu a écrit :
> >>>>> Hi,
> >>>>>
> >>>>> On Fri, 14 Feb 2020 12:47:49 +0000 (UTC)
> >>>>> Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> >>>>>
> >>>>>> When a program check exception happens while MMU translation is
> >>>>>> disabled, following Oops happens in kprobe_handler() in the following
> >>>>>> test:
> >>>>>>
> >>>>>> } else if (*addr != BREAKPOINT_INSTRUCTION) {
> >>>>>
> >>>>> Thanks for the report and patch. I'm not so sure about powerpc implementation
> >>>>> but at where the MMU translation is disabled, can the handler work correctly?
> >>>>> (And where did you put the probe on?)
> >>>>>
> >>>>> Your fix may fix this Oops, but if the handler needs special care, it is an
> >>>>> option to blacklist such place (if possible).
> >>>>
> >>>> I guess that's another story. Here we are not talking about a place
> >>>> where kprobe has been illegitimately activated, but a place where there
> >>>> is a valid trap, which generated a valid 'program check exception'. And
> >>>> kprobe was off at that time.
> >>>
> >>> Ah, I got it. It is not a kprobe breakpoint, but to check that correctly,
> >>> it has to know the address where the breakpoint happens. OK.
> >>>
> >>>>
> >>>> As any 'program check exception' due to a trap (ie a BUG_ON, a WARN_ON,
> >>>> a debugger breakpoint, a perf breakpoint, etc...) calls
> >>>> kprobe_handler(), kprobe_handler() must be prepared to handle the case
> >>>> where the MMU translation is disabled, even if probes are not supposed
> >>>> to be set for functions running with MMU translation disabled.
> >>>
> >>> Can't we check the MMU is disabled there (as same as checking the exception
> >>> happened in user space or not)?
> >>>
> >>
> >> What do you mean by 'there' ? At the entry of kprobe_handler() ?
> >>
> >> That's what my patch does, it checks whether MMU is disabled or not. If
> >> it is, it converts the address to a virtual address.
> >>
> >> Do you mean kprobe_handler() should bail out early as it does when the
> >> trap happens in user mode ?
> >
> > Yes, that is what I meant.
> >
> >> Of course we can do that, I don't know
> >> enough about kprobe to know if kprobe_handler() should manage events
> >> that happened in real-mode or just ignore them. But I tested adding an
> >> event on a function that runs in real-mode, and it (now) works.
> >>
> >> So, what should we do really ?
> >
> > I'm not sure how the powerpc kernel runs in real mode.
> > But clearly, at least kprobe event can not handle that case because
> > it tries to access memory by probe_kernel_read(). Unless that function
> > correctly handles the address translation, I want to prohibit kprobes
> > on such address.
> >
> > So what I would like to see is, something like below.
> >
> > diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> > index 2d27ec4feee4..4771be152416 100644
> > --- a/arch/powerpc/kernel/kprobes.c
> > +++ b/arch/powerpc/kernel/kprobes.c
> > @@ -261,7 +261,7 @@ int kprobe_handler(struct pt_regs *regs)
> > unsigned int *addr = (unsigned int *)regs->nip;
> > struct kprobe_ctlblk *kcb;
> >
> > - if (user_mode(regs))
> > + if (user_mode(regs) || !(regs->msr & MSR_IR))
> > return 0;
> >
> > /*
> >
> >
>
> With this instead change of my patch, I get an Oops everytime a kprobe
> event occurs in real-mode.
>
> This is because kprobe_handler() is now saying 'this trap doesn't belong
> to me' for a trap that has been installed by it.
Hmm, on powerpc, kprobes is allowed to probe on the code which runs
in the real mode? I think we should also prohibit it by blacklisting.
(It is easy to add blacklist by NOKPROBE_SYMBOL(func))
Or, some parts are possble to run under both real mode and kernel mode?
>
> So the 'program check' exception handler doesn't find the owner of the
> trap hence generate an Oops.
>
> Even if we don't want kprobe() to proceed with the event entirely
> (allthough it works at least for simple events), I'd expect it to fail
> gracefully.
Agreed. I thought it was easy to identify real mode code. But if it is
hard, we should apply your first patch and also skip user handlers
if we are in the real mode (and increment missed count).
BTW, can the emulater handle the real mode code correctly?
Thank you,
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply
* Re: [PATCH] powerpc/chrp: Fix enter_rtas() with CONFIG_VMAP_STACK
From: Michael Neuling @ 2020-02-18 0:40 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <40126489-adf8-1b65-8974-25bca584bc9b@c-s.fr>
On Mon, 2020-02-17 at 07:40 +0100, Christophe Leroy wrote:
>
> Le 16/02/2020 à 23:40, Michael Neuling a écrit :
> > On Fri, 2020-02-14 at 08:33 +0000, Christophe Leroy wrote:
> > > With CONFIG_VMAP_STACK, data MMU has to be enabled
> > > to read data on the stack.
> >
> > Can you describe what goes wrong without this? Some oops message? rtas blows
> > up?
> > Get corrupt data?
>
> Larry reported a machine check. Or in fact, he reported a Oops in
> kprobe_handler(), that Oops being a bug in kprobe_handle() triggered by
> this machine check.
>
> By converting a VM address to a phys-like address as if is was linear
> mem, you get in the dark. Either there is some physical memory at that
> address and you corrupt it. Or there is none and you get a machine check.
Excellent. Please put that in the commit message.
> > Also can you say what you're actually doing (ie turning on MSR[DR])
>
> Euh ... I'm saying that data MMU has to be enabled, so I'm enabling it.
Yeah, it's a minor point.
> >
> > > Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> > > ---
> > > arch/powerpc/kernel/entry_32.S | 9 +++++++--
> > > 1 file changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/powerpc/kernel/entry_32.S
> > > b/arch/powerpc/kernel/entry_32.S
> > > index 0713daa651d9..bc056d906b51 100644
> > > --- a/arch/powerpc/kernel/entry_32.S
> > > +++ b/arch/powerpc/kernel/entry_32.S
> > > @@ -1354,12 +1354,17 @@ _GLOBAL(enter_rtas)
> > > mtspr SPRN_SRR0,r8
> > > mtspr SPRN_SRR1,r9
> > > RFI
> > > -1: tophys(r9,r1)
> > > +1: tophys_novmstack r9, r1
> > > +#ifdef CONFIG_VMAP_STACKisntruction
> > > + li r0, MSR_KERNEL & ~MSR_IR /* can take DTLB miss */
> >
> > You're potentially turning on more than MSR DR here. This should be clear in
> > the
> > commit message.
>
> Am I ?
>
> At the time of the RFI just above, SRR1 contains the value of r9 which
> has been set 2 lines before to MSR_KERNEL & ~(MSR_IR|MSR_DR).
>
> What should be clear in the commit message ?
You're right. I was just looking at the patch and not the code. It's clearer in
the code.
Mikey
>
> > > + mtmsr r0
> > > + isync
> > > +#endif
> > > lwz r8,INT_FRAME_SIZE+4(r9) /* get return address */
> > > lwz r9,8(r9) /* original msr value */
> > > addi r1,r1,INT_FRAME_SIZE
> > > li r0,0
> > > - tophys(r7, r2)
> > > + tophys_novmstack r7, r2
> > > stw r0, THREAD + RTAS_SP(r7)
> > > mtspr SPRN_SRR0,r8
> > > mtspr SPRN_SRR1,r9
>
> Christophe
>
^ permalink raw reply
* Re: [PATCH v6 02/11] mm/gup: Use functions to track lockless pgtbl walks on gup_pgd_range
From: Leonardo Bras @ 2020-02-17 20:55 UTC (permalink / raw)
To: John Hubbard, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Arnd Bergmann, Andrew Morton, Aneesh Kumar K.V,
Nicholas Piggin, Christophe Leroy, Steven Price, Robin Murphy,
Mahesh Salgaonkar, Balbir Singh, Reza Arbab, Thomas Gleixner,
Allison Randal, Greg Kroah-Hartman, Mike Rapoport,
Michal Suchanek
Cc: linux-arch, linux-mm, linuxppc-dev, linux-kernel, kvm-ppc
In-Reply-To: <0c2f5a89-4890-fd84-6a6d-e470ba110399@nvidia.com>
[-- Attachment #1: Type: text/plain, Size: 1235 bytes --]
Hello John, comments inline;
On Fri, 2020-02-07 at 14:54 -0800, John Hubbard wrote:
> On 2/5/20 7:25 PM, Leonardo Bras wrote:
> > On Thu, 2020-02-06 at 00:08 -0300, Leonardo Bras wrote:
> > > gup_pgd_range(addr, end, gup_flags, pages, &nr);
> > > - local_irq_enable();
> > > + end_lockless_pgtbl_walk(IRQS_ENABLED);
> > > ret = nr;
> > > }
> > >
> >
> > Just noticed IRQS_ENABLED is not available on other archs than ppc64.
> > I will fix this for v7.
> >
>
> What's the fix going to look like, approximately?
I am not sure what is the best approach yet.
1. On irq_mask == 0, always enable irq on end_lockless_pgtbl_walk().
Not sure how bat would that affect other archs.
2. Add another function like end_lockless_pgtbl_walk_irqen() that
always enables IRQ.
3. Add another parameter in end_lockless_pgtbl_walk(), so that caller
can choose ii IRQ must be enabled.
Also, not sure if internal_get_user_pages_fast() can possibly be called
with IRQ disabled, and then return with it enabled. Maybe just
saving/restoring should be fine.
Other suggestions are welcome.
>
>
> thanks,
Best regards,
Leonardo Bras
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v2 21/24] docs: powerpc: convert vcpudispatch_stats.txt to ReST
From: Mauro Carvalho Chehab @ 2020-02-17 16:20 UTC (permalink / raw)
To: Linux Doc Mailing List
Cc: Jonathan Corbet, Mauro Carvalho Chehab, Paul Mackerras,
linuxppc-dev
In-Reply-To: <cover.1581956285.git.mchehab+huawei@kernel.org>
- Add a SPDX header;
- Use standard markup for document title;
- Adjust identation on lists and add blank lines where
needed;
- Add it to the powerpc index.rst file.
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
Documentation/powerpc/index.rst | 1 +
...ispatch_stats.txt => vcpudispatch_stats.rst} | 17 ++++++++++++-----
2 files changed, 13 insertions(+), 5 deletions(-)
rename Documentation/powerpc/{vcpudispatch_stats.txt => vcpudispatch_stats.rst} (94%)
diff --git a/Documentation/powerpc/index.rst b/Documentation/powerpc/index.rst
index 0d45f0fc8e57..29b90b1b6f20 100644
--- a/Documentation/powerpc/index.rst
+++ b/Documentation/powerpc/index.rst
@@ -30,6 +30,7 @@ powerpc
syscall64-abi
transactional_memory
ultravisor
+ vcpudispatch_stats
.. only:: subproject and html
diff --git a/Documentation/powerpc/vcpudispatch_stats.txt b/Documentation/powerpc/vcpudispatch_stats.rst
similarity index 94%
rename from Documentation/powerpc/vcpudispatch_stats.txt
rename to Documentation/powerpc/vcpudispatch_stats.rst
index e21476bfd78c..5704657a5987 100644
--- a/Documentation/powerpc/vcpudispatch_stats.txt
+++ b/Documentation/powerpc/vcpudispatch_stats.rst
@@ -1,5 +1,8 @@
-VCPU Dispatch Statistics:
-=========================
+.. SPDX-License-Identifier: GPL-2.0
+
+========================
+VCPU Dispatch Statistics
+========================
For Shared Processor LPARs, the POWER Hypervisor maintains a relatively
static mapping of the LPAR processors (vcpus) to physical processor
@@ -20,25 +23,29 @@ The statistics themselves are available by reading the procfs file
a vcpu as represented by the first field, followed by 8 numbers.
The first number corresponds to:
+
1. total vcpu dispatches since the beginning of statistics collection
The next 4 numbers represent vcpu dispatch dispersions:
+
2. number of times this vcpu was dispatched on the same processor as last
time
3. number of times this vcpu was dispatched on a different processor core
as last time, but within the same chip
4. number of times this vcpu was dispatched on a different chip
5. number of times this vcpu was dispatches on a different socket/drawer
-(next numa boundary)
+ (next numa boundary)
The final 3 numbers represent statistics in relation to the home node of
the vcpu:
+
6. number of times this vcpu was dispatched in its home node (chip)
7. number of times this vcpu was dispatched in a different node
8. number of times this vcpu was dispatched in a node further away (numa
-distance)
+ distance)
+
+An example output::
-An example output:
$ sudo cat /proc/powerpc/vcpudispatch_stats
cpu0 6839 4126 2683 30 0 6821 18 0
cpu1 2515 1274 1229 12 0 2509 6 0
--
2.24.1
^ permalink raw reply related
* Re: [PATCH v6 03/11] powerpc/mm: Adds arch-specificic functions to track lockless pgtable walks
From: Leonardo Bras @ 2020-02-17 20:32 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Arnd Bergmann, Andrew Morton, Aneesh Kumar K.V,
Nicholas Piggin, Steven Price, Robin Murphy, Mahesh Salgaonkar,
Balbir Singh, Reza Arbab, Thomas Gleixner, Allison Randal,
Greg Kroah-Hartman, Mike Rapoport, Michal Suchanek
Cc: linux-arch, linux-mm, linuxppc-dev, linux-kernel, kvm-ppc
In-Reply-To: <efcf780cee767d0f4b06b56e216725c6bd8d60d4.camel@linux.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 589 bytes --]
On Fri, 2020-02-07 at 01:38 -0300, Leonardo Bras wrote:
> > Why not make them static inline just like the generic ones ?
> >
>
> Sure, can be done. It would save some function calls.
> For that I will define the per-cpu variable in .c and declare it in .h
> All new function can be moved to .h, while changing adding the inline
> modifier.
Just tried doing that, but percpu stuff relies in paca definitions, and
this ends up creating some cyclic dependencies.
I could try to change that, but the amount of change is big and
probably should be dealt in another patchset.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* vdso function descriptors (VDS64_HAS_DESCRIPTORS)?
From: Joe Lawrence @ 2020-02-17 16:08 UTC (permalink / raw)
To: linuxppc-dev
I was wondering if there was history behind VDS64_HAS_DESCRIPTORS and in
what cases would one want to turn them on? (Note, I'm assuming they are
an implementation of Function Descriptors. [1])
arch/powerpc/include/asm/vdso.h unsets the macro:
/* Define if 64 bits VDSO has procedure descriptors */
#undef VDS64_HAS_DESCRIPTORS
so I don't believe they are ever used by default -- in this case
V_FUNCTION_BEGIN doesn't add to the .opd section with .name, .TOC base,
etc.
Manually setting VDS64_HAS_DESCRIPTORS results in a vdso64.so in which
binutils tools like readelf properly report functions with symbol type
FUNC instead of NOTYPE.
Are there pieces of the build/etc toolchain unprepared for function
descriptors? I'm just trying to figure out why the code defaults to
unsetting them.
Thanks,
-- Joe
[1] http://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi.html#FUNC-DES
^ permalink raw reply
* Re: [PATCH] powerpc/kprobes: Fix trap address when trap happened in real mode
From: Christophe Leroy @ 2020-02-17 17:41 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: linux-kernel, Anil S Keshavamurthy, stable, Paul Mackerras,
Naveen N. Rao, linuxppc-dev, David S. Miller, Larry Finger
In-Reply-To: <c6257b49-bf02-d30a-1e2e-99abba5955e6@c-s.fr>
On 02/17/2020 03:38 PM, Christophe Leroy wrote:
>
>
> Le 17/02/2020 à 11:27, Masami Hiramatsu a écrit :
>> On Mon, 17 Feb 2020 10:03:22 +0100
>> Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>>
>>>
>>>
>>> Le 16/02/2020 à 13:34, Masami Hiramatsu a écrit :
>>>> On Sat, 15 Feb 2020 11:28:49 +0100
>>>> Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>> Le 14/02/2020 à 14:54, Masami Hiramatsu a écrit :
>>>>>> Hi,
>>>>>>
>>>>>> On Fri, 14 Feb 2020 12:47:49 +0000 (UTC)
>>>>>> Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>>>>>>
>>>>>>> When a program check exception happens while MMU translation is
>>>>>>> disabled, following Oops happens in kprobe_handler() in the
>>>>>>> following
>>>>>>> test:
>>>>>>>
>>>>>>> } else if (*addr != BREAKPOINT_INSTRUCTION) {
>>>>>>
>>>>>> Thanks for the report and patch. I'm not so sure about powerpc
>>>>>> implementation
>>>>>> but at where the MMU translation is disabled, can the handler work
>>>>>> correctly?
>>>>>> (And where did you put the probe on?)
>>>>>>
>>>>>> Your fix may fix this Oops, but if the handler needs special care,
>>>>>> it is an
>>>>>> option to blacklist such place (if possible).
>>>>>
>>>>> I guess that's another story. Here we are not talking about a place
>>>>> where kprobe has been illegitimately activated, but a place where
>>>>> there
>>>>> is a valid trap, which generated a valid 'program check exception'.
>>>>> And
>>>>> kprobe was off at that time.
>>>>
>>>> Ah, I got it. It is not a kprobe breakpoint, but to check that
>>>> correctly,
>>>> it has to know the address where the breakpoint happens. OK.
>>>>
>>>>>
>>>>> As any 'program check exception' due to a trap (ie a BUG_ON, a
>>>>> WARN_ON,
>>>>> a debugger breakpoint, a perf breakpoint, etc...) calls
>>>>> kprobe_handler(), kprobe_handler() must be prepared to handle the case
>>>>> where the MMU translation is disabled, even if probes are not supposed
>>>>> to be set for functions running with MMU translation disabled.
>>>>
>>>> Can't we check the MMU is disabled there (as same as checking the
>>>> exception
>>>> happened in user space or not)?
>>>>
>>>
>>> What do you mean by 'there' ? At the entry of kprobe_handler() ?
>>>
>>> That's what my patch does, it checks whether MMU is disabled or not. If
>>> it is, it converts the address to a virtual address.
>>>
>>> Do you mean kprobe_handler() should bail out early as it does when the
>>> trap happens in user mode ?
>>
>> Yes, that is what I meant.
>>
>>> Of course we can do that, I don't know
>>> enough about kprobe to know if kprobe_handler() should manage events
>>> that happened in real-mode or just ignore them. But I tested adding an
>>> event on a function that runs in real-mode, and it (now) works.
>>>
>>> So, what should we do really ?
>>
>> I'm not sure how the powerpc kernel runs in real mode.
>> But clearly, at least kprobe event can not handle that case because
>> it tries to access memory by probe_kernel_read(). Unless that function
>> correctly handles the address translation, I want to prohibit kprobes
>> on such address.
>>
>> So what I would like to see is, something like below.
>>
>> diff --git a/arch/powerpc/kernel/kprobes.c
>> b/arch/powerpc/kernel/kprobes.c
>> index 2d27ec4feee4..4771be152416 100644
>> --- a/arch/powerpc/kernel/kprobes.c
>> +++ b/arch/powerpc/kernel/kprobes.c
>> @@ -261,7 +261,7 @@ int kprobe_handler(struct pt_regs *regs)
>> unsigned int *addr = (unsigned int *)regs->nip;
>> struct kprobe_ctlblk *kcb;
>> - if (user_mode(regs))
>> + if (user_mode(regs) || !(regs->msr & MSR_IR))
>> return 0;
>> /*
>>
>>
>
> With this instead change of my patch, I get an Oops everytime a kprobe
> event occurs in real-mode.
>
> This is because kprobe_handler() is now saying 'this trap doesn't belong
> to me' for a trap that has been installed by it.
>
> So the 'program check' exception handler doesn't find the owner of the
> trap hence generate an Oops.
>
> Even if we don't want kprobe() to proceed with the event entirely
> (allthough it works at least for simple events), I'd expect it to fail
> gracefully.
>
What about something like that:
@@ -264,6 +265,13 @@ int kprobe_handler(struct pt_regs *regs)
if (user_mode(regs))
return 0;
+ if (!(regs->msr & MSR_IR)) {
+ if (!get_kprobe(phys_to_virt(regs->nip)))
+ return 0;
+ regs->nip += 4;
+ return 1;
+ }
+
/*
* We don't want to be preempted for the entire
* duration of kprobe processing
Christophe
^ permalink raw reply
* Re: [PATCH] powerpc/kprobes: Fix trap address when trap happened in real mode
From: Christophe Leroy @ 2020-02-17 15:38 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: linux-kernel, Anil S Keshavamurthy, Paul Mackerras, stable,
Naveen N. Rao, linuxppc-dev, David S. Miller, Larry Finger
In-Reply-To: <20200217192735.5070f0925c4159ccffa4e465@kernel.org>
Le 17/02/2020 à 11:27, Masami Hiramatsu a écrit :
> On Mon, 17 Feb 2020 10:03:22 +0100
> Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>
>>
>>
>> Le 16/02/2020 à 13:34, Masami Hiramatsu a écrit :
>>> On Sat, 15 Feb 2020 11:28:49 +0100
>>> Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>>>
>>>> Hi,
>>>>
>>>> Le 14/02/2020 à 14:54, Masami Hiramatsu a écrit :
>>>>> Hi,
>>>>>
>>>>> On Fri, 14 Feb 2020 12:47:49 +0000 (UTC)
>>>>> Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>>>>>
>>>>>> When a program check exception happens while MMU translation is
>>>>>> disabled, following Oops happens in kprobe_handler() in the following
>>>>>> test:
>>>>>>
>>>>>> } else if (*addr != BREAKPOINT_INSTRUCTION) {
>>>>>
>>>>> Thanks for the report and patch. I'm not so sure about powerpc implementation
>>>>> but at where the MMU translation is disabled, can the handler work correctly?
>>>>> (And where did you put the probe on?)
>>>>>
>>>>> Your fix may fix this Oops, but if the handler needs special care, it is an
>>>>> option to blacklist such place (if possible).
>>>>
>>>> I guess that's another story. Here we are not talking about a place
>>>> where kprobe has been illegitimately activated, but a place where there
>>>> is a valid trap, which generated a valid 'program check exception'. And
>>>> kprobe was off at that time.
>>>
>>> Ah, I got it. It is not a kprobe breakpoint, but to check that correctly,
>>> it has to know the address where the breakpoint happens. OK.
>>>
>>>>
>>>> As any 'program check exception' due to a trap (ie a BUG_ON, a WARN_ON,
>>>> a debugger breakpoint, a perf breakpoint, etc...) calls
>>>> kprobe_handler(), kprobe_handler() must be prepared to handle the case
>>>> where the MMU translation is disabled, even if probes are not supposed
>>>> to be set for functions running with MMU translation disabled.
>>>
>>> Can't we check the MMU is disabled there (as same as checking the exception
>>> happened in user space or not)?
>>>
>>
>> What do you mean by 'there' ? At the entry of kprobe_handler() ?
>>
>> That's what my patch does, it checks whether MMU is disabled or not. If
>> it is, it converts the address to a virtual address.
>>
>> Do you mean kprobe_handler() should bail out early as it does when the
>> trap happens in user mode ?
>
> Yes, that is what I meant.
>
>> Of course we can do that, I don't know
>> enough about kprobe to know if kprobe_handler() should manage events
>> that happened in real-mode or just ignore them. But I tested adding an
>> event on a function that runs in real-mode, and it (now) works.
>>
>> So, what should we do really ?
>
> I'm not sure how the powerpc kernel runs in real mode.
> But clearly, at least kprobe event can not handle that case because
> it tries to access memory by probe_kernel_read(). Unless that function
> correctly handles the address translation, I want to prohibit kprobes
> on such address.
>
> So what I would like to see is, something like below.
>
> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> index 2d27ec4feee4..4771be152416 100644
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -261,7 +261,7 @@ int kprobe_handler(struct pt_regs *regs)
> unsigned int *addr = (unsigned int *)regs->nip;
> struct kprobe_ctlblk *kcb;
>
> - if (user_mode(regs))
> + if (user_mode(regs) || !(regs->msr & MSR_IR))
> return 0;
>
> /*
>
>
With this instead change of my patch, I get an Oops everytime a kprobe
event occurs in real-mode.
This is because kprobe_handler() is now saying 'this trap doesn't belong
to me' for a trap that has been installed by it.
So the 'program check' exception handler doesn't find the owner of the
trap hence generate an Oops.
Even if we don't want kprobe() to proceed with the event entirely
(allthough it works at least for simple events), I'd expect it to fail
gracefully.
Christophe
^ permalink raw reply
* Re: QEMU/KVM snapshot restore bug
From: Cédric Le Goater @ 2020-02-17 15:20 UTC (permalink / raw)
To: dftxbs3e, linuxppc-dev, Greg Kurz
In-Reply-To: <49161ca3-941a-4f8b-f0ef-cce49ab0f9cd@free.fr>
On 2/17/20 3:48 AM, dftxbs3e wrote:
> On 2/16/20 7:16 PM, Cédric Le Goater wrote:
>>
>> I think this is fixed by commit f55750e4e4fb ("spapr/xive: Mask the EAS when
>> allocating an IRQ") which is not in QEMU 4.1.1. The same problem should also
>> occur with LE guests.
>>
>> Could you possibly regenerate the QEMU rpm with this patch ?
>>
>> Thanks,
>>
>> C.
>
> Hello!
>
> I applied the patch and reinstalled the RPM then tried to restore the
> snapshot I created previously and it threw the same error.
>
> Do I need to re-create the snapshot and/or restart the machine?
yes. The problem is at the source.
> I have
> important workloads running so that'll be possible only in a few days if
> needed.
OK.
Thanks,
C.
^ permalink raw reply
* Re: [PATCH 1/1] powerpc/cputable: Remove unnecessary copy of cpu_spec->oprofile_type
From: Leonardo Bras @ 2020-02-17 14:18 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Greg Kroah-Hartman, Thomas Gleixner, desnesn
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <0d33a8b2-a7e5-c3e0-b28a-fd39f1231d97@c-s.fr>
[-- Attachment #1: Type: text/plain, Size: 565 bytes --]
Hello Christophe, thank you for the feedback.
On Mon, 2020-02-17 at 07:31 +0100, Christophe Leroy wrote:
> > if (old.oprofile_cpu_type != NULL) {
> > t->oprofile_cpu_type = old.oprofile_cpu_type;
> > - t->oprofile_type = old.oprofile_type;
> > }
>
> The action being reduced to a single line, the { } should be removed.
>
> Christophe
I intentionally let it this way because I just reviewed a patch that
will add more itens here, and should be merged before this one.
This will avoid conflicts.
Best regards,
Leonardo Bras
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 1/1] powerpc/cputable: Remove unnecessary copy of cpu_spec->oprofile_type
From: Leonardo Bras @ 2020-02-17 14:15 UTC (permalink / raw)
To: Michael Neuling, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Christophe Leroy, Greg Kroah-Hartman,
Thomas Gleixner, desnesn
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <f61f9a59ddb0f103cd62792e13afde4ca8afa7bb.camel@neuling.org>
[-- Attachment #1: Type: text/plain, Size: 1074 bytes --]
On Mon, 2020-02-17 at 09:33 +1100, Michael Neuling wrote:
> On Sat, 2020-02-15 at 02:36 -0300, Leonardo Bras wrote:
> > Before checking for cpu_type == NULL, this same copy happens, so doing
> > it here will just write the same value to the t->oprofile_type
> > again.
> >
> > Remove the repeated copy, as it is unnecessary.
> >
> > Signed-off-by: Leonardo Bras <leonardo@linux.ibm.com>
>
> LGTM
>
> Reviewed-by: Michael Neuling <mikey@neuling.org>
>
Thanks!
> > ---
> > arch/powerpc/kernel/cputable.c | 1 -
> > 1 file changed, 1 deletion(-)
> >
> > diff --git a/arch/powerpc/kernel/cputable.c b/arch/powerpc/kernel/cputable.c
> > index e745abc5457a..5a87ec96582f 100644
> > --- a/arch/powerpc/kernel/cputable.c
> > +++ b/arch/powerpc/kernel/cputable.c
> > @@ -2197,7 +2197,6 @@ static struct cpu_spec * __init setup_cpu_spec(unsigned
> > long offset,
> > */
> > if (old.oprofile_cpu_type != NULL) {
> > t->oprofile_cpu_type = old.oprofile_cpu_type;
> > - t->oprofile_type = old.oprofile_type;
> > }
> > }
> >
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] powerpc: Fix /proc/cpuinfo revision for POWER9P processors
From: Leonardo Bras @ 2020-02-17 14:11 UTC (permalink / raw)
To: Mahesh Salgaonkar, linuxppc-dev; +Cc: Michael Neuling
In-Reply-To: <2f2e88ca614ea1fd37d347a189f89c8bfa6b1c9c.camel@linux.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 1817 bytes --]
On Sat, 2020-02-15 at 03:23 -0300, Leonardo Bras wrote:
> Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com> writes:
>
> Hello Mahesh,
>
> > POWER9P PVR bits are same as that of POWER9. Hence mask off only the
> > relevant bits for the major revision similar to POWER9.
> >
> > Without this patch the cpuinfo output shows 17.0 as revision:
> >
> > $ cat /proc/cpuinfo
> > processor : 0
> > cpu : POWER9P, altivec supported
> > clock : 2950.000000MHz
> > revision : 17.0 (pvr 004f 1100)
> >
> > With this change it reflects the correct revision:
> >
> > $ cat /proc/cpuinfo
> > processor : 0
> > cpu : POWER9P, altivec supported
> > clock : 2633.000000MHz
> > revision : 1.0 (pvr 004f 1100)
> >
> > Signed-off-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
> > ---
> > arch/powerpc/kernel/setup-common.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
> > index 488f1eecc0de..37342ec42cb3 100644
> > --- a/arch/powerpc/kernel/setup-common.c
> > +++ b/arch/powerpc/kernel/setup-common.c
> > @@ -315,6 +315,7 @@ static int show_cpuinfo(struct seq_file *m, void *v)
> > min = pvr & 0xFF;
> > break;
> > case 0x004e: /* POWER9 bits 12-15 give chip type */
> > + case 0x004f: /* POWER9P bits 12-15 give chip type */
> > maj = (pvr >> 8) & 0x0F;
> > min = pvr & 0xFF;
> > break;
>
> Seems fine to me.
>
> FWIW:
> Reviwed-by: Leonardo Bras <leonardo@linux.ibm.com>
Sorry, typo.
Reviewed-by: Leonardo Bras <leonardo@linux.ibm.com>
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] selftests/vm: Fix map_hugetlb length used for testing read and write
From: Leonardo Bras @ 2020-02-17 14:10 UTC (permalink / raw)
To: Christophe Leroy, Michael Ellerman, Shuah Khan
Cc: linux-mm, linuxppc-dev, linux-kernel, linux-kselftest
In-Reply-To: <59cdcd821f794a9cbd8ab315b441a7ee2e43e431.camel@linux.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 2599 bytes --]
On Sat, 2020-02-15 at 03:49 -0300, Leonardo Bras wrote:
> Hello Christophe, thank you for the patch.
>
> On Thu, 2020-02-06 at 08:42 +0000, Christophe Leroy wrote:
> > Commit fa7b9a805c79 ("tools/selftest/vm: allow choosing mem size and
> > page size in map_hugetlb") added the possibility to change the size
> > of memory mapped for the test, but left the read and write test using
> > the default value. This is unnoticed when mapping a length greater
> > than the default one, but segfaults otherwise.
> >
> > Fix read_bytes() and write_bytes() by giving them the real length.
> >
> > Also fix the call to munmap().
> >
> > Fixes: fa7b9a805c79 ("tools/selftest/vm: allow choosing mem size and page size in map_hugetlb")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> > ---
> > tools/testing/selftests/vm/map_hugetlb.c | 14 +++++++-------
> > 1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/tools/testing/selftests/vm/map_hugetlb.c b/tools/testing/selftests/vm/map_hugetlb.c
> > index 5a2d7b8efc40..6af951900aa3 100644
> > --- a/tools/testing/selftests/vm/map_hugetlb.c
> > +++ b/tools/testing/selftests/vm/map_hugetlb.c
> > @@ -45,20 +45,20 @@ static void check_bytes(char *addr)
> > printf("First hex is %x\n", *((unsigned int *)addr));
> > }
> >
> > -static void write_bytes(char *addr)
> > +static void write_bytes(char *addr, size_t length)
> > {
> > unsigned long i;
> >
> > - for (i = 0; i < LENGTH; i++)
> > + for (i = 0; i < length; i++)
> > *(addr + i) = (char)i;
> > }
> >
> > -static int read_bytes(char *addr)
> > +static int read_bytes(char *addr, size_t length)
> > {
> > unsigned long i;
> >
> > check_bytes(addr);
> > - for (i = 0; i < LENGTH; i++)
> > + for (i = 0; i < length; i++)
> > if (*(addr + i) != (char)i) {
> > printf("Mismatch at %lu\n", i);
> > return 1;
> > @@ -96,11 +96,11 @@ int main(int argc, char **argv)
> >
> > printf("Returned address is %p\n", addr);
> > check_bytes(addr);
> > - write_bytes(addr);
> > - ret = read_bytes(addr);
> > + write_bytes(addr, length);
> > + ret = read_bytes(addr, length);
> >
> > /* munmap() length of MAP_HUGETLB memory must be hugepage aligned */
> > - if (munmap(addr, LENGTH)) {
> > + if (munmap(addr, length)) {
> > perror("munmap");
> > exit(1);
> > }
>
> I agree with you, it's a needed fix.
>
> FWIW:
> Reviwed-by: Leonardo Bras <leonardo@linux.ibm.com>
Sorry, typo.
Reviewed-by: Leonardo Bras <leonardo@linux.ibm.com>
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] powerpc/8xx: Fix clearing of bits 20-23 in ITLB miss
From: Leonardo Bras @ 2020-02-17 14:10 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <1d6a53ab-ea72-7452-ea5f-43dd70b223c9@c-s.fr>
[-- Attachment #1: Type: text/plain, Size: 2020 bytes --]
On Sat, 2020-02-15 at 11:17 +0100, Christophe Leroy wrote:
>
> Le 15/02/2020 à 07:28, Leonardo Bras a écrit :
> > On Sun, 2020-02-09 at 18:14 +0000, Christophe Leroy wrote:
> > > In ITLB miss handled the line supposed to clear bits 20-23 on the
> > > L2 ITLB entry is buggy and does indeed nothing, leading to undefined
> > > value which could allow execution when it shouldn't.
> > >
> > > Properly do the clearing with the relevant instruction.
> > >
> > > Fixes: 74fabcadfd43 ("powerpc/8xx: don't use r12/SPRN_SPRG_SCRATCH2 in TLB Miss handlers")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> > > ---
> > > arch/powerpc/kernel/head_8xx.S | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/arch/powerpc/kernel/head_8xx.S b/arch/powerpc/kernel/head_8xx.S
> > > index 9922306ae512..073a651787df 100644
> > > --- a/arch/powerpc/kernel/head_8xx.S
> > > +++ b/arch/powerpc/kernel/head_8xx.S
> > > @@ -256,7 +256,7 @@ InstructionTLBMiss:
> > > * set. All other Linux PTE bits control the behavior
> > > * of the MMU.
> > > */
> > > - rlwimi r10, r10, 0, 0x0f00 /* Clear bits 20-23 */
> > > + rlwinm r10, r10, 0, ~0x0f00 /* Clear bits 20-23 */
> > > rlwimi r10, r10, 4, 0x0400 /* Copy _PAGE_EXEC into bit 21 */
> > > ori r10, r10, RPN_PATTERN | 0x200 /* Set 22 and 24-27 */
> > > mtspr SPRN_MI_RPN, r10 /* Update TLB entry */
> >
> > Looks a valid change.
> > rlwimi r10, r10, 0, 0x0f00 means:
> > r10 = ((r10 << 0) & 0x0f00) | (r10 & ~0x0f00) which ends up being
> > r10 = r10
> >
> > On ISA, rlwinm is recommended for clearing high order bits.
> > rlwinm r10, r10, 0, ~0x0f00 means:
> > r10 = (r10 << 0) & ~0x0f00
> >
> > Which does exactly what the comments suggests.
> >
> > FWIW:
> > Reviwed-by: Leonardo Bras <leonardo@linux.ibm.com>
> >
>
> I guess you mean
>
> Reviewed-by: Leonardo Bras <leonardo@linux.ibm.com>
Yes, sorry for the typo.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [Bug 206525] BUG: KASAN: stack-out-of-bounds in test_bit+0x30/0x44 (kernel 5.6-rc1)
From: bugzilla-daemon @ 2020-02-17 11:52 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <bug-206525-206035@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=206525
Erhard F. (erhard_f@mailbox.org) changed:
What |Removed |Added
----------------------------------------------------------------------------
Component|PPC-32 |Other
Hardware|PPC-32 |All
Product|Platform Specific/Hardware |Networking
--
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* [Bug 206525] BUG: KASAN: stack-out-of-bounds in test_bit+0x30/0x44 (kernel 5.6-rc1)
From: bugzilla-daemon @ 2020-02-17 10:53 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <bug-206525-206035@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=206525
--- Comment #5 from Christophe Leroy (christophe.leroy@c-s.fr) ---
That's not a PPC32 bug but a Network bug affecting all 32 bits architectures.
--
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* Re: [PATCH] powerpc/kprobes: Fix trap address when trap happened in real mode
From: Masami Hiramatsu @ 2020-02-17 10:27 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-kernel, Anil S Keshavamurthy, Paul Mackerras, stable,
Naveen N. Rao, linuxppc-dev, David S. Miller, Larry Finger
In-Reply-To: <baee8186-549a-f6cf-3619-884b6d708185@c-s.fr>
On Mon, 17 Feb 2020 10:03:22 +0100
Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>
>
> Le 16/02/2020 à 13:34, Masami Hiramatsu a écrit :
> > On Sat, 15 Feb 2020 11:28:49 +0100
> > Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> >
> >> Hi,
> >>
> >> Le 14/02/2020 à 14:54, Masami Hiramatsu a écrit :
> >>> Hi,
> >>>
> >>> On Fri, 14 Feb 2020 12:47:49 +0000 (UTC)
> >>> Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> >>>
> >>>> When a program check exception happens while MMU translation is
> >>>> disabled, following Oops happens in kprobe_handler() in the following
> >>>> test:
> >>>>
> >>>> } else if (*addr != BREAKPOINT_INSTRUCTION) {
> >>>
> >>> Thanks for the report and patch. I'm not so sure about powerpc implementation
> >>> but at where the MMU translation is disabled, can the handler work correctly?
> >>> (And where did you put the probe on?)
> >>>
> >>> Your fix may fix this Oops, but if the handler needs special care, it is an
> >>> option to blacklist such place (if possible).
> >>
> >> I guess that's another story. Here we are not talking about a place
> >> where kprobe has been illegitimately activated, but a place where there
> >> is a valid trap, which generated a valid 'program check exception'. And
> >> kprobe was off at that time.
> >
> > Ah, I got it. It is not a kprobe breakpoint, but to check that correctly,
> > it has to know the address where the breakpoint happens. OK.
> >
> >>
> >> As any 'program check exception' due to a trap (ie a BUG_ON, a WARN_ON,
> >> a debugger breakpoint, a perf breakpoint, etc...) calls
> >> kprobe_handler(), kprobe_handler() must be prepared to handle the case
> >> where the MMU translation is disabled, even if probes are not supposed
> >> to be set for functions running with MMU translation disabled.
> >
> > Can't we check the MMU is disabled there (as same as checking the exception
> > happened in user space or not)?
> >
>
> What do you mean by 'there' ? At the entry of kprobe_handler() ?
>
> That's what my patch does, it checks whether MMU is disabled or not. If
> it is, it converts the address to a virtual address.
>
> Do you mean kprobe_handler() should bail out early as it does when the
> trap happens in user mode ?
Yes, that is what I meant.
> Of course we can do that, I don't know
> enough about kprobe to know if kprobe_handler() should manage events
> that happened in real-mode or just ignore them. But I tested adding an
> event on a function that runs in real-mode, and it (now) works.
>
> So, what should we do really ?
I'm not sure how the powerpc kernel runs in real mode.
But clearly, at least kprobe event can not handle that case because
it tries to access memory by probe_kernel_read(). Unless that function
correctly handles the address translation, I want to prohibit kprobes
on such address.
So what I would like to see is, something like below.
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 2d27ec4feee4..4771be152416 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -261,7 +261,7 @@ int kprobe_handler(struct pt_regs *regs)
unsigned int *addr = (unsigned int *)regs->nip;
struct kprobe_ctlblk *kcb;
- if (user_mode(regs))
+ if (user_mode(regs) || !(regs->msr & MSR_IR))
return 0;
/*
Thank you,
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply related
* [PATCH] powerpc/mm: Don't kmap_atomic() in pte_offset_map() on PPC32
From: Christophe Leroy @ 2020-02-17 9:41 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
On PPC32, pte_offset_map() does a kmap_atomic() in order to support
page tables allocated in high memory, just like ARM and x86/32.
But since at least 2008 and commit 8054a3428fbe ("powerpc: Remove dead
CONFIG_HIGHPTE"), page tables are never allocated in high memory.
When the page is in low mem, kmap_atomic() just returns the page
address but still disable preemption and pagefault. And it is
not an inlined function, so we suffer function call for no reason.
Make pte_offset_map() the same as pte_offset_kernel() and make
pte_unmap() void, in the same way as PPC64 which doesn't have HIGHMEM.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/include/asm/book3s/32/pgtable.h | 6 ++----
arch/powerpc/include/asm/nohash/32/pgtable.h | 6 ++----
2 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/32/pgtable.h b/arch/powerpc/include/asm/book3s/32/pgtable.h
index 8bf8cdd92ea5..e490c1012c27 100644
--- a/arch/powerpc/include/asm/book3s/32/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/32/pgtable.h
@@ -367,10 +367,8 @@ static inline void __ptep_set_access_flags(struct vm_area_struct *vma,
(((address) >> PAGE_SHIFT) & (PTRS_PER_PTE - 1))
#define pte_offset_kernel(dir, addr) \
((pte_t *) pmd_page_vaddr(*(dir)) + pte_index(addr))
-#define pte_offset_map(dir, addr) \
- ((pte_t *)(kmap_atomic(pmd_page(*(dir))) + \
- (pmd_page_vaddr(*(dir)) & ~PAGE_MASK)) + pte_index(addr))
-#define pte_unmap(pte) kunmap_atomic(pte)
+#define pte_offset_map(dir, addr) pte_offset_kernel((dir), (addr))
+static inline void pte_unmap(pte_t *pte) { }
/*
* Encode and decode a swap entry.
diff --git a/arch/powerpc/include/asm/nohash/32/pgtable.h b/arch/powerpc/include/asm/nohash/32/pgtable.h
index 5ad7e439dcf4..fcd22737eb5b 100644
--- a/arch/powerpc/include/asm/nohash/32/pgtable.h
+++ b/arch/powerpc/include/asm/nohash/32/pgtable.h
@@ -373,10 +373,8 @@ static inline int pte_young(pte_t pte)
#define pte_offset_kernel(dir, addr) \
(pmd_bad(*(dir)) ? NULL : (pte_t *)pmd_page_vaddr(*(dir)) + \
pte_index(addr))
-#define pte_offset_map(dir, addr) \
- ((pte_t *)(kmap_atomic(pmd_page(*(dir))) + \
- (pmd_page_vaddr(*(dir)) & ~PAGE_MASK)) + pte_index(addr))
-#define pte_unmap(pte) kunmap_atomic(pte)
+#define pte_offset_map(dir, addr) pte_offset_kernel((dir), (addr))
+static inline void pte_unmap(pte_t *pte) { }
/*
* Encode and decode a swap entry.
--
2.25.0
^ permalink raw reply related
* Re: [PATCH 2/5] mm/vma: Make vma_is_accessible() available for general use
From: Anshuman Khandual @ 2020-02-17 9:28 UTC (permalink / raw)
To: linux-mm, linux-kernel
Cc: Rich Felker, Yoshinori Sato, Paul Burton, linux-sh,
Peter Zijlstra, Dave Hansen, linux-mips, Guo Ren, linuxppc-dev,
linux-m68k, Ingo Molnar, Geert Uytterhoeven, Ralf Baechle,
Andy Lutomirski, Paul Mackerras, Thomas Gleixner, Steven Rostedt,
Andrew Morton, Mel Gorman
In-Reply-To: <1581915833-21984-3-git-send-email-anshuman.khandual@arm.com>
On 02/17/2020 10:33 AM, Anshuman Khandual wrote:
> Lets move vma_is_accessible() helper to include/linux/mm.h which makes it
> available for general use. While here, this replaces all remaining open
> encodings for VMA access check with vma_is_accessible().
>
> Cc: Guo Ren <guoren@kernel.org>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
> Cc: Rich Felker <dalias@libc.org>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-m68k@lists.linux-m68k.org
> Cc: linux-mips@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux-sh@vger.kernel.org
> Cc: linux-mm@kvack.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
> arch/csky/mm/fault.c | 2 +-
> arch/m68k/mm/fault.c | 2 +-
> arch/mips/mm/fault.c | 2 +-
> arch/powerpc/mm/fault.c | 2 +-
> arch/sh/mm/fault.c | 2 +-
> arch/x86/mm/fault.c | 2 +-
> include/linux/mm.h | 5 +++++
> kernel/sched/fair.c | 2 +-
> mm/gup.c | 2 +-
> mm/memory.c | 5 -----
> mm/mempolicy.c | 3 +--
> 11 files changed, 14 insertions(+), 15 deletions(-)
There are couple of places in mm/mmap.c which could use vma_is_accessible()
as well. Probably missed them, as the order of the VMA flags were different.
Will fold the following changes next time around.
diff --git a/mm/mmap.c b/mm/mmap.c
index 6756b8bb0033..9b9bb4031fd4 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2338,8 +2338,7 @@ int expand_upwards(struct vm_area_struct *vma, unsigned long address)
gap_addr = TASK_SIZE;
next = vma->vm_next;
- if (next && next->vm_start < gap_addr &&
- (next->vm_flags & (VM_WRITE|VM_READ|VM_EXEC))) {
+ if (next && next->vm_start < gap_addr && vma_is_accessible(next)) {
if (!(next->vm_flags & VM_GROWSUP))
return -ENOMEM;
/* Check that both stack segments have the same anon_vma? */
@@ -2420,7 +2419,7 @@ int expand_downwards(struct vm_area_struct *vma,
prev = vma->vm_prev;
/* Check that both stack segments have the same anon_vma? */
if (prev && !(prev->vm_flags & VM_GROWSDOWN) &&
- (prev->vm_flags & (VM_WRITE|VM_READ|VM_EXEC))) {
+ vma_is_accessible(prev)) {
if (address - prev->vm_end < stack_guard_gap)
return -ENOMEM;
}
^ permalink raw reply related
* Re: [PATCH 3/5] mm/vma: Replace all remaining open encodings with is_vm_hugetlb_page()
From: Anshuman Khandual @ 2020-02-17 9:21 UTC (permalink / raw)
To: linux-mm, linux-kernel
Cc: kvm-ppc, linux-arch, Arnaldo Carvalho de Melo, Arnd Bergmann,
linuxppc-dev, Nick Piggin, Peter Zijlstra, Ingo Molnar,
Alexander Viro, Aneesh Kumar K.V, linux-fsdevel, Andrew Morton,
Will Deacon
In-Reply-To: <1581915833-21984-4-git-send-email-anshuman.khandual@arm.com>
On 02/17/2020 10:33 AM, Anshuman Khandual wrote:
> This replaces all remaining open encodings with is_vm_hugetlb_page().
>
> Cc: Paul Mackerras <paulus@ozlabs.org>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: Will Deacon <will@kernel.org>
> Cc: "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Nick Piggin <npiggin@gmail.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: kvm-ppc@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-fsdevel@vger.kernel.org
> Cc: linux-arch@vger.kernel.org
> Cc: linux-mm@kvack.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
> arch/powerpc/kvm/e500_mmu_host.c | 2 +-
> fs/binfmt_elf.c | 2 +-
> include/asm-generic/tlb.h | 2 +-
> kernel/events/core.c | 3 ++-
> 4 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/kvm/e500_mmu_host.c b/arch/powerpc/kvm/e500_mmu_host.c
> index 425d13806645..3922575a1c31 100644
> --- a/arch/powerpc/kvm/e500_mmu_host.c
> +++ b/arch/powerpc/kvm/e500_mmu_host.c
> @@ -422,7 +422,7 @@ static inline int kvmppc_e500_shadow_map(struct kvmppc_vcpu_e500 *vcpu_e500,
> break;
> }
> } else if (vma && hva >= vma->vm_start &&
> - (vma->vm_flags & VM_HUGETLB)) {
> + (is_vm_hugetlb_page(vma))) {
Additional braces around is_vm_hugetlb_page() can be dropped here.
^ permalink raw reply
* Re: [PATCH] powerpc/kprobes: Fix trap address when trap happened in real mode
From: Christophe Leroy @ 2020-02-17 9:03 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: linux-kernel, Anil S Keshavamurthy, Paul Mackerras, stable,
Naveen N. Rao, linuxppc-dev, David S. Miller, Larry Finger
In-Reply-To: <20200216213411.824295a321d8fa979dedbbbe@kernel.org>
Le 16/02/2020 à 13:34, Masami Hiramatsu a écrit :
> On Sat, 15 Feb 2020 11:28:49 +0100
> Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>
>> Hi,
>>
>> Le 14/02/2020 à 14:54, Masami Hiramatsu a écrit :
>>> Hi,
>>>
>>> On Fri, 14 Feb 2020 12:47:49 +0000 (UTC)
>>> Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>>>
>>>> When a program check exception happens while MMU translation is
>>>> disabled, following Oops happens in kprobe_handler() in the following
>>>> test:
>>>>
>>>> } else if (*addr != BREAKPOINT_INSTRUCTION) {
>>>
>>> Thanks for the report and patch. I'm not so sure about powerpc implementation
>>> but at where the MMU translation is disabled, can the handler work correctly?
>>> (And where did you put the probe on?)
>>>
>>> Your fix may fix this Oops, but if the handler needs special care, it is an
>>> option to blacklist such place (if possible).
>>
>> I guess that's another story. Here we are not talking about a place
>> where kprobe has been illegitimately activated, but a place where there
>> is a valid trap, which generated a valid 'program check exception'. And
>> kprobe was off at that time.
>
> Ah, I got it. It is not a kprobe breakpoint, but to check that correctly,
> it has to know the address where the breakpoint happens. OK.
>
>>
>> As any 'program check exception' due to a trap (ie a BUG_ON, a WARN_ON,
>> a debugger breakpoint, a perf breakpoint, etc...) calls
>> kprobe_handler(), kprobe_handler() must be prepared to handle the case
>> where the MMU translation is disabled, even if probes are not supposed
>> to be set for functions running with MMU translation disabled.
>
> Can't we check the MMU is disabled there (as same as checking the exception
> happened in user space or not)?
>
What do you mean by 'there' ? At the entry of kprobe_handler() ?
That's what my patch does, it checks whether MMU is disabled or not. If
it is, it converts the address to a virtual address.
Do you mean kprobe_handler() should bail out early as it does when the
trap happens in user mode ? Of course we can do that, I don't know
enough about kprobe to know if kprobe_handler() should manage events
that happened in real-mode or just ignore them. But I tested adding an
event on a function that runs in real-mode, and it (now) works.
So, what should we do really ?
Christophe
^ permalink raw reply
* Re: [PATCH AUTOSEL 5.5 096/542] powerpc/powernv/ioda: Fix ref count for devices with their own PE
From: Frederic Barrat @ 2020-02-17 8:49 UTC (permalink / raw)
To: Sasha Levin, linux-kernel, stable; +Cc: linuxppc-dev, Andrew Donnellan
In-Reply-To: <20200214154854.6746-96-sashal@kernel.org>
Le 14/02/2020 à 16:41, Sasha Levin a écrit :
> From: Frederic Barrat <fbarrat@linux.ibm.com>
>
> [ Upstream commit 05dd7da76986937fb288b4213b1fa10dbe0d1b33 ]
Hi,
Upstream commit 05dd7da76986937fb288b4213b1fa10dbe0d1b33 doesn't really
need to go to stable (any of 4.19, 5.4 and 5.5). While it's probably
safe, the patch replaces a refcount leak by another one, which makes
sense as part of the full series merged in 5.6-rc1, but isn't terribly
useful standalone on the current stable branches.
Fred
> The pci_dn structure used to store a pointer to the struct pci_dev, so
> taking a reference on the device was required. However, the pci_dev
> pointer was later removed from the pci_dn structure, but the reference
> was kept for the npu device.
> See commit 902bdc57451c ("powerpc/powernv/idoa: Remove unnecessary
> pcidev from pci_dn").
>
> We don't need to take a reference on the device when assigning the PE
> as the struct pnv_ioda_pe is cleaned up at the same time as
> the (physical) device is released. Doing so prevents the device from
> being released, which is a problem for opencapi devices, since we want
> to be able to remove them through PCI hotplug.
>
> Now the ugly part: nvlink npu devices are not meant to be
> released. Because of the above, we've always leaked a reference and
> simply removing it now is dangerous and would likely require more
> work. There's currently no release device callback for nvlink devices
> for example. So to be safe, this patch leaks a reference on the npu
> device, but only for nvlink and not opencapi.
>
> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> Link: https://lore.kernel.org/r/20191121134918.7155-2-fbarrat@linux.ibm.com
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
> arch/powerpc/platforms/powernv/pci-ioda.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
> index 4374836b033b4..67b836f102402 100644
> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> @@ -1062,14 +1062,13 @@ static struct pnv_ioda_pe *pnv_ioda_setup_dev_PE(struct pci_dev *dev)
> return NULL;
> }
>
> - /* NOTE: We get only one ref to the pci_dev for the pdn, not for the
> - * pointer in the PE data structure, both should be destroyed at the
> - * same time. However, this needs to be looked at more closely again
> - * once we actually start removing things (Hotplug, SR-IOV, ...)
> + /* NOTE: We don't get a reference for the pointer in the PE
> + * data structure, both the device and PE structures should be
> + * destroyed at the same time. However, removing nvlink
> + * devices will need some work.
> *
> * At some point we want to remove the PDN completely anyways
> */
> - pci_dev_get(dev);
> pdn->pe_number = pe->pe_number;
> pe->flags = PNV_IODA_PE_DEV;
> pe->pdev = dev;
> @@ -1084,7 +1083,6 @@ static struct pnv_ioda_pe *pnv_ioda_setup_dev_PE(struct pci_dev *dev)
> pnv_ioda_free_pe(pe);
> pdn->pe_number = IODA_INVALID_PE;
> pe->pdev = NULL;
> - pci_dev_put(dev);
> return NULL;
> }
>
> @@ -1205,6 +1203,14 @@ static struct pnv_ioda_pe *pnv_ioda_setup_npu_PE(struct pci_dev *npu_pdev)
> struct pci_controller *hose = pci_bus_to_host(npu_pdev->bus);
> struct pnv_phb *phb = hose->private_data;
>
> + /*
> + * Intentionally leak a reference on the npu device (for
> + * nvlink only; this is not an opencapi path) to make sure it
> + * never goes away, as it's been the case all along and some
> + * work is needed otherwise.
> + */
> + pci_dev_get(npu_pdev);
> +
> /*
> * Due to a hardware errata PE#0 on the NPU is reserved for
> * error handling. This means we only have three PEs remaining
> @@ -1228,7 +1234,6 @@ static struct pnv_ioda_pe *pnv_ioda_setup_npu_PE(struct pci_dev *npu_pdev)
> */
> dev_info(&npu_pdev->dev,
> "Associating to existing PE %x\n", pe_num);
> - pci_dev_get(npu_pdev);
> npu_pdn = pci_get_pdn(npu_pdev);
> rid = npu_pdev->bus->number << 8 | npu_pdn->devfn;
> npu_pdn->pe_number = pe_num;
>
^ permalink raw reply
* [PATCH v7 12/12] doc/admin-guide: update kernel.rst with CAP_PERFMON information
From: Alexey Budankov @ 2020-02-17 8:13 UTC (permalink / raw)
To: James Morris, Serge Hallyn, Stephen Smalley, Peter Zijlstra,
Arnaldo Carvalho de Melo, Ingo Molnar,
joonas.lahtinen@linux.intel.com, Alexei Starovoitov, Will Deacon,
Paul Mackerras, Helge Deller, Thomas Gleixner
Cc: linux-man, Andi Kleen, linux-parisc@vger.kernel.org,
linux-doc@vger.kernel.org, selinux@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, intel-gfx@lists.freedesktop.org,
Igor Lubashev, linux-kernel, Stephane Eranian,
linux-security-module@vger.kernel.org, oprofile-list, Jiri Olsa,
linux-arm-kernel
In-Reply-To: <c8de937a-0b3a-7147-f5ef-69f467e87a13@linux.intel.com>
Update kernel.rst documentation file with the information
related to usage of CAP_PERFMON capability to secure performance
monitoring and observability operations in system.
Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
Documentation/admin-guide/sysctl/kernel.rst | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
index def074807cee..b06ae9389809 100644
--- a/Documentation/admin-guide/sysctl/kernel.rst
+++ b/Documentation/admin-guide/sysctl/kernel.rst
@@ -720,20 +720,26 @@ perf_event_paranoid:
====================
Controls use of the performance events system by unprivileged
-users (without CAP_SYS_ADMIN). The default value is 2.
+users (without CAP_PERFMON). The default value is 2.
+
+For backward compatibility reasons access to system performance
+monitoring and observability remains open for CAP_SYS_ADMIN
+privileged processes but CAP_SYS_ADMIN usage for secure system
+performance monitoring and observability operations is discouraged
+with respect to CAP_PERFMON use cases.
=== ==================================================================
-1 Allow use of (almost) all events by all users
Ignore mlock limit after perf_event_mlock_kb without CAP_IPC_LOCK
->=0 Disallow ftrace function tracepoint by users without CAP_SYS_ADMIN
+>=0 Disallow ftrace function tracepoint by users without CAP_PERFMON
- Disallow raw tracepoint access by users without CAP_SYS_ADMIN
+ Disallow raw tracepoint access by users without CAP_PERFMON
->=1 Disallow CPU event access by users without CAP_SYS_ADMIN
+>=1 Disallow CPU event access by users without CAP_PERFMON
->=2 Disallow kernel profiling by users without CAP_SYS_ADMIN
+>=2 Disallow kernel profiling by users without CAP_PERFMON
=== ==================================================================
--
2.20.1
^ permalink raw reply related
* [PATCH v7 11/12] doc/admin-guide: update perf-security.rst with CAP_PERFMON information
From: Alexey Budankov @ 2020-02-17 8:12 UTC (permalink / raw)
To: James Morris, Serge Hallyn, Stephen Smalley, Peter Zijlstra,
Arnaldo Carvalho de Melo, Ingo Molnar,
joonas.lahtinen@linux.intel.com, Alexei Starovoitov, Will Deacon,
Paul Mackerras, Helge Deller, Thomas Gleixner
Cc: linux-man, Andi Kleen, linux-parisc@vger.kernel.org,
linux-doc@vger.kernel.org, selinux@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, intel-gfx@lists.freedesktop.org,
Igor Lubashev, linux-kernel, Stephane Eranian,
linux-security-module@vger.kernel.org, oprofile-list, Jiri Olsa,
linux-arm-kernel
In-Reply-To: <c8de937a-0b3a-7147-f5ef-69f467e87a13@linux.intel.com>
Update perf-security.rst documentation file with the information
related to usage of CAP_PERFMON capability to secure performance
monitoring and observability operations in system.
Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
Documentation/admin-guide/perf-security.rst | 65 +++++++++++++--------
1 file changed, 40 insertions(+), 25 deletions(-)
diff --git a/Documentation/admin-guide/perf-security.rst b/Documentation/admin-guide/perf-security.rst
index 72effa7c23b9..81202d46a1ae 100644
--- a/Documentation/admin-guide/perf-security.rst
+++ b/Documentation/admin-guide/perf-security.rst
@@ -1,6 +1,6 @@
.. _perf_security:
-Perf Events and tool security
+Perf events and tool security
=============================
Overview
@@ -42,11 +42,11 @@ categories:
Data that belong to the fourth category can potentially contain
sensitive process data. If PMUs in some monitoring modes capture values
of execution context registers or data from process memory then access
-to such monitoring capabilities requires to be ordered and secured
-properly. So, perf_events/Perf performance monitoring is the subject for
-security access control management [5]_ .
+to such monitoring modes requires to be ordered and secured properly.
+So, perf_events performance monitoring and observability operations is
+the subject for security access control management [5]_ .
-perf_events/Perf access control
+perf_events access control
-------------------------------
To perform security checks, the Linux implementation splits processes
@@ -66,11 +66,25 @@ into distinct units, known as capabilities [6]_ , which can be
independently enabled and disabled on per-thread basis for processes and
files of unprivileged users.
-Unprivileged processes with enabled CAP_SYS_ADMIN capability are treated
+Unprivileged processes with enabled CAP_PERFMON capability are treated
as privileged processes with respect to perf_events performance
-monitoring and bypass *scope* permissions checks in the kernel.
-
-Unprivileged processes using perf_events system call API is also subject
+monitoring and observability operations, thus, bypass *scope* permissions
+checks in the kernel. CAP_PERFMON implements the principal of least
+privilege [13]_ (POSIX 1003.1e: 2.2.2.39) for performance monitoring and
+observability operations in the kernel and provides secure approach to
+perfomance monitoring and observability in the system.
+
+For backward compatibility reasons access to perf_events monitoring and
+observability operations is also open for CAP_SYS_ADMIN privileged
+processes but CAP_SYS_ADMIN usage for secure monitoring and observability
+use cases is discouraged with respect to CAP_PERFMON capability.
+If system audit records [14]_ for a process using perf_events system call
+API contain denial records of acquiring both CAP_PERFMON and CAP_SYS_ADMIN
+capabilities then providing the process with CAP_PERFMON capability singly
+is recommended as the preferred secure approach to resolve double access
+denial logging related to usage of performance monitoring and observability.
+
+Unprivileged processes using perf_events system call are also subject
for PTRACE_MODE_READ_REALCREDS ptrace access mode check [7]_ , whose
outcome determines whether monitoring is permitted. So unprivileged
processes provided with CAP_SYS_PTRACE capability are effectively
@@ -82,14 +96,14 @@ performance analysis of monitored processes or a system. For example,
CAP_SYSLOG capability permits reading kernel space memory addresses from
/proc/kallsyms file.
-perf_events/Perf privileged users
+Privileged Perf users groups
---------------------------------
Mechanisms of capabilities, privileged capability-dumb files [6]_ and
-file system ACLs [10]_ can be used to create a dedicated group of
-perf_events/Perf privileged users who are permitted to execute
-performance monitoring without scope limits. The following steps can be
-taken to create such a group of privileged Perf users.
+file system ACLs [10]_ can be used to create dedicated groups of
+privileged Perf users who are permitted to execute performance monitoring
+and observability without scope limits. The following steps can be
+taken to create such groups of privileged Perf users.
1. Create perf_users group of privileged Perf users, assign perf_users
group to Perf tool executable and limit access to the executable for
@@ -108,30 +122,30 @@ taken to create such a group of privileged Perf users.
-rwxr-x--- 2 root perf_users 11M Oct 19 15:12 perf
2. Assign the required capabilities to the Perf tool executable file and
- enable members of perf_users group with performance monitoring
+ enable members of perf_users group with monitoring and observability
privileges [6]_ :
::
- # setcap "cap_sys_admin,cap_sys_ptrace,cap_syslog=ep" perf
- # setcap -v "cap_sys_admin,cap_sys_ptrace,cap_syslog=ep" perf
+ # setcap "cap_perfmon,cap_sys_ptrace,cap_syslog=ep" perf
+ # setcap -v "cap_perfmon,cap_sys_ptrace,cap_syslog=ep" perf
perf: OK
# getcap perf
- perf = cap_sys_ptrace,cap_sys_admin,cap_syslog+ep
+ perf = cap_sys_ptrace,cap_syslog,cap_perfmon+ep
As a result, members of perf_users group are capable of conducting
-performance monitoring by using functionality of the configured Perf
-tool executable that, when executes, passes perf_events subsystem scope
-checks.
+performance monitoring and observability by using functionality of the
+configured Perf tool executable that, when executes, passes perf_events
+subsystem scope checks.
This specific access control management is only available to superuser
or root running processes with CAP_SETPCAP, CAP_SETFCAP [6]_
capabilities.
-perf_events/Perf unprivileged users
+Unprivileged users
-----------------------------------
-perf_events/Perf *scope* and *access* control for unprivileged processes
+perf_events *scope* and *access* control for unprivileged processes
is governed by perf_event_paranoid [2]_ setting:
-1:
@@ -166,7 +180,7 @@ is governed by perf_event_paranoid [2]_ setting:
perf_event_mlock_kb locking limit is imposed but ignored for
unprivileged processes with CAP_IPC_LOCK capability.
-perf_events/Perf resource control
+Resource control
---------------------------------
Open file descriptors
@@ -227,4 +241,5 @@ Bibliography
.. [10] `<http://man7.org/linux/man-pages/man5/acl.5.html>`_
.. [11] `<http://man7.org/linux/man-pages/man2/getrlimit.2.html>`_
.. [12] `<http://man7.org/linux/man-pages/man5/limits.conf.5.html>`_
-
+.. [13] `<https://sites.google.com/site/fullycapable>`_
+.. [14] `<http://man7.org/linux/man-pages/man8/auditd.8.html>`_
--
2.20.1
^ permalink raw reply related
* [PATCH v7 10/12] drivers/oprofile: open access for CAP_PERFMON privileged process
From: Alexey Budankov @ 2020-02-17 8:12 UTC (permalink / raw)
To: James Morris, Serge Hallyn, Stephen Smalley, Peter Zijlstra,
Arnaldo Carvalho de Melo, Ingo Molnar,
joonas.lahtinen@linux.intel.com, Alexei Starovoitov, Will Deacon,
Paul Mackerras, Helge Deller, Thomas Gleixner
Cc: linux-man, Andi Kleen, linux-parisc@vger.kernel.org,
linux-doc@vger.kernel.org, selinux@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, intel-gfx@lists.freedesktop.org,
Igor Lubashev, linux-kernel, Stephane Eranian,
linux-security-module@vger.kernel.org, oprofile-list, Jiri Olsa,
linux-arm-kernel
In-Reply-To: <c8de937a-0b3a-7147-f5ef-69f467e87a13@linux.intel.com>
Open access to monitoring for CAP_PERFMON privileged process.
Providing the access under CAP_PERFMON capability singly, without
the rest of CAP_SYS_ADMIN credentials, excludes chances to misuse
the credentials and makes operation more secure.
CAP_PERFMON implements the principal of least privilege for performance
monitoring and observability operations (POSIX IEEE 1003.1e 2.2.2.39
principle of least privilege: A security design principle that states
that a process or program be granted only those privileges (e.g.,
capabilities) necessary to accomplish its legitimate function, and only
for the time that such privileges are actually required)
For backward compatibility reasons access to the monitoring remains
open for CAP_SYS_ADMIN privileged processes but CAP_SYS_ADMIN usage
for secure monitoring is discouraged with respect to CAP_PERFMON
capability.
Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
drivers/oprofile/event_buffer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/oprofile/event_buffer.c b/drivers/oprofile/event_buffer.c
index 12ea4a4ad607..6c9edc8bbc95 100644
--- a/drivers/oprofile/event_buffer.c
+++ b/drivers/oprofile/event_buffer.c
@@ -113,7 +113,7 @@ static int event_buffer_open(struct inode *inode, struct file *file)
{
int err = -EPERM;
- if (!capable(CAP_SYS_ADMIN))
+ if (!perfmon_capable())
return -EPERM;
if (test_and_set_bit_lock(0, &buffer_opened))
--
2.20.1
^ 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