Skip to contentSkip to conversation

/dev/mem: Add missing memory barriers for devmem_inode

PATCH In review Eric Biggers 2 participants 2 messages Jul 16, 2020

Subsystems (2)
Memory management Char and misc drivers
lore
Share & tools
lore

Eric Biggers

author
#0 lore raw

PATCH 1/1 /dev/mem: Add missing memory barriers for devmem_inode

From: Eric Biggers <[email protected]>

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 <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Kees Cook <[email protected]>
Cc Matthew Wilcox <[email protected]>
Cc Russell King <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Fixes 3234ac664a87 ("/dev/mem: Revoke mappings when a driver claims the region")
Signed-off-by Eric Biggers <[email protected]>
---
 drivers/char/mem.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
drivers/char/mem.c +7 -3
index 934c92dcb9ab..687d4af6945d 100644
@@ -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;
 }

base-commit: f8456690ba8eb18ea4714e68554e242a04f65cff

Signature · 1 line
2.27.0
Acked-by on v1 1/1
#1 lore raw
Quoted 17 lines · Eric Biggers

From: Eric Biggers <[email protected]>

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

Quoted 10 lines
Cc Dan Williams <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Kees Cook <[email protected]>
Cc Matthew Wilcox <[email protected]>
Cc Russell King <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Fixes 3234ac664a87 ("/dev/mem: Revoke mappings when a driver claims the region")
Signed-off-by Eric Biggers <[email protected]>

Makes sense:

Acked-by Dan Williams <[email protected]>