[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAKMK7uFpPP-Q0jC0vM7vYPEcg0m4NzTw+Ld=swdTF3BgMX5Qug@mail.gmail.com>
Date: Fri, 9 Oct 2020 16:24:45 +0200
From: Daniel Vetter <daniel.vetter@...ll.ch>
To: Jason Gunthorpe <jgg@...pe.ca>
Cc: DRI Development <dri-devel@...ts.freedesktop.org>,
LKML <linux-kernel@...r.kernel.org>,
KVM list <kvm@...r.kernel.org>, Linux MM <linux-mm@...ck.org>,
Linux ARM <linux-arm-kernel@...ts.infradead.org>,
linux-samsung-soc <linux-samsung-soc@...r.kernel.org>,
"open list:DMA BUFFER SHARING FRAMEWORK"
<linux-media@...r.kernel.org>,
linux-s390 <linux-s390@...r.kernel.org>,
Daniel Vetter <daniel.vetter@...el.com>,
Kees Cook <keescook@...omium.org>,
Dan Williams <dan.j.williams@...el.com>,
Andrew Morton <akpm@...ux-foundation.org>,
John Hubbard <jhubbard@...dia.com>,
Jérôme Glisse <jglisse@...hat.com>,
Jan Kara <jack@...e.cz>, Arnd Bergmann <arnd@...db.de>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
David Hildenbrand <david@...hat.com>,
"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>
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@...pe.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
Powered by blists - more mailing lists