[PATCH v6 4/9] Improve fd filtering
Eugene Syromiatnikov
esyr at redhat.com
Mon Aug 7 04:52:52 UTC 2017
On Wed, Aug 02, 2017 at 12:36:44PM +0700, Nikolay Marchuk wrote:
> * pathtrace.c (match_fd_common, pathtrace_match_set): Move fd matching to
> separate function.
> * filter.h (match_fd_common): Add new declaration.
> * basic_filters.c (run_fd_filter): Use match_fd_common for fd filter.
> ---
> basic_filters.c | 82 ++++++++++++++++++++++-
> filter.h | 2 +
> pathtrace.c | 202 ++++++++++++++++++++++++++++++++++++++------------------
> 3 files changed, 219 insertions(+), 67 deletions(-)
>
> diff --git a/basic_filters.c b/basic_filters.c
> index 85eb58b..2761a74 100644
> --- a/basic_filters.c
> +++ b/basic_filters.c
> @@ -29,6 +29,7 @@
> #include "defs.h"
> #include <regex.h>
> #include "filter.h"
> +#include "syscall.h"
>
> typedef unsigned int number_slot_t;
> #define BITS_PER_SLOT (sizeof(number_slot_t) * 8)
> @@ -414,15 +415,90 @@ parse_fd_filter(const char *str)
> return set;
> }
>
> +static bool
> +is_fd_in_set(struct tcb *tcp, int fd, void *data) {
> + struct number_set *set = data;
> +
> + if (fd < 0)
> + return set->not;
> + return is_number_in_set(fd, set);
> +}
> +
> bool
> run_fd_filter(struct tcb *tcp, void *_priv_data)
> {
> - int fd = tcp->u_arg[0];
> struct number_set *set = _priv_data;
> + const struct_sysent *s_ent = tcp->s_ent;
>
> - if (fd < 0)
> + /*
> + * mq_timedsend and mq_timedreceive are not marked as descriptor
> + * syscalls, but they can be dumped with -e read/write.
> + */
> + switch (s_ent->sen) {
> + case SEN_mq_timedsend:
> + case SEN_mq_timedreceive:
> + return is_fd_in_set(tcp, tcp->u_arg[0], set);
> + }
> +
> + if (!(s_ent->sys_flags & (TRACE_DESC | TRACE_NETWORK)))
> return false;
> - return is_number_in_set(fd, set);
> +
> + if (match_fd_common(tcp, &is_fd_in_set, set))
> + return true;
> +
> + switch (s_ent->sen) {
> + /* Already tested with match_fd_common. */
> + case SEN_dup2:
> + case SEN_dup3:
> + case SEN_kexec_file_load:
> + case SEN_sendfile:
> + case SEN_sendfile64:
> + case SEN_tee:
> + case SEN_linkat:
> + case SEN_renameat2:
> + case SEN_renameat:
> + case SEN_copy_file_range:
> + case SEN_splice:
> + case SEN_old_mmap:
> +#if defined(S390)
> + case SEN_old_mmap_pgoff:
> +#endif
> + case SEN_mmap:
> + case SEN_mmap_4koff:
> + case SEN_mmap_pgoff:
> + case SEN_ARCH_mmap:
> + case SEN_symlinkat:
> + case SEN_fanotify_mark:
> + case SEN_oldselect:
> + case SEN_pselect6:
> + case SEN_select:
> + case SEN_poll:
> + case SEN_ppoll:
> + /*
> + * These have TRACE_DESCRIPTOR or TRACE_NETWORK set,
> + * but they don't have any file descriptor to test.
> + */
> + case SEN_bpf:
> + case SEN_creat:
> + case SEN_epoll_create:
> + case SEN_epoll_create1:
> + case SEN_eventfd2:
> + case SEN_eventfd:
> + case SEN_fanotify_init:
> + case SEN_inotify_init1:
> + case SEN_memfd_create:
> + case SEN_open:
> + case SEN_perf_event_open:
> + case SEN_pipe:
> + case SEN_pipe2:
> + case SEN_printargs:
> + case SEN_socket:
> + case SEN_socketpair:
> + case SEN_timerfd_create:
> + case SEN_userfaultfd:
> + return false;
> + }
> + return is_fd_in_set(tcp, tcp->u_arg[0], set);
> }
>
> void
> diff --git a/filter.h b/filter.h
> index 4085d45..ead8387 100644
> --- a/filter.h
> +++ b/filter.h
> @@ -39,6 +39,8 @@ void parse_set(const char *const, struct number_set *const,
> string_to_uint_func, const char *const);
> void parse_inject_common_args(char *, struct inject_opts *, const char *delim,
> const bool fault_tokens_only);
> +typedef bool (*match_fd_func)(struct tcb *, int, void *);
> +int match_fd_common(struct tcb *, match_fd_func, void *);
>
> /* filter api */
> struct filter* add_filter_to_array(struct filter **, unsigned int *nfilters,
> diff --git a/pathtrace.c b/pathtrace.c
> index 26a52fe..b1e1f08 100644
> --- a/pathtrace.c
> +++ b/pathtrace.c
> @@ -69,6 +69,8 @@ upathmatch(struct tcb *const tcp, const kernel_ulong_t upath,
> static bool
> fdmatch(struct tcb *tcp, int fd, struct path_set *set)
> {
> + if (fd < 0)
> + return false;
> char path[PATH_MAX + 1];
> int n = getfdpath(tcp, fd, path, sizeof(path));
>
> @@ -143,36 +145,23 @@ pathtrace_select_set(const char *path, struct path_set *set)
> storepath(rpath, set);
> }
>
> -/*
> - * Return true if syscall accesses a selected path
> - * (or if no paths have been specified for tracing).
> - */
> -bool
> -pathtrace_match_set(struct tcb *tcp, struct path_set *set)
> +typedef bool (*match_fd_func)(struct tcb *, int, void *);
> +
> +static
> +bool fdmatch_fd_func(struct tcb *tcp, int fd, void *data)
> {
> - const struct_sysent *s;
> + return fdmatch(tcp, fd, (struct path_set *) data);
> +}
>
> - s = tcp->s_ent;
> +/* Match fd with func. */
> +bool
> +match_fd_common(struct tcb *tcp, match_fd_func func, void *data)
> +{
> + const struct_sysent *s = tcp->s_ent;
>
> - if (!(s->sys_flags & (TRACE_FILE | TRACE_DESC | TRACE_NETWORK)))
> + if (!(s->sys_flags & (TRACE_DESC | TRACE_NETWORK)))
> return false;
> -
> - /*
> - * Check for special cases where we need to do something
> - * other than test arg[0].
> - */
> -
> switch (s->sen) {
> - case SEN_dup2:
> - case SEN_dup3:
> - case SEN_kexec_file_load:
> - case SEN_sendfile:
> - case SEN_sendfile64:
> - case SEN_tee:
> - /* fd, fd */
> - return fdmatch(tcp, tcp->u_arg[0], set) ||
> - fdmatch(tcp, tcp->u_arg[1], set);
> -
> case SEN_faccessat:
> case SEN_fchmodat:
> case SEN_fchownat:
> @@ -188,29 +177,27 @@ pathtrace_match_set(struct tcb *tcp, struct path_set *set)
> case SEN_statx:
> case SEN_unlinkat:
> case SEN_utimensat:
> - /* fd, path */
> - return fdmatch(tcp, tcp->u_arg[0], set) ||
> - upathmatch(tcp, tcp->u_arg[1], set);
> -
> - case SEN_link:
> - case SEN_mount:
> - case SEN_pivotroot:
> - /* path, path */
> - return upathmatch(tcp, tcp->u_arg[0], set) ||
> - upathmatch(tcp, tcp->u_arg[1], set);
> + /* fd, path special case */
What do you mean by "special case"?
> + return func(tcp, tcp->u_arg[0], data);
>
> - case SEN_quotactl:
> - /* x, path */
> - return upathmatch(tcp, tcp->u_arg[1], set);
> + case SEN_dup2:
> + case SEN_dup3:
> + case SEN_kexec_file_load:
> + case SEN_sendfile:
> + case SEN_sendfile64:
> + case SEN_tee:
> + /* fd, fd */
> + return func(tcp, tcp->u_arg[0], data) ||
> + func(tcp, tcp->u_arg[1], data);
>
> case SEN_linkat:
> case SEN_renameat2:
> case SEN_renameat:
> - /* fd, path, fd, path */
> - return fdmatch(tcp, tcp->u_arg[0], set) ||
> - fdmatch(tcp, tcp->u_arg[2], set) ||
> - upathmatch(tcp, tcp->u_arg[1], set) ||
> - upathmatch(tcp, tcp->u_arg[3], set);
> + case SEN_copy_file_range:
> + case SEN_splice:
> + /* fd, x, fd */
> + return func(tcp, tcp->u_arg[0], data) ||
> + func(tcp, tcp->u_arg[2], data);
>
> case SEN_old_mmap:
> #if defined(S390)
> @@ -221,33 +208,25 @@ pathtrace_match_set(struct tcb *tcp, struct path_set *set)
> case SEN_mmap_pgoff:
> case SEN_ARCH_mmap:
> /* x, x, x, x, fd */
> - return fdmatch(tcp, tcp->u_arg[4], set);
> + return func(tcp, tcp->u_arg[4], data);
>
> case SEN_symlinkat:
> - /* path, fd, path */
> - return fdmatch(tcp, tcp->u_arg[1], set) ||
> - upathmatch(tcp, tcp->u_arg[0], set) ||
> - upathmatch(tcp, tcp->u_arg[2], set);
> -
> - case SEN_copy_file_range:
> - case SEN_splice:
> - /* fd, x, fd, x, x, x */
> - return fdmatch(tcp, tcp->u_arg[0], set) ||
> - fdmatch(tcp, tcp->u_arg[2], set);
> + /* x, fd, x */
> + return func(tcp, tcp->u_arg[1], data);
>
> case SEN_epoll_ctl:
> /* x, x, fd, x */
> - return fdmatch(tcp, tcp->u_arg[2], set);
> -
> + return func(tcp, tcp->u_arg[2], data);
>
> case SEN_fanotify_mark:
> {
> /* x, x, mask (64 bit), fd, path */
> unsigned long long mask = 0;
> int argn = getllval(tcp, &mask, 2);
> - return fdmatch(tcp, tcp->u_arg[argn], set) ||
> - upathmatch(tcp, tcp->u_arg[argn + 1], set);
> +
> + return func(tcp, tcp->u_arg[argn], data);
> }
> +
> case SEN_oldselect:
> case SEN_pselect6:
> case SEN_select:
> @@ -302,7 +281,7 @@ pathtrace_match_set(struct tcb *tcp, struct path_set *set)
> j = next_set_bit(fds, j, nfds);
> if (j < 0)
> break;
> - if (fdmatch(tcp, j, set)) {
> + if (func(tcp, j, data)) {
> free(fds);
> return true;
> }
> @@ -329,12 +308,111 @@ pathtrace_match_set(struct tcb *tcp, struct path_set *set)
>
> for (cur = start; cur < end; cur += sizeof(fds))
> if ((umove(tcp, cur, &fds) == 0)
> - && fdmatch(tcp, fds.fd, set))
> + && func(tcp, fds.fd, data))
> return true;
>
> return false;
> }
> + }
> + return false;
> +}
> +
> +/*
> + * Return true if syscall accesses a selected path
> + * (or if no paths have been specified for tracing).
> + */
> +bool
> +pathtrace_match_set(struct tcb *tcp, struct path_set *set)
> +{
> + const struct_sysent *s;
> +
> + s = tcp->s_ent;
> +
> + if (!(s->sys_flags & (TRACE_FILE | TRACE_DESC | TRACE_NETWORK)))
> + return false;
>
> + if (match_fd_common(tcp, fdmatch_fd_func, set))
> + return true;
> +
> + /*
> + * Check for special cases where we need to do something
> + * other than test arg[0].
> + */
> + switch (s->sen) {
> + case SEN_faccessat:
> + case SEN_fchmodat:
> + case SEN_fchownat:
> + case SEN_fstatat64:
> + case SEN_futimesat:
> + case SEN_inotify_add_watch:
> + case SEN_mkdirat:
> + case SEN_mknodat:
> + case SEN_name_to_handle_at:
> + case SEN_newfstatat:
> + case SEN_openat:
> + case SEN_quotactl:
> + case SEN_readlinkat:
> + case SEN_statx:
> + case SEN_unlinkat:
> + case SEN_utimensat:
> + /* x, path */
> + return upathmatch(tcp, tcp->u_arg[1], set);
> +
> + case SEN_link:
> + case SEN_mount:
> + case SEN_pivotroot:
> + /* path, path */
> + return upathmatch(tcp, tcp->u_arg[0], set) ||
> + upathmatch(tcp, tcp->u_arg[1], set);
> +
> + case SEN_linkat:
> + case SEN_renameat2:
> + case SEN_renameat:
> + /* x, path, x, path */
> + return upathmatch(tcp, tcp->u_arg[1], set) ||
> + upathmatch(tcp, tcp->u_arg[3], set);
> +
> + case SEN_symlinkat:
> + /* path, x, path */
> + return upathmatch(tcp, tcp->u_arg[0], set) ||
> + upathmatch(tcp, tcp->u_arg[2], set);
> +
> + case SEN_fanotify_mark:
> + {
> + /* x, x, mask (64 bit), fd, path */
> + unsigned long long mask = 0;
> + int argn = getllval(tcp, &mask, 2);
> +
> + return upathmatch(tcp, tcp->u_arg[argn + 1], set);
> + }
> +
> + /* Already tested with match_fd_common. */
> + case SEN_dup2:
> + case SEN_dup3:
> + case SEN_kexec_file_load:
> + case SEN_sendfile:
> + case SEN_sendfile64:
> + case SEN_tee:
> + case SEN_copy_file_range:
> + case SEN_splice:
> + case SEN_old_mmap:
> +#if defined(S390)
> + case SEN_old_mmap_pgoff:
> +#endif
> + case SEN_mmap:
> + case SEN_mmap_4koff:
> + case SEN_mmap_pgoff:
> + case SEN_ARCH_mmap:
> + case SEN_epoll_ctl:
> + case SEN_oldselect:
> + case SEN_pselect6:
> + case SEN_select:
> + case SEN_poll:
> + case SEN_ppoll:
> + /*
> + * These have TRACE_FILE or TRACE_DESCRIPTOR or TRACE_NETWORK set,
> + * but they don't have any file descriptor or path args to test.
> + */
> case SEN_bpf:
> case SEN_epoll_create:
> case SEN_epoll_create1:
> @@ -353,10 +431,6 @@ pathtrace_match_set(struct tcb *tcp, struct path_set *set)
> case SEN_timerfd_gettime:
> case SEN_timerfd_settime:
> case SEN_userfaultfd:
> - /*
> - * These have TRACE_FILE or TRACE_DESCRIPTOR or TRACE_NETWORK set,
> - * but they don't have any file descriptor or path args to test.
> - */
> return false;
> }
>
> --
> 2.1.4
What makes incredibly difficult to review this patch is the abundance of
all these syscall sets. I think, making return code of
match_fd_common/pathtrace_match_state tri-state (positive match,
negative match, no match) instead of boolean could significantly
simplify the code and avoid the duplication in run_fd_filter and
pathtrace_match_set.
More information about the Strace-devel
mailing list