diff mbox series

/dev/mem: Add missing memory barriers for devmem_inode

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

Commit Message

Eric Biggers July 16, 2020, 6:05 a.m. UTC 
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>
---
 drivers/char/mem.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)


base-commit: f8456690ba8eb18ea4714e68554e242a04f65cff

Comments

Dan Williams July 16, 2020, 3:53 p.m. UTC | #1 
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 mbox series

Patch

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;
 }