[PATCH 3/4] Introduce syscall fault injection feature

[Dmitry V. Levin]

On Wed, Jul 27, 2016 at 08:41:16PM +0200, Nahim El Atmani wrote:
> From: Nahim El Atmani <naam at lse.epita.fr>
> 
> From: Nahim El Atmani <nahim+dev at naam.me>
> 
> * defs.h: Add new qualifier and struct fault_opts
> * linux/x86_64/fault.h: New file.
>   (fault_set_sc_err): New function.
>   (fault_discard_sc): Likewise.
> * Makefile.am: Add it.
> * syscall.c (reallocate_fault): New function.
>   (qualify_one): Also extend the size of faults_vec.
>   (qual_fault): New function.
>   (qual_signal): Also reallocate faults_vec.
>   (fault_syscall_enter): New function.
>   (fault_syscall_exit): Likewise.
>   (trace_syscall_entering): Discard syscall if needed.
>   (trace_syscall_exiting): Set syscall's return error value, print when syscall
>   has been discarded.
> 
> Signed-off-by: Nahim El Atmani <nahim+dev at naam.me>
> Reviewed-By: Gabriel Laskar <gabriel at lse.epita.fr>
> ---
> * The fault injection is currently only available for i386 & x86_64, hence a

Why the fault injection is limited to i386 and x86_64?
The mechanism of setting syscall number and error code is similar on all
architectures.

>   new configure flag have been created (--enable-fault-injection) to keep the
>   build clean on other architectures.

Please don't make this option configurable.  It complicates the code
(== causes mistakes) and gives very little in return.

> * Concerning the struct qual_options, the name 'fault' and short name 'f' are
>   not fixed, feel free to comment if something better comes to your mind.
> * Regarding the error number checks in qual_fault(), they're light on purpose
>   to allow one to return any kind of return value.

Even if they are light, they should be correct.

> * There is still one TODO left which is supporting the fuzzy approach using
>   percentage as an occurrence. It should comes soon since all the option
>   parsing is done. On that note fuzzing means we want to be able to reproduce,
>   so we have to keep the seed somewhere and take it as an input. I my opinion
>   using the environment for this kind of things is better than adding a new
>   option, what do you think?

Using environment is OK.

> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -659,6 +659,11 @@ EXTRA_DIST =				\
>  	xlat/gen.sh			\
>  	xlate.el
>  
> +if ENABLE_FAULT_INJECTION
> +EXTRA_DIST += linux/x86_64/fault.h \
> +	      linux/i386/fault.h
> +endif
> +

When configured without --enable-fault-injection, this would create
tarball without these fault.h files; such tarball won't support
--enable-fault-injection because of missing files.

> @@ -767,7 +788,6 @@ if test "x$use_libunwind" = xyes; then
>  	AC_SUBST(libunwind_CPPFLAGS)
>  fi
>  AM_CONDITIONAL([USE_LIBUNWIND], [test "x$use_libunwind" = xyes])
> -AC_MSG_RESULT([$use_libunwind])

What was wrong with the message?

>  if test "$arch" = mips && test "$no_create" != yes; then
>  	mkdir -p linux/mips
> diff --git a/defs.h b/defs.h
> index 2edf943..d4b0dde 100644
> --- a/defs.h
> +++ b/defs.h
> @@ -363,6 +363,9 @@ struct tcb {
>  #define QUAL_SIGNAL	0x010	/* report events with this signal */
>  #define QUAL_READ	0x020	/* dump data read on this file descriptor */
>  #define QUAL_WRITE	0x040	/* dump data written to this file descriptor */
> +#if ENABLE_FAULT_INJECTION
> +#define QUAL_FAULT	0x080	/* this system call fail on purpose */
> +#endif

Why this new macro is ifdef'ed?

>  typedef uint8_t qualbits_t;
>  
>  #define DEFAULT_QUAL_FLAGS (QUAL_TRACE | QUAL_ABBREV | QUAL_VERBOSE)
> @@ -458,6 +461,28 @@ enum iov_decode {
>  	IOV_DECODE_NETLINK
>  };
>  
> +#if ENABLE_FAULT_INJECTION
> +/* Fault injection qualifiers concerning syscalls:
> + * FAULT_ENTER: already discarded, but error is not yet propagated
> + * FAULT_DONE: already discarded and we were using FAULT_AT (prevent overflow)
> + * FAULT_AT: discard syscall at the nth time
> + * FAULT_EVERY: discard syscall every nth time
> + * FAULT_FUZZY: discard syscall on a random basis every nth percent of the time
> + */
> +#define FAULT_ENTER 1
> +#define FAULT_EVERY (1 << 2)
> +#define FAULT_FUZZY (1 << 3)
> +#define FAULT_AT (1 << 4)
> +#define FAULT_DONE (1 << 5)

I prefer enums in new code.  BTW, (1 << 1) is missing.

> +static inline long
> +fault_set_sc_err(struct tcb *tcp, int error)
> +{
> +	return ptrace(PTRACE_POKEUSER, tcp->pid,
> +		      offsetof(struct user, regs.eax),
> +		      (unsigned long long int)error);
> +}

I'd rather call this function set_error or arch_set_error, opposite to
get_error, and placed it into $arch/get_error.c

It might be a good idea to omit the second argument and just set
the error register from tcp->u_error.

> +static inline long
> +fault_discard_sc(struct tcb *tcp)
> +{
> +	return ptrace(PTRACE_POKEUSER, tcp->pid,
> +		      offsetof(struct user, regs.orig_eax),
> +		      (unsigned long long int)-1);
> +}

Likewise, arch_set_scno and $arch/get_scno.c

get_error.c and get_scno.c could be renamed if necessary.

> --- a/syscall.c
> +++ b/syscall.c
> @@ -266,6 +266,14 @@ enum {
>  	MIN_QUALS = MAX_NSYSCALLS > 255 ? MAX_NSYSCALLS : 255
>  };
>  
> +#if ENABLE_FAULT_INJECTION
> +#include "fault.h"
> +struct fault_opts *faults_vec[SUPPORTED_PERSONALITIES];
> +#define syscall_failed(tcp)					\
> +	(((tcp)->qual_flg & QUAL_FAULT) &&			\
> +	 (faults_vec[current_personality][(tcp)->scno].flag & FAULT_ENTER))
> +#endif

This is wrong: faults_vec[current_personality][(tcp)->scno].flag
is a global flag, it cannot be used as a state of syscall parsing
for a tracee.

Use tcp->flags to store this information.

> +	opts.occ = strtol(ss, &end, 10);

Result of strtol is assigned to int.

> +	if (end == ss || (*end != '\0' && *end != '%' && *end != '.')
> +	    || errno == ERANGE || errno == EINVAL || opts.occ < 1

errno is not cleared before the call.


-- 
ldv
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20160729/ad5bd0cf/attachment.bin>