On Fri, Oct 09, 2020 at 04:24:45PM +0200, Daniel Vetter wrote:
On Fri, Oct 9, 2020 at 2:31 PM Jason Gunthorpe jgg@ziepe.ca wrote:
On Fri, Oct 09, 2020 at 09:59:31AM +0200, Daniel Vetter wrote:
+struct address_space *iomem_get_mapping(void) +{
return iomem_inode->i_mapping;This should pair an acquire with the release below
/** Publish /dev/mem initialized.* Pairs with smp_load_acquire() in revoke_iomem().*/smp_store_release(&iomem_inode, inode);However, this seems abnormal, initcalls rarely do this kind of stuff with global data..
The kernel crashes if this fs_initcall is raced with iomem_get_mapping() due to the unconditional dereference, so I think it can be safely switched to a simple assignment.
Ah yes I checked this all, but forgot to correctly annotate the iomem_get_mapping access. For reference, see b34e7e298d7a ("/dev/mem: Add missing memory barriers for devmem_inode").
Oh yikes, so revoke_iomem can run concurrently during early boot, tricky.
The reasons for the annotations is that iomem requests can happen fairly early, way before fs_initcalls happen. That means revoke_iomem needs to check for that and bail out if we race - nothing bad can happen since userspace isn't running at this point anyway. And apparently it needs to be a full acquire fence since we don't just write a value, but need a barrier for the struct stuff.
Yes, if that is what is happening it release/acquire is needed.
Jason