Messages in this thread Patch in this message | | | Date | Thu, 24 Feb 2011 20:54:06 -0300 | From | Herton Ronaldo Krzesinski <> | Subject | Re: Linux 2.6.38-rc6 |
| |
On Thu, Feb 24, 2011 at 02:21:15PM -0300, Herton Ronaldo Krzesinski wrote: > On Thu, Feb 24, 2011 at 08:37:11AM -0800, Linus Torvalds wrote: > > On Thu, Feb 24, 2011 at 5:20 AM, Anca Emanuel <anca.emanuel@gmail.com> wrote: > > >> > > >> Every boot? > > > > > > Yes. > > > > > >> And just out of interest, what happens if you don't have the vesafb > > >> driver at all? > > > > > > I used 'e' option from grub, removed the 'set gfxpayload = $linux_gfx_mode' > > > and it works. > > > > > > dmesg: http://pastebin.com/JAZsk4vD > > > > Hmm. So it definitely seems to be the hand-over. > > > > Does this patch make any difference? When we unregister the old > > framebuffer, we still leave it in the registered_fb[] array, which > > looks wrong. But it would also be interesting to hear if setting > > CONFIG_SLUB_DEBUG_ON or CONFIG_DEBUG_PAGEALLOC makes any difference > > (they'd help detect accesses to free'd data structures). > > Hi Linus, > > I opened a bug about this issue in January, while I was still working > with Mandriva and got a similar issue reported. Basically it's a race on > vesafb removal with i915 with modesetting enabled. And indeed you have > to use slub_debug to always reproduce it, sometimes the use after free > of struct fb_info not always trigers it. I posted a testcase and a > proposed patch at https://bugzilla.kernel.org/show_bug.cgi?id=26232
Sorry, I have a correction to this.
What I wrote here is confusing, the problem should happen on any "firmware" framebuffer which gets replaced by any modesetting framebuffer, like intelfb or nouveaufb, not only i915 as it can be understood from what I stated. Just the test case I made and problem reported was with i915, but same holds for nouveaufb as reported here.
The oops here first is because struct fb_info of vesafb is freed while plymouthd has fb opened. In fb_open, we assign info to file->private_data. So if the application opens it, and before it is closed some framebuffer from drm (intelfb, nouveaufb...) replaces vesafb, remove_conflicting_framebuffers removes the vesafb. Inside remove_conflicting_framebuffers we call unregister_framebuffer, which in the end will call fb_info->fbops->fb_destroy (vesafb_destroy) -> framebuffer_release(info) -> kfree(info)
Then if application closes its file descriptor after the drm framebuffer loaded, it still has the reference of struct fb_info of vesafb in file->private_data, then we get the oops as it tries to dereference the info already freed.
But there is also races in this framebuffer removal also while it is being unregistered, the accesses to registered_fb[] array, so I made the testcase and attached to the bug report to show them.
> > I remember to have posted here on LKML the patch too, but didn't got > answers to it. > > Andy Whitcroft fixed it too with a similar patch, > http://kernel.ubuntu.com/git?p=ubuntu/ubuntu-natty.git;a=commit;h=c5a742b5f78e161d6a13853a7e3e6e1dfa429e69 > I CC'd Andy, the author of the patch, he will push his version, looks > more complete as it takes care of mm_lock in do_mmap too. > > My bug report has also another test case and fix for a inverse locking > problem, it would be good to take a look too. > > In any case, any of these problems are not recent regressions. The race > on framebuffer removal at least exists since unregister_framebuffer > started to be used to remove it while loading framebuffer from modesetting > drivers. > > > > > Linus > > > drivers/video/fbmem.c | 1 + > > 1 files changed, 1 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c > > index e2bf953..e8f8925 100644 > > --- a/drivers/video/fbmem.c > > +++ b/drivers/video/fbmem.c > > @@ -1511,6 +1511,7 @@ void remove_conflicting_framebuffers(struct apertures_struct *a, > > "%s vs %s - removing generic driver\n", > > name, registered_fb[i]->fix.id); > > unregister_framebuffer(registered_fb[i]); > > + registered_fb[i] = NULL; > > } > > } > > } > > > -- > []'s > Herton
-- []'s Herton
| |