Re: [PATCH net] net: devmem: use READ_ONCE/WRITE_ONCE on binding->dev

[Stanislav Fomichev <stfomichev@gmail.com>]

On 02/23, Bobby Eshleman wrote:
> From: Bobby Eshleman <bobbyeshleman@meta.com>
> 
> binding->dev is protected on the write-side in
> mp_dmabuf_devmem_uninstall() against concurrent writes, but due to the
> concurrent bare read in net_devmem_get_binding() it should be wrapped in
> a READ_ONCE/WRITE_ONCE pair to make sure no compiler optimizations play
> with the underlying register in unforeseen ways.
> 
> Fixes: bd61848900bf ("net: devmem: Implement TX path")
> Signed-off-by: Bobby Eshleman <bobbyeshleman@meta.com>
> ---
> Note1: This didn't crop up in a discrete error, but just something that
> didn't seem to quite follow my understanding of memory-barriers.txt, as
> frail and feeble as that understanding may be.
> 
> Note2: the "Fixes" commit I referenced is the first one to introduce
> binding->dev bare accesses, but the later patch '6a2108c78069 ("net:
> devmem: refresh devmem TX dst in case of route invalidation")' carried
> that forward. I wasn't sure which was the ideal one to select for the
> "Fixes" label.
> ---
>  net/core/devmem.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/devmem.c b/net/core/devmem.c
> index 63f093f7d2b2..cb989949d43c 100644
> --- a/net/core/devmem.c
> +++ b/net/core/devmem.c
> @@ -398,7 +398,8 @@ struct net_devmem_dmabuf_binding *net_devmem_get_binding(struct sock *sk,
>  	 * net_device.
>  	 */
>  	dst_dev = dst_dev_rcu(dst);
> -	if (unlikely(!dst_dev) || unlikely(dst_dev != binding->dev)) {
> +	if (unlikely(!dst_dev) ||
> +	    unlikely(dst_dev != READ_ONCE(binding->dev))) {
>  		err = -ENODEV;
>  		goto out_unlock;
>  	}

What about the other similar check in validate_xmit_unreadable_skb?

I don't have a strong opinion, but it feels like as long as we are not
using these ->dev pointers (and we are only using them for comparisons),
we should be fine (plus, memory tearing for u64 is not something that
can happen?).