Messages in this thread | | | From | Daniel Vetter <> | Date | Fri, 9 Oct 2020 16:24:45 +0200 | Subject | Re: [PATCH v2 14/17] resource: Move devmem revoke code to resource framework |
| |
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").
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.
Now iomem_get_mapping otoh can only be called after userspace is up & running, so way after all the fs_initcalls are guaranteed to have fininshed. Hence we don't really need anything there. But I expect the kernel race checker thing to complain, plus that then gives me a good spot to explain why we can't race and don't have to check for a NULL iomem_inode.
I'll add that in v3. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
| |