Re: [PATCH v3 0/9] trace_uprobe: Support SDT markers having reference count (semaphore)
On 04/17/2018 10:02 AM, Ravi Bangoria wrote:
> Userspace Statically Defined Tracepoints[1] are dtrace style markers
> inside userspace applications. Applications like PostgreSQL, MySQL,
> Pthread, Perl, Python, Java, Ruby, Node.js, libvirt, QEMU, glib etc
> have these markers embedded in them. These markers are added by developer
> at important places in the code. Each marker source expands to a single
> nop instruction in the compiled code but there may be additional
> overhead for computing the marker arguments which expands to couple of
> instructions. In case the overhead is more, execution of it can be
> omitted by runtime if() condition when no one is tracing on the marker:
>
> if (reference_counter > 0) {
> Execute marker instructions;
> }
>
> Default value of reference counter is 0. Tracer has to increment the
> reference counter before tracing on a marker and decrement it when
> done with the tracing.
Hi Oleg, Masami,
Can you please review this :) ?
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Spende für Sie
Mein Name ist RICHARD WAHL. Ich habe eine Spende von 3,5 Millionen Euro für Sie gewonnen. Ich habe $ 533 Millionen Mega Millions Jackpot 2018 gewonnen. Ich habe derzeit keine Krebserkrankungen und mein Gesundheitszustand ist schlecht. Bitte mailen Sie mich an on- (richardwah...@gmail.com) -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6] mm, arm64: untag user addresses in mm/gup.c
On Wed, May 2, 2018 at 7:25 PM, Andrey Konovalov wrote: > On Wed, May 2, 2018 at 5:36 PM, Kirill A. Shutemov > wrote: >> On Wed, May 02, 2018 at 02:38:42PM +, Andrey Konovalov wrote: >>> > Does having a tagged address here makes any difference? I couldn't hit a >>> > failure with my simple tests (LD_PRELOAD a library that randomly adds >>> > tags to pointers returned by malloc). >>> >>> I think you're right, follow_page_mask is only called from >>> __get_user_pages, which already untagged the address. I'll remove >>> untagging here. >> >> It also called from follow_page(). Have you covered all its callers? > > Oh, missed that, will take a look. I wasn't able to find anything that calls follow_page with pointers passed from userspace except for the memory subsystem syscalls, and we deliberately don't add untagging in those. -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/6] uaccess: add untagged_addr definition for other arches
To allow arm64 syscalls accept tagged pointers from userspace, we must
untag them when they are passed to the kernel. Since untagging is done in
generic parts of the kernel (like the mm subsystem), the untagged_addr
macro should be defined for all architectures.
Define it as a noop for other architectures besides arm64.
Signed-off-by: Andrey Konovalov
---
include/linux/uaccess.h | 4
1 file changed, 4 insertions(+)
diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index efe79c1cdd47..c045b4eff95e 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -13,6 +13,10 @@
#include
+#ifndef untagged_addr
+#define untagged_addr(addr) addr
+#endif
+
/*
* Architectures should provide two primitives (raw_copy_{to,from}_user())
* and get rid of their private instances of copy_{to,from}_user() and
--
2.17.0.441.gb46fe60e1d-goog
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/6] arm64: untag user pointers passed to the kernel
Hi! arm64 has a feature called Top Byte Ignore, which allows to embed pointer tags into the top byte of each pointer. Userspace programs (such as HWASan, a memory debugging tool [1]) might use this feature and pass tagged user pointers to the kernel through syscalls or other interfaces. This patch makes a few of the kernel interfaces accept tagged user pointers. The kernel is already able to handle user faults with tagged pointers and has the untagged_addr macro, which this patchset reuses. We're not trying to cover all possible ways the kernel accepts user pointers in one patchset, so this one should be considered as a start. Thanks! [1] http://clang.llvm.org/docs/HardwareAssistedAddressSanitizerDesign.html Changes in v2: - Rebased onto 2d618bdf (4.17-rc3+). - Removed excessive untagging in gup.c. - Removed untagging pointers returned from __uaccess_mask_ptr. Changes in v1: - Rebased onto 4.17-rc1. Changes in RFC v2: - Added "#ifndef untagged_addr..." fallback in linux/uaccess.h instead of defining it for each arch individually. - Updated Documentation/arm64/tagged-pointers.txt. - Dropped “mm, arm64: untag user addresses in memory syscalls”. - Rebased onto 3eb2ce82 (4.16-rc7). Andrey Konovalov (6): arm64: add type casts to untagged_addr macro uaccess: add untagged_addr definition for other arches arm64: untag user addresses in access_ok and __uaccess_mask_ptr mm, arm64: untag user addresses in mm/gup.c lib, arm64: untag addrs passed to strncpy_from_user and strnlen_user arm64: update Documentation/arm64/tagged-pointers.txt Documentation/arm64/tagged-pointers.txt | 5 +++-- arch/arm64/include/asm/uaccess.h| 14 +- include/linux/uaccess.h | 4 lib/strncpy_from_user.c | 2 ++ lib/strnlen_user.c | 2 ++ mm/gup.c| 4 6 files changed, 24 insertions(+), 7 deletions(-) -- 2.17.0.441.gb46fe60e1d-goog -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 5/6] lib, arm64: untag addrs passed to strncpy_from_user and strnlen_user
strncpy_from_user and strnlen_user accept user addresses as arguments, and
do not go through the same path as copy_from_user and others, so here we
need to handle the case of tagged user addresses separately.
Untag user pointers passed to these functions.
Signed-off-by: Andrey Konovalov
---
lib/strncpy_from_user.c | 2 ++
lib/strnlen_user.c | 2 ++
2 files changed, 4 insertions(+)
diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c
index b53e1b5d80f4..97467cd2bc59 100644
--- a/lib/strncpy_from_user.c
+++ b/lib/strncpy_from_user.c
@@ -106,6 +106,8 @@ long strncpy_from_user(char *dst, const char __user *src,
long count)
if (unlikely(count <= 0))
return 0;
+ src = untagged_addr(src);
+
max_addr = user_addr_max();
src_addr = (unsigned long)src;
if (likely(src_addr < max_addr)) {
diff --git a/lib/strnlen_user.c b/lib/strnlen_user.c
index 60d0bbda8f5e..8b5f56466e00 100644
--- a/lib/strnlen_user.c
+++ b/lib/strnlen_user.c
@@ -108,6 +108,8 @@ long strnlen_user(const char __user *str, long count)
if (unlikely(count <= 0))
return 0;
+ str = untagged_addr(str);
+
max_addr = user_addr_max();
src_addr = (unsigned long)str;
if (likely(src_addr < max_addr)) {
--
2.17.0.441.gb46fe60e1d-goog
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/6] arm64: untag user addresses in access_ok and __uaccess_mask_ptr
copy_from_user (and a few other similar functions) are used to copy data from user memory into the kernel memory or vice versa. Since a user can provided a tagged pointer to one of the syscalls that use copy_from_user, we need to correctly handle such pointers. Do this by untagging user pointers in access_ok and in __uaccess_mask_ptr, before performing access validity checks. Signed-off-by: Andrey Konovalov --- arch/arm64/include/asm/uaccess.h | 11 +++ 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h index 2d6451cbaa86..fa7318d3d7d5 100644 --- a/arch/arm64/include/asm/uaccess.h +++ b/arch/arm64/include/asm/uaccess.h @@ -105,7 +105,8 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si #define untagged_addr(addr)\ ((__typeof__(addr))sign_extend64((__u64)(addr), 55)) -#define access_ok(type, addr, size)__range_ok(addr, size) +#define access_ok(type, addr, size)\ + __range_ok(untagged_addr(addr), size) #define user_addr_max get_fs #define _ASM_EXTABLE(from, to) \ @@ -237,7 +238,8 @@ static inline void uaccess_enable_not_uao(void) /* * Sanitise a uaccess pointer such that it becomes NULL if above the - * current addr_limit. + * current addr_limit. In case the pointer is tagged (has the top byte set), + * untag the pointer before checking. */ #define uaccess_mask_ptr(ptr) (__typeof__(ptr))__uaccess_mask_ptr(ptr) static inline void __user *__uaccess_mask_ptr(const void __user *ptr) @@ -245,10 +247,11 @@ static inline void __user *__uaccess_mask_ptr(const void __user *ptr) void __user *safe_ptr; asm volatile( - " bicsxzr, %1, %2\n" + " bicsxzr, %3, %2\n" " csel%0, %1, xzr, eq\n" : "=&r" (safe_ptr) - : "r" (ptr), "r" (current_thread_info()->addr_limit) + : "r" (ptr), "r" (current_thread_info()->addr_limit), + "r" (untagged_addr(ptr)) : "cc"); csdb(); -- 2.17.0.441.gb46fe60e1d-goog -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 6/6] arm64: update Documentation/arm64/tagged-pointers.txt
Add a note that work on passing tagged user pointers to the kernel via syscalls has started, but might not be complete yet. Signed-off-by: Andrey Konovalov --- Documentation/arm64/tagged-pointers.txt | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Documentation/arm64/tagged-pointers.txt b/Documentation/arm64/tagged-pointers.txt index a25a99e82bb1..361481283f00 100644 --- a/Documentation/arm64/tagged-pointers.txt +++ b/Documentation/arm64/tagged-pointers.txt @@ -35,8 +35,9 @@ Using non-zero address tags in any of these locations may result in an error code being returned, a (fatal) signal being raised, or other modes of failure. -For these reasons, passing non-zero address tags to the kernel via -system calls is forbidden, and using a non-zero address tag for sp is +Some initial work for supporting non-zero address tags passed to the +kernel via system calls has been done, but the kernel doesn't provide +any guarantees at this point. Using a non-zero address tag for sp is strongly discouraged. Programs maintaining a frame pointer and frame records that use non-zero -- 2.17.0.441.gb46fe60e1d-goog -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 4/6] mm, arm64: untag user addresses in mm/gup.c
mm/gup.c provides a kernel interface that accepts user addresses and manipulates user pages directly (for example get_user_pages, that is used by the futex syscall). Here we also need to handle the case of tagged user pointers. Add untagging to gup.c functions that use user pointers for vma lookup. Signed-off-by: Andrey Konovalov --- mm/gup.c | 4 1 file changed, 4 insertions(+) diff --git a/mm/gup.c b/mm/gup.c index 76af4cfeaf68..65a9566c96d3 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -647,6 +647,8 @@ static long __get_user_pages(struct task_struct *tsk, struct mm_struct *mm, if (!nr_pages) return 0; + start = untagged_addr(start); + VM_BUG_ON(!!pages != !!(gup_flags & FOLL_GET)); /* @@ -801,6 +803,8 @@ int fixup_user_fault(struct task_struct *tsk, struct mm_struct *mm, struct vm_area_struct *vma; int ret, major = 0; + address = untagged_addr(address); + if (unlocked) fault_flags |= FAULT_FLAG_ALLOW_RETRY; -- 2.17.0.441.gb46fe60e1d-goog -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/6] arm64: add type casts to untagged_addr macro
This patch makes the untagged_addr macro accept all kinds of address types (void *, unsigned long, etc.) and allows not to specify type casts in each place where it is used. This is done by using __typeof__. Signed-off-by: Andrey Konovalov --- arch/arm64/include/asm/uaccess.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h index e66b0fca99c2..2d6451cbaa86 100644 --- a/arch/arm64/include/asm/uaccess.h +++ b/arch/arm64/include/asm/uaccess.h @@ -102,7 +102,8 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si * up with a tagged userland pointer. Clear the tag to get a sane pointer to * pass on to access_ok(), for instance. */ -#define untagged_addr(addr)sign_extend64(addr, 55) +#define untagged_addr(addr)\ + ((__typeof__(addr))sign_extend64((__u64)(addr), 55)) #define access_ok(type, addr, size)__range_ok(addr, size) #define user_addr_max get_fs -- 2.17.0.441.gb46fe60e1d-goog -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 9/9] perf probe: Support SDT markers having reference counter (semaphore)
On Tue, 17 Apr 2018 10:02:44 +0530
Ravi Bangoria wrote:
> From: Ravi Bangoria
>
> With this, perf buildid-cache will save SDT markers with reference
> counter in probe cache. Perf probe will be able to probe markers
> having reference counter. Ex,
>
> # readelf -n /tmp/tick | grep -A1 loop2
> Name: loop2
> ... Semaphore: 0x10020036
>
> # ./perf buildid-cache --add /tmp/tick
> # ./perf probe sdt_tick:loop2
> # ./perf stat -e sdt_tick:loop2 /tmp/tick
> hi: 0
> hi: 1
> hi: 2
> ^C
> Performance counter stats for '/tmp/tick':
> 3 sdt_tick:loop2
>2.561851452 seconds time elapsed
>
> Signed-off-by: Ravi Bangoria
Looks good to me.
Acked-by: Masami Hiramatsu
Thanks!
> ---
> tools/perf/util/probe-event.c | 39
> tools/perf/util/probe-event.h | 1 +
> tools/perf/util/probe-file.c | 34 ++--
> tools/perf/util/probe-file.h | 1 +
> tools/perf/util/symbol-elf.c | 46
> ---
> tools/perf/util/symbol.h | 7 +++
> 6 files changed, 106 insertions(+), 22 deletions(-)
>
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index e1dbc98..9b9c26e 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -1832,6 +1832,12 @@ int parse_probe_trace_command(const char *cmd, struct
> probe_trace_event *tev)
> tp->offset = strtoul(fmt2_str, NULL, 10);
> }
>
> + if (tev->uprobes) {
> + fmt2_str = strchr(p, '(');
> + if (fmt2_str)
> + tp->ref_ctr_offset = strtoul(fmt2_str + 1, NULL, 0);
> + }
> +
> tev->nargs = argc - 2;
> tev->args = zalloc(sizeof(struct probe_trace_arg) * tev->nargs);
> if (tev->args == NULL) {
> @@ -2025,6 +2031,22 @@ static int synthesize_probe_trace_arg(struct
> probe_trace_arg *arg,
> return err;
> }
>
> +static int
> +synthesize_uprobe_trace_def(struct probe_trace_event *tev, struct strbuf
> *buf)
> +{
> + struct probe_trace_point *tp = &tev->point;
> + int err;
> +
> + err = strbuf_addf(buf, "%s:0x%lx", tp->module, tp->address);
> +
> + if (err >= 0 && tp->ref_ctr_offset) {
> + if (!uprobe_ref_ctr_is_supported())
> + return -1;
> + err = strbuf_addf(buf, "(0x%lx)", tp->ref_ctr_offset);
> + }
> + return err >= 0 ? 0 : -1;
> +}
> +
> char *synthesize_probe_trace_command(struct probe_trace_event *tev)
> {
> struct probe_trace_point *tp = &tev->point;
> @@ -2054,15 +2076,17 @@ char *synthesize_probe_trace_command(struct
> probe_trace_event *tev)
> }
>
> /* Use the tp->address for uprobes */
> - if (tev->uprobes)
> - err = strbuf_addf(&buf, "%s:0x%lx", tp->module, tp->address);
> - else if (!strncmp(tp->symbol, "0x", 2))
> + if (tev->uprobes) {
> + err = synthesize_uprobe_trace_def(tev, &buf);
> + } else if (!strncmp(tp->symbol, "0x", 2)) {
> /* Absolute address. See try_to_find_absolute_address() */
> err = strbuf_addf(&buf, "%s%s0x%lx", tp->module ?: "",
> tp->module ? ":" : "", tp->address);
> - else
> + } else {
> err = strbuf_addf(&buf, "%s%s%s+%lu", tp->module ?: "",
> tp->module ? ":" : "", tp->symbol, tp->offset);
> + }
> +
> if (err)
> goto error;
>
> @@ -2646,6 +2670,13 @@ static void warn_uprobe_event_compat(struct
> probe_trace_event *tev)
> {
> int i;
> char *buf = synthesize_probe_trace_command(tev);
> + struct probe_trace_point *tp = &tev->point;
> +
> + if (tp->ref_ctr_offset && !uprobe_ref_ctr_is_supported()) {
> + pr_warning("A semaphore is associated with %s:%s and "
> +"seems your kernel doesn't support it.\n",
> +tev->group, tev->event);
> + }
>
> /* Old uprobe event doesn't support memory dereference */
> if (!tev->uprobes || tev->nargs == 0 || !buf)
> diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h
> index 45b14f0..15a98c3 100644
> --- a/tools/perf/util/probe-event.h
> +++ b/tools/perf/util/probe-event.h
> @@ -27,6 +27,7 @@ struct probe_trace_point {
> char*symbol;/* Base symbol */
> char*module;/* Module name */
> unsigned long offset; /* Offset from symbol */
> + unsigned long ref_ctr_offset; /* SDT reference counter offset */
> unsigned long address;/* Actual address of the trace point */
> boolretprobe; /* Return probe flag */
> };
> diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> index 4ae1123..a17ba6a 100644
> --- a/tools/perf/util/probe-file.c
> +++ b/tools/perf/util/probe-file.c
> @@ -697
Re: [PATCH v3 8/9] trace_uprobe/sdt: Document about reference counter
On Tue, 17 Apr 2018 10:02:43 +0530 Ravi Bangoria wrote: > From: Ravi Bangoria > > Reference counter gate the invocation of probe. If present, > by default reference count is 0. Kernel needs to increment > it before tracing the probe and decrement it when done. This > is identical to semaphore in Userspace Statically Defined > Tracepoints (USDT). > > Document usage of reference counter. > > Signed-off-by: Ravi Bangoria Looks good to me. Acked-by: Masami Hiramatsu Thanks! > --- > Documentation/trace/uprobetracer.txt | 16 +--- > kernel/trace/trace.c | 2 +- > 2 files changed, 14 insertions(+), 4 deletions(-) > > diff --git a/Documentation/trace/uprobetracer.txt > b/Documentation/trace/uprobetracer.txt > index bf526a7c..cb6751d 100644 > --- a/Documentation/trace/uprobetracer.txt > +++ b/Documentation/trace/uprobetracer.txt > @@ -19,15 +19,25 @@ user to calculate the offset of the probepoint in the > object. > > Synopsis of uprobe_tracer > - > - p[:[GRP/]EVENT] PATH:OFFSET [FETCHARGS] : Set a uprobe > - r[:[GRP/]EVENT] PATH:OFFSET [FETCHARGS] : Set a return uprobe (uretprobe) > - -:[GRP/]EVENT : Clear uprobe or uretprobe event > + p[:[GRP/]EVENT] PATH:OFFSET[(REF_CTR_OFFSET)] [FETCHARGS] > + r[:[GRP/]EVENT] PATH:OFFSET[(REF_CTR_OFFSET)] [FETCHARGS] > + -:[GRP/]EVENT > + > + p : Set a uprobe > + r : Set a return uprobe (uretprobe) > + - : Clear uprobe or uretprobe event > >GRP : Group name. If omitted, "uprobes" is the default value. >EVENT : Event name. If omitted, the event name is generated based >on PATH+OFFSET. >PATH : Path to an executable or a library. >OFFSET: Offset where the probe is inserted. > + REF_CTR_OFFSET: Reference counter offset. Optional field. Reference count > + gate the invocation of probe. If present, by default > + reference count is 0. Kernel needs to increment it before > + tracing the probe and decrement it when done. This is > + identical to semaphore in Userspace Statically Defined > + Tracepoints (USDT). > >FETCHARGS : Arguments. Each probe can have up to 128 args. > %REG : Fetch register REG > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > index 300f4ea..d211937 100644 > --- a/kernel/trace/trace.c > +++ b/kernel/trace/trace.c > @@ -4604,7 +4604,7 @@ static int tracing_trace_options_open(struct inode > *inode, struct file *file) >"place (kretprobe): [:][+]|\n" > #endif > #ifdef CONFIG_UPROBE_EVENTS > - "\tplace: :\n" > + " place (uprobe): :[(ref_ctr_offset)]\n" > #endif > "\t args: =fetcharg[:type]\n" > "\t fetcharg: %, @, @[+|-],\n" > -- > 1.8.3.1 > -- Masami Hiramatsu -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/9] mm: Prefix vma_ to vaddr_to_offset() and offset_to_vaddr()
On Tue, 17 Apr 2018 10:02:37 +0530 Ravi Bangoria wrote: > From: Ravi Bangoria > > Make function names more meaningful by adding vma_ prefix > to them. Actually, I would have done this patch before the first one, since the first one makes the functions global. -- Steve > > Signed-off-by: Ravi Bangoria > Reviewed-by: Jérôme Glisse > --- > -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6] mm, arm64: untag user addresses in mm/gup.c
On Thu, May 03, 2018 at 04:09:56PM +0200, Andrey Konovalov wrote: > On Wed, May 2, 2018 at 7:25 PM, Andrey Konovalov > wrote: > > On Wed, May 2, 2018 at 5:36 PM, Kirill A. Shutemov > > wrote: > >> On Wed, May 02, 2018 at 02:38:42PM +, Andrey Konovalov wrote: > >>> > Does having a tagged address here makes any difference? I couldn't hit a > >>> > failure with my simple tests (LD_PRELOAD a library that randomly adds > >>> > tags to pointers returned by malloc). > >>> > >>> I think you're right, follow_page_mask is only called from > >>> __get_user_pages, which already untagged the address. I'll remove > >>> untagging here. > >> > >> It also called from follow_page(). Have you covered all its callers? > > > > Oh, missed that, will take a look. > > I wasn't able to find anything that calls follow_page with pointers > passed from userspace except for the memory subsystem syscalls, and we > deliberately don't add untagging in those. I guess I missed this part, but could you elaborate on this? Why? Not yet or not ever? Also I wounder if we can find (with sparse?) all places where we cast out __user. This would give a nice list of places where to pay attention. -- Kirill A. Shutemov -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6] mm, arm64: untag user addresses in mm/gup.c
On Thu, May 3, 2018 at 5:24 PM, Kirill A. Shutemov wrote: > On Thu, May 03, 2018 at 04:09:56PM +0200, Andrey Konovalov wrote: >> On Wed, May 2, 2018 at 7:25 PM, Andrey Konovalov >> wrote: >> I wasn't able to find anything that calls follow_page with pointers >> passed from userspace except for the memory subsystem syscalls, and we >> deliberately don't add untagging in those. > > I guess I missed this part, but could you elaborate on this? Why? > Not yet or not ever? Check out the discussion here: https://www.spinics.net/lists/arm-kernel/msg640936.html > > Also I wounder if we can find (with sparse?) all places where we cast out > __user. This would give a nice list of places where to pay attention. The way I tested this is I added BUG_ON(top byte tag is set) to find_vma and find_extend_vma and ran a modified version of syzkaller that embeds tags into pointers overnight. The only crashes that I saw were coming from memory subsystem syscalls. I then temporarily added untagging to suppress those crashes (https://gist.github.com/xairy/3aa1f57798fa62522c8ac53fad9b74ca), and didn't see any crashes after that. -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] seccomp: Audit attempts to modify the actions_logged sysctl
On Wed, May 2, 2018 at 2:18 PM, Steve Grubb wrote: > On Wednesday, May 2, 2018 11:53:19 AM EDT Tyler Hicks wrote: >> The decision to log a seccomp action will always be subject to the >> value of the kernel.seccomp.actions_logged sysctl, even for processes >> that are being inspected via the audit subsystem, in an upcoming patch. >> Therefore, we need to emit an audit record on attempts at writing to the >> actions_logged sysctl when auditing is enabled. >> >> This patch updates the write handler for the actions_logged sysctl to >> emit an audit record on attempts to write to the sysctl. Successful >> writes to the sysctl will result in a record that includes a normalized >> list of logged actions in the "actions" field and a "res" field equal to >> 0. Unsuccessful writes to the sysctl will result in a record that >> doesn't include the "actions" field and has a "res" field equal to 1. >> >> Not all unsuccessful writes to the sysctl are audited. For example, an >> audit record will not be emitted if an unprivileged process attempts to >> open the sysctl file for reading since that access control check is not >> part of the sysctl's write handler. >> >> Below are some example audit records when writing various strings to the >> actions_logged sysctl. >> >> Writing "not-a-real-action", when the kernel.seccomp.actions_logged >> sysctl previously was "kill_process kill_thread trap errno trace log", >> emits this audit record: >> >> type=CONFIG_CHANGE msg=audit(1525275273.537:130): op=seccomp-logging >> old-actions=kill_process,kill_thread,trap,errno,trace,log res=0 >> >> If you then write "kill_process kill_thread errno trace log", this audit >> record is emitted: >> >> type=CONFIG_CHANGE msg=audit(1525275310.208:136): op=seccomp-logging >> actions=kill_process,kill_thread,errno,trace,log >> old-actions=kill_process,kill_thread,trap,errno,trace,log res=1 >> >> If you then write the string "log log errno trace kill_process >> kill_thread", which is unordered and contains the log action twice, >> it results in the same actions value as the previous record: >> >> type=CONFIG_CHANGE msg=audit(1525275325.613:142): op=seccomp-logging >> actions=kill_process,kill_thread,errno,trace,log >> old-actions=kill_process,kill_thread,errno,trace,log res=1 >> >> No audit records are generated when reading the actions_logged sysctl. > > ACK for the format of the records. I just wanted to clarify the record format with you Steve ... the "actions" and "old-actions" fields may not be included in the record in cases where there is an error building the action value string, are you okay with that or would you prefer the fields to always be included but with a "?" for the value? -- paul moore www.paul-moore.com -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] seccomp: Audit attempts to modify the actions_logged sysctl
On Thursday, May 3, 2018 4:18:26 PM EDT Paul Moore wrote: > On Wed, May 2, 2018 at 2:18 PM, Steve Grubb wrote: > > On Wednesday, May 2, 2018 11:53:19 AM EDT Tyler Hicks wrote: > >> The decision to log a seccomp action will always be subject to the > >> value of the kernel.seccomp.actions_logged sysctl, even for processes > >> that are being inspected via the audit subsystem, in an upcoming patch. > >> Therefore, we need to emit an audit record on attempts at writing to the > >> actions_logged sysctl when auditing is enabled. > >> > >> This patch updates the write handler for the actions_logged sysctl to > >> emit an audit record on attempts to write to the sysctl. Successful > >> writes to the sysctl will result in a record that includes a normalized > >> list of logged actions in the "actions" field and a "res" field equal to > >> 0. Unsuccessful writes to the sysctl will result in a record that > >> doesn't include the "actions" field and has a "res" field equal to 1. > >> > >> Not all unsuccessful writes to the sysctl are audited. For example, an > >> audit record will not be emitted if an unprivileged process attempts to > >> open the sysctl file for reading since that access control check is not > >> part of the sysctl's write handler. > >> > >> Below are some example audit records when writing various strings to the > >> actions_logged sysctl. > >> > >> Writing "not-a-real-action", when the kernel.seccomp.actions_logged > >> sysctl previously was "kill_process kill_thread trap errno trace log", > >> > >> emits this audit record: > >> type=CONFIG_CHANGE msg=audit(1525275273.537:130): op=seccomp-logging > >> old-actions=kill_process,kill_thread,trap,errno,trace,log res=0 > >> > >> If you then write "kill_process kill_thread errno trace log", this audit > >> > >> record is emitted: > >> type=CONFIG_CHANGE msg=audit(1525275310.208:136): op=seccomp-logging > >> actions=kill_process,kill_thread,errno,trace,log > >> old-actions=kill_process,kill_thread,trap,errno,trace,log res=1 > >> > >> If you then write the string "log log errno trace kill_process > >> kill_thread", which is unordered and contains the log action twice, > >> > >> it results in the same actions value as the previous record: > >> type=CONFIG_CHANGE msg=audit(1525275325.613:142): op=seccomp-logging > >> actions=kill_process,kill_thread,errno,trace,log > >> old-actions=kill_process,kill_thread,errno,trace,log res=1 > >> > >> No audit records are generated when reading the actions_logged sysctl. > > > > ACK for the format of the records. > > I just wanted to clarify the record format with you Steve ... the > "actions" and "old-actions" fields may not be included in the record > in cases where there is an error building the action value string, are > you okay with that or would you prefer the fields to always be > included but with a "?" for the value? A ? would be more in line with how other things are handled. -Steve -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] seccomp: Audit attempts to modify the actions_logged sysctl
On Thu, May 3, 2018 at 4:42 PM, Steve Grubb wrote: > On Thursday, May 3, 2018 4:18:26 PM EDT Paul Moore wrote: >> On Wed, May 2, 2018 at 2:18 PM, Steve Grubb wrote: >> > On Wednesday, May 2, 2018 11:53:19 AM EDT Tyler Hicks wrote: >> >> The decision to log a seccomp action will always be subject to the >> >> value of the kernel.seccomp.actions_logged sysctl, even for processes >> >> that are being inspected via the audit subsystem, in an upcoming patch. >> >> Therefore, we need to emit an audit record on attempts at writing to the >> >> actions_logged sysctl when auditing is enabled. >> >> >> >> This patch updates the write handler for the actions_logged sysctl to >> >> emit an audit record on attempts to write to the sysctl. Successful >> >> writes to the sysctl will result in a record that includes a normalized >> >> list of logged actions in the "actions" field and a "res" field equal to >> >> 0. Unsuccessful writes to the sysctl will result in a record that >> >> doesn't include the "actions" field and has a "res" field equal to 1. >> >> >> >> Not all unsuccessful writes to the sysctl are audited. For example, an >> >> audit record will not be emitted if an unprivileged process attempts to >> >> open the sysctl file for reading since that access control check is not >> >> part of the sysctl's write handler. >> >> >> >> Below are some example audit records when writing various strings to the >> >> actions_logged sysctl. >> >> >> >> Writing "not-a-real-action", when the kernel.seccomp.actions_logged >> >> sysctl previously was "kill_process kill_thread trap errno trace log", >> >> >> >> emits this audit record: >> >> type=CONFIG_CHANGE msg=audit(1525275273.537:130): op=seccomp-logging >> >> old-actions=kill_process,kill_thread,trap,errno,trace,log res=0 >> >> >> >> If you then write "kill_process kill_thread errno trace log", this audit >> >> >> >> record is emitted: >> >> type=CONFIG_CHANGE msg=audit(1525275310.208:136): op=seccomp-logging >> >> actions=kill_process,kill_thread,errno,trace,log >> >> old-actions=kill_process,kill_thread,trap,errno,trace,log res=1 >> >> >> >> If you then write the string "log log errno trace kill_process >> >> kill_thread", which is unordered and contains the log action twice, >> >> >> >> it results in the same actions value as the previous record: >> >> type=CONFIG_CHANGE msg=audit(1525275325.613:142): op=seccomp-logging >> >> actions=kill_process,kill_thread,errno,trace,log >> >> old-actions=kill_process,kill_thread,errno,trace,log res=1 >> >> >> >> No audit records are generated when reading the actions_logged sysctl. >> > >> > ACK for the format of the records. >> >> I just wanted to clarify the record format with you Steve ... the >> "actions" and "old-actions" fields may not be included in the record >> in cases where there is an error building the action value string, are >> you okay with that or would you prefer the fields to always be >> included but with a "?" for the value? > > A ? would be more in line with how other things are handled. That's what I thought. Would you mind putting together a v3 Tyler? :) -- paul moore www.paul-moore.com -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] seccomp: Audit attempts to modify the actions_logged sysctl
On 05/03/2018 03:48 PM, Paul Moore wrote: > On Thu, May 3, 2018 at 4:42 PM, Steve Grubb wrote: >> On Thursday, May 3, 2018 4:18:26 PM EDT Paul Moore wrote: >>> On Wed, May 2, 2018 at 2:18 PM, Steve Grubb wrote: On Wednesday, May 2, 2018 11:53:19 AM EDT Tyler Hicks wrote: > The decision to log a seccomp action will always be subject to the > value of the kernel.seccomp.actions_logged sysctl, even for processes > that are being inspected via the audit subsystem, in an upcoming patch. > Therefore, we need to emit an audit record on attempts at writing to the > actions_logged sysctl when auditing is enabled. > > This patch updates the write handler for the actions_logged sysctl to > emit an audit record on attempts to write to the sysctl. Successful > writes to the sysctl will result in a record that includes a normalized > list of logged actions in the "actions" field and a "res" field equal to > 0. Unsuccessful writes to the sysctl will result in a record that > doesn't include the "actions" field and has a "res" field equal to 1. > > Not all unsuccessful writes to the sysctl are audited. For example, an > audit record will not be emitted if an unprivileged process attempts to > open the sysctl file for reading since that access control check is not > part of the sysctl's write handler. > > Below are some example audit records when writing various strings to the > actions_logged sysctl. > > Writing "not-a-real-action", when the kernel.seccomp.actions_logged > sysctl previously was "kill_process kill_thread trap errno trace log", > > emits this audit record: > type=CONFIG_CHANGE msg=audit(1525275273.537:130): op=seccomp-logging > old-actions=kill_process,kill_thread,trap,errno,trace,log res=0 > > If you then write "kill_process kill_thread errno trace log", this audit > > record is emitted: > type=CONFIG_CHANGE msg=audit(1525275310.208:136): op=seccomp-logging > actions=kill_process,kill_thread,errno,trace,log > old-actions=kill_process,kill_thread,trap,errno,trace,log res=1 > > If you then write the string "log log errno trace kill_process > kill_thread", which is unordered and contains the log action twice, > > it results in the same actions value as the previous record: > type=CONFIG_CHANGE msg=audit(1525275325.613:142): op=seccomp-logging > actions=kill_process,kill_thread,errno,trace,log > old-actions=kill_process,kill_thread,errno,trace,log res=1 > > No audit records are generated when reading the actions_logged sysctl. ACK for the format of the records. >>> >>> I just wanted to clarify the record format with you Steve ... the >>> "actions" and "old-actions" fields may not be included in the record >>> in cases where there is an error building the action value string, are >>> you okay with that or would you prefer the fields to always be >>> included but with a "?" for the value? >> >> A ? would be more in line with how other things are handled. > > That's what I thought. > > Would you mind putting together a v3 Tyler? :) To be clear, "?" is only to be used when the call to seccomp_names_from_actions_logged() fails, right? If the sysctl write fails for some other reason, such as when an invalid action name is specified, can you confirm that you still want *no* "actions" field, the "old-actions" field to be the value prior to attempting the update to the sysctl, and res to be 0? Tyler signature.asc Description: OpenPGP digital signature
Re: [PATCH v2 3/4] seccomp: Audit attempts to modify the actions_logged sysctl
On Thursday, May 3, 2018 4:51:36 PM EDT Tyler Hicks wrote: > On 05/03/2018 03:48 PM, Paul Moore wrote: > > On Thu, May 3, 2018 at 4:42 PM, Steve Grubb wrote: > >> On Thursday, May 3, 2018 4:18:26 PM EDT Paul Moore wrote: > >>> On Wed, May 2, 2018 at 2:18 PM, Steve Grubb wrote: > On Wednesday, May 2, 2018 11:53:19 AM EDT Tyler Hicks wrote: > > The decision to log a seccomp action will always be subject to the > > value of the kernel.seccomp.actions_logged sysctl, even for processes > > that are being inspected via the audit subsystem, in an upcoming > > patch. > > Therefore, we need to emit an audit record on attempts at writing to > > the > > actions_logged sysctl when auditing is enabled. > > > > This patch updates the write handler for the actions_logged sysctl to > > emit an audit record on attempts to write to the sysctl. Successful > > writes to the sysctl will result in a record that includes a > > normalized > > list of logged actions in the "actions" field and a "res" field equal > > to > > 0. Unsuccessful writes to the sysctl will result in a record that > > doesn't include the "actions" field and has a "res" field equal to 1. > > > > Not all unsuccessful writes to the sysctl are audited. For example, > > an > > audit record will not be emitted if an unprivileged process attempts > > to > > open the sysctl file for reading since that access control check is > > not > > part of the sysctl's write handler. > > > > Below are some example audit records when writing various strings to > > the > > actions_logged sysctl. > > > > Writing "not-a-real-action", when the kernel.seccomp.actions_logged > > sysctl previously was "kill_process kill_thread trap errno trace > > log", > > > > emits this audit record: > > type=CONFIG_CHANGE msg=audit(1525275273.537:130): op=seccomp-logging > > old-actions=kill_process,kill_thread,trap,errno,trace,log res=0 > > > > If you then write "kill_process kill_thread errno trace log", this > > audit > > > > record is emitted: > > type=CONFIG_CHANGE msg=audit(1525275310.208:136): op=seccomp-logging > > actions=kill_process,kill_thread,errno,trace,log > > old-actions=kill_process,kill_thread,trap,errno,trace,log res=1 > > > > If you then write the string "log log errno trace kill_process > > kill_thread", which is unordered and contains the log action twice, > > > > it results in the same actions value as the previous record: > > type=CONFIG_CHANGE msg=audit(1525275325.613:142): op=seccomp-logging > > actions=kill_process,kill_thread,errno,trace,log > > old-actions=kill_process,kill_thread,errno,trace,log res=1 > > > > No audit records are generated when reading the actions_logged > > sysctl. > > ACK for the format of the records. > >>> > >>> I just wanted to clarify the record format with you Steve ... the > >>> "actions" and "old-actions" fields may not be included in the record > >>> in cases where there is an error building the action value string, are > >>> you okay with that or would you prefer the fields to always be > >>> included but with a "?" for the value? > >> > >> A ? would be more in line with how other things are handled. > > > > That's what I thought. > > > > Would you mind putting together a v3 Tyler? :) > > To be clear, "?" is only to be used when the call to > seccomp_names_from_actions_logged() fails, right? Yes and that is a question mark with no quotes in the audit record. > If the sysctl write fails for some other reason, such as when an invalid > action name is specified, can you confirm that you still want *no* > "actions" field, Its best that fields do not disappear. In the case of invalid input, you can just leave the new value as ? so that nothing malicious can be injected into the logs > the "old-actions" field to be the value prior to attempting the update to > the sysctl, and res to be 0? Yes -Steve -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] seccomp: Audit attempts to modify the actions_logged sysctl
On 05/03/2018 04:12 PM, Steve Grubb wrote: > On Thursday, May 3, 2018 4:51:36 PM EDT Tyler Hicks wrote: >> On 05/03/2018 03:48 PM, Paul Moore wrote: >>> On Thu, May 3, 2018 at 4:42 PM, Steve Grubb wrote: On Thursday, May 3, 2018 4:18:26 PM EDT Paul Moore wrote: > On Wed, May 2, 2018 at 2:18 PM, Steve Grubb wrote: >> On Wednesday, May 2, 2018 11:53:19 AM EDT Tyler Hicks wrote: >>> The decision to log a seccomp action will always be subject to the >>> value of the kernel.seccomp.actions_logged sysctl, even for processes >>> that are being inspected via the audit subsystem, in an upcoming >>> patch. >>> Therefore, we need to emit an audit record on attempts at writing to >>> the >>> actions_logged sysctl when auditing is enabled. >>> >>> This patch updates the write handler for the actions_logged sysctl to >>> emit an audit record on attempts to write to the sysctl. Successful >>> writes to the sysctl will result in a record that includes a >>> normalized >>> list of logged actions in the "actions" field and a "res" field equal >>> to >>> 0. Unsuccessful writes to the sysctl will result in a record that >>> doesn't include the "actions" field and has a "res" field equal to 1. >>> >>> Not all unsuccessful writes to the sysctl are audited. For example, >>> an >>> audit record will not be emitted if an unprivileged process attempts >>> to >>> open the sysctl file for reading since that access control check is >>> not >>> part of the sysctl's write handler. >>> >>> Below are some example audit records when writing various strings to >>> the >>> actions_logged sysctl. >>> >>> Writing "not-a-real-action", when the kernel.seccomp.actions_logged >>> sysctl previously was "kill_process kill_thread trap errno trace >>> log", >>> >>> emits this audit record: >>> type=CONFIG_CHANGE msg=audit(1525275273.537:130): op=seccomp-logging >>> old-actions=kill_process,kill_thread,trap,errno,trace,log res=0 >>> >>> If you then write "kill_process kill_thread errno trace log", this >>> audit >>> >>> record is emitted: >>> type=CONFIG_CHANGE msg=audit(1525275310.208:136): op=seccomp-logging >>> actions=kill_process,kill_thread,errno,trace,log >>> old-actions=kill_process,kill_thread,trap,errno,trace,log res=1 >>> >>> If you then write the string "log log errno trace kill_process >>> kill_thread", which is unordered and contains the log action twice, >>> >>> it results in the same actions value as the previous record: >>> type=CONFIG_CHANGE msg=audit(1525275325.613:142): op=seccomp-logging >>> actions=kill_process,kill_thread,errno,trace,log >>> old-actions=kill_process,kill_thread,errno,trace,log res=1 >>> >>> No audit records are generated when reading the actions_logged >>> sysctl. >> >> ACK for the format of the records. > > I just wanted to clarify the record format with you Steve ... the > "actions" and "old-actions" fields may not be included in the record > in cases where there is an error building the action value string, are > you okay with that or would you prefer the fields to always be > included but with a "?" for the value? A ? would be more in line with how other things are handled. >>> >>> That's what I thought. >>> >>> Would you mind putting together a v3 Tyler? :) >> >> To be clear, "?" is only to be used when the call to >> seccomp_names_from_actions_logged() fails, right? > > Yes and that is a question mark with no quotes in the audit record. > >> If the sysctl write fails for some other reason, such as when an invalid >> action name is specified, can you confirm that you still want *no* >> "actions" field, > > Its best that fields do not disappear. In the case of invalid input, you can > just leave the new value as ? so that nothing malicious can be injected into > the logs > >> the "old-actions" field to be the value prior to attempting the update to >> the sysctl, and res to be 0? > > Yes I came up with one more question after hitting a corner case while testing. It is valid to write an empty string to the sysctl. If the sysctl was set to "errno" and then later set to "", you'd see this with the current revision: type=CONFIG_CHANGE msg=audit(1525385824.643:173): op=seccomp-logging actions= old-actions=errno res=1 Is that what you want or should the value of the "actions" field be something be something like this: actions=(none) Tyler signature.asc Description: OpenPGP digital signature
Re: [PATCH v2 3/4] seccomp: Audit attempts to modify the actions_logged sysctl
On Thursday, May 3, 2018 6:36:18 PM EDT Tyler Hicks wrote: > On 05/03/2018 04:12 PM, Steve Grubb wrote: > > On Thursday, May 3, 2018 4:51:36 PM EDT Tyler Hicks wrote: > >> On 05/03/2018 03:48 PM, Paul Moore wrote: > >>> On Thu, May 3, 2018 at 4:42 PM, Steve Grubb wrote: > On Thursday, May 3, 2018 4:18:26 PM EDT Paul Moore wrote: > > On Wed, May 2, 2018 at 2:18 PM, Steve Grubb wrote: > >> On Wednesday, May 2, 2018 11:53:19 AM EDT Tyler Hicks wrote: > >>> The decision to log a seccomp action will always be subject to the > >>> value of the kernel.seccomp.actions_logged sysctl, even for > >>> processes > >>> that are being inspected via the audit subsystem, in an upcoming > >>> patch. > >>> Therefore, we need to emit an audit record on attempts at writing > >>> to > >>> the > >>> actions_logged sysctl when auditing is enabled. > >>> > >>> This patch updates the write handler for the actions_logged sysctl > >>> to > >>> emit an audit record on attempts to write to the sysctl. Successful > >>> writes to the sysctl will result in a record that includes a > >>> normalized > >>> list of logged actions in the "actions" field and a "res" field > >>> equal > >>> to > >>> 0. Unsuccessful writes to the sysctl will result in a record that > >>> doesn't include the "actions" field and has a "res" field equal to > >>> 1. > >>> > >>> Not all unsuccessful writes to the sysctl are audited. For example, > >>> an > >>> audit record will not be emitted if an unprivileged process > >>> attempts > >>> to > >>> open the sysctl file for reading since that access control check is > >>> not > >>> part of the sysctl's write handler. > >>> > >>> Below are some example audit records when writing various strings > >>> to > >>> the > >>> actions_logged sysctl. > >>> > >>> Writing "not-a-real-action", when the kernel.seccomp.actions_logged > >>> sysctl previously was "kill_process kill_thread trap errno trace > >>> log", > >>> > >>> emits this audit record: > >>> type=CONFIG_CHANGE msg=audit(1525275273.537:130): > >>> op=seccomp-logging > >>> old-actions=kill_process,kill_thread,trap,errno,trace,log res=0 > >>> > >>> If you then write "kill_process kill_thread errno trace log", this > >>> audit > >>> > >>> record is emitted: > >>> type=CONFIG_CHANGE msg=audit(1525275310.208:136): > >>> op=seccomp-logging > >>> actions=kill_process,kill_thread,errno,trace,log > >>> old-actions=kill_process,kill_thread,trap,errno,trace,log res=1 > >>> > >>> If you then write the string "log log errno trace kill_process > >>> kill_thread", which is unordered and contains the log action twice, > >>> > >>> it results in the same actions value as the previous record: > >>> type=CONFIG_CHANGE msg=audit(1525275325.613:142): > >>> op=seccomp-logging > >>> actions=kill_process,kill_thread,errno,trace,log > >>> old-actions=kill_process,kill_thread,errno,trace,log res=1 > >>> > >>> No audit records are generated when reading the actions_logged > >>> sysctl. > >> > >> ACK for the format of the records. > > > > I just wanted to clarify the record format with you Steve ... the > > "actions" and "old-actions" fields may not be included in the record > > in cases where there is an error building the action value string, > > are > > you okay with that or would you prefer the fields to always be > > included but with a "?" for the value? > > A ? would be more in line with how other things are handled. > >>> > >>> That's what I thought. > >>> > >>> Would you mind putting together a v3 Tyler? :) > >> > >> To be clear, "?" is only to be used when the call to > >> seccomp_names_from_actions_logged() fails, right? > > > > Yes and that is a question mark with no quotes in the audit record. > > > >> If the sysctl write fails for some other reason, such as when an invalid > >> action name is specified, can you confirm that you still want *no* > >> "actions" field, > > > > Its best that fields do not disappear. In the case of invalid input, you > > can just leave the new value as ? so that nothing malicious can be > > injected into the logs > > > >> the "old-actions" field to be the value prior to attempting the update > >> to the sysctl, and res to be 0? > > > > Yes > > I came up with one more question after hitting a corner case while testing. > > It is valid to write an empty string to the sysctl. If the sysctl was > set to "errno" and then later set to "", you'd see this with the current > revision: > > type=CONFIG_CHANGE msg=audit(1525385824.643:173): op=seccomp-logging > actions= old-actions=errno res=1 > > Is that what you want or should the value of the "actions" field be > something be something like this: > > actions=(
Re: [PATCH v4 2/2] ThunderX2: Add Cavium ThunderX2 SoC UNCORE PMU driver
On Tue, 1 May 2018 12:54:05 +0100 Will Deacon wrote: > Hi Kim, Hi Will, thanks for responding. > On Fri, Apr 27, 2018 at 11:56:25AM -0500, Kim Phillips wrote: > > On Fri, 27 Apr 2018 17:09:14 +0100 > > Will Deacon wrote: > > > On Fri, Apr 27, 2018 at 10:46:29AM -0500, Kim Phillips wrote: > > > > On Fri, 27 Apr 2018 15:37:20 +0100 > > > > Will Deacon wrote: > > > > > For anything under drivers/perf/, I'd prefer not to have these prints > > > > > and instead see efforts to improve error reporting via the perf system > > > > > call interface. > > > > > > > > We'd all prefer that, and for all PMU drivers, why should ones under > > > > drivers/perf be treated differently? > > > > > > Because they're the ones I maintain... > > > > You represent a minority on your opinion on this matter though. > > I'm not sure that's true I haven't seen another arch/PMU driver maintainer actively oppose it other than you (and Mark). > -- you tend to be the only one raising this issue; If you're basing that on list activity, I wouldn't, since most Arm perf users don't subscribe to LKML. I'm trying to represent current and future Arm perf users that shouldn't be expected to hang out here on LKML and review PMU drivers. Couldn't tell you why PMU driver authors themselves don't chime in, but will note that some drivers in drivers/perf do print things from their event_init functions. Also, you have been cc'd on off-list email threads where SPE users had to debug the SPE driver's event_init() manually. There are other SPE users I know that have had to go through the same painstaking process of debugging the SPE driver. Last but not least, there's me, and I'd sure appreciate more verbose PMU drivers, based on my real-life performance monitoring experience(s). > I think most people don't actually care one way or the other. If you know of Like I said, I think you're limiting your audience to LKML subscribers, who are more amenable to being convinced they need to debug their kernel. > others who care about it too then I suggest you work together as a group to > solve the problem in a way which is amenable to the upstream maintainers. I know a few off-list people that would like a more user-friendly Arm perf experience, but we're not qualified to solve the larger problem that perf has had since its inception, and I think it's a little unfair for you to suggest that, esp. given you're one of the maintainers, and there's Linus on top of you. > Then you won't have to worry about fixing it personally. You could even > propose a topic for plumbers if there is enough interest in a general > framework for propagating error messages to userspace. I'm not worried about fixing it personally or not. I'm worried about our perf users' experience given your objection to using the vehicle already in use on other architectures and PMU drivers. If you - or anyone else for that matter - know of a way that'll get us past this, by all means, say something. If it helps, I recently asked acme if we could borrow bits from struct perf_event and got no response. > > > > As you are already aware, I've personally tried to fix this problem - > > > > that has existed since before the introduction of the perf tool (I > > > > consider it a syscall-independent enhanced error interface), multiple > > > > times, and failed. > > > > > > Why is that my problem? Try harder? > > > > It's your problem because we're here reviewing a patch that happens to > > fall under your maintainership. I'll be the first person to tell you > > I'm obviously incompetent and haven't been able to come up with a > > solution that is acceptable for everyone up to and including Linus > > Torvalds. I'm just noticing a chronic usability problem that can be > > easily alleviated in the context of this patch review. > > I don't see how incompetence has anything to do with it and, like most Well you told me to try harder, and I'm trying to let you know that I can try harder - ad infinitum - and still not get it, so telling me to try harder isn't going to necessarily resolve the issue. > people (lucky gits...), I doubt Torvalds cares too deeply about PMU driver > internals. No arguments on the usability problem, but this ain't the fix Like acme, I doubt Linus runs perf on real Arm h/w, so yes, I don't think they care that much, but if they did try to do it one day, I'd like to that they'd be able to have dmesg contain that extra error information for them. As you know, David Howells wrote a supplemental error syscall facility [1], that Linus nacked (off list no less). Does Linus care about what David Howells was developing it for? I don't know, but he does have enough experience to know whether a certain approach to a problem is sustainable in his kernel or not. > you're looking for: it's a bodge. We've been over this many times before. I think it's OK - necessary even - until this error infrastructure problem perf has is fixed, and I think you're being unrealisti
[PATCH v3 3/4] seccomp: Audit attempts to modify the actions_logged sysctl
The decision to log a seccomp action will always be subject to the
value of the kernel.seccomp.actions_logged sysctl, even for processes
that are being inspected via the audit subsystem, in an upcoming patch.
Therefore, we need to emit an audit record on attempts at writing to the
actions_logged sysctl when auditing is enabled.
This patch updates the write handler for the actions_logged sysctl to
emit an audit record on attempts to write to the sysctl. Successful
writes to the sysctl will result in a record that includes a normalized
list of logged actions in the "actions" field and a "res" field equal to
1. Unsuccessful writes to the sysctl will result in a record that
doesn't include the "actions" field and has a "res" field equal to 0.
Not all unsuccessful writes to the sysctl are audited. For example, an
audit record will not be emitted if an unprivileged process attempts to
open the sysctl file for reading since that access control check is not
part of the sysctl's write handler.
Below are some example audit records when writing various strings to the
actions_logged sysctl.
Writing "not-a-real-action", when the kernel.seccomp.actions_logged
sysctl previously was "kill_process kill_thread trap errno trace log",
emits this audit record:
type=CONFIG_CHANGE msg=audit(1525392371.454:120): op=seccomp-logging
actions=? old-actions=kill_process,kill_thread,trap,errno,trace,log
res=0
If you then write "kill_process kill_thread errno trace log", this audit
record is emitted:
type=CONFIG_CHANGE msg=audit(1525392401.645:126): op=seccomp-logging
actions=kill_process,kill_thread,errno,trace,log
old-actions=kill_process,kill_thread,trap,errno,trace,log res=1
If you then write "log log errno trace kill_process kill_thread", which
is unordered and contains the log action twice, it results in the same
actions value as the previous record:
type=CONFIG_CHANGE msg=audit(1525392436.354:132): op=seccomp-logging
actions=kill_process,kill_thread,errno,trace,log
old-actions=kill_process,kill_thread,errno,trace,log res=1
If you then write an empty string to the sysctl, this audit record is
emitted:
type=CONFIG_CHANGE msg=audit(1525392494.413:138): op=seccomp-logging
actions=(none) old-actions=kill_process,kill_thread,errno,trace,log
res=1
No audit records are generated when reading the actions_logged sysctl.
Suggested-by: Steve Grubb
Signed-off-by: Tyler Hicks
---
include/linux/audit.h | 5 +
kernel/auditsc.c | 20 ++
kernel/seccomp.c | 58 +++
3 files changed, 74 insertions(+), 9 deletions(-)
diff --git a/include/linux/audit.h b/include/linux/audit.h
index 75d5b03..d4e35e7 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -233,6 +233,8 @@ extern void __audit_inode_child(struct inode *parent,
const struct dentry *dentry,
const unsigned char type);
extern void __audit_seccomp(unsigned long syscall, long signr, int code);
+extern void audit_seccomp_actions_logged(const char *names,
+const char *old_names, int res);
extern void __audit_ptrace(struct task_struct *t);
static inline bool audit_dummy_context(void)
@@ -502,6 +504,9 @@ static inline void __audit_seccomp(unsigned long syscall,
long signr, int code)
{ }
static inline void audit_seccomp(unsigned long syscall, long signr, int code)
{ }
+static inline void audit_seccomp_actions_logged(const char *names,
+ const char *old_names, int res)
+{ }
static inline int auditsc_get_stamp(struct audit_context *ctx,
struct timespec64 *t, unsigned int *serial)
{
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 4e0a4ac..5195a29 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2478,6 +2478,26 @@ void __audit_seccomp(unsigned long syscall, long signr,
int code)
audit_log_end(ab);
}
+void audit_seccomp_actions_logged(const char *names, const char *old_names,
+ int res)
+{
+ struct audit_buffer *ab;
+
+ if (!audit_enabled)
+ return;
+
+ ab = audit_log_start(current->audit_context, GFP_KERNEL,
+AUDIT_CONFIG_CHANGE);
+ if (unlikely(!ab))
+ return;
+
+ audit_log_format(ab, "op=seccomp-logging");
+ audit_log_format(ab, " actions=%s", names);
+ audit_log_format(ab, " old-actions=%s", old_names);
+ audit_log_format(ab, " res=%d", res);
+ audit_log_end(ab);
+}
+
struct list_head *audit_killed_trees(void)
{
struct audit_context *ctx = current->audit_context;
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index b36ac1e..f5630d1 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -1219,11 +1219,10 @@ static int read_actions_logged(struct ctl_table
*ro_table, void __user *buffer,
}
static int write_actions_log
[PATCH v3 2/4] seccomp: Configurable separator for the actions_logged string
The function that converts a bitmask of seccomp actions that are
allowed to be logged is currently only used for constructing the display
string for the kernel.seccomp.actions_logged sysctl. That string wants a
space character to be used for the separator between actions.
A future patch will make use of the same function for building a string
that will be sent to the audit subsystem for tracking modifications to
the kernel.seccomp.actions_logged sysctl. That string will need to use a
comma as a separator. This patch allows the separator character to be
configurable to meet both needs.
Signed-off-by: Tyler Hicks
---
kernel/seccomp.c | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index f4afe67..b36ac1e 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -1135,10 +1135,11 @@ static const struct seccomp_log_name
seccomp_log_names[] = {
};
static bool seccomp_names_from_actions_logged(char *names, size_t size,
- u32 actions_logged)
+ u32 actions_logged,
+ const char *sep)
{
const struct seccomp_log_name *cur;
- bool append_space = false;
+ bool append_sep = false;
for (cur = seccomp_log_names; cur->name && size; cur++) {
ssize_t ret;
@@ -1146,15 +1147,15 @@ static bool seccomp_names_from_actions_logged(char
*names, size_t size,
if (!(actions_logged & cur->log))
continue;
- if (append_space) {
- ret = strscpy(names, " ", size);
+ if (append_sep) {
+ ret = strscpy(names, sep, size);
if (ret < 0)
return false;
names += ret;
size -= ret;
} else
- append_space = true;
+ append_sep = true;
ret = strscpy(names, cur->name, size);
if (ret < 0)
@@ -1208,7 +1209,7 @@ static int read_actions_logged(struct ctl_table
*ro_table, void __user *buffer,
memset(names, 0, sizeof(names));
if (!seccomp_names_from_actions_logged(names, sizeof(names),
- seccomp_actions_logged))
+ seccomp_actions_logged, " "))
return -EINVAL;
table = *ro_table;
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 4/4] seccomp: Don't special case audited processes when logging
Seccomp logging for "handled" actions such as RET_TRAP, RET_TRACE, or
RET_ERRNO can be very noisy for processes that are being audited. This
patch modifies the seccomp logging behavior to treat processes that are
being inspected via the audit subsystem the same as processes that
aren't under inspection. Handled actions will no longer be logged just
because the process is being inspected. Since v4.14, applications have
the ability to request logging of handled actions by using the
SECCOMP_FILTER_FLAG_LOG flag when loading seccomp filters.
With this patch, the logic for deciding if an action will be logged is:
if action == RET_ALLOW:
do not log
else if action not in actions_logged:
do not log
else if action == RET_KILL:
log
else if action == RET_LOG:
log
else if filter-requests-logging:
log
else:
do not log
Reported-by: Steve Grubb
Signed-off-by: Tyler Hicks
---
Documentation/userspace-api/seccomp_filter.rst | 7 ---
include/linux/audit.h | 10 +-
kernel/auditsc.c | 14 +-
kernel/seccomp.c | 17 +++--
4 files changed, 21 insertions(+), 27 deletions(-)
diff --git a/Documentation/userspace-api/seccomp_filter.rst
b/Documentation/userspace-api/seccomp_filter.rst
index 099c412..82a468b 100644
--- a/Documentation/userspace-api/seccomp_filter.rst
+++ b/Documentation/userspace-api/seccomp_filter.rst
@@ -207,13 +207,6 @@ directory. Here's a description of each file in that
directory:
to the file do not need to be in ordered form but reads from the file
will be ordered in the same way as the actions_avail sysctl.
- It is important to note that the value of ``actions_logged`` does not
- prevent certain actions from being logged when the audit subsystem is
- configured to audit a task. If the action is not found in
- ``actions_logged`` list, the final decision on whether to audit the
- action for that task is ultimately left up to the audit subsystem to
- decide for all seccomp return values other than ``SECCOMP_RET_ALLOW``.
-
The ``allow`` string is not accepted in the ``actions_logged`` sysctl
as it is not possible to log ``SECCOMP_RET_ALLOW`` actions. Attempting
to write ``allow`` to the sysctl will result in an EINVAL being
diff --git a/include/linux/audit.h b/include/linux/audit.h
index d4e35e7..b639cf1 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -232,7 +232,7 @@ extern void __audit_file(const struct file *);
extern void __audit_inode_child(struct inode *parent,
const struct dentry *dentry,
const unsigned char type);
-extern void __audit_seccomp(unsigned long syscall, long signr, int code);
+extern void audit_seccomp(unsigned long syscall, long signr, int code);
extern void audit_seccomp_actions_logged(const char *names,
const char *old_names, int res);
extern void __audit_ptrace(struct task_struct *t);
@@ -304,12 +304,6 @@ static inline void audit_inode_child(struct inode *parent,
}
void audit_core_dumps(long signr);
-static inline void audit_seccomp(unsigned long syscall, long signr, int code)
-{
- if (audit_enabled && unlikely(!audit_dummy_context()))
- __audit_seccomp(syscall, signr, code);
-}
-
static inline void audit_ptrace(struct task_struct *t)
{
if (unlikely(!audit_dummy_context()))
@@ -500,8 +494,6 @@ static inline void audit_inode_child(struct inode *parent,
{ }
static inline void audit_core_dumps(long signr)
{ }
-static inline void __audit_seccomp(unsigned long syscall, long signr, int code)
-{ }
static inline void audit_seccomp(unsigned long syscall, long signr, int code)
{ }
static inline void audit_seccomp_actions_logged(const char *names,
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 5195a29..15c20ba 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2464,7 +2464,19 @@ void audit_core_dumps(long signr)
audit_log_end(ab);
}
-void __audit_seccomp(unsigned long syscall, long signr, int code)
+/**
+ * audit_seccomp - record information about a seccomp action
+ * @syscall: syscall number
+ * @signr: signal value
+ * @code: the seccomp action
+ *
+ * Record the information associated with a seccomp action. Event filtering for
+ * seccomp actions that are not to be logged is done in seccomp_log().
+ * Therefore, this function forces auditing independent of the audit_enabled
+ * and dummy context state because seccomp actions should be logged even when
+ * audit is not in use.
+ */
+void audit_seccomp(unsigned long syscall, long signr, int code)
{
struct audit_buffer *ab;
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index f5630d1..5386749 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -584,18 +584,15 @@ static inline void seccomp_log(unsigned long sy
[PATCH v3 0/4] Better integrate seccomp logging and auditing
Seccomp received improved logging controls in v4.14. Applications can opt into logging of "handled" actions (SECCOMP_RET_TRAP, SECCOMP_RET_TRACE, SECCOMP_RET_ERRNO) using the SECCOMP_FILTER_FLAG_LOG bit when loading filters. They can also debug filter matching with the new SECCOMP_RET_LOG action. Administrators can prevent specific actions from being logged using the kernel.seccomp.actions_logged sysctl. However, one corner case intentionally wasn't addressed in those v4.14 changes. When a process is being inspected by the audit subsystem, seccomp's decision making for logging ignores the new controls and unconditionally logs every action taken except for SECCOMP_RET_ALLOW. This isn't particularly useful since many existing applications don't intend to log handled actions due to them occurring very frequently. This amount of logging fills the audit logs without providing many benefits now that application authors have fine grained controls at their disposal. This patch set aligns the seccomp logging behavior for both audited and non-audited processes. It also emits an audit record, if auditing is enabled, when the kernel.seccomp.actions_logged sysctl is written to so that there's a paper trail when entire actions are quieted. Changes in v3: * Patch 3 - Never drop a field when emitting the audit record - Use the value "?" for the actions field when an error occurred while writing to the sysctl - Use the value "?" for the actions and/or old-actions fields when a failure to translate actions to names - Use the value "(none)" for the actions and/or old-actions fields when no actions are specified + This is possible when writing an empty string to the sysctl - Update the commit message to note the new values and give an example of when an empty string is written * Patch 4 - Adjust the control flow of seccomp_log() to exit early if nothing should be logged Changes in v2: * Patch 2 - New patch, allowing for a configurable separator between action names * Patch 3 - The value of the actions field in the audit record now uses a comma instead of a space - The value of the actions field in the audit record is no longer enclosed in quotes - audit_log_start() is called with the current processes' audit_context in audit_seccomp_actions_logged() - audit_seccomp_actions_logged() no longer records the pid, uid, auid, tty, ses, task context, comm, or executable path - The new and old value of seccomp_actions_logged is recorded in the AUDIT_CONFIG_CHANGE record - The value of the "res" field in the CONFIG_CHANGE audit record is corrected (1 indicates success, 0 failure) - Updated patch 3's commit message to reflect the updated audit record format in the examples * Patch 4 - A function comment for audit_seccomp() was added to explain, among other things, that event filtering is performed in seccomp_log() Tyler -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/4] seccomp: Separate read and write code for actions_logged sysctl
Break the read and write paths of the kernel.seccomp.actions_logged
sysctl into separate functions to maintain readability. An upcoming
change will need to audit writes, but not reads, of this sysctl which
would introduce too many conditional code paths on whether or not the
'write' parameter evaluates to true.
Signed-off-by: Tyler Hicks
---
kernel/seccomp.c | 60 +++-
1 file changed, 38 insertions(+), 22 deletions(-)
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index dc77548..f4afe67 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -1199,48 +1199,64 @@ static bool seccomp_actions_logged_from_names(u32
*actions_logged, char *names)
return true;
}
-static int seccomp_actions_logged_handler(struct ctl_table *ro_table, int
write,
- void __user *buffer, size_t *lenp,
- loff_t *ppos)
+static int read_actions_logged(struct ctl_table *ro_table, void __user *buffer,
+ size_t *lenp, loff_t *ppos)
{
char names[sizeof(seccomp_actions_avail)];
struct ctl_table table;
+
+ memset(names, 0, sizeof(names));
+
+ if (!seccomp_names_from_actions_logged(names, sizeof(names),
+ seccomp_actions_logged))
+ return -EINVAL;
+
+ table = *ro_table;
+ table.data = names;
+ table.maxlen = sizeof(names);
+ return proc_dostring(&table, 0, buffer, lenp, ppos);
+}
+
+static int write_actions_logged(struct ctl_table *ro_table, void __user
*buffer,
+ size_t *lenp, loff_t *ppos)
+{
+ char names[sizeof(seccomp_actions_avail)];
+ struct ctl_table table;
+ u32 actions_logged;
int ret;
- if (write && !capable(CAP_SYS_ADMIN))
+ if (!capable(CAP_SYS_ADMIN))
return -EPERM;
memset(names, 0, sizeof(names));
- if (!write) {
- if (!seccomp_names_from_actions_logged(names, sizeof(names),
- seccomp_actions_logged))
- return -EINVAL;
- }
-
table = *ro_table;
table.data = names;
table.maxlen = sizeof(names);
- ret = proc_dostring(&table, write, buffer, lenp, ppos);
+ ret = proc_dostring(&table, 1, buffer, lenp, ppos);
if (ret)
return ret;
- if (write) {
- u32 actions_logged;
-
- if (!seccomp_actions_logged_from_names(&actions_logged,
- table.data))
- return -EINVAL;
-
- if (actions_logged & SECCOMP_LOG_ALLOW)
- return -EINVAL;
+ if (!seccomp_actions_logged_from_names(&actions_logged, table.data))
+ return -EINVAL;
- seccomp_actions_logged = actions_logged;
- }
+ if (actions_logged & SECCOMP_LOG_ALLOW)
+ return -EINVAL;
+ seccomp_actions_logged = actions_logged;
return 0;
}
+static int seccomp_actions_logged_handler(struct ctl_table *ro_table, int
write,
+ void __user *buffer, size_t *lenp,
+ loff_t *ppos)
+{
+ if (write)
+ return write_actions_logged(ro_table, buffer, lenp, ppos);
+ else
+ return read_actions_logged(ro_table, buffer, lenp, ppos);
+}
+
static struct ctl_path seccomp_sysctl_path[] = {
{ .procname = "kernel", },
{ .procname = "seccomp", },
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 6/9] trace_uprobe: Support SDT markers having reference count (semaphore)
Hi Ravi,
I have some comments, please see below.
On Tue, 17 Apr 2018 10:02:41 +0530
Ravi Bangoria wrote:\
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index 7bd2760..2db3ed1 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -122,6 +122,8 @@ struct uprobe_map_info {
> unsigned long vaddr;
> };
>
> +extern void (*uprobe_mmap_callback)(struct vm_area_struct *vma);
> +
> extern int set_swbp(struct arch_uprobe *aup, struct mm_struct *mm, unsigned
> long vaddr);
> extern int set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm,
> unsigned long vaddr);
> extern bool is_swbp_insn(uprobe_opcode_t *insn);
> @@ -136,6 +138,8 @@ struct uprobe_map_info {
> extern void uprobe_munmap(struct vm_area_struct *vma, unsigned long start,
> unsigned long end);
> extern void uprobe_start_dup_mmap(void);
> extern void uprobe_end_dup_mmap(void);
> +extern void uprobe_down_write_dup_mmap(void);
> +extern void uprobe_up_write_dup_mmap(void);
> extern void uprobe_dup_mmap(struct mm_struct *oldmm, struct mm_struct
> *newmm);
> extern void uprobe_free_utask(struct task_struct *t);
> extern void uprobe_copy_process(struct task_struct *t, unsigned long flags);
> @@ -192,6 +196,12 @@ static inline void uprobe_start_dup_mmap(void)
> static inline void uprobe_end_dup_mmap(void)
> {
> }
> +static inline void uprobe_down_write_dup_mmap(void)
> +{
> +}
> +static inline void uprobe_up_write_dup_mmap(void)
> +{
> +}
> static inline void
> uprobe_dup_mmap(struct mm_struct *oldmm, struct mm_struct *newmm)
> {
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 096d1e6..e26ad83 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1044,6 +1044,9 @@ static void build_probe_list(struct inode *inode,
> spin_unlock(&uprobes_treelock);
> }
>
> +/* Rightnow the only user of this is trace_uprobe. */
> +void (*uprobe_mmap_callback)(struct vm_area_struct *vma);
> +
> /*
> * Called from mmap_region/vma_adjust with mm->mmap_sem acquired.
> *
> @@ -1056,7 +1059,13 @@ int uprobe_mmap(struct vm_area_struct *vma)
> struct uprobe *uprobe, *u;
> struct inode *inode;
>
> - if (no_uprobe_events() || !valid_vma(vma, true))
> + if (no_uprobe_events())
> + return 0;
> +
> + if (uprobe_mmap_callback)
> + uprobe_mmap_callback(vma);
> +
> + if (!valid_vma(vma, true))
> return 0;
>
> inode = file_inode(vma->vm_file);
> @@ -1247,6 +1256,16 @@ void uprobe_end_dup_mmap(void)
> percpu_up_read(&dup_mmap_sem);
> }
>
> +void uprobe_down_write_dup_mmap(void)
> +{
> + percpu_down_write(&dup_mmap_sem);
> +}
> +
> +void uprobe_up_write_dup_mmap(void)
> +{
> + percpu_up_write(&dup_mmap_sem);
> +}
> +
I'm not sure why these hunks are not done in previous patch.
If you separate "uprobe_map_info" export patch, this also
should be separated. (Or both merged into this patch)
> void uprobe_dup_mmap(struct mm_struct *oldmm, struct mm_struct *newmm)
> {
> if (test_bit(MMF_HAS_UPROBES, &oldmm->flags)) {
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index 0d450b4..1a48b04 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -25,6 +25,8 @@
> #include
> #include
> #include
> +#include
> +#include
>
> #include "trace_probe.h"
>
> @@ -58,6 +60,7 @@ struct trace_uprobe {
> struct inode*inode;
> char*filename;
> unsigned long offset;
> + unsigned long ref_ctr_offset;
> unsigned long nhit;
> struct trace_probe tp;
> };
> @@ -364,10 +367,10 @@ static int create_trace_uprobe(int argc, char **argv)
> {
> struct trace_uprobe *tu;
> struct inode *inode;
> - char *arg, *event, *group, *filename;
> + char *arg, *event, *group, *filename, *rctr, *rctr_end;
> char buf[MAX_EVENT_NAME_LEN];
> struct path path;
> - unsigned long offset;
> + unsigned long offset, ref_ctr_offset;
> bool is_delete, is_return;
> int i, ret;
>
> @@ -377,6 +380,7 @@ static int create_trace_uprobe(int argc, char **argv)
> is_return = false;
> event = NULL;
> group = NULL;
> + ref_ctr_offset = 0;
>
> /* argc must be >= 1 */
> if (argv[0][0] == '-')
> @@ -456,6 +460,26 @@ static int create_trace_uprobe(int argc, char **argv)
> goto fail_address_parse;
> }
>
> + /* Parse reference counter offset if specified. */
> + rctr = strchr(arg, '(');
> + if (rctr) {
> + rctr_end = strchr(rctr, ')');
> + if (rctr > rctr_end || *(rctr_end + 1) != 0) {
> + ret = -EINVAL;
> + pr_info("Invalid reference counter offset.\n");
> + goto fail_address_parse;
> + }
> +
> +
