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

[Bobby Eshleman <bobbyeshleman@gmail.com>]

On Wed, Feb 25, 2026 at 11:49:31AM -0800, Bobby Eshleman wrote:
> On Wed, Feb 25, 2026 at 09:31:48AM -0800, Mina Almasry wrote:
> > On Wed, Feb 25, 2026 at 7:14 AM Bobby Eshleman <bobbyeshleman@gmail.com> wrote:
> > >
> > > 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>
> > 
> > Looks correct to me, and AFAIU Stan is right, we might as well
> > annotate all the reads of ->dev as technically there could be a dmabuf
> > uninstall happing concurrently on another CPU. I also think it's
> > probably good to annotate potential races.
> > 
> > The ->dev write in dmabuf binding doesn't need WRITE_ONCE annotation I
> > guess because it's initialization, it can't race with any reads.
> > 
> > This makes me wonder what other fields in dmabuf need annotations. I
> > hope I didn't miss many more.
> > 
> > I would add this is really not a critical bug because
> > net_devmem_get_binding() is in TX path, and it is more than fine here
> > if we fail this check if there is an unbind happening in paraller with
> > sendmsg(), but it's probably good to annotate potential races anyway.
> > 
> 
> Sounds good. I'll take a look at some of the other fields while my mind
> is in this space.

I looked through the other fields of binding and binding->dev is the
only one that wants READ_ONCE/WRITE_ONCE AFAICT. Most of them are only
modified after the refcount hits zero (dmabuf, tx_vec, chunk_pool,
attachment, etc..) and I think binding->list is protected by the netlink
priv->lock.