svn commit: r365326 - stable/12/sys/sys

[Konstantin Belousov]

On Fri, Sep 04, 2020 at 11:22:18AM +0000, Marcin Wojtas wrote:
> Author: mw
> Date: Fri Sep  4 11:22:18 2020
> New Revision: 365326
> URL: https://svnweb.freebsd.org/changeset/base/365326
> 
> Log:
>   MFC: r346593
>   
>   Add barrier in buf ring peek function to prevent race in ARM and ARM64.
>   
>   Obtained from: Semihalf
>   Sponsored by: Amazon, Inc.
> 
> Modified:
>   stable/12/sys/sys/buf_ring.h
> Directory Properties:
>   stable/12/   (props changed)
> 
> Modified: stable/12/sys/sys/buf_ring.h
> ==============================================================================
> --- stable/12/sys/sys/buf_ring.h	Fri Sep  4 04:31:56 2020	(r365325)
> +++ stable/12/sys/sys/buf_ring.h	Fri Sep  4 11:22:18 2020	(r365326)
> @@ -310,14 +310,23 @@ buf_ring_peek_clear_sc(struct buf_ring *br)
>  	if (!mtx_owned(br->br_lock))
>  		panic("lock not held on single consumer dequeue");
>  #endif	
> -	/*
> -	 * I believe it is safe to not have a memory barrier
> -	 * here because we control cons and tail is worst case
> -	 * a lagging indicator so we worst case we might
> -	 * return NULL immediately after a buffer has been enqueued
> -	 */
> +
>  	if (br->br_cons_head == br->br_prod_tail)
>  		return (NULL);
> +
> +#if defined(__arm__) || defined(__aarch64__)
> +	/*
> +	 * The barrier is required there on ARM and ARM64 to ensure, that
> +	 * br->br_ring[br->br_cons_head] will not be fetched before the above
> +	 * condition is checked.
> +	 * Without the barrier, it is possible, that buffer will be fetched
> +	 * before the enqueue will put mbuf into br, then, in the meantime, the
> +	 * enqueue will update the array and the br_prod_tail, and the
> +	 * conditional check will be true, so we will return previously fetched
> +	 * (and invalid) buffer.
> +	 */
> +	atomic_thread_fence_acq();
> +#endif
Putting the semantic of the change aside, why did you added the fence (it is
a fence, not barrier as stated in the comment) only to arm* ?   If it is
needed, it is needed for all arches.

>  
>  #ifdef DEBUG_BUFRING
>  	/*