Re: [PATCH 04/10] fault injection: prevent recursive fault injection

Re: [PATCH 04/10] fault injection: prevent recursive fault injection

[Hillf Danton]

> 
> If something we call in the fail_dump() code path tries to acquire a
> resource that might fail (due to fault injection), then we should not
> try to recurse back into the fault injection code.
> 
> I've seen this happen with the console semaphore in the upcoming
> semaphore trylock fault injection code.
> 
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> ---
>  lib/fault-inject.c | 34 ++++++++++++++++++++++++++++------
>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/fault-inject.c b/lib/fault-inject.c
> index 6a823a5..adba7c9 100644
> --- a/lib/fault-inject.c
> +++ b/lib/fault-inject.c
> @@ -100,6 +100,33 @@ static inline bool fail_stacktrace(struct fault_attr *attr)
> 
>  #endif /* CONFIG_FAULT_INJECTION_STACKTRACE_FILTER */
> 
> +static DEFINE_PER_CPU(int, fault_active);
> +
> +static bool __fail(struct fault_attr *attr)
> +{
> +	bool ret = false;
> +
> +	/*
> +	 * Prevent recursive fault injection (this could happen if for
> +	 * example printing the fault would itself run some code that
> +	 * could fail)
> +	 */
> +	preempt_disable();
> +	if (unlikely(__this_cpu_inc_return(fault_active) != 1))
> +		goto out;
> +
> +	ret = true;
> +	fail_dump(attr);
> +
> +	if (atomic_read(&attr->times) != -1)
> +		atomic_dec_not_zero(&attr->times);
> +
> +out:
> +	__this_cpu_dec(fault_active);
> +	preempt_enable();

Well schedule entry point is add in paths like
	rt_mutex_trylock
	__alloc_pages_nodemask
and please add one or two sentences in log
message for it.

thanks
Hillf
> +	return ret;
> +}
> +
>  /*
>   * This code is stolen from failmalloc-1.0
>   * http://www.nongnu.org/failmalloc/
> @@ -134,12 +161,7 @@ bool should_fail(struct fault_attr *attr, ssize_t size)
>  	if (!fail_stacktrace(attr))
>  		return false;
> 
> -	fail_dump(attr);
> -
> -	if (atomic_read(&attr->times) != -1)
> -		atomic_dec_not_zero(&attr->times);
> -
> -	return true;
> +	return __fail(attr);
>  }
>  EXPORT_SYMBOL_GPL(should_fail);
> 
> --
> 1.9.1
> 
> 


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 04/10] fault injection: prevent recursive fault injection

[Vegard Nossum]

On 4 August 2016 at 06:09, Hillf Danton <hillf.zj@alibaba-inc.com> wrote:
>>
>> If something we call in the fail_dump() code path tries to acquire a
>> resource that might fail (due to fault injection), then we should not
>> try to recurse back into the fault injection code.
>>
>> I've seen this happen with the console semaphore in the upcoming
>> semaphore trylock fault injection code.
>>
>> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
>> ---
>>  lib/fault-inject.c | 34 ++++++++++++++++++++++++++++------
>>  1 file changed, 28 insertions(+), 6 deletions(-)
>>
>> diff --git a/lib/fault-inject.c b/lib/fault-inject.c
>> index 6a823a5..adba7c9 100644
>> --- a/lib/fault-inject.c
>> +++ b/lib/fault-inject.c
>> @@ -100,6 +100,33 @@ static inline bool fail_stacktrace(struct fault_attr *attr)
>>
>>  #endif /* CONFIG_FAULT_INJECTION_STACKTRACE_FILTER */
>>
>> +static DEFINE_PER_CPU(int, fault_active);
>> +
>> +static bool __fail(struct fault_attr *attr)
>> +{
>> +     bool ret = false;
>> +
>> +     /*
>> +      * Prevent recursive fault injection (this could happen if for
>> +      * example printing the fault would itself run some code that
>> +      * could fail)
>> +      */
>> +     preempt_disable();
>> +     if (unlikely(__this_cpu_inc_return(fault_active) != 1))
>> +             goto out;
>> +
>> +     ret = true;
>> +     fail_dump(attr);
>> +
>> +     if (atomic_read(&attr->times) != -1)
>> +             atomic_dec_not_zero(&attr->times);
>> +
>> +out:
>> +     __this_cpu_dec(fault_active);
>> +     preempt_enable();
>
> Well schedule entry point is add in paths like
>         rt_mutex_trylock
>         __alloc_pages_nodemask
> and please add one or two sentences in log
> message for it.

I'm sorry, but I don't really get what you are saying or what you want
me to add.

Are you saying that because I'm adding a fail_dump() call to
mutex_trylock() that we can now end up calling schedule() from a weird
context?

This patch is just to prevent __fail() from looping on itself, I don't
see what the connection is to rt_mutex_trylock(),
__alloc_pages_nodemask(), or schedule().

Could you please clarify? Thanks,


Vegard

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>