Message-ID: <20200319152736.GF3199@paulmck-ThinkPad-P72>
Date:   Thu, 19 Mar 2020 08:27:36 -0700
From:   "Paul E. McKenney" <paulmck@...nel.org>
To:     Marco Elver <elver@...gle.com>
Cc:     dvyukov@...gle.com, glider@...gle.com, andreyknvl@...gle.com,
        cai@....pw, kasan-dev@...glegroups.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] kcsan: Introduce report access_info and other_info

On Wed, Mar 18, 2020 at 06:38:44PM +0100, Marco Elver wrote:
> Improve readability by introducing access_info and other_info structs,
> and in preparation of the following commit in this series replaces the
> single instance of other_info with an array of size 1.
> 
> No functional change intended.
> 
> Signed-off-by: Marco Elver <elver@...gle.com>

Queued both for review and testing, and I am trying it out on one of
the scenarios that proved problematic earlier on.  Thank you!!!

							Thanx, Paul

> ---
>  kernel/kcsan/core.c   |   6 +-
>  kernel/kcsan/kcsan.h  |   2 +-
>  kernel/kcsan/report.c | 147 +++++++++++++++++++++---------------------
>  3 files changed, 77 insertions(+), 78 deletions(-)
> 
> diff --git a/kernel/kcsan/core.c b/kernel/kcsan/core.c
> index ee8200835b60..f1c38620e3cf 100644
> --- a/kernel/kcsan/core.c
> +++ b/kernel/kcsan/core.c
> @@ -321,7 +321,7 @@ static noinline void kcsan_found_watchpoint(const volatile void *ptr,
>  	flags = user_access_save();
>  
>  	if (consumed) {
> -		kcsan_report(ptr, size, type, true, raw_smp_processor_id(),
> +		kcsan_report(ptr, size, type, KCSAN_VALUE_CHANGE_MAYBE,
>  			     KCSAN_REPORT_CONSUMED_WATCHPOINT);
>  	} else {
>  		/*
> @@ -500,8 +500,7 @@ kcsan_setup_watchpoint(const volatile void *ptr, size_t size, int type)
>  		if (is_assert && value_change == KCSAN_VALUE_CHANGE_TRUE)
>  			kcsan_counter_inc(KCSAN_COUNTER_ASSERT_FAILURES);
>  
> -		kcsan_report(ptr, size, type, value_change, raw_smp_processor_id(),
> -			     KCSAN_REPORT_RACE_SIGNAL);
> +		kcsan_report(ptr, size, type, value_change, KCSAN_REPORT_RACE_SIGNAL);
>  	} else if (value_change == KCSAN_VALUE_CHANGE_TRUE) {
>  		/* Inferring a race, since the value should not have changed. */
>  
> @@ -511,7 +510,6 @@ kcsan_setup_watchpoint(const volatile void *ptr, size_t size, int type)
>  
>  		if (IS_ENABLED(CONFIG_KCSAN_REPORT_RACE_UNKNOWN_ORIGIN) || is_assert)
>  			kcsan_report(ptr, size, type, KCSAN_VALUE_CHANGE_TRUE,
> -				     raw_smp_processor_id(),
>  				     KCSAN_REPORT_RACE_UNKNOWN_ORIGIN);
>  	}
>  
> diff --git a/kernel/kcsan/kcsan.h b/kernel/kcsan/kcsan.h
> index e282f8b5749e..6630dfe32f31 100644
> --- a/kernel/kcsan/kcsan.h
> +++ b/kernel/kcsan/kcsan.h
> @@ -135,7 +135,7 @@ enum kcsan_report_type {
>   * Print a race report from thread that encountered the race.
>   */
>  extern void kcsan_report(const volatile void *ptr, size_t size, int access_type,
> -			 enum kcsan_value_change value_change, int cpu_id,
> +			 enum kcsan_value_change value_change,
>  			 enum kcsan_report_type type);
>  
>  #endif /* _KERNEL_KCSAN_KCSAN_H */
> diff --git a/kernel/kcsan/report.c b/kernel/kcsan/report.c
> index 18f9d3bc93a5..de234d1c1b3d 100644
> --- a/kernel/kcsan/report.c
> +++ b/kernel/kcsan/report.c
> @@ -19,18 +19,23 @@
>   */
>  #define NUM_STACK_ENTRIES 64
>  
> +/* Common access info. */
> +struct access_info {
> +	const volatile void	*ptr;
> +	size_t			size;
> +	int			access_type;
> +	int			task_pid;
> +	int			cpu_id;
> +};
> +
>  /*
>   * Other thread info: communicated from other racing thread to thread that set
>   * up the watchpoint, which then prints the complete report atomically. Only
>   * need one struct, as all threads should to be serialized regardless to print
>   * the reports, with reporting being in the slow-path.
>   */
> -static struct {
> -	const volatile void	*ptr;
> -	size_t			size;
> -	int			access_type;
> -	int			task_pid;
> -	int			cpu_id;
> +struct other_info {
> +	struct access_info	ai;
>  	unsigned long		stack_entries[NUM_STACK_ENTRIES];
>  	int			num_stack_entries;
>  
> @@ -52,7 +57,9 @@ static struct {
>  	 * that populated @other_info until it has been consumed.
>  	 */
>  	struct task_struct	*task;
> -} other_info;
> +};
> +
> +static struct other_info other_infos[1];
>  
>  /*
>   * Information about reported races; used to rate limit reporting.
> @@ -238,7 +245,7 @@ static const char *get_thread_desc(int task_id)
>  }
>  
>  /* Helper to skip KCSAN-related functions in stack-trace. */
> -static int get_stack_skipnr(unsigned long stack_entries[], int num_entries)
> +static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries)
>  {
>  	char buf[64];
>  	int skip = 0;
> @@ -279,9 +286,10 @@ static void print_verbose_info(struct task_struct *task)
>  /*
>   * Returns true if a report was generated, false otherwise.
>   */
> -static bool print_report(const volatile void *ptr, size_t size, int access_type,
> -			 enum kcsan_value_change value_change, int cpu_id,
> -			 enum kcsan_report_type type)
> +static bool print_report(enum kcsan_value_change value_change,
> +			 enum kcsan_report_type type,
> +			 const struct access_info *ai,
> +			 const struct other_info *other_info)
>  {
>  	unsigned long stack_entries[NUM_STACK_ENTRIES] = { 0 };
>  	int num_stack_entries = stack_trace_save(stack_entries, NUM_STACK_ENTRIES, 1);
> @@ -297,9 +305,9 @@ static bool print_report(const volatile void *ptr, size_t size, int access_type,
>  		return false;
>  
>  	if (type == KCSAN_REPORT_RACE_SIGNAL) {
> -		other_skipnr = get_stack_skipnr(other_info.stack_entries,
> -						other_info.num_stack_entries);
> -		other_frame = other_info.stack_entries[other_skipnr];
> +		other_skipnr = get_stack_skipnr(other_info->stack_entries,
> +						other_info->num_stack_entries);
> +		other_frame = other_info->stack_entries[other_skipnr];
>  
>  		/* @value_change is only known for the other thread */
>  		if (skip_report(value_change, other_frame))
> @@ -321,13 +329,13 @@ static bool print_report(const volatile void *ptr, size_t size, int access_type,
>  		 */
>  		cmp = sym_strcmp((void *)other_frame, (void *)this_frame);
>  		pr_err("BUG: KCSAN: %s in %ps / %ps\n",
> -		       get_bug_type(access_type | other_info.access_type),
> +		       get_bug_type(ai->access_type | other_info->ai.access_type),
>  		       (void *)(cmp < 0 ? other_frame : this_frame),
>  		       (void *)(cmp < 0 ? this_frame : other_frame));
>  	} break;
>  
>  	case KCSAN_REPORT_RACE_UNKNOWN_ORIGIN:
> -		pr_err("BUG: KCSAN: %s in %pS\n", get_bug_type(access_type),
> +		pr_err("BUG: KCSAN: %s in %pS\n", get_bug_type(ai->access_type),
>  		       (void *)this_frame);
>  		break;
>  
> @@ -341,30 +349,28 @@ static bool print_report(const volatile void *ptr, size_t size, int access_type,
>  	switch (type) {
>  	case KCSAN_REPORT_RACE_SIGNAL:
>  		pr_err("%s to 0x%px of %zu bytes by %s on cpu %i:\n",
> -		       get_access_type(other_info.access_type), other_info.ptr,
> -		       other_info.size, get_thread_desc(other_info.task_pid),
> -		       other_info.cpu_id);
> +		       get_access_type(other_info->ai.access_type), other_info->ai.ptr,
> +		       other_info->ai.size, get_thread_desc(other_info->ai.task_pid),
> +		       other_info->ai.cpu_id);
>  
>  		/* Print the other thread's stack trace. */
> -		stack_trace_print(other_info.stack_entries + other_skipnr,
> -				  other_info.num_stack_entries - other_skipnr,
> +		stack_trace_print(other_info->stack_entries + other_skipnr,
> +				  other_info->num_stack_entries - other_skipnr,
>  				  0);
>  
>  		if (IS_ENABLED(CONFIG_KCSAN_VERBOSE))
> -			print_verbose_info(other_info.task);
> +			print_verbose_info(other_info->task);
>  
>  		pr_err("\n");
>  		pr_err("%s to 0x%px of %zu bytes by %s on cpu %i:\n",
> -		       get_access_type(access_type), ptr, size,
> -		       get_thread_desc(in_task() ? task_pid_nr(current) : -1),
> -		       cpu_id);
> +		       get_access_type(ai->access_type), ai->ptr, ai->size,
> +		       get_thread_desc(ai->task_pid), ai->cpu_id);
>  		break;
>  
>  	case KCSAN_REPORT_RACE_UNKNOWN_ORIGIN:
>  		pr_err("race at unknown origin, with %s to 0x%px of %zu bytes by %s on cpu %i:\n",
> -		       get_access_type(access_type), ptr, size,
> -		       get_thread_desc(in_task() ? task_pid_nr(current) : -1),
> -		       cpu_id);
> +		       get_access_type(ai->access_type), ai->ptr, ai->size,
> +		       get_thread_desc(ai->task_pid), ai->cpu_id);
>  		break;
>  
>  	default:
> @@ -386,22 +392,23 @@ static bool print_report(const volatile void *ptr, size_t size, int access_type,
>  	return true;
>  }
>  
> -static void release_report(unsigned long *flags, enum kcsan_report_type type)
> +static void release_report(unsigned long *flags, struct other_info *other_info)
>  {
> -	if (type == KCSAN_REPORT_RACE_SIGNAL)
> -		other_info.ptr = NULL; /* mark for reuse */
> +	if (other_info)
> +		other_info->ai.ptr = NULL; /* Mark for reuse. */
>  
>  	spin_unlock_irqrestore(&report_lock, *flags);
>  }
>  
>  /*
> - * Sets @other_info.task and awaits consumption of @other_info.
> + * Sets @other_info->task and awaits consumption of @other_info.
>   *
>   * Precondition: report_lock is held.
>   * Postcondition: report_lock is held.
>   */
> -static void
> -set_other_info_task_blocking(unsigned long *flags, const volatile void *ptr)
> +static void set_other_info_task_blocking(unsigned long *flags,
> +					 const struct access_info *ai,
> +					 struct other_info *other_info)
>  {
>  	/*
>  	 * We may be instrumenting a code-path where current->state is already
> @@ -418,7 +425,7 @@ set_other_info_task_blocking(unsigned long *flags, const volatile void *ptr)
>  	 */
>  	int timeout = max(kcsan_udelay_task, kcsan_udelay_interrupt);
>  
> -	other_info.task = current;
> +	other_info->task = current;
>  	do {
>  		if (is_running) {
>  			/*
> @@ -438,19 +445,19 @@ set_other_info_task_blocking(unsigned long *flags, const volatile void *ptr)
>  		spin_lock_irqsave(&report_lock, *flags);
>  		if (timeout-- < 0) {
>  			/*
> -			 * Abort. Reset other_info.task to NULL, since it
> +			 * Abort. Reset @other_info->task to NULL, since it
>  			 * appears the other thread is still going to consume
>  			 * it. It will result in no verbose info printed for
>  			 * this task.
>  			 */
> -			other_info.task = NULL;
> +			other_info->task = NULL;
>  			break;
>  		}
>  		/*
>  		 * If @ptr nor @current matches, then our information has been
>  		 * consumed and we may continue. If not, retry.
>  		 */
> -	} while (other_info.ptr == ptr && other_info.task == current);
> +	} while (other_info->ai.ptr == ai->ptr && other_info->task == current);
>  	if (is_running)
>  		set_current_state(TASK_RUNNING);
>  }
> @@ -460,9 +467,8 @@ set_other_info_task_blocking(unsigned long *flags, const volatile void *ptr)
>   * acquires the matching other_info and returns true. If other_info is not
>   * required for the report type, simply acquires report_lock and returns true.
>   */
> -static bool prepare_report(unsigned long *flags, const volatile void *ptr,
> -			   size_t size, int access_type, int cpu_id,
> -			   enum kcsan_report_type type)
> +static bool prepare_report(unsigned long *flags, enum kcsan_report_type type,
> +			   const struct access_info *ai, struct other_info *other_info)
>  {
>  	if (type != KCSAN_REPORT_CONSUMED_WATCHPOINT &&
>  	    type != KCSAN_REPORT_RACE_SIGNAL) {
> @@ -476,18 +482,14 @@ static bool prepare_report(unsigned long *flags, const volatile void *ptr,
>  
>  	switch (type) {
>  	case KCSAN_REPORT_CONSUMED_WATCHPOINT:
> -		if (other_info.ptr != NULL)
> +		if (other_info->ai.ptr)
>  			break; /* still in use, retry */
>  
> -		other_info.ptr			= ptr;
> -		other_info.size			= size;
> -		other_info.access_type		= access_type;
> -		other_info.task_pid		= in_task() ? task_pid_nr(current) : -1;
> -		other_info.cpu_id		= cpu_id;
> -		other_info.num_stack_entries	= stack_trace_save(other_info.stack_entries, NUM_STACK_ENTRIES, 1);
> +		other_info->ai = *ai;
> +		other_info->num_stack_entries = stack_trace_save(other_info->stack_entries, NUM_STACK_ENTRIES, 1);
>  
>  		if (IS_ENABLED(CONFIG_KCSAN_VERBOSE))
> -			set_other_info_task_blocking(flags, ptr);
> +			set_other_info_task_blocking(flags, ai, other_info);
>  
>  		spin_unlock_irqrestore(&report_lock, *flags);
>  
> @@ -498,37 +500,31 @@ static bool prepare_report(unsigned long *flags, const volatile void *ptr,
>  		return false;
>  
>  	case KCSAN_REPORT_RACE_SIGNAL:
> -		if (other_info.ptr == NULL)
> +		if (!other_info->ai.ptr)
>  			break; /* no data available yet, retry */
>  
>  		/*
>  		 * First check if this is the other_info we are expecting, i.e.
>  		 * matches based on how watchpoint was encoded.
>  		 */
> -		if (!matching_access((unsigned long)other_info.ptr &
> -					     WATCHPOINT_ADDR_MASK,
> -				     other_info.size,
> -				     (unsigned long)ptr & WATCHPOINT_ADDR_MASK,
> -				     size))
> +		if (!matching_access((unsigned long)other_info->ai.ptr & WATCHPOINT_ADDR_MASK, other_info->ai.size,
> +				     (unsigned long)ai->ptr & WATCHPOINT_ADDR_MASK, ai->size))
>  			break; /* mismatching watchpoint, retry */
>  
> -		if (!matching_access((unsigned long)other_info.ptr,
> -				     other_info.size, (unsigned long)ptr,
> -				     size)) {
> +		if (!matching_access((unsigned long)other_info->ai.ptr, other_info->ai.size,
> +				     (unsigned long)ai->ptr, ai->size)) {
>  			/*
>  			 * If the actual accesses to not match, this was a false
>  			 * positive due to watchpoint encoding.
>  			 */
> -			kcsan_counter_inc(
> -				KCSAN_COUNTER_ENCODING_FALSE_POSITIVES);
> +			kcsan_counter_inc(KCSAN_COUNTER_ENCODING_FALSE_POSITIVES);
>  
>  			/* discard this other_info */
> -			release_report(flags, KCSAN_REPORT_RACE_SIGNAL);
> +			release_report(flags, other_info);
>  			return false;
>  		}
>  
> -		access_type |= other_info.access_type;
> -		if ((access_type & KCSAN_ACCESS_WRITE) == 0) {
> +		if (!((ai->access_type | other_info->ai.access_type) & KCSAN_ACCESS_WRITE)) {
>  			/*
>  			 * While the address matches, this is not the other_info
>  			 * from the thread that consumed our watchpoint, since
> @@ -561,15 +557,11 @@ static bool prepare_report(unsigned long *flags, const volatile void *ptr,
>  			 * data, and at this point the likelihood that we
>  			 * re-report the same race again is high.
>  			 */
> -			release_report(flags, KCSAN_REPORT_RACE_SIGNAL);
> +			release_report(flags, other_info);
>  			return false;
>  		}
>  
> -		/*
> -		 * Matching & usable access in other_info: keep other_info_lock
> -		 * locked, as this thread consumes it to print the full report;
> -		 * unlocked in release_report.
> -		 */
> +		/* Matching access in other_info. */
>  		return true;
>  
>  	default:
> @@ -582,10 +574,19 @@ static bool prepare_report(unsigned long *flags, const volatile void *ptr,
>  }
>  
>  void kcsan_report(const volatile void *ptr, size_t size, int access_type,
> -		  enum kcsan_value_change value_change, int cpu_id,
> +		  enum kcsan_value_change value_change,
>  		  enum kcsan_report_type type)
>  {
>  	unsigned long flags = 0;
> +	const struct access_info ai = {
> +		.ptr		= ptr,
> +		.size		= size,
> +		.access_type	= access_type,
> +		.task_pid	= in_task() ? task_pid_nr(current) : -1,
> +		.cpu_id		= raw_smp_processor_id()
> +	};
> +	struct other_info *other_info = type == KCSAN_REPORT_RACE_UNKNOWN_ORIGIN
> +					? NULL : &other_infos[0];
>  
>  	/*
>  	 * With TRACE_IRQFLAGS, lockdep's IRQ trace state becomes corrupted if
> @@ -596,19 +597,19 @@ void kcsan_report(const volatile void *ptr, size_t size, int access_type,
>  	lockdep_off();
>  
>  	kcsan_disable_current();
> -	if (prepare_report(&flags, ptr, size, access_type, cpu_id, type)) {
> +	if (prepare_report(&flags, type, &ai, other_info)) {
>  		/*
>  		 * Never report if value_change is FALSE, only if we it is
>  		 * either TRUE or MAYBE. In case of MAYBE, further filtering may
>  		 * be done once we know the full stack trace in print_report().
>  		 */
>  		bool reported = value_change != KCSAN_VALUE_CHANGE_FALSE &&
> -				print_report(ptr, size, access_type, value_change, cpu_id, type);
> +				print_report(value_change, type, &ai, other_info);
>  
>  		if (reported && panic_on_warn)
>  			panic("panic_on_warn set ...\n");
>  
> -		release_report(&flags, type);
> +		release_report(&flags, other_info);
>  	}
>  	kcsan_enable_current();
>  
> -- 
> 2.25.1.481.gfbce0eb801-goog
> 

Powered by blists - more mailing lists