Re: Linux 2.6.38-rc6

[Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>]

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