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

[Bobby Eshleman <bobbyeshleman@gmail.com>]

On Tue, Feb 24, 2026 at 05:49:42PM -0800, Stanislav Fomichev wrote:
> 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?).

Makes sense. I don't think it presents a current problem, its just
defensive (e.g., someday other functions referencing binding->dev get
inlined here and the compiler does load omission or invented loads). I
didn't know about u64 being immune to memory tearing.

If it doesn't look like an issue to anyone else, I'll not try to push on
this.

Best,
Bobby