Message ID | 20200716060553.24618-1-ebiggers@kernel.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | /dev/mem: Add missing memory barriers for devmem_inode | expand |
On Wed, Jul 15, 2020 at 11:07 PM Eric Biggers <ebiggers@kernel.org> wrote: > > From: Eric Biggers <ebiggers@google.com> > > WRITE_ONCE() isn't the correct way to publish a pointer to a data > structure, since it doesn't include a write memory barrier. Therefore > other tasks may see that the pointer has been set but not see that the > pointed-to memory has finished being initialized yet. Instead a > primitive with "release" semantics is needed. > > Use smp_store_release() for this. > > The use of READ_ONCE() on the read side is still potentially correct if > there's no control dependency, i.e. if all memory being "published" is > transitively reachable via the pointer itself. But this pairing is > somewhat confusing and error-prone. So just upgrade the read side to > smp_load_acquire() so that it clearly pairs with smp_store_release(). > > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Kees Cook <keescook@chromium.org> > Cc: Matthew Wilcox <willy@infradead.org> > Cc: Russell King <linux@arm.linux.org.uk> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Fixes: 3234ac664a87 ("/dev/mem: Revoke mappings when a driver claims the region") > Signed-off-by: Eric Biggers <ebiggers@google.com> Makes sense: Acked-by: Dan Williams <dan.j.williams@intel.com>
diff --git a/drivers/char/mem.c b/drivers/char/mem.c index 934c92dcb9ab..687d4af6945d 100644 --- a/drivers/char/mem.c +++ b/drivers/char/mem.c @@ -814,7 +814,8 @@ static struct inode *devmem_inode; #ifdef CONFIG_IO_STRICT_DEVMEM void revoke_devmem(struct resource *res) { - struct inode *inode = READ_ONCE(devmem_inode); + /* pairs with smp_store_release() in devmem_init_inode() */ + struct inode *inode = smp_load_acquire(&devmem_inode); /* * Check that the initialization has completed. Losing the race @@ -1028,8 +1029,11 @@ static int devmem_init_inode(void) return rc; } - /* publish /dev/mem initialized */ - WRITE_ONCE(devmem_inode, inode); + /* + * Publish /dev/mem initialized. + * Pairs with smp_load_acquire() in revoke_devmem(). + */ + smp_store_release(&devmem_inode, inode); return 0; }