Re: + list-prevent-compiler-reloads-inside-safe-list-iteration.patch added to -mm tree

["Paul E. McKenney" <paulmck@kernel.org>]

On Wed, Mar 11, 2020 at 07:58:09PM -0700, Andrew Morton wrote:
> 
> The patch titled
>      Subject: lib/list: prevent compiler reloads inside 'safe' list iteration
> has been added to the -mm tree.  Its filename is
>      list-prevent-compiler-reloads-inside-safe-list-iteration.patch
> 
> This patch should soon appear at
>     http://ozlabs.org/~akpm/mmots/broken-out/list-prevent-compiler-reloads-inside-safe-list-iteration.patch
> and later at
>     http://ozlabs.org/~akpm/mmotm/broken-out/list-prevent-compiler-reloads-inside-safe-list-iteration.patch
> 
> Before you just go and hit "reply", please:
>    a) Consider who else should be cc'ed
>    b) Prefer to cc a suitable mailing list as well
>    c) Ideally: find the original patch on the mailing list and do a
>       reply-to-all to that, adding suitable additional cc's
> 
> *** Remember to use Documentation/process/submit-checklist.rst when testing your code ***
> 
> The -mm tree is included into linux-next and is updated
> there every 3-4 working days
> 
> ------------------------------------------------------
> From: Chris Wilson <chris@chris-wilson.co.uk>
> Subject: lib/list: prevent compiler reloads inside 'safe' list iteration
> 
> Instruct the compiler to read the next element in the list iteration
> once, and that it is not allowed to reload the value from the stale
> element later. This is important as during the course of the safe
> iteration, the stale element may be poisoned (unbeknownst to the
> compiler).
> 
> This helps prevent kcsan warnings over 'unsafe' conduct in releasing the
> list elements during list_for_each_entry_safe() and friends.
> 
> Link: http://lkml.kernel.org/r/20200310092119.14965-1-chris@chris-wilson.co.uk
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: "Paul E. McKenney" <paulmck@kernel.org>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: David Laight <David.Laight@ACULAB.COM>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Marco Elver <elver@google.com>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

One possible concern is that we are overloading the suffix "_safe()",
but that looks better to me than yet another explosion of this API.
It would be good to have uses, of course.

Reviewed-by: Paul E. McKenney <paulmck@kernel.org>

> ---
> 
>  include/linux/list.h |   50 +++++++++++++++++++++++++++++------------
>  1 file changed, 36 insertions(+), 14 deletions(-)
> 
> --- a/include/linux/list.h~list-prevent-compiler-reloads-inside-safe-list-iteration
> +++ a/include/linux/list.h
> @@ -537,6 +537,17 @@ static inline void list_splice_tail_init
>  	list_entry((pos)->member.next, typeof(*(pos)), member)
>  
>  /**
> + * list_next_entry_safe - get the next element in list [once]
> + * @pos:	the type * to cursor
> + * @member:	the name of the list_head within the struct.
> + *
> + * Like list_next_entry() but prevents the compiler from reloading the
> + * next element.
> + */
> +#define list_next_entry_safe(pos, member) \
> +	list_entry(READ_ONCE((pos)->member.next), typeof(*(pos)), member)
> +
> +/**
>   * list_prev_entry - get the prev element in list
>   * @pos:	the type * to cursor
>   * @member:	the name of the list_head within the struct.
> @@ -545,6 +556,17 @@ static inline void list_splice_tail_init
>  	list_entry((pos)->member.prev, typeof(*(pos)), member)
>  
>  /**
> + * list_prev_entry_safe - get the prev element in list [once]
> + * @pos:	the type * to cursor
> + * @member:	the name of the list_head within the struct.
> + *
> + * Like list_prev_entry() but prevents the compiler from reloading the
> + * previous element.
> + */
> +#define list_prev_entry_safe(pos, member) \
> +	list_entry(READ_ONCE((pos)->member.prev), typeof(*(pos)), member)
> +
> +/**
>   * list_for_each	-	iterate over a list
>   * @pos:	the &struct list_head to use as a loop cursor.
>   * @head:	the head for your list.
> @@ -686,9 +708,9 @@ static inline void list_splice_tail_init
>   */
>  #define list_for_each_entry_safe(pos, n, head, member)			\
>  	for (pos = list_first_entry(head, typeof(*pos), member),	\
> -		n = list_next_entry(pos, member);			\
> +		n = list_next_entry_safe(pos, member);			\
>  	     &pos->member != (head); 					\
> -	     pos = n, n = list_next_entry(n, member))
> +	     pos = n, n = list_next_entry_safe(n, member))
>  
>  /**
>   * list_for_each_entry_safe_continue - continue list iteration safe against removal
> @@ -700,11 +722,11 @@ static inline void list_splice_tail_init
>   * Iterate over list of given type, continuing after current point,
>   * safe against removal of list entry.
>   */
> -#define list_for_each_entry_safe_continue(pos, n, head, member) 		\
> -	for (pos = list_next_entry(pos, member), 				\
> -		n = list_next_entry(pos, member);				\
> -	     &pos->member != (head);						\
> -	     pos = n, n = list_next_entry(n, member))
> +#define list_for_each_entry_safe_continue(pos, n, head, member) 	\
> +	for (pos = list_next_entry(pos, member), 			\
> +		n = list_next_entry_safe(pos, member);			\
> +	     &pos->member != (head);					\
> +	     pos = n, n = list_next_entry_safe(n, member))
>  
>  /**
>   * list_for_each_entry_safe_from - iterate over list from current point safe against removal
> @@ -716,10 +738,10 @@ static inline void list_splice_tail_init
>   * Iterate over list of given type from current point, safe against
>   * removal of list entry.
>   */
> -#define list_for_each_entry_safe_from(pos, n, head, member) 			\
> -	for (n = list_next_entry(pos, member);					\
> -	     &pos->member != (head);						\
> -	     pos = n, n = list_next_entry(n, member))
> +#define list_for_each_entry_safe_from(pos, n, head, member) 		\
> +	for (n = list_next_entry_safe(pos, member);			\
> +	     &pos->member != (head);					\
> +	     pos = n, n = list_next_entry_safe(n, member))
>  
>  /**
>   * list_for_each_entry_safe_reverse - iterate backwards over list safe against removal
> @@ -733,9 +755,9 @@ static inline void list_splice_tail_init
>   */
>  #define list_for_each_entry_safe_reverse(pos, n, head, member)		\
>  	for (pos = list_last_entry(head, typeof(*pos), member),		\
> -		n = list_prev_entry(pos, member);			\
> +		n = list_prev_entry_safe(pos, member);			\
>  	     &pos->member != (head); 					\
> -	     pos = n, n = list_prev_entry(n, member))
> +	     pos = n, n = list_prev_entry_safe(n, member))
>  
>  /**
>   * list_safe_reset_next - reset a stale list_for_each_entry_safe loop
> @@ -750,7 +772,7 @@ static inline void list_splice_tail_init
>   * completing the current iteration of the loop body.
>   */
>  #define list_safe_reset_next(pos, n, member)				\
> -	n = list_next_entry(pos, member)
> +	n = list_next_entry_safe(pos, member)
>  
>  /*
>   * Double linked lists with a single pointer list head.
> _
> 
> Patches currently in -mm which might be from chris@chris-wilson.co.uk are
> 
> list-prevent-compiler-reloads-inside-safe-list-iteration.patch
>