From: Ravi Bangoria <hidden> Date: 2021-03-04 05:09:59
As per ISA 3.1, prefixed instruction should not cross 64-byte
boundary. So don't allow Uprobe on such prefixed instruction.
There are two ways probed instruction is changed in mapped pages.
First, when Uprobe is activated, it searches for all the relevant
pages and replace instruction in them. In this case, if that probe
is on the 64-byte unaligned prefixed instruction, error out
directly. Second, when Uprobe is already active and user maps a
relevant page via mmap(), instruction is replaced via mmap() code
path. But because Uprobe is invalid, entire mmap() operation can
not be stopped. In this case just print an error and continue.
Signed-off-by: Ravi Bangoria <redacted>
---
v2: https://lore.kernel.org/r/20210204104703.273429-1-ravi.bangoria@linux.ibm.com
v2->v3:
- Drop restriction for Uprobe on suffix of prefixed instruction.
It needs lot of code change including generic code but what
we get in return is not worth it.
arch/powerpc/kernel/uprobes.c | 8 ++++++++
1 file changed, 8 insertions(+)
From: Naveen N. Rao <hidden> Date: 2021-03-04 05:55:21
On 2021/03/04 10:35AM, Ravi Bangoria wrote:
quoted hunk
As per ISA 3.1, prefixed instruction should not cross 64-byte
boundary. So don't allow Uprobe on such prefixed instruction.
There are two ways probed instruction is changed in mapped pages.
First, when Uprobe is activated, it searches for all the relevant
pages and replace instruction in them. In this case, if that probe
is on the 64-byte unaligned prefixed instruction, error out
directly. Second, when Uprobe is already active and user maps a
relevant page via mmap(), instruction is replaced via mmap() code
path. But because Uprobe is invalid, entire mmap() operation can
not be stopped. In this case just print an error and continue.
Signed-off-by: Ravi Bangoria <redacted>
---
v2: https://lore.kernel.org/r/20210204104703.273429-1-ravi.bangoria@linux.ibm.com
v2->v3:
- Drop restriction for Uprobe on suffix of prefixed instruction.
It needs lot of code change including generic code but what
we get in return is not worth it.
arch/powerpc/kernel/uprobes.c | 8 ++++++++
1 file changed, 8 insertions(+)
@@ -41,6 +41,14 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe,if(addr&0x03)return-EINVAL;+if(!IS_ENABLED(CONFIG_PPC64)||!cpu_has_feature(CPU_FTR_ARCH_31))+return0;
Sorry, I missed this last time, but I think we can drop the above
checks. ppc_inst_prefixed() already factors in the dependency on
CONFIG_PPC64, and I don't think we need to confirm if we're running on a
ISA V3.1 for the below check.
With that:
Acked-by: Naveen N. Rao <redacted>
+
+ if (ppc_inst_prefixed(auprobe->insn) && (addr & 0x3F) == 0x3C) {
+ pr_info_ratelimited("Cannot register a uprobe on 64 byte unaligned prefixed instruction\n");
+ return -EINVAL;
+ }
+
As per ISA 3.1, prefixed instruction should not cross 64-byte
boundary. So don't allow Uprobe on such prefixed instruction.
There are two ways probed instruction is changed in mapped pages.
First, when Uprobe is activated, it searches for all the relevant
pages and replace instruction in them. In this case, if that probe
is on the 64-byte unaligned prefixed instruction, error out
directly. Second, when Uprobe is already active and user maps a
relevant page via mmap(), instruction is replaced via mmap() code
path. But because Uprobe is invalid, entire mmap() operation can
not be stopped. In this case just print an error and continue.
Signed-off-by: Ravi Bangoria <redacted>
---
v2: https://lore.kernel.org/r/20210204104703.273429-1-ravi.bangoria@linux.ibm.com
v2->v3:
- Drop restriction for Uprobe on suffix of prefixed instruction.
It needs lot of code change including generic code but what
we get in return is not worth it.
arch/powerpc/kernel/uprobes.c | 8 ++++++++
1 file changed, 8 insertions(+)
Maybe 3C instead of 4F ? : (addr & 0x3C) == 0x3C
What about
(addr & (SZ_64 - 4)) == SZ_64 - 4 to make it more explicit ?
Or ALIGN(addr, SZ_64) != ALIGN(addr + 8, SZ_64)
+ pr_info_ratelimited("Cannot register a uprobe on 64 byte unaligned prefixed instruction\n");
+ return -EINVAL;
+ }
+
return 0;
}
From: Ravi Bangoria <hidden> Date: 2021-03-04 07:42:21
quoted
@@ -41,6 +41,14 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, if (addr & 0x03) return -EINVAL;+ if (!IS_ENABLED(CONFIG_PPC64) || !cpu_has_feature(CPU_FTR_ARCH_31))+ return 0;
Sorry, I missed this last time, but I think we can drop the above
checks. ppc_inst_prefixed() already factors in the dependency on
CONFIG_PPC64,
Yeah, makes sense. I initially added CONFIG_PPC64 check because
I was using ppc_inst_prefix(x, y) macro which is not available
for !CONFIG_PPC64.
and I don't think we need to confirm if we're running on a
ISA V3.1 for the below check.
Prefixed instructions would be supported only when ARCH_31 is set.
(Ignoring insane scenario where user probes on prefixed instruction
on p10 predecessors). So I guess I still need ARCH_31 check?
Ravi
From: Ravi Bangoria <hidden> Date: 2021-03-04 10:16:31
On 3/4/21 1:02 PM, Christophe Leroy wrote:
Le 04/03/2021 à 06:05, Ravi Bangoria a écrit :
quoted
As per ISA 3.1, prefixed instruction should not cross 64-byte
boundary. So don't allow Uprobe on such prefixed instruction.
There are two ways probed instruction is changed in mapped pages.
First, when Uprobe is activated, it searches for all the relevant
pages and replace instruction in them. In this case, if that probe
is on the 64-byte unaligned prefixed instruction, error out
directly. Second, when Uprobe is already active and user maps a
relevant page via mmap(), instruction is replaced via mmap() code
path. But because Uprobe is invalid, entire mmap() operation can
not be stopped. In this case just print an error and continue.
Signed-off-by: Ravi Bangoria <redacted>
---
v2: https://lore.kernel.org/r/20210204104703.273429-1-ravi.bangoria@linux.ibm.com
v2->v3:
- Drop restriction for Uprobe on suffix of prefixed instruction.
It needs lot of code change including generic code but what
we get in return is not worth it.
arch/powerpc/kernel/uprobes.c | 8 ++++++++
1 file changed, 8 insertions(+)
As per ISA 3.1, prefixed instruction should not cross 64-byte
boundary. So don't allow Uprobe on such prefixed instruction.
There are two ways probed instruction is changed in mapped pages.
First, when Uprobe is activated, it searches for all the relevant
pages and replace instruction in them. In this case, if that probe
is on the 64-byte unaligned prefixed instruction, error out
directly. Second, when Uprobe is already active and user maps a
relevant page via mmap(), instruction is replaced via mmap() code
path. But because Uprobe is invalid, entire mmap() operation can
not be stopped. In this case just print an error and continue.
Signed-off-by: Ravi Bangoria <redacted>
---
v2: https://lore.kernel.org/r/20210204104703.273429-1-ravi.bangoria@linux.ibm.com
v2->v3:
- Drop restriction for Uprobe on suffix of prefixed instruction.
It needs lot of code change including generic code but what
we get in return is not worth it.
arch/powerpc/kernel/uprobes.c | 8 ++++++++
1 file changed, 8 insertions(+)
Sorry I meant 3c instead of 3f (And usually we don't use capital letters for that).
The last two bits are supposed to always be 0, so it doesn't really matter, I just thought it would
look better having the same value both sides of the test, ie (addr & 0x3c) == 0x3c.
quoted
What about
(addr & (SZ_64 - 4)) == SZ_64 - 4 to make it more explicit ?
Yes this is bit better. Though, it should be:
(addr & (SZ_64 - 1)) == SZ_64 - 4
-1 or -4 should give the same results as instructions are always 32 bits aligned though.
Christophe
From: Ravi Bangoria <hidden> Date: 2021-03-04 11:05:03
On 3/4/21 4:21 PM, Christophe Leroy wrote:
Le 04/03/2021 à 11:13, Ravi Bangoria a écrit :
quoted
On 3/4/21 1:02 PM, Christophe Leroy wrote:
quoted
Le 04/03/2021 à 06:05, Ravi Bangoria a écrit :
quoted
As per ISA 3.1, prefixed instruction should not cross 64-byte
boundary. So don't allow Uprobe on such prefixed instruction.
There are two ways probed instruction is changed in mapped pages.
First, when Uprobe is activated, it searches for all the relevant
pages and replace instruction in them. In this case, if that probe
is on the 64-byte unaligned prefixed instruction, error out
directly. Second, when Uprobe is already active and user maps a
relevant page via mmap(), instruction is replaced via mmap() code
path. But because Uprobe is invalid, entire mmap() operation can
not be stopped. In this case just print an error and continue.
Signed-off-by: Ravi Bangoria <redacted>
---
v2: https://lore.kernel.org/r/20210204104703.273429-1-ravi.bangoria@linux.ibm.com
v2->v3:
- Drop restriction for Uprobe on suffix of prefixed instruction.
It needs lot of code change including generic code but what
we get in return is not worth it.
arch/powerpc/kernel/uprobes.c | 8 ++++++++
1 file changed, 8 insertions(+)
Sorry I meant 3c instead of 3f (And usually we don't use capital letters for that).
The last two bits are supposed to always be 0, so it doesn't really matter, I just thought it would look better having the same value both sides of the test, ie (addr & 0x3c) == 0x3c.
Ok yeah makes sense. Thanks.
quoted
quoted
What about
(addr & (SZ_64 - 4)) == SZ_64 - 4 to make it more explicit ?
Yes this is bit better. Though, it should be:
(addr & (SZ_64 - 1)) == SZ_64 - 4
-1 or -4 should give the same results as instructions are always 32 bits aligned though.