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

[Bobby Eshleman <bobbyeshleman@gmail.com>]

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