Re: [PATCH 3/4] Introduce syscall fault injection feature

[Dmitry V. L. <ld...@al...>]

On Wed, Aug 10, 2016 at 10:31:48PM +0200, Nahim El Atmani wrote:
[...]
> > > * 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.
> 
> Maybe the word permissive is more appropriate than lite. The idea here is to
> allow one to return any numeric value. The string_to_int() function is
> correctly used in the next patchset. Let me know if you still find anything
> that does not make sense to you on the incoming v2.

I'm not sure I understand your question.  string_to_int() is OK if it's
needed.

> > > +/* 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.
> 
> Are you sure about using an enum in this case? These defines are flags used in
> the structure fault_opts. Using enum would ends up wrapping flags in a quite
> strange fashion like:
> 
> enum fault_flag {
> 	FAULT_ENTER = (1 << 1),
> 	FAULT_EVERY = (1 << 2),
> 	FAULT_FUZZY = (1 << 3),
> 	FAULT_AT = (1 << 4),
> 	FAULT_DONE = (1 << 5)
> };

It looks great, isn't it? ;)

> > > +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.
> 
> I feel wrong about putting set_error() in get_error.c. Does the following
> renaming suit you?
> - set_error() in arch_sc_error.c instead of get_error.c. Actually I wanted to
>   go for arch_set_error() but set_error() seems more appropriate to keep
>   consistency with the existing get_error() function.
> - arch_set_scno() in arch_scno.c instead of get_scno.c

You can create new files if you like it better.
If you are going to create a file containing just one functions, why not
name the file after the function, e.g. set_error.c for set_error() ?

> > > --- 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.
> 
> True, I moved all accounting informations in a pointer to an array of
> struct fault_opts in struct tcb. To reduce memory footprint the array is
> not lacunar anymore and only contains relevant syscalls informations. We don't
> have O(1) access anymore but since the array size can not exceed nsyscalls
> I'll go for a binary search to retrieve fault informations.

Still, the code for sparse arrays is simpler, it works faster,
and the memory cost in negligible.


-- 
ldv