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

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

[Bobby Eshleman]

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;
 	}
@@ -515,7 +516,7 @@ static void mp_dmabuf_devmem_uninstall(void *mp_priv,
 			xa_erase(&binding->bound_rxqs, xa_idx);
 			if (xa_empty(&binding->bound_rxqs)) {
 				mutex_lock(&binding->lock);
-				binding->dev = NULL;
+				WRITE_ONCE(binding->dev, NULL);
 				mutex_unlock(&binding->lock);
 			}
 			break;

---
base-commit: d4f687fbbce45b5e88438e89b5e26c0c15847992
change-id: 20260223-devmem-membar-fix-3a5cd9618f8a

Best regards,
-- 
Bobby Eshleman <bobbyeshleman@meta.com>

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

[Stanislav Fomichev]

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?).

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

[Bobby Eshleman]

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

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

[Mina Almasry]

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.

-- 
Thanks,
Mina

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

[Bobby Eshleman]

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 agree about it being non-critical, good point about TX passing through
fine on failed check.

Best,
Bobby

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

[Bobby Eshleman]

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.

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

[Bobby Eshleman]

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.
> 
> -- 
> Thanks,
> Mina

I also noticed in tools/memory-model/Documentation/access-marking.txt
section "Lock-Protected Writes With Lockless Reads" it is recommended to
also use ASSERT_EXCLUSIVE_WRITER(), with explanation:

"The purpose of the ASSERT_EXCLUSIVE_WRITER() is to allow KCSAN to check
for a buggy concurrent write, whether marked or not."

Do we think it is worth adding here? I'd think so but didn't want to
throw it in without checking.

Best,
Bobby