Re: [PATCH] initialize fb_info->lock mutex in the framebuffer_alloc()

Re: [PATCH] initialize fb_info->lock mutex in the framebuffer_alloc()

[Ville Syrjälä]

On Sun, May 03, 2009 at 11:30:08PM +0200, Krzysztof Helt wrote:
> From: Krzysztof Helt <krzysztof.h1@wp.pl>
> 
> The fb_info->lock mutex should be initialized inside 
> the framebuffer_alloc() function. Otherwise, someone
> may call a fb layer function which indirectly uses uninitialized 
> mutex.

How can that someone find the fb_info without it being in the
registered_fb[] array?

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/

------------------------------------------------------------------------------
Register Now & Save for Velocity, the Web Performance & Operations 
Conference from O'Reilly Media. Velocity features a full day of 
expert-led, hands-on workshops and two days of sessions from industry 
leaders in dedicated Performance & Operations tracks. Use code vel09scf 
and Save an extra 15% before 5/3. http://p.sf.net/sfu/velocityconf

Re: [PATCH] initialize fb_info->lock mutex in the framebuffer_alloc()

[Geert Uytterhoeven]

On Sun, May 3, 2009 at 23:30, Krzysztof Helt <krzysztof.h1@poczta.fm> wrote:
> From: Krzysztof Helt <krzysztof.h1@wp.pl>
>
> The fb_info->lock mutex should be initialized inside
> the framebuffer_alloc() function. Otherwise, someone
> may call a fb layer function which indirectly uses uninitialized
> mutex.
> Move initialization of the mutex from the register_framebuffer()
> to framebuffer_alloc().
>
> This may cause problems for drivers which do not use
> the framebuffer_alloc() to allocate the fb_info structure.
> Convert the two last drivers to use the framebuffer_alloc().

> diff -urp linux-orig/drivers/video/igafb.c linux-2.6.30/drivers/video/igafb.c
> diff -urp linux-orig/drivers/video/offb.c linux-2.6.30/drivers/video/offb.c

Are these really the only 2 remaining ones? What about amifb and atafb
(from the top
of my head)?

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

------------------------------------------------------------------------------
Register Now & Save for Velocity, the Web Performance & Operations 
Conference from O'Reilly Media. Velocity features a full day of 
expert-led, hands-on workshops and two days of sessions from industry 
leaders in dedicated Performance & Operations tracks. Use code vel09scf 
and Save an extra 15% before 5/3. http://p.sf.net/sfu/velocityconf

Re: [PATCH] initialize fb_info->lock mutex in the framebuffer_alloc()

[krzysztof.h1]

Geert Uytterhoeven napisa³(a):
> On Sun, May 3, 2009 at 23:30, Krzysztof Helt >krzysztof.h1@poczta.fm>
> wrote:
> > From: Krzysztof Helt >krzysztof.h1@wp.pl>
> >
> > The fb_info->lock mutex should be initialized inside
> > the framebuffer_alloc() function. Otherwise, someone
> > may call a fb layer function which indirectly uses uninitialized
> > mutex.
> > Move initialization of the mutex from the register_framebuffer()
> > to framebuffer_alloc().
> >
> > This may cause problems for drivers which do not use
> > the framebuffer_alloc() to allocate the fb_info structure.
> > Convert the two last drivers to use the framebuffer_alloc().
> 
> > diff -urp linux-orig/drivers/video/igafb.c
> linux-2.6.30/drivers/video/igafb.c
> > diff -urp linux-orig/drivers/video/offb.c
> linux-2.6.30/drivers/video/offb.c
> 
> Are these really the only 2 remaining ones? What about amifb and atafb
> (from the top of my head)?
> 

They use static global structures like this:

static struct fb_info fb_info = {
    .fix = {
        .id = "Atari ",
        .visual = FB_VISUAL_PSEUDOCOLOR,
        .accel  = FB_ACCEL_NONE,
    }
};

The fb_info is not allocated.

Still, there is a chance I have missed a driver. I will recheck each driver.

Regards,
Krzysztof

----------------------------------------------------------------------
Wygraj wycieczke do Eurodisneylandu!
Sprawdz >> http://link.interia.pl/f2153


------------------------------------------------------------------------------
Register Now & Save for Velocity, the Web Performance & Operations 
Conference from O'Reilly Media. Velocity features a full day of 
expert-led, hands-on workshops and two days of sessions from industry 
leaders in dedicated Performance & Operations tracks. Use code vel09scf 
and Save an extra 15% before 5/3. http://p.sf.net/sfu/velocityconf

Re: [PATCH] initialize fb_info->lock mutex in the framebuffer_alloc()

[Geert Uytterhoeven]

2009/5/4  <krzysztof.h1@poczta.fm>:
> Geert Uytterhoeven napisał(a):
>> On Sun, May 3, 2009 at 23:30, Krzysztof Helt >krzysztof.h1@poczta.fm>
>> wrote:
>> > From: Krzysztof Helt >krzysztof.h1@wp.pl>
>> >
>> > The fb_info->lock mutex should be initialized inside
>> > the framebuffer_alloc() function. Otherwise, someone
>> > may call a fb layer function which indirectly uses uninitialized
>> > mutex.
>> > Move initialization of the mutex from the register_framebuffer()
>> > to framebuffer_alloc().
>> >
>> > This may cause problems for drivers which do not use
>> > the framebuffer_alloc() to allocate the fb_info structure.
>> > Convert the two last drivers to use the framebuffer_alloc().
>>
>> > diff -urp linux-orig/drivers/video/igafb.c
>> linux-2.6.30/drivers/video/igafb.c
>> > diff -urp linux-orig/drivers/video/offb.c
>> linux-2.6.30/drivers/video/offb.c
>>
>> Are these really the only 2 remaining ones? What about amifb and atafb
>> (from the top of my head)?
>>
>
> They use static global structures like this:
>
> static struct fb_info fb_info = {
>    .fix = {
>        .id = "Atari ",
>        .visual = FB_VISUAL_PSEUDOCOLOR,
>        .accel  = FB_ACCEL_NONE,
>    }
> };
>
> The fb_info is not allocated.

OK. Anyway, they're on my list to be converted, as at least amifb
already has a platform_device
in my private tree.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

------------------------------------------------------------------------------
Register Now & Save for Velocity, the Web Performance & Operations 
Conference from O'Reilly Media. Velocity features a full day of 
expert-led, hands-on workshops and two days of sessions from industry 
leaders in dedicated Performance & Operations tracks. Use code vel09scf 
and Save an extra 15% before 5/3. http://p.sf.net/sfu/velocityconf
_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

Re: [PATCH] initialize fb_info->lock mutex in the framebuffer_alloc()

[krzysztof.h1]

Ville Syrjälä napisa³(a):
> On Sun, May 03, 2009 at 11:30:08PM +0200, Krzysztof Helt wrote:
> > From: Krzysztof Helt >krzysztof.h1@wp.pl>
> > 
> > The fb_info->lock mutex should be initialized inside 
> > the framebuffer_alloc() function. Otherwise, someone
> > may call a fb layer function which indirectly uses uninitialized 
> > mutex.
> 
> How can that someone find the fb_info without it being in the
> registered_fb[] array?
> 

It cannot but one can call functions modyfing fb_info structure.

An example is the fb_check_var() function which is called
by most drivers indirectly by the fb_try_mode().

The question to answer is what is a purpose of the
fb_info->lock. What is there assumed to be protected?

Currenlty, it seems randomly set all around the layer.
Is supposed to protect against concurent access
from the userspace or something more?

Regards,
Krzysztof


----------------------------------------------------------------------
Gotowka na koncie. Otworz konto direct i wez podwojny limit. 	 
http://link.interia.pl/f2145


------------------------------------------------------------------------------
Register Now & Save for Velocity, the Web Performance & Operations 
Conference from O'Reilly Media. Velocity features a full day of 
expert-led, hands-on workshops and two days of sessions from industry 
leaders in dedicated Performance & Operations tracks. Use code vel09scf 
and Save an extra 15% before 5/3. http://p.sf.net/sfu/velocityconf

Re: [PATCH] initialize fb_info->lock mutex in the framebuffer_alloc()

[krzysztof.h1]

Krzysztof Helt napisa³(a):
> From: Krzysztof Helt >krzysztof.h1@wp.pl>
> 
> The fb_info->lock mutex should be initialized inside 
> the framebuffer_alloc() function. Otherwise, someone
> may call a fb layer function which indirectly uses uninitialized 
> mutex.
> Move initialization of the mutex from the register_framebuffer()
> to framebuffer_alloc().
> 
> This may cause problems for drivers which do not use
> the framebuffer_alloc() to allocate the fb_info structure.
> Convert the two last drivers to use the framebuffer_alloc().
> 
> Signed-off-by: Krzysztof Helt >krzysztof.h1@wp.pl>
> ---
> 

Please drop this patch as it breaks framebuffers with
static global fb_info structures. The fb_info->lock
will be uninitialized. Thanks to Geert for helping
me realizing that.

I will repost driver conversion to the framebuffer_alloc()
without moving mutex initialization.

Regards,
Krzysztof


----------------------------------------------------------------------
Gotowka na koncie. Otworz konto direct i wez podwojny limit. 	 
http://link.interia.pl/f2145


------------------------------------------------------------------------------
Register Now & Save for Velocity, the Web Performance & Operations 
Conference from O'Reilly Media. Velocity features a full day of 
expert-led, hands-on workshops and two days of sessions from industry 
leaders in dedicated Performance & Operations tracks. Use code vel09scf 
and Save an extra 15% before 5/3. http://p.sf.net/sfu/velocityconf

Re: [PATCH] initialize fb_info->lock mutex in the framebuffer_alloc()

[krzysztof.h1]

Geert Uytterhoeven napisa³(a):
> On Sun, May 3, 2009 at 23:30, Krzysztof Helt >krzysztof.h1@poczta.fm>
> wrote:
> > From: Krzysztof Helt >krzysztof.h1@wp.pl>
> >
> > The fb_info->lock mutex should be initialized inside
> > the framebuffer_alloc() function. Otherwise, someone
> > may call a fb layer function which indirectly uses uninitialized
> > mutex.
> > Move initialization of the mutex from the register_framebuffer()
> > to framebuffer_alloc().
> >
> > This may cause problems for drivers which do not use
> > the framebuffer_alloc() to allocate the fb_info structure.
> > Convert the two last drivers to use the framebuffer_alloc().
> 
> > diff -urp linux-orig/drivers/video/igafb.c
> linux-2.6.30/drivers/video/igafb.c
> > diff -urp linux-orig/drivers/video/offb.c
> linux-2.6.30/drivers/video/offb.c
> 
> Are these really the only 2 remaining ones? What about amifb and atafb
> (from the top of my head)?
> 

I have checked the latest 2.6.30-rc4 tree and there are more drivers
which do not use the framebuffer_alloc(). There are four groups:

1. The fb_info structure is declared as a static variable (9 drivers).  

2. The fb_info structure is part of another structure declared as
the static variable (2 drivers)

3. The fb_info structure is allocated using kzalloc or kmalloc (3 drivers).

4. The fb_info structure is part of a bigger structure which is
allocated using kzalloc or kmalloc (12 drivers).

The third group is an obvious candidate to use the framebuffer_alloc() 
and I will send patches for them (these are two drivers I already converted).

The fourth group should be converted to use framebuffer_alloc either by
putting fb_info pointer in the bigger structure or
using fb_info->par to point to the bigger structure.

The groups 1 and 2 are probably not worth converting. It is
ok to have a static fb_info if there is no physical possibility
to have two the same framebuffers inside one machine, e.g.
probably only one on-board amifb framebuffer is possible 
in the Amiga computer.

The 1 and 2 groups require a function fb_info_init() to
initialize non-zero fields in the framebuffer (e.g. mutexes).
The function can be used for the 4th group as well (at least
as a temporary solution).

Regards,
Krzysztof


----------------------------------------------------------------------
Szukasz pracy? Sprawd¼ nasze oferty!
http://link.interia.pl/f2142


------------------------------------------------------------------------------
Register Now & Save for Velocity, the Web Performance & Operations 
Conference from O'Reilly Media. Velocity features a full day of 
expert-led, hands-on workshops and two days of sessions from industry 
leaders in dedicated Performance & Operations tracks. Use code vel09scf 
and Save an extra 15% before 5/3. http://p.sf.net/sfu/velocityconf