[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